feat(tmproto)!: add UntrustedText wrapper for prompt-injection carriers - #478
feat(tmproto)!: add UntrustedText wrapper for prompt-injection carriers#478sujanchalla0510 wants to merge 2 commits into
Conversation
| // boundary markers, regardless of nonce value. Fenced() uses this to defang | ||
| // publisher-supplied content that tries to pre-empt the real boundary — | ||
| // see the Fenced doc comment for why this matters. | ||
| var fenceLookalike = regexp.MustCompile(`<<<ADCP:UNTRUSTED-CONTENT-(BEGIN|END):[0-9a-f]+>>>`) |
There was a problem hiding this comment.
Medium: fenceLookalike matches [0-9a-f]+ only, but the doc comment (L37/L52-53) promises to defang any marker look-alike with "any hex nonce." Hex canonically includes A-F. A publisher who embeds <<<ADCP:UNTRUSTED-CONTENT-END:ABCDEF01...>>> (uppercase) survives the defang verbatim. The primary nonce-check guarantee still holds — but the defang exists precisely for the documented secondary case, "a consumer that only checks the fixed prefix and ignores the nonce." Such a consumer matching case-insensitively would stop at the forged uppercase boundary. Make the class [0-9a-fA-F] (or (?i)) so the code matches the promised shape.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — UntrustedText wrapper (feat(tmproto)!), clean and tightly scoped.
What I checked:
- Wire-shape fidelity: defined string type produces byte-identical JSON, proven by fixture tests through the custom asset MarshalJSON.
- Breaking-change marker present and correct (
feat(tmproto)!:), satisfying the conventional-commit requirement for a public-API/wire change. - Compile-safety of the four field-type changes verified across the repo.
- TMP signing/verify, TEE boundary, and schema/generated-types are untouched — no signing-semantics, tenant/TEE, or schema↔generated-type coherence concerns.
Medium findings (non-blocking):
- tmproto/untrusted.go:46 — fenceLookalike defang regex matches
[0-9a-f]+only, so uppercase-hex marker look-alikes survive despite the doc promising to defang "any hex nonce."
Decision path: no critical/high findings (row 1 n/a); gated_paths is false (row 2 n/a); high_risk is true but the only modified/added files carry a single Medium finding — row 5 requires a Medium on a modified high-risk file, and this Medium is on untrusted.go, an added file (added files are normal scaffolding, not escalation-worthy). No data-loss/schema/infra Medium (row 4 n/a). No prior escalation, no team gate, only 1 Medium total (rows 6–8 n/a). Falls through to row 9 → approve.
bokelley
left a comment
There was a problem hiding this comment.
Please address the inline fence-lookalike finding before merge: match uppercase hexadecimal nonces too and add a regression test. The implementation should satisfy the documented “any hex nonce” guarantee.
Several tmproto fields carry publisher-supplied content that is eventually fed to LLM-based buyer agents — TextAsset.Content, VideoAsset.Transcript, AudioAsset.Transcript, ContextSignals.Summary. The AdCP spec explicitly requires consumers to treat these as untrusted input, but they were plain `string` with only a doc comment: nothing grep-able stopped a caller from splicing publisher text directly into a prompt. Add UntrustedText, a defined string type (tmproto/untrusted.go). JSON encoding is byte-for-byte identical to plain string (proven in untrusted_test.go against both ad hoc structs and the real TextAsset / ContextSignals fixtures), so the wire format does not change — this is a compile-time-only marker. Switch all four carrier fields to UntrustedText. Reading the raw text now requires an explicit string(t.Content) conversion, which is visible in code review and in `grep -rn UntrustedText`. Add UntrustedText.Fenced(), the safe boundary-wrapped form the issue proposed as optional. It wraps content between BEGIN/END markers carrying a fresh 128-bit crypto/rand nonce per call, and defangs any substring of the content that already has the shape of one of our markers (any nonce) before laying down the real boundary. This means: (1) a publisher cannot have pre-crafted a forged closing tag that matches the real nonce, since the nonce doesn't exist until Fenced() runs (~2^-128 collision chance); and (2) a downstream parser that only pattern-matches marker shape rather than checking the nonce still can't be tricked by a publisher-embedded look-alike tag, because look-alikes get neutralized first. The doc comment on Fenced is explicit about what this does NOT guarantee: it is a structural boundary guarantee, not a semantic one — an LLM told to distrust the fenced region can still be confused by adversarial phrasing inside it, and a consumer that ignores the nonce and only checks the fixed prefix gets the defanging protection but not the unforgeability property. Every read/write/construction site for these four fields was grepped across the whole repo (not just tmproto), including targeting/, router/, reference/context-agent, reference/seller-agent, e2e/, and bench/. The only production or test code touched by the type change was inside tmproto itself (validate_ladder.go needed no changes — len() and =="" both work unmodified against UntrustedText — and every other Summary/ Content/Transcript symbol in the repo, e.g. tmproto.Offer.Summary, mcp.Content, is an unrelated field with the same name). Breaking: TextAsset.Content, VideoAsset.Transcript, AudioAsset.Transcript, and ContextSignals.Summary change from string to UntrustedText. JSON callers are unaffected. Go callers that assign a non-constant string variable (as opposed to a string literal, which converts implicitly) into these fields, or that type-assert/compare them against string, need an explicit UntrustedText(...) conversion. This follows the same feat(tmproto)!: convention used for every prior source-incompatible narrowing in this package's history (e.g. the HashURL/url_hash and AssetAccess.Credentials narrowings) rather than PR adcontextprotocol#340's unqualified feat(codegen):, since issue adcontextprotocol#341 (the RFC on this exact question) is still open and unresolved — tmproto's own bump-minor-pre-major release config makes feat: and feat!: version-bump-equivalent today, so this is a documentation choice consistent with existing precedent in this package, revisitable once adcontextprotocol#341 lands a repo-wide policy. Closes adcontextprotocol#50 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFth3sHBrJbHuFeA8Cegdc
fenceLookalike only matched [0-9a-f]+, but the doc comment on Fenced promises to defang any marker look-alike with "any hex nonce" — hex canonically includes A-F. A publisher embedding an uppercase-nonce look-alike marker survived the defang step verbatim. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VPVi4oM4fC7MLRfcFW7KzE
a7d8742 to
37fadc5
Compare
|
Fixed in 37fadc5, and rebased onto main to clear the merge conflict (was against an unrelated doc-comment collision in `tmproto/artifact.go` from the `ValidateFetchableURL` PR that landed on main since this branch was cut — merged both doc notes rather than dropping either). `fenceLookalike` now matches `[0-9a-fA-F]+` instead of `[0-9a-f]+`, per "any hex nonce" in `Fenced`'s doc comment. New test `TestUntrustedText_Fenced_AdversarialForgedClosingTag_UppercaseHexNonce` mirrors the existing adversarial forged-tag tests with an uppercase nonce. Generated by Claude Code |
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — security-hardening delta over a prior clean approve.
What I checked:
- This is a subsequent pass on PR #478. The prior decision was
approve(clean, no findings). The delta since then is one security-hardening commit:fenceLookalikewidened from[0-9a-f]+to[0-9a-fA-F]+so publisher-embedded uppercase-hex marker look-alikes are defanged, matchingFenced()'s documented "any hex nonce" guarantee. - Shipped with a matching adversarial test. Wire format and TMP signing untouched.
- New file
tmproto/untrusted.gois(added)— inherently low-risk scaffolding; the rest are(modified)undertmproto/**with no medium-or-higher concerns.
Decision path:
- No critical/high findings (row 1 no).
gated_paths: false, so row 2 does not apply despitereview_decision: CHANGES_REQUESTED.- No
(deleted)high-risk reasons (row 3 no). - No medium findings at all → rows 4, 5, 8 no.
- Prior decision was
approve, notescalate(row 6 no). - No no-auto-approve team match (row 7 no).
- Falls through to row 9 → approve.
Note: review_decision is CHANGES_REQUESTED, but the deprecated protected-branch / review-status gate does not block auto-approve here because gated_paths is false and no blocking findings exist. This is a strict improvement over the prior-approved state.
Summary
Closes #50.
Several
tmprotofields carry publisher-supplied content that eventually gets fed to LLM-based buyer agents:TextAsset.Content,VideoAsset.Transcript,AudioAsset.Transcript,ContextSignals.Summary. The AdCP spec explicitly says consumers MUST treat these as untrusted input, but until now they were plainstringwith only a doc comment — nothing grep-able stopped a caller from splicing publisher text directly into a prompt.This PR introduces
UntrustedText, a defined string type, and switches all four carrier fields to it.What changed
tmproto/untrusted.go:type UntrustedText string, with a doc comment explaining the fencing requirement, plusFenced() string.TextAsset.Content,VideoAsset.Transcript,AudioAsset.Transcript,ContextSignals.Summaryare nowUntrustedTextinstead ofstring.string(t.Content)conversion — visible in code review andgrep -rn UntrustedText. No shortcut method that returns the raw string under another name was added; that would just relocate the visible cast and defeat the point.tmproto/validate_ladder.goneeded no changes —len(a.Content)anda.Content == ""both work unmodified against the new type.Fenced()design and its guaranteesOutput shape:
Adversarial design goals and how they're met:
{nonce}is 128 bits ofcrypto/rand, generated fresh on every call — it doesn't exist untilFenced()runs, so the publisher cannot have pre-crafted content containing the exact closing tag for this call. Collision probability is ~2⁻¹²⁸.<<<ADCP:UNTRUSTED-CONTENT-END:...>>>with a made-up nonce and hope a lazy parser (one that checks marker shape but not the specific nonce) stops there instead of at the real boundary.Both are exercised adversarially in
untrusted_test.go(TestUntrustedText_Fenced_AdversarialForgedClosingTag,TestUntrustedText_Fenced_AdversarialForgedOpeningTag) — content containing a fake marker with a guessed nonce is confirmed to (a) not survive verbatim in the output, and (b) leave exactly two real marker-shaped substrings in the fenced output: the genuine BEGIN/END pair.Honest limits, documented directly on
Fenced's doc comment:Fenced()makes the boundary unforgeable; it does not make the model immune to what it reads there.Fenced()doesn't sanitize markdown/HTML/control characters — only the marker-shaped substrings described above.Fenced()output across differently-untrusted inputs; the nonce's guarantee is per-call.Wire-format-unchanged proof
UntrustedTextbeing a defined string type means defaultencoding/jsonmarshaling is already byte-identical tostring— but this is proven, not just asserted:TestUntrustedText_MarshalJSON_MatchesPlainString: marshals the same value asstringand asUntrustedText(empty, ASCII, quotes/backslash, unicode, newlines/tabs) and asserts byte-identical output.TestUntrustedText_StructField_WireFormatUnchanged: marshals an equivalent struct with astringfield vs. anUntrustedTextfield and asserts identical JSON; also round-trips a fixed JSON fixture into both struct shapes and confirms identical decoded values.TestTextAsset_Content_WireFormatUnchanged/TestContextSignals_Summary_WireFormatUnchanged: pin the actual production fields against fixed JSON fixtures, so a future accidental customMarshalJSON/UnmarshalJSONonUntrustedTextwould break CI here.Compat-break reasoning (
feat(tmproto)!:)Go callers that assign a non-constant
stringvariable into these fields (untyped string literals still convert implicitly, so most call-site construction is unaffected — verified across the whole repo, see below) now need an explicitUntrustedText(...)conversion. JSON callers are completely unaffected.I used
feat(tmproto)!:rather than an unqualifiedfeat(tmproto):, matching this package's own prior history of source-incompatible narrowings (e.g. theHashURL/url_hashandAssetAccess.Credentialschanges, bothfeat(tmproto)!:). I considered PR #340's unqualifiedfeat(codegen):precedent too, but issue #341 (the open RFC on exactly this question) hasn't landed a repo-wide decision yet, andtmproto'sbump-minor-pre-major: truerelease config makesfeat:andfeat!:version-bump-equivalent today regardless — so this is a documentation-only call consistent with the existing pattern in this specific package, easy to revisit once #341 resolves.Scope check
Per this repo's culture around not under- or over-scoping a change: grepped the entire repo (not just
tmproto) for every read/write/construction site of these four fields —targeting/,router/,reference/context-agent,reference/seller-agent,e2e/,bench/. The only production or test code touched by the type change was insidetmprotoitself. Notably,tmproto.Offer.Summary(a different field, buyer-generated ad-offer description, not publisher content) andmcp.Contentinadcp/are unrelated same-named symbols that this change does not touch.Testing
tmproto/untrusted_test.go(new): wire-format proofs above,Fenced()basic shape across empty/plain/multiline/unicode content, nonce-is-random-per-call (20 calls, no repeats), both adversarial forgery cases, and a pinned nonce-length regression test.tmproto/artifact_test.go,tmproto/robustness_test.go,tmproto/types_test.gofor the handful of assertions/constructions that needed an explicitUntrustedText(...)/string(...)conversion (non-constantstrings.Repeat(...)values, andassert.Equalcomparisons against string literals).go build ./...andgo vet ./...clean at repo root, intmproto/, and workspace-wide (go.work.examplecopied to a local, gitignoredgo.workcovering all 22 modules).go test ./...clean intmproto/, the root module,tmpclient/,adcp/,adcp/v3/,targeting/,reference/context-agent/,bench/.reference/seller-agenthas a pre-existing compile failure (undefined: adcp.ProductAllowedAction,p.Budgettype mismatch) — confirmed viagit stashto reproduce identically on unmodifiedupstream/main, unrelated to this change.golangci-lint run ./...clean in bothtmproto/and the root module.🤖 Generated with Claude Code
https://claude.ai/code/session_01NFth3sHBrJbHuFeA8Cegdc