fix(import): escape triple-quotes in collaborationInstruction #1329
Conversation
…ent docstring injection
Package TarballHow to installgh release download pr-1329-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.14.1.tgz |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Thanks for catching the docstring injection — the security fix is correct in spirit, but the way the two escape helpers are chained introduces a new bug for any input containing backslashes.
escapePySingleQuote already does \\ → \\\\, and escapePyTripleQuote does \\ → \\\\ again. Composing them therefore quadruples every backslash.
I verified by simulating in Python with the same code paths:
User input: C:\path\to\file
After this PR (chained): C:\\\\path\\\\to\\\\file (in source) → C:\\path\\to\\file (at runtime)
Before this PR: C:\\path\\to\\file (in source) → C:\path\to\file (at runtime, correct)
So a collaborationInstruction containing backslashes (Windows paths, regex patterns, JSON-escaped strings, etc.) will be silently corrupted in the generated docstring after this change.
A couple of options to fix:
- Apply only
escapePyTripleQuote. The value lives in a triple-quoted docstring; the surrounding'…'is just decorative text, not Python syntax. Escaping"""and backslashes is sufficient —'doesn't need escaping inside a triple-quoted string. - Compose them without re-escaping backslashes. E.g. introduce a single helper that escapes
\, then', then""", then\nin one pass, or strip the redundant\\→\\\\step from one of the helpers when chained.
Either way, please add a test with a backslash-bearing payload (e.g. 'C:\\path\\to\\file' or 'pattern: \\d+') that asserts the runtime docstring (or at least the emitted source) preserves the original backslash count, so this regression is caught.
A nit on the existing test: the negative assertion not.toMatch(/""".*"""\s*"""/s) is quite weak. Asserting that mainPyContent still parses as valid Python (or at least that the dangerous tokens like import subprocess don't appear at top-level / outside the docstring) would give a stronger guarantee that injection is actually neutralized.
Coverage Report
|
|
Claude Security Review: no high-confidence findings. (run) |
…ent docstring injection
collaborationInstruction is free-form text that gets embedded inside a
Python triple-quoted docstring ("""...""") in the generated main.py.
Using only escapePySingleQuote left """ unescaped, allowing a malicious
collaborator instruction to break out of the docstring and inject
executable Python code into the generated file (HackerOne #3733333).
Fix: use escapePyTripleQuote (escapes """ and \) instead of the previous
escapePySingleQuote for collaborationInstruction. Chaining both helpers
was also incorrect as it doubled backslash escaping.
agentName is not affected — Bedrock enforces [0-9a-zA-Z_-] on that field.
|
Claude Security Review: no high-confidence findings. (run) |
Description
escape triple-quotes in collaborationInstruction