fix(passkeys): pass challenge as Buffer to simplewebauthn to fix lookup#20362
fix(passkeys): pass challenge as Buffer to simplewebauthn to fix lookup#20362
Conversation
1dca6f7 to
d99d74d
Compare
f1cfa5c to
1e1e1bb
Compare
| export * from './lib/passkey.provider'; | ||
| export * from './lib/passkey.challenge.manager'; | ||
| export * from './lib/webauthn-adapter'; | ||
| export * from './lib/virtual-authenticator'; |
There was a problem hiding this comment.
This is test scaffolding — should it be included in the public barrel? Noting a similar question about passkey.repository and webauthn-adapter, perhaps a quick follow-up?
vpomerleau
left a comment
There was a problem hiding this comment.
Thank you for spotting and resolving the challenge failure. The addition of new tests with virtual authenticator (instead of mocks) is much appreciated. I could see potential for expanding tests to cover more options (like backupState, prfEnabled, signCount regression), but major paths all look covered. No blockers other than ensuring that tests pass, most comments are questions/non-blocking.
| const options = await generateWebauthnRegistrationOptions( | ||
| testConfig({ | ||
| residentKey: 'required', | ||
| userVerification: 'discouraged', |
There was a problem hiding this comment.
Is there a reason to use discouraged and not required here?
There was a problem hiding this comment.
There is not. This gen-ai, but it seemed okay to me. I can switch to required if you think that's more typical.
There was a problem hiding this comment.
We're planning to always require user verification
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| /** | ||
| * Integration tests for the webauthn-adapter that exercise the real |
There was a problem hiding this comment.
Do we need a ticket to add similar tests for authentication?
There was a problem hiding this comment.
I think it'd be helpful. That would ensure wiring it all up goes smoothly.
Because: - simplewebauthn v13 treats string challenges as UTF-8 text and re-encodes them to base64url, producing a different value than what was stored in Redis. This caused challenge lookup to fail on registration/authentication finish. - There were some issues converting UID to a buffer. We must specify 'hex' when converting. - The tests mocked simplewebauth lib, but we want to excercise this code to validate our test cases actually work. This commit: - Passes challenge as Buffer.from(challenge, 'base64url') to generateRegistrationOptions and generateAuthenticationOptions so simplewebauthn base64url-encodes the raw bytes (roundtrip-safe). - Fixes PasskeyService constructor args in the integration test (was missing PasskeyConfig parameter). - Adds webauthn-adapter.in.spec.ts with a virtual authenticator that exercises real simplewebauthn verifyRegistrationResponse (no mocks). - Adds happy-path integration test for registration start → finish.
Because
This pull request
Issue that this pull request solves
Closes: FXA-13343
Checklist
Put an
xin the boxes that applyHow to review (Optional)
Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
At first it appeared that challenges output by
libGenerateAuthenticationOptionswere always changing from the original challenge that is provided to the library call. I assumed that this was by design, however, after adding more tests that actually exercised the library call and doing some debugging it appears the core issue was just the encoding on the challenge. (Which is what Claude thought.)There was one red herring in previous tests worth noting. When using an arbitrary challenge like
test-challengein tests, failures would occur since it doesn't encode cleanly, e.g.Buffer.from('test-challenge', 'base64url').toString('base64url'); -> 'test-challengQ'. Using a actual challenge value likeconst challenge = randomBytes(32).toString('base64url');won't have this problem since the challenge is already base64 encoded.