fix(webauthn): fix check for the full allowedCredentials in validateLogin #487
Conversation
WalkthroughMoved per-credential ownership validation into the loop over session.AllowedCredentialIDs so ownership is checked per allowed credential; removed the post-loop ownership check. Added an integration-style test verifying FinishLogin errors when a session includes an allowed credential not owned by the user. No exported/public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant App as Relying Party
participant WA as WebAuthn Service
Browser->>App: send assertion (RawID + response)
App->>WA: FinishLogin(request, session)
note right of WA #DDEBF7: Ownership check moved\ninside loop over AllowedCredentialIDs
WA->>WA: iterate session.AllowedCredentialIDs
WA->>WA: verify ownership of current allowed credential
alt credential not owned
WA-->>App: error (user does not own allowed credential)
App-->>Browser: auth failed
else owned
WA->>WA: check parsedResponse.RawID ∈ AllowedCredentialIDs
alt RawID not allowed
WA-->>App: error "User does not own the credential returned"
App-->>Browser: auth failed
else RawID allowed
WA->>WA: continue assertion verification
WA-->>App: success/failure result
App-->>Browser: auth success/failure
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Please add a test that fails before the change and passes after the change. Also ensure the check itself remains. The user returned must own all of the credential ID's present in the session's allowed credential ID's. This check mitigates potential vulnerabilities at the implementers end which could allow a login when the user is not correctly matched by domain logic.
Change-Id: I5b4370b4a91a92c3c1f9a28d181f88480c0b3798
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #487 +/- ##
==========================================
+ Coverage 43.82% 44.63% +0.80%
==========================================
Files 34 34
Lines 3087 3087
==========================================
+ Hits 1353 1378 +25
+ Misses 1561 1533 -28
- Partials 173 176 +3
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
webauthn/login_test.go (5)
4-8: Replace deprecated ioutil with io.NopCloser; add errors import for stronger assertionsUse io.NopCloser instead of io/ioutil (deprecated since Go 1.16), and import errors to allow errors.As checks in the test below.
import ( - "bytes" - "encoding/base64" - "fmt" - "io/ioutil" - "net/http" + "bytes" + "encoding/base64" + "errors" + "fmt" + "io" + "net/http" "testing"
148-152: Don’t ignore base64 decode errors; validate with require.NoError for deterministic failuresIgnoring decode errors can hide regressions if any test vector changes. Decode explicitly and assert.
- var ( - byteUserHandle, _ = base64.RawURLEncoding.DecodeString(userHandle) - byteID, _ = base64.RawURLEncoding.DecodeString(credentialID) - byteCredentialPubKey, _ = base64.RawURLEncoding.DecodeString("pQMmIAEhWCAoCF-x0dwEhzQo-ABxHIAgr_5WL6cJceREc81oIwFn7iJYIHEHx8ZhBIE42L26-rSC_3l0ZaWEmsHAKyP9rgslApUdAQI") - byteAAGUID, _ = base64.RawURLEncoding.DecodeString("rc4AAjW8xgpkiwsl8fBVAw") - ) + byteUserHandle, err := base64.RawURLEncoding.DecodeString(userHandle) + require.NoError(t, err) + byteID, err := base64.RawURLEncoding.DecodeString(credentialID) + require.NoError(t, err) + byteCredentialPubKey, err := base64.RawURLEncoding.DecodeString("pQMmIAEhWCAoCF-x0dwEhzQo-ABxHIAgr_5WL6cJceREc81oIwFn7iJYIHEHx8ZhBIE42L26-rSC_3l0ZaWEmsHAKyP9rgslApUdAQI") + require.NoError(t, err) + byteAAGUID, err := base64.RawURLEncoding.DecodeString("rc4AAjW8xgpkiwsl8fBVAw") + require.NoError(t, err)
187-201: Prefer http.NewRequest, set Content-Type, use io.NopCloser, and close the bodyConstructing the request via http.NewRequest better mirrors real usage, sets method/URL, and allows setting headers. Also switch to io.NopCloser and ensure the body is closed.
- // build returned response from authenticator - reqBody := ioutil.NopCloser(bytes.NewReader([]byte(fmt.Sprintf(`{ + // build returned response from authenticator + reqBody := io.NopCloser(bytes.NewReader([]byte(fmt.Sprintf(`{ "id":"%[1]s", "rawId":"%[1]s", "type":"public-key", "response":{ "authenticatorData":"dKbqkhPJnC90siSSsyDPQCYqlMGpUKA5fyklC2CEHvBFXJJiGa3OAAI1vMYKZIsLJfHwVQMANwCOw-atj9C0vhWpfWU-whzNjeQS21Lpxfdk_G-omAtffWztpGoErlNOfuXWRqm9Uj9ANJck1p6lAQIDJiABIVggKAhfsdHcBIc0KPgAcRyAIK_-Vi-nCXHkRHPNaCMBZ-4iWCBxB8fGYQSBONi9uvq0gv95dGWlhJrBwCsj_a4LJQKVHQ", "clientDataJSON":"eyJjaGFsbGVuZ2UiOiJFNFBUY0lIX0hmWDFwQzZTaWdrMVNDOU5BbGdlenROMDQzOXZpOHpfYzlrIiwibmV3X2tleXNfbWF5X2JlX2FkZGVkX2hlcmUiOiJkbyBub3QgY29tcGFyZSBjbGllbnREYXRhSlNPTiBhZ2FpbnN0IGEgdGVtcGxhdGUuIFNlZSBodHRwczovL2dvby5nbC95YWJQZXgiLCJvcmlnaW4iOiJodHRwczovL3dlYmF1dGhuLmlvIiwidHlwZSI6IndlYmF1dGhuLmdldCJ9", "signature":"MEUCIBtIVOQxzFYdyWQyxaLR0tik1TnuPhGVhXVSNgFwLmN5AiEAnxXdCq0UeAVGWxOaFcjBZ_mEZoXqNboY5IkQDdlWZYc", "userHandle":"%[2]s" } }`, credentialID, userHandle, )))) - httpReq := &http.Request{Body: reqBody} + httpReq, err := http.NewRequest(http.MethodPost, "https://webauthn.io/login", reqBody) + require.NoError(t, err) + httpReq.Header.Set("Content-Type", "application/json") + defer httpReq.Body.Close()
202-206: Avoid brittle string comparison; assert on protocol.Error type and DetailsComparing err.Error() strings is fragile. Assert the error type and details using errors.As.
- expectedErr := protocol.ErrBadRequest.WithDetails("User does not own all credentials from the allowedCredentialList") - _, err := webauthn.FinishLogin(user, session, httpReq) - if err == nil || err.Error() != expectedErr.Error() { - t.Fatalf("FinishLogin() expected err=%v, got=%v", expectedErr, err) - } + _, err = webauthn.FinishLogin(user, session, httpReq) + require.Error(t, err) + var protoErr *protocol.Error + if errors.As(err, &protoErr) { + assert.Equal(t, protocol.ErrBadRequest.Type, protoErr.Type) + assert.Equal(t, "User does not own all credentials from the allowedCredentialList", protoErr.Details) + } else { + t.Fatalf("FinishLogin() expected protocol.Error, got %T: %v", err, err) + }
187-199: Optional: build the JSON with encoding/json to avoid formatting pitfallsUsing fmt.Sprintf with multi-line JSON is error-prone if fields change. Consider defining a small struct that matches the expected input and json.Marshal it to keep escaping/quoting correct.
If you want, I can provide a follow-up patch that introduces a helper to generate these request bodies from data structs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
webauthn/login.go(1 hunks)webauthn/login_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- webauthn/login.go
🧰 Additional context used
🧬 Code graph analysis (1)
webauthn/login_test.go (5)
webauthn/credential.go (1)
Credential(15-36)webauthn/authenticator.go (1)
Authenticator(7-26)webauthn/types.go (3)
SessionData(209-220)WebAuthn(24-26)Config(29-76)protocol/errors.go (2)
ErrBadRequest(47-50)Error(3-15)webauthn/registration_test.go (1)
TestRegistration_FinishRegistrationFailure(101-120)
🔇 Additional comments (2)
webauthn/login_test.go (2)
171-177: Good negative test shape to catch the original loop bugPlacing a foreign ID ([]byte("test")) before a valid ID in AllowedCredentialIDs mirrors the overwrite bug described in the report and ensures we fail fast on ownership mismatches.
142-147: Clarify intent: ownership check still in place but PR proposes its removalThe test in webauthn/login_test.go (lines 174–205) expects FinishLogin to return ErrBadRequest when session.AllowedCredentialIDs contains an ID not owned by the user, and the implementation in webauthn/login.go (lines 331–346) still enforces this ownership validation. However, the PR title/description indicates that this check will be removed entirely. Please confirm which behavior is intended and adjust accordingly:
- If we keep the ownership validation:
- Update the PR title/summary to remove any mention of dropping this check.
- If we remove the ownership validation:
- Delete the block in webauthn/login.go (lines 331–346) that returns ErrBadRequest for unowned credentials.
- Update or remove the corresponding test in webauthn/login_test.go (lines 174–205) so it no longer expects this error.
I have updated the change to only fixing the bug. Also added a test case for this. Before the change, test case would fail After the change in login.go |
deanyxu
changed the title
2 participants
There is a bug in this check:
if a credential in the middle of
session.AllowedCredentialIDsdoes not exist incredentials,credentialsOwnedwill be set tofalse, andvalidateLoginis supposed to return the error "User does not own all credentials from the allowedCredentialList". However, since the outer loop continues, if the last credential insession.AllowedCredentialIDsexist incredentials,credentialsOwnedwill be overwritten toTrue.One option is to fix this, but I'd suggest that we remove this check entirely since
session.AllowedCredentialIDsandcredentialsare constructed on the server side