forgejo_admin/basketball-api
1
0
Fork 1
Code Issues 12 Pull requests 2 Projects Releases Packages Wiki Activity Actions

fix: return HTTP 400 for invalid/expired/used tokens on confirm endpoint #139

Merged
forgejo_admin merged 1 commit from 138-fix-confirm-status-code into main 2026-03-21 21:08:25 +00:00
Conversation 2 Commits 1 Files changed 2 +19 -9
forgejo_admin commented 2026-03-21 20:57:21 +00:00
Owner
Copy link

Summary

  • The password reset confirm endpoint was returning HTTP 200 for client errors (invalid, expired, and used tokens), making it impossible for frontends to distinguish success from failure by status code
  • Changed to return HTTP 400 for these three error paths while preserving the same JSON message body
  • Request endpoint remains 200 for all cases (no user enumeration)

Changes

  • src/basketball_api/routes/password_reset.py: Import JSONResponse; change three error return paths (invalid token, used token, expired token) from PasswordResetResponse(...) (200) to JSONResponse(status_code=400, content={...})
  • tests/test_password_reset.py: Update three test assertions (test_rejects_invalid_token, test_rejects_expired_token, test_rejects_used_token) to expect HTTP 400 instead of 200; update docstrings

Test Plan

  • pytest tests/test_password_reset.py -v -- all 7 tests pass
  • ruff check and ruff format --check -- clean
  • Request endpoint (/request) still returns 200 for all cases (no user enumeration change)
  • Server error paths (Keycloak unavailable) unchanged -- still return 200

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive

Related

## Summary - The password reset confirm endpoint was returning HTTP 200 for client errors (invalid, expired, and used tokens), making it impossible for frontends to distinguish success from failure by status code - Changed to return HTTP 400 for these three error paths while preserving the same JSON message body - Request endpoint remains 200 for all cases (no user enumeration) ## Changes - `src/basketball_api/routes/password_reset.py`: Import `JSONResponse`; change three error return paths (invalid token, used token, expired token) from `PasswordResetResponse(...)` (200) to `JSONResponse(status_code=400, content={...})` - `tests/test_password_reset.py`: Update three test assertions (`test_rejects_invalid_token`, `test_rejects_expired_token`, `test_rejects_used_token`) to expect HTTP 400 instead of 200; update docstrings ## Test Plan - [x] `pytest tests/test_password_reset.py -v` -- all 7 tests pass - [x] `ruff check` and `ruff format --check` -- clean - [x] Request endpoint (`/request`) still returns 200 for all cases (no user enumeration change) - [x] Server error paths (Keycloak unavailable) unchanged -- still return 200 ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes forgejo_admin/basketball-api#138
fix: return HTTP 400 for invalid/expired/used tokens on confirm endpoint
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
fa7eaf4792 
The password reset confirm endpoint was returning HTTP 200 for client
errors (invalid token, expired token, already-used token). This made it
impossible for frontends to distinguish success from failure by status
code. Now returns 400 with the same JSON message body.

Closes #138

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-03-21 20:58:18 +00:00
Author
Owner
Copy link

Self-Review

Reviewed the diff. No issues found.

Verified:

  • Only 2 files changed, +19/-9 lines -- minimal and scoped to the bug
  • Three client-error paths in confirm_password_reset changed from PasswordResetResponse(...) (200) to JSONResponse(status_code=400, ...)
  • Request endpoint unchanged -- still returns 200 for all cases (no user enumeration)
  • Server error paths (Keycloak unavailable) unchanged -- still return 200
  • All 7 tests pass, ruff clean
  • No secrets, no extraneous file changes

Ready for human review.

## Self-Review Reviewed the diff. No issues found. **Verified:** - Only 2 files changed, +19/-9 lines -- minimal and scoped to the bug - Three client-error paths in `confirm_password_reset` changed from `PasswordResetResponse(...)` (200) to `JSONResponse(status_code=400, ...)` - Request endpoint unchanged -- still returns 200 for all cases (no user enumeration) - Server error paths (Keycloak unavailable) unchanged -- still return 200 - All 7 tests pass, ruff clean - No secrets, no extraneous file changes Ready for human review.
forgejo_admin commented 2026-03-21 21:04:34 +00:00
Author
Owner
Copy link

PR #139 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic

Correctness verified:

  • All 3 client-error paths in /confirm now return HTTP 400 via JSONResponse: invalid token, used token, expired token. Message bodies are identical to the previous PasswordResetResponse values -- no frontend breakage.
  • Success path (PasswordResetResponse(message="Your password has been reset successfully.")) still returns 200. Correct.
  • Server-error paths (Keycloak unavailable, admin token failure, user not found, reset failure) still return 200 via PasswordResetResponse. This is intentional -- server-side failures should not be exposed as 4xx to the client.
  • /request endpoint is completely untouched. Still returns 200 for all cases (no user enumeration). Correct.
  • JSONResponse import added cleanly. No unused imports.

FastAPI pattern note: The response_model=PasswordResetResponse on the /confirm endpoint describes the 200 success shape. The JSONResponse returns for error paths bypass the response model, which is standard FastAPI practice for non-200 responses. No issue here.

Test coverage: 7 tests covering all paths -- 3 error paths now assert 400, success path asserts 200, Keycloak failure asserts 200, request endpoint tests assert 200 for both known and unknown emails. Docstrings updated to match.

BLOCKERS

None.

NITS

None. This is a clean, minimal, correct fix.

SOP COMPLIANCE

  • Branch named after issue (138-fix-confirm-status-code references #138)
  • PR body follows template (Summary, Changes, Test Plan, Related)
  • Related references parent issue (Closes forgejo_admin/basketball-api#138)
  • No secrets committed
  • No unnecessary file changes (exactly 2 files, both expected)
  • Commit messages are descriptive

PROCESS OBSERVATIONS

Tight, well-scoped bugfix. Two files changed, +19/-9 lines, all directly relevant. The test plan in the PR body explicitly confirms ruff compliance and that the request endpoint was verified unchanged. Low change failure risk.

VERDICT: APPROVED

## PR #139 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Pydantic **Correctness verified**: - All 3 client-error paths in `/confirm` now return HTTP 400 via `JSONResponse`: invalid token, used token, expired token. Message bodies are identical to the previous `PasswordResetResponse` values -- no frontend breakage. - Success path (`PasswordResetResponse(message="Your password has been reset successfully.")`) still returns 200. Correct. - Server-error paths (Keycloak unavailable, admin token failure, user not found, reset failure) still return 200 via `PasswordResetResponse`. This is intentional -- server-side failures should not be exposed as 4xx to the client. - `/request` endpoint is completely untouched. Still returns 200 for all cases (no user enumeration). Correct. - `JSONResponse` import added cleanly. No unused imports. **FastAPI pattern note**: The `response_model=PasswordResetResponse` on the `/confirm` endpoint describes the 200 success shape. The `JSONResponse` returns for error paths bypass the response model, which is standard FastAPI practice for non-200 responses. No issue here. **Test coverage**: 7 tests covering all paths -- 3 error paths now assert 400, success path asserts 200, Keycloak failure asserts 200, request endpoint tests assert 200 for both known and unknown emails. Docstrings updated to match. ### BLOCKERS None. ### NITS None. This is a clean, minimal, correct fix. ### SOP COMPLIANCE - [x] Branch named after issue (`138-fix-confirm-status-code` references #138) - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [x] Related references parent issue (`Closes forgejo_admin/basketball-api#138`) - [x] No secrets committed - [x] No unnecessary file changes (exactly 2 files, both expected) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS Tight, well-scoped bugfix. Two files changed, +19/-9 lines, all directly relevant. The test plan in the PR body explicitly confirms ruff compliance and that the request endpoint was verified unchanged. Low change failure risk. ### VERDICT: APPROVED
forgejo_admin deleted branch 138-fix-confirm-status-code 2026-03-21 21:08:25 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
domain:backend

   
domain:devops

   
domain:frontend

   
status:approved

QA passed, awaiting merge approval

  
status:in-progress

Dev agent is actively working

  
status:needs-fix

QA found issues, back to dev

  
status:qa

PR submitted, awaiting QA review

  
type:bug

Bug fix

  
type:devops

Infrastructure/CI/config work

  
type:feature

New functionality

No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo_admin/basketball-api!139
No description provided.