Add passkey support.#5872
Open
LxnChan wants to merge 3 commits intousememos:mainfrom
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds WebAuthn/passkey authentication support across the backend and frontend. It includes a new API v1 router for passkey registration and authentication flows, store layer for persisting passkeys, database updates for user settings key handling, React components for passkey management, WebAuthn client utilities, and localization strings. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
web/src/components/Settings/MyAccountSection.tsx (1)
7-7: Use the frontend alias for the new component import.Suggested change
-import PasskeyDialog from "../PasskeyDialog"; +import PasskeyDialog from "@/components/PasskeyDialog";As per coding guidelines,
web/src/**/*.{ts,tsx}should use@/alias for absolute imports in the frontend instead of relative paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Settings/MyAccountSection.tsx` at line 7, The import of PasskeyDialog in MyAccountSection.tsx uses a relative path; update the import to use the frontend alias (@"...") per project conventions — replace the current import of PasskeyDialog with an alias-based import (e.g., import PasskeyDialog from "@/components/PasskeyDialog") so the component reference (PasskeyDialog) remains the same but uses the frontend absolute import style.web/src/components/PasskeyDialog.tsx (1)
23-35: Move passkey server state into a React Query hook.This component owns fetching, mutations, refresh, and local cache updates directly. Please wrap these operations in hooks (for example
usePasskeys) and invalidate/update through React Query instead.As per coding guidelines, “Manage server data in the frontend via React Query hooks located in
hooks/. Manage client state via React Context located incontexts/.”Also applies to: 45-68, 77-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/PasskeyDialog.tsx` around lines 23 - 35, Replace the inline data fetching/mutation logic in PasskeyDialog (the loadPasskeys function and the local state setters setPasskeys/setIsLoading and mutation handlers around lines 23-35, 45-68, 77-92) with a React Query hook (e.g., create usePasskeys in hooks/) that encapsulates listPasskeys, create/update/delete mutations, and refresh logic; implement useQuery inside usePasskeys to fetch passkeys and useMutation for changes, update the React Query cache via queryClient.invalidateQueries or queryClient.setQueryData instead of manually calling setPasskeys, and have the component consume usePasskeys (removing loadPasskeys and local server-state management) while still calling handleError/toast in mutation error handlers as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/router/api/v1/auth_passkey.go`:
- Around line 555-561: The current logic allows a zero authData.SignCount to
overwrite a non-zero stored counter; change the update rule so a zero incoming
counter cannot downgrade an existing non-zero counter. In the auth passkey
handling (use symbols matched, authData, updatedPasskey, SignCount, LastUsedTs,
s.writeNativeError), either reject requests where matched.SignCount > 0 and
authData.SignCount == 0 (return a permission error) or only apply the update
when authData.SignCount > 0 (e.g., set updatedPasskey.SignCount =
authData.SignCount only if authData.SignCount > 0, otherwise keep
matched.SignCount), and always update LastUsedTs as before. Ensure the existing
validation that rejects non-incrementing non-zero counters (authData.SignCount >
0 && authData.SignCount <= matched.SignCount) remains intact.
- Around line 299-302: Change the authenticator selection to require user
verification by setting UserVerification from "preferred" to "required" in the
passkeyAuthenticatorSelectionJSON (and update the same occurrences referenced in
the comment). Additionally, update the server-side assertion verification to
check the "user verified" (UV) flag in the authenticator's assertion response
(where you verify assertions, e.g., VerifyAuthenticatorAssertionResponse / the
assertion-verification handler) and reject logins if UV is not true, ensuring
passwordless discoverable credentials enforce "something you have + user
verification", not just presence.
In `@server/router/api/v1/connect_interceptors.go`:
- Around line 58-60: The interceptor currently calls setMetadataHeader(md,
header, "X-Forwarded-Host", "x-forwarded-host") and setMetadataHeader(md,
header, "X-Forwarded-Proto", "x-forwarded-proto") without verifying they
originated from a trusted proxy, allowing clients to spoof RP resolution; update
the interceptor and/or resolvePasskeyRelyingParty to only accept or use
x-forwarded-host/x-forwarded-proto when the request is from a configured/trusted
proxy: either (A) modify the interceptor to strip client-supplied X-Forwarded-*
headers and only copy forwarded headers when the connection/peer info proves a
trusted proxy, or (B) add a trust check in resolvePasskeyRelyingParty before
reading metadata keys "x-forwarded-host" and "x-forwarded-proto" (e.g., verify
peer address, a proxy-authenticated flag, or an internal header set by a trusted
reverse proxy); ensure origin header from the client remains preferred and
documented fallback to forwarded headers is only allowed after the explicit
proxy-trust validation.
In `@server/router/api/v1/v1.go`:
- Line 125: The passkey routes registered by registerPasskeyRoutes are not being
wrapped by http.MaxBytesHandler like other API handlers; update the registration
so that all routes added by registerPasskeyRoutes are created under the same
gateway group that is wrapped with http.MaxBytesHandler (or explicitly wrap the
passkey route handlers with http.MaxBytesHandler) so POST bodies for endpoints
such as beginPasskeyAuthenticationHandler and finishPasskeyAuthenticationHandler
are size-limited; also add a short comment or entry in acl_config.go listing
which passkey endpoints are intentionally public so the ACL/gRPC whitelist
expectations are clear.
In `@store/passkey_setting.go`:
- Around line 84-158: Current AddUserPasskey, UpdateUserPasskey, and
DeleteUserPasskey read-modify-write the whole JSON blob (via GetUserPasskeys and
upsertUserPasskeys) which is not atomic and allows race conditions; change the
implementation to perform the mutation inside a transactional/locked operation
or use a CAS/row-version check so the read/modify/write is atomic: either wrap
the read + modify + UpsertUserSetting call in a DB transaction/row lock provided
by s.driver (or add a version/etag column and use a conditional update), enforce
the duplicate ID check and signCount update inside that atomic operation, and
update the userSettingCache only after a successful commit; reference functions
to change: AddUserPasskey, UpdateUserPasskey, DeleteUserPasskey,
upsertUserPasskeys, GetUserPasskeys and the call to s.driver.UpsertUserSetting
(or replace with a transactional/conditional API) so concurrent operations
cannot lose or resurrect passkeys.
In `@web/src/components/PasskeyDialog.tsx`:
- Around line 137-139: The delete Button in PasskeyDialog.tsx, which currently
renders only a TrashIcon and calls setDeletingPasskey(passkey), lacks an
accessible name for screen readers; update the Button (the element using
onClick={() => setDeletingPasskey(passkey)}) to provide an accessible label by
adding an aria-label (e.g., aria-label="Delete passkey") or include visually
hidden text next to the TrashIcon so screen readers can announce the action,
ensuring the label uniquely identifies the passkey when needed (e.g., include
passkey identifier in the label if context requires).
In `@web/src/components/PasswordSignInForm.tsx`:
- Around line 43-49: The form submit handler handleFormSubmit calls
handleSignInButtonClick or handlePasskeySignInButtonClick, but the sign-in
buttons also call those handlers via onClick, causing duplicate actions on a
single click; fix by making the sign-in buttons non-submit controls (set their
HTML button elements to type="button") so clicking them only runs their onClick
handler and does not also trigger the form onSubmit, or alternatively remove the
onClick and rely solely on onSubmit — update the button elements that invoke
handleSignInButtonClick and handlePasskeySignInButtonClick (also at the other
occurrence around the 155-166 area) accordingly.
In `@web/src/utils/passkey.ts`:
- Around line 85-97: getPasskeyErrorKey currently only recognizes
NotSupportedError but createPasskey()/signInWithPasskey() throw plain Error with
messages like "unsupported" for unsupported browsers, so update
getPasskeyErrorKey to detect those plain-error messages (e.g.
message.includes("unsupported") or message.includes("not supported") or specific
"unsupported-passkey" text) and map them to the same NotSupportedError return
value (use the same branch that returns the unsupported/availability key based
on action in getPasskeyErrorKey); apply the same text-match mapping in all other
occurrences that perform passkey error normalization so unsupported plain Errors
are handled consistently.
---
Nitpick comments:
In `@web/src/components/PasskeyDialog.tsx`:
- Around line 23-35: Replace the inline data fetching/mutation logic in
PasskeyDialog (the loadPasskeys function and the local state setters
setPasskeys/setIsLoading and mutation handlers around lines 23-35, 45-68, 77-92)
with a React Query hook (e.g., create usePasskeys in hooks/) that encapsulates
listPasskeys, create/update/delete mutations, and refresh logic; implement
useQuery inside usePasskeys to fetch passkeys and useMutation for changes,
update the React Query cache via queryClient.invalidateQueries or
queryClient.setQueryData instead of manually calling setPasskeys, and have the
component consume usePasskeys (removing loadPasskeys and local server-state
management) while still calling handleError/toast in mutation error handlers as
needed.
In `@web/src/components/Settings/MyAccountSection.tsx`:
- Line 7: The import of PasskeyDialog in MyAccountSection.tsx uses a relative
path; update the import to use the frontend alias (@"...") per project
conventions — replace the current import of PasskeyDialog with an alias-based
import (e.g., import PasskeyDialog from "@/components/PasskeyDialog") so the
component reference (PasskeyDialog) remains the same but uses the frontend
absolute import style.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 873e6f7d-d719-4ddb-ba80-e0c58db8cfb8
⛔ Files ignored due to path filters (1)
server/router/frontend/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (16)
server/router/api/v1/auth_passkey.goserver/router/api/v1/connect_interceptors.goserver/router/api/v1/v1.gostore/db/mysql/user_setting.gostore/db/postgres/user_setting.gostore/db/sqlite/user_setting.gostore/passkey_setting.gostore/user_setting.goweb/src/components/PasskeyDialog.tsxweb/src/components/PasswordSignInForm.tsxweb/src/components/Settings/MyAccountSection.tsxweb/src/locales/en-GB.jsonweb/src/locales/en.jsonweb/src/locales/zh-Hans.jsonweb/src/pages/SignIn.tsxweb/src/utils/passkey.ts
Comment on lines
+299
to
+302
| AuthenticatorSelection: &passkeyAuthenticatorSelectionJSON{ | ||
| ResidentKey: "preferred", | ||
| UserVerification: "preferred", | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WebAuthn passwordless authentication require userVerification required and verify UV flag in authenticator data
💡 Result:
Yes, for WebAuthn passwordless authentication using userVerification: 'required', the server must verify that the UV (User Verification) flag is set to true in the authenticator data of the assertion response. When userVerification is set to 'required', the authenticator is mandated to perform user verification (biometric, PIN, etc.), and if successful, sets the UV flag (bit 2) to 1/true in the flags byte of the authenticator data. The server validates this by parsing the authenticator data after signature verification and checking the UV flag. If the flag is false, authentication should fail as it indicates user verification was not performed despite being required. This applies to both registration (via authenticatorSelection.userVerification) and authentication (via PublicKeyCredentialRequestOptions.userVerification). For passwordless flows with discoverable credentials (passkeys), this ensures "something you have + something you are/know" security.
Citations:
- 1: https://web.dev/articles/webauthn-user-verification?authuser=2
- 2: https://w3.org/TR/webauthn-3
- 3: https://webauthn.passwordless.id/authentication/
- 4: https://developer.mozilla.org/docs/Web/API/Web_Authentication_API/Authenticator_data
- 5: https://web.dev/articles/webauthn-user-verification
🏁 Script executed:
# First, check if the file exists and get its structure
find . -name "auth_passkey.go" -type fRepository: usememos/memos
Length of output: 96
🏁 Script executed:
# Get the total line count to understand file size
wc -l server/router/api/v1/auth_passkey.goRepository: usememos/memos
Length of output: 99
🏁 Script executed:
# Read the specific sections mentioned in the review (299-302, 455-460, 844-858)
echo "=== Lines 299-302 ===" && sed -n '299,302p' server/router/api/v1/auth_passkey.go
echo ""
echo "=== Lines 455-460 ===" && sed -n '455,460p' server/router/api/v1/auth_passkey.go
echo ""
echo "=== Lines 844-858 ===" && sed -n '844,858p' server/router/api/v1/auth_passkey.goRepository: usememos/memos
Length of output: 1204
🏁 Script executed:
# Check the entire context around these sections to understand the functions they're in
echo "=== Context for 299-302 (5 lines before and after) ===" && sed -n '294,307p' server/router/api/v1/auth_passkey.go
echo ""
echo "=== Context for 455-460 (5 lines before and after) ===" && sed -n '450,465p' server/router/api/v1/auth_passkey.go
echo ""
echo "=== Context for 844-858 (5 lines before and after) ===" && sed -n '839,863p' server/router/api/v1/auth_passkey.goRepository: usememos/memos
Length of output: 2210
🏁 Script executed:
# Search for UserVerification settings and UV flag checks in the entire file
echo "=== All UserVerification occurrences ===" && grep -n "UserVerification" server/router/api/v1/auth_passkey.go
echo ""
echo "=== All UV flag check occurrences ===" && grep -n "Flags&0x04\|Flags & 0x04" server/router/api/v1/auth_passkey.goRepository: usememos/memos
Length of output: 370
🏁 Script executed:
# Let's understand the function context for each location
echo "=== Function containing lines 299-302 ===" && sed -n '280,310p' server/router/api/v1/auth_passkey.go | head -20
echo ""
echo "=== Function containing lines 455-460 ===" && sed -n '430,470p' server/router/api/v1/auth_passkey.go | head -30
echo ""
# Search for where validatePasskeyAuthData is called to understand if it's used for both registration and assertion
echo "=== Calls to validatePasskeyAuthData ===" && grep -n "validatePasskeyAuthData" server/router/api/v1/auth_passkey.goRepository: usememos/memos
Length of output: 2285
🏁 Script executed:
# Check if these are passwordless/registration and assertion flows by looking at function names
grep -n "^func.*\(registration\|assertion\|passwordless\|passkey\)" server/router/api/v1/auth_passkey.go | head -20Repository: usememos/memos
Length of output: 642
🏁 Script executed:
# Let's see the flow: where validatePasskeyAuthData is called in the assertion flow
# Find the function that contains the call to validatePasskeyAuthData
grep -B 30 "validatePasskeyAuthData" server/router/api/v1/auth_passkey.go | grep -E "^func|validatePasskeyAuthData"Repository: usememos/memos
Length of output: 495
🏁 Script executed:
# Let's find the function names that contain the registration and authentication flows
echo "=== Registration flow (lines 299-302 context) ===" && sed -n '250,305p' server/router/api/v1/auth_passkey.go | grep -E "^func|BeginPasskeyRegistration"
echo ""
echo "=== Authentication flow (lines 455-460 context) ===" && sed -n '400,465p' server/router/api/v1/auth_passkey.go | grep -E "^func|BeginPasskeyAuthentication"Repository: usememos/memos
Length of output: 322
🏁 Script executed:
# Get the full function signatures for better understanding
echo "=== Line 365 context (registration validatePasskeyAuthData call) ===" && sed -n '350,375p' server/router/api/v1/auth_passkey.go
echo ""
echo "=== Line 529 context (authentication validatePasskeyAuthData call) ===" && sed -n '515,540p' server/router/api/v1/auth_passkey.goRepository: usememos/memos
Length of output: 2371
🏁 Script executed:
# Check if there's a distinction between passwordless and regular passkey flows
echo "=== Check passkey discovery/resident key settings ===" && grep -n "ResidentKey\|discoverable" server/router/api/v1/auth_passkey.go
echo ""
# Let's look at how passkeys are stored and if they track if they're discoverable
echo "=== Check passkey store model ===" && find . -name "*.go" -type f -exec grep -l "type Passkey" {} \;Repository: usememos/memos
Length of output: 271
🏁 Script executed:
# Look at the entire validatePasskeyAuthData function to confirm it doesn't check UV flag
echo "=== Full validatePasskeyAuthData function ===" && sed -n '844,865p' server/router/api/v1/auth_passkey.goRepository: usememos/memos
Length of output: 1004
🏁 Script executed:
# Check if there's context about whether UserVerification should differ between registration and authentication
echo "=== Check if UserVerification settings differ between registration and authentication ===" && grep -A 2 -B 2 "UserVerification" server/router/api/v1/auth_passkey.go | head -30Repository: usememos/memos
Length of output: 979
🏁 Script executed:
# Let's verify the parsedPasskeyAuthData structure to confirm Flags field exists and understand the bit representation
echo "=== parsedPasskeyAuthData structure ===" && grep -A 10 "type parsedPasskeyAuthData struct" server/router/api/v1/auth_passkey.go
echo ""
# Verify the UV flag bit position (should be 0x04 for bit 2)
echo "=== Understanding UV flag: bit 2 should be 0x04 ===" && python3 << 'EOF'
# WebAuthn authenticator data flags:
# Bit 0 (0x01): User Present (UP)
# Bit 2 (0x04): User Verified (UV)
# Bit 6 (0x40): Attested Credential Data Included (AT)
# Bit 7 (0x80): Extension Data Included (ED)
print("UV flag position verification:")
print(f" Bit 2 = 0x{1 << 2:02x} (matches proposed 0x04)")
print(f" 0x04 & 0x04 = 0x{0x04 & 0x04:02x} (correctly checks if bit 2 is set)")
EOFRepository: usememos/memos
Length of output: 652
🏁 Script executed:
# Check if there are any other references to UserVerification or passwordless flows that might be affected
echo "=== Other passwordless/registration/authentication context ===" && grep -n "passwordless\|discoverable" server/router/api/v1/auth_passkey.goRepository: usememos/memos
Length of output: 120
Require user verification for passwordless passkey sign-in.
The flow currently accepts "preferred" verification which only checks user presence. Since registration enables discoverable credentials (passwordless), an attacker with a stolen security key that only requires touch could complete authentication. Require UV to enforce "something you have + something you are/know" security, and validate the UV flag in the server-side check.
🔐 Proposed fix
AuthenticatorSelection: &passkeyAuthenticatorSelectionJSON{
ResidentKey: "preferred",
- UserVerification: "preferred",
+ UserVerification: "required",
}, Challenge: challenge,
RPID: rp.ID,
Timeout: int((60 * time.Second) / time.Millisecond),
- UserVerification: "preferred",
+ UserVerification: "required",
AllowCredentials: buildPasskeyCredentialDescriptors(passkeys), if authData.Flags&0x01 == 0 {
return status.Errorf(codes.InvalidArgument, "user presence is required")
}
+ if authData.Flags&0x04 == 0 {
+ return status.Errorf(codes.PermissionDenied, "user verification is required")
+ }
if requireAttestedCredential && len(authData.CredentialID) == 0 {Also applies to: 455-460, 844-858
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/router/api/v1/auth_passkey.go` around lines 299 - 302, Change the
authenticator selection to require user verification by setting UserVerification
from "preferred" to "required" in the passkeyAuthenticatorSelectionJSON (and
update the same occurrences referenced in the comment). Additionally, update the
server-side assertion verification to check the "user verified" (UV) flag in the
authenticator's assertion response (where you verify assertions, e.g.,
VerifyAuthenticatorAssertionResponse / the assertion-verification handler) and
reject logins if UV is not true, ensuring passwordless discoverable credentials
enforce "something you have + user verification", not just presence.
Comment on lines
+555
to
+561
| if matched.SignCount > 0 && authData.SignCount > 0 && authData.SignCount <= matched.SignCount { | ||
| return s.writeNativeError(c, status.Errorf(codes.PermissionDenied, "passkey sign count is invalid")) | ||
| } | ||
|
|
||
| updatedPasskey := *matched | ||
| updatedPasskey.SignCount = authData.SignCount | ||
| updatedPasskey.LastUsedTs = time.Now().Unix() |
Contributor
There was a problem hiding this comment.
Do not let a zero sign counter disable clone detection.
If a stored counter is already non-zero, accepting authData.SignCount == 0 and writing it back erases the counter baseline. Preserve/reject that regression instead of downgrading the stored credential.
🔐 Proposed fix
- if matched.SignCount > 0 && authData.SignCount > 0 && authData.SignCount <= matched.SignCount {
- return s.writeNativeError(c, status.Errorf(codes.PermissionDenied, "passkey sign count is invalid"))
+ if matched.SignCount > 0 {
+ if authData.SignCount == 0 || authData.SignCount <= matched.SignCount {
+ return s.writeNativeError(c, status.Errorf(codes.PermissionDenied, "passkey sign count is invalid"))
+ }
}
updatedPasskey := *matched
- updatedPasskey.SignCount = authData.SignCount
+ if authData.SignCount > 0 {
+ updatedPasskey.SignCount = authData.SignCount
+ }
updatedPasskey.LastUsedTs = time.Now().Unix()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/router/api/v1/auth_passkey.go` around lines 555 - 561, The current
logic allows a zero authData.SignCount to overwrite a non-zero stored counter;
change the update rule so a zero incoming counter cannot downgrade an existing
non-zero counter. In the auth passkey handling (use symbols matched, authData,
updatedPasskey, SignCount, LastUsedTs, s.writeNativeError), either reject
requests where matched.SignCount > 0 and authData.SignCount == 0 (return a
permission error) or only apply the update when authData.SignCount > 0 (e.g.,
set updatedPasskey.SignCount = authData.SignCount only if authData.SignCount >
0, otherwise keep matched.SignCount), and always update LastUsedTs as before.
Ensure the existing validation that rejects non-incrementing non-zero counters
(authData.SignCount > 0 && authData.SignCount <= matched.SignCount) remains
intact.
Comment on lines
+58
to
+60
| setMetadataHeader(md, header, "Origin", "origin") | ||
| setMetadataHeader(md, header, "X-Forwarded-Host", "x-forwarded-host") | ||
| setMetadataHeader(md, header, "X-Forwarded-Proto", "x-forwarded-proto") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where passkey RP/origin values are derived from and whether forwarded
# headers are gated by a trusted-proxy check.
rg -n -C4 'x-forwarded-host|x-forwarded-proto|metadata\.FromIncomingContext|Origin|Relying|RP|rpID|rpID|registerPasskeyRoutes' server/router/api/v1 store serverRepository: usememos/memos
Length of output: 50371
X-Forwarded-Host and X-Forwarded-Proto are not validated against a trusted proxy boundary.
In resolvePasskeyRelyingParty() (line 638), the code reads x-forwarded-host and x-forwarded-proto from metadata (lines 659–660) as a fallback when the origin header is unavailable. These are set in metadata by the interceptor without any check that they came from a trusted proxy. An unauthenticated client can inject or spoof these headers, causing the RP ID and origin validation to fail during WebAuthn flows.
While the code prioritizes the browser-set origin header when present (lines 646–657), non-browser clients (e.g., mobile apps using Connect RPC) may not include it, forcing reliance on the spoofable forwarded headers. Ensure that reverse proxy configuration strips untrusted X-Forwarded-* headers from clients, or add explicit proxy trust validation before consuming these headers for RP resolution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/router/api/v1/connect_interceptors.go` around lines 58 - 60, The
interceptor currently calls setMetadataHeader(md, header, "X-Forwarded-Host",
"x-forwarded-host") and setMetadataHeader(md, header, "X-Forwarded-Proto",
"x-forwarded-proto") without verifying they originated from a trusted proxy,
allowing clients to spoof RP resolution; update the interceptor and/or
resolvePasskeyRelyingParty to only accept or use
x-forwarded-host/x-forwarded-proto when the request is from a configured/trusted
proxy: either (A) modify the interceptor to strip client-supplied X-Forwarded-*
headers and only copy forwarded headers when the connection/peer info proves a
trusted proxy, or (B) add a trust check in resolvePasskeyRelyingParty before
reading metadata keys "x-forwarded-host" and "x-forwarded-proto" (e.g., verify
peer address, a proxy-authenticated flag, or an internal header set by a trusted
reverse proxy); ensure origin header from the client remains preferred and
documented fallback to forwarded headers is only allowed after the explicit
proxy-trust validation.
| gwGroup.Use(middleware.CORSWithConfig(middleware.CORSConfig{ | ||
| AllowOrigins: []string{"*"}, | ||
| })) | ||
| s.registerPasskeyRoutes(gwGroup) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify passkey routes enforce request limits and public endpoints are whitelisted.
rg -n -C4 'registerPasskeyRoutes|BodyLimit|MaxBytesHandler|PublicMethods|IsPublicMethod|passkey' server/router/api/v1Repository: usememos/memos
Length of output: 48065
Wrap passkey routes with request size limit protection.
The passkey routes bypass the http.MaxBytesHandler wrapping applied to other API handlers at lines 128 and 155. Since registerPasskeyRoutes registers plain Echo routes before the wrapped gateway handler, requests to /api/v1/auth/passkeys* endpoints have no body size limit enforced.
The ACL whitelisting system in acl_config.go applies only to gRPC methods (format /memos.api.v1.Service/Method), not HTTP routes. Public passkey endpoints (e.g., beginPasskeyAuthenticationHandler, finishPasskeyAuthenticationHandler) already handle authentication at the handler level, but consider documenting which endpoints are public and ensure all routes are wrapped with http.MaxBytesHandler to match the protection applied to other API routes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/router/api/v1/v1.go` at line 125, The passkey routes registered by
registerPasskeyRoutes are not being wrapped by http.MaxBytesHandler like other
API handlers; update the registration so that all routes added by
registerPasskeyRoutes are created under the same gateway group that is wrapped
with http.MaxBytesHandler (or explicitly wrap the passkey route handlers with
http.MaxBytesHandler) so POST bodies for endpoints such as
beginPasskeyAuthenticationHandler and finishPasskeyAuthenticationHandler are
size-limited; also add a short comment or entry in acl_config.go listing which
passkey endpoints are intentionally public so the ACL/gRPC whitelist
expectations are clear.
Comment on lines
+84
to
+158
| // AddUserPasskey stores a new passkey for the user. | ||
| func (s *Store) AddUserPasskey(ctx context.Context, userID int32, passkey *Passkey) error { | ||
| passkeys, err := s.GetUserPasskeys(ctx, userID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| passkeys = append(passkeys, clonePasskey(passkey)) | ||
| return s.upsertUserPasskeys(ctx, userID, passkeys) | ||
| } | ||
|
|
||
| // UpdateUserPasskey updates an existing passkey for the user. | ||
| func (s *Store) UpdateUserPasskey(ctx context.Context, userID int32, passkey *Passkey) error { | ||
| passkeys, err := s.GetUserPasskeys(ctx, userID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| updated := false | ||
| for i, existing := range passkeys { | ||
| if existing.ID == passkey.ID { | ||
| passkeys[i] = clonePasskey(passkey) | ||
| updated = true | ||
| break | ||
| } | ||
| } | ||
| if !updated { | ||
| return errors.Errorf("passkey %s not found", passkey.ID) | ||
| } | ||
|
|
||
| return s.upsertUserPasskeys(ctx, userID, passkeys) | ||
| } | ||
|
|
||
| // DeleteUserPasskey deletes a passkey for the user. | ||
| func (s *Store) DeleteUserPasskey(ctx context.Context, userID int32, passkeyID string) error { | ||
| passkeys, err := s.GetUserPasskeys(ctx, userID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| filtered := make([]*Passkey, 0, len(passkeys)) | ||
| deleted := false | ||
| for _, existing := range passkeys { | ||
| if existing.ID == passkeyID { | ||
| deleted = true | ||
| continue | ||
| } | ||
| filtered = append(filtered, clonePasskey(existing)) | ||
| } | ||
| if !deleted { | ||
| return errors.Errorf("passkey %s not found", passkeyID) | ||
| } | ||
|
|
||
| return s.upsertUserPasskeys(ctx, userID, filtered) | ||
| } | ||
|
|
||
| func (s *Store) upsertUserPasskeys(ctx context.Context, userID int32, passkeys []*Passkey) error { | ||
| value, err := json.Marshal(&passkeysUserSetting{ | ||
| Passkeys: clonePasskeys(passkeys), | ||
| }) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to marshal passkeys") | ||
| } | ||
|
|
||
| if _, err := s.driver.UpsertUserSetting(ctx, &UserSetting{ | ||
| UserID: userID, | ||
| Key: UserSettingKeyPasskeys, | ||
| Value: string(value), | ||
| }); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| s.userSettingCache.Set(ctx, getUserSettingCacheKey(userID, userSettingKeyPasskeysString), clonePasskeys(passkeys)) | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Make passkey mutations atomic.
AddUserPasskey, UpdateUserPasskey, and DeleteUserPasskey read the full JSON list and then overwrite it. Concurrent registration/authentication/delete flows can lose entries, resurrect deleted passkeys, or regress SignCount; the duplicate check also needs to be enforced inside the same atomic operation.
Please move this behind a transaction/row lock/versioned CAS, or store passkeys as credential rows with uniqueness on credential ID.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@store/passkey_setting.go` around lines 84 - 158, Current AddUserPasskey,
UpdateUserPasskey, and DeleteUserPasskey read-modify-write the whole JSON blob
(via GetUserPasskeys and upsertUserPasskeys) which is not atomic and allows race
conditions; change the implementation to perform the mutation inside a
transactional/locked operation or use a CAS/row-version check so the
read/modify/write is atomic: either wrap the read + modify + UpsertUserSetting
call in a DB transaction/row lock provided by s.driver (or add a version/etag
column and use a conditional update), enforce the duplicate ID check and
signCount update inside that atomic operation, and update the userSettingCache
only after a successful commit; reference functions to change: AddUserPasskey,
UpdateUserPasskey, DeleteUserPasskey, upsertUserPasskeys, GetUserPasskeys and
the call to s.driver.UpsertUserSetting (or replace with a
transactional/conditional API) so concurrent operations cannot lose or resurrect
passkeys.
Comment on lines
+137
to
+139
| <Button variant="ghost" size="sm" onClick={() => setDeletingPasskey(passkey)}> | ||
| <TrashIcon className="h-4 w-4 text-destructive" /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Add an accessible name to the delete button.
The button only contains an SVG icon, so screen readers do not get a meaningful action label.
♿ Proposed fix
- <Button variant="ghost" size="sm" onClick={() => setDeletingPasskey(passkey)}>
+ <Button
+ variant="ghost"
+ size="sm"
+ aria-label={t("setting.account.passkey-deletion", { label: passkey.label })}
+ onClick={() => setDeletingPasskey(passkey)}
+ >
<TrashIcon className="h-4 w-4 text-destructive" />
</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Button variant="ghost" size="sm" onClick={() => setDeletingPasskey(passkey)}> | |
| <TrashIcon className="h-4 w-4 text-destructive" /> | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| aria-label={t("setting.account.passkey-deletion", { label: passkey.label })} | |
| onClick={() => setDeletingPasskey(passkey)} | |
| > | |
| <TrashIcon className="h-4 w-4 text-destructive" /> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/PasskeyDialog.tsx` around lines 137 - 139, The delete
Button in PasskeyDialog.tsx, which currently renders only a TrashIcon and calls
setDeletingPasskey(passkey), lacks an accessible name for screen readers; update
the Button (the element using onClick={() => setDeletingPasskey(passkey)}) to
provide an accessible label by adding an aria-label (e.g., aria-label="Delete
passkey") or include visually hidden text next to the TrashIcon so screen
readers can announce the action, ensuring the label uniquely identifies the
passkey when needed (e.g., include passkey identifier in the label if context
requires).
Comment on lines
43
to
+49
| const handleFormSubmit = (e: React.FormEvent<HTMLFormElement>) => { | ||
| e.preventDefault(); | ||
| handleSignInButtonClick(); | ||
| if (allowPasswordAuth) { | ||
| handleSignInButtonClick(); | ||
| } else { | ||
| handlePasskeySignInButtonClick(); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid starting sign-in twice from the same click.
The submit buttons also call the same handlers via onClick; a click can run onClick and then the form onSubmit, potentially issuing duplicate password requests or opening duplicate WebAuthn prompts.
🐛 Proposed fix
{allowPasswordAuth && (
- <Button type="submit" className="w-full h-10" disabled={actionBtnLoadingState.isLoading} onClick={handleSignInButtonClick}>
+ <Button type="submit" className="w-full h-10" disabled={actionBtnLoadingState.isLoading}>
{t("common.sign-in")}
{actionBtnLoadingState.isLoading && <LoaderIcon className="w-5 h-auto ml-2 animate-spin opacity-60" />}
</Button>
)}
<Button
type={allowPasswordAuth ? "button" : "submit"}
className="w-full h-10"
variant="outline"
disabled={passkeyBtnLoadingState.isLoading || !supportsPasskeys()}
- onClick={handlePasskeySignInButtonClick}
+ onClick={allowPasswordAuth ? handlePasskeySignInButtonClick : undefined}
>Also applies to: 155-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/PasswordSignInForm.tsx` around lines 43 - 49, The form
submit handler handleFormSubmit calls handleSignInButtonClick or
handlePasskeySignInButtonClick, but the sign-in buttons also call those handlers
via onClick, causing duplicate actions on a single click; fix by making the
sign-in buttons non-submit controls (set their HTML button elements to
type="button") so clicking them only runs their onClick handler and does not
also trigger the form onSubmit, or alternatively remove the onClick and rely
solely on onSubmit — update the button elements that invoke
handleSignInButtonClick and handlePasskeySignInButtonClick (also at the other
occurrence around the 155-166 area) accordingly.
Comment on lines
+85
to
+97
| const message = error instanceof Error ? error.message.toLowerCase() : typeof error === "string" ? error.toLowerCase() : ""; | ||
| if (message.includes("timed out or was not allowed") || message.includes("cancelled")) { | ||
| return action === "create" ? "auth.passkey-create-cancelled" : "auth.passkey-sign-in-cancelled"; | ||
| } | ||
| if (message.includes("passkey already exists")) { | ||
| return "auth.passkey-already-exists"; | ||
| } | ||
| if (message.includes("sign in is not available")) { | ||
| return "auth.passkey-sign-in-unavailable"; | ||
| } | ||
| if (message.includes("security")) { | ||
| return "auth.passkey-security-error"; | ||
| } |
Contributor
There was a problem hiding this comment.
Map the explicit unsupported-passkey error.
createPasskey() and signInWithPasskey() throw a plain Error for unsupported browsers, but getPasskeyErrorKey() only catches NotSupportedError, so some paths fall back to a generic sign-in failure.
💬 Proposed fix
if (message.includes("sign in is not available")) {
return "auth.passkey-sign-in-unavailable";
}
+ if (message.includes("does not support passkeys") || message.includes("does not support passkey")) {
+ return "auth.passkey-unsupported";
+ }
if (message.includes("security")) {
return "auth.passkey-security-error";
}Also applies to: 195-196, 261-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/utils/passkey.ts` around lines 85 - 97, getPasskeyErrorKey currently
only recognizes NotSupportedError but createPasskey()/signInWithPasskey() throw
plain Error with messages like "unsupported" for unsupported browsers, so update
getPasskeyErrorKey to detect those plain-error messages (e.g.
message.includes("unsupported") or message.includes("not supported") or specific
"unsupported-passkey" text) and map them to the same NotSupportedError return
value (use the same branch that returns the unsupported/availability key based
on action in getPasskeyErrorKey); apply the same text-match mapping in all other
occurrences that perform passkey error normalization so unsupported plain Errors
are handled consistently.
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.
Add passkey support, users can login with passkey now.