Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Login with security keys no longer possible #22507

Closed
TommyTran732 opened this issue Jan 18, 2023 · 17 comments · Fixed by #22651
Closed

Login with security keys no longer possible #22507

TommyTran732 opened this issue Jan 18, 2023 · 17 comments · Fixed by #22651
Labels
skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Milestone

Comments

@TommyTran732
Copy link

Description

I get Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded when doing the FIDO2 authentication and cannot login.

Screenshot 2023-01-18 at 7 59 14 AM

Gitea Version

1.19.0+dev-320-gde484e86b

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

No response

Screenshots

No response

Git Version

No response

Operating System

No response

How are you running Gitea?

gitea:dev-rootless container

Database

MySQL

@lunny lunny added this to the 1.19.0 milestone Jan 18, 2023
@lunny lunny added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jan 18, 2023
@zeripath
Copy link
Contributor

Are there any server logs associated?

@wxiaoguang
Copy link
Contributor

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)

@lunny
Copy link
Member

lunny commented Jan 19, 2023

Maybe related #22400

@silverwind
Copy link
Member

silverwind commented 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?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented 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
Copy link
Contributor

wxiaoguang commented 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

image

@wxiaoguang
Copy link
Contributor

And you see, I have questioned before:

image

"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.

@silverwind
Copy link
Member

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.

zeripath added a commit to zeripath/gitea that referenced this issue Jan 29, 2023
There is a missing import within the `uint8-to-base64` javascript
package which assumes that `atob` and `btoa` are present and exported
instead of using the `window.atob` and `window.btoa` functions. This
previously worked but as far as I can see things have become more strict
and this no longer works.

The dependency is small and I do not believe that we gain much from
having this code as an external dependency. I think instead we should
just consume this dependency and bring the code directly into Gitea
itself - the code is itself just some standard incantation for creating
base64 arrays in javascript.

Therefore this PR simply removes the dependency on `uint8-to-base64` and
rewrites the functions used in it.

Fix go-gitea#22507

Signed-off-by: Andrew Thornton <art27@cantab.net>
@delvh delvh changed the title Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded. Login with security keys no longer possible Jan 29, 2023
@zeripath
Copy link
Contributor

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.

@delvh
Copy link
Member

delvh commented 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.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented 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.

@zeripath
Copy link
Contributor

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

@zeripath
Copy link
Contributor

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

@silverwind
Copy link
Member

silverwind commented 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
Copy link
Member

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.

@james-d-elliott
Copy link

james-d-elliott commented Feb 1, 2023

This is an issue or related to an issue introduced in github.com/go-webauthn/webauthn most likely. Basically when the library was being maintained previously shadowed a method improperly and was not properly marshalling/unmarshalling the []byte values into Base64 Web Encoding. This was technically fixed in 0.6.0 but it had ill effects like this if the browser library also implemented it expecting this input/output. The specification dictates the format of these values be base64 web encoding with the padding omitted.

Two potential ways to fix this in the short term would be to revert the mod version temporarily, or potentially upgrade to 0.7.0 where I sured up these changes a bit more accurately.

For reference these changes in 0.7.0 have been tested with @simplewebauthn/browser.

zeripath added a commit that referenced this issue Feb 1, 2023
This PR fixes two bugs with Webauthn support:

* There was a longstanding bug within webauthn due to the backend using
URLEncodedBase64 but the javascript using decoding using plain base64.
This causes intermittent issues with users reporting decoding errors.
* Following the recent upgrade to webauthn there was a change in the way
the library expects RPOrigins to be configured. This leads to the
Relying Party Origin not being configured and prevents registration.

Fix #22507

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zeripath added a commit to zeripath/gitea that referenced this issue Feb 1, 2023
…#22651)

Partial Backport go-gitea#22651

This PR fixes a longstanding bug within webauthn due to the backend using
URLEncodedBase64 but the javascript using decoding using plain base64.
This causes intermittent issues with users reporting decoding errors.

Fix go-gitea#22507

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
lunny added a commit that referenced this issue Feb 2, 2023
…22721)

Partial Backport #22651

This PR fixes a longstanding bug within webauthn due to the backend
using URLEncodedBase64 but the javascript using decoding using plain
base64. This causes intermittent issues with users reporting decoding
errors.

Fix #22507

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants