[PM-43134] Match the WebAuthn spec when parsing passkey creation options - #7360
Open
psolvy wants to merge 1 commit into
Open
[PM-43134] Match the WebAuthn spec when parsing passkey creation options#7360psolvy wants to merge 1 commit into
psolvy wants to merge 1 commit into
Conversation
PasskeyAttestationOptions differs from PublicKeyCredentialCreationOptionsJSON in four places. One breaks registration, the other three lose data quietly. authenticatorSelection is optional in the spec but was declared non-nullable with no default, so a request without it failed to deserialize with MissingFieldException. Registration then ended in MissingHostUrl for an unprivileged caller and InternalError for a privileged one. The remaining three are name mismatches, and they matter beyond parsing: registerFido2CredentialInternal re-serializes this model and sends the result to the SDK, so anything the model cannot name never reaches the authenticator. excludedCredentials -> excludeCredentials, so the relying party's exclusion list is no longer replaced by an empty one and duplicate passkeys can be prevented again cross_platform -> cross-platform, so the attachment survives instead of being coerced to null ResidentKeyRequirement gains discouraged, which the spec defines and which was being coerced to null the same way The fixtures in this repo already used the spec spelling excludeCredentials with empty arrays, which is why none of this showed up in the tests.
|
|
Collaborator
|
Thank you for your contribution! We've added this to our internal tracking system for review. Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process. |
bitwarden-bot
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
#7359
📔 Objective
PasskeyAttestationOptionsdoes not matchPublicKeyCredentialCreationOptionsJSON. There are four mismatches. One breaks registration outright, the other three lose data without an error.authenticatorSelectionwas required. The spec marks onlyrp,user,challengeandpubKeyCredParamsas required. A request that omitsauthenticatorSelectioncurrently fails to deserialize withMissingFieldException, sogetPasskeyAttestationOptionsOrNullreturns null and registration ends inError.MissingHostUrlfor an unprivileged caller, orError.InternalErrorfor a privileged one.VaultAddEditViewModelalso falls back to the package name and an empty username.explicitNulls = falsedoes not cover this, since it only relaxes nullable properties, andcoerceInputValuesonly rewrites values that are present but invalid.Three name mismatches.
excludedCredentialsshould beexcludeCredentials,cross_platformshould becross-platform, andResidentKeyRequirementwas missingdiscouraged. These are not only parsing details, becauseregisterFido2CredentialInternalre-serializes this model and passes the result to the SDK. Anything the model cannot name is dropped before the authenticator sees it, so the relying party's exclusion list never arrives and duplicate passkeys are not prevented.ignoreUnknownKeyshides the first one andcoerceInputValueshides the other two, which is why none of this surfaced as an error.Fixtures in this repo already use the spec spelling
excludeCredentials, atRelyingPartyParserTest.kt:105and:139andBitwardenCredentialManagerTest.kt:1520, but always with an empty array. That is why the existing tests stayed green.Nothing else needed to change. There are no main source construction sites for this model, both test construction sites use named arguments, and the only read of
authenticatorSelectionis already a null safe call inBitwardenCredentialManagerImpl.getUserVerificationRequirement, which falls back to the same value as before.Testing
New
PasskeyAttestationOptionsTestwith 7 tests. I checked each fix separately rather than all at once:@SerialNamerenames fails exactly the 4 tests that cover them and leaves the other 3 passingauthenticatorSelectionis covered by the decode test, which fails withexpected: not <null>on the old modelDISCOURAGEDmember does not exist on the old model, so the test file does not compile against itThe two serialization tests build the model directly instead of decoding first, so they exercise the encoder rather than passing for the wrong reason.
:app:testStandardDebugUnitTestfordata.credentials.*,ui.vault.feature.addedit.*andui.vault.feature.itemlisting.*is green at 956 tests, anddetektreports no findings.Out of scope
The round trip through this model is lossy beyond the fields above.
attestation,timeout,extensionsandhintsare all dropped before the SDK sees the request, and real requests do carry them. That needs a different fix than renaming members, so I left it alone here.