Skip to content

Commit 027014d

Browse files
wxiaoguangwxiaoguang
authored
Fix webauthn regression and improve code (#25113)
Follow: * #22697 There are some bugs in #22697: * #22697 (comment) * the webauthn failure message is never shown and causes console error * The `document.getElementById('register-button')` and `document.getElementById('login-button')` is wrong * there is no such element in code * it causes JS error when a browser doesn't provide webauthn * the end user can't see the real error message These bugs are fixed in this PR. Other changes: * Use simple HTML/CSS layouts, no need to use too many `gt-` patches * Make the webauthn page have correct "page-content" layout * The "data-webauthn-error-msg" elements are only used to provide locale texts, so move them into a single "gt-hidden", then no need to repeat a lot of "gt-hidden" in code * The `{{.CsrfTokenHtml}}` is a no-op because there is no form * Many `hideElem('#webauthn-error')` in code is no-op because the `webauthn-error` already has "gt-hidden" by default * Make the tests for "URLEncodedBase64" really test with concrete cases. Screenshots: * Error message when webauthn fails (before, there is no error message): <details> ![image](https://github.com/go-gitea/gitea/assets/2114189/93cf9559-d93b-4f06-9d98-0f7032d9c65b) </details> * Error message when webauthn is unavailable <details> ![image](https://github.com/go-gitea/gitea/assets/2114189/ffc0fcd9-b93b-4418-979c-c89bb627aaf2) </details>
1 parent 5853609 commit 027014d

File tree

6 files changed

+57
-66
lines changed

6 files changed

+57
-66
lines changed

‎templates/user/auth/webauthn.tmpl

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,22 @@
11
{{template "base/head" .}}
2-
<div class="user signin webauthn-prompt">
3-
<div class="ui middle centered very relaxed page grid">
2+
<div role="main" aria-label="{{.Title}}" class="page-content user signin webauthn-prompt">
3+
<div class="ui page grid">
44
<div class="column center aligned">
5-
<h3 class="ui top attached header">
6-
{{.locale.Tr "twofa"}}
7-
</h3>
8-
{{template "user/auth/webauthn_error" .}}
9-
<div class="ui attached segment">
10-
{{svg "octicon-key" 56}}
11-
<h3>{{.locale.Tr "webauthn_insert_key"}}</h3>
12-
{{template "base/alert" .}}
13-
<p>{{.locale.Tr "webauthn_sign_in"}}</p>
14-
</div>
15-
<div class="ui attached segment"><div class="ui active indeterminate inline loader"></div> {{.locale.Tr "webauthn_press_button"}} </div>
16-
<div class="ui attached segment">
17-
<a href="{{AppSubUrl}}/user/two_factor">{{.locale.Tr "webauthn_use_twofa"}}</a>
18-
</div>
5+
{{template "user/auth/webauthn_error" .}}
6+
<h3 class="ui top attached header">{{.locale.Tr "twofa"}}</h3>
7+
<div class="ui attached segment">
8+
{{svg "octicon-key" 56}}
9+
<h3>{{.locale.Tr "webauthn_insert_key"}}</h3>
10+
{{template "base/alert" .}}
11+
<p>{{.locale.Tr "webauthn_sign_in"}}</p>
12+
</div>
13+
<div class="ui attached segment">
14+
<div class="ui active indeterminate inline loader"></div>
15+
{{.locale.Tr "webauthn_press_button"}}
16+
</div>
17+
<div class="ui attached segment">
18+
<a href="{{AppSubUrl}}/user/two_factor">{{.locale.Tr "webauthn_use_twofa"}}</a>
19+
</div>
1920
</div>
2021
</div>
2122
</div>

‎templates/user/auth/webauthn_error.tmpl

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
1-
<div id="webauthn-error" class="ui small gt-hidden">
2-
<div class="content ui negative message gt-df gt-fc gt-gap-3">
3-
<div class="header">{{.locale.Tr "webauthn_error"}}</div>
4-
<div id="webauthn-error-msg"></div>
5-
<div class="gt-hidden" data-webauthn-error-msg="browser">{{.locale.Tr "webauthn_unsupported_browser"}}</div>
6-
<div class="gt-hidden" data-webauthn-error-msg="unknown">{{.locale.Tr "webauthn_error_unknown"}}</div>
7-
<div class="gt-hidden" data-webauthn-error-msg="insecure">{{.locale.Tr "webauthn_error_insecure"}}</div>
8-
<div class="gt-hidden" data-webauthn-error-msg="unable-to-process">{{.locale.Tr "webauthn_error_unable_to_process"}}</div>
9-
<div class="gt-hidden" data-webauthn-error-msg="duplicated">{{.locale.Tr "webauthn_error_duplicated"}}</div>
10-
<div class="gt-hidden" data-webauthn-error-msg="empty">{{.locale.Tr "webauthn_error_empty"}}</div>
11-
<div class="gt-hidden" data-webauthn-error-msg="timeout">{{.locale.Tr "webauthn_error_timeout"}}</div>
1+
<div id="webauthn-error" class="ui negative message gt-hidden">
2+
<div class="header">{{.locale.Tr "webauthn_error"}}</div>
3+
<div id="webauthn-error-msg" class="gt-pt-3"></div>
4+
<div class="gt-hidden">
5+
<div data-webauthn-error-msg="browser">{{.locale.Tr "webauthn_unsupported_browser"}}</div>
6+
<div data-webauthn-error-msg="unknown">{{.locale.Tr "webauthn_error_unknown"}}</div>
7+
<div data-webauthn-error-msg="insecure">{{.locale.Tr "webauthn_error_insecure"}}</div>
8+
<div data-webauthn-error-msg="unable-to-process">{{.locale.Tr "webauthn_error_unable_to_process"}}</div>
9+
<div data-webauthn-error-msg="duplicated">{{.locale.Tr "webauthn_error_duplicated"}}</div>
10+
<div data-webauthn-error-msg="empty">{{.locale.Tr "webauthn_error_empty"}}</div>
11+
<div data-webauthn-error-msg="timeout">{{.locale.Tr "webauthn_error_timeout"}}</div>
1212
</div>
1313
</div>

‎templates/user/settings/security/webauthn.tmpl

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,4 @@
1-
<h4 class="ui top attached header">
2-
{{.locale.Tr "settings.webauthn"}}
3-
</h4>
1+
<h4 class="ui top attached header">{{.locale.Tr "settings.webauthn"}}</h4>
42
<div class="ui attached segment">
53
<p>{{.locale.Tr "settings.webauthn_desc" | Str2html}}</p>
64
{{template "user/auth/webauthn_error" .}}
@@ -20,7 +18,6 @@
2018
{{end}}
2119
</div>
2220
<div class="ui form">
23-
{{.CsrfTokenHtml}}
2421
<div class="required field">
2522
<label for="nickname">{{.locale.Tr "settings.webauthn_nickname"}}</label>
2623
<input id="nickname" name="nickname" type="text" required>
@@ -29,7 +26,6 @@
2926
</div>
3027
</div>
3128

32-
3329
<div class="ui g-modal-confirm delete modal" id="delete-registration">
3430
<div class="header">
3531
{{svg "octicon-trash"}}

‎web_src/css/form.css

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -394,10 +394,6 @@ textarea:focus,
394394
margin: 0;
395395
}
396396

397-
.user.signin.webauthn-prompt {
398-
margin-top: 15px;
399-
}
400-
401397
.repository.new.repo form,
402398
.repository.new.migrate form,
403399
.repository.new.fork form {

‎web_src/js/features/user-auth-webauthn.js

Lines changed: 16 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
11
import {encodeURLEncodedBase64, decodeURLEncodedBase64} from '../utils.js';
2-
import {showElem, hideElem} from '../utils/dom.js';
2+
import {showElem} from '../utils/dom.js';
33

44
const {appSubUrl, csrfToken} = window.config;
55

66
export async function initUserAuthWebAuthn() {
7-
hideElem('#webauthn-error');
8-
97
const elPrompt = document.querySelector('.user.signin.webauthn-prompt');
108
if (!elPrompt) {
119
return;
@@ -25,21 +23,21 @@ export async function initUserAuthWebAuthn() {
2523
for (const cred of options.publicKey.allowCredentials) {
2624
cred.id = decodeURLEncodedBase64(cred.id);
2725
}
28-
const credential = await navigator.credentials.get({
29-
publicKey: options.publicKey
30-
});
3126
try {
27+
const credential = await navigator.credentials.get({
28+
publicKey: options.publicKey
29+
});
3230
await verifyAssertion(credential);
3331
} catch (err) {
3432
if (!options.publicKey.extensions?.appid) {
3533
webAuthnError('general', err.message);
3634
return;
3735
}
3836
delete options.publicKey.extensions.appid;
39-
const credential = await navigator.credentials.get({
40-
publicKey: options.publicKey
41-
});
4237
try {
38+
const credential = await navigator.credentials.get({
39+
publicKey: options.publicKey
40+
});
4341
await verifyAssertion(credential);
4442
} catch (err) {
4543
webAuthnError('general', err.message);
@@ -48,7 +46,7 @@ export async function initUserAuthWebAuthn() {
4846
}
4947

5048
async function verifyAssertion(assertedCredential) {
51-
// Move data into Arrays incase it is super long
49+
// Move data into Arrays in case it is super long
5250
const authData = new Uint8Array(assertedCredential.response.authenticatorData);
5351
const clientDataJSON = new Uint8Array(assertedCredential.response.clientDataJSON);
5452
const rawId = new Uint8Array(assertedCredential.rawId);
@@ -137,15 +135,11 @@ function webAuthnError(errorType, message) {
137135

138136
function detectWebAuthnSupport() {
139137
if (!window.isSecureContext) {
140-
document.getElementById('register-button').disabled = true;
141-
document.getElementById('login-button').disabled = true;
142138
webAuthnError('insecure');
143139
return false;
144140
}
145141

146142
if (typeof window.PublicKeyCredential !== 'function') {
147-
document.getElementById('register-button').disabled = true;
148-
document.getElementById('login-button').disabled = true;
149143
webAuthnError('browser');
150144
return false;
151145
}
@@ -158,15 +152,13 @@ export function initUserAuthWebAuthnRegister() {
158152
if (!elRegister) {
159153
return;
160154
}
161-
162-
hideElem('#webauthn-error');
163-
164-
elRegister.addEventListener('click', (e) => {
155+
if (!detectWebAuthnSupport()) {
156+
elRegister.disabled = true;
157+
return;
158+
}
159+
elRegister.addEventListener('click', async (e) => {
165160
e.preventDefault();
166-
if (!detectWebAuthnSupport()) {
167-
return;
168-
}
169-
webAuthnRegisterRequest();
161+
await webAuthnRegisterRequest();
170162
});
171163
}
172164

@@ -203,15 +195,12 @@ async function webAuthnRegisterRequest() {
203195
}
204196
}
205197

206-
let credential;
207198
try {
208-
credential = await navigator.credentials.create({
199+
const credential = await navigator.credentials.create({
209200
publicKey: options.publicKey
210201
});
202+
await webauthnRegistered(credential);
211203
} catch (err) {
212204
webAuthnError('unknown', err);
213-
return;
214205
}
215-
216-
webauthnRegistered(credential);
217206
}

‎web_src/js/utils.test.js

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,17 @@ test('toAbsoluteUrl', () => {
133133
expect(() => toAbsoluteUrl('path')).toThrowError('unsupported');
134134
});
135135

136+
const uint8array = (s) => new TextEncoder().encode(s);
136137
test('encodeURLEncodedBase64, decodeURLEncodedBase64', () => {
137-
expect(encodeURLEncodedBase64(decodeURLEncodedBase64('foo'))).toEqual('foo'); // No = padding
138-
expect(encodeURLEncodedBase64(decodeURLEncodedBase64('a-minus'))).toEqual('a-minus');
139-
expect(encodeURLEncodedBase64(decodeURLEncodedBase64('_underscorc'))).toEqual('_underscorc');
138+
expect(encodeURLEncodedBase64(uint8array('AA?'))).toEqual('QUE_'); // standard base64: "QUE/"
139+
expect(encodeURLEncodedBase64(uint8array('AA~'))).toEqual('QUF-'); // standard base64: "QUF+"
140+
141+
expect(decodeURLEncodedBase64('QUE/')).toEqual(uint8array('AA?'));
142+
expect(decodeURLEncodedBase64('QUF+')).toEqual(uint8array('AA~'));
143+
expect(decodeURLEncodedBase64('QUE_')).toEqual(uint8array('AA?'));
144+
expect(decodeURLEncodedBase64('QUF-')).toEqual(uint8array('AA~'));
145+
146+
expect(encodeURLEncodedBase64(uint8array('a'))).toEqual('YQ'); // standard base64: "YQ=="
147+
expect(decodeURLEncodedBase64('YQ')).toEqual(uint8array('a'));
148+
expect(decodeURLEncodedBase64('YQ==')).toEqual(uint8array('a'));
140149
});

0 commit comments

Comments
 (0)