From ca27b2c2a41155163c09a6862c807ee9538fbdfe Mon Sep 17 00:00:00 2001 From: Alexander Date: Mon, 27 Jul 2026 10:51:57 -0400 Subject: [PATCH] fix(security): pin the destructive /api/memory routes as ADMIN above the prefix rule (#1024) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reduced onto main after 610150d7 (#1357) landed the #1302 half of the original PR #1359 independently. Everything that overlapped is dropped — main owns the /mcp/memory bulk-delete approval gate (make_dispatcher's approval_queue, delegating classification to admin.is_gated) and the /api/memory auth-posture comment. What is left is the slice 610150d7 does not touch at all: it never edits src/hal0/security/exposure.py. #1024's incident was one unauthenticated `DELETE /api/memory/banks/{id}` cascade-deleting ~632 live records. Its confirm guard (#1028) and audit row (#1030) landed; the classification did not. `/api/memory` was ADMIN only by generic prefix, and nothing asserted it — while #1024's own follow-up proposes "keep reads open if desired", whose natural expression (`_prefix("/api/memory")` -> CLIENT) silently takes the bank wipe with it under first-match-wins. Verified: with the pinned rows removed, that widening downgrades all 8 destructive routes to CLIENT. - exposure.py: `_DELETE`, two narrow rules ordered ABOVE the generic `memory` row, and `DESTRUCTIVE_MEMORY_ROUTES` as the named ratchet. Zero behaviour change today, by design — the rows exist so the classification cannot be widened without a conscious diff. - tests/security/test_memory_delete_auth.py: the constant tracks the live route table; each route resolves via a pinned rule (not the ADMIN fallback); all survive a simulated reads-are-CLIENT widening; and against the real armed AuthEnforcementMiddleware an anonymous call 401s, a client key 403s, and the admin key clears the gate. Note the posture this pins is only enforced when auth is enabled: `require_auth_enabled()` ships OFF by default (operator decision 2026-07-19, finding O19), so on a default install these routes are still reachable unauthenticated. This makes the classification correct and un-widenable; it does not by itself close #1024 on a default box. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 4 +- docs/reference/api/rest-api.mdx | 8 + src/hal0/security/exposure.py | 44 ++++ tests/security/test_memory_delete_auth.py | 277 ++++++++++++++++++++++ 4 files changed, 332 insertions(+), 1 deletion(-) create mode 100644 tests/security/test_memory_delete_auth.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 828323186..5ca6dad0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,9 @@ applying. Add those subsections to a version's section to surface them; see ## [Unreleased] -_Nothing yet._ +### Security + +- **Pin the destructive `/api/memory` routes as ADMIN, above the generic prefix rule** (closes the last open slice of #1024). #1024's incident was a single unauthenticated `DELETE /api/memory/banks/{bank_id}` that cascade-deleted ~632 live records; its echoed-`?confirm=` guard (#1028) and audit row (#1030) landed, but the classification itself was only ever ADMIN *by generic prefix* and nothing asserted it. That matters because #1024's own follow-up proposes "keep reads open if desired", and the natural expression of that — a `_prefix("/api/memory")` CLIENT rule — silently takes the bank wipe with it under first-match-wins. Two narrow rules (`any DELETE under /api/memory`, `POST /api/memory/delete`) now sit *above* the generic memory row, and `exposure.DESTRUCTIVE_MEMORY_ROUTES` enumerates the irreversible surface so a new memory delete route, or a reclassification, has to touch the constant in the same diff. `tests/security/test_memory_delete_auth.py` asserts the constant tracks the live route table, that each route resolves via a pinned rule (not the ADMIN fallback), that all of them survive a simulated reads-are-CLIENT widening, and — against the real `AuthEnforcementMiddleware`, armed — that an anonymous call gets `401`, a client/inference key gets `403`, and the operator's admin key clears the gate. ## [1.0.0-rc.1] — 2026-07-27 (R5 · the rework release) diff --git a/docs/reference/api/rest-api.mdx b/docs/reference/api/rest-api.mdx index 7f6a883cd..5309d01fe 100644 --- a/docs/reference/api/rest-api.mdx +++ b/docs/reference/api/rest-api.mdx @@ -95,6 +95,14 @@ See [Services](/docs/operate/services) for the operator-facing guide (registry, ### `/api/memory` — hardening +- The whole `/api/memory` surface — reads included — is **ADMIN**-classed in + the route exposure table, so once auth is enabled every call needs an admin + credential (session cookie or admin bearer); a client/inference key gets + `403`. The irreversible subset (every `DELETE` under `/api/memory`, plus the + namespace `POST /api/memory/delete`) is additionally pinned by name in + `DESTRUCTIVE_MEMORY_ROUTES` and matched by rules that sit *above* the generic + prefix rule, so a later decision to open memory reads cannot take the bank + wipe with it. - Every destructive `/api/memory/*` operation — bank delete, and the memories/config/document/directive/operation/mental-model deletes, plus the namespace `POST /api/memory/delete` — records a durable audit row (actor + diff --git a/src/hal0/security/exposure.py b/src/hal0/security/exposure.py index d07c3443c..b1b20d5c2 100644 --- a/src/hal0/security/exposure.py +++ b/src/hal0/security/exposure.py @@ -130,6 +130,7 @@ def applies(self, method: str, path: str) -> bool: _GET: frozenset[str] = frozenset({"GET", "HEAD"}) _POST: frozenset[str] = frozenset({"POST"}) _PUT: frozenset[str] = frozenset({"PUT"}) +_DELETE: frozenset[str] = frozenset({"DELETE"}) # Ordered, first-match-wins. See the module docstring for the full design # rationale and hal0-rework-plan.md §23.5 for the architecture this @@ -241,6 +242,22 @@ def applies(self, method: str, path: str) -> bool: _Rule("services", _prefix("/api/services"), AuthClass.ADMIN, None), _Rule("settings", _prefix("/api/settings"), AuthClass.ADMIN, None), _Rule("secrets", _prefix("/api/secrets"), AuthClass.ADMIN, None), + # ── memory: the irreversible subset, pinned AHEAD of the generic rule ── + # + # Issues #1024 (a single unauthenticated `DELETE /api/memory/banks/{id}` + # cascade-deleted ~632 live records) and #1302 (delete surface hardening). + # The whole `/api/memory` prefix is ADMIN (rule immediately below), so + # these two rows change nothing today — that is the point. They exist so + # the classification of the *destructive* memory routes cannot be widened + # by accident: #1024's own follow-up comment proposes "keep reads open if + # desired", and the natural expression of that ("/api/memory reads are + # CLIENT") is a `_prefix("/api/memory")` rule which, written without a + # method filter or placed above the generic row, would silently take the + # bank wipe with it. First-match-wins means these resolve ADMIN before any + # such rule is consulted, and DESTRUCTIVE_MEMORY_ROUTES (below, next to + # OPEN_ALLOWLIST) is the CI ratchet that forces a conscious diff. + _Rule("memory destructive (any DELETE)", _prefix("/api/memory"), AuthClass.ADMIN, _DELETE), + _Rule("memory namespace bulk delete", _exact("/api/memory/delete"), AuthClass.ADMIN, _POST), _Rule("memory", _prefix("/api/memory"), AuthClass.ADMIN, None), _Rule("board", _prefix("/api/board"), AuthClass.ADMIN, None), # hal0-brain steward chat (R4 §G): primary /api/brain/chat surface, sibling @@ -337,7 +354,34 @@ def classify(method: str, path: str) -> AuthClass: ) +# The memory routes that destroy stored data irreversibly (issues #1024, +# #1302), as (method, FastAPI path template) pairs. Every one of these MUST +# classify ADMIN and MUST NOT appear in OPEN_ALLOWLIST; +# ``tests/security/test_memory_delete_auth.py`` asserts both against the +# live route table AND asserts this set is exactly the destructive memory +# surface the app actually serves — so adding a new memory delete route, or +# reclassifying an existing one, has to touch this constant in the same diff. +# +# ``POST /api/memory/delete`` is here because it is a *bulk* id-scoped delete +# (body ``{"ids": [...]}``), not a create — the verb hides the blast radius. +DESTRUCTIVE_MEMORY_ROUTES: frozenset[tuple[str, str]] = frozenset( + { + # The #1024 incident route: cascade-drops every memory, document and + # entity in the bank. + ("DELETE", "/api/memory/banks/{bank_id}"), + ("DELETE", "/api/memory/banks/{bank_id}/config"), + ("DELETE", "/api/memory/banks/{bank_id}/directives/{directive_id}"), + ("DELETE", "/api/memory/banks/{bank_id}/documents/{document_id}"), + ("DELETE", "/api/memory/banks/{bank_id}/memories"), + ("DELETE", "/api/memory/banks/{bank_id}/mental-models/{model_id}"), + ("DELETE", "/api/memory/banks/{bank_id}/operations/{operation_id}"), + ("POST", "/api/memory/delete"), + } +) + + __all__ = [ + "DESTRUCTIVE_MEMORY_ROUTES", "OPEN_ALLOWLIST", "RULES", "AuthClass", diff --git a/tests/security/test_memory_delete_auth.py b/tests/security/test_memory_delete_auth.py new file mode 100644 index 000000000..e8351fb68 --- /dev/null +++ b/tests/security/test_memory_delete_auth.py @@ -0,0 +1,277 @@ +"""Issues #1024 / #1302 (1): the memory delete surface is ADMIN, provably. + +#1024's incident was ``DELETE /api/memory/banks/{bank_id}`` reached with one +unauthenticated curl, cascade-deleting ~632 live records. Two of that +issue's three hardening asks landed already — the echoed-``?confirm=`` guard +(#1028) and the ``record_action`` audit row (#1030) — and both have their own +coverage in ``tests/api/test_memory_admin_routes.py``. The third, "reintroduce +write-auth on mutating ``/api/memory/*`` routes", was left open pending a +posture decision, and #1302 restates it. + +The posture, encoded here: **the whole ``/api/memory`` surface is ADMIN, and +its irreversible subset is pinned by name so it cannot be widened by +accident.** ``hal0.security.exposure`` already classified the prefix ADMIN, +but nothing anywhere asserted it for the destructive routes, and #1024's own +follow-up comment proposes "keep reads open if desired" — a widening whose +natural expression (``_prefix("/api/memory")`` for reads) silently takes the +bank wipe with it unless the destructive rows are pinned first. + +So this file covers three distinct claims: + +1. ``DESTRUCTIVE_MEMORY_ROUTES`` is exactly the destructive memory surface + the app really serves (adding one forces a diff to the constant). +2. Each of those routes classifies ADMIN, is absent from ``OPEN_ALLOWLIST``, + and *stays* ADMIN under a simulated memory-reads-are-CLIENT widening. +3. The real ``AuthEnforcementMiddleware``, armed, 401s an anonymous delete + and lets an admin bearer through — golden path plus negative control, + the shape ``tests/agents/test_hermes_provision_mcp_auth.py`` established. +""" + +from __future__ import annotations + +import pytest +from fastapi.testclient import TestClient + +from hal0.api import create_app +from hal0.security import exposure +from hal0.security.exposure import ( + DESTRUCTIVE_MEMORY_ROUTES, + OPEN_ALLOWLIST, + AuthClass, + classify, + match_rule, +) +from tests.security.test_exposure import _enumerate_routes + +ADMIN_KEY = "memory-delete-admin-key" +CLIENT_KEY = "memory-delete-client-key" + +#: Concrete, grammar-valid substitutions for every ``{...}`` path arg that +#: appears in a DESTRUCTIVE_MEMORY_ROUTES template. They must satisfy +#: ``memory_admin._BANK_RE`` / ``_SEG_RE`` so a request that clears the auth +#: gate reaches a real handler — otherwise a "not 401" assertion could be +#: satisfied by a 400 from path validation instead. +_PATH_ARGS = { + "bank_id": "shared", + "document_id": "doc-1", + "directive_id": "dir-1", + "model_id": "mm-1", + "operation_id": "op-1", +} + + +def _url(path: str) -> str: + return path.format(**_PATH_ARGS) + + +# ── 1. the constant tracks the live surface ────────────────────────────────── + + +@pytest.fixture(scope="module") +def memory_routes(tmp_path_factory: pytest.TempPathFactory) -> set[tuple[str, str]]: + """Every ``(method, template)`` the live app serves under /api/memory.""" + hal0_home = tmp_path_factory.mktemp("memory-delete-auth-home") + with pytest.MonkeyPatch.context() as monkeypatch: + monkeypatch.setenv("HAL0_HOME", str(hal0_home)) + monkeypatch.setattr("hal0.api._mount_dashboard", lambda _app: None) + app = create_app() + routes = {(m, p) for m, p in _enumerate_routes(app) if p.startswith("/api/memory")} + assert routes, "no /api/memory routes mounted — the walker or the router moved" + return routes + + +def test_destructive_memory_routes_tracks_the_live_route_table( + memory_routes: set[tuple[str, str]], +) -> None: + """The constant is the destructive memory surface — exactly. + + "Destructive" = any DELETE under /api/memory, plus the one POST that + hides a bulk delete behind a create-shaped verb (``POST + /api/memory/delete``, body ``{"ids": [...]}``). A new memory delete + route that nobody pinned trips this, which is the point: the ratchet + forces the classification question to be answered in the same diff. + """ + live_destructive = { + (method, path) + for method, path in memory_routes + if method == "DELETE" or (method == "POST" and path.endswith("/delete")) + } + assert live_destructive == DESTRUCTIVE_MEMORY_ROUTES, ( + "the destructive /api/memory surface drifted from " + "exposure.DESTRUCTIVE_MEMORY_ROUTES.\n" + f"Live but unpinned: {sorted(live_destructive - DESTRUCTIVE_MEMORY_ROUTES)}\n" + f"Pinned but gone: {sorted(DESTRUCTIVE_MEMORY_ROUTES - live_destructive)}" + ) + + +# ── 2. classification ──────────────────────────────────────────────────────── + + +@pytest.mark.parametrize(("method", "path"), sorted(DESTRUCTIVE_MEMORY_ROUTES)) +def test_destructive_memory_route_is_admin(method: str, path: str) -> None: + assert classify(method, path) is AuthClass.ADMIN + assert (method, path) not in OPEN_ALLOWLIST + + +@pytest.mark.parametrize(("method", "path"), sorted(DESTRUCTIVE_MEMORY_ROUTES)) +def test_destructive_memory_route_matches_a_pinned_rule(method: str, path: str) -> None: + """Not ADMIN-by-generic-prefix, and not ADMIN-by-silent-fallback. + + ``match_rule`` (rather than ``classify``) distinguishes the three ways a + route can end up ADMIN. These must resolve to one of the two rows added + for this lane, because that ordering is what makes the widening test + below hold. + """ + rule = match_rule(method, path) + assert rule is not None, f"{method} {path} falls through to the ADMIN fallback" + assert rule.label in { + "memory destructive (any DELETE)", + "memory namespace bulk delete", + }, f"{method} {path} resolved via {rule.label!r}, not a pinned destructive rule" + + +def test_destructive_routes_survive_a_memory_reads_are_client_widening( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The threat the pinning exists for, simulated. + + #1024's follow-up comment proposes keeping memory reads open. Encode + that proposal the way a developer editing the memory group actually + would — a broad ``/api/memory`` CLIENT rule dropped in next to the + generic ``memory`` row — and every destructive route must STILL + classify ADMIN, because the pinned rows sit above that whole group. + Before this lane there was nothing above it: the same edit downgraded + ``DELETE /api/memory/banks/{bank_id}`` to CLIENT, which is #1024's + incident minus the "unauthenticated" adjective. + + (Rules are first-match-wins, so this only holds while the pinned rows + stay at the TOP of the memory group. ``exposure.py``'s ordering comment + says as much; this test is what enforces it.) + """ + generic = next(i for i, rule in enumerate(exposure.RULES) if rule.label == "memory") + widened = ( + *exposure.RULES[:generic], + exposure._Rule( + "HYPOTHETICAL memory reads are client", + exposure._prefix("/api/memory"), + AuthClass.CLIENT, + None, + ), + *exposure.RULES[generic:], + ) + monkeypatch.setattr(exposure, "RULES", widened) + + # Sanity: the hypothetical rule really is in force for a plain read. + assert classify("GET", "/api/memory/banks") is AuthClass.CLIENT + + for method, path in sorted(DESTRUCTIVE_MEMORY_ROUTES): + assert classify(method, path) is AuthClass.ADMIN, ( + f"{method} {path} was downgraded to " + f"{classify(method, path).value} by a memory-read widening" + ) + + +# ── 3. the real gate, armed ────────────────────────────────────────────────── + + +@pytest.fixture +def armed_client( + monkeypatch: pytest.MonkeyPatch, tmp_path_factory: pytest.TempPathFactory +) -> TestClient: + """A real ``create_app()`` with ``AuthEnforcementMiddleware`` enforcing. + + Not a hand-rolled mini-app: the classification decision under test is + only meaningful against the route table and middleware ``create_app`` + actually ships. + """ + hal0_home = tmp_path_factory.mktemp("memory-delete-armed-home") + (hal0_home / "etc" / "hal0").mkdir(parents=True, exist_ok=True) + monkeypatch.setenv("HAL0_HOME", str(hal0_home)) + monkeypatch.setenv("HAL0_REQUIRE_AUTH", "1") + monkeypatch.setenv("HAL0_ADMIN_KEY", ADMIN_KEY) + monkeypatch.setenv("HAL0_CLIENT_KEY", CLIENT_KEY) + monkeypatch.setattr("hal0.api._mount_dashboard", lambda _app: None) + return TestClient(create_app()) + + +@pytest.mark.parametrize(("method", "path"), sorted(DESTRUCTIVE_MEMORY_ROUTES)) +def test_destructive_memory_route_401s_without_credentials( + armed_client: TestClient, method: str, path: str +) -> None: + """NEGATIVE CONTROL — the #1024 one-curl wipe, refused at the perimeter. + + Every path arg is filled with a syntactically valid id so the request + would reach a real handler if the gate let it: a 401 here can only come + from the auth middleware, never from routing or validation. + """ + url = _url(path) + resp = armed_client.request(method, url, json={"ids": ["a", "b"], "confirm": "shared"}) + assert resp.status_code == 401, f"{method} {url} -> {resp.status_code}: {resp.text}" + assert resp.json()["error"]["code"] == "auth.required" + + +@pytest.mark.parametrize(("method", "path"), sorted(DESTRUCTIVE_MEMORY_ROUTES)) +def test_destructive_memory_route_clears_the_gate_with_the_admin_key( + armed_client: TestClient, method: str, path: str +) -> None: + """GOLDEN PATH — the operator's own key still reaches the handler. + + The handler then fails for an unrelated reason (no memory engine in this + sandbox → 503 ``memory.unavailable``, or 400 for the missing echoed + confirm); what matters is that the response is not an auth denial. + """ + url = _url(path) + resp = armed_client.request( + method, + url, + headers={"Authorization": f"Bearer {ADMIN_KEY}"}, + json={"ids": ["a", "b"], "confirm": "shared"}, + ) + assert resp.status_code not in (401, 403), f"{method} {url} -> {resp.text}" + + +@pytest.mark.parametrize(("method", "path"), sorted(DESTRUCTIVE_MEMORY_ROUTES)) +def test_destructive_memory_route_403s_for_a_client_key( + armed_client: TestClient, method: str, path: str +) -> None: + """ADMIN tier, not merely "any valid key". + + The inference/client key is the one an agent or an OpenAI-SDK caller on + the LAN is most likely to hold; it must not be able to delete memory. + """ + url = _url(path) + resp = armed_client.request( + method, + url, + headers={"Authorization": f"Bearer {CLIENT_KEY}"}, + json={"ids": ["a", "b"], "confirm": "shared"}, + ) + assert resp.status_code == 403, f"{method} {url} -> {resp.status_code}: {resp.text}" + assert resp.json()["error"]["code"] == "auth.forbidden" + + +def test_mcp_memory_mount_401s_without_credentials(armed_client: TestClient) -> None: + """#1302's other delete path — the ``/mcp/memory`` JSON-RPC mount. + + The bulk-delete approval gate on that mount (landed separately for + #1302 — ``hal0.mcp.memory.make_dispatcher``'s ``approval_queue``, covered + by ``tests/mcp/test_memory_delete_gate_mount.py``) is the second line; the + first is that an anonymous caller never gets to speak MCP at all. + """ + resp = armed_client.post( + "/mcp/memory/mcp", + headers={"Accept": "application/json, text/event-stream"}, + json={"jsonrpc": "2.0", "id": 1, "method": "tools/list", "params": {}}, + ) + assert resp.status_code == 401, resp.text + + +def test_memory_reads_are_admin_too(armed_client: TestClient) -> None: + """Records the posture decision itself, so a future reader can't guess. + + Reads are ADMIN as well — the "keep reads open" option from #1024 was + NOT taken. If that changes, this test is the place the change is argued. + """ + assert classify("GET", "/api/memory/banks") is AuthClass.ADMIN + resp = armed_client.get("/api/memory/banks") + assert resp.status_code == 401, resp.text