fix(protocol): conditional create fail #434
Conversation
This fixes conditional create would fail. In this case, UP and UV flags will always false. According to WebAuthn L3 spec, UP flag will be verified when options.mediation is not set to "conditional". Step 15: https://www.w3.org/TR/webauthn-3/#sctn-registering-a-new-credential Related to go-webauthn#361
WalkthroughThe changes introduce a new boolean parameter to several Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebAuthnServer
participant Protocol
Client->>WebAuthnServer: Submit credential (login/register)
WebAuthnServer->>SessionData: Check Mediation (registration only)
WebAuthnServer->>WebAuthnServer: Set shouldVerifyUserPresence flag
WebAuthnServer->>Protocol: Call Verify(..., verifyUser, verifyUserPresence, ...)
Protocol->>Protocol: Verify user presence if verifyUserPresence is true
Protocol-->>WebAuthnServer: Return verification result
WebAuthnServer-->>Client: Respond with outcome
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
protocol/assertion.go(2 hunks)protocol/assertion_test.go(1 hunks)protocol/attestation.go(1 hunks)protocol/attestation_test.go(1 hunks)protocol/authenticator.go(2 hunks)protocol/authenticator_test.go(4 hunks)protocol/credential.go(2 hunks)protocol/credential_test.go(1 hunks)webauthn/login.go(2 hunks)webauthn/registration.go(2 hunks)webauthn/types.go(1 hunks)
🔇 Additional comments (19)
webauthn/login.go (1)
371-371: Verify the Verify method signature update.The addition of the
shouldVerifyUserPresenceparameter to theVerifymethod call looks correct and aligns with the protocol-level changes.However, ensure this change is coordinated with the fix for line 360 to properly implement conditional user presence verification.
webauthn/types.go (1)
215-215: Good addition for conditional verification support.The
Mediationfield addition toSessionDataproperly supports the conditional user presence verification feature. The field type and JSON tag are appropriate.protocol/attestation_test.go (1)
32-32: Test method signature update looks correct.The addition of the
falseparameter forverifyUserPresencecorrectly matches the updatedVerifymethod signature from the protocol changes.protocol/assertion_test.go (1)
183-183: Test method signature update is correct.The addition of the
falseparameter forverifyUserPresencecorrectly aligns with the updatedVerifymethod signature in the protocol layer.webauthn/registration.go (3)
104-104: Proper mediation tracking in session data.Setting
Mediation: creation.Mediationcorrectly stores the mediation requirement in the session for later use during verification.
235-235: Correct implementation of conditional user presence logic.The logic
session.Mediation != protocol.MediationConditionalproperly implements the WebAuthn Level 3 specification requirement that user presence should only be verified when mediation is not "conditional".
239-239: Verify method call updated correctly.The addition of the
shouldVerifyUserPresenceparameter to theparsedResponse.Verify()call correctly integrates with the protocol-level changes.protocol/credential_test.go (1)
246-246: LGTM: Test updated correctly for new method signature.The test call properly includes the new
verifyUserPresenceparameter set tofalse, which is appropriate for this general verification test case.protocol/authenticator_test.go (3)
434-434: LGTM: Test structure properly extended for user presence requirements.The addition of
userPresenceRequiredparameter to the test args struct correctly supports testing the new conditional user presence verification logic.
477-483: LGTM: Test case correctly validates conditional user presence checking.The test properly sets
userPresenceRequired: trueand validates the improved error message when the User Present flag is not set but required.
512-512: LGTM: Method call correctly passes the new parameter.The call to
a.Verify()properly includes theuserPresenceRequiredparameter from the test arguments.protocol/attestation.go (2)
131-131: LGTM: Method signature correctly updated for conditional user presence verification.The addition of the
userPresenceRequiredparameter to theVerifymethod signature is consistent with the broader changes to support conditional user presence checking.
135-135: LGTM: Parameter properly propagated to AuthData verification.The
userPresenceRequiredparameter is correctly passed to the underlyingAuthData.Verify()method, maintaining the verification flow integrity.protocol/authenticator.go (3)
388-388: LGTM: Method signature correctly updated for conditional user presence verification.The addition of the
userPresenceRequiredparameter enables conditional enforcement of the User Present flag check, which is the core fix for this PR.
397-401: LGTM: Conditional user presence verification correctly implemented.The logic
if userPresenceRequired && !a.Flags.UserPresent()properly implements conditional UP flag checking per WebAuthn Level 3 specification. The error message improvement removes unnecessaryfmt.Sprintffor a static string.
407-407: LGTM: Error message simplified appropriately.Removing
fmt.Sprintffor the static error message is a good improvement for readability and performance.protocol/assertion.go (2)
142-142: LGTM: Method signature consistently updated for conditional user presence verification.The addition of the
verifyUserPresenceparameter maintains consistency with the conditional user presence verification functionality across the assertion verification flow.
163-163: LGTM: Parameter correctly propagated to authenticator data verification.The
verifyUserPresenceparameter is properly passed to theAuthenticatorData.Verify()method, ensuring the conditional user presence logic is applied during assertion verification.protocol/credential.go (1)
164-164: Verify the parameter order matches the updated AttestationObject.Verify signature.The
verifyUserPresenceparameter is correctly passed to theAttestationObject.Verifymethod. Ensure that the parameter order in this call matches the expected signature of theAttestationObject.Verifymethod.#!/bin/bash # Description: Verify the AttestationObject.Verify method signature to ensure parameter order matches # Search for the AttestationObject.Verify method definition ast-grep --pattern $'func ($$$) Verify($$$) $$$'
| @@ -357,6 +357,7 @@ func (webauthn *WebAuthn) validateLogin(user User, session SessionData, parsedRe | |||
| } | |||
|
|
|||
| shouldVerifyUser := session.UserVerification == protocol.VerificationRequired | |||
| shouldVerifyUserPresence := true | |||
There was a problem hiding this comment.
Implement conditional user presence verification logic.
The hardcoded shouldVerifyUserPresence := true doesn't align with the WebAuthn Level 3 specification and the fix described in the PR objectives. According to the spec, user presence should only be verified when mediation is not "conditional".
Apply this diff to implement proper conditional logic:
- shouldVerifyUserPresence := true
+ shouldVerifyUserPresence := session.Mediation != protocol.MediationConditionalThis matches the implementation in webauthn/registration.go line 235 and ensures consistency across login and registration flows.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| shouldVerifyUserPresence := true | |
| - shouldVerifyUserPresence := true | |
| + shouldVerifyUserPresence := session.Mediation != protocol.MediationConditional |
🤖 Prompt for AI Agents
In webauthn/login.go at line 360, replace the hardcoded assignment of
shouldVerifyUserPresence to true with conditional logic that sets it to true
only when the mediation value is not "conditional". This aligns with the
WebAuthn Level 3 specification and matches the approach used in
webauthn/registration.go at line 235, ensuring consistent user presence
verification behavior between login and registration flows.
| @@ -145,7 +145,7 @@ func (ccr CredentialCreationResponse) Parse() (pcc *ParsedCredentialCreationData | |||
| // Verify the Client and Attestation data. | |||
| // | |||
| // Specification: §7.1. Registering a New Credential (https://www.w3.org/TR/webauthn/#sctn-registering-a-new-credential) | |||
| func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, mds metadata.Provider, credParams []CredentialParameter) (clientDataHash []byte, err error) { | |||
| func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, verifyUserPresence bool, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, mds metadata.Provider, credParams []CredentialParameter) (clientDataHash []byte, err error) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider parameter placement for less disruptive API changes.
Adding the new verifyUserPresence parameter in the middle of the parameter list creates a more disruptive breaking change than necessary. Consider placing new parameters at the end of the parameter list in future API changes to minimize disruption.
Additionally, the method documentation should be updated to explain the new parameter's purpose and its relationship to the WebAuthn Level 3 conditional mediation behavior.
-func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, verifyUserPresence bool, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, mds metadata.Provider, credParams []CredentialParameter) (clientDataHash []byte, err error) {
+func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, mds metadata.Provider, credParams []CredentialParameter, verifyUserPresence bool) (clientDataHash []byte, err error) {Update the method documentation:
// Verify the Client and Attestation data.
+// The verifyUserPresence parameter controls whether the User Present (UP) flag is verified,
+// allowing conditional mediation behavior as per WebAuthn Level 3 specification.
//
// Specification: §7.1. Registering a New Credential (https://www.w3.org/TR/webauthn/#sctn-registering-a-new-credential)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, verifyUserPresence bool, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, mds metadata.Provider, credParams []CredentialParameter) (clientDataHash []byte, err error) { | |
| // Verify the Client and Attestation data. | |
| // The verifyUserPresence parameter controls whether the User Present (UP) flag is verified, | |
| // allowing conditional mediation behavior as per WebAuthn Level 3 specification. | |
| // | |
| // Specification: §7.1. Registering a New Credential (https://www.w3.org/TR/webauthn/#sctn-registering-a-new-credential) | |
| func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, mds metadata.Provider, credParams []CredentialParameter, verifyUserPresence bool) (clientDataHash []byte, err error) { | |
| // existing implementation... | |
| } |
🤖 Prompt for AI Agents
In protocol/credential.go at line 148, the new parameter verifyUserPresence is
inserted in the middle of the Verify method's parameter list, causing a
disruptive API change. To fix this, move verifyUserPresence to the end of the
parameter list to minimize breaking changes. Also, update the method's
documentation to describe the purpose of verifyUserPresence and how it relates
to WebAuthn Level 3 conditional mediation behavior.
This fixes conditional create would fail.
In this case, UP and UV flags will always false.
According to WebAuthn L3 spec, UP flag will be verified when options.mediation is not set to "conditional".
Step 15:
https://www.w3.org/TR/webauthn-3/#sctn-registering-a-new-credential
Related to #361