Skip to content

fix: passkey security - #1931

Merged
seefs001 merged 1 commit into
QuantumNous:mainfrom
seefs001:feature/passkey
Sep 30, 2025
Merged

fix: passkey security#1931
seefs001 merged 1 commit into
QuantumNous:mainfrom
seefs001:feature/passkey

Conversation

@seefs001

@seefs001 seefs001 commented Sep 30, 2025

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • UI

    • Consolidated user actions (Reset Passkey, Reset 2FA, Logout) into a single “More” dropdown in the Users table.
  • API Changes

    • Admin reset passkey endpoint updated to /:id/reset_passkey.
    • Frontend now calls the new reset passkey path.
    • Passkey status response reduced to essential fields for leaner payloads.
  • Bug Fixes

    • Improved handling of certain user handle inputs during passkey login to reduce unexpected failures.

@coderabbitai

coderabbitai Bot commented Sep 30, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Backend passkey controller trims response fields and relaxes userHandle parsing in login finish. Admin API DELETE path renamed to /:id/reset_passkey. Frontend users table consolidates actions into a Dropdown menu. Frontend hook updates reset passkey endpoint to match new route.

Changes

Cohort / File(s) Summary
Backend: Passkey controller
controller/passkey.go
Simplifies PasskeyStatus payload by removing credential metadata fields; retains last_used_at and enabled. Adjusts PasskeyLoginFinish to attempt non-fatal userHandle parse, enforcing equality only when parse succeeds; logs on parse failure.
Backend: API routing
router/api-router.go
Changes admin DELETE route from /api/user/:id/passkey to /api/user/:id/reset_passkey; handler unchanged.
Frontend: Users table UI actions
web/src/components/table/users/UsersColumnDefs.jsx
Replaces individual Reset Passkey/Reset 2FA/Logout buttons with a Dropdown menu (IconMore trigger); introduces moreMenu items and divider; imports Dropdown and IconMore. Other actions unchanged.
Frontend: Users data hook
web/src/hooks/users/useUsersData.jsx
Updates reset passkey API endpoint to /api/user/${user.id}/reset_passkey; logic and UX flow unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Client
  participant S as Server (PasskeyLoginFinish)
  participant L as Logger

  C->>S: POST /passkey/login/finish (userHandle, assertion)
  rect rgba(200,230,255,0.25)
    note over S: UserHandle validation (relaxed parsing)
    S->>S: Try parse userHandle as numeric
    alt Parse succeeds
      S->>S: Compare parsed ID == expected user ID
      alt IDs match
        S-->>C: Proceed to verify assertion
      else IDs mismatch
        S-->>C: Reject (user mismatch)
      end
    else Parse fails
      S->>L: Log parse failure (non-fatal)
      S-->>C: Proceed to verify assertion
    end
  end
  S-->>C: Return auth result
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • feat: passkey #1912 — Earlier changes adding/modifying passkey controller functions; this PR updates the same areas (status payload, login finish validation).

Suggested reviewers

  • Calcium-Ion

Poem

A hop, a bop—new routes align,
The keys now whisper, slim and fine.
A dropdown blooms where buttons were,
One click—reset, confirm, transfer.
I thump the ground: “Flow’s now neat!” 🥕
Fewer crumbs, more tidy feet.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title “fix: passkey security” is related to the changes affecting passkey handling but is too generic to convey the specific updates made, such as endpoint renaming, data shape reduction, validation logic adjustments, and UI dropdown integration. It does not clearly summarize the primary change or scope of the pull request. Consider renaming the pull request to explicitly reflect the main change, for example “refactor passkey reset endpoint and streamline passkey payload,” so that reviewers immediately understand the scope and intent of the updates.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d026edc and e71407e.

📒 Files selected for processing (4)
  • controller/passkey.go (2 hunks)
  • router/api-router.go (1 hunks)
  • web/src/components/table/users/UsersColumnDefs.jsx (3 hunks)
  • web/src/hooks/users/useUsersData.jsx (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
web/src/components/table/users/UsersColumnDefs.jsx (1)
web/src/components/table/users/UsersTable.jsx (3)
  • showResetPasskeyModal (62-62)
  • showResetTwoFAModal (63-63)
  • showDeleteModal (59-59)
controller/passkey.go (1)
common/sys_log.go (1)
  • SysLog (11-14)
router/api-router.go (1)
controller/passkey.go (1)
  • AdminResetPasskey (329-363)
🔇 Additional comments (6)
controller/passkey.go (2)

191-194: LGTM - Reduced information disclosure.

The response payload has been trimmed to only include enabled and last_used_at, removing credential-specific fields. This reduces the attack surface by not exposing sensitive credential metadata (e.g., AAGUID, credential IDs) to clients.


276-284: userHandle parse-failure fallback aligns with WebAuthn spec
The WebAuthn specification treats userHandle as an opaque byte sequence and your credentialID lookup already provides the authoritative user binding, so logging and skipping the numeric parse on non-numeric handles has no security impact.

router/api-router.go (1)

102-102: LGTM - Improved route semantics.

The route has been renamed from /:id/passkey to /:id/reset_passkey, which better reflects the operation (reset) and aligns with the handler function name AdminResetPasskey. The previous DELETE on /passkey was ambiguous between deletion and reset.

web/src/hooks/users/useUsersData.jsx (1)

162-162: LGTM - Frontend endpoint updated to match backend route.

The API endpoint has been updated from /api/user/${user.id}/passkey to /api/user/${user.id}/reset_passkey, correctly aligning with the backend route change in router/api-router.go (line 102).

web/src/components/table/users/UsersColumnDefs.jsx (2)

29-31: LGTM - Required imports for dropdown menu.

Added Dropdown component and IconMore icon to support the consolidated actions menu.


218-292: LGTM - Improved UX with consolidated actions menu.

The individual action buttons (Reset Passkey, Reset 2FA, Logout) have been consolidated into a dropdown menu accessed via an IconMore button. This improves the UI by:

  • Reducing visual clutter
  • Grouping related administrative actions
  • Using a divider to separate safety actions from the destructive "Logout" action
  • Following common UI patterns for "more actions"

The action handlers remain properly connected to the existing modal callbacks (showResetPasskeyModal, showResetTwoFAModal, showDeleteModal).


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.

❤️ Share
🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

  • Public repositories are always opted into early access features.
  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

Comment @coderabbitai help to get the list of available commands and usage tips.

@seefs001
seefs001 merged commit 7533ffc into QuantumNous:main Sep 30, 2025
1 check passed
x22x22 pushed a commit to x22x22/new-api that referenced this pull request Apr 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants