fix(attachment): stop attachment content from closing its own envelope, and label the region untrusted - #3942
Open
dwin-gharibi wants to merge 3 commits into
Open
Conversation
…e, and label the region untrusted
…hing envelope bug
docker-agent
reviewed
Aug 7, 2026
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR correctly closes the primary delimiter break-out vector (injecting </tag> verbatim) and adds a clear untrusted-data notice. One gap remains in the neutralization regex: the self-closing form <tag/> is not covered. See the inline comment for details and a one-line fix.
The neutralization pattern required a `>` immediately after the tag, so it matched `</document-x>` and `<document-x>` but not the self-closing `<document-x/>`. A model reading the transcript treats that as ending the region just as readily, so the break-out it was meant to close stayed open through that spelling. Allows an optional `/` before the closing bracket as well, which covers `<tag/>`, `<tag />` and `<tag/ >` in any case. Neutralization stays scoped to this envelope's own tag, so unrelated self-closing markup in an HTML attachment (`<br/>`, `<img … />`) is still preserved verbatim — there is now a test for that.
Contributor
Author
|
Done. @aheritier |
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
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.
TXTEnvelope's doc comment claimed tag break-out was "practically impossible". It wasn't: the tag isa deterministic slug of the document name and MIME type — both routinely attacker-influenced — and
the body was interpolated verbatim. Content could close the region early and make injected text look
like it came from outside it.
Closes #3941.
Before
A document named
report.md(text/markdown) produces the tagdocument-report-md-text-markdown. With that in the body:the envelope contained the closing delimiter twice, so the injected line sat outside the first
one as far as the model could tell.
The fix
Two changes to
pkg/attachment/attachment.go, plus an honest doc comment.1. Defuse the envelope's own delimiters in the body. Any occurrence of this envelope's opening
or closing delimiter inside the body is replaced with a visible placeholder:
Case-insensitive and whitespace-tolerant inside the brackets, because a model treats
</DOCUMENT-X >as closing the region just as readily as the exact bytes.2. Label the region. The envelope now opens with a notice:
Without it, attachment text is indistinguishable from the operator's own instructions even to a
well-behaved model.
Three design decisions worth reviewing
The tag stays deterministic. Randomising it per call would also stop break-out — but it would
change the prompt prefix on every request and defeat provider prompt caching for the attachment.
Escaping the body is cheaper and keeps caching intact. The doc comment now says this explicitly so
the next person doesn't "improve" it into a nonce.
Neutralization is surgical, not a blanket escape. Only this envelope's tag is targeted. An
HTML attachment legitimately contains
</div>,</script>, even another document's tag — manglingthose would corrupt the document. There's a test asserting all of those survive verbatim.
The placeholder is visible.
[docker-agent: envelope delimiter removed]rather than silentdeletion or a zero-width character. Silent removal hides the attempt; an invisible substitution
would be worse, since it would still look like a working delimiter to a human reading the
transcript.
Tests
pkg/attachment/envelope_test.go:BodyCannotCloseTheEnvelopeDelimiterNeutralizationIsCaseAndSpaceTolerantUnrelatedMarkupIsPreserved</div>,</p>,</script>, another document's tag all surviveMarksContentAsUntrustedDataShapeIsUnchanged<document-, body present, opening tag still appears verbatim as the closing tag (the invariantTestTXTEnvelope_UniqueTagdepends on)EmptyBodyWritten test-first; the regression and notice tests failed on unpatched code, while the three
compatibility controls passed before and after.
One note on the test-writing itself: my first version of the case/space test asserted against the
whole envelope and so matched the envelope's own legitimate opening tag, producing a false
failure. Fixed with an
innerRegionhelper that strips the first and last line, so a body assertioncan never match the envelope's own delimiters. Worth knowing because the same trap will catch the
next person who extends these tests.
Verification
Toolchain
go1.26.5, darwin/arm64.go test ./pkg/attachment/go test ./pkg/model/provider/...TXTEnvelopegolangci-lint run ./pkg/attachment/...(v2.12.2, CI's pin)go run ./lint .go build ./...,gofmt -lgo test ./...pkg/teamloaderfails — pre-existing (Google Cloud ADC), unrelatedScope — please read
This is one layer, not a solution to prompt injection. It closes the exact-match break-out hole
and gives the model a stated reason to treat the region as data. A model can still be talked into
something by content that never touches the delimiter, and taint cannot be tracked through a model.
The claim is "raises the cost, closes a concrete hole" — deliberately not "prevents injection". I'd
rather scope it honestly than repeat the overclaim the old doc comment made.
It changes prompt text for every text attachment (the notice line, ~30 tokens). That's a
behaviour change across all five providers and may want an eval pass before merge — I have not
measured whether the notice affects task performance either way.