Add API contract and authorization tests for the Phase 1 surface #8
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Context
The full Phase 1 read and write API surface shipped in v0.1.0 with no contract tests.
The frontend is typed against these shapes, so an unnoticed response change breaks the
UI at runtime rather than at build time. Authorization is likewise unverified.
Scope
Response-shape tests for every Phase 1 endpoint, plus authentication and role enforcement.
Implementation notes
{ data, meta }for success and{ error: { code, message, details } }for errors. A handler that returns a bareobject instead of the envelope should fail the suite.
photos.py,jobs.py,ingest.py,auth.py,health.py.GET /api/photosandGET /api/jobs, includingan invalid cursor and an out-of-range limit.
and an insufficient role gets 403. A read-only user must not be able to reach any write
endpoint. This is the highest-value part of this issue; the app guards family photo data
behind a single OAuth check today.
Request a photo id crafted to escape the root and assert it is refused.
Done when
docs/circa-api-spec.mdReferences
backend/app/api/routes/docs/circa-api-spec.mdDepends on: the backend test harness.
Amended by the audit of 2026-07-28.
The full 24-route × role matrix has now been built. Findings to fold into these tests:
while there is one collection, but the schema carries
collection_idon all seven owned tablesin anticipation of more. See #70.
POST /api/evidence/{id}/supersedeis the worst case — no photo lookup, no collection check,and no
review_versioncheck, unlike every sibling route.GET /api/jobshas no collection filter at all, and the spec says reviewers should see onlyjobs they triggered.
nothing to test against until that is fixed.
require_roleimplements a threshold, not set membership (#71) —require_role(reviewer, admin)is exactlyrequire_role(reviewer). Currently harmless, but a trap once roles expand.These tests also cannot be meaningful until the wire format is settled — see #75 (native error
shape) and #76 (response models).
Verified not vulnerable, so no test needed beyond a regression guard: path traversal in media
serving (storage keys are server-generated), SQL injection (all queries parameterized), and open
redirect (post-login destination is static config).
Done in
b36ee1d.backend/tests/test_api_contract.py, 38 tests / 133 cases.The matrix is derived, not written down
The audit built the 24-route × role matrix and then it lived in a comment. The problem with writing it down again as a list of tests is that a hand-listed matrix goes stale silently — a missing entry is an absent test, not a failing one, so the one route nobody remembered to cover is exactly the one with the hole.
ROUTE_ACCESSdeclares an access level for each of the 29 routes and is compared against the running application in both directions:test_every_served_route_declares_an_access_level)So adding an endpoint now requires stating, in one line, who may reach it — and then every claim in that table is exercised against the real server.
Route discovery walks the router tree, not
app.routes. Current FastAPI wraps included routers rather than flattening them, soapp.routesreturns four documentation endpoints and six opaque wrappers. An inventory built from it would have been silently empty — the worst possible failure for a completeness check, because it passes.What the matrix asserts
get_current_user" is the stronger question anyway)Path parameters resolve to real objects. A 401 on a route whose object does not exist proves nothing about ordering — a 404 would be equally consistent with the check never running. And the refusals are asserted to leak nothing: a 401 must not contain the id it was protecting, because existence is itself a disclosure.
test_an_admin_is_not_locked_out_of_a_reviewer_routeis the one that would catch a regression to #71's set-membership semantics.Two fixes fell out
1. Storage keys had no containment check.
LocalStorage._full_pathwasself._root / key, andPath("/srv/storage") / "/etc/passwd"is/etc/passwd— joining an absolute path discards the base, the same primitive that made #55 an RCE, arrived at from the other direction.The audit's note ("not vulnerable, storage keys are server-generated") is true of the route. It is a property of ingest being the only writer, not of the storage backend — a bulk importer, a migration that rewrites keys, or a manifest-driven restore all reach
get_pathwithout passing through ingest. Containment belongs where the guarantee is.put,get_path,existsanddeletenow all refuse a key that resolves outside the root.deleteincluded, deliberately: a containment check on reads alone would leavedeleteable to unlink a file outside the archive, which is the failure that cannot be undone.resolve()rather than a string prefix test, so a symlink inside the root pointing out of it is caught too.The media routes turn both a containment failure and a missing file into a 404 with one message — the difference is not the caller's to know — and log the containment failure at error level, because a corrupt row is an incident and a 404 in an access log is not. Previously either produced an unhandled exception.
2.
statuson the list endpoints raised a bare 400.GET /api/photosandGET /api/jobsparsed the filter by hand. Those were the only validation failures in the API not arriving as a 422 in thevalidation_errorenvelope, so a client with one error path actually had two and the second was undocumented. Typed as the enum, they also now publish their permitted values in the OpenAPI document, so the generated types (#76) can tell a callerneeds_reviewexists rather than accepting any string and failing at the server.openapi.jsonandgenerated.tsregenerated.Note on the issue text
The scope says to assert
{ error: { code, message, details } }for errors. #75 deliberately replaced that with FastAPI-nativedetail— the old envelope never reached a client at all. These tests pin the settled shape (docs/circa-api-spec.md§Errors): a string for display-only errors, an object withcodefor errors a client branches on.Done when
docs/circa-api-spec.md923 passed, 8 skipped; ruff clean.