[igorbenav]

Fix three security and correctness bugs found in review

Three defects, each reproduced against real Postgres before being touched, each fixed in its own commit, plus one hardening commit. The first is the subject of security advisory GHSA-h3fc-96x2-49rx (High, CWE-284 / CWE-915, reported by @pavelknespl): unauthenticated signup could mass-assign email_verified and the OAuth identifier columns, which defeats crudauth's account-claim protection and enables OAuth account pre-hijacking. The other two are a 500 on a valid user lookup and /admin pages being marked publicly cacheable. No new dependencies. Three response-header strings and one OpenAPI response schema change; everything else is behaviour-preserving.

---

1. Signup could pre-claim someone else's account

UserCreate declared google_id, github_id, oauth_provider, email_verified, oauth_created_at and oauth_updated_at as body fields, so POST /api/v1/users/ — which has no auth dependency — let a client mark its own address verified. That matters because crudauth links a verified provider login to an existing account without claiming it: a verified address is supposed to have proved itself, so there's no password reset, no MFA wipe, no token_version bump and no session termination. Registering under someone's address with email_verified: true turns that protection off.

Reproduced end to end before the fix, then again as the control:

SIGNUP (with email_verified: true):     201
STORED email_verified:                  True
victim's Google callback:               307
ATTACKER LOGIN AFTER VICTIM SIGN-IN:    200   | google_id: victim-google-sub
CONTROL (same steps, no self-set flag): 401

The attacker keeps password access to the account the victim now signs into with Google; the control run shows the claim working whenever the flag isn't set.

The fix removes those six fields from the public UserCreate, which now carries name, username, email and password. UserCreateInternal keeps all of them, so every server-side path is untouched: crudauth's OAuth provisioning builds the User row itself through new_user_fields, the admin panel's form_create_rules never listed these fields, and scripts/create_first_superuser.py passes only the four public ones.

The advisory's Stage 1 payload against the patched code:

POST /api/v1/users/ -> 422
extra_forbidden: email_verified, oauth_provider, github_id
row created: False

Why 422 and not silently dropping the fields

UserCreate already had extra="forbid", so not declaring the fields is enough to reject them, and that's the same treatment #302 gave the update path. Silently ignoring them would look to an integrator like the field was accepted — the failure would surface later as "why is this user not verified?" rather than at the call that was never going to work.

What else can reach those columns

Checked, because the schema is only one door: POST /api/v1/users/ is the only route that writes a user row from client input; the auth router mounts login, logout, logout-all, refresh-csrf, check-auth and the two OAuth routes, with no crudauth /register and no email-verification endpoints; UserAdminUpdate is referenced only by the sqladmin edit form, which is superuser-gated; and is_superuser was never on UserCreate.

---

2. Hardening: the row is built from an allowlist

Removing the fields made the row safe by omission rather than by construction. UserService.create still did user.model_dump() → UserCreateInternal(**dict), so anything that reaches it carrying those keys would still write them — a re-added field, or a subclass. A UserCreate subclass declaring email_verified = True proved it: the row came back verified.

create now names what it reads — name, username, email, and the hash it computes — so a password signup always leaves the verification flag and the OAuth identifiers at their column defaults, whatever the payload carries. This is the advisory's "defense in depth" recommendation.

It stops short of its "better still": routing self-registration through crudauth's own registration entry point. That changes the endpoint's contract and error shapes, and belongs in its own PR.

---

3. Looking up a valid user answered 500

UserProfileRead is the response_model of GET /api/v1/users/{username} and hard-coded stricter rules than the column allows: username as max_length=20 with pattern=r"^[a-z0-9]+$" against a String(32) column and USERNAME_PATTERN = ^[a-z0-9_]+$, plus name as a literal max_length=30. Response validation failed, the failure wasn't an HTTPException, and the global handler turned it into a 500. So with_underscore — or any username over 20 characters — signed up fine and was then unreadable. Both cases returned 500 before the fix and 200 after.

name and username are now plain str with examples kept for the docs. A read schema describes what's stored: re-validating on the way out can only turn valid data into a 500, and it leaves a second copy of the rules to drift from the model. The visible effect is that this one response model loses maxLength, minLength and pattern in the OpenAPI document; no body or status code changes.

The other read schemas were checked and left alone. UserRead uses the shared constants and matches its columns, and TierRead / APIKeyRead / KeyUsageRead agree with theirs. One is worth its own change later: RateLimitRead inherits a path validator requiring ^\/[a-zA-Z0-9_\-\/]+$, so a rate-limit row whose path is a route template like /api/v1/users/{username} — reachable through the admin panel, which writes through the model rather than the schema — would 500 on read.

---

4. /admin pages were marked publicly cacheable

ClientCacheMiddleware gave private, no-cache, no-store, must-revalidate to anything under /api/ and public, max-age=<CLIENT_CACHE_MAX_AGE> to everything else — the SQLAdmin panel, /health, /docs, /openapi.json and the OAuth redirects. Behind a CDN or any shared proxy, one admin's page could be stored and served to the next visitor, and health checks could go stale.

The rule is now, in order:

1. A Cache-Control the response already set is left alone, so a route can opt into caching deliberately.
2. /api/ keeps its exact current header, so nothing API-facing changes.
3. public, max-age=<CLIENT_CACHE_MAX_AGE> for an explicit prefix allowlist, which is ("/admin/statics/",) — SQLAdmin serves its own CSS and JS from /statics under the panel's mount. The prefixes are a constructor argument, so an application that mounts its own assets can add them.
4. Everything else: private, no-store. That covers the admin pages, /health, the docs and the OAuth redirects.

CLIENT_CACHE_MAX_AGE therefore governs the static allowlist only. Anyone who was relying on it to let a CDN cache an HTML page now sets the header on that route, which point 1 preserves.

fastroai-template carries the same middleware, verbatim, so it has the same defect and wants the same change.

---

Behaviour changes a client can notice

• Signup answers 422 for email_verified, google_id, github_id, oauth_provider, oauth_created_at and oauth_updated_at instead of accepting them.
• GET /api/v1/users/{username} answers 200 where it used to answer 500, and its OpenAPI response schema no longer constrains name and username.
• Non-API, non-static responses carry private, no-store instead of public, max-age=….

What was deliberately left out

• Existing rows. The patch can't clean deployments that already ran the vulnerable code. This app has no email pipeline, so email_verified can only legitimately be true through an OAuth link or an admin edit, which makes the audit precise; the query and the remediation belong in the advisory, since people who copied the template can't fix it by upgrading:

  SELECT id, username, email, oauth_provider, google_id, github_id, created_at
  FROM "user"
  WHERE email_verified AND google_id IS NULL AND github_id IS NULL;

• UserRead and the other read schemas, per above — only the broken one changed.

• RateLimitRead's path validator, flagged above, not fixed here.

• crudauth's own registration entry point as the signup path.

---

Test Plan

Automated

• uv run pytest — 422 passed (407 on main, 15 new)
• uv run ruff check src tests and uv run ruff format --check src tests — clean
• uv run mypy src — no issues

Signup (8 new)

• Each of the six fields answers 422 and writes no row — all six failed before the fix
• A normal signup stores email_verified=False with no OAuth identifiers
• End to end: sign up for an address, the address then arrives verified from Google, and the pre-registered password answers 401 with google_id and email_verified now set — this pins crudauth's claim itself
• The advisory's own payload re-run against the patch: 422, no row

Allowlist hardening (1 new)

• A UserCreate subclass declaring email_verified=True and a google_id still produces an unverified row with no provider — failed before the change

Profile lookup (2 new)

• with_underscore and a 25-character username sign up and then look up with 200 — both 500'd before

Client cache (4 new, 1 rewritten)

• /admin/user/list, /health and an unlisted /static/ path are private, no-store — all failed before
• /admin/statics/css/main.css is public, max-age=…
• A route that sets its own Cache-Control keeps it — failed before
• test_static_paths_get_public_cache rewritten as test_admin_statics_get_public_cache, since the old path is no longer public
• /api/ keeps private, no-cache, no-store, must-revalidate (existing test, unchanged)

Dependencies

None new.

After merging

The advisory has no affected or patched versions set, and the latest release is v0.19.0, so these commits want a tagged patch release to point at: affected <= 0.19.0, patched at the new tag. The advisory text should carry the diff and the SQL above, not just a version, because downstream users apply patches rather than upgrade a dependency.