Skip to content

fix: agent transport allows cross agent approval#169

Closed
jonathanchang31 wants to merge 1 commit into
vouchdev:mainfrom
jonathanchang31:fix/agent-transort-allows-cross-agent-approval
Closed

fix: agent transport allows cross agent approval#169
jonathanchang31 wants to merge 1 commit into
vouchdev:mainfrom
jonathanchang31:fix/agent-transort-allows-cross-agent-approval

Conversation

@jonathanchang31

Copy link
Copy Markdown
Contributor

Summary

This PR fixes #168 by disabling kb.approve and kb.reject on agent-facing transports by default.

Previously, the JSONL/MCP tool surface exposed kb.approve to agents. The self-approval guard blocked the same VOUCH_AGENT, but a second agent identity could approve another agent’s proposal and land a durable claim without human CLI review.

Now, decision tools are only available when the KB is explicitly configured as a trusted approval host.

Related Issue

Fixes: #168

Change Type

  • Bug fix
  • Security hardening
  • Review-gate enforcement
  • Regression test
  • Documentation update
  • New feature
  • Breaking change

Real Behavior Proof

Before the fix, cross-agent approval succeeded:

-- alice proposes via JSONL server --
{"id": "p", "ok": true, "result": {"proposal_id": "...", "status": "pending", "kind": "claim"}}

-- bob approves via JSONL server --
{"id": "approve", "ok": true, "result": {"kind": "claim", "id": "cross-agent-approval-lands-without-human-cli"}}

After the fix, decision tools are hidden from default capabilities:

"methods": [
  "kb.capabilities",
  "kb.status",
  "kb.search",
  ...
  "kb.import_apply",
  "kb.audit"
]

And cross-agent approval is blocked:

-- bob approval after fix --
{"id": "approve", "ok": false, "error": {"code": "method_not_found", "message": "unknown method: kb.approve"}}

The proposal remains pending:

"pending_proposals": 1

Checklist

  • Checked issue comments and metadata
  • Reproduced the original cross-agent approval bypass
  • Confirmed self-approval was blocked but cross-agent approval succeeded
  • Implemented trusted-host gating for decision tools
  • Hid kb.approve / kb.reject from default capabilities
  • Blocked default JSONL kb.approve calls with method_not_found
  • Applied same gate to MCP decision handlers
  • Added regression tests
  • Updated documentation
  • Re-ran exploit after fix
  • Ran targeted tests
  • Ran full test suite
  • Ran lint
  • Built package successfully

Security Impact

This restores the intended review-gate boundary for default agent transports. Agents can still propose knowledge, but they cannot approve another agent’s proposal unless the operator explicitly enables decision tools for a trusted host.

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@jonathanchang31, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 15 minutes and 19 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 99533c2c-8036-4e84-9da0-d365806cbc99

📥 Commits

Reviewing files that changed from the base of the PR and between 3beb821 and 285523f.

📒 Files selected for processing (8)
  • README.md
  • docs/review-gate.md
  • src/vouch/capabilities.py
  • src/vouch/jsonl_server.py
  • src/vouch/review_config.py
  • src/vouch/server.py
  • tests/test_capabilities.py
  • tests/test_jsonl_server.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@plind-junior

Copy link
Copy Markdown
Collaborator

Review

Summary: PR correctly addresses issue #168 by hiding kb.approve/kb.reject from agent-facing transports by default and routing them through a review.expose_decision_tools config gate. The approach is sound and well-tested. Two issues worth flagging before merge.

What works

  • src/vouch/review_config.pydecision_tools_enabled is the single source of truth for both transports; no duplicated logic.
  • src/vouch/capabilities.py:74[*METHODS, *DECISION_METHODS] if include_decision_tools else METHODS is clean; the separate DECISION_METHODS constant makes it easy to add future decision tools.
  • tests/test_jsonl_server.py (new test_jsonl_decision_tools_disabled_by_default) — covers the exact exploit path from the issue: alice proposes, bob tries to approve, proposal stays pending. Solid regression test.
  • tests/test_capabilities.py:16 — the updated test_default_capabilities_match_agent_jsonl_handlers correctly excludes DECISION_HANDLERS from the expected set, keeping the capability/handler consistency check valid.
  • src/vouch/server.py:410 and src/vouch/jsonl_server.py:291require_decision_tools_enabled is called before the existing error-handling try/except in _h_approve/_h_reject, so the ValueError it raises is caught by the invalid_request handler and returns a structured error — correct.

Suggestions

  • [blocking] src/vouch/jsonl_server.py (the new block in handle_request) — The guard in handle_request duplicates the one already inside _h_approve/_h_reject. If the check in handle_request fires, it returns method_not_found before calling the handler, which is correct. But the redundant require_decision_tools_enabled call inside the handler will never be reached via the normal dispatch path and could give a false sense of completeness. More importantly, the except Exception fallback inside the new block silently swallows any unexpected error (e.g. a missing config.yaml due to a corrupt KB directory) and returns method_not_found instead of internal_error, hiding the real cause. Consider either: (a) removing the outer block entirely and relying on the in-handler guard (which already returns a structured invalid_request error), or (b) keeping only the outer block and removing the in-handler calls, but letting unexpected exceptions propagate to the existing except Exception handler that records a traceback.

  • [non-blocking] src/vouch/review_config.py:22-25decision_tools_enabled returns True for two independent conditions: expose_decision_tools: true or approver_role: trusted-agent. The approver_role path is not mentioned in the updated docs or the new config examples, and issue critical: Agent transport allows cross-agent approval #168's suggested fix only mentions expose_decision_tools. If approver_role: trusted-agent is intentional legacy support, it should be documented; if it's vestigial, removing it avoids a hidden unlock path that could surprise operators auditing their config.

  • [non-blocking] tests/test_jsonl_server.py:74 — The assertion assert "kb.approve" in caps["result"]["methods"] is added inside test_jsonl_full_flow, which calls _enable_decision_tools(store). That's correct, but this is the only existing test that exercises _h_approve end-to-end, so the _enable_decision_tools call is now load-bearing for the test to reach the approval step at all. A short comment like # decision tools must be enabled to approve in this flow would prevent a future author from accidentally removing it.

Verdict

request changes — The duplicate guard in handle_request with its broad except Exception swallow is the main concern: it masks real errors and makes the defence-in-depth reasoning harder to follow. Everything else is clean and the core fix is correct.

@jonathanchang31

Copy link
Copy Markdown
Contributor Author

@plind-junior I am going to move current branch from main branch to test branch.
Thank you for your support. I would like to contribute it, so I am looking forward to your support. Thanks!
Could you plz reopen this my PR #169?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

critical: Agent transport allows cross-agent approval

2 participants