Repository navigation
fix(api): a malformed id is a 422 or 400, not a server error - #546
Open
jwvanderstam wants to merge 3 commits into
Open
jwvanderstam wants to merge 3 commits into
jwvanderstam wants to merge 3 commits into
Conversation
A path id addressing a UUID-keyed row was typed `str`, so a value that is not a UUID reached Postgres, which refused it; each route's broad `except` turned that into a 500. About thirty routes did this, across users, workspaces, conversations, connectors, memories, annotations and keys. A malformed `X-Workspace-ID` did the same inside the workspace guard. - `UuidId` in `src/models.py` validates those path ids at the boundary: a malformed one is a 422 before the route runs, as an `int` id already was; a well-formed unknown one is still a 404. - `check_workspace_access` refuses a header or query workspace that is not a UUID with a 400, before any lookup. It only refuses more. - `tests/integration/test_malformed_ids_are_not_server_errors.py` walks every object route over HTTP against Postgres with a malformed id. - Unit tests that addressed routes with ids like `ws-1` now use UUIDs, and `/users/me`'s fixture no longer lets a bare mock become the pinned workspace. Touches `src/security_fastapi.py`: needs the QP-9 separate-session review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkWPq9YWxtZ6hbDvMft8fh
Review findings on #546: - The guard checked only a workspace it resolved itself from the header, so a route handing it a value directly still answered 500 to a malformed one: `GET /api/workspaces/active` (the header) and `POST /api/workspaces/switch` (the body). It now checks whatever names the workspace. - `uuid.UUID` accepts `urn:uuid:` forms, which Postgres refuses, and the value reaches Postgres as written. Only the canonical form passes now, in either case. - Three unit test files still addressed routes with ids like `some-id` (`test_auth_routes_unit`, `test_cw2_soft_delete`, `test_cw2cdef_soft_delete`); two more passed `ws-1` to the guard. They use UUIDs now. The integration test probes both routes and the urn form, beside a control that the caller's own workspace is admitted on each; five of its probes fail on the previous guard. Unit tests cover the guard directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkWPq9YWxtZ6hbDvMft8fh
`test_memory_routes_full` got a 422 for `abc-123` and `ghost`. And `test_workspace_role_enforcement`'s export check asserted only `!= 403`, which a 422 satisfies: it still passed while testing nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KkWPq9YWxtZ6hbDvMft8fh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
On
main, about thirty routes answered 500 to a path id that is not a UUID: users, workspaces, conversations, connectors, memories, annotations and API keys. The id was typedstr, Postgres refused it (InvalidTextRepresentation), and the route's ownexcept Exceptionturned that into a 500. A malformedX-Workspace-IDheader did the same inside the workspace guard.The fix
UuidId(src/models.py): anAnnotated[str, AfterValidator]on every UUID path parameter. A malformed id is a 422 before the route runs, as anintid already was. A well-formed unknown id is still a 404.check_workspace_accessrefuses a header or query workspace that is not a UUID with a 400, before any database lookup. The change only refuses more requests; it admits nothing new.Tests
tests/integration/test_malformed_ids_are_not_server_errors.pywalks every object route fromapp.openapi()with a malformed id and expects no 5xx. It fails onmainand passes here; together with the HTTP authorisation matrix, 123 passed.ws-1orsome-idnow use fixed UUIDs (41 tests in 10 files). The ten files pass locally: 232 passed, 1 skipped.test_users_me_identity:members.for_useris no longer a bare mock. A bare mock is truthy, so it became the pinned workspace, and the second guard call refused it. This is the traptesting.mddescribes.Review
This PR touches
src/security_fastapi.py, a security-critical path, so it needs the QP-9 separate-session review before merge.🤖 Generated with Claude Code
https://claude.ai/code/session_01KkWPq9YWxtZ6hbDvMft8fh