diff --git a/changelog.d/2356-registry-existence-hiding.md b/changelog.d/2356-registry-existence-hiding.md new file mode 100644 index 000000000..7ce8f5f25 --- /dev/null +++ b/changelog.d/2356-registry-existence-hiding.md @@ -0,0 +1,7 @@ +### Security + +- All owner-gated agent-registry routes are now existence-hiding: a caller who + does not own an agent gets the same 404 as a nonexistent id, on the + scope-request create/approve/deny routes and on registry PATCH, revoke, + rotate-tokens, and org update. Previously a 403-vs-404 difference disclosed + whether an agent id existed (issue #2106, reported by hognek) (#2356). diff --git a/docs/agent-coordination.md b/docs/agent-coordination.md index 0a1b52358..a01af46ec 100644 --- a/docs/agent-coordination.md +++ b/docs/agent-coordination.md @@ -396,6 +396,15 @@ that SAME canonical_id instead: - `POST /api/agents/registry/{canonical_id}/scope-requests/{req_id}/deny`: owner/admin only. +All owner-gated registry routes are existence-hiding (#2106): an authenticated +caller who is not the owner gets the same 404 body as a nonexistent +`canonical_id`, on the scope-request create/approve/deny routes above and on +registry PATCH, DELETE (revoke), rotate-tokens, and `PUT /api/agents/{id}/org`. +Agents must not treat a 404 from these routes as proof an id does not exist, +and must not expect a 403 to distinguish "exists, not yours". Admin-only +lifecycle routes (approve/reject/suspend/reactivate) still 403 non-admins +before any lookup, which discloses nothing. + Requested scopes are validated against the same closed `VALID_SCOPES` vocabulary as the consent flow. `project_tasks` and the canvas scopes still require an explicit `project_id`; `decisions_read` / `decisions_write` (and the other global diff --git a/tests/test_agent_registry.py b/tests/test_agent_registry.py index d40b12bb8..1073d9c48 100644 --- a/tests/test_agent_registry.py +++ b/tests/test_agent_registry.py @@ -829,3 +829,88 @@ def test_allowed_scopes_includes_project_doc_review(): def test_allowed_scopes_includes_observatory_control(): """observatory_control must be in the mint allowlist so internal agents can be granted it.""" assert "observatory_control" in _ALLOWED_SCOPES + + +# --------------------------------------------------------------------------- +# Existence-hiding: non-owner vs non-existent must be byte-identical +# (same contract the scope-request routes assert in +# tests/test_agent_scope_requests.py; GET already had it, these four +# write routes gained it in the same pass) +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +class TestRegistryWriteExistenceHiding: + async def _non_owner_client(self, app): + """Session client for a real NON-admin user who owns nothing.""" + code = app.state.auth.add_user_invite("mallory", "admin") + app.state.auth.complete_invite("mallory", code, "Mallory", "", "malpass123") + record = app.state.auth.find_user("mallory") + session = app.state.auth.create_session(user_id=record["id"], long_lived=True) + return AsyncClient( + transport=ASGITransport(app=app), + base_url="http://test", + cookies={"taos_session": session}, + ) + + async def _register(self, registry_client): + resp = await registry_client.post( + "/api/agents/registry/register", + json={"framework": "openclaw", "display_name": "Hidden Agent"}, + ) + assert resp.status_code == 200 + return resp.json()["canonical_id"] + + async def test_patch_non_owner_and_nonexistent_identical(self, app, registry_client): + cid = await self._register(registry_client) + async with await self._non_owner_client(app) as mallory: + resp_owned = await mallory.patch( + f"/api/agents/registry/{cid}", json={"display_name": "Stolen"} + ) + resp_missing = await mallory.patch( + "/api/agents/registry/does-not-exist", json={"display_name": "Stolen"} + ) + assert resp_owned.status_code == resp_missing.status_code == 404 + assert resp_owned.content == resp_missing.content + # And the record was not modified. + check = await registry_client.get(f"/api/agents/registry/{cid}") + assert check.json()["display_name"] == "Hidden Agent" + + async def test_delete_non_owner_and_nonexistent_identical(self, app, registry_client): + cid = await self._register(registry_client) + async with await self._non_owner_client(app) as mallory: + resp_owned = await mallory.delete(f"/api/agents/registry/{cid}") + resp_missing = await mallory.delete("/api/agents/registry/does-not-exist") + assert resp_owned.status_code == resp_missing.status_code == 404 + assert resp_owned.content == resp_missing.content + # And the record was not revoked. + check = await registry_client.get(f"/api/agents/registry/{cid}") + assert check.status_code == 200 + assert not check.json().get("revoked_at") + + async def test_rotate_tokens_non_owner_and_nonexistent_identical( + self, app, registry_client + ): + cid = await self._register(registry_client) + async with await self._non_owner_client(app) as mallory: + resp_owned = await mallory.post( + f"/api/agents/registry/{cid}/rotate-tokens" + ) + resp_missing = await mallory.post( + "/api/agents/registry/does-not-exist/rotate-tokens" + ) + assert resp_owned.status_code == resp_missing.status_code == 404 + assert resp_owned.content == resp_missing.content + + async def test_org_put_non_owner_and_nonexistent_identical( + self, app, registry_client + ): + cid = await self._register(registry_client) + async with await self._non_owner_client(app) as mallory: + resp_owned = await mallory.put( + f"/api/agents/{cid}/org", json={"role": "usurper"} + ) + resp_missing = await mallory.put( + "/api/agents/does-not-exist/org", json={"role": "usurper"} + ) + assert resp_owned.status_code == resp_missing.status_code == 404 + assert resp_owned.content == resp_missing.content diff --git a/tests/test_agent_scope_requests.py b/tests/test_agent_scope_requests.py index 0e43c8c80..00970100f 100644 --- a/tests/test_agent_scope_requests.py +++ b/tests/test_agent_scope_requests.py @@ -157,7 +157,7 @@ async def test_agent_cannot_request_for_another_identity(client, monkeypatch, tm headers={"Authorization": f"Bearer {token_a}"}, json={"requested_scopes": ["a2a_send"]}, ) - assert resp.status_code == 403, resp.text + assert resp.status_code == 404, resp.text assert await env.scope_store.count_pending_for(cid_b) == 0 finally: await env.close() @@ -294,7 +294,7 @@ async def test_non_owner_cannot_approve(client, monkeypatch, tmp_path): f"/api/agents/registry/{cid}/scope-requests/{rec['id']}/approve", json={"granted_scopes": ["memory_read"]}, ) - assert resp.status_code == 403, resp.text + assert resp.status_code == 404, resp.text assert await env.grants.list_grants(cid) == [] finally: await env.close() @@ -459,7 +459,7 @@ async def test_create_authorizes_before_scope_vocab(client, monkeypatch, tmp_pat json={"requested_scopes": ["not_a_real_scope"]}, ) # Authz runs first -> 403, NOT a 400 vocab error confirming the bad scope. - assert resp.status_code == 403, resp.text + assert resp.status_code == 404, resp.text assert "not_a_real_scope" not in resp.text finally: await env.close() @@ -598,3 +598,160 @@ def test_every_project_bound_scope_is_a_valid_scope(): from tinyagentos.routes.agent_auth_requests import VALID_SCOPES, _PROJECT_SCOPES assert _PROJECT_SCOPES <= set(VALID_SCOPES) + + +# --------------------------------------------------------------------------- +# Existence-hiding: non-owner vs non-existent must be byte-identical +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_create_scope_request_non_owner_and_nonexistent_identical( + client, monkeypatch, tmp_path +): + """An authenticated non-owner and a nonexistent canonical_id must produce + byte-identical responses on create_scope_request (status + body).""" + app = client._transport.app + code = app.state.auth.add_user_invite("carol", "admin") + app.state.auth.complete_invite("carol", code, "Carol", "", "carpass123") + carol = app.state.auth.find_user("carol") + carol_session = app.state.auth.create_session(user_id=carol["id"], long_lived=True) + + env = await _wire(client, monkeypatch, tmp_path) + try: + cid = await _register_active(env) # owned by admin, not carol + + async with AsyncClient( + transport=ASGITransport(app=app), + base_url="http://test", + cookies={"taos_session": carol_session}, + ) as carol_client: + resp_owner = await carol_client.post( + f"/api/agents/registry/{cid}/scope-requests", + json={"requested_scopes": ["memory_read"]}, + ) + # Same caller for the nonexistent probe: an admin would 404 on a + # missing id too, but the contract under test is what ONE + # unprivileged caller can distinguish. + resp_nonexistent = await carol_client.post( + "/api/agents/registry/does-not-exist/scope-requests", + json={"requested_scopes": ["memory_read"]}, + ) + + assert resp_owner.status_code == resp_nonexistent.status_code == 404 + assert resp_owner.content == resp_nonexistent.content + finally: + await env.close() + + +@pytest.mark.asyncio +async def test_approve_scope_request_non_owner_and_nonexistent_identical( + client, monkeypatch, tmp_path +): + """An authenticated non-owner and a nonexistent canonical_id must produce + byte-identical responses on approve_scope_request (status + body).""" + app = client._transport.app + code = app.state.auth.add_user_invite("carol", "admin") + app.state.auth.complete_invite("carol", code, "Carol", "", "carpass123") + carol = app.state.auth.find_user("carol") + carol_session = app.state.auth.create_session(user_id=carol["id"], long_lived=True) + + env = await _wire(client, monkeypatch, tmp_path) + try: + cid = await _register_active(env) # owned by admin, not carol + rec = await env.scope_store.create( + canonical_id=cid, requested_scopes=["memory_read"] + ) + + async with AsyncClient( + transport=ASGITransport(app=app), + base_url="http://test", + cookies={"taos_session": carol_session}, + ) as carol_client: + resp_owner = await carol_client.post( + f"/api/agents/registry/{cid}/scope-requests/{rec['id']}/approve", + json={"granted_scopes": ["memory_read"]}, + ) + resp_nonexistent = await carol_client.post( + "/api/agents/registry/does-not-exist/scope-requests/does-not-exist/approve", + json={"granted_scopes": ["memory_read"]}, + ) + + assert resp_owner.status_code == resp_nonexistent.status_code == 404 + assert resp_owner.content == resp_nonexistent.content + finally: + await env.close() + + +@pytest.mark.asyncio +async def test_deny_scope_request_non_owner_and_nonexistent_identical( + client, monkeypatch, tmp_path +): + """An authenticated non-owner and a nonexistent canonical_id must produce + byte-identical responses on deny_scope_request (status + body).""" + app = client._transport.app + code = app.state.auth.add_user_invite("carol", "admin") + app.state.auth.complete_invite("carol", code, "Carol", "", "carpass123") + carol = app.state.auth.find_user("carol") + carol_session = app.state.auth.create_session(user_id=carol["id"], long_lived=True) + + env = await _wire(client, monkeypatch, tmp_path) + try: + cid = await _register_active(env) # owned by admin, not carol + rec = await env.scope_store.create( + canonical_id=cid, requested_scopes=["memory_read"] + ) + + async with AsyncClient( + transport=ASGITransport(app=app), + base_url="http://test", + cookies={"taos_session": carol_session}, + ) as carol_client: + resp_owner = await carol_client.post( + f"/api/agents/registry/{cid}/scope-requests/{rec['id']}/deny", + ) + resp_nonexistent = await carol_client.post( + "/api/agents/registry/does-not-exist/scope-requests/does-not-exist/deny", + ) + + assert resp_owner.status_code == resp_nonexistent.status_code == 404 + assert resp_owner.content == resp_nonexistent.content + finally: + await env.close() + + +@pytest.mark.asyncio +async def test_create_scope_request_inactive_token_no_existence_oracle( + client, monkeypatch, tmp_path +): + """A suspended agent's (validly signed) token must get the SAME response + for an existing target as for a nonexistent one. Before the fix, + check_agent_identity's 403 surfaced only when the target existed (an + unknown target 404s first), disclosing existence through the + credential-error path.""" + env = await _wire(client, monkeypatch, tmp_path) + try: + cid_target = await _register_active(env, handle="@target", display="target") + cid_b = await _register_active(env, handle="@suspended", display="suspended") + token_b = env.agent_token(cid_b) + await env.registry.set_status(cid_b, "suspended", actor="test") + + app = client._transport.app + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://test" + ) as bare: + resp_existing = await bare.post( + f"/api/agents/registry/{cid_target}/scope-requests", + headers={"Authorization": f"Bearer {token_b}"}, + json={"requested_scopes": ["a2a_send"]}, + ) + resp_missing = await bare.post( + "/api/agents/registry/does-not-exist/scope-requests", + headers={"Authorization": f"Bearer {token_b}"}, + json={"requested_scopes": ["a2a_send"]}, + ) + + assert resp_existing.status_code == resp_missing.status_code == 404 + assert resp_existing.content == resp_missing.content + assert await env.scope_store.count_pending_for(cid_target) == 0 + finally: + await env.close() diff --git a/tests/test_registry_governance_lifecycle.py b/tests/test_registry_governance_lifecycle.py index a0c2b87e8..735588f3a 100644 --- a/tests/test_registry_governance_lifecycle.py +++ b/tests/test_registry_governance_lifecycle.py @@ -846,12 +846,12 @@ async def test_patch_by_non_owner_member_returns_403( user_id="other-user-uid", ) cid = rec["canonical_id"] - # member tries to patch another user's entry → 403 + # member tries to patch another user's entry → 404 (existence-hiding) resp = await gov_member_client.patch( f"/api/agents/registry/{cid}", json={"display_name": "Hijacked"}, ) - assert resp.status_code == 403 + assert resp.status_code == 404 async def test_patch_empty_body_is_noop(self, gov_client, tmp_data_dir): client, _ = gov_client diff --git a/tests/test_routes_agent_org.py b/tests/test_routes_agent_org.py index 295849d14..282a5cb7c 100644 --- a/tests/test_routes_agent_org.py +++ b/tests/test_routes_agent_org.py @@ -258,4 +258,4 @@ async def test_member_cannot_update_others_entry(self, org_client, app): ) finally: await member_client.aclose() - assert resp.status_code == 403 + assert resp.status_code == 404 diff --git a/tests/test_token_rotation.py b/tests/test_token_rotation.py index 20c99c38b..3a3b53aae 100644 --- a/tests/test_token_rotation.py +++ b/tests/test_token_rotation.py @@ -241,7 +241,7 @@ async def test_nonadmin_owner_can_rotate_own_identity(self, agent_app, app): @pytest.mark.asyncio async def test_nonadmin_cannot_rotate_others_agent(self, agent_app, app): - """A non-admin, non-owner rotating someone ELSE'S agent → 403.""" + """A non-admin, non-owner rotating someone ELSE'S agent → 404 (existence-hiding).""" # First user owns an agent. owner_client, owner_uid = _make_nonadmin_client( app, app.state.auth, username="owner2", full_name="Owner Two", @@ -260,22 +260,22 @@ async def test_nonadmin_cannot_rotate_others_agent(self, agent_app, app): resp = await intruder_client.post( f"/api/agents/registry/{cid}/rotate-tokens" ) - assert resp.status_code == 403 + assert resp.status_code == 404 @pytest.mark.asyncio async def test_empty_userid_agent_is_admin_only(self, agent_app, app): """An agent_registry row with user_id='' can ONLY be rotated by admin. - Non-admin sessions get 403 because require_owner_or_admin compares - the session's user_id against an empty string (owner match fails) - and the session is not admin. + Non-admin sessions get 404 (existence-hiding): the owner match + against an empty string fails, the session is not admin, and the + route answers exactly as it would for an unknown id. """ # Register an agent with the default user_id="" (admin-only). cid, _token = await _register_and_mint(app, user_id="admin") r = await app.state.agent_registry.get(cid) assert r["user_id"] == "" - # A non-admin session trying to rotate it must get 403. + # A non-admin session trying to rotate it must get the not-found 404. nonadmin_client, _uid = _make_nonadmin_client( app, app.state.auth, username="randouser", full_name="Rando", password="password123", @@ -284,7 +284,7 @@ async def test_empty_userid_agent_is_admin_only(self, agent_app, app): resp = await nonadmin_client.post( f"/api/agents/registry/{cid}/rotate-tokens" ) - assert resp.status_code == 403 + assert resp.status_code == 404 @pytest.mark.asyncio async def test_rotate_nonexistent_returns_404(self, agent_app): diff --git a/tinyagentos/routes/agent_auth_requests.py b/tinyagentos/routes/agent_auth_requests.py index a3645f1a2..50842548b 100644 --- a/tinyagentos/routes/agent_auth_requests.py +++ b/tinyagentos/routes/agent_auth_requests.py @@ -951,24 +951,38 @@ async def _authorize_scope_request_creation( Allowed: an owner/admin session (or admin local token), OR the agent's own registry bearer token whose ``sub`` matches *canonical_id*. Any other caller - (including a different agent's token, or no credentials) is a 403/401. + (including a different agent's token, or no credentials) is a 404 (the + response is uniform with the not-found case to avoid leaking existence). """ is_admin = bool(getattr(request.state, "is_admin", False)) uid = getattr(request.state, "user_id", None) if is_admin or (uid and uid == record.get("user_id")): return - # The agent's own registry token. check_agent_identity returns None when no - # Authorization header is present (an unauthenticated caller never reaches - # here anyway — the middleware 401s a credential-less non-exempt request) and - # raises 401/403 for a malformed/inactive token. from tinyagentos.agent_token_auth import check_agent_identity - agent_cid = await check_agent_identity(request) + try: + agent_cid = await check_agent_identity(request) + except HTTPException as exc: + # A malformed or inactive-agent token must learn no more than an + # anonymous caller: letting check_agent_identity's 401/403 surface + # here would pair with the earlier 404-on-unknown to form an + # existence oracle (unknown target 404, existing target 401/403). + logger.info( + "scope request create 404-bad-credentials for %s (%s)", + canonical_id, exc.detail, + ) + raise HTTPException( + status_code=404, detail="agent not found or not active" + ) from None if agent_cid is not None and agent_cid == canonical_id: return - raise HTTPException(status_code=403, detail="forbidden") + logger.info( + "scope request create 404-not-owner for %s by %s", + canonical_id, uid, + ) + raise HTTPException(status_code=404, detail="agent not found or not active") @router.post("/api/agents/registry/{canonical_id}/scope-requests") @@ -983,8 +997,7 @@ async def create_scope_request( registry = _get_registry_store(request) record = await registry.get(canonical_id) if record is None or record.get("status") != "active": - # Existence-hiding is unnecessary here (the caller must already be the - # agent or its owner/admin) but an inactive/unknown id is simply a 404. + logger.info("scope request create 404-unknown for %s", canonical_id) raise HTTPException( status_code=404, detail="agent not found or not active" ) @@ -1069,8 +1082,14 @@ async def approve_scope_request( registry = _get_registry_store(request) record = await registry.get(canonical_id) if record is None or record.get("status") != "active": + logger.info("scope request approve 404-unknown for %s", canonical_id) + raise HTTPException(status_code=404, detail="agent not found or not active") + if not (user.is_admin or user.user_id == record["user_id"]): + logger.info( + "scope request approve 404-not-owner for %s by %s", + canonical_id, user.user_id, + ) raise HTTPException(status_code=404, detail="agent not found or not active") - require_owner_or_admin(user, record["user_id"]) store = _get_scope_requests_store(request) @@ -1189,8 +1208,14 @@ async def deny_scope_request( registry = _get_registry_store(request) record = await registry.get(canonical_id) if record is None: + logger.info("scope request deny 404-unknown for %s", canonical_id) + raise HTTPException(status_code=404, detail="agent not found") + if not (user.is_admin or user.user_id == record["user_id"]): + logger.info( + "scope request deny 404-not-owner for %s by %s", + canonical_id, user.user_id, + ) raise HTTPException(status_code=404, detail="agent not found") - require_owner_or_admin(user, record["user_id"]) store = _get_scope_requests_store(request) diff --git a/tinyagentos/routes/agent_registry.py b/tinyagentos/routes/agent_registry.py index a175c1a7e..98964be18 100644 --- a/tinyagentos/routes/agent_registry.py +++ b/tinyagentos/routes/agent_registry.py @@ -33,7 +33,7 @@ from tinyagentos.agent_registry_store import mint_registry_token from tinyagentos.agent_token_auth import check_agent_scope -from tinyagentos.auth_context import CurrentUser, current_user, require_owner_or_admin +from tinyagentos.auth_context import CurrentUser, current_user logger = logging.getLogger(__name__) @@ -612,13 +612,17 @@ async def patch_registry_entry( Allowed fields: display_name, handle, role, capabilities. Status, framework, user_id, and timestamps are immutable. - Only the owning user or an admin may update an entry. + Only the owning user or an admin may update an entry; anyone else gets + the same 404 as an unknown id (existence-hiding, as on GET). """ store = _get_store(request) record = await store.get(canonical_id) if record is None: + logger.info("registry patch 404-unknown for %s", canonical_id) + return JSONResponse({"error": "not found"}, status_code=404) + if not user.is_admin and user.user_id != record["user_id"]: + logger.info("registry patch 404-not-owner for %s by %s", canonical_id, user.user_id) return JSONResponse({"error": "not found"}, status_code=404) - require_owner_or_admin(user, record["user_id"]) old_name = record.get("display_name") or "" try: updated = await store.update( @@ -664,13 +668,17 @@ async def revoke_registry_entry( ): """Revoke a registry entry (sets revoked_at, does not delete). - Only the owning user or an admin may revoke an entry. + Only the owning user or an admin may revoke an entry; anyone else gets + the same 404 as an unknown id (existence-hiding, as on GET). """ store = _get_store(request) record = await store.get(canonical_id) if record is None: + logger.info("registry revoke 404-unknown for %s", canonical_id) + return JSONResponse({"error": "not found or already revoked"}, status_code=404) + if not user.is_admin and user.user_id != record["user_id"]: + logger.info("registry revoke 404-not-owner for %s by %s", canonical_id, user.user_id) return JSONResponse({"error": "not found or already revoked"}, status_code=404) - require_owner_or_admin(user, record["user_id"]) before_status = record.get("status") or "active" revoked = await store.revoke(canonical_id) await _audit_governance( @@ -777,8 +785,11 @@ async def rotate_tokens( store = _get_store(request) record = await store.get(canonical_id) if record is None: + logger.info("registry rotate-tokens 404-unknown for %s", canonical_id) + return JSONResponse({"error": "not found"}, status_code=404) + if not user.is_admin and user.user_id != record["user_id"]: + logger.info("registry rotate-tokens 404-not-owner for %s by %s", canonical_id, user.user_id) return JSONResponse({"error": "not found"}, status_code=404) - require_owner_or_admin(user, record["user_id"]) ts = int(time.time()) before_iat = record.get("token_min_iat") or 0 @@ -840,8 +851,11 @@ async def update_org_fields( store = _get_store(request) record = await store.get(canonical_id) if record is None: + logger.info("registry org update 404-unknown for %s", canonical_id) + return JSONResponse({"error": "not found"}, status_code=404) + if not user.is_admin and user.user_id != record["user_id"]: + logger.info("registry org update 404-not-owner for %s by %s", canonical_id, user.user_id) return JSONResponse({"error": "not found"}, status_code=404) - require_owner_or_admin(user, record["user_id"]) if body.role is None and body.title is None and body.reports_to is None: # An all-None body is a no-op write; reject it rather than returning a