Skip to content

feat: passkey#131

Open
hanakannzashi wants to merge 9 commits into
mainfrom
feat/passkey
Open

feat: passkey#131
hanakannzashi wants to merge 9 commits into
mainfrom
feat/passkey

Conversation

@hanakannzashi

Copy link
Copy Markdown
Member

No description provided.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello @hanakannzashi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the application's authentication capabilities by integrating passkey (WebAuthn) support. It provides a secure and modern authentication method, allowing users to register and log in using their device's built-in authenticators. The changes span across the API, configuration, database, and services layers, ensuring a robust and configurable implementation of this new authentication standard. Additionally, minor security improvements were made to logging practices in the authentication middleware.

Highlights

  • New Feature: Passkey (WebAuthn) Authentication: This pull request introduces comprehensive support for passkey (WebAuthn) authentication, allowing users to register and authenticate using passkeys for enhanced security and convenience. This includes new API endpoints for managing passkeys and integrating with the webauthn-rs library.
  • Database Schema and Repository Updates: New database tables (passkeys and passkey_challenges) have been added to store registered passkeys and temporary challenge states required during WebAuthn ceremonies. Corresponding PostgreSQL repositories (PostgresPasskeyRepository and PostgresPasskeyChallengeRepository) were implemented to manage these new data structures.
  • API Endpoint and Service Integration: New API routes have been created for initiating and completing passkey registration and authentication flows. These routes are integrated into the main application router, with registration requiring prior user authentication. A new PasskeyServiceImpl handles the core logic for WebAuthn operations.
  • Configuration and Dependency Management: A new WebAuthnConfig structure has been added to manage passkey-related settings such as Relying Party ID, origin, name, and timeouts, configurable via environment variables. The project dependencies have been updated to include the webauthn-rs crate and its transitive dependencies.
  • Security Enhancements in Logging: Logging of sensitive information, such as full authorization headers and token hashes, has been removed from authentication middleware to prevent potential exposure of credentials in logs.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@claude

claude Bot commented Jan 27, 2026

Copy link
Copy Markdown

PR Review - Passkey Authentication Implementation

I've analyzed this PR adding passkey (WebAuthn) authentication. Here are the CRITICAL issues that need addressing:

⚠️ CRITICAL: Race Condition in Authentication Flow (crates/services/src/auth/passkey.rs:214-232)

The finish_authentication method has a data race vulnerability that could lead to credential replay attacks:

// 1. Challenge consumed and validated (line 186)
let challenge = self.passkey_challenge_repository.consume_challenge(challenge_id).await?;

// 2. Authentication verified (line 204)
let auth_result = self.webauthn.finish_passkey_authentication(&credential, &auth_state)?;

// 3. RACE CONDITION: Passkey update happens AFTER successful auth (line 215-232)
if let Some(rec) = self.passkey_repository.get_by_credential_id(&credential.id).await? {
    let mut passkey: StoredPasskey = rec.passkey;
    passkey.update_credential(&auth_result);  // Updates counter
    self.passkey_repository.update_passkey_and_last_used_at(rec.id, passkey, now).await?;
}

// 4. Session created (line 235) - user is now authenticated

Problem: If the passkey update (step 3) fails silently or takes time, the counter won't be updated, but the user still gets authenticated. An attacker could:

  1. Intercept a valid authentication credential
  2. Replay it before the counter update completes
  3. Both authentications succeed because counter validation happens against stale data

Impact: This violates WebAuthn's signature counter mechanism designed to prevent credential replay attacks. In a distributed/multi-instance deployment, this becomes even more critical.

Fix Required:

// Option 1: Make update mandatory and fail auth if it fails
let rec = self.passkey_repository
    .get_by_credential_id(&credential.id)
    .await?
    .ok_or_else(|| anyhow::anyhow!("Passkey not found after authentication"))?;

if rec.user_id != user_id {
    return Err(anyhow::anyhow!("Credential does not belong to user"));
}

let mut passkey: StoredPasskey = rec.passkey;
passkey.update_credential(&auth_result);
self.passkey_repository
    .update_passkey_and_last_used_at(rec.id, passkey, Utc::now())
    .await?;  // Propagate error - don't ignore update failures

// Option 2: Use database transaction to ensure atomic counter update + session creation

⚠️ CRITICAL: No Transaction Boundaries (crates/database/src/repositories/passkey_repository.rs)

All passkey operations lack transaction boundaries. This is particularly dangerous for:

  1. finish_registration: Creates session without transactional guarantee that passkey was stored
  2. finish_authentication: Updates passkey counter + creates session without atomicity
  3. consume_challenge + passkey operations: Challenge deletion is separate from credential validation

Scenario:

  • User completes registration ceremony
  • Passkey insert succeeds
  • Database connection drops before response
  • User has no session token but passkey exists
  • User cannot re-register (credential already exists) or login (no other credentials)

Fix Required: Wrap multi-step operations in transactions:

// In finish_registration handler
let mut transaction = pool.begin().await?;
// 1. consume_challenge (within txn)
// 2. insert_passkey (within txn)  
// 3. create_session (within txn)
transaction.commit().await?;

⚠️ CRITICAL: Challenge Expiry Check After Consumption (crates/services/src/auth/passkey.rs:118, 190)

Challenges are consumed (deleted) before expiry check:

let challenge = self.passkey_challenge_repository
    .consume_challenge(challenge_id)  // DELETED from DB
    .await?
    .ok_or_else(|| anyhow::anyhow!("Challenge not found"))?;

if challenge.expires_at < Utc::now() {  // Check AFTER deletion
    return Err(anyhow::anyhow!("Challenge expired"));
}

Problem: Expired challenges are permanently deleted even when authentication fails. This prevents:

  1. Proper audit logging of expired challenge attempts
  2. Rate limiting based on expired challenge abuse
  3. Cleanup jobs from removing truly expired challenges (already deleted)

Fix: Check expiry before consuming:

// Add peek method or check expiry in the DELETE query:
DELETE FROM passkey_challenges 
WHERE id = $1 AND expires_at > $2  -- Only delete if not expired
RETURNING ...

⚠️ Production Safety: Missing Timeout on External WebAuthn Library (crates/services/src/auth/passkey.rs)

The webauthn.finish_passkey_authentication() and finish_passkey_registration() calls have no timeouts. If the webauthn-rs library hangs on cryptographic verification:

  • Request threads will block
  • Connection pool could exhaust under load
  • No circuit breaker exists

Recommendation: Wrap in tokio::time::timeout:

tokio::time::timeout(
    Duration::from_secs(5),
    tokio::task::spawn_blocking(move || 
        webauthn.finish_passkey_authentication(&credential, &auth_state)
    )
).await??;

Minor Issues (non-blocking but important):

  1. Missing Index on passkeys.credential_id (V16__add_passkeys.sql:14)

    • credential_id has UNIQUE constraint but is frequently queried in get_by_credential_id
    • UNIQUE constraint creates implicit index, so this is OK, but comment should clarify
  2. No Cleanup Job for Expired Challenges (main.rs)

    • delete_expired is called best-effort during flows but no scheduled cleanup
    • Expired challenges accumulate if users abandon flows
    • Recommendation: Add periodic cleanup task in main.rs
  3. Passkey Update Silently Ignored (passkey.rs:226)

    • let _ = passkey.update_credential(&auth_result); discards return value
    • Should log if update was needed but failed
  4. Error Messages Leak Implementation Details (passkey.rs)

    • Errors like "Challenge does not belong to user" reveal system state to attackers
    • Recommendation: Return generic "Authentication failed" to clients, log specifics server-side

Verdict: ⚠️ CHANGES REQUIRED

Must Fix Before Merge:

  1. Race condition in finish_authentication (credential replay vulnerability)
  2. Transaction boundaries for multi-step operations
  3. Challenge expiry check ordering

Should Fix:
4. Timeout on cryptographic operations

The passkey implementation is architecturally sound and follows WebAuthn best practices structurally, but the identified race condition and transaction issues are security-critical in a production multi-cluster deployment.

Please address these issues and I'll review again. Great work on the comprehensive WebAuthn integration otherwise! 🔐

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces passkey (WebAuthn) authentication, which is a significant security and usability enhancement. The changes involve adding new dependencies, database migrations for storing passkeys and challenges, new service implementations for handling WebAuthn flows, and corresponding API routes and OpenAPI documentation. The middleware logging has also been improved to prevent logging sensitive information.

Overall, the implementation appears well-structured and follows good practices for integrating a new authentication method. However, there are a couple of points regarding security and configurability that warrant attention.

Comment thread Cargo.lock
Comment thread crates/api/Cargo.toml
Comment thread crates/database/src/migrations/sql/V16__add_passkeys.sql
Comment thread crates/database/src/repositories/passkey_repository.rs
Comment thread crates/services/Cargo.toml
Comment thread crates/api/src/middleware/auth.rs
Comment thread crates/api/src/middleware/auth.rs
Comment thread crates/api/src/middleware/auth.rs
Comment thread crates/database/src/repositories/session_repository.rs
Comment thread crates/services/src/auth/passkey.rs Outdated
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.

1 participant