feat!: tighten cross-origin defaults#647
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThreads an explicit RP-level Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.5)
Browser->>Server: Send credential/assertion response (clientData, topOrigin, crossOrigin)
end
rect rgba(200,255,200,0.5)
Server->>Config: Read RPAllowCrossOrigin, RPTopOriginVerificationMode
end
rect rgba(255,230,200,0.5)
Server->>CollectedClientData: Verify(storedChallenge, ceremony, rpOrigins, rpTopOrigins, rpTopOriginMode, allowCrossOrigin)
Note right of CollectedClientData: if c.CrossOrigin && !allowCrossOrigin -> ErrVerification
CollectedClientData-->>Server: verification result / clientDataHash
end
rect rgba(230,200,255,0.5)
Server->>ParsedCredential*: Continue creation/assertion verify flows (attestation / signature checks)
ParsedCredential*-->>Server: final verification result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #647 +/- ##
==========================================
+ Coverage 79.23% 79.33% +0.09%
==========================================
Files 46 46
Lines 3145 3150 +5
==========================================
+ Hits 2492 2499 +7
+ Misses 461 460 -1
+ Partials 192 191 -1
🚀 New features to boost your workflow:
|
james-d-elliott
force-pushed
the
feat-cross-origin-policy
branch
3 times, most recently
from
a0851f7 to
01aad92
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
protocol/attestation_spec_e2e_test.go (1)
31-42:⚠️ Potential issue | 🟠 MajorAdd a real cross-origin E2E vector.
Lines 39 and 110 add
allowCrossOrigin, but every testcase still leaves itfalse, and the fixtures in this file keepcrossOrigin:false. That means lines 91 and 160 only reassert same-origin behavior, so the new policy path is never exercised. Please add at least one passing case withcrossOrigin:true+ matchingtopOrigin, and one failing case with eitherallowCrossOrigin=falseor a mismatchedtopOrigin.I can help generate the minimal hex-encoded
clientDataJSONfixtures if that would save time.Also applies to: 91-92, 100-111, 160-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/attestation_spec_e2e_test.go` around lines 31 - 42, Test vectors never exercise the cross-origin path: update TestSpecVectors_Registration_E2E test cases and associated fixtures to include at least one passing vector with allowCrossOrigin:true and a clientDataJSON whose topOrigin matches the relying party (so the cross-origin check succeeds) and one failing vector where allowCrossOrigin is false or the topOrigin is deliberately mismatched; specifically add entries in the testCases slice (in TestSpecVectors_Registration_E2E) with allowCrossOrigin:true and allowCrossOrigin:false (or mismatched topOrigin) and create corresponding hex-encoded clientDataJSON fixtures that set the topOrigin field to the intended origin values so the test exercises both the success and failure branches for the cross-origin logic.protocol/client.go (1)
210-213:⚠️ Potential issue | 🟡 MinorStale comment references removed constant
TopOriginIgnoreVerificationMode.The comment states "this mode is the same as TopOriginIgnoreVerificationMode" but this constant was removed in this PR. The comment should be updated to reflect the new behavior.
📝 Suggested fix
// TopOriginDefaultVerificationMode represents the default verification mode for the Top Origin. At this time this - // mode is the same as TopOriginIgnoreVerificationMode until such a time as the specification becomes stable. This + // mode is intended as a placeholder until such a time as the specification becomes stable. This // value is intended as a fallback value and implementers should very intentionally pick another option if they want // stability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/client.go` around lines 210 - 213, Update the stale comment for TopOriginDefaultVerificationMode to remove the reference to the removed constant TopOriginIgnoreVerificationMode and describe the current behavior instead; locate the comment adjacent to the TopOriginDefaultVerificationMode declaration and rephrase it to say this is the fallback/default verification mode for Top Origin and specify what the mode currently does (e.g., whether verification is disabled, permissive, or unspecified) so readers don't see a reference to a removed symbol.
🧹 Nitpick comments (2)
protocol/client_test.go (1)
20-20: Typo in field name:allowCrossOrignshould beallowCrossOrigin.This typo is consistent throughout the test file but reduces readability and creates a naming inconsistency with the production code parameter name
allowCrossOrigin.🔤 Suggested fix
- allowCrossOrign bool + allowCrossOrigin boolAlso update all usages in test cases (lines 33, 40, 47, 55, 65, 73, 82, 91, 102, 113, 135) and the Verify call (line 175) from
tc.allowCrossOrigntotc.allowCrossOrigin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/client_test.go` at line 20, The test struct and all usages contain a typo: the field is named allowCrossOrign but should be allowCrossOrigin; rename the struct field to allowCrossOrigin and update every reference in the test file (all tc.allowCrossOrign occurrences in the test cases and the Verify call where tc is used) to tc.allowCrossOrigin so the test names match production parameter naming and compile correctly.protocol/client.go (1)
152-154: Consider usingifinstead ofswitchfor length check.The
switch len(c.TopOrigin)withcase 0: breakanddefault:is an unusual pattern. A straightforwardifstatement would be more idiomatic.♻️ Suggested refactor
- switch len(c.TopOrigin) { - case 0: - break - default: + if len(c.TopOrigin) > 0 { if !c.CrossOrigin { return ErrVerification. WithDetails("Error validating topOrigin"). WithInfo("The topOrigin can't have values unless crossOrigin is true.") } // ... rest of the validation logic ... + } - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/client.go` around lines 152 - 154, The switch on len(c.TopOrigin) (switch len(c.TopOrigin) { case 0: break ... default: ... }) is non-idiomatic; replace it with a simple if/else using the same symbol names so it’s easier to read — e.g., check if len(c.TopOrigin) == 0 then handle that branch (or return) else execute the existing default branch; update the surrounding code in the same method where c.TopOrigin is used so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protocol/assertion.go`:
- Around line 148-155: The Verify method on ParsedCredentialAssertionData
exposes three adjacent bools (allowCrossOrigin, verifyUser, verifyUserPresence)
which is error-prone; replace them with a single options parameter (e.g., type
AssertionVerificationOptions { AllowCrossOrigin bool; VerifyUser bool;
VerifyUserPresence bool }) or add a new method
ParseCredentialAssertionData.VerifyWithOptions(opts
AssertionVerificationOptions) that accepts that struct and leaves Verify as a
thin deprecated shim that calls the new method for backward compatibility;
update all call sites to build and pass the options struct (or use the new
method), and update doc/comments for
ParsedCredentialAssertionData.Verify/VerifyWithOptions to describe the options
and defaults.
In `@webauthn/types.go`:
- Around line 145-146: The Config comment and tests need updating to reflect
that RPTopOriginVerificationMode now defaults to
TopOriginExplicitVerificationMode instead of the ignore/default behavior; update
the Config doc comment above the Config type to describe the new explicit
default and add a regression test (e.g., in types_test.go) that constructs the
config via the public constructor (New(...) or calls validate()) and asserts
that RPTopOriginVerificationMode is set to
protocol.TopOriginExplicitVerificationMode when the zero value or
TopOriginDefaultVerificationMode is provided, preventing future regressions.
Ensure the test references RPTopOriginVerificationMode,
protocol.TopOriginDefaultVerificationMode, and
protocol.TopOriginExplicitVerificationMode and fails if the value differs.
---
Outside diff comments:
In `@protocol/attestation_spec_e2e_test.go`:
- Around line 31-42: Test vectors never exercise the cross-origin path: update
TestSpecVectors_Registration_E2E test cases and associated fixtures to include
at least one passing vector with allowCrossOrigin:true and a clientDataJSON
whose topOrigin matches the relying party (so the cross-origin check succeeds)
and one failing vector where allowCrossOrigin is false or the topOrigin is
deliberately mismatched; specifically add entries in the testCases slice (in
TestSpecVectors_Registration_E2E) with allowCrossOrigin:true and
allowCrossOrigin:false (or mismatched topOrigin) and create corresponding
hex-encoded clientDataJSON fixtures that set the topOrigin field to the intended
origin values so the test exercises both the success and failure branches for
the cross-origin logic.
In `@protocol/client.go`:
- Around line 210-213: Update the stale comment for
TopOriginDefaultVerificationMode to remove the reference to the removed constant
TopOriginIgnoreVerificationMode and describe the current behavior instead;
locate the comment adjacent to the TopOriginDefaultVerificationMode declaration
and rephrase it to say this is the fallback/default verification mode for Top
Origin and specify what the mode currently does (e.g., whether verification is
disabled, permissive, or unspecified) so readers don't see a reference to a
removed symbol.
---
Nitpick comments:
In `@protocol/client_test.go`:
- Line 20: The test struct and all usages contain a typo: the field is named
allowCrossOrign but should be allowCrossOrigin; rename the struct field to
allowCrossOrigin and update every reference in the test file (all
tc.allowCrossOrign occurrences in the test cases and the Verify call where tc is
used) to tc.allowCrossOrigin so the test names match production parameter naming
and compile correctly.
In `@protocol/client.go`:
- Around line 152-154: The switch on len(c.TopOrigin) (switch len(c.TopOrigin) {
case 0: break ... default: ... }) is non-idiomatic; replace it with a simple
if/else using the same symbol names so it’s easier to read — e.g., check if
len(c.TopOrigin) == 0 then handle that branch (or return) else execute the
existing default branch; update the surrounding code in the same method where
c.TopOrigin is used so behavior remains identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7d54ab26-e566-486c-b1ca-3c53c62e9e6a
📒 Files selected for processing (11)
protocol/assertion.goprotocol/assertion_test.goprotocol/attestation_spec_e2e_test.goprotocol/attestation_test.goprotocol/client.goprotocol/client_test.goprotocol/credential.goprotocol/credential_test.gowebauthn/login.gowebauthn/registration.gowebauthn/types.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
protocol/client.go (1)
152-180: Consider simplifying the switch to an if-else.The
switch len(c.TopOrigin)with onlycase 0:anddefault:branches could be expressed more idiomatically as anifstatement. Additionally, thebreakis unnecessary in Go since switch cases don't fall through by default.♻️ Suggested simplification
- switch len(c.TopOrigin) { - case 0: - break - default: + if len(c.TopOrigin) != 0 { if !c.CrossOrigin { return ErrVerification. WithDetails("Error validating topOrigin"). WithInfo("The topOrigin can't have values unless crossOrigin is true.") } var possibleTopOrigins []string switch rpTopOriginsVerify { case TopOriginExplicitVerificationMode: possibleTopOrigins = rpTopOrigins case TopOriginAutoVerificationMode: possibleTopOrigins = append(rpTopOrigins, rpOrigins...) //nolint:gocritic // This is intentional. case TopOriginImplicitVerificationMode: possibleTopOrigins = rpOrigins default: return ErrNotImplemented.WithDetails("Error handling unknown Top Origin verification mode") } if !IsOriginInHaystack(c.TopOrigin, possibleTopOrigins) { return ErrVerification. WithDetails("Error validating top origin"). WithInfo(fmt.Sprintf("Expected Values: %s, Received: %s", possibleTopOrigins, c.TopOrigin)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/client.go` around lines 152 - 180, The switch on len(c.TopOrigin) is unnecessarily verbose; replace the switch block with a simple if len(c.TopOrigin) == 0 { /* no-op */ } else { ... } (or more idiomatically if len(c.TopOrigin) != 0 { ... }) to handle the non-empty top origin path. Keep the existing logic inside the non-empty branch: check c.CrossOrigin and return ErrVerification with details if false, compute possibleTopOrigins based on rpTopOriginsVerify (TopOriginExplicitVerificationMode, TopOriginAutoVerificationMode, TopOriginImplicitVerificationMode) and return ErrNotImplemented for the default case, then call IsOriginInHaystack(c.TopOrigin, possibleTopOrigins) and return ErrVerification with formatted info if it returns false; preserve all referenced symbols (c.TopOrigin, c.CrossOrigin, rpTopOriginsVerify, rpTopOrigins, rpOrigins, possibleTopOrigins, IsOriginInHaystack, ErrVerification, ErrNotImplemented).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@protocol/client.go`:
- Around line 152-180: The switch on len(c.TopOrigin) is unnecessarily verbose;
replace the switch block with a simple if len(c.TopOrigin) == 0 { /* no-op */ }
else { ... } (or more idiomatically if len(c.TopOrigin) != 0 { ... }) to handle
the non-empty top origin path. Keep the existing logic inside the non-empty
branch: check c.CrossOrigin and return ErrVerification with details if false,
compute possibleTopOrigins based on rpTopOriginsVerify
(TopOriginExplicitVerificationMode, TopOriginAutoVerificationMode,
TopOriginImplicitVerificationMode) and return ErrNotImplemented for the default
case, then call IsOriginInHaystack(c.TopOrigin, possibleTopOrigins) and return
ErrVerification with formatted info if it returns false; preserve all referenced
symbols (c.TopOrigin, c.CrossOrigin, rpTopOriginsVerify, rpTopOrigins,
rpOrigins, possibleTopOrigins, IsOriginInHaystack, ErrVerification,
ErrNotImplemented).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e8ee3ab0-6f59-4c3f-bb4a-4eb5504a5833
📒 Files selected for processing (11)
protocol/assertion.goprotocol/assertion_test.goprotocol/attestation_spec_e2e_test.goprotocol/attestation_test.goprotocol/client.goprotocol/client_test.goprotocol/credential.goprotocol/credential_test.gowebauthn/login.gowebauthn/registration.gowebauthn/types.go
✅ Files skipped from review due to trivial changes (2)
- webauthn/registration.go
- protocol/attestation_spec_e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- protocol/attestation_test.go
- protocol/credential_test.go
- webauthn/types.go
- webauthn/login.go
- protocol/assertion.go
- protocol/credential.go
- protocol/client_test.go
james-d-elliott
force-pushed
the
feat-cross-origin-policy
branch
from
01aad92 to
287c48d
Compare
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 29652279 | Triggered | Generic High Entropy Secret | 18f46fb | protocol/specification_vectors_e2e_test.go | View secret |
| 29652279 | Triggered | Generic High Entropy Secret | 18f46fb | protocol/specification_vectors_e2e_test.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webauthn/registration.go (1)
95-98:⚠️ Potential issue | 🟠 MajorUse
session.RelyingPartyIDfor finish-time verification.
BeginRegistrationpersists the exact RP ID used for this ceremony inSessionData.RelyingPartyID, but Line 146 still verifies againstwebauthn.Config.RPID. If a functional option overrides the RP ID, or the config changes between begin and finish, valid registrations will be checked against the wrong RP ID.💡 Minimal fix
- if clientDataHash, err = parsedResponse.Verify(session.Challenge, webauthn.Config.RPID, webauthn.Config.RPOrigins, webauthn.Config.RPTopOrigins, webauthn.Config.RPTopOriginVerificationMode, webauthn.Config.RPAllowCrossOrigin, shouldVerifyUser, shouldVerifyUserPresence, webauthn.Config.MDS, session.CredParams); err != nil { + if clientDataHash, err = parsedResponse.Verify(session.Challenge, session.RelyingPartyID, webauthn.Config.RPOrigins, webauthn.Config.RPTopOrigins, webauthn.Config.RPTopOriginVerificationMode, webauthn.Config.RPAllowCrossOrigin, shouldVerifyUser, shouldVerifyUserPresence, webauthn.Config.MDS, session.CredParams); err != nil {Also applies to: 146-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webauthn/registration.go` around lines 95 - 98, BeginRegistration stores the exact RP ID in SessionData.RelyingPartyID but FinishRegistration still uses webauthn.Config.RPID for verification; change the finish-time verification to use session.RelyingPartyID instead of webauthn.Config.RPID. Locate where FinishRegistration (or the code that calls webauthn.FinishRegistration / verifies the attestation) passes the RPID or verification options and replace the hardcoded webauthn.Config.RPID with session.RelyingPartyID (or pass session.RelyingPartyID into the verification options) so the same RP ID saved in SessionData is used for verification.webauthn/login.go (1)
125-127:⚠️ Potential issue | 🟠 MajorValidate assertions against
session.RelyingPartyID, not the current config.
BeginLoginsaves the RP ID actually sent to the client inSessionData.RelyingPartyID, but this path still feedswebauthn.Config.RPIDintoparsedResponse.Verify. That makes valid assertions fail if the ceremony overrode the RP ID or the config changed after the session was created.💡 Minimal fix
- rpID := webauthn.Config.RPID + rpID := session.RelyingPartyIDAlso applies to: 357-366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webauthn/login.go` around lines 125 - 127, The verification is using the current config RPID instead of the RP ID stored on the session (SessionData.RelyingPartyID), so assertions created under an overridden RP ID or with a changed config will fail; update the call sites that call parsedResponse.Verify (and any other verification that pass webauthn.Config.RPID) to pass the RP ID from the session (session.RelyingPartyID) instead of webauthn.Config.RPID—look for uses in the login flow including the block creating SessionData (where Challenge and RelyingPartyID are set) and the similar verification block around the 357-366 region and replace the config reference with session.RelyingPartyID.
♻️ Duplicate comments (1)
webauthn/types.go (1)
54-57:⚠️ Potential issue | 🟡 MinorUpdate the
RPTopOriginVerificationModedocstring.Line 146 now defaults to
protocol.TopOriginExplicitVerificationMode, but the exported comment still says ignore-mode is the default. That leaves the public API docs wrong for a security-sensitive setting.Also applies to: 145-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webauthn/types.go` around lines 54 - 57, The doc comment for RPTopOriginVerificationMode is stale (it says TopOriginIgnoreVerificationMode is the default) — update the exported comment for RPTopOriginVerificationMode to state that the default is now protocol.TopOriginExplicitVerificationMode (or explain the new behavior) and note that implementers should set it explicitly if they need a different mode; ensure the comment text mentions the exact symbol protocol.TopOriginExplicitVerificationMode so generated docs reflect the current default.
🧹 Nitpick comments (1)
protocol/specification_vectors_e2e_test.go (1)
363-390: Give the allowed and denied top-origin auth cases distinct names.Both subtests use
NoneES256TopOrigin, sogo testwill auto-suffix one of them and make failures less obvious to triage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/specification_vectors_e2e_test.go` around lines 363 - 390, The two test cases share the same name value "NoneES256TopOrigin", causing go test to auto-suffix one; update the name field on each test vector to unique, descriptive values (e.g., "NoneES256TopOrigin_DenyCrossOrigin" for the case with allowCrossOrigin: false and "NoneES256TopOrigin_AllowCrossOrigin" for the case with allowCrossOrigin: true) so failures are unambiguous—modify the name field in the test entries that include rpTopOriginVerificationMode (TopOriginExplicitVerificationMode) and allowCrossOrigin to reflect the different behaviors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@webauthn/login.go`:
- Around line 125-127: The verification is using the current config RPID instead
of the RP ID stored on the session (SessionData.RelyingPartyID), so assertions
created under an overridden RP ID or with a changed config will fail; update the
call sites that call parsedResponse.Verify (and any other verification that pass
webauthn.Config.RPID) to pass the RP ID from the session
(session.RelyingPartyID) instead of webauthn.Config.RPID—look for uses in the
login flow including the block creating SessionData (where Challenge and
RelyingPartyID are set) and the similar verification block around the 357-366
region and replace the config reference with session.RelyingPartyID.
In `@webauthn/registration.go`:
- Around line 95-98: BeginRegistration stores the exact RP ID in
SessionData.RelyingPartyID but FinishRegistration still uses
webauthn.Config.RPID for verification; change the finish-time verification to
use session.RelyingPartyID instead of webauthn.Config.RPID. Locate where
FinishRegistration (or the code that calls webauthn.FinishRegistration /
verifies the attestation) passes the RPID or verification options and replace
the hardcoded webauthn.Config.RPID with session.RelyingPartyID (or pass
session.RelyingPartyID into the verification options) so the same RP ID saved in
SessionData is used for verification.
---
Duplicate comments:
In `@webauthn/types.go`:
- Around line 54-57: The doc comment for RPTopOriginVerificationMode is stale
(it says TopOriginIgnoreVerificationMode is the default) — update the exported
comment for RPTopOriginVerificationMode to state that the default is now
protocol.TopOriginExplicitVerificationMode (or explain the new behavior) and
note that implementers should set it explicitly if they need a different mode;
ensure the comment text mentions the exact symbol
protocol.TopOriginExplicitVerificationMode so generated docs reflect the current
default.
---
Nitpick comments:
In `@protocol/specification_vectors_e2e_test.go`:
- Around line 363-390: The two test cases share the same name value
"NoneES256TopOrigin", causing go test to auto-suffix one; update the name field
on each test vector to unique, descriptive values (e.g.,
"NoneES256TopOrigin_DenyCrossOrigin" for the case with allowCrossOrigin: false
and "NoneES256TopOrigin_AllowCrossOrigin" for the case with allowCrossOrigin:
true) so failures are unambiguous—modify the name field in the test entries that
include rpTopOriginVerificationMode (TopOriginExplicitVerificationMode) and
allowCrossOrigin to reflect the different behaviors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cdd7d4d4-59de-464d-b484-e4d8fdb6212a
📒 Files selected for processing (11)
protocol/assertion.goprotocol/assertion_test.goprotocol/attestation_test.goprotocol/client.goprotocol/client_test.goprotocol/credential.goprotocol/credential_test.goprotocol/specification_vectors_e2e_test.gowebauthn/login.gowebauthn/registration.gowebauthn/types.go
✅ Files skipped from review due to trivial changes (1)
- protocol/assertion_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- protocol/attestation_test.go
- protocol/credential.go
- protocol/credential_test.go
james-d-elliott
force-pushed
the
feat-cross-origin-policy
branch
from
287c48d to
8dcde35
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protocol/client_test.go`:
- Around line 188-197: The test constructs collected client data with
crossOrigin=true via setupCollectedClientData but calls ccd.Verify(..., false)
so the verify call disallows cross-origin and could cause a cross-origin failure
before the intended challenge mismatch assertion; change the Verify invocation
in TestVerifyCollectedClientData_IncorrectChallenge to pass true for the
allowCrossOrigin parameter (i.e., call ccd.Verify(...,
TopOriginExplicitVerificationMode, true)) so the test isolates and asserts the
challenge mismatch as intended.
In `@protocol/client.go`:
- Line 168: The exported documentation for the TopOrigin mode is out of sync
with the implementation: remove references to the deleted
TopOriginIgnoreVerificationMode and update the TopOrigin auto mode docs to state
that "auto" behaves as the union of RPTopOrigins and RPOrigins (matching the
implementation where possibleTopOrigins = append(rpTopOrigins, rpOrigins...)),
not a fallback-only behavior; edit the exported TopOrigin mode doc block and the
comments around the TopOrigin constants/variables (the doc text covering lines
~210-218) to accurately describe current behavior and remove the stale reference
to the removed mode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5ddba587-3366-42c9-a6b0-c1fa943d71e7
📒 Files selected for processing (12)
protocol/assertion.goprotocol/assertion_test.goprotocol/attestation_test.goprotocol/client.goprotocol/client_test.goprotocol/credential.goprotocol/credential_test.goprotocol/specification_vectors_e2e_test.gowebauthn/login.gowebauthn/registration.gowebauthn/types.gowebauthn/types_test.go
✅ Files skipped from review due to trivial changes (1)
- protocol/attestation_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- protocol/assertion_test.go
- protocol/assertion.go
- protocol/credential_test.go
- protocol/specification_vectors_e2e_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protocol/client.go`:
- Around line 162-168: The switch branch for TopOriginAutoVerificationMode
mutates rpTopOrigins via append(rpTopOrigins, rpOrigins...) which can alter its
backing array; change it to create an independent slice for possibleTopOrigins
(capacity len(rpTopOrigins)+len(rpOrigins)), copy rpTopOrigins into it, then
copy/append rpOrigins into that new slice so rpTopOrigins and rpOrigins are not
modified—update the code paths in the switch that set possibleTopOrigins (and
any uses of possibleTopOrigins) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
james-d-elliott
changed the title
james-d-elliott
changed the title
james-d-elliott
force-pushed
the
feat-cross-origin-policy
branch
from
a3c4eda to
d446583
Compare
james-d-elliott
changed the title
james-d-elliott
changed the title
Cross-Origin WebAuthn ceremonies are now rejected by default.
Integrators that need to accept them must opt in by setting the new
[webauthn.Config.RPAllowCrossOrigin] field to true.
The previously-permissive Top Origin handling has also been tightened.
The zero value of [protocol.TopOriginVerificationMode] is now coerced
by [webauthn.Config.validate] to
[protocol.TopOriginExplicitVerificationMode]; the strictest active
mode, which accepts a Top Origin only if it matches an entry in
[webauthn.Config.RPTopOrigins]. The opt-out mode
protocol.TopOriginIgnoreVerificationMode has been removed entirely;
[protocol.CollectedClientData.Verify] returns an "unknown Top Origin
verification mode" error if it is ever encountered.
Together these changes align the library with the W3C WebAuthn Level 3
guidance that Cross-Origin embedding and Top Origin acceptance should
be explicit relying-party decisions rather than defaults.
BREAKING CHANGE: The Cross-Origin verification semantics have changed
significantly due to the stabilization of the WebAuthn Level 3
specification. It is no longer possible to disable verification, and
Cross-Origin ceremonies must explicitly be allowed in this release.
- protocol.TopOriginIgnoreVerificationMode has been removed. Code that
referenced it must switch to one of the other constants as there is
no longer a mode which disables the Top Origin verification such as:
- TopOriginExplicitVerificationMode; match against RPTopOrigins only
(recommended, and the new coerced default)
- TopOriginAutoVerificationMode; match against the union of
RPTopOrigins and RPOrigins
- TopOriginImplicitVerificationMode; match against RPOrigins only
- webauthn.Config.validate now rewrites a zero-valued
RPTopOriginVerificationMode to TopOriginExplicitVerificationMode.
Integrators that left the field unset previously got ignore-mode
semantics (any Top Origin accepted); they now get strict matching
against RPTopOrigins and must populate that list, or explicitly
select a different mode; for Cross-Origin flows to succeed.
- Cross-Origin ceremonies (those where the authenticator reports
crossOrigin = true in the ClientData) are rejected by default.
Integrators that rely on iframe-embedded or other Cross-Origin WebAuthn
flows must set webauthn.Config.RPAllowCrossOrigin = true. The library
continues to enforce Top Origin verification on accepted Cross-Origin
ceremonies per the configured mode.
- protocol.CollectedClientData.Verify no longer accepts
TopOriginIgnoreVerificationMode; callers that pass an unknown mode
receive ErrNotImplemented with detail "unknown Top Origin
verification mode".
james-d-elliott
force-pushed
the
feat-cross-origin-policy
branch
from
d446583 to
18f46fb
Compare
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
1 participant
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.
Cross-Origin WebAuthn ceremonies are now rejected by default.
Integrators that need to accept them must opt in by setting the new
[webauthn.Config.RPAllowCrossOrigin] field to true.
The previously-permissive Top Origin handling has also been tightened.
The zero value of [protocol.TopOriginVerificationMode] is now coerced
by [webauthn.Config.validate] to
[protocol.TopOriginExplicitVerificationMode]; the strictest active
mode, which accepts a Top Origin only if it matches an entry in
[webauthn.Config.RPTopOrigins]. The opt-out mode
protocol.TopOriginIgnoreVerificationMode has been removed entirely;
[protocol.CollectedClientData.Verify] returns an "unknown Top Origin
verification mode" error if it is ever encountered.
Together these changes align the library with the W3C WebAuthn Level 3
guidance that Cross-Origin embedding and Top Origin acceptance should
be explicit relying-party decisions rather than defaults.
BREAKING CHANGE: The Cross-Origin verification semantics have changed
significantly due to the stabilization of the WebAuthn Level 3
specification. It is no longer possible to disable verification, and
Cross-Origin ceremonies must explicitly be allowed in this release.
protocol.TopOriginIgnoreVerificationMode has been removed. Code that
referenced it must switch to one of the other constants as there is
no longer a mode which disables the Top Origin verification such as:
(recommended, and the new coerced default)
RPTopOrigins and RPOrigins
webauthn.Config.validate now rewrites a zero-valued
RPTopOriginVerificationMode to TopOriginExplicitVerificationMode.
Integrators that left the field unset previously got ignore-mode
semantics (any Top Origin accepted); they now get strict matching
against RPTopOrigins and must populate that list, or explicitly
select a different mode; for Cross-Origin flows to succeed.
Cross-Origin ceremonies (those where the authenticator reports
crossOrigin = true in the ClientData) are rejected by default.
Integrators that rely on iframe-embedded or other Cross-Origin WebAuthn
flows must set webauthn.Config.RPAllowCrossOrigin = true. The library
continues to enforce Top Origin verification on accepted Cross-Origin
ceremonies per the configured mode.
protocol.CollectedClientData.Verify no longer accepts
TopOriginIgnoreVerificationMode; callers that pass an unknown mode
receive ErrNotImplemented with detail "unknown Top Origin
verification mode".