Fix API bugs surfaced by live tracing (422/400 instead of 500)

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)]

backend/app/api/routes/items.py

@@ -5,14 +5,18 @@
 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"])
 
 
 @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.

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}
     )

backend/app/api/routes/private.py

@@ -1,38 +1,43 @@
 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,
 )
 
 router = APIRouter(tags=["private"], prefix="/private")
 
 
 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)
 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

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.
     """

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

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()

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

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