Several endpoints return 500 on invalid input (pagination, private user create, password recovery) — open a PR?

[noQbot]

First Check

• I added a very descriptive title here.
• I used the GitHub search to find a similar question and didn't find it.
• I searched the documentation, with the integrated search.
• I already searched in Google "How to X" and didn't find any information.
• I already read and followed all the tutorials in the docs and didn't find an answer.

Example Code

# B4 — negative pagination -> 500 (Postgres OFFSET/LIMIT must be >= 0)
curl -i "$API/api/v1/users/?skip=-1"   -H "Authorization: Bearer $SUPERUSER"
curl -i "$API/api/v1/items/?limit=-1"  -H "Authorization: Bearer $NORMAL"

# B1 — invalid email is committed, then every later GET /users/ 500s on
#      UserPublic(EmailStr)   (dev-only /private router, FASTAPI_ENV=development)
curl -i "$API/api/v1/private/users/" -H "Content-Type: application/json" \
  -d '{"email":"not-an-email","password":"password123","full_name":"X"}'

# B3 — duplicate email -> unhandled UniqueViolation -> 500
#      (post the same valid payload twice)

# B5 — superuser recovery-HTML endpoint sets header "subject:" (invalid HTTP
#      header name) -> RuntimeError: Invalid HTTP header name
curl -i "$API/api/v1/password-recovery-html-content/user@example.com" \
  -H "Authorization: Bearer $SUPERUSER"

Description

While exercising a fresh copy of the template I hit several endpoints that return 500 (or crash) on input that should be a 4xx:

Pagination (GET /items/, GET /users/): skip/limit are unbounded, so negative values reach Postgres OFFSET/LIMIT and 500. Expected: 422.
POST /private/users/ (dev-only router): email is a bare str, so an invalid address is committed and every later read 500s on UserPublic (EmailStr); no password bounds; is_verified is accepted but has no column; a duplicate email 500s on UniqueViolation. Expected: 422 for bad input, 400 for duplicate.
POST /password-recovery-html-content/{email}: the response header name is "subject:" — an invalid HTTP header name that raises RuntimeError.
Minor: password recovery sends SMTP synchronously, so the HTTP response waits on mail.

I have a fix ready (shared SkipQuery/LimitQuery, EmailStr + bounds + duplicate→400 via crud.create_user, "subject" header, BackgroundTasks for the email) with tests for each case, and all changes pass ruff. Would you be open to a PR?

Operating System

Windows

Operating System Details

No response

Project Version

main @ commit 68adb40

Python Version

3.14

Additional Context

No response

[0xshubhs]

Checked all of these against master, they're all real. Line references so you can point at them in the PR.

Pagination, users.py:37 and items.py:15:

def read_users(session: SessionDep, skip: int = 0, limit: int = 100) -> Any:
def read_items(session: SessionDep, current_user: CurrentUser, skip: int = 0, limit: int = 100)

Bare int with no Query(ge=0), so negatives sail through validation into OFFSET/LIMIT. This is the one that matters most since both sit on the always-mounted routers.

/private/users/, private.py:17:

class PrivateUserCreate(BaseModel):
    email: str
    password: str
    full_name: str
    is_verified: bool = False

Your read is right, and the email case is worse than a 500 on the next request. UserPublic inherits UserBase where email: EmailStr (models.py:15 and :63), so one bad POST writes a row that GET /users/ can never serialize again. It isn't a transient 500, the list endpoint stays broken until somebody deletes that row by hand.

On is_verified, it's not only missing a column. Grep the backend and it appears exactly once, on line 20 of that file. Nothing reads it anywhere. It's accepted and silently dropped.

And create_user goes straight from session.add to session.commit with no existing-email check, so the duplicate really is an unhandled UniqueViolation.

The header, login.py:122:

content=email_data.html_content, headers={"subject:": email_data.subject}

Trailing colon in the key. One character.

Something worth putting in the PR description: private.router is only mounted when FASTAPI_ENV == "development" (api/main.py:13), so the private-router items aren't a production exposure. Not a reason to skip them, but it changes how a reviewer weighs them against the pagination fix, which is production-facing.

I'd split it into two PRs. Pagination on its own is clean and uncontroversial and should merge quickly. The private-router changes are more opinionated (422 vs 400 on the duplicate, whether is_verified gets implemented or removed) and will attract discussion, so bundling them risks holding up the easy fix.

[noQbot]

Thanks, useful pass. The permanent-breakage angle is the one that matters: that bad email row can't be serialized by UserPublic, so GET /users/ stays broken until someone deletes it by hand - not a one-off 500. And is_verified is only ever set on that model, nothing reads it back.

I've actually already put these up as one PR, #2455, with tests. Your split point is fair though. Pagination is the clean, always-mounted one and could stand on its own; the /private changes are more of a judgment call - that router only mounts in development, and there's a real question of whether is_verified should be dropped or implemented. If a maintainer would rather review those separately, I'm happy to break #2455 into a pagination PR plus a /private one and keep the opinionated part in this thread.

[YuriiMotov]

@noQbot, thanks for reporting! We will take a look when we have time

[aditinfinity]

Hi Yuri,

Found and verified against master: read_users (users.py:37) and read_items (items.py:15) accept skip/limit as bare int with no Query() constraints. Negative values pass validation straight into the SQL OFFSET/LIMIT clause.

This sits on the always-mounted routers,not behind a dev-only flag ,so it’s live in production right now. Concretely:

•	Negative skip/limit bypass pagination entirely
•	No upper bound on limit means a single request can pull the full table

both are easy fixes ( hopefully)🤞

Are you updating this now anytime soon?
Given its production-facing and the fix is urgent, could you please help with this sooner.

[noQbot]

Thanks @YuriiMotov, and understood on closing #2455 - I shouldn't have opened it unrequested or bundled unrelated changes.

Could I get the go-ahead to open two focused PRs instead: one for the pagination 500s (both list routes, skip/limit bounds + tests), and a separate one for the dev-only /private fixes? Each would stay a single concern, and I'll review the diff line by line myself before opening.

If you'd rather just fix these internally, that's completely fine - happy to leave it as a report.

[YuriiMotov]

Thanks! We are tracking this internally and will address when we have time for this