Skip to content

Mount every route under /api, matching what is deployed - #283

Open
ColinToft wants to merge 4 commits into
mainfrom
colin/api-path-prefix
Open

Mount every route under /api, matching what is deployed#283
ColinToft wants to merge 4 commits into
mainfrom
colin/api-path-prefix

Conversation

@ColinToft

Copy link
Copy Markdown
Collaborator

Stacked on #280, #281 and #282 — the diff shown here includes those until they merge.

JIRA Ticket

N/A — closes the gap between main and what is actually deployed.

Implementation Description

This is not a proposal, it is reconciliation. The deployed Cloud Run service (backend-service, revision 00054) already serves every route under /api/api/locations/ returns 401 and /locations/ returns 404 against it right now. That code came from feat/deployment-setup (#210), whose author is away indefinitely. So production and main currently disagree about the shape of the API, and nobody can reproduce what is deployed from main. This makes main match.

Why the prefix exists at all. Firebase Hosting rewrites /api/** to the Cloud Run service, which puts the SPA and the API on one origin. That is what lets the refresh cookie stay SameSite=strict; a separate API domain would force SameSite=None, which is weaker and squarely in the path of third-party cookie restrictions. Hosting forwards the matched path verbatim rather than stripping the prefix, so the routes have to carry it themselves.

What changed

  • app/routers/__init__.py mounts every router under API_PREFIX = "/api".
  • frontend/openapi.json and the generated client regenerated — 50 paths, all prefixed.
  • boot-smoke.yml's authed curls moved to /api. The /openapi.json readiness probe stays at the root, where FastAPI serves it regardless of router prefixes.
  • Test clients built from create_app() now use base_url=".../api". Tests that mount their own bare FastAPI (test_auth_middleware) and the /_probe routers added directly to the app are untouched — they were never under the prefix.

Two bugs this turned up

1. POST /auth/refresh would have 404'd. axiosClient.ts issues the session refresh directly rather than through the generated client, because it must not re-enter the 401 interceptor — so nothing would have added the prefix for it. This is the quiet kind: AuthProvider fires a refresh on every load and a logged-out visit already produces a failing one, so a 404 would have read as business as usual while every session restore broke. isRefreshRequest matches with endsWith, so it still works.

The existing tests would not have caught it — they match on endsWith('/auth/refresh'), which cannot tell a prefixed path from an unprefixed one. There is now a test asserting the exact URL.

2. The auth coverage guard would have stayed green if the prefix vanished. test_every_exposed_route_is_classified compares the OpenAPI schema against ROUTE_POLICIES. Rather than prefix ~50 table entries — where the API is mounted is a deployment concern, not an auth one — it strips API_PREFIX before comparing. That is the right call for that test, but it means the guard is now blind to the prefix disappearing, which would 404 every request through the rewrite without failing anything.

tests/test_api_prefix.py is the test that would not stay green: every exposed path carries the prefix, a prefixed path reaches a route, the unprefixed path is gone, /openapi.json stays at the root, and a guard against the whole file passing vacuously on an empty schema.

Note on /docs and /redoc

They stay at the root, so once Hosting rewrites only /api/** they are reachable on the Cloud Run URL but not through the app domain. Deliberate, and pinned by a test — moving them under the prefix would publish them on the public site. It does not resolve the separate problem that they are currently public on the Cloud Run URL, because the service runs APP_ENV=development; that needs the secret-mount work before it can be flipped.

Steps to Test

  1. docker exec -e APP_ENV=testing f4k_backend python -m pytest -q
  2. cd frontend && pnpm test && npx tsc -b
  3. Run the app locally and sign in — the login and the session refresh both exercise the new paths.

What Should Reviewers Focus On?

  • Anything calling the backend outside the generated client. I found two (axiosClient.ts's refresh, TestImageUpload.tsx) by grepping for fetch( and URL literals. A third would fail at runtime, not at build time, so a second pair of eyes on that search is worth more than on the mechanical parts.
  • Stripping the prefix in the auth guard rather than prefixing the table.
  • The TestReachability assertions are "not 404" rather than a status code. Without the lifespan a handler that reaches for the database raises, so the status reflects the harness rather than the auth gate. What that gate returns is test_auth_integration.py's job.

Checklist

  • PR name and commits are descriptive, imperative, and atomic (trivial commits squashed)
  • I have requested a review from Claude, understood its suggestions, and implemented fixes where needed (or documented disagreements)
  • I have requested a review from the PL and relevant devs with background on this PR

Verification

  • Backend: ruff check / ruff format --check clean, mypy . — no issues in 157 source files, full suite below
  • Frontend: pnpm test 65 passed, tsc -b clean, eslint src --max-warnings=0 clean
  • OpenAPI: regenerated, 50/50 paths prefixed
  • Both new tests were checked by breaking what they guard: API_PREFIX = "" fails three of six backend cases, and reverting REFRESH_PATH gives expected '/auth/refresh' to be '/api/auth/refresh'

🤖 Generated with Claude Code

ColinToft and others added 4 commits August 19, 2026 13:44
`ENVIRONMENT` was never set anywhere in the repo, so `settings.environment`
always fell back to its "development" default. `is_production` was therefore
always False and `is_development` always True, in every environment — which on
a real deploy means the refresh cookie ships without Secure and /docs and
/redoc are publicly reachable. `is_testing` was always False, leaving the
testing branch of the logging config unreachable.

Two mechanisms had grown up around one fact: APP_ENV picked the Settings
subclass, ENVIRONMENT drove the is_* properties, and the subclasses never set
`environment`. Collapse them onto one `Environment` enum read from APP_ENV.
Pydantic now rejects an unrecognized value, so a typo fails at startup instead
of silently meaning development — the old `get_settings()` ended in a catch-all
`else` that returned DevelopmentSettings for anything it didn't recognize.

Delete DevelopmentSettings, ProductionSettings and TestingSettings: they
existed only to set `debug` and `testing`, which nothing in the backend reads,
and TestingSettings still carried a `mongodb_url` from the starter code.

The logging config becomes a match with assert_never, so adding an environment
without deciding how it logs is a mypy error rather than a silent fall-through.
Verified by temporarily adding a STAGING member.

models/__init__.py, migrations/env.py and seed_database.py were each parsing
APP_ENV themselves; they now read settings.environment, so there is one parse.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The app, Alembic and the seed script each had their own copy of
get_database_url, and they had already drifted: the seed script always used
POSTGRES_DB_DEV, ignoring APP_ENV, so seeding under testing wrote to the
development database. Replace all three with app/database_url.py.

Read the credentials from Settings rather than os.getenv. Every one of them is
already declared as a Settings field, so the os.getenv reads were a second
config mechanism sitting alongside the first — the same shape as the
APP_ENV/ENVIRONMENT split. It also matters for deployment: on Cloud Run the
config arrives as a mounted secrets file, which os.getenv cannot see.

FIREBASE_WEB_API_KEY was not a Settings field at all; firebase_rest_client
interpolated os.getenv straight into the sign-in URL, so an unset key sent
"?key=None" to Firebase. Promote it to a field alongside the others.

Fail on missing credentials, naming all of them at once, instead of building
postgresql+asyncpg://None:None@None:5432/None and failing at connect time.

Build the URL with URL.create so the password is escaped. The f-string
interpolation this replaces mangled any password containing URL syntax — an
"@" ends the userinfo section early and connects to the wrong host.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every entry in allow_origins is trusted with credentialed requests, so two
things in the list were worth removing.

The default carried uw-blueprint-starter-code.firebaseapp.com and .web.app,
inherited from the starter code. Those are Hosting sites for a different
project; whoever controls it could make credentialed cross-origin requests
against this API.

The preview-deploy entry never worked. allow_origins compares the Origin
header by exact string, so the appended pattern matched nothing — a dead entry
that reads like a working one. Patterns belong in allow_origin_regex. A
working replacement is deliberately left out: production already serves the
API through the Hosting /api/** rewrite, so a preview channel may be
same-origin and need no entry at all. That belongs with the /api prefix work.

cors_origins now defaults to empty. A deployed origin belongs to a specific
deployment and is configured there; the default cannot know it, and failing
closed is the right way to be wrong. Development still adds its localhost
entries in create_app().

The two tests asserting a 500 and a 422 keep their CORS headers were relying
on that default to supply localhost. They pin middleware ordering, so they now
configure the origin they need instead of inheriting whichever ones ship
enabled.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Cloud Run service already serves the API under /api — /api/locations/
answers and /locations/ 404s against revision 00054. That came from
feat/deployment-setup (#210), whose author is away indefinitely, so production
and main currently disagree about the shape of the API and nobody can
reproduce what is deployed from main. This makes main match.

The prefix is what lets Firebase Hosting rewrite /api/** to Cloud Run and put
the SPA and the API on one origin, which is what keeps the refresh cookie
SameSite=strict. Hosting forwards the matched path verbatim rather than
stripping it, so the routes carry the prefix themselves.

axiosClient issues the session refresh directly rather than through the
generated client — it must not re-enter the 401 interceptor — so nothing
would have added the prefix for it and every refresh would have 404ed. That
failure is quiet: AuthProvider fires a refresh on load and a logged-out visit
already produces a failing one, so it would have looked normal. The existing
tests matched with endsWith and could not tell a prefixed path from an
unprefixed one; one now asserts the exact URL.

The auth coverage guard strips API_PREFIX before comparing rather than
carrying it in fifty table keys, since where the API is mounted is a
deployment concern and not an auth one. That leaves the guard blind to the
prefix disappearing, so test_api_prefix.py pins it separately.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ColinToft
ColinToft requested a review from ChloeW125 August 19, 2026 23:56
@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown

Review

Reviewed the diff introduced by this PR (the "Mount every route under /api" commit; the rest of the shown diff is the stacked #280#282 content per the PR description, so I've focused on this commit's own changes).

Summary

This is a clean, well-scoped fix. The core change (app/routers/__init__.py wrapping all routers in an APIRouter(prefix=API_PREFIX)) is the correct FastAPI idiom for this, and the two follow-on bugs called out in the description (the hand-rolled /auth/refresh call in axiosClient.ts, and the auth-coverage guard's blind spot) are real and were caught and fixed/pinned correctly.

Things I verified and did not find issues with

  • httpx base_url + prefix interaction: conftest.py and the various test files switch base_url from "http://test" to "http://test/api" while call sites still do ac.get("/system-settings/") etc. (leading slash). I double-checked this isn't a bug — httpx.Client._merge_url always appends the relative path to base_url.raw_path (after stripping a leading /), unlike urllib.parse.urljoin's RFC 3986 "leading slash replaces the whole path" behavior. So "http://test/api" + "/system-settings/" correctly yields "http://test/api/system-settings/". Good catch making sure all the AsyncClient/TestClient construction sites were updated consistently (test_routes.py, test_org_contact_route.py, test_auth_integration.py, the various conftest.py fixtures).
  • isRefreshRequest: still matches correctly via endsWith(REFRESH_PATH) now that REFRESH_PATH = '/api/auth/refresh', and there's a new test (sends the refresh to the prefixed path) asserting the exact URL rather than just the suffix, which is exactly the gap the description calls out.
  • Docs/openapi.json placement: confirmed docs_url/redoc_url/openapi.json are intentionally left unprefixed and outside the Hosting rewrite (only reachable on the Cloud Run origin directly, and docs are disabled outside development anyway). test_api_prefix.py::TestDocsStayAtTheRoot pins this.
  • No other hand-rolled fetch calls were missed: grepped the frontend for other direct fetch()/hardcoded localhost:8080 usage outside the generated client — TestImageUpload.tsx was the only other one, and it's correctly updated.
  • test_every_exposed_route_is_classified: stripping API_PREFIX before comparing against ROUTE_POLICIES is the right call (auth policy is per-route, not per-mount-point), and test_api_prefix.py exists precisely to cover the blind spot that creates. Good self-awareness in the test design.

Minor/nit

  • test_api_prefix.py::test_there_are_routes_to_check guards against the "prefix check passes vacuously" case with len(schema_paths) > 40. That's a slightly magic threshold tied to the current route count — not wrong, just something that'll need bumping (or could quietly stop being a meaningful guard) as routes are added/removed over time. Not blocking, just flagging since it's the kind of assertion that can silently drift from its original intent.
  • Worth double checking (outside this PR's diff, since it's infra config): the actual Firebase Hosting rewrites config for /api/** isn't part of this repo as far as I could find, so nothing here enforces that the deployed rewrite rule matches API_PREFIX = "/api" other than convention/documentation. Given the PR's whole premise is "reconcile with what's actually deployed," it might be worth a comment pointing at wherever that Hosting config actually lives, for the next person who touches this.

Test coverage

Strong — test_api_prefix.py is a good, tightly-scoped addition (schema-level prefix check, reachability check, docs-stay-at-root check), and the axios refresh-path regression is now pinned with an exact-URL assertion instead of the old suffix match that couldn't distinguish prefixed from unprefixed. boot-smoke.yml's authed curls were correctly updated in lockstep.

No blocking issues found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant