From 027014d7de19a566cde306c868a31895903d00d5 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 7 Jun 2023 19:20:18 +0800 Subject: [PATCH] Fix webauthn regression and improve code (#25113) Follow: * #22697 There are some bugs in #22697: * https://github.com/go-gitea/gitea/pull/22697#issuecomment-1577957966 * 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):
![image](https://github.com/go-gitea/gitea/assets/2114189/93cf9559-d93b-4f06-9d98-0f7032d9c65b)
* Error message when webauthn is unavailable
![image](https://github.com/go-gitea/gitea/assets/2114189/ffc0fcd9-b93b-4418-979c-c89bb627aaf2)
--- templates/user/auth/webauthn.tmpl | 33 +++++++------- templates/user/auth/webauthn_error.tmpl | 22 +++++----- .../user/settings/security/webauthn.tmpl | 6 +-- web_src/css/form.css | 4 -- web_src/js/features/user-auth-webauthn.js | 43 +++++++------------ web_src/js/utils.test.js | 15 +++++-- 6 files changed, 57 insertions(+), 66 deletions(-) diff --git a/templates/user/auth/webauthn.tmpl b/templates/user/auth/webauthn.tmpl index f1c4f29fd9..ef04886405 100644 --- a/templates/user/auth/webauthn.tmpl +++ b/templates/user/auth/webauthn.tmpl @@ -1,21 +1,22 @@ {{template "base/head" .}} -
-
+
+
-

- {{.locale.Tr "twofa"}} -

- {{template "user/auth/webauthn_error" .}} -
- {{svg "octicon-key" 56}} -

{{.locale.Tr "webauthn_insert_key"}}

- {{template "base/alert" .}} -

{{.locale.Tr "webauthn_sign_in"}}

-
-
{{.locale.Tr "webauthn_press_button"}}
-
- {{.locale.Tr "webauthn_use_twofa"}} -
+ {{template "user/auth/webauthn_error" .}} +

{{.locale.Tr "twofa"}}

+
+ {{svg "octicon-key" 56}} +

{{.locale.Tr "webauthn_insert_key"}}

+ {{template "base/alert" .}} +

{{.locale.Tr "webauthn_sign_in"}}

+
+
+
+ {{.locale.Tr "webauthn_press_button"}} +
+
+ {{.locale.Tr "webauthn_use_twofa"}} +
diff --git a/templates/user/auth/webauthn_error.tmpl b/templates/user/auth/webauthn_error.tmpl index f90882ef12..59029bbf91 100644 --- a/templates/user/auth/webauthn_error.tmpl +++ b/templates/user/auth/webauthn_error.tmpl @@ -1,13 +1,13 @@ -
-
-
{{.locale.Tr "webauthn_error"}}
-
-
{{.locale.Tr "webauthn_unsupported_browser"}}
-
{{.locale.Tr "webauthn_error_unknown"}}
-
{{.locale.Tr "webauthn_error_insecure"}}
-
{{.locale.Tr "webauthn_error_unable_to_process"}}
-
{{.locale.Tr "webauthn_error_duplicated"}}
-
{{.locale.Tr "webauthn_error_empty"}}
-
{{.locale.Tr "webauthn_error_timeout"}}
+
+
{{.locale.Tr "webauthn_error"}}
+
+
+
{{.locale.Tr "webauthn_unsupported_browser"}}
+
{{.locale.Tr "webauthn_error_unknown"}}
+
{{.locale.Tr "webauthn_error_insecure"}}
+
{{.locale.Tr "webauthn_error_unable_to_process"}}
+
{{.locale.Tr "webauthn_error_duplicated"}}
+
{{.locale.Tr "webauthn_error_empty"}}
+
{{.locale.Tr "webauthn_error_timeout"}}
diff --git a/templates/user/settings/security/webauthn.tmpl b/templates/user/settings/security/webauthn.tmpl index e541f764bc..ec1df7cdcd 100644 --- a/templates/user/settings/security/webauthn.tmpl +++ b/templates/user/settings/security/webauthn.tmpl @@ -1,6 +1,4 @@ -

-{{.locale.Tr "settings.webauthn"}} -

+

{{.locale.Tr "settings.webauthn"}}

{{.locale.Tr "settings.webauthn_desc" | Str2html}}

{{template "user/auth/webauthn_error" .}} @@ -20,7 +18,6 @@ {{end}}
- {{.CsrfTokenHtml}}
@@ -29,7 +26,6 @@
-
{{svg "octicon-trash"}} diff --git a/web_src/css/form.css b/web_src/css/form.css index 9e1a72380b..dfa7208ca2 100644 --- a/web_src/css/form.css +++ b/web_src/css/form.css @@ -394,10 +394,6 @@ textarea:focus, margin: 0; } -.user.signin.webauthn-prompt { - margin-top: 15px; -} - .repository.new.repo form, .repository.new.migrate form, .repository.new.fork form { diff --git a/web_src/js/features/user-auth-webauthn.js b/web_src/js/features/user-auth-webauthn.js index 8085313d22..c4c2356cb3 100644 --- a/web_src/js/features/user-auth-webauthn.js +++ b/web_src/js/features/user-auth-webauthn.js @@ -1,11 +1,9 @@ import {encodeURLEncodedBase64, decodeURLEncodedBase64} from '../utils.js'; -import {showElem, hideElem} from '../utils/dom.js'; +import {showElem} from '../utils/dom.js'; const {appSubUrl, csrfToken} = window.config; export async function initUserAuthWebAuthn() { - hideElem('#webauthn-error'); - const elPrompt = document.querySelector('.user.signin.webauthn-prompt'); if (!elPrompt) { return; @@ -25,10 +23,10 @@ export async function initUserAuthWebAuthn() { for (const cred of options.publicKey.allowCredentials) { cred.id = decodeURLEncodedBase64(cred.id); } - const credential = await navigator.credentials.get({ - publicKey: options.publicKey - }); try { + const credential = await navigator.credentials.get({ + publicKey: options.publicKey + }); await verifyAssertion(credential); } catch (err) { if (!options.publicKey.extensions?.appid) { @@ -36,10 +34,10 @@ export async function initUserAuthWebAuthn() { return; } delete options.publicKey.extensions.appid; - const credential = await navigator.credentials.get({ - publicKey: options.publicKey - }); try { + const credential = await navigator.credentials.get({ + publicKey: options.publicKey + }); await verifyAssertion(credential); } catch (err) { webAuthnError('general', err.message); @@ -48,7 +46,7 @@ export async function initUserAuthWebAuthn() { } async function verifyAssertion(assertedCredential) { - // Move data into Arrays incase it is super long + // Move data into Arrays in case it is super long const authData = new Uint8Array(assertedCredential.response.authenticatorData); const clientDataJSON = new Uint8Array(assertedCredential.response.clientDataJSON); const rawId = new Uint8Array(assertedCredential.rawId); @@ -137,15 +135,11 @@ function webAuthnError(errorType, message) { function detectWebAuthnSupport() { if (!window.isSecureContext) { - document.getElementById('register-button').disabled = true; - document.getElementById('login-button').disabled = true; webAuthnError('insecure'); return false; } if (typeof window.PublicKeyCredential !== 'function') { - document.getElementById('register-button').disabled = true; - document.getElementById('login-button').disabled = true; webAuthnError('browser'); return false; } @@ -158,15 +152,13 @@ export function initUserAuthWebAuthnRegister() { if (!elRegister) { return; } - - hideElem('#webauthn-error'); - - elRegister.addEventListener('click', (e) => { + if (!detectWebAuthnSupport()) { + elRegister.disabled = true; + return; + } + elRegister.addEventListener('click', async (e) => { e.preventDefault(); - if (!detectWebAuthnSupport()) { - return; - } - webAuthnRegisterRequest(); + await webAuthnRegisterRequest(); }); } @@ -203,15 +195,12 @@ async function webAuthnRegisterRequest() { } } - let credential; try { - credential = await navigator.credentials.create({ + const credential = await navigator.credentials.create({ publicKey: options.publicKey }); + await webauthnRegistered(credential); } catch (err) { webAuthnError('unknown', err); - return; } - - webauthnRegistered(credential); } diff --git a/web_src/js/utils.test.js b/web_src/js/utils.test.js index cf73b63b99..812ef3bedf 100644 --- a/web_src/js/utils.test.js +++ b/web_src/js/utils.test.js @@ -133,8 +133,17 @@ test('toAbsoluteUrl', () => { expect(() => toAbsoluteUrl('path')).toThrowError('unsupported'); }); +const uint8array = (s) => new TextEncoder().encode(s); test('encodeURLEncodedBase64, decodeURLEncodedBase64', () => { - expect(encodeURLEncodedBase64(decodeURLEncodedBase64('foo'))).toEqual('foo'); // No = padding - expect(encodeURLEncodedBase64(decodeURLEncodedBase64('a-minus'))).toEqual('a-minus'); - expect(encodeURLEncodedBase64(decodeURLEncodedBase64('_underscorc'))).toEqual('_underscorc'); + expect(encodeURLEncodedBase64(uint8array('AA?'))).toEqual('QUE_'); // standard base64: "QUE/" + expect(encodeURLEncodedBase64(uint8array('AA~'))).toEqual('QUF-'); // standard base64: "QUF+" + + expect(decodeURLEncodedBase64('QUE/')).toEqual(uint8array('AA?')); + expect(decodeURLEncodedBase64('QUF+')).toEqual(uint8array('AA~')); + expect(decodeURLEncodedBase64('QUE_')).toEqual(uint8array('AA?')); + expect(decodeURLEncodedBase64('QUF-')).toEqual(uint8array('AA~')); + + expect(encodeURLEncodedBase64(uint8array('a'))).toEqual('YQ'); // standard base64: "YQ==" + expect(decodeURLEncodedBase64('YQ')).toEqual(uint8array('a')); + expect(decodeURLEncodedBase64('YQ==')).toEqual(uint8array('a')); });