-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(webauthn): add WithChallenge login option #359
feat(webauthn): add WithChallenge login option #359
Conversation
This option customizes the challenge sent to the client. Can be used as a form of document signing method. Closes #353
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
webauthn/login.go (1)
174-185
: Consider early length validation in WithChallenge.
While the 16-byte minimum length check is handled in beginLogin, you might also validate it here to provide immediate user feedback and possibly reduce error-handling overhead downstream.func WithChallenge(challenge []byte) LoginOption { + if len(challenge) < 16 { + panic("challenge must be at least 16 bytes") // or return an error-based option + } return func(cco *protocol.PublicKeyCredentialRequestOptions) { cco.Challenge = challenge } }🧰 Tools
🪛 golangci-lint (1.62.2)
177-177: File is not
goimports
-ed with -local github.com/go-webauthn/webauthn(goimports)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
webauthn/login.go
(3 hunks)webauthn/login_test.go
(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
webauthn/login.go
177-177: File is not goimports
-ed with -local github.com/go-webauthn/webauthn
(goimports)
🔇 Additional comments (7)
webauthn/login.go (3)
71-78
: Good job on conditional challenge creation.
Generating a random challenge only when it is missing offers the necessary flexibility for user-provided challenges.
79-82
: Appropriate minimum challenge length enforcement.
Enforcing 16 bytes is in line with recommended security practices for WebAuthn challenges.
99-99
: Storing challenge in session as a string.
Serializing the challenge as a string allows it to be easily passed around, but verify any downstream usage that might require the raw bytes. Overall, this is acceptable for session data.
webauthn/login_test.go (4)
35-40
: Extended test struct with expected challenge.
Introducing 'expectedChallenge' enhances test coverage and clarity, ensuring challenge values are verified precisely.
88-97
: Testing error for short challenge.
Great addition: verifying that providing a challenge shorter than 16 bytes triggers an error ensures compliance with security requirements.
98-108
: Valid challenge test.
Verifying that a valid 32-byte challenge is round-tripped correctly strengthens the correctness of the WithChallenge feature.
129-131
: Challenge validation in test results.
Confirming the returned challenge matches 'expectedChallenge' ensures user-provided challenges are respected and preserved in the final response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This option customizes the challenge sent to the client.
Can be used as a form of document signing method.
Closes #353