domain: expired/invalid password-reset token surfaces as 500 Unknown instead of link-expired #54

New issue

Closed

opened 2026-07-03 01:41:45 +00:00 by rosa · 1 comment

rosa commented

2026-07-03 01:41:45 +00:00

Owner

Found during a code review of the domain crate.

Location: crates/domain/src/services.rs:380 (reset_password)
Severity: Correctness / UX

Problem

reset_password wraps Confirmation::verify_nouser failures in .with_context(...)?, converting ConfirmationError::Expired and verifier-mismatch into ResetPasswordError::Unknown via #[from] anyhow::Error. The dedicated ConfirmationExpired/ConfirmationVerifierMismatch variants are never constructed, and the web layer renders Unknown as a 500.

Failure scenario

A user clicks a reset link more than 30 minutes after requesting it and gets a generic 500 page instead of a 400 link-expired message, with no way to distinguish an expired link from a real fault.

Suggested fix

Map verify_nouser errors to the existing dedicated variants instead of funnelling through anyhow context, and render them as 4xx.

Found during a code review of the `domain` crate. **Location:** `crates/domain/src/services.rs:380` (`reset_password`) **Severity:** Correctness / UX ## Problem `reset_password` wraps `Confirmation::verify_nouser` failures in `.with_context(...)?`, converting `ConfirmationError::Expired` and verifier-mismatch into `ResetPasswordError::Unknown` via `#[from] anyhow::Error`. The dedicated `ConfirmationExpired`/`ConfirmationVerifierMismatch` variants are never constructed, and the web layer renders `Unknown` as a 500. ## Failure scenario A user clicks a reset link more than 30 minutes after requesting it and gets a generic 500 page instead of a 400 link-expired message, with no way to distinguish an expired link from a real fault. ## Suggested fix Map `verify_nouser` errors to the existing dedicated variants instead of funnelling through anyhow context, and render them as 4xx.

rosa added the

needs-triage

label

2026-07-03 01:43:54 +00:00

rosa commented

2026-07-03 20:55:18 +00:00

Author

Owner

This was generated by AI during triage.

Verified during triage by code inspection: the reset flow wraps Confirmation::verify_nouser failures in an anyhow context, which funnels every verification failure into ResetPasswordError::Unknown. The dedicated ConfirmationExpired and ConfirmationVerifierMismatch variants are constructed nowhere in the codebase, while the web error mapping already routes them to the invalid-confirmation (4xx) response — the plumbing exists on both ends and has simply never been connected.

Agent Brief

Category: bug
Summary: Map password-reset token verification failures to their dedicated error variants so users see a link-expired message instead of a 500

Current behavior:
reset_password calls Confirmation::verify_nouser and wraps any failure in an anyhow context, converting ConfirmationError::Expired and the verifier-mismatch case into ResetPasswordError::Unknown. The web layer renders Unknown as an internal error (500). The dedicated ResetPasswordError::ConfirmationExpired and ResetPasswordError::ConfirmationVerifierMismatch variants exist and the web layer already maps them to the invalid-confirmation response, but nothing ever constructs them. A user clicking a reset link after the 30-minute window gets a generic 500 page.

Desired behavior:
Verification failures map to their specific variants: an expired confirmation becomes ConfirmationExpired, a verifier mismatch becomes ConfirmationVerifierMismatch, and each renders through the existing invalid-confirmation (4xx) path with its user-facing message. Only genuine faults (repository errors, hashing failures) remain Unknown/500. No web-layer mapping changes should be needed — the From<ResetPasswordError> for AppError arms already exist.

Key interfaces:

The ConfirmationError → ResetPasswordError conversion in reset_password — replace the anyhow-context funnel with a per-variant mapping (a From impl or explicit map_err).
Check get_confirmation_by_token (used to render the reset form) for consistency: the form-rendering path should treat an expired/mismatched token the same way the submission path does, so the user doesn't see a form they can't submit.

Acceptance criteria:

Submitting a password reset with an expired token produces ResetPasswordError::ConfirmationExpired and renders as the invalid-confirmation response (4xx), not a 500.
A verifier mismatch produces ResetPasswordError::ConfirmationVerifierMismatch and renders as 4xx.
A unit test covers the mapping for each ConfirmationError variant reachable from verify_nouser.
Genuine repository failures still surface as Unknown (500).
The ci mise task passes.

Out of scope:

Changing the 30-minute confirmation TTL.
The email-confirmation flow (confirm_email_address) — it returns ConfirmationError directly and is a separate surface.
Wording/design of the invalid-confirmation page.

> *This was generated by AI during triage.* Verified during triage by code inspection: the reset flow wraps `Confirmation::verify_nouser` failures in an anyhow context, which funnels every verification failure into `ResetPasswordError::Unknown`. The dedicated `ConfirmationExpired` and `ConfirmationVerifierMismatch` variants are constructed nowhere in the codebase, while the web error mapping already routes them to the invalid-confirmation (4xx) response — the plumbing exists on both ends and has simply never been connected. ## Agent Brief **Category:** bug **Summary:** Map password-reset token verification failures to their dedicated error variants so users see a link-expired message instead of a 500 **Current behavior:** `reset_password` calls `Confirmation::verify_nouser` and wraps any failure in an anyhow context, converting `ConfirmationError::Expired` and the verifier-mismatch case into `ResetPasswordError::Unknown`. The web layer renders `Unknown` as an internal error (500). The dedicated `ResetPasswordError::ConfirmationExpired` and `ResetPasswordError::ConfirmationVerifierMismatch` variants exist and the web layer already maps them to the invalid-confirmation response, but nothing ever constructs them. A user clicking a reset link after the 30-minute window gets a generic 500 page. **Desired behavior:** Verification failures map to their specific variants: an expired confirmation becomes `ConfirmationExpired`, a verifier mismatch becomes `ConfirmationVerifierMismatch`, and each renders through the existing invalid-confirmation (4xx) path with its user-facing message. Only genuine faults (repository errors, hashing failures) remain `Unknown`/500. No web-layer mapping changes should be needed — the `From<ResetPasswordError> for AppError` arms already exist. **Key interfaces:** - The `ConfirmationError` → `ResetPasswordError` conversion in `reset_password` — replace the anyhow-context funnel with a per-variant mapping (a `From` impl or explicit `map_err`). - Check `get_confirmation_by_token` (used to render the reset form) for consistency: the form-rendering path should treat an expired/mismatched token the same way the submission path does, so the user doesn't see a form they can't submit. **Acceptance criteria:** - [ ] Submitting a password reset with an expired token produces `ResetPasswordError::ConfirmationExpired` and renders as the invalid-confirmation response (4xx), not a 500. - [ ] A verifier mismatch produces `ResetPasswordError::ConfirmationVerifierMismatch` and renders as 4xx. - [ ] A unit test covers the mapping for each `ConfirmationError` variant reachable from `verify_nouser`. - [ ] Genuine repository failures still surface as `Unknown` (500). - [ ] The `ci` mise task passes. **Out of scope:** - Changing the 30-minute confirmation TTL. - The email-confirmation flow (`confirm_email_address`) — it returns `ConfirmationError` directly and is a separate surface. - Wording/design of the invalid-confirmation page.

rosa referenced this issue

2026-07-03 20:55:18 +00:00

domain: password-reset job retries mint additional valid reset tokens #59

rosa added

bug

ready-for-agent

and removed

needs-triage

labels

2026-07-03 20:55:31 +00:00

rosa referenced this issue from a commit

2026-07-03 21:12:10 +00:00