feat(webauthn): add partial unsupported json v2 compat #327
Conversation
WalkthroughThe changes made in the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Outside diff range and nitpick comments (1)
webauthn/login.go (1)
218-220: LGTM: Improved validation check for UserHandleThe change from nil check to length check is consistent with the previous change and handles both nil and empty byte slices properly.
Consider making the error message more specific by changing "blank" to "empty or nil" to better reflect the actual check being performed.
- return nil, nil, protocol.ErrBadRequest.WithDetails("Client-side Discoverable Assertion was attempted with a blank User Handle") + return nil, nil, protocol.ErrBadRequest.WithDetails("Client-side Discoverable Assertion was attempted with an empty or nil User Handle")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
webauthn/login.go(1 hunks)webauthn/types.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- webauthn/types.go
🔇 Additional comments (1)
webauthn/login.go (1)
214-216: LGTM: Improved validation check for UserID
The change from nil check to length check is more robust as it handles both nil and empty byte slices consistently, which aligns with the PR's goal of fixing JSON encoding issues.
|
What specific scenario does the omitempty change fix? I wouldn't consider the challenge to be at all compatible with this change as it's arbitrarily required. |
|
A login looks like:
If the SessionData is not just held in memory (e.g. stored in a secure cookie, or shared cache between server instances), it needs to be serialized, which is most naturally done through json. Before this PR, for the field
Unmarshaling This PR makes 2 changes: setting |
|
As for compatibility, this package checks the UserID field with The docs for bytes.Equal note:
|
|
I think we should wait on the change to omit empty since the other change actually fixes the edge case issue. My reasoning is this experimental package is subject to sudden breaking changes without warning, and in addition both the challenge and relying party ID may not be empty for any reason, leaving the only change which is reasonable being the change to the user ID field's tags. |
james-d-elliott
changed the title
james-d-elliott
changed the title
Using the candidate json/v2 encoder https://github.com/go-json-experiment/json SessionData doesn't roundtrip properly through json as an empty []byte is encoded as "". Use len(field) == 0 instead of nil checks to be more resilient.
seankhliao
force-pushed
the
json-marshal
branch
from
7fbd1ee to
33c0a2c
Compare
|
Sure, I've dropped the omitempty change, leaving just the change to check against length instead of nil. |
Using the candidate json/v2 encoder
https://github.com/go-json-experiment/json SessionData doesn't roundtrip properly through json as an empty []byte is encoded as "". Mark compatible fields as omitempty, and use len(field) == 0 instead of nil checks to be more resilient.