Skip to content

fix(password_reset): return 400 on ErrNotFound in ResetPassword#233

Merged
veverkap merged 2 commits into
mainfrom
copilot/fix-password-reset-error-handling
May 11, 2026
Merged

fix(password_reset): return 400 on ErrNotFound in ResetPassword#233
veverkap merged 2 commits into
mainfrom
copilot/fix-password-reset-error-handling

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 10, 2026

Bug Fix

What was the bug?

ResetPassword() was the only handler calling FindByID without checking auth.ErrNotFound — a deleted or non-existent user would yield a 500 instead of a 400, leaking an internal error for what is a client-side invalid token condition.

How did you fix it?

Added the missing ErrNotFound guard before the 500 path, consistent with every other FindByID call site in the codebase:

user, err := h.Users.FindByID(r.Context(), resetToken.UserID)
if err != nil {
    if errors.Is(err, auth.ErrNotFound) {
        writeError(r.Context(), w, http.StatusBadRequest, "invalid or expired reset token")
        return
    }
    slog.ErrorContext(r.Context(), "password reset: lookup user", slog.Any("error", err))
    writeError(r.Context(), w, http.StatusInternalServerError, "internal server error")
    return
}

Testing

Covered by existing test suite (go test ./...).

Greptile Summary

This PR adds a missing ErrNotFound guard in ResetPassword() so that a deleted or non-existent user returns HTTP 400 instead of HTTP 500, consistent with every other FindByID call site in the handler package.

  • handler/password_reset.go: Inserts an errors.Is(err, auth.ErrNotFound) check before the generic 500 path, returning 400 with the same "invalid or expired reset token" message used elsewhere in the handler.
  • handler/password_reset_test.go: Adds TestResetPassword_userDeletedAfterTokenIssued which verifies the 400 response and confirms that neither UpdatePassword nor DeletePasswordResetToken are called when the user no longer exists.

Confidence Score: 5/5

This is a minimal, well-targeted change that closes a gap in error-code handling with no risk of regression to the happy path.

The change is a single guard clause inserted in an already well-covered code path, and the new test directly validates the corrected behavior and absence of unintended side effects.

No files require special attention.

Important Files Changed

Filename Overview
handler/password_reset.go Adds ErrNotFound guard in ResetPassword before the 500 path; slog call already uses ErrorContext with r.Context() per project convention.
handler/password_reset_test.go New test verifies 400 status and absence of side-effect calls when FindByID returns ErrNotFound; mock field names correctly match helper definitions.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Handler as ResetPassword Handler
    participant Resets as ResetStore
    participant Users as UserStore

    Client->>Handler: POST /reset-password
    Handler->>Resets: FindPasswordResetToken(hash)
    Resets-->>Handler: PasswordResetToken or error
    alt invalid or expired token
        Handler-->>Client: 400 Bad Request
    end
    Handler->>Users: FindByID(token.UserID)
    Users-->>Handler: User or error
    alt ErrNotFound (user deleted)
        Handler-->>Client: 400 Bad Request (new guard)
    else other error
        Handler-->>Client: 500 Internal Server Error
    end
    alt OIDC-only account
        Handler-->>Client: 400 Bad Request
    end
    Handler->>Users: UpdatePassword(userID, hash)
    Handler->>Resets: DeletePasswordResetToken(id)
    Handler-->>Client: 200 OK
Loading

Reviews (2): Last reviewed commit: "test: add test for FindByID returning Er..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes password reset confirmation behavior to avoid returning HTTP 500 when the reset token points to a deleted/non-existent user, aligning the response with other invalid-token conditions.

Changes:

  • Handle auth.ErrNotFound from Users.FindByID in ResetPassword() by returning HTTP 400 "invalid or expired reset token" instead of HTTP 500.
  • Keep logging/500 behavior for unexpected user store errors.
Show a summary per file
File Description
handler/password_reset.go Adds an ErrNotFound guard in the reset confirmation flow to treat missing users as an invalid/expired token (400).

Copilot's findings

Tip

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

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment thread handler/password_reset.go
Cover the case where a user is deleted after a valid reset token is
issued. Asserts HTTP 400 and that UpdatePassword and
DeletePasswordResetToken are not called.
@veverkap veverkap merged commit 34e72bd into main May 11, 2026
8 checks passed
@veverkap veverkap deleted the copilot/fix-password-reset-error-handling branch May 11, 2026 15:31
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.

3 participants