Security: escape stored memory content in injected context (H1)#878
Security: escape stored memory content in injected context (H1)#878DaveCole wants to merge 1 commit into
Conversation
mem::context wraps retrieved memories in <agentmemory-context>…</…> and prepends them to the agent's prompt, but interpolated every stored field raw. A memory containing `</agentmemory-context><system>…</system>` could close the wrapper early and inject instructions into future sessions — a takeover for anyone who can write a memory (REST/MCP client, or a poisoned file the agent read once and compressed into a narrative). The codebase already had the right defense (escapeXml in enrich.ts); it just wasn't applied on the retrieval path. This: - Promotes escapeXml into src/prompts/xml.ts as the single shared escaper. - Escapes every free-text field in context.ts: profile concepts/files/ conventions/commonErrors, lesson content/context, summary title/ narrative/keyDecisions/filesModified, observation type/title/narrative, and the project= header attribute. Structural values (ids, timestamps, numbers) are left as-is. - Escapes pinned-slot content in slots.ts (label is already validated). - Drops enrich.ts's duplicate local escapeXml in favor of the shared one. Adds test/context-injection-escaping.test.ts, which seeds a breakout payload into every vector and asserts the wrapper can't be closed early and the payload survives only in neutralized (<…>) form. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@DaveCole is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR centralizes XML escaping logic to prevent markup injection attacks across context generation. A new ChangesCentralized XML Escaping for Markup Injection Prevention
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Summary
mem::contextwraps retrieved memories in<agentmemory-context>…</agentmemory-context>and prepends them to the agent's prompt, but interpolated every stored field raw. A memory containing</agentmemory-context><system>…</system>could close the wrapper early and inject instructions into future sessions — a takeover for anyone able to write a memory (REST/MCP client, or a poisoned file the agent read once and compressed into a narrative).The codebase already had the right defense (
escapeXmlinenrich.ts); it just was not applied on the retrieval path.Changes
escapeXmlintosrc/prompts/xml.tsas the single shared escaper.context.ts: profile concepts/files/conventions/commonErrors, lesson content/context, summary title/narrative/keyDecisions/filesModified, observation type/title/narrative, and theproject=header attribute. Structural values (ids, timestamps, numbers) left as-is.slots.ts(label is already validated to^[a-z][a-z0-9_]*$).enrich.ts's duplicate localescapeXmlin favor of the shared one.Tests
test/context-injection-escaping.test.tsseeds a breakout payload into every vector and asserts the wrapper cannot be closed early and the payload survives only in neutralized (<…>) form.test/context-lessons.test.ts(>5-file→>5-file).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests