fix(mcp): quarantine tool output returned to MCP hosts (prompt-injection gap)#145
Merged
Conversation
…ion gap) The MCP server wired the consent gate and audit logging but returned RAW tool output to the connected MCP host via NewToolResultText(result). The agent's own dispatch loop quarantines hardware-origin output before it reaches the model (sanitises control bytes, wraps attacker-controllable content in <untrusted-hardware-output> tags, neutralises smuggled close tags). The MCP surface skipped that step — so a hardware-reading tool (NFC/NDEF URI, SSID, BLE device name, SD-card filename, sub-GHz payload) invoked over MCP could smuggle instructions straight into the host LLM's context. Closes that gap. The quarantine logic was previously private to internal/agent (the exported QuarantineOutput existed only for cross-package safety tests). Promoted it to a new dependency-free internal/quarantine package — the correct home for a safety primitive shared by every surface that feeds tool output to an LLM — keyed solely by tool name so both surfaces share one source of truth for which tools are attacker-controllable and how they're wrapped. - internal/quarantine: Output, SanitizeControlChars, KindFor, IsUntrustedHardwareOutput + the notWrapped / auditWrapped allowlists and the close-tag neutraliser. Moved verbatim; policy unchanged. - internal/agent/quarantine.go: now thin forwarders (quarantineOutput, QuarantineOutput, sanitizeControlChars, isUntrustedHardwareOutput) so the agent loop, toolerror.go, setters/eval/adversarial callers, and their tests are untouched — behaviour preserved. - internal/mcp/server.go: wraps both the success result and the handler error via quarantine.Output (errors carry the same risk — an SSID in a failure message). Our own consent / validation messages stay raw. Verified: existing agent quarantine + test/adversarial + eval suites pass unchanged (behaviour preserved); new internal/quarantine package tests (sanitise, per-policy wrapping, smuggled-close-tag neutralisation); new MCP test proves device_info (hardware) is wrapped and list_devices (structured) is not. task ci green (lint 0 / vet / build / test:full 0 fail / govulncheck clean); task eval 17/17. No new tool; registry count unchanged.
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
1 participant
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.
Summary
Safety fix. The MCP server wired the consent gate and audit logging but returned raw tool output to the connected MCP host via
NewToolResultText(result). The agent's own dispatch loop quarantines hardware-origin output before it reaches the model (sanitises control bytes, wraps attacker-controllable content in<untrusted-hardware-output>, neutralises smuggled close tags). The MCP surface skipped that step — so a hardware-reading tool (NFC/NDEF URI, SSID, BLE device name, SD-card filename, sub-GHz payload) invoked over MCP could smuggle instructions straight into the host LLM's context. This closes the gap.Change
The quarantine logic was private to
internal/agent(the exportedQuarantineOutputexisted only for cross-package safety tests). Promoted it to a new dependency-freeinternal/quarantinepackage — the correct home for a safety primitive shared by every surface that feeds tool output to an LLM — keyed solely by tool name so both surfaces share one source of truth.internal/quarantine:Output,SanitizeControlChars,KindFor,IsUntrustedHardwareOutput+ the notWrapped / auditWrapped allowlists + the close-tag neutraliser. Moved verbatim; policy unchanged.internal/agent/quarantine.go: now thin forwarders — the agent loop,toolerror.go, and the setters/eval/adversarial callers and their tests are untouched (behaviour preserved).internal/mcp/server.go: wraps both the success result and the handler error viaquarantine.Output(errors carry the same risk — an SSID in a failure message). Our own consent/validation messages stay raw.Verification
test/adversarial+ eval suites pass unchanged (behaviour preserved).internal/quarantinetests (sanitise, per-policy wrapping, smuggled-close-tag neutralisation).device_info(hardware) is wrapped andlist_devices(structured) is not.task cigreen (lint 0 / vet / build /test:full0 fail / govulncheck clean);task eval17/17. No new tool; registry count unchanged.Safety fix — anchors v0.766.0.