fix(security): shared agent-token operation policy — REST server mutations 403 for agents; index/search honors quarantine (#878, #877)#888
Merged
Conversation
…search honors quarantine Two related gaps let an agent token (mcp_agt_) do over REST what it is denied over MCP, and let quarantined servers' tool descriptions leak via REST search. #878 — REST mutating /api/v1/servers/{id} routes (enable/disable/restart/ quarantine/unquarantine/patch/remove/login/logout/config-to-secret/refresh/ discover-tools, tool approve/block, and the top-level add/import/reconnect/ *_all bulk routes) had no agent-token gate, while the MCP upstream_servers / quarantine_security tools deny them. Extract one shared policy (internal/auth.AuthorizeServerOp + AgentDeniedServerOp) consumed by BOTH the MCP denylist and a REST requireServerOp wrapper, so the surfaces can't drift. Agent tokens now get 403 "operation requires admin access"; admin keys and OS-auth socket connections are unaffected. #877 — GET /api/v1/index/search served the tools of server-quarantined upstreams (their descriptions are the TPA payload quarantine withholds). server.SearchTools now applies the same server-level visibility gate retrieve_tools relies on, reading authoritative quarantine state from storage; response shape is unchanged (bare tool name + separate server_name), only which tools appear is filtered. Tests: table-driven httpapi test over every mutating route (agent=403, admin=next-layer); auth policy unit tests; a deterministic E2E reproduction (index-then-quarantine) that fails if the REST filter is removed; and the previously-vacuous TestE2E_QuarantineConfigApply step-4 assertion rewritten to grep server_name (what the surface returns) instead of a prefix it never emits. OpenAPI + docs (rest-api.md, agent-tokens.md) document the 403 behavior and the search quarantine gate.
Deploying mcpproxy-docs with
|
| Latest commit: |
ad7b4f4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4dbec464.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-878-877-agent-token-gati.mcpproxy-docs.pages.dev |
…d search filter Codex round-1 review of PR #888 found three genuine gaps: - Agents could bypass the /servers gate via sibling mutating routes: POST /config/apply and PATCH /config[/docker-isolation] (config can add/remove/enable/disable servers), the registry keystone add (add_from_registry equivalent), registry source CRUD, and the per-server security scanner (scan start/cancel, security approve/reject). All now route through requireServerOp with new shared-policy op ids (config_write, scan, security_approve/reject). - The index/search quarantine filter failed OPEN on an empty server name, nil storage, or a storage error — a stale/legacy index entry or transient BBolt failure could expose withheld descriptions. It now fails CLOSED: a hit is withheld unless positively attributed to a resolvable, non-quarantined server. Table-driven httpapi test extended to cover the new routes; docs + OpenAPI 403s updated.
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 29900225671 --repo smart-mcp-proxy/mcpproxy-go
|
…es for agents Codex round-2 review of PR #888 found three more agent-token-reachable admin mutation surfaces outside /servers: - Secrets: POST /secrets, DELETE /secrets/{name}, POST /secrets/migrate — write or delete keyring credentials and restart affected servers. - Global security scanner: enable/disable/config/install/remove scanners and scan-all/cancel-all batch runs. - POST /diagnostics/fix — executes administrative fixers (OAuth reauth, scanner disable, config migration). All now route through requireServerOp with shared-policy op ids (secret_write, scan, diagnostics_fix). Token management was already admin-gated (requireAdminAuth) and needs no change. Table test + policy test extended.
Codex round-3 review of PR #888: POST /connect/{client}, its /undo, and DELETE /connect/{client} write, restore, or delete local MCP client config files and may embed the admin API key into that config. An agent token could trigger them to exfiltrate the admin key or mutate client config. Gate all three via requireServerOp (config_write); status/preview reads stay open. Table test extended.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Threat model
Agent tokens (
mcp_agt_) authenticate against the REST API but are meant to be scoped, read-oriented — the MCP surface already denies them server administration. Two gaps let them (or any REST caller) exceed that boundary:/api/v1/servers/...routes had no agent-token gate. An agent blocked fromenable/disable/restart/quarantine/… on the MCPupstream_servers/quarantine_securitytools could perform the same operation over HTTP.GET /api/v1/index/searchreturned the tools of server-quarantined upstreams. Those descriptions/schemas are exactly the Tool Poisoning Attack payload quarantine exists to withhold; only the MCPretrieve_toolspath filtered them.Fix
internal/auth):AgentDeniedServerOp/AuthorizeServerOpname the server/tool-mutating operations denied to agent tokens. Consumed by both the MCPupstream_serversdenylist and a RESTrequireServerOpwrapper, so the two surfaces cannot drift again. The MCP denylist behavior is byte-preserved (list/tail_logstill allowed)./serversroute (add/remove/patch/enable/disable/restart/reconnect/quarantine/unquarantine/login/logout/config-to-secret/discover-tools/refresh, tool approve/block, and the*_allbulk routes) returns403 operation requires admin accessfor an agent token. Full API keys and OS-authenticated socket (tray) connections are unaffected.server.SearchToolsapplies the same server-level visibility gateretrieve_toolsrelies on, reading authoritative quarantine state from storage. Response shape is unchanged — bare toolname+ separateserver_name(the#871/mcp-eval contract); only which tools appear is filtered.Tests
internal/httpapitest iterating every mutating server route: agent token → 403, admin key → next-layer (back-compat).internal/authpolicy unit tests.TestE2E_IndexSearchHonorsQuarantine(index-then-quarantine) — Bleve still holds the entry,SearchToolsreturns 0; fails if the filter is removed.TestE2E_QuarantineConfigApplystep-4 assertion rewritten to grepserver_name(what the REST surface returns) instead of aserver:prefix it never emits — the old check was vacuously true and masked the leak.OpenAPI (
oas/swagger.yaml) + docs (docs/api/rest-api.md,docs/features/agent-tokens.md) document the 403 behavior and the search quarantine gate.Fixes #878
Fixes #877
Bypass-route hardening (cross-model review)
A shared policy is only as good as its coverage. Cross-model review surfaced sibling routes through which an agent could reach the same server/security/config/credential state without touching
/servers. All are now gated through the samerequireServerOpwrapper +internal/authpolicy:POST /config/apply,PATCH /config,PATCH /config/docker-isolation(config can add/remove/enable/disable servers)POST/PUT/DELETE /registries[/{id}]) and the keystone add (POST /registries/{id}/servers/{serverId}/add)scan,scan/cancel,security/approve,security/reject) and global (scanners/{id}/{enable,disable,config},scanners/install,DELETE scanners/{id},scan-all,cancel-all)POST /diagnostics/fix(executes admin fixers — OAuth reauth, scanner disable, config migration)POST /connect/{client}+/undo+DELETE(write local client config, can embed the admin API key)Token management (
/api/v1/tokens*) was already admin-gated (requireAdminAuth). The index/search quarantine filter fails closed (an unresolvable server name, nil storage, or a lookup error withholds the hit).