From 64a0b6d79ef106fdf5d1590d2e46e277d5f338a4 Mon Sep 17 00:00:00 2001 From: noQbot Date: Sat, 22 Aug 2026 14:19:22 +0530 Subject: [PATCH] Fix API bugs surfaced by live tracing (422/400 instead of 500) Several endpoints returned 500 (or crashed) on inputs that should be rejected with a 4xx. Fixes: - Pagination (items, users): skip/limit were unbounded, so negative values reached Postgres OFFSET/LIMIT and 500'd. Add shared SkipQuery (ge=0) and LimitQuery (ge=1, le=100) in app/api/params.py; out-of-range -> 422. - Private user create (dev-only /private router): email was a bare str, so an invalid address was committed and every later read 500'd on UserPublic (EmailStr). Use EmailStr (max_length=255) and password bounds (8-128) -> 422 before insert; drop the accepted-but-ignored is_verified (no column). - Private user create: duplicate email raised an unhandled UniqueViolation -> 500. Look up first and return 400; create via crud.create_user. - Password recovery HTML endpoint: response header name was "subject:", an invalid HTTP header name that raised RuntimeError. Use "subject". - Password recovery: send the email via BackgroundTasks so the HTTP response no longer waits on SMTP (enumeration-safe uniform response unchanged). Tests added for each case (invalid email, short password, duplicate email, negative skip, negative limit, valid recovery header, background/no-op email). Co-Authored-By: Vinv-AI <309466812+Vinv-AI@users.noreply.github.com> --- backend/app/api/params.py | 9 ++++++ backend/app/api/routes/items.py | 6 +++- backend/app/api/routes/login.py | 16 ++++++---- backend/app/api/routes/private.py | 35 ++++++++++++--------- backend/app/api/routes/users.py | 3 +- backend/tests/api/routes/test_items.py | 10 ++++++ backend/tests/api/routes/test_login.py | 35 +++++++++++++++++++++ backend/tests/api/routes/test_private.py | 40 ++++++++++++++++++++++++ backend/tests/api/routes/test_users.py | 10 ++++++ 9 files changed, 141 insertions(+), 23 deletions(-) create mode 100644 backend/app/api/params.py diff --git a/backend/app/api/params.py b/backend/app/api/params.py new file mode 100644 index 0000000000..e3cbe4e5e0 --- /dev/null +++ b/backend/app/api/params.py @@ -0,0 +1,9 @@ +from typing import Annotated + +from fastapi import Query + +# Shared pagination query parameters. Bounds are enforced at the API layer so +# invalid values return 422 instead of reaching Postgres OFFSET/LIMIT (which +# rejects negative values and would otherwise surface as a 500). +SkipQuery = Annotated[int, Query(ge=0)] +LimitQuery = Annotated[int, Query(ge=1, le=100)] diff --git a/backend/app/api/routes/items.py b/backend/app/api/routes/items.py index f0eb30e4ce..7d08a87121 100644 --- a/backend/app/api/routes/items.py +++ b/backend/app/api/routes/items.py @@ -5,6 +5,7 @@ from sqlmodel import col, func, select from app.api.deps import CurrentUser, SessionDep +from app.api.params import LimitQuery, SkipQuery from app.models import Item, ItemCreate, ItemPublic, ItemsPublic, ItemUpdate, Message router = APIRouter(prefix="/items", tags=["items"]) @@ -12,7 +13,10 @@ @router.get("/", response_model=ItemsPublic) def read_items( - session: SessionDep, current_user: CurrentUser, skip: int = 0, limit: int = 100 + session: SessionDep, + current_user: CurrentUser, + skip: SkipQuery = 0, + limit: LimitQuery = 100, ) -> Any: """ Retrieve items. diff --git a/backend/app/api/routes/login.py b/backend/app/api/routes/login.py index 58441e37e9..28f94911ea 100644 --- a/backend/app/api/routes/login.py +++ b/backend/app/api/routes/login.py @@ -1,7 +1,7 @@ from datetime import timedelta from typing import Annotated, Any -from fastapi import APIRouter, Depends, HTTPException +from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException from fastapi.responses import HTMLResponse from fastapi.security import OAuth2PasswordRequestForm @@ -51,20 +51,24 @@ def test_token(current_user: CurrentUser) -> Any: @router.post("/password-recovery/{email}") -def recover_password(email: str, session: SessionDep) -> Message: +def recover_password( + email: str, session: SessionDep, background_tasks: BackgroundTasks +) -> 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 + # Always return the same response to prevent email enumeration attacks. + # Only send the email if the user actually exists, and send it in the + # background so the HTTP response does not wait on SMTP. 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( + background_tasks.add_task( + send_email, email_to=user.email, subject=email_data.subject, html_content=email_data.html_content, @@ -119,5 +123,5 @@ def recover_password_html_content(email: str, session: SessionDep) -> Any: ) return HTMLResponse( - content=email_data.html_content, headers={"subject:": email_data.subject} + content=email_data.html_content, headers={"subject": email_data.subject} ) diff --git a/backend/app/api/routes/private.py b/backend/app/api/routes/private.py index 9f33ef1900..0090b5caf8 100644 --- a/backend/app/api/routes/private.py +++ b/backend/app/api/routes/private.py @@ -1,12 +1,12 @@ from typing import Any -from fastapi import APIRouter -from pydantic import BaseModel +from fastapi import APIRouter, HTTPException +from pydantic import BaseModel, EmailStr, Field +from app import crud from app.api.deps import SessionDep -from app.core.security import get_password_hash from app.models import ( - User, + UserCreate, UserPublic, ) @@ -14,10 +14,9 @@ class PrivateUserCreate(BaseModel): - email: str - password: str + email: EmailStr = Field(max_length=255) + password: str = Field(min_length=8, max_length=128) full_name: str - is_verified: bool = False @router.post("/users/", response_model=UserPublic) @@ -25,14 +24,20 @@ def create_user(user_in: PrivateUserCreate, session: SessionDep) -> Any: """ Create a new user. """ - - user = User( - email=user_in.email, - full_name=user_in.full_name, - hashed_password=get_password_hash(user_in.password), + existing_user = crud.get_user_by_email(session=session, email=user_in.email) + if existing_user: + raise HTTPException( + status_code=400, + detail="The user with this email already exists in the system", + ) + + user = crud.create_user( + session=session, + user_create=UserCreate( + email=user_in.email, + password=user_in.password, + full_name=user_in.full_name, + ), ) - session.add(user) - session.commit() - return user diff --git a/backend/app/api/routes/users.py b/backend/app/api/routes/users.py index 1748f58484..2e6e05e1f4 100644 --- a/backend/app/api/routes/users.py +++ b/backend/app/api/routes/users.py @@ -10,6 +10,7 @@ SessionDep, get_current_active_superuser, ) +from app.api.params import LimitQuery, SkipQuery from app.core.config import settings from app.core.security import get_password_hash, verify_password from app.models import ( @@ -34,7 +35,7 @@ dependencies=[Depends(get_current_active_superuser)], response_model=UsersPublic, ) -def read_users(session: SessionDep, skip: int = 0, limit: int = 100) -> Any: +def read_users(session: SessionDep, skip: SkipQuery = 0, limit: LimitQuery = 100) -> Any: """ Retrieve users. """ diff --git a/backend/tests/api/routes/test_items.py b/backend/tests/api/routes/test_items.py index 3e82cd0134..9a4ec42781 100644 --- a/backend/tests/api/routes/test_items.py +++ b/backend/tests/api/routes/test_items.py @@ -162,3 +162,13 @@ def test_delete_item_not_enough_permissions( assert response.status_code == 403 content = response.json() assert content["detail"] == "Not enough permissions" + + +def test_read_items_negative_limit_returns_422( + client: TestClient, normal_user_token_headers: dict[str, str] +) -> None: + r = client.get( + f"{settings.API_V1_STR}/items/?limit=-1", + headers=normal_user_token_headers, + ) + assert r.status_code == 422 diff --git a/backend/tests/api/routes/test_login.py b/backend/tests/api/routes/test_login.py index 96677a25f6..e7f9e6263a 100644 --- a/backend/tests/api/routes/test_login.py +++ b/backend/tests/api/routes/test_login.py @@ -189,3 +189,38 @@ def test_login_with_argon2_password_keeps_hash(client: TestClient, db: Session) assert user.hashed_password == original_hash assert user.hashed_password.startswith("$argon2") + + +def test_recover_password_html_content_has_valid_subject_header( + client: TestClient, superuser_token_headers: dict[str, str] +) -> None: + r = client.post( + f"{settings.API_V1_STR}/password-recovery-html-content/{settings.FIRST_SUPERUSER}", + headers=superuser_token_headers, + ) + # Header name must be "subject" (not "subject:"); the invalid name + # previously raised RuntimeError: Invalid HTTP header name. + assert r.status_code == 200 + assert "subject" in r.headers + + +def test_recovery_password_sends_email_in_background_for_existing_user( + client: TestClient, +) -> None: + with patch("app.api.routes.login.send_email") as mock_send: + r = client.post( + f"{settings.API_V1_STR}/password-recovery/{settings.FIRST_SUPERUSER}", + ) + assert r.status_code == 200 + mock_send.assert_called_once() + + +def test_recovery_password_skips_email_for_unknown_user( + client: TestClient, +) -> None: + with patch("app.api.routes.login.send_email") as mock_send: + r = client.post( + f"{settings.API_V1_STR}/password-recovery/does-not-exist@example.com", + ) + assert r.status_code == 200 + mock_send.assert_not_called() diff --git a/backend/tests/api/routes/test_private.py b/backend/tests/api/routes/test_private.py index 1e1f985021..0a9597ffe8 100644 --- a/backend/tests/api/routes/test_private.py +++ b/backend/tests/api/routes/test_private.py @@ -24,3 +24,43 @@ def test_create_user(client: TestClient, db: Session) -> None: assert user assert user.email == "pollo@listo.com" assert user.full_name == "Pollo Listo" + + +def test_create_user_invalid_email(client: TestClient) -> None: + r = client.post( + f"{settings.API_V1_STR}/private/users/", + json={ + "email": "not-an-email", + "password": "password123", + "full_name": "Bad Email", + }, + ) + + assert r.status_code == 422 + + +def test_create_user_short_password(client: TestClient) -> None: + r = client.post( + f"{settings.API_V1_STR}/private/users/", + json={ + "email": "shortpw@example.com", + "password": "ab", + "full_name": "Short Password", + }, + ) + + assert r.status_code == 422 + + +def test_create_user_duplicate_email(client: TestClient) -> None: + payload = { + "email": "dupe@example.com", + "password": "password123", + "full_name": "Dupe User", + } + + r1 = client.post(f"{settings.API_V1_STR}/private/users/", json=payload) + assert r1.status_code == 200 + + r2 = client.post(f"{settings.API_V1_STR}/private/users/", json=payload) + assert r2.status_code == 400 diff --git a/backend/tests/api/routes/test_users.py b/backend/tests/api/routes/test_users.py index 9c4cdd5991..650d134c9e 100644 --- a/backend/tests/api/routes/test_users.py +++ b/backend/tests/api/routes/test_users.py @@ -519,3 +519,13 @@ def test_delete_user_without_privileges( ) assert r.status_code == 403 assert r.json()["detail"] == "The user doesn't have enough privileges" + + +def test_read_users_negative_skip_returns_422( + client: TestClient, superuser_token_headers: dict[str, str] +) -> None: + r = client.get( + f"{settings.API_V1_STR}/users/?skip=-1", + headers=superuser_token_headers, + ) + assert r.status_code == 422