Fix nil pointer panic in FinishRegistration when FindByID fails

[Copilot]

FinishRegistration silently discarded the error from Users.FindByID, passing a nil *auth.User into passkeyUser — whose methods dereference u.user.ID, u.user.Email, etc. — causing a guaranteed runtime panic on any transient DB failure.

Changes

• handler/passkey.go — propagate the FindByID error in FinishRegistration, returning HTTP 500 on failure (consistent with BeginRegistration and every other FindByID call in the codebase):

// Before
user, _ := h.Users.FindByID(r.Context(), userID)

// After
user, err := h.Users.FindByID(r.Context(), userID)
if err != nil {
    writeError(r.Context(), w, http.StatusInternalServerError, "failed to fetch user")
    return
}

• handler/passkey_test.go — adds TestPasskey_finishRegistration_findByIDError covering the new error path (asserts HTTP 500 when FindByID returns an error).

Greptile Summary

This PR fixes a nil pointer panic in FinishRegistration by propagating the FindByID error instead of silently discarding it, consistent with BeginRegistration and every other FindByID call in the file. A targeted test covering the new error path is also added.

Confidence Score: 5/5

Safe to merge — the fix is minimal, correct, and consistent with existing patterns; the new test validates the error path directly.

The change is a single-hunk bug fix that eliminates a guaranteed runtime panic. It mirrors the identical error-handling pattern used in BeginRegistration and everywhere else FindByID is called in this file. The test is well-formed and covers exactly the new code path. No P0/P1 issues found.

No files require special attention.

Important Files Changed

Filename	Overview
handler/passkey.go	Single-line fix propagates FindByID error in FinishRegistration, preventing nil dereference on passkeyUser methods; matches existing error-handling pattern throughout the file.
handler/passkey_test.go	Adds TestPasskey_finishRegistration_findByIDError covering the new error path; mock setup is correct and assertion verifies HTTP 500 is returned.

Sequence Diagram

sequenceDiagram
    participant Client
    participant FinishRegistration
    participant ChallengeStore
    participant UserStore
    participant WebAuthn
    participant CredStore

    Client->>FinishRegistration: POST /passkeys/register/finish?session_id=X
    FinishRegistration->>ChallengeStore: GetAndDeleteChallenge(sessionID)
    ChallengeStore-->>FinishRegistration: challengeData / err
    alt challenge missing or expired
        FinishRegistration-->>Client: 400 Bad Request
    end
    FinishRegistration->>UserStore: FindByID(userID)
    UserStore-->>FinishRegistration: user / err
    alt err != nil (NEW PATH)
        FinishRegistration-->>Client: 500 Internal Server Error
    end
    FinishRegistration->>CredStore: ListCredentialsByUser(userID)
    CredStore-->>FinishRegistration: existingCreds
    FinishRegistration->>WebAuthn: FinishRegistration(waUser, sessionData, r)
    WebAuthn-->>FinishRegistration: credential / err
    alt verification failed
        FinishRegistration-->>Client: 400 Bad Request
    end
    FinishRegistration->>CredStore: CreateCredential(...)
    CredStore-->>FinishRegistration: stored / err
    FinishRegistration-->>Client: 201 Created (PasskeyCredentialDTO)

Loading

_{Reviews (1): Last reviewed commit: "Fix nil pointer panic in FinishRegistrat..." | Re-trigger Greptile}

[veverkananobot]

✅ APPROVED

Critical bug fix: prevents nil pointer panic in FinishRegistration on database errors.

The Bug

FinishRegistration was silently discarding the error from Users.FindByID:

// Before (BUG)
user, _ := h.Users.FindByID(r.Context(), userID)

On any transient DB failure, this passes nil into passkeyUser{user: user, ...}, whose methods then dereference u.user.ID, u.user.Email, etc. → guaranteed panic.

The Fix

Error is now properly propagated:

// After (correct)
user, err := h.Users.FindByID(r.Context(), userID)
if err != nil {
    writeError(r.Context(), w, http.StatusInternalServerError, "failed to fetch user")
    return
}

Consistency Verification

✅ Matches BeginRegistration — same function handles FindByID errors identically (error → HTTP 500)
✅ Consistent with codebase — all other FindByID calls in passkey.go already propagate errors correctly
✅ Error message — matches pattern: "failed to fetch user"
✅ HTTP status — HTTP 500 is correct for transient DB failures

Test Coverage

✅ New test TestPasskey_finishRegistration_findByIDError:

• Mocks FindByID to return error ("db unavailable")
• Verifies HTTP 500 response
• Proper WebAuthn config + challenge setup to reach the error path

Impact Assessment

✅ Severity: High (prevents crashes on DB errors)
✅ Risk: Low (straightforward error propagation, consistent with existing patterns)
✅ Scope: Minimal (single error path)
✅ No breaking changes: Error path was previously panic → now returns 500 (correct)

CI Status

✅ Analyze (actions, go) — SUCCESS
✅ CodeQL — SUCCESS
⏳ Go Tests, Greptile Review — in progress

---

Recommendation: Merge. This is a solid, minimal fix for a real crash bug.

[Copilot]

Pull request overview

Fixes a runtime panic in the passkey registration flow by correctly handling Users.FindByID failures in FinishRegistration, and adds a regression test to ensure the error path returns an HTTP 500.

Changes:

• Propagate Users.FindByID errors in PasskeyHandler.FinishRegistration and return HTTP 500 instead of proceeding with a nil user.
• Add a unit test covering the FindByID error case for FinishRegistration.

Show a summary per file

File	Description
handler/passkey.go	Adds error handling around Users.FindByID in FinishRegistration to prevent nil dereference/panic.
handler/passkey_test.go	Adds a regression test asserting HTTP 500 when FindByID fails during FinishRegistration.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 2/2 changed files
• Comments generated: 0