Existence-hiding 404 across ALL agent-registry routes (issue #2106, reported by hognek) - #2356
Conversation
Convert create_scope_request, approve_scope_request, and deny_scope_request
to return the same 404 response for both non-existent canonical_ids and
authenticated non-owners, matching the pattern already used by
GET /api/agents/registry/{id}.
Server-side logs distinguish 403-not-owner from 404-unknown; only the
response is uniform.
Updated existing tests asserting 403 to assert 404, and added red-first
identical-response tests for each converted route.
Timing: non-owner path performs the same work as before (registry lookup
plus authz check); no new fast path on the not-found side.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughScope-request and agent-registry mutation routes now return 404 for unauthorized access. The routes log rejected and unavailable-agent requests. Tests verify identical responses for unauthorized and nonexistent agent IDs. ChangesAgent existence hiding
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_agent_scope_requests.py`:
- Around line 633-639: Update tests/test_agent_scope_requests.py lines 633-639,
673-679, and 712-717 so each nonexistent create, approve, and deny request is
issued through carol_client, then compare resp_owner.content with
resp_nonexistent.content instead of normalized JSON responses while preserving
the existing 404 assertions.
In `@tinyagentos/routes/agent_auth_requests.py`:
- Around line 1197-1205: Update the deny-route record guard in the surrounding
scope-request handler to also reject records whose status is not active by
checking record.get("status") != "active". Preserve the existing 404 detail and
ownership-check behavior for missing or inactive agents.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ead8e52d-6bfe-4370-b6a3-3dfe20f201b6
📒 Files selected for processing (2)
tests/test_agent_scope_requests.pytinyagentos/routes/agent_auth_requests.py
| 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 403-not-owner for %s by %s", | ||
| canonical_id, user.user_id, | ||
| ) | ||
| raise HTTPException(status_code=404, detail="agent not found") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject inactive agents on the deny route.
Line 1197 rejects only missing records. An inactive record can reach store.set_decision, while the create and approve routes reject inactive records. Include record.get("status") != "active" in this guard and keep the same 404 response.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/routes/agent_auth_requests.py` around lines 1197 - 1205, Update
the deny-route record guard in the surrounding scope-request handler to also
reject records whose status is not active by checking record.get("status") !=
"active". Preserve the existing 404 detail and ownership-check behavior for
missing or inactive agents.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| @@ -1189,8 +1195,14 @@ async def deny_scope_request( | |||
| registry = _get_registry_store(request) | |||
| record = await registry.get(canonical_id) | |||
| if record is None: | |||
There was a problem hiding this comment.
WARNING: deny_scope_request is missing the record.get("status") != "active" guard that approve_scope_request now has at line 1071, breaking the uniform 404 existence-hiding contract for this route.
approve_scope_request correctly returns 404 for inactive agents:
if record is None or record.get("status") != "active":
raise HTTPException(status_code=404, ...)
deny_scope_request only checks if record is None — an inactive agent record bypasses this guard and falls through to the owner check, allowing the owning user to successfully deny a scope request for an inactive agent. This directly contradicts the PR's stated goal of a uniform 404 across all agent-registry scope-request routes (see create_scope_request line 986, which already guards both conditions).
Suggested fix — mirror the active-status check from approve_scope_request:
| if record is None: | |
| if record is None or record.get("status") != "active": | |
| logger.info("scope request deny 404-unknown for %s", canonical_id) | |
| raise HTTPException(status_code=404, detail="agent not found or not active") |
A companion test (analogous to the three new red-first tests) should cover the deny + inactive-agent case.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summaries (4 snapshots, latest commit 999f7ab)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 999f7ab)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous review (commit 74ba0d2)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 0b632eb)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 2b1ffb3)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Reviewed by step-3.7-flash · Input: 97.7K · Output: 21.3K · Cached: 279.8K |
|
nemotron-super review VERDICT: Log messages incorrectly state 403 when raising 404 for non-owner cases
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
…e routes Sweep of the same class the scope-request routes fixed: PATCH, DELETE, rotate-tokens, and org PUT all returned 404-if-missing then 403-if-not-owner, disclosing id existence to any authenticated non-owner. All four now return the not-found response for non-owners, using the exact idiom of the GET route this file already documents as the reference implementation. Lifecycle routes (_transition) and the consent approve/deny routes check admin BEFORE any lookup, so they respond uniformly already and are unchanged. Byte-identical tests (status + body, non-owner vs nonexistent) for all four routes with a real non-admin user; PATCH test proven red against the old require_owner_or_admin behaviour. The import that check left orphaned is removed. Changelog fragment added for doc-gate.
|
Lead review (tsk-vzkbao, issue #2106). The lane's fix is correct and well-tested: the three scope-request routes now return the not-found response for authenticated non-owners, the true cause is logged server-side, and the byte-identical comparison tests (status + body, non-owner vs nonexistent) are exactly the right assertion shape. The consent approve/deny routes the issue also listed turn out not to leak: both check Pushed 0b632eb completing the class the issue's own goal statement names ("make every agent-registry route existence-hiding"): four owner-gated write routes in Merging via the gate once checks are green on the new head. |
| "/api/agents/registry/does-not-exist", json={"display_name": "Stolen"} | ||
| ) | ||
| assert resp_owned.status_code == resp_missing.status_code == 404 | ||
| assert resp_owned.json() == resp_missing.json() |
There was a problem hiding this comment.
WARNING: .json() does not verify byte-identical responses
The test docstring asserts byte-identical responses (status + body), but resp_owned.json() == resp_missing.json() compares normalized JSON objects — whitespace, key ordering, and trailing-newline differences in the raw response body are silently ignored. Use resp_owned.content == resp_missing.content to enforce the contract the docstring promises.
| assert resp_owned.json() == resp_missing.json() | |
| assert resp_owned.content == resp_missing.content |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "/api/agents/registry/does-not-exist/rotate-tokens" | ||
| ) | ||
| assert resp_owned.status_code == resp_missing.status_code == 404 | ||
| assert resp_owned.json() == resp_missing.json() |
There was a problem hiding this comment.
WARNING: .json() does not verify byte-identical responses
Same issue as test_patch_non_owner_and_nonexistent_identical at line 873. resp_owned.json() == resp_missing.json() compares normalized JSON objects, not raw response bodies. Use resp_owned.content == resp_missing.content to enforce the byte-identical contract.
| assert resp_owned.json() == resp_missing.json() | |
| assert resp_owned.content == resp_missing.content |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…04, document contract The class sweep changed four write routes but missed four pre-existing tests asserting the old 403 in OTHER files (caught by CI shards): two rotate-tokens tests, the lifecycle PATCH non-owner test, and the org PUT non-owner test. All now assert the not-found 404 with docstrings explaining why. docs/agent-coordination.md gains the existence-hiding contract for the whole owner-gated registry surface (doc-gate agent-manual rule), including the warning that a 404 no longer proves nonexistence.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_agent_scope_requests.py (1)
633-639: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winTest raw responses with the same caller.
.json()normalizes response bodies and does not verify byte identity. In the scope-request tests, the missing-record request also usesclientinstead ofcarol_client. Send both requests through the same non-owner client and compare.content.
tests/test_agent_scope_requests.py#L633-L639: usecarol_clientfor the missing create request and compare.content.tests/test_agent_scope_requests.py#L673-L679: usecarol_clientfor the missing approve request and compare.content.tests/test_agent_scope_requests.py#L712-L717: usecarol_clientfor the missing deny request and compare.content.tests/test_agent_registry.py#L872-L876: compare PATCH response.content.tests/test_agent_registry.py#L883-L888: compare DELETE response.content.tests/test_agent_registry.py#L901-L902: compare token-rotation response.content.tests/test_agent_registry.py#L915-L916: compare organization-update response.content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_agent_scope_requests.py` around lines 633 - 639, The scope-request tests at tests/test_agent_scope_requests.py lines 633-639, 673-679, and 712-717 must send missing-record requests through carol_client and compare raw response.content rather than normalized JSON. In tests/test_agent_registry.py lines 872-876, 883-888, 901-902, and 915-916, replace response JSON comparisons with response.content comparisons for the PATCH, DELETE, token-rotation, and organization-update checks.tinyagentos/routes/agent_auth_requests.py (1)
1197-1205: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject inactive agents on the deny route.
Line 1197 accepts an inactive record. Its owner can then deny a scope request, while the create and approve routes return 404. Include
record.get("status") != "active"in this guard and preserve the same 404 response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/agent_auth_requests.py` around lines 1197 - 1205, Update the record validation guard after the lookup in the deny route to also reject records whose status is not active, using record.get("status") != "active" alongside the existing missing-record and ownership checks. Preserve the current 404 response and logging behavior for inactive agents.
🧹 Nitpick comments (1)
tinyagentos/routes/agent_auth_requests.py (1)
968-971: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the diagnostic status label with the HTTP response.
These paths return HTTP 404 but log
403-not-owner. Use404-not-owner, or remove the status code from the event name.
tinyagentos/routes/agent_auth_requests.py#L968-L971: update the create-route non-owner log label.tinyagentos/routes/agent_auth_requests.py#L1075-L1078: update the approve-route non-owner log label.tinyagentos/routes/agent_auth_requests.py#L1201-L1204: update the deny-route non-owner log label.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/agent_auth_requests.py` around lines 968 - 971, Update the non-owner diagnostic labels in the create, approve, and deny routes of tinyagentos/routes/agent_auth_requests.py at lines 968-971, 1075-1078, and 1201-1204 to use 404-not-owner, matching their HTTP responses, or remove the status code from all three event names consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tinyagentos/routes/agent_registry.py`:
- Around line 618-623: In tinyagentos/routes/agent_registry.py, add distinct
diagnostic logs immediately before the uniform 404 responses for unknown records
and non-owner access in the PATCH route (618-623), revoke route (672-677),
token-rotation route (781-786), and organization-update route (845-850).
Preserve the existing response behavior while clearly identifying which outcome
occurred at each site.
---
Duplicate comments:
In `@tests/test_agent_scope_requests.py`:
- Around line 633-639: The scope-request tests at
tests/test_agent_scope_requests.py lines 633-639, 673-679, and 712-717 must send
missing-record requests through carol_client and compare raw response.content
rather than normalized JSON. In tests/test_agent_registry.py lines 872-876,
883-888, 901-902, and 915-916, replace response JSON comparisons with
response.content comparisons for the PATCH, DELETE, token-rotation, and
organization-update checks.
In `@tinyagentos/routes/agent_auth_requests.py`:
- Around line 1197-1205: Update the record validation guard after the lookup in
the deny route to also reject records whose status is not active, using
record.get("status") != "active" alongside the existing missing-record and
ownership checks. Preserve the current 404 response and logging behavior for
inactive agents.
---
Nitpick comments:
In `@tinyagentos/routes/agent_auth_requests.py`:
- Around line 968-971: Update the non-owner diagnostic labels in the create,
approve, and deny routes of tinyagentos/routes/agent_auth_requests.py at lines
968-971, 1075-1078, and 1201-1204 to use 404-not-owner, matching their HTTP
responses, or remove the status code from all three event names consistently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 890a4ed8-8c18-4284-a769-701c8eda4a65
📒 Files selected for processing (9)
changelog.d/2356-registry-existence-hiding.mddocs/agent-coordination.mdtests/test_agent_registry.pytests/test_agent_scope_requests.pytests/test_registry_governance_lifecycle.pytests/test_routes_agent_org.pytests/test_token_rotation.pytinyagentos/routes/agent_auth_requests.pytinyagentos/routes/agent_registry.py
…es, route logs Folds the Kilo + CodeRabbit findings on the identical-response tests: .json() compares normalized objects so it cannot back the byte-identical claim; all seven tests now compare resp.content. The three scope-request tests also send their nonexistent probe from the SAME non-owner client instead of the admin fixture, since the contract under test is what one unprivileged caller can distinguish. The four registry write routes gain the same unknown-vs-not-owner server-side logs the scope-request routes already emit.
|
Bot findings adjudicated (999f7ab): Folded — both bots' byte-identity point was right: Not folded, with reasoning — "reject inactive agents on the deny route": the deny route's None-only check predates this PR and is semantically deliberate, not an oversight. Create and approve gate on |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tinyagentos/routes/agent_auth_requests.py`:
- Around line 968-971: Rename every non-owner diagnostic label from
403-not-owner to 404-not-owner in tinyagentos/routes/agent_auth_requests.py at
lines 968-971, 1075-1078, and 1201-1204, and in
tinyagentos/routes/agent_registry.py at lines 624, 680, 791, and 857, covering
the create, approve, deny, PATCH, revoke, token-rotation, and
organization-update logs.
- Around line 962-965: Update create_scope_request to validate the Authorization
bearer credentials before calling registry.get(canonical_id), preserving the
existing 401/403 behavior for malformed and inactive tokens. Store and reuse the
resolved identity from check_agent_identity for the canonical_id ownership
check, and add response-pair tests covering malformed and inactive bearer tokens
against known and unknown targets.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 51e55af7-a87f-4c19-92c2-a18e9dac2d5e
📒 Files selected for processing (9)
changelog.d/2356-registry-existence-hiding.mddocs/agent-coordination.mdtests/test_agent_registry.pytests/test_agent_scope_requests.pytests/test_registry_governance_lifecycle.pytests/test_routes_agent_org.pytests/test_token_rotation.pytinyagentos/routes/agent_auth_requests.pytinyagentos/routes/agent_registry.py
…reate check_agent_identity raises 401 (malformed token) or 403 (inactive agent) in-route, but create_scope_request 404s on an unknown target BEFORE auth runs, so a caller holding a bad token could distinguish existing targets (401/403) from nonexistent ones (404). The authorize helper now converts those raises into the uniform 404, logging the true cause server-side. Regression test: a suspended agent's validly signed token gets byte-identical 404s for an existing and a nonexistent target; proven red against the unguarded call. Also renames the create/approve/deny log labels from 403-not-owner to 404-not-owner to match what the routes actually return (CodeRabbit findings, both folded).
|
CodeRabbit round 2, both folded in the new head: the Major was real — a malformed/inactive bearer token got 401/403 on an existing target but 404 on an unknown one (the not-found check runs before auth), an existence oracle through the credential-error path. |
|
@coderabbitai full review |
|
|
Merging with the CodeRabbit status on e44e721 explicitly adjudicated, not trusted: its 'pass' there is the rate-limited fake green (description 'Review rate limited', never ran, retriggered twice with a 10-minute wait each). It is not being counted as a review. What is counted: CodeRabbit's two REAL reviews on 74ba0d2 and 999f7ab (every actionable finding from both rounds folded or adjudicated with reasoning above), Kilo's fresh pass on this exact head (19:36Z), green CI across the board, and the fact that the final delta consists solely of CodeRabbit's own round-2 findings implemented with a mutation-proven regression test. |
1 participant
CARD TITLE (intent, not commit subject): Existence-hiding 404 across ALL agent-registry routes (issue #2106, reported by hognek)
Autonomous build of board card tsk-vzkbao.
Convert create_scope_request, approve_scope_request, and deny_scope_request
to return the same 404 response for both non-existent canonical_ids and
authenticated non-owners, matching the pattern already used by
GET /api/agents/registry/{id}.
Server-side logs distinguish 403-not-owner from 404-unknown; only the
response is uniform.
Updated existing tests asserting 403 to assert 404, and added red-first
identical-response tests for each converted route.
Timing: non-owner path performs the same work as before (registry lookup
plus authz check); no new fast path on the not-found side.
Files:
tests/test_agent_scope_requests.py | 125 +++++++++++++++++++++++++++++-
tinyagentos/routes/agent_auth_requests.py | 32 +++++---
2 files changed, 144 insertions(+), 13 deletions(-)
Summary by CodeRabbit
Bug Fixes
404instead of403, preventing agent existence disclosure.404responses to unauthorized callers.Documentation
Red-first evidence (lead-run at merge ref)
All 7 new identical-response tests executed at the merge ref
origin/dev(5910eb1), before the change:Timing statement (card point 4)
No obviously faster branch was introduced on the not-found side. Every touched route performs the same single
store.get(canonical_id)(or registry lookup) on both branches; the non-owner branch additionally does one in-memory id comparison before returning the identical response. Constant-time behaviour is not claimed.