feat(protocol): validate native app origins #468
Conversation
WalkthroughThe verification logic for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 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
🔭 Outside diff range comments (1)
protocol/client.go (1)
145-149: topOrigin: uninitialized variable in error path and wrong slice formatting; also subject to raw-string equality
- fqTopOrigin is never assigned, so the error always reports an empty Received value.
- %s is incorrect for []string; use %v (or strings.Join).
- Raw-string comparison shares the same normalization issue as Origin.
Minimal fix for the error reporting:
- return ErrVerification. - WithDetails("Error validating top origin"). - WithInfo(fmt.Sprintf("Expected Values: %s, Received: %s", possibleTopOrigins, fqTopOrigin)) + return ErrVerification. + WithDetails("Error validating top origin"). + WithInfo(fmt.Sprintf("Expected Values: %v, Received: %s", possibleTopOrigins, c.TopOrigin))Additionally, remove the unused fqTopOrigin variable declared above:
// remove 'fqTopOrigin string' from the var block var ( // fqTopOrigin string // <- delete this possibleTopOrigins []string )Recommend using the normalized comparison in the helper (see Lines 198-216) to keep HTTP(S) semantics intact while supporting native identifiers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protocol/client.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
protocol/client.go (3)
protocol/errors.go (1)
ErrVerification(63-66)protocol/client_test.go (5)
TestVerifyCollectedClientDataUnexpectedOrigin(45-58)TestVerifyCollectedClientDataWithMultipleExpectedOrigins(60-75)TestVerifyCollectedClientData(20-29)TestFullyQualifiedOrigin(77-108)setupCollectedClientData(10-18)webauthn/types.go (1)
GetTopOriginVerificationMode(155-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
james-d-elliott
force-pushed
the
feat-origin-native
branch
from
7ab8e2e to
9c9d7d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
protocol/client.go (2)
129-133: Remove unused fqTopOrigin varThe variable fqTopOrigin is declared but never set; it only leaks into error messages as an empty string.
- var ( - fqTopOrigin string - possibleTopOrigins []string - ) + var possibleTopOrigins []string
145-149: Fix incorrect error details for topOrigin and slice formatting
- Received should report c.TopOrigin (not the unused fqTopOrigin).
- Use strings.Join (or %v) to format the slice.
- if !IsOriginInHaystack(c.TopOrigin, possibleTopOrigins) { + if !IsOriginInHaystack(c.TopOrigin, possibleTopOrigins) { return ErrVerification. WithDetails("Error validating top origin"). - WithInfo(fmt.Sprintf("Expected Values: %s, Received: %s", possibleTopOrigins, fqTopOrigin)) + WithInfo(fmt.Sprintf("Expected Values: %s, Received: %s", strings.Join(possibleTopOrigins, ", "), c.TopOrigin)) }
♻️ Duplicate comments (2)
protocol/client.go (2)
112-116: Fix log formatting for slices and keep messages readableUse strings.Join (or %v) for []string; %s prints the slice’s pointer. Also keep the received value as-is.
- WithInfo(fmt.Sprintf("Expected Values: %s, Received: %s", rpOrigins, c.Origin)) + WithInfo(fmt.Sprintf("Expected Values: %s, Received: %s", strings.Join(rpOrigins, ", "), c.Origin))
198-233: Restore correct “origin” semantics: compare scheme://host[:port] only; ignore path/query/fragment/userinfoCurrent logic equates entire URLs (path, query, fragment, userinfo), which regresses from HTML origin semantics and prior FullyQualifiedOrigin behavior. Origin per HTML is scheme://host[:port]. For native identifiers, support exact-string matching.
-// IsOriginInHaystack checks if the needle is in the haystack using the mechanism to determine origin equality defined -// in HTML5 Section 5.3 and RFC3986 Section 6.2.1. -// -// Specifically if the needle value includes the '://' character sequence and can be parsed as URL's; -// we check each item in the haystack to see if it matches the same rules, and then if the scheme and host components -// match case-insensitively and check all other components with simple string comparison. The query arguments have a -// special match condition where we check if the values are equal in length, then iteratively check if each key value -// pair are matches. This allows the query arguments to have a different order. -// -// If the needle value does not include the '://' character sequence or can't be parsed as a URL equality is determined -// using simple string comparison. -// -// See (Origin Definition): https://www.w3.org/TR/2011/WD-html5-20110525/origin-0.html -// -// See (Simple String Comparison Definition): https://datatracker.ietf.org/doc/html/rfc3986#section-6.2.1 -func IsOriginInHaystack(needle string, haystack []string) bool { - needleURI := parseNormalizedURI(needle) - - if needleURI != nil { - for _, hay := range haystack { - if hayURI := parseNormalizedURI(hay); hayURI != nil { - if isOriginEqual(needleURI, hayURI) { - return true - } - } - } - } else { - for _, hay := range haystack { - if needle == hay { - return true - } - } - } - - return false -} +// IsOriginInHaystack checks whether needle matches any allowed origins. +// Web origins (http/https) are compared using FullyQualifiedOrigin (scheme://host[:port]) case-insensitively. +// Non-web/native identifiers fall back to exact string equality. +func IsOriginInHaystack(needle string, haystack []string) bool { + nFQ, nErr := FullyQualifiedOrigin(needle) + nWeb := nErr == nil && strings.Contains(nFQ, "://") + + for _, hay := range haystack { + hFQ, hErr := FullyQualifiedOrigin(hay) + hWeb := hErr == nil && strings.Contains(hFQ, "://") + + // Compare normalized web origins (ignore path/query/fragment/userinfo) + if nWeb && hWeb { + if strings.EqualFold(nFQ, hFQ) { + return true + } + continue + } + + // Fallback for native identifiers or non-web tokens + if needle == hay { + return true + } + } + return false +}If you prefer, I can follow up with a clean-up patch removing the now-unneeded helpers below.
🧹 Nitpick comments (2)
protocol/client.go (1)
235-306: Remove over-complicated and incorrect URL component comparators
- isOriginEqual enforces path/query/fragment/userinfo equality, which is not part of origin semantics.
- isURLValuesEqual uses Values.Get and loses multi-value semantics.
- parseNormalizedURI ignores parse errors and may proceed with partial URLs.
These are now unnecessary if IsOriginInHaystack uses FullyQualifiedOrigin.
-// remove isOriginEqual, isURLUserEqual, isURLValuesEqual, parseNormalizedURI helpers -// (entire blocks from lines 235-306 and 308-331)If you want to keep a helper, retain a minimal isOriginEqual that only compares scheme+host (incl. port) case-insensitively.
Also applies to: 308-331
protocol/client_test.go (1)
208-216: Quiet gitleaks false positives for test fixturesThe android:apk-key-hash strings are test fixtures, not secrets. Add an allow rule or inline allow comment to avoid noisy scans.
Options:
- Inline comment before the block:
// gitleaks:allow test fixtures for android key hashes (non-secret)
- Or add an allowlist rule in the repo’s gitleaks config to ignore these test paths/patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
protocol/client.go(3 hunks)protocol/client_test.go(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.27.2)
protocol/client_test.go
209-209: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
210-210: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
215-215: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
216-216: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
james-d-elliott
force-pushed
the
feat-origin-native
branch
from
9c9d7d8 to
e11cd00
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
protocol/client_test.go (1)
220-229: Rename misnamed tests: these are userinfo cases, not non-FQDNThese cases validate ignoring userinfo, not FQDN-ness. This mirrors earlier feedback.
Apply this minimal rename for clarity:
- "ShouldHandleNonFQDNOrigin", + "ShouldIgnoreUserInfoForOriginComparison", @@ - "ShouldHandleNonFQDNOriginExactStringMatch", + "ShouldHandleUserInfoExactStringMatch",
🧹 Nitpick comments (3)
protocol/client_test.go (3)
148-152: Confirm default-port equivalence semantics for originsMany implementations consider https://host and https://host:443 same-origin (likewise http:80). Your test asserts inequality. If your intent is spec-aligned equivalence, add coverage and adjust isOriginEqual accordingly.
Proposed additional tests (if equivalence is desired):
{ "ShouldHandleFullyQualifiedOriginWithPort", "https://app.example.com:443", []string{"https://app.example.com:443"}, true, }, + { + "ShouldTreatDefaultHTTPSPortAsEqual", + "https://app.example.com", + []string{"https://app.example.com:443"}, + true, + }, + { + "ShouldTreatDefaultHTTPPortAsEqual", + "http://app.example.com:80", + []string{"http://app.example.com"}, + true, + }, { "ShouldHandleFullyQualifiedOriginDifferentScheme", "http://app.example.com", []string{"https://app.example.com"}, false, },If inequality is intentional, consider adding a brief comment in code/tests to document that non-normalized default ports are considered different. Would you like me to open a follow-up to codify this?
196-200: Add empty-slice haystack caseYou cover nil haystack; also add []string{} to avoid nil-vs-empty edge regressions.
{ "ShouldHandleFullyQualifiedOriginWithoutAllowed", "https://app.example.com", nil, false, }, + { + "ShouldHandleFullyQualifiedOriginWithEmptyAllowed", + "https://app.example.com", + []string{}, + false, + },
117-231: Optional: add an iOS native-origin sample for parityTo complement Android, consider adding an iOS-style origin example to document exact-match behavior for other native formats as well.
{ "ShouldHandleNativeAppAndroid", "android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0", // gitleaks:allow test fixture []string{"android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0"}, // gitleaks:allow test fixture true, }, + { + "ShouldHandleNativeAppIOS", + "ios:bundle-id:com.example.app", + []string{"ios:bundle-id:com.example.app"}, + true, + }, + { + "ShouldHandleNativeAppIOSCaseSensitive", + "ios:bundle-id:com.example.App", + []string{"ios:bundle-id:com.example.app"}, + false, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
protocol/client.go(3 hunks)protocol/client_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protocol/client.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
protocol/client_test.go (1)
protocol/client.go (1)
IsOriginInHaystack(211-231)
🪛 Gitleaks (8.27.2)
protocol/client_test.go
209-209: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
210-210: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
215-215: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
216-216: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
protocol/client_test.go (3)
154-170: Correctly ignore path and query for web-origin comparisonGood alignment with HTML origin semantics. Treating differing paths and queries as equal origins is correct.
190-194: Correctly ignore fragment for web-origin comparisonFragments are not origin-significant; expecting equality here is correct.
208-218: Silence Gitleaks false positives on test android hash stringsCI scanners flag these as generic API keys. Mark them as test fixtures inline to avoid noise without weakening coverage.
{ "ShouldHandleNativeAppAndroid", - "android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0", - []string{"android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0"}, + "android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0", // gitleaks:allow test fixture + []string{"android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0"}, // gitleaks:allow test fixture true, }, { "ShouldHandleNativeAppAndroidCaseSensitive", - "android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69F0", - []string{"android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0"}, + "android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69F0", // gitleaks:allow test fixture + []string{"android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0"}, // gitleaks:allow test fixture false, },Alternatively, we can change the hex payload to a clearly fake value (e.g., 40 'a's) and add a brief comment.
james-d-elliott
force-pushed
the
feat-origin-native
branch
from
e11cd00 to
0628c40
Compare
james-d-elliott
force-pushed
the
feat-origin-native
branch
from
0628c40 to
85e2bba
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
webauthn/types_test.go (1)
49-55: Avoid asserting full parse error textThe expected string for bad RPID includes the exact parse message from net/url. Even with the loop change above, consider trimming tc.err to a stable substring like "field 'RPID' is not a valid URI".
protocol/client_test.go (3)
196-200: Add coverage for empty allowed origins (not just nil)You cover nil; also cover empty slice to ensure behavior is consistent.
{ "ShouldHandleFullyQualifiedOriginWithoutAllowed", "https://app.example.com", nil, false, }, + { + "ShouldHandleFullyQualifiedOriginWithEmptyAllowed", + "https://app.example.com", + []string{}, + false, + },
219-231: Rename misleading test names (they’re FQDNs with userinfo, not non-FQDN)Clarify names to reflect userinfo-ignoring behavior and exact-match variant.
- { - "ShouldHandleNonFQDNOrigin", + { + "ShouldIgnoreUserInfoForOriginComparison", "https://user:[email protected]/", []string{"https://app.example.com/"}, true, }, - { - "ShouldHandleNonFQDNOriginExactStringMatch", + { + "ShouldHandleFullyQualifiedOriginExactMatchWithCredentials", "https://user:[email protected]/", []string{"https://user:[email protected]/"}, true, },
209-216: Gitleaks false positive: allowlist Android key-hash test constantsThe Android apk key-hash values trigger generic key detection. They are test constants, not secrets. Consider a repo-level gitleaks allowlist.
Example gitleaks config snippet:
# .gitleaks.toml [allowlist] description = "Allowlist android apk key-hash test constants" regexes = [ '''android:apk-key-hash:[0-9a-f]{40}''' ]Alternatively, split the hash in tests (e.g., join parts at runtime) to avoid matching.
webauthn/types.go (2)
36-41: Clarify origin matching semantics (port normalization; ignore path/query/fragment/userinfo)Make the comment explicit about:
- For http/https: compare scheme and host case-insensitively AND port (normalizing defaults), ignoring path, query, fragment, and userinfo.
- Otherwise: exact string comparison.
-// RPOrigins configures the list of Relying Party Server Origins that are permitted. The provided origins can either -// be fully qualified origins or strings for simple string comparison. The strings are matched using canonical -// origin matching semantics specifically if they start with 'http://' or 'https://' if the provided origin has a -// case-insensitive equal scheme and host component they are equal, otherwise simple string comparison is utilized -// to determine equality. +// RPOrigins configures the list of permitted RP origins. Entries may be fully-qualified web origins or arbitrary +// strings. Matching semantics: +// - For http/https: compare scheme and host case-insensitively and compare ports with default normalization +// (https→443, http→80). Ignore path, query, fragment, and userinfo. +// - For all other schemes: use exact, case-sensitive string comparison.
43-48: Mirror the clarified semantics for RPTopOriginsKeep RPTopOrigins documentation consistent with RPOrigins.
-// RPTopOrigins configures the list of Relying Party Server Top Origins that are permitted. The provided origins can -// either be fully qualified origins or strings for simple string comparison. The strings are matched using -// canonical origin matching semantics specifically if they start with 'http://' or 'https://' if the provided -// origin has a case-insensitive equal scheme and host component they are equal, otherwise simple string comparison -// is utilized to determine equality. +// RPTopOrigins configures the list of permitted RP top-level origins. Semantics are identical to RPOrigins: +// - For http/https: scheme+host case-insensitive, port compared with default normalization; path, query, +// fragment, userinfo ignored. +// - For other schemes: exact, case-sensitive string comparison.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
protocol/client.go(3 hunks)protocol/client_test.go(1 hunks)webauthn/types.go(3 hunks)webauthn/types_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protocol/client.go
🧰 Additional context used
🪛 Gitleaks (8.27.2)
protocol/client_test.go
209-209: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
210-210: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
215-215: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
216-216: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (4)
webauthn/types_test.go (1)
3-9: Imports look goodThe added imports are appropriate for the new tests.
protocol/client_test.go (2)
154-194: Good alignment with web origin semantics (path/query/fragment/userinfo ignored)These cases correctly ignore path, query, fragment, and userinfo for HTTP(S) origins. This matches HTML origin semantics.
208-218: LGTM on native app exact matchingCase-sensitive exact string comparison for non-HTTP(S) origins (Android) is appropriate.
webauthn/types.go (1)
133-140: Correct fix for explicit top-origin verification modeValidating that RPTopOrigins is non-empty when RPTopOriginVerificationMode is set to Explicit is correct.
james-d-elliott
force-pushed
the
feat-origin-native
branch
3 times, most recently
from
2f5e852 to
c9d252a
Compare
This enables matching native application origins using simple string comparison. Effective web origins are still matched using case-insensitive matches provided they are valid web origins per HTML5 Section 5.3 (https://www.w3.org/TR/2011/WD-html5-20110525/origin-0.html) and/or simple string comparison per RFC3986 Section 6.2.1 (https://datatracker.ietf.org/doc/html/rfc3986#section-6.2.1). It is important to note that this function completely ignores Apple Associated Domains entirely as Apple is using an unassigned Well-Known URI in breech of Well-Known Uniform Resource Identifiers Section 3 (https://datatracker.ietf.org/doc/html/rfc8615#section-3). Closes #462, Closes #463
james-d-elliott
force-pushed
the
feat-origin-native
branch
from
c9d252a to
49d0578
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #468 +/- ##
==========================================
+ Coverage 41.62% 43.82% +2.20%
==========================================
Files 34 34
Lines 3063 3087 +24
==========================================
+ Hits 1275 1353 +78
+ Misses 1610 1561 -49
+ Partials 178 173 -5
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
protocol/client_test.go (2)
226-236: Rename misnamed tests: these validate userinfo handling, not “NonFQDN”The names suggest non-FQDN, but the URLs are FQDNs with userinfo. Rename for clarity.
- { - "ShouldHandleNonFQDNOrigin", + { + "ShouldIgnoreUserInfoForOriginComparison", "https://user:[email protected]/", []string{"https://app.example.com/"}, true, }, - { - "ShouldHandleNonFQDNOriginExactStringMatch", + { + "ShouldHandleUserInfoExactStringMatch", "https://user:[email protected]/", []string{"https://user:[email protected]/"}, true, },
148-152: Fix “DifferentPort” test to actually test a non-default port (avoid duplicate with default-port cases)This test currently duplicates the default-port equivalence (also covered at Lines 238-242). Make it a non-default port to assert inequality.
- { - "ShouldHandleFullyQualifiedOriginDifferentPort", - "https://app.example.com:443", - []string{"https://app.example.com"}, - true, - }, + { + "ShouldHandleFullyQualifiedOriginDifferentPort", + "https://app.example.com:8443", + []string{"https://app.example.com"}, + false, + },
🧹 Nitpick comments (2)
protocol/client_test.go (2)
214-224: Silence gitleaks false positives in test data (use obvious non-secrets or allowlist)The android:apk-key-hash values trigger “Generic API Key” findings. Since these are fixtures, prefer obviously non-secret placeholders to keep scanners quiet.
{ "ShouldHandleNativeAppAndroid", - "android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0", - []string{"android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0"}, + "android:apk-key-hash:exampleapkkeyhash", + []string{"android:apk-key-hash:exampleapkkeyhash"}, true, }, { "ShouldHandleNativeAppAndroidCaseSensitive", - "android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69F0", - []string{"android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0"}, + "android:apk-key-hash:EXAMPLEAPKKEYHASH", + []string{"android:apk-key-hash:exampleapkkeyhash"}, false, },Alternatively, add an allowlist entry in the gitleaks config for these known-safe test constants.
249-249: Optional: Add a few more cases to tighten coverage (iOS native, empty haystack, reverse default-port)
- iOS native origins should be simple string-compare like Android; add positive and case-sensitive negative.
- Test empty haystack (distinct from nil).
- Add reverse direction for default-port equivalence.
Example additions to the table:
{ "ShouldHandleNativeAppiOS", "ios:bundle-id:com.example.app", []string{"ios:bundle-id:com.example.app"}, true, }, { "ShouldHandleNativeAppiOSCaseSensitive", "ios:bundle-id:com.Example.App", []string{"ios:bundle-id:com.example.app"}, false, }, { "ShouldHandleEmptyHaystack", "https://app.example.com", []string{}, false, }, { "ShouldHandleFullyQualifiedOriginDefaultPortEquivalentHTTPSReverse", "https://app.example.com", []string{"https://app.example.com:443"}, true, }, { "ShouldHandleFullyQualifiedOriginDefaultPortEquivalentHTTPReverse", "http://app.example.com", []string{"http://app.example.com:80"}, true, },If helpful, I can push a patch updating the table and names.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
protocol/client.go(3 hunks)protocol/client_test.go(1 hunks)webauthn/types.go(3 hunks)webauthn/types_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- webauthn/types_test.go
- webauthn/types.go
- protocol/client.go
🧰 Additional context used
🪛 Gitleaks (8.27.2)
protocol/client_test.go
215-215: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
216-216: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
221-221: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
222-222: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
protocol/client_test.go (1)
110-249: Good coverage and alignment with HTML origin semanticsThe new table-driven tests for IsOriginInHaystack comprehensively cover scheme/host case-insensitivity, default port normalization, and ignoring path/query/fragment/userinfo for web origins. Nice job.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
protocol/client_test.go (2)
204-356: LGTM: Test coverage aligns with origin semantics (scheme/host/port only) and native app exact-matchThe new IsOriginInHaystack tests correctly ignore path/query/fragment/userinfo for web origins, normalize default ports, and treat native app origins as exact case-sensitive strings. This addresses prior review feedback.
320-324: Rename test for clarity: it’s about userinfo, not non-FQDNThe host is an FQDN; the test asserts userinfo is ignored. Rename to reflect behavior.
- "ShouldHandleNonFQDNOrigin", + "ShouldIgnoreUserInfoForOriginComparison",
🧹 Nitpick comments (10)
protocol/func_test.go (3)
12-12: Mark helper to improve failure locationsCall t.Helper() so failures report the caller’s line, not inside this helper.
-func AssertIsProtocolError(t *testing.T, err error, errType, errDetails, errInfo any) { +func AssertIsProtocolError(t *testing.T, err error, errType, errDetails, errInfo any) { + t.Helper()
17-24: Allow nil to skip specific assertions and reduce brittlenessPermitting nil for errType/errDetails/errInfo lets tests opt-out of individual checks without crafting placeholder values. Add a nil case in each type-switch.
- switch et := errType.(type) { + switch et := errType.(type) { + case nil: + // skip case string: assert.Equal(t, et, e.Type) case *regexp.Regexp: assert.Regexp(t, et, e.Type) default: t.Fatalf("%T is not a known type", errType) } - switch ed := errDetails.(type) { + switch ed := errDetails.(type) { + case nil: + // skip case string: assert.Equal(t, ed, e.Details) case *regexp.Regexp: assert.Regexp(t, ed, e.Details) default: t.Fatalf("%T is not a known type", errDetails) } - switch ed := errInfo.(type) { + switch ed := errInfo.(type) { + case nil: + // skip case string: assert.Equal(t, ed, e.DevInfo) case *regexp.Regexp: assert.Regexp(t, ed, e.DevInfo) default: t.Fatalf("%T is not a known type", errInfo) }Also applies to: 26-33, 35-42
23-24: Clarify fatal messagesInclude the parameter name and allowed types to speed up failures diagnosis.
- t.Fatalf("%T is not a known type", errType) + t.Fatalf("errType: %T is not a supported type (allowed: string, *regexp.Regexp, nil)", errType) ... - t.Fatalf("%T is not a known type", errDetails) + t.Fatalf("errDetails: %T is not a supported type (allowed: string, *regexp.Regexp, nil)", errDetails) ... - t.Fatalf("%T is not a known type", errInfo) + t.Fatalf("errInfo: %T is not a supported type (allowed: string, *regexp.Regexp, nil)", errInfo)Also applies to: 32-33, 41-42
protocol/client_test.go (7)
3-9: Prefer regexp over fmt string for dev info assertions; update importsYou’re using fmt only to build an exact dev-info string which is brittle. Switch to regexp and drop fmt.
-import ( - "fmt" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) +import ( + "regexp" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +)
42-42: Avoid passing []string{""} as allowed top origins when topOrigin is emptyPassing a single empty string can be interpreted as “one allowed origin that is empty”. Prefer nil to state “no allowed top origins provided” in explicit mode, or switch the mode to Implicit/Ignore depending on intent.
- require.NoError(t, ccd.Verify(storedChallenge.String(), ccd.Type, []string{ccd.Origin}, []string{ccd.TopOrigin}, TopOriginExplicitVerificationMode)) + require.NoError(t, ccd.Verify(storedChallenge.String(), ccd.Type, []string{ccd.Origin}, nil, TopOriginExplicitVerificationMode))
86-86: TopOrigin auto mode: align schemes to avoid false positivesIn Auto mode, if top origins are consulted, http vs https should not match. Either ensure the implementation ignores the provided top origins here or align the test data’s scheme to match the actual topOrigin.
- require.NoError(t, ccd.Verify(storedChallenge.String(), ccd.Type, []string{ccd.Origin}, []string{"https://example.com"}, TopOriginAutoVerificationMode)) + require.NoError(t, ccd.Verify(storedChallenge.String(), ccd.Type, []string{ccd.Origin}, []string{"http://example.com"}, TopOriginAutoVerificationMode))
111-111: Use a regexp for dev-info to reduce brittlenessThe debug text format may change; assert shape instead of exact string.
- AssertIsProtocolError(t, ccd.Verify(bogusChallenge.String(), ccd.Type, []string{ccd.Origin}, []string{ccd.TopOrigin}, TopOriginExplicitVerificationMode), "verification_error", "Error validating challenge", fmt.Sprintf("Expected b Value: \"%s\"\nReceived b: \"%s\"\n", bogusChallenge.String(), newChallenge.String())) + re := regexp.MustCompile(`^Expected b Value: ".+"\nReceived b: ".+"\n$`) + AssertIsProtocolError(t, ccd.Verify(bogusChallenge.String(), ccd.Type, []string{ccd.Origin}, []string{ccd.TopOrigin}, TopOriginExplicitVerificationMode), "verification_error", "Error validating challenge", re)
149-151: Be consistent and precise with error assertionsUse AssertIsProtocolError to document the expected type/details and verify debug info shape. This prevents silent acceptance of wrong error types/messages.
- if err = ccd.Verify(storedChallenge.String(), ccd.Type, []string{ccd.TopOrigin}, expectedOrigins, TopOriginExplicitVerificationMode); err == nil { - t.Fatalf("error expected but not received. expected %#v got %#v", expectedOrigins, ccd.Origin) - } + err = ccd.Verify(storedChallenge.String(), ccd.Type, []string{ccd.TopOrigin}, expectedOrigins, TopOriginExplicitVerificationMode) + AssertIsProtocolError(t, err, "verification_error", "Error validating topOrigin", regexp.MustCompile(`(?s)Expected.*Received.*`))
326-330: Rename test for clarity: exact-match with userinfoClarify that this case expects exact string matching including userinfo.
- "ShouldHandleNonFQDNOriginExactStringMatch", + "ShouldMatchUserInfoWithExactStringMatch",
308-316: Gitleaks false-positive: allowlist the android apk-key-hash test dataThese literal values trigger “Generic API Key” in Gitleaks. Add an inline allow comment to prevent pipeline noise.
{ - "ShouldHandleNativeAppAndroid", + // gitleaks:allow + "ShouldHandleNativeAppAndroid", "android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0", []string{"android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0"}, true, }, { - "ShouldHandleNativeAppAndroidCaseSensitive", + // gitleaks:allow + "ShouldHandleNativeAppAndroidCaseSensitive", "android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69F0", []string{"android:apk-key-hash:7d1043473d55bfa90e8530d35801d4e381bc69f0"}, false, },If inline allow isn’t enabled, consider adding these patterns to the repo’s Gitleaks allowlist configuration targeting test files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
protocol/client_test.go(6 hunks)protocol/func_test.go(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.27.2)
protocol/client_test.go
309-309: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
310-310: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
315-315: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
316-316: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
This enables matching native application origins using simple string comparison. Effective web origins are still matched using case-insensitive matches provided they are valid web origins per HTML5 Section 5.3 (https://www.w3.org/TR/2011/WD-html5-20110525/origin-0.html) and/or simple string comparison per RFC3986 Section 6.2.1 (https://datatracker.ietf.org/doc/html/rfc3986#section-6.2.1). It is important to note that this function completely ignores Apple Associated Domains entirely as Apple is using an unassigned Well-Known URI in breech of Well-Known Uniform Resource Identifiers Section 3 (https://datatracker.ietf.org/doc/html/rfc8615#section-3).
Closes #462, Closes #463