From 57a840735e296e02989262315926fc9a3e3beede Mon Sep 17 00:00:00 2001 From: Divyam Talwar Date: Sat, 4 Jul 2026 03:22:37 +0530 Subject: [PATCH] Keep password recovery generic when email delivery is unavailable 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. --- backend/app/api/routes/login.py | 38 +++--- backend/tests/api/routes/test_login.py | 159 +++++++++++++++++++++++++ 2 files changed, 182 insertions(+), 15 deletions(-) diff --git a/backend/app/api/routes/login.py b/backend/app/api/routes/login.py index 58441e37e9..d5fb9237c2 100644 --- a/backend/app/api/routes/login.py +++ b/backend/app/api/routes/login.py @@ -1,3 +1,4 @@ +import logging from datetime import timedelta from typing import Annotated, Any @@ -18,6 +19,7 @@ ) router = APIRouter(tags=["login"]) +logger = logging.getLogger(__name__) @router.post("/login/access-token") @@ -55,23 +57,29 @@ def recover_password(email: str, session: SessionDep) -> Message: """ Password Recovery """ - user = crud.get_user_by_email(session=session, email=email) - - # Always return the same response to prevent email enumeration attacks - # Only send email if user actually exists - if user: - password_reset_token = generate_password_reset_token(email=email) - email_data = generate_reset_password_email( - email_to=user.email, email=email, token=password_reset_token - ) - send_email( - email_to=user.email, - subject=email_data.subject, - html_content=email_data.html_content, - ) - return Message( + # Always return the same response to prevent email enumeration attacks. + recovery_message = Message( message="If that email is registered, we sent a password recovery link" ) + if not settings.emails_enabled: + logger.warning("Password recovery requested, but email delivery is disabled") + return recovery_message + + user = crud.get_user_by_email(session=session, email=email) + if user: + try: + password_reset_token = generate_password_reset_token(email=email) + email_data = generate_reset_password_email( + email_to=user.email, email=email, token=password_reset_token + ) + send_email( + email_to=user.email, + subject=email_data.subject, + html_content=email_data.html_content, + ) + except Exception: + return recovery_message + return recovery_message @router.post("/reset-password/") diff --git a/backend/tests/api/routes/test_login.py b/backend/tests/api/routes/test_login.py index 96677a25f6..16c2050fc5 100644 --- a/backend/tests/api/routes/test_login.py +++ b/backend/tests/api/routes/test_login.py @@ -1,5 +1,8 @@ +import logging +from types import SimpleNamespace from unittest.mock import patch +import pytest from fastapi.testclient import TestClient from pwdlib.hashers.bcrypt import BcryptHasher from sqlmodel import Session @@ -79,6 +82,162 @@ def test_recovery_password_user_not_exits( } +def test_recovery_password_with_disabled_email_skips_user_lookup_and_email( + client: TestClient, + caplog: pytest.LogCaptureFixture, +) -> None: + email = random_email() + + with caplog.at_level(logging.WARNING, logger="app.api.routes.login"): + with ( + patch("app.api.routes.login.crud.get_user_by_email") as get_user_mock, + patch("app.core.config.settings.SMTP_HOST", None), + patch("app.core.config.settings.EMAILS_FROM_EMAIL", None), + patch("app.api.routes.login.generate_password_reset_token") as token_mock, + patch("app.api.routes.login.generate_reset_password_email") as email_mock, + patch("app.api.routes.login.send_email") as send_email_mock, + ): + r = client.post(f"{settings.API_V1_STR}/password-recovery/{email}") + + assert r.status_code == 200 + assert r.json() == { + "message": "If that email is registered, we sent a password recovery link" + } + get_user_mock.assert_not_called() + token_mock.assert_not_called() + email_mock.assert_not_called() + send_email_mock.assert_not_called() + assert "email delivery is disabled" in caplog.text + assert email not in caplog.text + + +def test_recovery_password_with_existing_user_and_email_enabled( + client: TestClient, +) -> None: + email = random_email() + user = User( + email=email, + hashed_password="not-used", + full_name="Test User", + is_active=True, + is_superuser=False, + ) + + email_data = SimpleNamespace( + subject="Reset password", + html_content="

reset-password

", + ) + + with ( + patch("app.api.routes.login.crud.get_user_by_email", return_value=user), + patch("app.core.config.settings.SMTP_HOST", "smtp.example.com"), + patch("app.core.config.settings.EMAILS_FROM_EMAIL", "test@example.com"), + patch( + "app.api.routes.login.generate_password_reset_token", + return_value="reset-token", + ) as token_mock, + patch( + "app.api.routes.login.generate_reset_password_email", + return_value=email_data, + ) as email_mock, + patch("app.api.routes.login.send_email") as send_email_mock, + ): + r = client.post(f"{settings.API_V1_STR}/password-recovery/{email}") + + assert r.status_code == 200 + assert r.json() == { + "message": "If that email is registered, we sent a password recovery link" + } + token_mock.assert_called_once_with(email=email) + email_mock.assert_called_once_with( + email_to=user.email, + email=email, + token="reset-token", + ) + send_email_mock.assert_called_once_with( + email_to=user.email, + subject=email_data.subject, + html_content=email_data.html_content, + ) + + +def test_recovery_password_with_email_failure_returns_generic_response( + client: TestClient, + caplog: pytest.LogCaptureFixture, +) -> None: + email = random_email() + user = User( + email=email, + hashed_password="not-used", + full_name="Test User", + is_active=True, + is_superuser=False, + ) + + with caplog.at_level(logging.ERROR, logger="app.api.routes.login"): + with ( + patch("app.api.routes.login.crud.get_user_by_email", return_value=user), + patch("app.core.config.settings.SMTP_HOST", "smtp.example.com"), + patch("app.core.config.settings.EMAILS_FROM_EMAIL", "test@example.com"), + patch( + "app.api.routes.login.send_email", + side_effect=RuntimeError("SMTP unavailable"), + ) as send_email_mock, + ): + r = client.post(f"{settings.API_V1_STR}/password-recovery/{email}") + + assert r.status_code == 200 + assert r.json() == { + "message": "If that email is registered, we sent a password recovery link" + } + send_email_mock.assert_called_once() + assert caplog.text == "" + + +def test_recovery_password_with_email_template_failure_returns_generic_response( + client: TestClient, + caplog: pytest.LogCaptureFixture, +) -> None: + email = random_email() + user = User( + email=email, + hashed_password="not-used", + full_name="Test User", + is_active=True, + is_superuser=False, + ) + + with caplog.at_level(logging.ERROR, logger="app.api.routes.login"): + with ( + patch("app.api.routes.login.crud.get_user_by_email", return_value=user), + patch("app.core.config.settings.SMTP_HOST", "smtp.example.com"), + patch("app.core.config.settings.EMAILS_FROM_EMAIL", "test@example.com"), + patch( + "app.api.routes.login.generate_password_reset_token", + return_value="reset-token", + ) as token_mock, + patch( + "app.api.routes.login.generate_reset_password_email", + side_effect=RuntimeError("template unavailable"), + ) as email_mock, + patch("app.api.routes.login.send_email") as send_email_mock, + ): + r = client.post(f"{settings.API_V1_STR}/password-recovery/{email}") + + assert r.status_code == 200 + assert r.json() == { + "message": "If that email is registered, we sent a password recovery link" + } + token_mock.assert_called_once_with(email=email) + email_mock.assert_called_once_with( + email_to=user.email, + email=email, + token="reset-token", + ) + send_email_mock.assert_not_called() + assert caplog.text == "" + + def test_reset_password(client: TestClient, db: Session) -> None: email = random_email() password = random_lower_string()