Login with security keys no longer possible #10124

@zeripath commented on GitHub (Jan 18, 2023):

Are there any server logs associated?

@zeripath commented on GitHub (Jan 18, 2023): Are there any server logs associated?

GiteaMirror commented

@wxiaoguang commented on GitHub (Jan 19, 2023):

I can confirm the same bug on my instance. It could be either a server-side bug or a client-side (JS) bug.

2023/01/19 12:03:35 [63c8c116] router: completed POST /user/login for CLIENT_IP:0, 303 See Other in 1239.5ms @ auth/auth.go:173(auth.SignInPost)
2023/01/19 12:03:35 [63c8c117] router: completed GET /user/webauthn for CLIENT_IP:0, 200 OK in 3.2ms @ auth/webauthn.go:26(auth.WebAuthn)
2023/01/19 12:03:36 [63c8c118-2] router: completed GET /user/webauthn/assertion for CLIENT_IP:0, 200 OK in 5.1ms @ auth/webauthn.go:44(auth.WebAuthnLoginAssertion)
2023/01/19 12:03:48 ...web/auth/webauthn.go:116:WebAuthnLoginAssertionPost() [I] [63c8c124] Failed authentication attempt for wangxiaoguang from CLIENT_IP:0: Error validating origin
2023/01/19 12:03:48 [63c8c124] router: completed POST /user/webauthn/assertion for CLIENT_IP:0, 403 Forbidden in 8.0ms @ auth/webauthn.go:82(auth.WebAuthnLoginAssertionPost)

@wxiaoguang commented on GitHub (Jan 19, 2023): I can confirm the same bug on my instance. It could be either a server-side bug or a client-side (JS) bug. ``` 2023/01/19 12:03:35 [63c8c116] router: completed POST /user/login for CLIENT_IP:0, 303 See Other in 1239.5ms @ auth/auth.go:173(auth.SignInPost) 2023/01/19 12:03:35 [63c8c117] router: completed GET /user/webauthn for CLIENT_IP:0, 200 OK in 3.2ms @ auth/webauthn.go:26(auth.WebAuthn) 2023/01/19 12:03:36 [63c8c118-2] router: completed GET /user/webauthn/assertion for CLIENT_IP:0, 200 OK in 5.1ms @ auth/webauthn.go:44(auth.WebAuthnLoginAssertion) 2023/01/19 12:03:48 ...web/auth/webauthn.go:116:WebAuthnLoginAssertionPost() [I] [63c8c124] Failed authentication attempt for wangxiaoguang from CLIENT_IP:0: Error validating origin 2023/01/19 12:03:48 [63c8c124] router: completed POST /user/webauthn/assertion for CLIENT_IP:0, 403 Forbidden in 8.0ms @ auth/webauthn.go:82(auth.WebAuthnLoginAssertionPost) ```

GiteaMirror commented

@lunny commented on GitHub (Jan 19, 2023):

Maybe related #22400

@lunny commented on GitHub (Jan 19, 2023): Maybe related #22400

GiteaMirror commented

@silverwind commented on GitHub (Jan 19, 2023):

There is only one atob case in our JS and it comes from ff5c87dd10/index.js (L20).

As for the reason, I can only guess. I know that atob and btoa are limited to ASCII characters when encoding/decoding, e.g. characters outside the ASCII set (UTF8) would fail.

BTW shouldn't this error be caught using window.onerror and display on the page?

@silverwind commented on GitHub (Jan 19, 2023): There is only one `atob` case in our JS and it comes from https://github.com/WebReflection/uint8-to-base64/blob/ff5c87dd100dbf72d2ed9edf4b81a4ce23f1a8c1/index.js#L20. As for the reason, I can only guess. I know that `atob` and `btoa` are limited to ASCII characters when encoding/decoding, e.g. characters outside the ASCII set (UTF8) would fail. BTW shouldn't this error be caught using `window.onerror` and display on the page?

GiteaMirror commented

@wxiaoguang commented on GitHub (Jan 19, 2023):

Invalid base64 chars all cause atob fail:

> window.atob('_')
VM110:1 Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded.
    at <anonymous>:1:8

The response of /user/webauthn/assertion is something like that on my side:

{"publicKey":{"challenge":"-xxxxxx-xxxxxxxxxxxx","rpId":"xxxxxx.com","allowCredentials":
[{"type":"public-key","id":"xxxxxxxxxxxxxxxx="},{"type":"public-key","id":"xxxxxxxxxxx="},{"type":"public-key","id":"xxxxxxxx="}],
"userVerification":"discouraged"}}

@wxiaoguang commented on GitHub (Jan 19, 2023): Invalid base64 chars all cause `atob` fail: ``` > window.atob('_') VM110:1 Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded. at <anonymous>:1:8 ``` The response of `/user/webauthn/assertion` is something like that on my side: ``` {"publicKey":{"challenge":"-xxxxxx-xxxxxxxxxxxx","rpId":"xxxxxx.com","allowCredentials": [{"type":"public-key","id":"xxxxxxxxxxxxxxxx="},{"type":"public-key","id":"xxxxxxxxxxx="},{"type":"public-key","id":"xxxxxxxx="}], "userVerification":"discouraged"}} ```

GiteaMirror commented

@wxiaoguang commented on GitHub (Jan 19, 2023):

I guess the problem is caused by inconsitent base64 encoding standard.

Standard base64: '+', '/' and '='
URL base64: '-', '_' and no '='

Then the atob in deocde tries to decode -_ base64 chars, then error occurs.

(ps: just my guess, I am not using the webauthn now, so feel free to continue)

@wxiaoguang commented on GitHub (Jan 19, 2023): I guess the problem is caused by inconsitent base64 encoding standard. * Standard base64: '+', '/' and '=' * URL base64: '-', '_' and no '=' Then the `atob` in `deocde` tries to decode `-_` base64 chars, then error occurs. (ps: just my guess, I am not using the webauthn now, so feel free to continue) ![image](https://user-images.githubusercontent.com/2114189/213372329-4cc8e91f-1e8b-40be-a9c7-9cb99a8b2e64.png) ![image](https://user-images.githubusercontent.com/2114189/213371787-5902527c-2641-43eb-ba12-9c0270a46242.png)

GiteaMirror commented

@wxiaoguang commented on GitHub (Jan 19, 2023):

And you see, I have questioned before:

https://github.com/go-gitea/gitea/pull/17957#pullrequestreview-832673109

"Unknown problems" always cause more problems. That's why I always insist to make things consistent and clear. But I doubt seldom people agree with me.

@wxiaoguang commented on GitHub (Jan 19, 2023): And you see, I have questioned before: * https://github.com/go-gitea/gitea/pull/17957#pullrequestreview-832673109 <details> ![image](https://user-images.githubusercontent.com/2114189/213373050-6d0ab15d-6614-4833-aca6-26c1648e3a19.png) </details> "Unknown problems" always cause more problems. That's why I always insist to make things consistent and clear. But I doubt seldom people agree with me.

GiteaMirror commented

@silverwind commented on GitHub (Jan 19, 2023):

There is https://www.npmjs.com/package/base64url, I guess one solution would be to incorporate https://www.npmjs.com/package/uint8-to-base64 in our code and use that module instead to encode/decode the base64.

@silverwind commented on GitHub (Jan 19, 2023): There is https://www.npmjs.com/package/base64url, I guess one solution would be to incorporate https://www.npmjs.com/package/uint8-to-base64 in our code and use that module instead to encode/decode the base64.

GiteaMirror commented

@zeripath commented on GitHub (Jan 29, 2023):

The problem is not to do with mis-encoding of base64 or otherwise. The issue is that the functions btoa and atob are not available when they're called.

The The string to be decoded is not correctly encoded is a red-herring and the error was written this way because it was assumed that that is the only way such a call could fail.

The uint8-to-base64 code does not import atob or btoa from window and just expects them to be available. Something in our configuration or the browser has recently become a lot stricter and this no longer works. Although I could try to bisect the error to figure out if it was something that we changed it would be quite difficult, time consuming and frankly infuriating.

@silverwind may know or be able to point to something where things were made more strict.

@zeripath commented on GitHub (Jan 29, 2023): The problem is not to do with mis-encoding of base64 or otherwise. The issue is that the functions `btoa` and `atob` are not available when they're called. The `The string to be decoded is not correctly encoded` is a red-herring and the error was written this way because it was assumed that that is the only way such a call could fail. The `uint8-to-base64` code does not import `atob` or `btoa` from `window` and just expects them to be available. Something in our configuration or the browser has recently become a lot stricter and this no longer works. Although I could try to bisect the error to figure out if it was something that we changed it would be quite difficult, time consuming and frankly infuriating. @silverwind may know or be able to point to something where things were made more strict.

GiteaMirror commented

@delvh commented on GitHub (Jan 29, 2023):

I've read a bit online, the problem does not seem to come from use strict;, but from Node deprecating and removing the btoa and atob API in favor of window.btoa/window.atob.

@delvh commented on GitHub (Jan 29, 2023): I've read a bit online, the problem does not seem to come from `use strict;`, but from Node deprecating and removing the `btoa` and `atob` API in favor of `window.btoa/window.atob`.

GiteaMirror commented

@wxiaoguang commented on GitHub (Jan 29, 2023):

I do not think you have understood this problem (update: I didn't mean to be impolite, just to confirm the problem, correct me if I was wrong)

The error message is clear: Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded. Your assumption that "atob is not imported" is not true.

I will make a simple demo to show what the problem is.

@wxiaoguang commented on GitHub (Jan 29, 2023): I do not think you have understood this problem (update: I didn't mean to be impolite, just to confirm the problem, correct me if I was wrong) The error message is clear: `Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded.` Your assumption that "`atob` is not imported" is not true. I will make a simple demo to show what the problem is.

GiteaMirror commented

@wxiaoguang commented on GitHub (Jan 29, 2023):

See:

#22654

@wxiaoguang commented on GitHub (Jan 29, 2023): See: * #22654

GiteaMirror commented

@zeripath commented on GitHub (Jan 29, 2023):

Perhaps I was experiencing a different problem - I definitely had some issue whereby the issue was that atob was not present.

@zeripath commented on GitHub (Jan 29, 2023): Perhaps I was experiencing a different problem - I definitely had some issue whereby the issue was that atob was not present.

GiteaMirror commented

@zeripath commented on GitHub (Jan 29, 2023):

OK I've updated #22651 to include these changes.

@zeripath commented on GitHub (Jan 29, 2023): OK I've updated #22651 to include these changes.

GiteaMirror commented

@silverwind commented on GitHub (Jan 30, 2023):

The problem is not to do with mis-encoding of base64 or otherwise. The issue is that the functions btoa and atob are not available when they're called.

I think in a browser environment, those globals are pretty much guaranteed to exist. window is one of the aliases of the global object in browsers so using atob or window.atob are equivalent.

Node.js situation may differ. They only recently introduced the globals. I recommend using globalThis as the global reference instead of window (browser) and global (Node), this is the new standard name and already very well supported.

@silverwind commented on GitHub (Jan 30, 2023): > The problem is not to do with mis-encoding of base64 or otherwise. The issue is that the functions btoa and atob are not available when they're called. I think in a browser environment, those globals are pretty much guaranteed to exist. `window` is one of the aliases of the global object in browsers so using `atob` or `window.atob` are equivalent. Node.js situation may differ. They only recently introduced the globals. I recommend using [`globalThis`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis) as the global reference instead of `window` (browser) and `global` (Node), this is the new standard name and already very well supported.

GiteaMirror commented

@silverwind commented on GitHub (Jan 30, 2023):

The refactor to globalThis can be done anytime. The webpack JS only runs in browsers and jsdom environments, both provide window. Still it should be done eventually.

@silverwind commented on GitHub (Jan 30, 2023): The refactor to `globalThis` can be done anytime. The webpack JS only runs in browsers and jsdom environments, both provide `window`. Still it should be done eventually.

GiteaMirror commented