Keep password recovery generic when email delivery is unavailable#2370
Open
DivyamTalwar wants to merge 1 commit into
Open
Keep password recovery generic when email delivery is unavailable#2370DivyamTalwar wants to merge 1 commit into
DivyamTalwar wants to merge 1 commit into
Conversation
Password recovery returned the same generic response for unknown accounts, but existing accounts could still fail when SMTP was disabled or the reset-email side-effect path raised. This keeps the endpoint response stable, skips the email workflow when delivery is not configured, and avoids adding account-conditioned recovery logs. Constraint: Upstream has no open issue for this and the repo permits focused direct bug-fix PRs. Rejected: Backgrounding password recovery delivery | That is a broader architecture change beyond this focused response-uniformity fix. Confidence: high Scope-risk: narrow Directive: Keep password recovery HTTP responses generic for disabled or failing email delivery. Tested: uv run --no-sync ruff check app/api/routes/login.py tests/api/routes/test_login.py Tested: uv run --no-sync ruff format --check app/api/routes/login.py tests/api/routes/test_login.py Tested: uv run --no-sync pytest tests/api/routes/test_login.py --collect-only -q Tested: temporary postgres:18-alpine + uv run --no-sync pytest tests/api/routes/test_login.py -q Not-tested: Latest full backend workflow-equivalent suite after refinement; rely on GitHub Actions for Compose/mailcatcher/coverage/Playwright surfaces.
Author
|
Thanks! The code/test checks are green; the remaining failed check is the repository label gate requiring one of the release labels. This looks like a bug-fix PR, but I do not have permission to apply labels to the upstream PR. A maintainer can unblock that check by adding the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This keeps
/password-recovery/{email}on the same generic HTTP response when reset email delivery is unavailable.The route now:
send_email()raisesWhy
The endpoint already returns a generic response for unknown emails. Before this change, an existing user could still hit an internal error when SMTP settings were disabled because
send_email()requires email delivery settings to be configured.This avoids status/body-based differences for disabled-email and reset-email failure paths. It does not try to equalize timing between existing and non-existing users, and it does not guarantee that an email was delivered.
Tests
cd backend && uv run --no-sync ruff check app/api/routes/login.py tests/api/routes/test_login.py— passedcd backend && uv run --no-sync ruff format --check app/api/routes/login.py tests/api/routes/test_login.py— passedcd backend && uv run --no-sync pytest tests/api/routes/test_login.py --collect-only -q— passed, collected 13 testscd backend && uv run --no-sync bash scripts/prestart.sh && uv run --no-sync pytest tests/api/routes/test_login.py -q— passed, 13 tests passed with a temporary localpostgres:18-alpineserviceI did not complete the full GitHub Actions-equivalent stack locally; CI should cover the Compose/mailcatcher/coverage/Playwright surfaces.
Notes
No existing issue. I checked the current open upstream issues and PRs for password recovery / SMTP / disabled email /
send_emailduplicates and did not find a matching open item.