diff --git a/.claude/rules/file-map.md b/.claude/rules/file-map.md index c4ce62ca..922ce7e2 100644 --- a/.claude/rules/file-map.md +++ b/.claude/rules/file-map.md @@ -138,6 +138,7 @@ Full module index for LocalChat. **Keep this current** — update in the same co | `docs/MIGRATIONS.md` | Migration docs — how to apply, write, and roll back | | `tests/integration/test_object_authorization_over_the_wire.py` | P2-2b — a plain user holding workspace A addresses workspace B's objects over HTTP against a real Postgres, and every route must refuse. The route list is derived from `app.openapi()` (not `app.routes`, which this FastAPI version leaves unflattened as `_IncludedRouter` with `path=None`), an unclassified path parameter fails rather than silently dropping its routes, and the fixture proves its own objects are reachable by their owner — a 404 is a pass here, so an empty fixture would be a perfect green run testing nothing. Runs as an editor and as an owner of A (R-5), and probes ids named in a body or header beside a control with the caller's own | | `tests/unit/test_all_workspaces_has_a_reason.py` | R-5b (QP-9 review) — every function in `src/`, `mcp_servers/` and `plugins/` passing `ALL_WORKSPACES` (bare or as `scope.ALL_WORKSPACES`) has a recorded reason that holds for every caller; a new use without one fails, so does a reason whose use is gone, and so does importing it under another name. A comparison on the scope is not a use | +| `tests/integration/test_malformed_ids_are_not_server_errors.py` | Every object route of the authorisation matrix, given `not-a-uuid` and a number past bigint, answers below 500: a malformed UUID path id is a 422 (`UuidId` in `src/models.py`), one well-formed but absent still reaches the database as a 404, and a malformed `X-Workspace-ID` is the guard's 400. Each used to be the route's own 500 | | `tests/integration/test_graph_expansion_scope.py` | R-12 (QP-9 review) — GraphRAG's 1-hop lookup returns a name that co-occurs in the caller's workspace, not one from another workspace's document or from a retired one | | `tests/integration/test_ontology_reads_live_documents.py` | A workspace's ontology counts only its live documents and live chunks: a retired document and a chunk retired by a re-ingest leave it | | `tests/integration/test_chunk_statistics_scope.py` | R-1 (QP-9 review) — document statistics count and sample only the caller's workspace's live chunks, with a second workspace and a retired document beside it | diff --git a/CHANGELOG.md b/CHANGELOG.md index 214181a0..5e19d9da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,12 @@ reasoning attached, in [docs/history/LESSONS_LEARNED.md](docs/history/LESSONS_LE ### Fixed +- **A malformed id is the caller's mistake, not a server error.** A path id that is not a + UUID reached Postgres, which refused it, and the route answered 500 on about thirty + routes, from users and workspaces to conversations and connectors. Such an id is now a + 422 before the route runs, as an integer id already was. The workspace guard answers + 400 to a workspace named in a header, query or body that is not a UUID in canonical + form; Python reads `urn:uuid:` forms that Postgres refuses. - **A failing plugin no longer leaves tools behind, and a reloaded one can be unloaded.** A plugin that raised after registering a tool kept that tool. A reload recorded none of the plugin's tools, so unloading it afterwards left them registered. And a loader given an diff --git a/src/models.py b/src/models.py index 2b8640fe..17d3ee82 100644 --- a/src/models.py +++ b/src/models.py @@ -1,9 +1,11 @@ # Python 3.14 compatibility: Use string annotations to avoid __annotate__ issues from __future__ import annotations -from typing import Literal +from typing import Annotated, Literal +from uuid import UUID from pydantic import ( + AfterValidator, BaseModel, Field, StrictBool, @@ -12,6 +14,15 @@ model_validator, ) + +def _canonical_uuid(value: str) -> str: + return str(UUID(value)) + + +#: A path id addressing a row keyed by UUID. Refused at the boundary with a 422, as an +#: ``int`` id already is: unchecked, Postgres refused it and the route answered 500. +UuidId = Annotated[str, AfterValidator(_canonical_uuid)] + _MODEL_NAME_EMPTY = 'Model name cannot be empty' _MODEL_NAME_INVALID_CHARS = 'Model name contains invalid characters' from . import config diff --git a/src/routes_fastapi/annotation_routes.py b/src/routes_fastapi/annotation_routes.py index 84564f79..4e18cab4 100644 --- a/src/routes_fastapi/annotation_routes.py +++ b/src/routes_fastapi/annotation_routes.py @@ -7,6 +7,7 @@ from fastapi import APIRouter, Depends, Request from fastapi.responses import JSONResponse +from ..models import UuidId from ..security_fastapi import get_current_user_id from ..services.annotations import AnnotationsService from ..utils.logging_config import get_logger @@ -70,7 +71,7 @@ def list_chunk_annotations(chunk_id: int, request: Request, annotations: Annotat @router.delete("/annotations/{annotation_id}") -def delete_annotation(annotation_id: str, request: Request, annotations: Annotations) -> Any: +def delete_annotation(annotation_id: UuidId, request: Request, annotations: Annotations) -> Any: denied = _deny(request, None, "editor") if denied: return denied diff --git a/src/routes_fastapi/auth_routes.py b/src/routes_fastapi/auth_routes.py index fa72dadf..78d0ee05 100644 --- a/src/routes_fastapi/auth_routes.py +++ b/src/routes_fastapi/auth_routes.py @@ -16,6 +16,7 @@ GrantWorkspaceRequest, LoginRequest, UpdateUserRequest, + UuidId, ) from ..security_fastapi import ( SESSION_COOKIE, @@ -205,7 +206,7 @@ def get_own_identity(request: Request, users: Users) -> Any: @router.get("/users/{user_id}/workspaces") def list_user_workspaces( - user_id: str, + user_id: UuidId, users: Users, _admin: Annotated[str, Depends(require_admin_dep)], ) -> Any: @@ -219,7 +220,7 @@ def list_user_workspaces( @router.post("/users/{user_id}/workspaces") async def grant_user_workspace( - user_id: str, + user_id: UuidId, request: Request, users: Users, _admin: Annotated[str, Depends(require_admin_dep)], @@ -240,8 +241,8 @@ async def grant_user_workspace( @router.delete("/users/{user_id}/workspaces/{workspace_id}") def revoke_user_workspace( - user_id: str, - workspace_id: str, + user_id: UuidId, + workspace_id: UuidId, users: Users, _admin: Annotated[str, Depends(require_admin_dep)], ) -> Any: @@ -258,7 +259,7 @@ def revoke_user_workspace( @router.get("/users/{user_id}") -def get_user(user_id: str, users: Users, _admin: Annotated[str, Depends(require_admin_dep)]) -> Any: +def get_user(user_id: UuidId, users: Users, _admin: Annotated[str, Depends(require_admin_dep)]) -> Any: try: user = users.get_user(user_id) if user is None: @@ -271,7 +272,7 @@ def get_user(user_id: str, users: Users, _admin: Annotated[str, Depends(require_ @router.put("/users/{user_id}") async def update_user( - user_id: str, request: Request, users: Users, _admin: Annotated[str, Depends(require_admin_dep)] + user_id: UuidId, request: Request, users: Users, _admin: Annotated[str, Depends(require_admin_dep)] ) -> Any: data = await request.json() if await request.body() else {} try: @@ -294,7 +295,7 @@ async def update_user( @router.delete("/users/{user_id}/purge") -def purge_user(user_id: str, users: Users, admin_user_id: Annotated[str, Depends(require_admin_dep)]) -> Any: +def purge_user(user_id: UuidId, users: Users, admin_user_id: Annotated[str, Depends(require_admin_dep)]) -> Any: try: if not users.purge_user(user_id): return JSONResponse( @@ -316,7 +317,7 @@ def purge_user(user_id: str, users: Users, admin_user_id: Annotated[str, Depends @router.delete("/users/{user_id}") -def delete_user(user_id: str, users: Users, admin_user_id: Annotated[str, Depends(require_admin_dep)]) -> Any: +def delete_user(user_id: UuidId, users: Users, admin_user_id: Annotated[str, Depends(require_admin_dep)]) -> Any: try: if not users.retire_user(user_id, actor=admin_user_id): return JSONResponse({"success": False, "message": _NOT_FOUND}, status_code=404) diff --git a/src/routes_fastapi/connector_routes.py b/src/routes_fastapi/connector_routes.py index 977298c0..b5d4873b 100644 --- a/src/routes_fastapi/connector_routes.py +++ b/src/routes_fastapi/connector_routes.py @@ -10,6 +10,7 @@ from fastapi import APIRouter, Depends, HTTPException, Request from fastapi.responses import JSONResponse +from ..models import UuidId from ..security_fastapi import get_current_user_id, require_admin_dep from ..services.connectors import ConnectorsService from ..services.errors import ServiceError @@ -113,7 +114,7 @@ async def create_connector(request: Request, connectors: Connectors) -> Any: @router.get("/connectors/{connector_id}") -def get_connector(connector_id: str, request: Request, connectors: Connectors) -> Any: +def get_connector(connector_id: UuidId, request: Request, connectors: Connectors) -> Any: denied = _deny(request, None, "owner") if denied: return denied @@ -128,7 +129,7 @@ def get_connector(connector_id: str, request: Request, connectors: Connectors) - @router.put("/connectors/{connector_id}") -async def update_connector(connector_id: str, request: Request, connectors: Connectors) -> Any: +async def update_connector(connector_id: UuidId, request: Request, connectors: Connectors) -> Any: denied = _deny(request, None, "owner") if denied: return denied @@ -153,7 +154,7 @@ async def update_connector(connector_id: str, request: Request, connectors: Conn @router.delete("/connectors/{connector_id}/purge") def purge_connector( - connector_id: str, + connector_id: UuidId, connectors: Connectors, _admin: Annotated[str, Depends(require_admin_dep)], ) -> Any: @@ -179,7 +180,7 @@ def purge_connector( @router.delete("/connectors/{connector_id}") -def delete_connector(connector_id: str, request: Request, connectors: Connectors) -> Any: +def delete_connector(connector_id: UuidId, request: Request, connectors: Connectors) -> Any: denied = _deny(request, None, "owner") if denied: return denied @@ -193,7 +194,7 @@ def delete_connector(connector_id: str, request: Request, connectors: Connectors @router.post("/connectors/{connector_id}/sync") -def trigger_sync(connector_id: str, request: Request, connectors: Connectors) -> Any: +def trigger_sync(connector_id: UuidId, request: Request, connectors: Connectors) -> Any: denied = _deny(request, None, "owner") if denied: return denied @@ -218,7 +219,7 @@ def _run() -> None: @router.get("/connectors/{connector_id}/history") def sync_history( - connector_id: str, request: Request, connectors: Connectors, limit: int = 20 + connector_id: UuidId, request: Request, connectors: Connectors, limit: int = 20 ) -> Any: denied = _deny(request, None, "owner") if denied: @@ -235,7 +236,7 @@ def sync_history( @router.post("/connectors/{connector_id}/webhook") -async def receive_webhook(connector_id: str, request: Request, connectors: Connectors) -> Any: +async def receive_webhook(connector_id: UuidId, request: Request, connectors: Connectors) -> Any: # No user session here: this is a public receiver, and the connector id plus its # secret is the whole credential. ALL_WORKSPACES says that rather than relying on # an omitted argument to mean it. diff --git a/src/routes_fastapi/gkb_routes.py b/src/routes_fastapi/gkb_routes.py index 69e0cc0f..7d26c6ff 100644 --- a/src/routes_fastapi/gkb_routes.py +++ b/src/routes_fastapi/gkb_routes.py @@ -8,7 +8,7 @@ from fastapi.responses import JSONResponse from pydantic import ValidationError -from ..models import ContributionRequest +from ..models import ContributionRequest, UuidId from ..security_fastapi import require_admin_dep from ..services.contributions import ContributionsService from ..services.errors import ServiceError @@ -27,7 +27,7 @@ @router.post("/workspaces/{workspace_id}/contribute") async def contribute( - workspace_id: str, + workspace_id: UuidId, request: Request, contributions: Contributions, admin_id: Annotated[str, Depends(require_admin_dep)], @@ -54,7 +54,7 @@ async def contribute( @router.delete("/workspaces/{workspace_id}/contributions") -def archive_contributions(workspace_id: str, request: Request, contributions: Contributions) -> Any: +def archive_contributions(workspace_id: UuidId, request: Request, contributions: Contributions) -> Any: # Withdrawing only narrows what every workspace reads, so the owner may do it alone. denied = _deny(request, workspace_id, "owner") if denied: diff --git a/src/routes_fastapi/longterm_memory_routes.py b/src/routes_fastapi/longterm_memory_routes.py index 04e13c22..8ad72200 100644 --- a/src/routes_fastapi/longterm_memory_routes.py +++ b/src/routes_fastapi/longterm_memory_routes.py @@ -8,6 +8,7 @@ from fastapi.responses import JSONResponse from .. import config +from ..models import UuidId from ..security_fastapi import get_current_user_id from ..services.memories import MemoriesService from ..utils.logging_config import get_logger @@ -67,7 +68,7 @@ async def extract_memories(request: Request, memories_svc: Memories) -> Any: @router.delete("/{memory_id}") -def delete_memory(memory_id: str, request: Request, memories_svc: Memories) -> Any: +def delete_memory(memory_id: UuidId, request: Request, memories_svc: Memories) -> Any: denied = _deny(request, None, "editor") if denied: return denied diff --git a/src/routes_fastapi/memory_routes.py b/src/routes_fastapi/memory_routes.py index e30b68a1..0cb675e3 100644 --- a/src/routes_fastapi/memory_routes.py +++ b/src/routes_fastapi/memory_routes.py @@ -8,6 +8,7 @@ from fastapi import APIRouter, Depends, Request from fastapi.responses import JSONResponse, Response +from ..models import UuidId from ..security_fastapi import get_current_user_id, require_admin_dep from ..services.conversations import ConversationsService from ..services.errors import ServiceError @@ -62,7 +63,7 @@ def delete_all_conversations(request: Request, conversations: Conversations) -> @router.get("/conversations/{conversation_id}") -def get_conversation(conversation_id: str, request: Request, conversations: Conversations) -> Any: +def get_conversation(conversation_id: UuidId, request: Request, conversations: Conversations) -> Any: denied = _deny(request, None, "viewer") if denied: return denied @@ -93,7 +94,7 @@ def _build_binary_export(fmt: str, title: str, messages: list) -> bytes: @router.get("/conversations/{conversation_id}/export") def export_conversation( - conversation_id: str, request: Request, conversations: Conversations, format: str = "json" + conversation_id: UuidId, request: Request, conversations: Conversations, format: str = "json" ) -> Any: denied = _deny(request, None, "viewer") if denied: @@ -143,7 +144,7 @@ def export_conversation( @router.get("/conversations/{conversation_id}/documents") def get_conversation_documents( - conversation_id: str, request: Request, conversations: Conversations + conversation_id: UuidId, request: Request, conversations: Conversations ) -> Any: denied = _deny(request, None, "viewer") if denied: @@ -156,7 +157,7 @@ def get_conversation_documents( @router.put("/conversations/{conversation_id}/documents") async def set_conversation_documents( - conversation_id: str, request: Request, conversations: Conversations + conversation_id: UuidId, request: Request, conversations: Conversations ) -> Any: denied = _deny(request, None, "editor") if denied: @@ -174,7 +175,7 @@ async def set_conversation_documents( @router.patch("/conversations/{conversation_id}") async def update_conversation( - conversation_id: str, request: Request, conversations: Conversations + conversation_id: UuidId, request: Request, conversations: Conversations ) -> Any: denied = _deny(request, None, "editor") if denied: @@ -192,7 +193,7 @@ async def update_conversation( @router.delete("/conversations/{conversation_id}/purge") def purge_conversation( - conversation_id: str, + conversation_id: UuidId, conversations: Conversations, _admin: Annotated[str, Depends(require_admin_dep)], ) -> Any: @@ -215,7 +216,7 @@ def purge_conversation( @router.delete("/conversations/{conversation_id}") -def delete_conversation(conversation_id: str, request: Request, conversations: Conversations) -> Any: +def delete_conversation(conversation_id: UuidId, request: Request, conversations: Conversations) -> Any: denied = _deny(request, None, "editor") if denied: return denied diff --git a/src/routes_fastapi/workspace_routes.py b/src/routes_fastapi/workspace_routes.py index e2e170e0..6b86dd4b 100644 --- a/src/routes_fastapi/workspace_routes.py +++ b/src/routes_fastapi/workspace_routes.py @@ -11,6 +11,7 @@ from fastapi.responses import JSONResponse, StreamingResponse from .. import config +from ..models import UuidId from ..security_fastapi import ( check_workspace_access, get_current_user_id, @@ -126,7 +127,7 @@ async def switch_workspace(request: Request, workspaces_svc: Workspaces) -> Any: @router.get("/workspaces/{workspace_id}") -def get_workspace(workspace_id: str, request: Request, workspaces_svc: Workspaces) -> Any: +def get_workspace(workspace_id: UuidId, request: Request, workspaces_svc: Workspaces) -> Any: denied = _deny(request, workspace_id, "viewer") if denied: return denied @@ -142,7 +143,7 @@ def get_workspace(workspace_id: str, request: Request, workspaces_svc: Workspace @router.put("/workspaces/{workspace_id}") -async def update_workspace(workspace_id: str, request: Request, workspaces_svc: Workspaces) -> Any: +async def update_workspace(workspace_id: UuidId, request: Request, workspaces_svc: Workspaces) -> Any: denied = _deny(request, workspace_id, "owner") if denied: return denied @@ -163,7 +164,7 @@ async def update_workspace(workspace_id: str, request: Request, workspaces_svc: @router.delete("/workspaces/{workspace_id}/purge") def purge_workspace( - workspace_id: str, + workspace_id: UuidId, workspaces_svc: Workspaces, _admin: Annotated[str, Depends(require_admin_dep)], ) -> Any: @@ -189,7 +190,7 @@ def purge_workspace( @router.delete("/workspaces/{workspace_id}") -def delete_workspace(workspace_id: str, request: Request, workspaces_svc: Workspaces) -> Any: +def delete_workspace(workspace_id: UuidId, request: Request, workspaces_svc: Workspaces) -> Any: denied = _deny(request, workspace_id, "owner") if denied: return denied @@ -203,7 +204,7 @@ def delete_workspace(workspace_id: str, request: Request, workspaces_svc: Worksp @router.get("/workspaces/{workspace_id}/members") -def list_workspace_members(workspace_id: str, request: Request, workspaces_svc: Workspaces) -> Any: +def list_workspace_members(workspace_id: UuidId, request: Request, workspaces_svc: Workspaces) -> Any: denied = _deny(request, workspace_id, "viewer") if denied: return denied @@ -216,7 +217,7 @@ def list_workspace_members(workspace_id: str, request: Request, workspaces_svc: @router.post("/workspaces/{workspace_id}/members") -async def add_workspace_member(workspace_id: str, request: Request, workspaces_svc: Workspaces) -> Any: +async def add_workspace_member(workspace_id: UuidId, request: Request, workspaces_svc: Workspaces) -> Any: denied = _deny(request, workspace_id, "owner") if denied: return denied @@ -237,7 +238,7 @@ async def add_workspace_member(workspace_id: str, request: Request, workspaces_s @router.put("/workspaces/{workspace_id}/members/{user_id}") async def update_workspace_member( - workspace_id: str, user_id: str, request: Request, workspaces_svc: Workspaces + workspace_id: UuidId, user_id: UuidId, request: Request, workspaces_svc: Workspaces ) -> Any: denied = _deny(request, workspace_id, "owner") if denied: @@ -256,7 +257,7 @@ async def update_workspace_member( @router.delete("/workspaces/{workspace_id}/members/{user_id}") def remove_workspace_member( - workspace_id: str, user_id: str, request: Request, workspaces_svc: Workspaces + workspace_id: UuidId, user_id: UuidId, request: Request, workspaces_svc: Workspaces ) -> Any: denied = _deny(request, workspace_id, "owner") if denied: @@ -274,7 +275,7 @@ def remove_workspace_member( @router.get("/workspaces/{workspace_id}/presence") -async def workspace_presence(workspace_id: str, request: Request) -> StreamingResponse: +async def workspace_presence(workspace_id: UuidId, request: Request) -> StreamingResponse: # Raised, not returned: this route's contract is a stream, and refusing before # the generator starts keeps the failure a plain 403 rather than an SSE error event. denial = check_workspace_access(request, workspace_id, "viewer") @@ -306,7 +307,7 @@ async def _generate() -> AsyncGenerator[str, None]: @router.get("/workspaces/{workspace_id}/suggestions") def workspace_suggestions( - workspace_id: str, request: Request, workspaces_svc: Workspaces, top_k: int = 10 + workspace_id: UuidId, request: Request, workspaces_svc: Workspaces, top_k: int = 10 ) -> Any: denied = _deny(request, workspace_id, "viewer") if denied: @@ -322,7 +323,7 @@ def workspace_suggestions( @router.get("/workspaces/{workspace_id}/ontology") def workspace_ontology( - workspace_id: str, request: Request, workspaces_svc: Workspaces, top_n: int = 20 + workspace_id: UuidId, request: Request, workspaces_svc: Workspaces, top_n: int = 20 ) -> Any: denied = _deny(request, workspace_id, "viewer") if denied: @@ -344,7 +345,7 @@ def workspace_ontology( @router.get("/workspaces/{workspace_id}/keys") -def list_workspace_keys(workspace_id: str, request: Request, workspaces_svc: Workspaces) -> Any: +def list_workspace_keys(workspace_id: UuidId, request: Request, workspaces_svc: Workspaces) -> Any: denied = _deny(request, workspace_id, "owner") if denied: return denied @@ -357,7 +358,7 @@ def list_workspace_keys(workspace_id: str, request: Request, workspaces_svc: Wor @router.post("/workspaces/{workspace_id}/keys", status_code=201) -async def create_workspace_key(workspace_id: str, request: Request, workspaces_svc: Workspaces) -> Any: +async def create_workspace_key(workspace_id: UuidId, request: Request, workspaces_svc: Workspaces) -> Any: denied = _deny(request, workspace_id, "owner") if denied: return denied @@ -385,7 +386,7 @@ async def create_workspace_key(workspace_id: str, request: Request, workspaces_s @router.delete("/workspaces/{workspace_id}/keys/{key_id}") def revoke_workspace_key( - workspace_id: str, key_id: str, request: Request, workspaces_svc: Workspaces + workspace_id: UuidId, key_id: UuidId, request: Request, workspaces_svc: Workspaces ) -> Any: denied = _deny(request, workspace_id, "owner") if denied: diff --git a/src/security_fastapi.py b/src/security_fastapi.py index dc48ec81..47ce79f3 100644 --- a/src/security_fastapi.py +++ b/src/security_fastapi.py @@ -498,6 +498,20 @@ def _check_api_key_access( return None +def _is_uuid(value: str | None) -> bool: + """True for a UUID in its canonical form, and for no value at all: naming no workspace is allowed. + + Canonical only: ``uuid.UUID`` also accepts ``urn:uuid:`` forms, which Postgres refuses, + and the value reaches Postgres as written. + """ + if value is None: + return True + try: + return str(uuid.UUID(value)) == value.lower() + except ValueError: + return False + + def _first_workspace_for(db: Any, user_id: str | None) -> str | None: """The caller's own workspace to use when the request names none. @@ -530,6 +544,11 @@ def check_workspace_access( Pass ``workspace_id=None`` for a header-scoped route; scope then resolves via ``get_workspace_id`` and falls back to the default workspace. """ + # Whatever names the workspace: a path, the header, a query or a body. Unchecked, Postgres + # refused a malformed one inside the route and the caller got a 500. + if not _is_uuid(workspace_id if workspace_id is not None else get_workspace_id(request)): + return (status.HTTP_400_BAD_REQUEST, "Not a workspace id") + # A workspace API key is a principal in its own right, checked before user # claims because it is not a user and must not fall through to user handling. api_key = _extract_api_key(request) diff --git a/tests/integration/test_connector_routes.py b/tests/integration/test_connector_routes.py index 57fe0010..edc4b20a 100644 --- a/tests/integration/test_connector_routes.py +++ b/tests/integration/test_connector_routes.py @@ -24,14 +24,14 @@ _WEBHOOK_SECRET = "a-webhook-secret-long-enough" -def _webhook_connector(id="conn-1"): +def _webhook_connector(id="00000000-0000-4000-8000-00000000c001"): """A webhook connector as one must now be configured: with a secret.""" connector = _connector(id=id, connector_type="webhook") connector["config"] = {"secret": _WEBHOOK_SECRET} return connector -def _connector(id="conn-1", connector_type="local_folder", enabled=True): +def _connector(id="00000000-0000-4000-8000-00000000c001", connector_type="local_folder", enabled=True): return { "id": id, "workspace_id": None, @@ -96,12 +96,12 @@ def test_creates_local_folder_connector(self, client, app, tmp_path, monkeypatch # A local_folder connector is refused unless its path is inside a configured # root; the default empty allowlist disables the type (audit C4). monkeypatch.setattr("src.config.CONNECTOR_LOCAL_ROOTS", [str(tmp_path)]) - app.state.db.connectors.create = MagicMock(return_value="conn-new") - app.state.db.connectors.get = MagicMock(return_value=_connector(id="conn-new")) + app.state.db.connectors.create = MagicMock(return_value="00000000-0000-4000-8000-00000000c00e") + app.state.db.connectors.get = MagicMock(return_value=_connector(id="00000000-0000-4000-8000-00000000c00e")) resp = client.post("/api/connectors", json={ "connector_type": "local_folder", "config": {"path": str(tmp_path)}, - }, headers={"X-Workspace-ID": "ws-1"}) + }, headers={"X-Workspace-ID": "00000000-0000-4000-8000-000000000001"}) assert resp.status_code == 201 assert resp.json()["success"] is True @@ -150,13 +150,13 @@ class TestGetConnector: def test_returns_connector(self, client, app): app.state.db.connectors.get = MagicMock(return_value=_connector()) - resp = client.get("/api/connectors/conn-1") + resp = client.get("/api/connectors/00000000-0000-4000-8000-00000000c001") assert resp.status_code == 200 - assert resp.json()["connector"]["id"] == "conn-1" + assert resp.json()["connector"]["id"] == "00000000-0000-4000-8000-00000000c001" def test_not_found_returns_404(self, client, app): app.state.db.connectors.get = MagicMock(return_value=None) - resp = client.get("/api/connectors/missing") + resp = client.get("/api/connectors/00000000-0000-4000-8000-0000000000aa") assert resp.status_code == 404 @@ -169,30 +169,30 @@ class TestUpdateConnector: def test_updates_display_name(self, client, app): app.state.db.connectors.update = MagicMock(return_value=True) app.state.db.connectors.get = MagicMock( - return_value=_connector(id="conn-1") + return_value=_connector(id="00000000-0000-4000-8000-00000000c001") ) - resp = client.put("/api/connectors/conn-1", json={"display_name": "Renamed"}) + resp = client.put("/api/connectors/00000000-0000-4000-8000-00000000c001", json={"display_name": "Renamed"}) assert resp.status_code == 200 def test_no_valid_fields_returns_400(self, client, app): - resp = client.put("/api/connectors/conn-1", json={"owner": "bob"}) + resp = client.put("/api/connectors/00000000-0000-4000-8000-00000000c001", json={"owner": "bob"}) assert resp.status_code == 400 def test_not_found_returns_404(self, client, app): app.state.db.connectors.update = MagicMock(return_value=False) - resp = client.put("/api/connectors/missing", json={"display_name": "X"}) + resp = client.put("/api/connectors/00000000-0000-4000-8000-0000000000aa", json={"display_name": "X"}) assert resp.status_code == 404 def test_disabling_removes_from_registry(self, client, app): app.state.db.connectors.update = MagicMock(return_value=True) app.state.db.connectors.get = MagicMock( - return_value=_connector(id="conn-1", enabled=False) + return_value=_connector(id="00000000-0000-4000-8000-00000000c001", enabled=False) ) mock_registry = MagicMock() app.state.connector_registry = mock_registry - resp = client.put("/api/connectors/conn-1", json={"enabled": False}) + resp = client.put("/api/connectors/00000000-0000-4000-8000-00000000c001", json={"enabled": False}) assert resp.status_code == 200 - mock_registry.remove.assert_called_once_with("conn-1") + mock_registry.remove.assert_called_once_with("00000000-0000-4000-8000-00000000c001") # --------------------------------------------------------------------------- @@ -205,13 +205,13 @@ def test_deletes_connector(self, client, app): app.state.db.connectors.retire = MagicMock(return_value=True) mock_registry = MagicMock() app.state.connector_registry = mock_registry - resp = client.delete("/api/connectors/conn-1") + resp = client.delete("/api/connectors/00000000-0000-4000-8000-00000000c001") assert resp.status_code == 200 - mock_registry.remove.assert_called_once_with("conn-1") + mock_registry.remove.assert_called_once_with("00000000-0000-4000-8000-00000000c001") def test_not_found_returns_404(self, client, app): app.state.db.connectors.retire = MagicMock(return_value=False) - resp = client.delete("/api/connectors/missing") + resp = client.delete("/api/connectors/00000000-0000-4000-8000-0000000000aa") assert resp.status_code == 404 @@ -231,21 +231,21 @@ def test_triggers_sync(self, client, app): app.state.sync_worker = mock_worker app.state.connector_registry = MagicMock() app.state.connector_registry.get.return_value = mock_instance - resp = client.post("/api/connectors/conn-1/sync") + resp = client.post("/api/connectors/00000000-0000-4000-8000-00000000c001/sync") assert resp.status_code == 200 assert resp.json()["success"] is True def test_connector_not_loaded_returns_404(self, client, app): app.state.connector_registry = MagicMock() app.state.connector_registry.get.return_value = None - resp = client.post("/api/connectors/conn-1/sync") + resp = client.post("/api/connectors/00000000-0000-4000-8000-00000000c001/sync") assert resp.status_code == 404 def test_no_worker_returns_503(self, client, app): app.state.sync_worker = None app.state.connector_registry = MagicMock() app.state.connector_registry.get.return_value = MagicMock() - resp = client.post("/api/connectors/conn-1/sync") + resp = client.post("/api/connectors/00000000-0000-4000-8000-00000000c001/sync") assert resp.status_code == 503 @@ -266,7 +266,7 @@ def test_returns_history(self, client, app): "files_added": 3, "files_updated": 0, "files_deleted": 0, "error": None} ] app.state.db.connectors.sync_history = MagicMock(return_value=history) - resp = client.get("/api/connectors/conn-1/history") + resp = client.get("/api/connectors/00000000-0000-4000-8000-00000000c001/history") assert resp.status_code == 200 data = resp.json() assert len(data["history"]) == 1 @@ -274,9 +274,9 @@ def test_returns_history(self, client, app): def test_limit_capped_at_100(self, client, app): app.state.db.connectors.sync_history = MagicMock(return_value=[]) - resp = client.get("/api/connectors/conn-1/history?limit=999") + resp = client.get("/api/connectors/00000000-0000-4000-8000-00000000c001/history?limit=999") assert resp.status_code == 200 - app.state.db.connectors.sync_history.assert_called_once_with("conn-1", limit=100) + app.state.db.connectors.sync_history.assert_called_once_with("00000000-0000-4000-8000-00000000c001", limit=100) # --------------------------------------------------------------------------- @@ -291,7 +291,7 @@ def test_accepts_valid_added_event(self, client, app): mock_instance.push_event.return_value = [] app.state.connector_registry = MagicMock() app.state.connector_registry.get.return_value = mock_instance - resp = client.post("/api/connectors/conn-1/webhook", json={ + resp = client.post("/api/connectors/00000000-0000-4000-8000-00000000c001/webhook", json={ "event_type": "added", "source_id": "doc-1", "filename": "report.pdf", @@ -307,7 +307,7 @@ def test_a_connector_without_a_secret_accepts_nothing(self, client, app): app.state.db.connectors.get = MagicMock(return_value=connector) app.state.connector_registry = MagicMock() - resp = client.post("/api/connectors/conn-1/webhook", json={ + resp = client.post("/api/connectors/00000000-0000-4000-8000-00000000c001/webhook", json={ "event_type": "added", "source_id": "doc-1", "filename": "r.pdf", "fetch_url": "http://example.com/r.pdf", }) @@ -324,7 +324,7 @@ def test_every_refusal_before_the_secret_reads_the_same(self, client, app, found if connector is not None: connector["config"] = {"secret": "correct-secret"} app.state.db.connectors.get = MagicMock(return_value=connector) - resp = client.post("/api/connectors/conn-1/webhook", json={}, + resp = client.post("/api/connectors/00000000-0000-4000-8000-00000000c001/webhook", json={}, headers={"X-LocalChat-Secret": "wrong-secret"}) assert (resp.status_code, resp.json()) == (403, {"success": False, "message": "Forbidden"}) @@ -335,7 +335,7 @@ def test_invalid_payload_returns_400(self, client, app): app.state.connector_registry = MagicMock() app.state.connector_registry.get.return_value = mock_instance resp = client.post( - "/api/connectors/conn-1/webhook", json={}, + "/api/connectors/00000000-0000-4000-8000-00000000c001/webhook", json={}, headers={"X-LocalChat-Secret": _WEBHOOK_SECRET}, ) assert resp.status_code == 400 @@ -345,7 +345,7 @@ def test_bad_secret_returns_403(self, client, app): connector["config"] = {"secret": "correct-secret"} app.state.db.connectors.get = MagicMock(return_value=connector) resp = client.post( - "/api/connectors/conn-1/webhook", + "/api/connectors/00000000-0000-4000-8000-00000000c001/webhook", json={"event_type": "added", "source_id": "x", "fetch_url": "http://x"}, headers={"X-LocalChat-Secret": "wrong-secret"}, ) @@ -360,7 +360,7 @@ def test_correct_secret_accepted(self, client, app): app.state.connector_registry = MagicMock() app.state.connector_registry.get.return_value = mock_instance resp = client.post( - "/api/connectors/conn-1/webhook", + "/api/connectors/00000000-0000-4000-8000-00000000c001/webhook", json={"event_type": "deleted", "source_id": "doc-1"}, headers={"X-LocalChat-Secret": "correct-secret"}, ) @@ -371,7 +371,7 @@ def test_instance_not_active_returns_503(self, client, app): app.state.connector_registry = MagicMock() app.state.connector_registry.get.return_value = None resp = client.post( - "/api/connectors/conn-1/webhook", json={}, + "/api/connectors/00000000-0000-4000-8000-00000000c001/webhook", json={}, headers={"X-LocalChat-Secret": _WEBHOOK_SECRET}, ) assert resp.status_code == 503 diff --git a/tests/integration/test_malformed_ids_are_not_server_errors.py b/tests/integration/test_malformed_ids_are_not_server_errors.py new file mode 100644 index 00000000..1a339ff9 --- /dev/null +++ b/tests/integration/test_malformed_ids_are_not_server_errors.py @@ -0,0 +1,109 @@ +"""An id that cannot exist is refused as the caller's mistake, never a 500. + +A path parameter typed ``str`` reached Postgres as text, a ``UUID`` column refused +``not-a-uuid`` with ``invalid_text_representation``, and the route's own broad ``except`` +answered 500. Nothing leaked, but a 500 pages someone and says the server is broken when +the request is. A UUID path id is now typed ``UuidId`` and refused with a 422, as an ``int`` +one always was; a malformed ``X-Workspace-ID`` is a 400 from the workspace guard. + +Driven over HTTP as an admin, so every guard admits the request and it reaches the +database. The routes come from the same derivation as the object-authorisation matrix. +""" + +from __future__ import annotations + +from typing import Any + +import pytest + +pytestmark = [pytest.mark.integration, pytest.mark.db, pytest.mark.slow] + +httpx = pytest.importorskip("httpx", reason="driven over HTTP") + +from tests.e2e.conftest import ( # noqa: E402,F401 + ADMIN_PASSWORD, + ADMIN_USERNAME, + live_server, +) +from tests.integration.test_object_authorization_over_the_wire import ( # noqa: E402 + _BODIES, + _PARAM_RE, + _login, + _server_errors, + _status, + object_routes, + server_env, # noqa: F401 +) + +#: Not a UUID, and past bigint: between them every id column type refuses one. +_MALFORMED = ("not-a-uuid", "99999999999999999999") + + +@pytest.fixture(scope="module") +def admin(live_server: str) -> httpx.Client: + return _login(live_server, ADMIN_USERNAME, ADMIN_PASSWORD) + + +def _probe(admin: httpx.Client, method: str, path: str, value: str, **kwargs: Any) -> int | str: + url = _PARAM_RE.sub(value, path) + body = _BODIES.get((method, path), {}) if method in {"POST", "PUT", "PATCH"} else None + return _status(admin, method, url, json=body, **kwargs) + + +def test_a_malformed_id_in_the_path_is_never_a_server_error(admin: httpx.Client) -> None: + errors = { + f"{method} {path} [{value}]": status + for method, path in object_routes() + for value in _MALFORMED + if not isinstance(status := _probe(admin, method, path, value), int) or status >= 500 + } + assert errors == {}, _server_errors(admin) + + +def test_a_malformed_id_is_refused_at_the_boundary_and_a_well_formed_one_is_not( + admin: httpx.Client, +) -> None: + """A malformed UUID is a 422; a well-formed one nobody holds still reaches the database.""" + absent = "00000000-0000-0000-0000-000000000000" + statuses = [ + _probe(admin, "DELETE", "/api/users/{user_id}", "not-a-uuid"), + _probe(admin, "GET", "/api/conversations/{conversation_id}", "not-a-uuid"), + _probe(admin, "DELETE", "/api/users/{user_id}", absent), + _probe(admin, "GET", "/api/conversations/{conversation_id}", absent), + _probe(admin, "DELETE", "/api/documents/{doc_id}", "99999999999999999999"), + ] + assert statuses == [422, 422, 404, 404, 404], _server_errors(admin) + + +#: ``uuid.UUID`` accepts the urn form; Postgres does not. +_MALFORMED_WORKSPACES = ("not-a-uuid", "urn:uuid:00000000-0000-0000-0000-000000000000") + + +@pytest.mark.parametrize("value", _MALFORMED_WORKSPACES) +@pytest.mark.parametrize( + "method,path", + [("GET", "/api/documents/list"), ("GET", "/api/workspaces/active")], +) +def test_a_malformed_workspace_header_is_a_400( + admin: httpx.Client, method: str, path: str, value: str +) -> None: + """``/workspaces/active`` hands the header to the guard itself; the others leave it to the guard.""" + status = _status(admin, method, path, headers={"X-Workspace-ID": value}) + assert status == 400, _server_errors(admin) + + +@pytest.mark.parametrize("value", _MALFORMED_WORKSPACES) +def test_a_malformed_workspace_in_a_body_is_a_400(admin: httpx.Client, value: str) -> None: + status = _status(admin, "POST", "/api/workspaces/switch", json={"workspace_id": value}) + assert status == 400, _server_errors(admin) + + +def test_the_callers_own_workspace_is_still_admitted(admin: httpx.Client) -> None: + """The control: without it, a guard refusing every workspace would pass the tests above.""" + own = admin.get("/api/workspaces").json()["workspaces"][0]["id"] + statuses = [ + _status(admin, "GET", "/api/documents/list", headers={"X-Workspace-ID": own}), + _status(admin, "GET", "/api/workspaces/active", headers={"X-Workspace-ID": own}), + _status(admin, "POST", "/api/workspaces/switch", json={"workspace_id": own}), + ] + assert statuses == [200, 200, 200], _server_errors(admin) diff --git a/tests/integration/test_workspace_routes.py b/tests/integration/test_workspace_routes.py index 99e57c36..93b20051 100644 --- a/tests/integration/test_workspace_routes.py +++ b/tests/integration/test_workspace_routes.py @@ -19,7 +19,7 @@ # Helpers # --------------------------------------------------------------------------- -def _workspace(id="ws-1", name="Test WS", active=False): +def _workspace(id="00000000-0000-4000-8000-000000000001", name="Test WS", active=False): return { "id": id, "name": name, @@ -53,9 +53,9 @@ def test_returns_workspaces_list(self, client, app): assert len(data["workspaces"]) == 1 def test_active_flag_set_on_matching_workspace(self, client, app): - ws = _workspace(id="ws-active") + ws = _workspace(id="00000000-0000-4000-8000-0000000000ac") app.state.db.workspaces.list_all = MagicMock(return_value=[ws]) - resp = client.get("/api/workspaces", headers={"X-Workspace-ID": "ws-active"}) + resp = client.get("/api/workspaces", headers={"X-Workspace-ID": "00000000-0000-4000-8000-0000000000ac"}) data = resp.json() assert data["workspaces"][0]["active"] is True @@ -72,13 +72,13 @@ def test_db_error_returns_500(self, client, app): class TestCreateWorkspace: def test_creates_workspace_returns_201(self, client, app): - app.state.db.workspaces.create = MagicMock(return_value="ws-new") - app.state.db.workspaces.get = MagicMock(return_value=_workspace(id="ws-new", name="New")) + app.state.db.workspaces.create = MagicMock(return_value="00000000-0000-4000-8000-0000000000ee") + app.state.db.workspaces.get = MagicMock(return_value=_workspace(id="00000000-0000-4000-8000-0000000000ee", name="New")) resp = client.post("/api/workspaces", json={"name": "New"}) assert resp.status_code == 201 data = resp.json() assert data["success"] is True - assert data["workspace"]["id"] == "ws-new" + assert data["workspace"]["id"] == "00000000-0000-4000-8000-0000000000ee" def test_missing_name_returns_400(self, client, app): resp = client.post("/api/workspaces", json={}) @@ -89,8 +89,8 @@ def test_empty_name_returns_400(self, client, app): assert resp.status_code == 400 def test_passes_optional_fields(self, client, app): - app.state.db.workspaces.create = MagicMock(return_value="ws-2") - app.state.db.workspaces.get = MagicMock(return_value=_workspace(id="ws-2")) + app.state.db.workspaces.create = MagicMock(return_value="00000000-0000-4000-8000-000000000002") + app.state.db.workspaces.get = MagicMock(return_value=_workspace(id="00000000-0000-4000-8000-000000000002")) resp = client.post("/api/workspaces", json={ "name": "Legal", "description": "Legal docs", @@ -119,13 +119,13 @@ class TestGetWorkspace: def test_returns_workspace(self, client, app): app.state.db.workspaces.get = MagicMock(return_value=_workspace()) - resp = client.get("/api/workspaces/ws-1") + resp = client.get("/api/workspaces/00000000-0000-4000-8000-000000000001") assert resp.status_code == 200 - assert resp.json()["workspace"]["id"] == "ws-1" + assert resp.json()["workspace"]["id"] == "00000000-0000-4000-8000-000000000001" def test_not_found_returns_404(self, client, app): app.state.db.workspaces.get = MagicMock(return_value=None) - resp = client.get("/api/workspaces/missing") + resp = client.get("/api/workspaces/00000000-0000-4000-8000-0000000000aa") assert resp.status_code == 404 @@ -138,17 +138,17 @@ class TestUpdateWorkspace: def test_updates_and_returns_workspace(self, client, app): app.state.db.workspaces.update = MagicMock(return_value=True) app.state.db.workspaces.get = MagicMock(return_value=_workspace(name="Renamed")) - resp = client.put("/api/workspaces/ws-1", json={"name": "Renamed"}) + resp = client.put("/api/workspaces/00000000-0000-4000-8000-000000000001", json={"name": "Renamed"}) assert resp.status_code == 200 assert resp.json()["workspace"]["name"] == "Renamed" def test_no_valid_fields_returns_400(self, client, app): - resp = client.put("/api/workspaces/ws-1", json={"owner": "bob"}) + resp = client.put("/api/workspaces/00000000-0000-4000-8000-000000000001", json={"owner": "bob"}) assert resp.status_code == 400 def test_not_found_returns_404(self, client, app): app.state.db.workspaces.update = MagicMock(return_value=False) - resp = client.put("/api/workspaces/ws-x", json={"name": "X"}) + resp = client.put("/api/workspaces/00000000-0000-4000-8000-0000000000ff", json={"name": "X"}) assert resp.status_code == 404 @@ -161,24 +161,24 @@ class TestDeleteWorkspace: def test_deletes_workspace(self, client, app): app.state.db.members.role = MagicMock(return_value=None) app.state.db.workspaces.retire = MagicMock(return_value=True) - app.state.db.workspaces.default_id = MagicMock(return_value="ws-default") - resp = client.delete("/api/workspaces/ws-1") + app.state.db.workspaces.default_id = MagicMock(return_value="00000000-0000-4000-8000-0000000000de") + resp = client.delete("/api/workspaces/00000000-0000-4000-8000-000000000001") assert resp.status_code == 200 assert resp.json()["success"] is True def test_not_found_returns_404(self, client, app): app.state.db.members.role = MagicMock(return_value=None) app.state.db.workspaces.retire = MagicMock(return_value=False) - resp = client.delete("/api/workspaces/missing") + resp = client.delete("/api/workspaces/00000000-0000-4000-8000-0000000000aa") assert resp.status_code == 404 def test_response_includes_fallback_workspace_id(self, client, app): app.state.db.members.role = MagicMock(return_value=None) app.state.db.workspaces.retire = MagicMock(return_value=True) - app.state.db.workspaces.default_id = MagicMock(return_value="ws-default") - resp = client.delete("/api/workspaces/ws-1") + app.state.db.workspaces.default_id = MagicMock(return_value="00000000-0000-4000-8000-0000000000de") + resp = client.delete("/api/workspaces/00000000-0000-4000-8000-000000000001") assert resp.status_code == 200 - assert resp.json()["fallback_workspace_id"] == "ws-default" + assert resp.json()["fallback_workspace_id"] == "00000000-0000-4000-8000-0000000000de" def test_non_owner_member_returns_403(self, client, app): # Needs the RBAC bypass off to mean anything. It previously passed with the @@ -198,15 +198,15 @@ def test_non_owner_member_returns_403(self, client, app): token = create_access_token("33333333-3333-3333-3333-333333333333", {"role": "user"}) with patch("src.security_fastapi._ADMIN_PASSWORD_RAW", "set-so-rbac-is-live"): resp = client.delete( - "/api/workspaces/ws-1", headers={"Authorization": f"Bearer {token}"} + "/api/workspaces/00000000-0000-4000-8000-000000000001", headers={"Authorization": f"Bearer {token}"} ) assert resp.status_code == 403 def test_owner_can_delete(self, client, app): app.state.db.members.role = MagicMock(return_value="owner") app.state.db.workspaces.retire = MagicMock(return_value=True) - app.state.db.workspaces.default_id = MagicMock(return_value="ws-default") - resp = client.delete("/api/workspaces/ws-1") + app.state.db.workspaces.default_id = MagicMock(return_value="00000000-0000-4000-8000-0000000000de") + resp = client.delete("/api/workspaces/00000000-0000-4000-8000-000000000001") assert resp.status_code == 200 @@ -223,10 +223,10 @@ def test_returns_none_when_no_active(self, client, app): assert data["workspace"] is None def test_returns_active_workspace(self, client, app): - app.state.db.workspaces.get = MagicMock(return_value=_workspace(id="ws-1")) - resp = client.get("/api/workspaces/active", headers={"X-Workspace-ID": "ws-1"}) + app.state.db.workspaces.get = MagicMock(return_value=_workspace(id="00000000-0000-4000-8000-000000000001")) + resp = client.get("/api/workspaces/active", headers={"X-Workspace-ID": "00000000-0000-4000-8000-000000000001"}) data = resp.json() - assert data["workspace"]["id"] == "ws-1" + assert data["workspace"]["id"] == "00000000-0000-4000-8000-000000000001" # --------------------------------------------------------------------------- @@ -236,8 +236,8 @@ def test_returns_active_workspace(self, client, app): class TestSwitchWorkspace: def test_switches_successfully(self, client, app): - app.state.db.workspaces.get = MagicMock(return_value=_workspace(id="ws-2", name="Proj")) - resp = client.post("/api/workspaces/switch", json={"workspace_id": "ws-2"}) + app.state.db.workspaces.get = MagicMock(return_value=_workspace(id="00000000-0000-4000-8000-000000000002", name="Proj")) + resp = client.post("/api/workspaces/switch", json={"workspace_id": "00000000-0000-4000-8000-000000000002"}) assert resp.status_code == 200 data = resp.json() assert data["success"] is True @@ -249,5 +249,5 @@ def test_missing_workspace_id_returns_400(self, client, app): def test_not_found_returns_404(self, client, app): app.state.db.workspaces.get = MagicMock(return_value=None) - resp = client.post("/api/workspaces/switch", json={"workspace_id": "ws-ghost"}) + resp = client.post("/api/workspaces/switch", json={"workspace_id": "00000000-0000-4000-8000-0000000000a0"}) assert resp.status_code == 404 diff --git a/tests/unit/test_auth_routes_unit.py b/tests/unit/test_auth_routes_unit.py index 02d56cc6..e51feb6c 100644 --- a/tests/unit/test_auth_routes_unit.py +++ b/tests/unit/test_auth_routes_unit.py @@ -52,14 +52,14 @@ def test_list_users_db_error_returns_500(self, client, app): def test_get_user_db_error_returns_500(self, client, app): app.state.db.users.get_by_id = MagicMock(side_effect=Exception("db gone")) - resp = client.get('/api/users/nonexistent-id') + resp = client.get('/api/users/00000000-0000-4000-8000-000000001001') assert resp.status_code == 500 assert resp.json()['message'] == 'Internal server error' def test_update_user_db_error_returns_500(self, client, app): app.state.db.users.update = MagicMock(side_effect=Exception("db gone")) resp = client.put( - '/api/users/some-id', + '/api/users/00000000-0000-4000-8000-000000001002', json={'email': 'new@example.com'}, ) assert resp.status_code == 500 @@ -67,7 +67,7 @@ def test_update_user_db_error_returns_500(self, client, app): def test_delete_user_db_error_returns_500(self, client, app): app.state.db.users.retire = MagicMock(side_effect=Exception("db gone")) - resp = client.delete('/api/users/some-id') + resp = client.delete('/api/users/00000000-0000-4000-8000-000000001002') assert resp.status_code == 500 assert resp.json()['message'] == 'Internal server error' diff --git a/tests/unit/test_connector_routes_unit.py b/tests/unit/test_connector_routes_unit.py index 937d066d..b3df15bc 100644 --- a/tests/unit/test_connector_routes_unit.py +++ b/tests/unit/test_connector_routes_unit.py @@ -14,14 +14,14 @@ def test_list_connectors_db_error_returns_500(self, client, app): def test_get_connector_db_error_returns_500(self, client, app): app.state.db.connectors.get = MagicMock(side_effect=Exception("db gone")) - resp = client.get('/api/connectors/some-id') + resp = client.get('/api/connectors/00000000-0000-4000-8000-000000000101') assert resp.status_code == 500 assert resp.json()['message'] == 'Internal server error' def test_update_connector_db_error_returns_500(self, client, app): app.state.db.connectors.update = MagicMock(side_effect=Exception("db gone")) resp = client.put( - '/api/connectors/some-id', + '/api/connectors/00000000-0000-4000-8000-000000000101', json={'display_name': 'new name'}, ) assert resp.status_code == 500 @@ -29,13 +29,13 @@ def test_update_connector_db_error_returns_500(self, client, app): def test_delete_connector_db_error_returns_500(self, client, app): app.state.db.connectors.retire = MagicMock(side_effect=Exception("db gone")) - resp = client.delete('/api/connectors/some-id') + resp = client.delete('/api/connectors/00000000-0000-4000-8000-000000000101') assert resp.status_code == 500 assert resp.json()['message'] == 'Internal server error' def test_sync_history_db_error_returns_500(self, client, app): app.state.db.connectors.sync_history = MagicMock(side_effect=Exception("db gone")) - resp = client.get('/api/connectors/some-id/history') + resp = client.get('/api/connectors/00000000-0000-4000-8000-000000000101/history') assert resp.status_code == 500 assert resp.json()['message'] == 'Internal server error' @@ -56,7 +56,7 @@ def test_create_connector_db_error_returns_500(self, client, app): resp = client.post( '/api/connectors', json={'connector_type': 'webhook', 'config': {}}, - headers={'X-Workspace-ID': 'ws-1'}, + headers={'X-Workspace-ID': '00000000-0000-4000-8000-000000000105'}, ) assert resp.status_code == 500 assert resp.json()['message'] == 'Internal server error' @@ -88,32 +88,32 @@ class TestAConnectorOutsideTheScopeIsNotFound: def test_history_of_a_foreign_connector_is_not_found(self, client, app): app.state.db.connectors.get = MagicMock(return_value=None) app.state.db.connectors.sync_history = MagicMock(return_value=[{"id": 1}]) - resp = client.get('/api/connectors/foreign/history', headers={'X-Workspace-ID': 'ws-a'}) + resp = client.get('/api/connectors/00000000-0000-4000-8000-000000000102/history', headers={'X-Workspace-ID': '00000000-0000-4000-8000-000000000104'}) assert resp.status_code == 404 app.state.db.connectors.sync_history.assert_not_called() def test_history_looks_the_connector_up_in_the_authorised_scope(self, client, app): - app.state.db.connectors.get = MagicMock(return_value={'id': 'own'}) + app.state.db.connectors.get = MagicMock(return_value={'id': '00000000-0000-4000-8000-000000000103'}) app.state.db.connectors.sync_history = MagicMock(return_value=[{"id": 1}]) - resp = client.get('/api/connectors/own/history', headers={'X-Workspace-ID': 'ws-a'}) + resp = client.get('/api/connectors/00000000-0000-4000-8000-000000000103/history', headers={'X-Workspace-ID': '00000000-0000-4000-8000-000000000104'}) assert resp.json()['history'] == [{"id": 1}] - assert app.state.db.connectors.get.call_args.kwargs['scope'] == 'ws-a' + assert app.state.db.connectors.get.call_args.kwargs['scope'] == '00000000-0000-4000-8000-000000000104' def test_a_foreign_connector_is_not_synced(self, client, app): app.state.db.connectors.get = MagicMock(return_value=None) app.state.connector_registry = MagicMock() app.state.sync_worker = MagicMock() - resp = client.post('/api/connectors/foreign/sync', headers={'X-Workspace-ID': 'ws-a'}) + resp = client.post('/api/connectors/00000000-0000-4000-8000-000000000102/sync', headers={'X-Workspace-ID': '00000000-0000-4000-8000-000000000104'}) assert resp.status_code == 404 app.state.connector_registry.get.assert_not_called() def test_the_callers_own_connector_is_synced(self, client, app): - app.state.db.connectors.get = MagicMock(return_value={'id': 'own'}) + app.state.db.connectors.get = MagicMock(return_value={'id': '00000000-0000-4000-8000-000000000103'}) app.state.connector_registry = MagicMock() app.state.sync_worker = MagicMock() - resp = client.post('/api/connectors/own/sync', headers={'X-Workspace-ID': 'ws-a'}) + resp = client.post('/api/connectors/00000000-0000-4000-8000-000000000103/sync', headers={'X-Workspace-ID': '00000000-0000-4000-8000-000000000104'}) assert resp.status_code == 200 - assert app.state.db.connectors.get.call_args.kwargs['scope'] == 'ws-a' + assert app.state.db.connectors.get.call_args.kwargs['scope'] == '00000000-0000-4000-8000-000000000104' class TestConnectorOwnerBinding: @@ -170,7 +170,7 @@ def test_create_binds_the_caller_as_owner(self, client, app): resp = client.post( '/api/connectors', json={'connector_type': 'google_drive', 'config': {'folder_id': 'f1'}}, - headers={'X-Workspace-ID': 'ws-1'}, + headers={'X-Workspace-ID': '00000000-0000-4000-8000-000000000105'}, ) assert resp.status_code == 201 @@ -182,7 +182,7 @@ def test_update_rejects_a_client_supplied_owner(self, client, app): app.state.db.connectors.update = MagicMock(return_value=True) resp = client.put( - '/api/connectors/some-id', + '/api/connectors/00000000-0000-4000-8000-000000000101', json={'config': {'user_id': 'someone-else'}}, ) @@ -194,11 +194,11 @@ def test_reregistration_after_update_carries_the_stored_owner(self, client, app) registry = self._registry(app) app.state.db.connectors.update = MagicMock(return_value=True) app.state.db.connectors.get = MagicMock(return_value={ - 'id': 'some-id', 'connector_type': 'google_drive', 'config': {}, - 'enabled': True, 'workspace_id': 'ws-1', 'created_by': 'creator', + 'id': '00000000-0000-4000-8000-000000000101', 'connector_type': 'google_drive', 'config': {}, + 'enabled': True, 'workspace_id': '00000000-0000-4000-8000-000000000105', 'created_by': 'creator', }) - resp = client.put('/api/connectors/some-id', json={'enabled': True}) + resp = client.put('/api/connectors/00000000-0000-4000-8000-000000000101', json={'enabled': True}) assert resp.status_code == 200 assert registry.add.call_args.kwargs['owner_user_id'] == 'creator' diff --git a/tests/unit/test_cw2_soft_delete.py b/tests/unit/test_cw2_soft_delete.py index 12a4cf83..b17a9d62 100644 --- a/tests/unit/test_cw2_soft_delete.py +++ b/tests/unit/test_cw2_soft_delete.py @@ -9,18 +9,18 @@ class TestDeleteConversation: def test_soft_delete_returns_200(self, client, app): app.state.db.conversations.retire = MagicMock(return_value=True) - resp = client.delete("/api/conversations/some-uuid") + resp = client.delete("/api/conversations/00000000-0000-4000-8000-000000001103") assert resp.status_code == 200 assert resp.json()["success"] is True def test_delete_missing_conversation_returns_404(self, client, app): app.state.db.conversations.retire = MagicMock(return_value=False) - resp = client.delete("/api/conversations/ghost-uuid") + resp = client.delete("/api/conversations/00000000-0000-4000-8000-000000001102") assert resp.status_code == 404 def test_delete_calls_db_with_deleted_by(self, client, app): app.state.db.conversations.retire = MagicMock(return_value=True) - client.delete("/api/conversations/abc-123") + client.delete("/api/conversations/00000000-0000-4000-8000-000000001101") app.state.db.conversations.retire.assert_called_once() call_kwargs = app.state.db.conversations.retire.call_args # Second positional arg or deleted_by kwarg must be present @@ -30,13 +30,13 @@ def test_delete_calls_db_with_deleted_by(self, client, app): class TestPurgeConversation: def test_purge_succeeds_when_no_citations(self, client, app): app.state.db.conversations.purge = MagicMock(return_value=True) - resp = client.delete("/api/conversations/some-uuid/purge") + resp = client.delete("/api/conversations/00000000-0000-4000-8000-000000001103/purge") assert resp.status_code == 200 assert resp.json()["success"] is True def test_purge_blocked_by_memories_returns_409(self, client, app): app.state.db.conversations.purge = MagicMock(return_value=False) - resp = client.delete("/api/conversations/some-uuid/purge") + resp = client.delete("/api/conversations/00000000-0000-4000-8000-000000001103/purge") assert resp.status_code == 409 assert "purged" in resp.json()["message"].lower() @@ -67,44 +67,44 @@ class TestDeleteUser: def test_soft_delete_returns_200(self, client, app, monkeypatch): _not_the_last_admin(app, monkeypatch) app.state.db.users.retire = MagicMock(return_value=True) - resp = client.delete("/api/users/some-user-id") + resp = client.delete("/api/users/00000000-0000-4000-8000-000000001106") assert resp.status_code == 200 assert resp.json()["success"] is True def test_delete_missing_user_returns_404(self, client, app, monkeypatch): _not_the_last_admin(app, monkeypatch) app.state.db.users.retire = MagicMock(return_value=False) - resp = client.delete("/api/users/ghost-id") + resp = client.delete("/api/users/00000000-0000-4000-8000-000000001104") assert resp.status_code == 404 def test_delete_calls_db_with_deleted_by(self, client, app, monkeypatch): _not_the_last_admin(app, monkeypatch) app.state.db.users.retire = MagicMock(return_value=True) - client.delete("/api/users/some-user-id") + client.delete("/api/users/00000000-0000-4000-8000-000000001106") app.state.db.users.retire.assert_called_once() call_kwargs = app.state.db.users.retire.call_args assert call_kwargs is not None def test_delete_db_error_returns_500(self, client, app): app.state.db.users.retire = MagicMock(side_effect=Exception("db gone")) - resp = client.delete("/api/users/some-id") + resp = client.delete("/api/users/00000000-0000-4000-8000-000000001105") assert resp.status_code == 500 class TestPurgeUser: def test_purge_succeeds_when_no_memberships(self, client, app): app.state.db.users.purge = MagicMock(return_value=True) - resp = client.delete("/api/users/some-user-id/purge") + resp = client.delete("/api/users/00000000-0000-4000-8000-000000001106/purge") assert resp.status_code == 200 assert resp.json()["success"] is True def test_purge_blocked_by_memberships_returns_409(self, client, app): app.state.db.users.purge = MagicMock(return_value=False) - resp = client.delete("/api/users/some-user-id/purge") + resp = client.delete("/api/users/00000000-0000-4000-8000-000000001106/purge") assert resp.status_code == 409 assert "purged" in resp.json()["message"].lower() def test_purge_db_error_returns_500(self, client, app): app.state.db.users.purge = MagicMock(side_effect=Exception("db gone")) - resp = client.delete("/api/users/some-user-id/purge") + resp = client.delete("/api/users/00000000-0000-4000-8000-000000001106/purge") assert resp.status_code == 500 diff --git a/tests/unit/test_cw2cdef_soft_delete.py b/tests/unit/test_cw2cdef_soft_delete.py index 76a182b8..d24f13d0 100644 --- a/tests/unit/test_cw2cdef_soft_delete.py +++ b/tests/unit/test_cw2cdef_soft_delete.py @@ -11,34 +11,34 @@ def test_soft_delete_returns_200(self, client, app): app.state.db.members.role = MagicMock(return_value=None) app.state.db.workspaces.retire = MagicMock(return_value=True) app.state.db.workspaces.default_id = MagicMock(return_value="fallback-id") - resp = client.delete("/api/workspaces/some-ws-id") + resp = client.delete("/api/workspaces/00000000-0000-4000-8000-000000001205") assert resp.status_code == 200 assert resp.json()["success"] is True def test_delete_missing_workspace_returns_404(self, client, app): app.state.db.members.role = MagicMock(return_value=None) app.state.db.workspaces.retire = MagicMock(return_value=False) - resp = client.delete("/api/workspaces/ghost-id") + resp = client.delete("/api/workspaces/00000000-0000-4000-8000-000000001201") assert resp.status_code == 404 def test_delete_passes_deleted_by(self, client, app): app.state.db.members.role = MagicMock(return_value=None) app.state.db.workspaces.retire = MagicMock(return_value=True) app.state.db.workspaces.default_id = MagicMock(return_value=None) - client.delete("/api/workspaces/some-ws-id") + client.delete("/api/workspaces/00000000-0000-4000-8000-000000001205") app.state.db.workspaces.retire.assert_called_once() class TestPurgeWorkspace: def test_purge_succeeds_when_empty(self, client, app): app.state.db.workspaces.purge = MagicMock(return_value=True) - resp = client.delete("/api/workspaces/some-ws-id/purge") + resp = client.delete("/api/workspaces/00000000-0000-4000-8000-000000001205/purge") assert resp.status_code == 200 assert resp.json()["success"] is True def test_purge_blocked_returns_409(self, client, app): app.state.db.workspaces.purge = MagicMock(return_value=False) - resp = client.delete("/api/workspaces/some-ws-id/purge") + resp = client.delete("/api/workspaces/00000000-0000-4000-8000-000000001205/purge") assert resp.status_code == 409 assert "purged" in resp.json()["message"].lower() @@ -50,13 +50,13 @@ def test_purge_blocked_returns_409(self, client, app): class TestDeleteMemory: def test_soft_delete_returns_200(self, client, app): app.state.db.memories.retire = MagicMock(return_value=True) - resp = client.delete("/api/memory/some-memory-id") + resp = client.delete("/api/memory/00000000-0000-4000-8000-000000001204") assert resp.status_code == 200 assert resp.json()["success"] is True def test_delete_missing_memory_returns_404(self, client, app): app.state.db.memories.retire = MagicMock(return_value=False) - resp = client.delete("/api/memory/ghost-id") + resp = client.delete("/api/memory/00000000-0000-4000-8000-000000001201") assert resp.status_code == 404 def test_delete_all_returns_count(self, client, app): @@ -73,18 +73,18 @@ def test_delete_all_returns_count(self, client, app): class TestDeleteAnnotation: def test_soft_delete_returns_200(self, client, app): app.state.db.annotations.retire = MagicMock(return_value=True) - resp = client.delete("/api/annotations/some-ann-id") + resp = client.delete("/api/annotations/00000000-0000-4000-8000-000000001202") assert resp.status_code == 200 assert resp.json()["success"] is True def test_delete_missing_annotation_returns_404(self, client, app): app.state.db.annotations.retire = MagicMock(return_value=False) - resp = client.delete("/api/annotations/ghost-id") + resp = client.delete("/api/annotations/00000000-0000-4000-8000-000000001201") assert resp.status_code == 404 def test_delete_passes_deleted_by(self, client, app): app.state.db.annotations.retire = MagicMock(return_value=True) - client.delete("/api/annotations/some-ann-id") + client.delete("/api/annotations/00000000-0000-4000-8000-000000001202") app.state.db.annotations.retire.assert_called_once() @@ -96,33 +96,33 @@ class TestDeleteConnector: def test_soft_delete_returns_200(self, client, app): app.state.db.connectors.retire = MagicMock(return_value=True) app.state.connector_registry = MagicMock() - resp = client.delete("/api/connectors/some-conn-id") + resp = client.delete("/api/connectors/00000000-0000-4000-8000-000000001203") assert resp.status_code == 200 assert resp.json()["success"] is True def test_delete_missing_connector_returns_404(self, client, app): app.state.db.connectors.retire = MagicMock(return_value=False) app.state.connector_registry = MagicMock() - resp = client.delete("/api/connectors/ghost-id") + resp = client.delete("/api/connectors/00000000-0000-4000-8000-000000001201") assert resp.status_code == 404 def test_registry_remove_called_on_success(self, client, app): app.state.db.connectors.retire = MagicMock(return_value=True) registry = MagicMock() app.state.connector_registry = registry - client.delete("/api/connectors/some-conn-id") - registry.remove.assert_called_once_with("some-conn-id") + client.delete("/api/connectors/00000000-0000-4000-8000-000000001203") + registry.remove.assert_called_once_with("00000000-0000-4000-8000-000000001203") class TestPurgeConnector: def test_purge_succeeds_when_no_documents(self, client, app): app.state.db.connectors.purge = MagicMock(return_value=True) - resp = client.delete("/api/connectors/some-conn-id/purge") + resp = client.delete("/api/connectors/00000000-0000-4000-8000-000000001203/purge") assert resp.status_code == 200 assert resp.json()["success"] is True def test_purge_blocked_by_documents_returns_409(self, client, app): app.state.db.connectors.purge = MagicMock(return_value=False) - resp = client.delete("/api/connectors/some-conn-id/purge") + resp = client.delete("/api/connectors/00000000-0000-4000-8000-000000001203/purge") assert resp.status_code == 409 assert "purged" in resp.json()["message"].lower() diff --git a/tests/unit/test_document_routes_full.py b/tests/unit/test_document_routes_full.py index 40b5236c..4ddb1b68 100644 --- a/tests/unit/test_document_routes_full.py +++ b/tests/unit/test_document_routes_full.py @@ -194,9 +194,9 @@ def test_stats_passes_workspace_id_to_db(self, client, app): app.state.db.documents.count = MagicMock(return_value=2) app.state.db.documents.chunk_count = MagicMock(return_value=10) app.state.db.documents.chunk_statistics = MagicMock(return_value={}) - client.get('/api/documents/stats', headers={'X-Workspace-ID': 'ws-abc'}) - app.state.db.documents.count.assert_called_once_with(scope='ws-abc') - app.state.db.documents.chunk_count.assert_called_once_with(scope='ws-abc') + client.get('/api/documents/stats', headers={'X-Workspace-ID': '00000000-0000-4000-8000-000000000901'}) + app.state.db.documents.count.assert_called_once_with(scope='00000000-0000-4000-8000-000000000901') + app.state.db.documents.chunk_count.assert_called_once_with(scope='00000000-0000-4000-8000-000000000901') def test_stats_no_workspace_header_passes_all_workspaces(self, client, app): app.state.db.documents.count = MagicMock(return_value=5) diff --git a/tests/unit/test_fastapi_routes.py b/tests/unit/test_fastapi_routes.py index 341e1d43..8575d61d 100644 --- a/tests/unit/test_fastapi_routes.py +++ b/tests/unit/test_fastapi_routes.py @@ -83,7 +83,7 @@ def test_delete_user_not_found(self): state.db.users.retire.return_value = False state.db.users.count_active_admins.return_value = 2 client = _make_client(router, "/api", state) - resp = client.delete("/api/users/missing-id") + resp = client.delete("/api/users/00000000-0000-4000-8000-000000000201") assert resp.status_code == 404 @@ -168,7 +168,7 @@ def test_delete_annotation_not_found(self): state = _base_state() state.db.annotations.retire.return_value = False client = _make_client(router, "/api", state) - resp = client.delete("/api/annotations/99") + resp = client.delete("/api/annotations/00000000-0000-4000-8000-000000000202") assert resp.status_code == 404 @@ -184,7 +184,7 @@ def _client(self): state = _base_state() state.db.conversations.list_page.return_value = [] state.db.conversations.count.return_value = 0 - state.db.conversations.create.return_value = "conv-1" + state.db.conversations.create.return_value = "00000000-0000-4000-8000-000000000204" state.db.conversations.messages.return_value = [] state.db.conversations.retire.return_value = True state.db.conversations.retire_all.return_value = 2 @@ -203,7 +203,7 @@ def test_create_conversation(self): client = self._client() resp = client.post("/api/conversations", json={"title": "My chat"}) assert resp.status_code == 201 - assert resp.json()["id"] == "conv-1" + assert resp.json()["id"] == "00000000-0000-4000-8000-000000000204" def test_get_conversation_not_found(self): from src.routes_fastapi.memory_routes import router @@ -211,18 +211,18 @@ def test_get_conversation_not_found(self): state = _base_state() state.db.conversations.messages.return_value = None client = _make_client(router, "/api", state) - resp = client.get("/api/conversations/missing") + resp = client.get("/api/conversations/00000000-0000-4000-8000-000000000203") assert resp.status_code == 404 def test_delete_conversation(self): client = self._client() - resp = client.delete("/api/conversations/conv-1") + resp = client.delete("/api/conversations/00000000-0000-4000-8000-000000000204") assert resp.status_code == 200 assert resp.json()["success"] is True def test_update_conversation_title_empty(self): client = self._client() - resp = client.patch("/api/conversations/conv-1", json={"title": ""}) + resp = client.patch("/api/conversations/00000000-0000-4000-8000-000000000204", json={"title": ""}) assert resp.status_code == 400 @@ -311,7 +311,7 @@ def test_delete_memory(self): state = _base_state() state.db.memories.retire.return_value = True client = _make_client(router, "/api/memory", state) - resp = client.delete("/api/memory/mem-id-1") + resp = client.delete("/api/memory/00000000-0000-4000-8000-000000000205") assert resp.status_code == 200 diff --git a/tests/unit/test_fastapi_routes_extended.py b/tests/unit/test_fastapi_routes_extended.py index ea804eac..f8c5e629 100644 --- a/tests/unit/test_fastapi_routes_extended.py +++ b/tests/unit/test_fastapi_routes_extended.py @@ -241,8 +241,8 @@ def _client(self): from src.routes_fastapi.workspace_routes import router state = _base_state() - state.db.workspaces.list_all.return_value = [{"id": "ws-1", "name": "Acme"}] - state.db.workspaces.get.return_value = {"id": "ws-1", "name": "Acme"} + state.db.workspaces.list_all.return_value = [{"id": "00000000-0000-4000-8000-000000000301", "name": "Acme"}] + state.db.workspaces.get.return_value = {"id": "00000000-0000-4000-8000-000000000301", "name": "Acme"} state.db.workspaces.create.return_value = "ws-new" state.db.workspaces.update.return_value = True state.db.workspaces.retire.return_value = True @@ -282,11 +282,11 @@ def test_get_active_workspace_none(self): def test_get_workspace_found(self): client = self._client() - resp = client.get("/api/workspaces/ws-1") + resp = client.get("/api/workspaces/00000000-0000-4000-8000-000000000301") assert resp.status_code == 200 data = resp.json() assert data["success"] is True - assert data["workspace"]["id"] == "ws-1" + assert data["workspace"]["id"] == "00000000-0000-4000-8000-000000000301" def test_get_workspace_not_found(self): from src.routes_fastapi.workspace_routes import router @@ -294,23 +294,23 @@ def test_get_workspace_not_found(self): state = _base_state() state.db.workspaces.get.return_value = None client = _make_client(router, "/api", state) - resp = client.get("/api/workspaces/missing") + resp = client.get("/api/workspaces/00000000-0000-4000-8000-000000000302") assert resp.status_code == 404 def test_update_workspace_no_fields(self): client = self._client() - resp = client.put("/api/workspaces/ws-1", json={"unknown_field": "x"}) + resp = client.put("/api/workspaces/00000000-0000-4000-8000-000000000301", json={"unknown_field": "x"}) assert resp.status_code == 400 def test_update_workspace_success(self): client = self._client() - resp = client.put("/api/workspaces/ws-1", json={"name": "New Name"}) + resp = client.put("/api/workspaces/00000000-0000-4000-8000-000000000301", json={"name": "New Name"}) assert resp.status_code == 200 assert resp.json()["success"] is True def test_delete_workspace(self): client = self._client() - resp = client.delete("/api/workspaces/ws-1") + resp = client.delete("/api/workspaces/00000000-0000-4000-8000-000000000301") assert resp.status_code == 200 data = resp.json() assert data["success"] is True @@ -323,24 +323,24 @@ def test_delete_workspace_not_found(self): state.db.workspaces.retire.return_value = False state.db.members.role.return_value = None client = _make_client(router, "/api", state) - resp = client.delete("/api/workspaces/ghost") + resp = client.delete("/api/workspaces/00000000-0000-4000-8000-000000000303") assert resp.status_code == 404 def test_list_workspace_members(self): client = self._client() - resp = client.get("/api/workspaces/ws-1/members") + resp = client.get("/api/workspaces/00000000-0000-4000-8000-000000000301/members") assert resp.status_code == 200 assert resp.json()["success"] is True assert resp.json()["members"] == [] def test_add_workspace_member_missing_user_id(self): client = self._client() - resp = client.post("/api/workspaces/ws-1/members", json={"role": "viewer"}) + resp = client.post("/api/workspaces/00000000-0000-4000-8000-000000000301/members", json={"role": "viewer"}) assert resp.status_code == 400 def test_add_workspace_member_invalid_role(self): client = self._client() - resp = client.post("/api/workspaces/ws-1/members", json={"user_id": "u1", "role": "superadmin"}) + resp = client.post("/api/workspaces/00000000-0000-4000-8000-000000000301/members", json={"user_id": "u1", "role": "superadmin"}) assert resp.status_code == 400 def test_switch_workspace_missing_id(self): @@ -350,7 +350,7 @@ def test_switch_workspace_missing_id(self): def test_switch_workspace_success(self): client = self._client() - resp = client.post("/api/workspaces/switch", json={"workspace_id": "ws-1"}) + resp = client.post("/api/workspaces/switch", json={"workspace_id": "00000000-0000-4000-8000-000000000301"}) assert resp.status_code == 200 assert resp.json()["success"] is True diff --git a/tests/unit/test_local_folder_least_privilege.py b/tests/unit/test_local_folder_least_privilege.py index d396305c..c0e9a31e 100644 --- a/tests/unit/test_local_folder_least_privilege.py +++ b/tests/unit/test_local_folder_least_privilege.py @@ -154,9 +154,9 @@ def test_a_workspace_owner_may_not_create_one(self, allowed_root): def test_an_admin_may_create_one_inside_an_allowed_root(self, allowed_root): client = _client(member_role="owner", global_role="admin") - client.app.state.db.connectors.create.return_value = "c-1" + client.app.state.db.connectors.create.return_value = "00000000-0000-4000-8000-000000000401" client.app.state.db.connectors.get.return_value = { - "id": "c-1", + "id": "00000000-0000-4000-8000-000000000401", "connector_type": "local_folder", "config": {"path": str(allowed_root)}, "workspace_id": WS, @@ -193,13 +193,13 @@ class TestReconfiguringIsTheSameDecision: def test_an_owner_may_not_repoint_a_local_folder_connector(self, allowed_root): client = _client(member_role="owner", global_role="user") client.app.state.db.connectors.get.return_value = { - "id": "c-1", + "id": "00000000-0000-4000-8000-000000000401", "connector_type": "local_folder", "config": {"path": str(allowed_root)}, "workspace_id": WS, } resp = client.put( - "/api/connectors/c-1", json={"config": {"path": "/etc"}}, headers=_auth() + "/api/connectors/00000000-0000-4000-8000-000000000401", json={"config": {"path": "/etc"}}, headers=_auth() ) assert resp.status_code == 403 assert not client.app.state.db.connectors.update.called @@ -207,14 +207,14 @@ def test_an_owner_may_not_repoint_a_local_folder_connector(self, allowed_root): def test_an_admin_repointing_outside_the_roots_is_refused(self, allowed_root): client = _client(member_role="owner", global_role="admin") client.app.state.db.connectors.get.return_value = { - "id": "c-1", + "id": "00000000-0000-4000-8000-000000000401", "connector_type": "local_folder", "config": {"path": str(allowed_root)}, "workspace_id": WS, } with patch("src.routes_fastapi.connector_routes.require_admin_dep", return_value=USER): resp = client.put( - "/api/connectors/c-1", json={"config": {"path": "/etc"}}, headers=_auth("admin") + "/api/connectors/00000000-0000-4000-8000-000000000401", json={"config": {"path": "/etc"}}, headers=_auth("admin") ) assert resp.status_code == 400 assert not client.app.state.db.connectors.update.called @@ -222,7 +222,7 @@ def test_an_admin_repointing_outside_the_roots_is_refused(self, allowed_root): def test_an_admin_may_repoint_within_the_roots(self, allowed_root): client = _client(member_role="owner", global_role="admin") client.app.state.db.connectors.get.return_value = { - "id": "c-1", + "id": "00000000-0000-4000-8000-000000000401", "connector_type": "local_folder", "config": {"path": str(allowed_root)}, "workspace_id": WS, @@ -230,7 +230,7 @@ def test_an_admin_may_repoint_within_the_roots(self, allowed_root): client.app.state.db.connectors.update.return_value = True with patch("src.routes_fastapi.connector_routes.require_admin_dep", return_value=USER): resp = client.put( - "/api/connectors/c-1", + "/api/connectors/00000000-0000-4000-8000-000000000401", json={"config": {"path": str(allowed_root / "inner")}}, headers=_auth("admin"), ) @@ -242,13 +242,13 @@ def test_a_rename_does_not_require_admin(self, allowed_root): client = _client(member_role="owner", global_role="user") client.app.state.db.connectors.update.return_value = True client.app.state.db.connectors.get.return_value = { - "id": "c-1", + "id": "00000000-0000-4000-8000-000000000401", "connector_type": "local_folder", "config": {"path": str(allowed_root)}, "workspace_id": WS, "enabled": False, } resp = client.put( - "/api/connectors/c-1", json={"display_name": "Renamed"}, headers=_auth() + "/api/connectors/00000000-0000-4000-8000-000000000401", json={"display_name": "Renamed"}, headers=_auth() ) assert resp.status_code == 200 diff --git a/tests/unit/test_longterm_memory_routes.py b/tests/unit/test_longterm_memory_routes.py index 38e63b86..b0fd66fc 100644 --- a/tests/unit/test_longterm_memory_routes.py +++ b/tests/unit/test_longterm_memory_routes.py @@ -202,26 +202,26 @@ def test_outer_exception_returns_500(self): class TestDeleteMemory: def test_returns_200_on_success(self): _, client = _make_app() - resp = client.delete("/api/memory/some-uuid") + resp = client.delete("/api/memory/00000000-0000-4000-8000-000000000501") assert resp.status_code == 200 assert resp.json()["success"] is True def test_calls_db_delete_with_id(self): app, client = _make_app() - client.delete("/api/memory/abc-123") + client.delete("/api/memory/00000000-0000-4000-8000-000000000502") # deleted_by names the authenticated caller. It read None while the RBAC # bypass was on, so this assertion documented the bypass rather than the # Clark-Wilson audit trail it exists for. from src.utils.scope import ALL_WORKSPACES kwargs = app.state.db.memories.retire.call_args.kwargs - assert app.state.db.memories.retire.call_args.args == ("abc-123",) + assert app.state.db.memories.retire.call_args.args == ("00000000-0000-4000-8000-000000000502",) assert kwargs["deleted_by"] == ADMIN_ID assert kwargs["scope"] is ALL_WORKSPACES def test_db_raises_returns_500(self): app, client = _make_app() app.state.db.memories.retire.side_effect = Exception("not found") - resp = client.delete("/api/memory/bad-id") + resp = client.delete("/api/memory/00000000-0000-4000-8000-000000000503") assert resp.status_code == 500 diff --git a/tests/unit/test_memory_routes_full.py b/tests/unit/test_memory_routes_full.py index 776d239f..604ed3f1 100644 --- a/tests/unit/test_memory_routes_full.py +++ b/tests/unit/test_memory_routes_full.py @@ -48,30 +48,30 @@ def test_create_returns_conversation_id(self, client, app): class TestGetConversation: def test_get_existing_conversation(self, client, app): app.state.db.conversations.messages = MagicMock(return_value=[]) - response = client.get('/api/conversations/abc-123') + response = client.get('/api/conversations/00000000-0000-4000-8000-000000001501') assert response.status_code in (200, 404) def test_get_nonexistent_conversation(self, client, app): app.state.db.conversations.messages = MagicMock(return_value=None) - response = client.get('/api/conversations/ghost-id') + response = client.get('/api/conversations/00000000-0000-4000-8000-000000001502') assert response.status_code in (404, 200) class TestRenameConversation: def test_rename_conversation(self, client, app): app.state.db.conversations.set_title = MagicMock(return_value=True) - response = client.patch('/api/conversations/abc-123', + response = client.patch('/api/conversations/00000000-0000-4000-8000-000000001501', json={'title': 'New Name'}) assert response.status_code in (200, 404) def test_rename_missing_title(self, client, app): - response = client.patch('/api/conversations/abc-123', + response = client.patch('/api/conversations/00000000-0000-4000-8000-000000001501', json={}) assert response.status_code in (200, 400, 404) def test_rename_not_found(self, client, app): app.state.db.conversations.set_title = MagicMock(return_value=False) - response = client.patch('/api/conversations/ghost', + response = client.patch('/api/conversations/00000000-0000-4000-8000-000000001503', json={'title': 'x'}) assert response.status_code in (200, 404) @@ -79,10 +79,10 @@ def test_rename_not_found(self, client, app): class TestDeleteConversation: def test_delete_existing_conversation(self, client, app): app.state.db.conversations.retire = MagicMock(return_value=True) - response = client.delete('/api/conversations/abc-123') + response = client.delete('/api/conversations/00000000-0000-4000-8000-000000001501') assert response.status_code in (200, 204) def test_delete_nonexistent_conversation(self, client, app): app.state.db.conversations.retire = MagicMock(return_value=False) - response = client.delete('/api/conversations/ghost') + response = client.delete('/api/conversations/00000000-0000-4000-8000-000000001503') assert response.status_code in (200, 404) diff --git a/tests/unit/test_object_authorization_matrix.py b/tests/unit/test_object_authorization_matrix.py index a89b139e..7d9d53b6 100644 --- a/tests/unit/test_object_authorization_matrix.py +++ b/tests/unit/test_object_authorization_matrix.py @@ -379,7 +379,7 @@ def test_memory_delete_is_scoped(self): client = _client(router, "/api/memory", "editor") client.app.state.db.memories.retire.return_value = True - client.delete("/api/memory/abc-123", headers=_auth()) + client.delete("/api/memory/00000000-0000-4000-8000-000000000601", headers=_auth()) assert client.app.state.db.memories.retire.call_args.kwargs["scope"] == WS def test_annotation_delete_is_scoped(self): @@ -387,7 +387,7 @@ def test_annotation_delete_is_scoped(self): client = _client(router, "/api", "editor") client.app.state.db.annotations.retire.return_value = True - client.delete("/api/annotations/abc-123", headers=_auth()) + client.delete("/api/annotations/00000000-0000-4000-8000-000000000601", headers=_auth()) assert client.app.state.db.annotations.retire.call_args.kwargs["scope"] == WS def test_connector_read_is_scoped(self): @@ -395,7 +395,7 @@ def test_connector_read_is_scoped(self): client = _client(router, "/api", "owner") client.app.state.db.connectors.get.return_value = None - client.get("/api/connectors/abc-123", headers=_auth()) + client.get("/api/connectors/00000000-0000-4000-8000-000000000601", headers=_auth()) assert client.app.state.db.connectors.get.call_args.kwargs["scope"] == WS diff --git a/tests/unit/test_plugin_contract.py b/tests/unit/test_plugin_contract.py index 6f8d8125..41230bc9 100644 --- a/tests/unit/test_plugin_contract.py +++ b/tests/unit/test_plugin_contract.py @@ -443,8 +443,8 @@ def _db(*, member_role: str | None = "editor", global_role: str | None = "user") db.is_connected = True db.members.role.return_value = member_role db.members.for_user.return_value = [] - db.workspaces.default_id.return_value = "ws-1" - db.api_keys.resolve.return_value = ("ws-1", "viewer") + db.workspaces.default_id.return_value = "00000000-0000-4000-8000-000000001301" + db.api_keys.resolve.return_value = ("00000000-0000-4000-8000-000000001301", "viewer") db.tokens.is_revoked.return_value = False db.users.get_role.return_value = global_role return db @@ -453,12 +453,12 @@ def _db(*, member_role: str | None = "editor", global_role: str | None = "user") class TestGuardRecordsTheCaller: def test_a_member_is_recorded_with_their_workspace_role(self): req = _request(token=create_access_token(_USER, {"role": "user"}), db=_db(member_role="owner")) - assert check_workspace_access(req, "ws-1", "viewer") is None + assert check_workspace_access(req, "00000000-0000-4000-8000-000000001301", "viewer") is None assert get_caller(req) == RequestCaller(user_id=_USER, role="owner", via_api_key=False) def test_a_global_admin_is_recorded_as_admin(self): req = _request(token=create_access_token(_USER, {"role": "admin"}), db=_db(global_role="admin")) - assert check_workspace_access(req, "ws-1", "viewer") is None + assert check_workspace_access(req, "00000000-0000-4000-8000-000000001301", "viewer") is None assert get_caller(req) == RequestCaller(user_id=_USER, role="admin", via_api_key=False) def test_a_token_claiming_admin_is_recorded_at_its_database_role(self): @@ -466,15 +466,15 @@ def test_a_token_claiming_admin_is_recorded_at_its_database_role(self): holding an old token is recorded as the member they now are.""" req = _request(token=create_access_token(_USER, {"role": "admin"}), db=_db(member_role="viewer", global_role="user")) - assert check_workspace_access(req, "ws-1", "viewer") is None + assert check_workspace_access(req, "00000000-0000-4000-8000-000000001301", "viewer") is None assert get_caller(req) == RequestCaller(user_id=_USER, role="viewer", via_api_key=False) def test_an_api_key_is_recorded_with_its_role_and_no_user(self): req = _request(api_key="lcw_abc", db=_db()) - assert check_workspace_access(req, "ws-1", "viewer") is None + assert check_workspace_access(req, "00000000-0000-4000-8000-000000001301", "viewer") is None assert get_caller(req) == RequestCaller(user_id=None, role="viewer", via_api_key=True) def test_a_refused_caller_is_not_recorded(self): req = _request(token=create_access_token(_USER, {"role": "user"}), db=_db(member_role=None)) - assert check_workspace_access(req, "ws-1", "viewer") is not None + assert check_workspace_access(req, "00000000-0000-4000-8000-000000001301", "viewer") is not None assert get_caller(req) is None diff --git a/tests/unit/test_rbac2_route_permissions.py b/tests/unit/test_rbac2_route_permissions.py index 331665c4..0cb3b7b3 100644 --- a/tests/unit/test_rbac2_route_permissions.py +++ b/tests/unit/test_rbac2_route_permissions.py @@ -107,12 +107,12 @@ def test_owner_may_list(self): def test_non_member_may_not_trigger_sync(self): client = _client("connector_routes", "/api", member_role=None) - assert client.post("/api/connectors/c1/sync", headers=_auth()).status_code == 403 + assert client.post("/api/connectors/00000000-0000-4000-8000-000000000701/sync", headers=_auth()).status_code == 403 def test_webhook_stays_open(self): """Deliberate: external systems POST here without a bearer token.""" client = _client("connector_routes", "/api", member_role=None) - assert client.post("/api/connectors/c1/webhook", json={}).status_code != 401 + assert client.post("/api/connectors/00000000-0000-4000-8000-000000000701/webhook", json={}).status_code != 401 @pytest.mark.unit diff --git a/tests/unit/test_users_me_identity.py b/tests/unit/test_users_me_identity.py index 96a11a05..bd397b72 100644 --- a/tests/unit/test_users_me_identity.py +++ b/tests/unit/test_users_me_identity.py @@ -31,6 +31,9 @@ def _client(member_role: str | None, global_role: str = "user"): state.db.api_keys.resolve.return_value = None state.db.members.role.return_value = member_role state.db.workspaces.default_id.return_value = WS + # Not a bare mock: a truthy one becomes the workspace the guard pins, and the + # second check this route makes then refuses it as no workspace id. + state.db.members.for_user.return_value = [] state.db.users.get_by_id.return_value = {"id": USER, "username": "jo", "role": global_role} app = FastAPI() app.include_router(router, prefix="/api") diff --git a/tests/unit/test_workspace_access_contract.py b/tests/unit/test_workspace_access_contract.py index 5515bc3b..e6cbc2dc 100644 --- a/tests/unit/test_workspace_access_contract.py +++ b/tests/unit/test_workspace_access_contract.py @@ -26,7 +26,7 @@ pytestmark = pytest.mark.unit _USER = "88888888-8888-8888-8888-888888888888" -_WS = "ws-1" +_WS = "00000000-0000-4000-8000-000000001401" def _request(*, token: str | None = None, db=None, api_key: str | None = None) -> MagicMock: @@ -129,6 +129,37 @@ def test_no_resolvable_workspace_says_so(self): assert denial == (400, "No workspace context") +class TestAMalformedWorkspaceIsRefusedBeforeAnyLookup: + """PR #546 review: Postgres refused these inside the route, and the caller got a 500.""" + + @pytest.mark.parametrize("value", [ + "not-a-uuid", + # uuid.UUID accepts both; Postgres refuses the urn form and the value reaches it as written. + "urn:uuid:" + _WS, + "{" + _WS + "}", + ]) + def test_a_named_workspace_that_is_not_a_canonical_uuid_is_a_400(self, value): + db = _member_db("owner") + assert check_workspace_access(_request(token=_user_token(), db=db), value, "viewer") == ( + 400, "Not a workspace id" + ) + db.members.role.assert_not_called() + + def test_a_malformed_header_is_a_400(self): + req = _request(token=_user_token(), db=_member_db("owner")) + req.state.resolved_workspace_id = None + headers = {"X-Workspace-ID": "urn:uuid:" + _WS} + req.headers.get = lambda k, default="": headers.get(k, default) + assert check_workspace_access(req, None, "viewer") == (400, "Not a workspace id") + + def test_an_upper_case_uuid_is_still_admitted(self): + """Over-correction guard: Postgres reads either case, so refusing it would lock out + a client that sends one.""" + assert check_workspace_access( + _request(token=_user_token(), db=_member_db("viewer")), _WS.upper(), "viewer" + ) is None + + class TestDenialMessages: """Mutants 153, 156, 162, 167, 186, 201 — what the caller is actually told. @@ -147,7 +178,7 @@ def test_a_key_for_another_workspace_is_named_as_such(self): """The rule that keeps a key scoped: a requested workspace may be compared against the key's own, never substituted for it.""" assert _check_api_key_access( - _request(db=_key_db(workspace="ws-other")), "lcw_x", _WS, "viewer" + _request(db=_key_db(workspace="00000000-0000-4000-8000-000000001402")), "lcw_x", _WS, "viewer" ) == (403, "API key is not valid for this workspace") def test_an_underpowered_key_names_the_role_it_needs(self): diff --git a/tests/unit/test_workspace_api_keys.py b/tests/unit/test_workspace_api_keys.py index 9dcdbf5e..3dd6ba0e 100644 --- a/tests/unit/test_workspace_api_keys.py +++ b/tests/unit/test_workspace_api_keys.py @@ -188,10 +188,10 @@ def _client(self, member_role: str = "owner"): state = authenticated_state(role="admin", member_role=member_role) state.db.api_keys.create.return_value = ( "lcw_plaintext_shown_once", - {"id": "k1", "name": "discord-bridge", "key_prefix": "lcw_plain", "role": "viewer"}, + {"id": "00000000-0000-4000-8000-000000000801", "name": "discord-bridge", "key_prefix": "lcw_plain", "role": "viewer"}, ) state.db.api_keys.list_for_workspace.return_value = [ - {"id": "k1", "name": "discord-bridge", "key_prefix": "lcw_plain", "role": "viewer"} + {"id": "00000000-0000-4000-8000-000000000801", "name": "discord-bridge", "key_prefix": "lcw_plain", "role": "viewer"} ] state.db.api_keys.revoke.return_value = True app = FastAPI() @@ -219,6 +219,6 @@ def test_owner_role_cannot_be_issued_to_a_key(self): def test_revoke_scopes_the_delete_to_the_workspace(self): client = self._client() - client.delete(f"/api/workspaces/{WS_A}/keys/k1") + client.delete(f"/api/workspaces/{WS_A}/keys/00000000-0000-4000-8000-000000000801") kwargs = client.app.state.db.api_keys.revoke.call_args.kwargs assert kwargs["workspace_id"] == WS_A diff --git a/tests/unit/test_workspace_role_enforcement.py b/tests/unit/test_workspace_role_enforcement.py index 9d8f0946..cee21b46 100644 --- a/tests/unit/test_workspace_role_enforcement.py +++ b/tests/unit/test_workspace_role_enforcement.py @@ -90,7 +90,7 @@ def test_viewer_may_list_conversations(self): def test_viewer_may_export_a_conversation(self): """Decision: export reformats what a viewer can already read.""" - resp = _conversations("viewer").get("/api/conversations/c1/export", headers=_auth()) + resp = _conversations("viewer").get("/api/conversations/00000000-0000-4000-8000-000000001601/export", headers=_auth()) assert resp.status_code != 403 def test_viewer_may_not_create_a_conversation(self):