fix(agents): prevent ReDoS in background-session name derivation#90976
fix(agents): prevent ReDoS in background-session name derivation#90976yetval wants to merge 2 commits into
Conversation
|
Codex review: passed. Reviewed June 7, 2026, 3:28 PM ET / 19:28 UTC. Summary PR surface: Source 0, Tests +24. Total +24 across 2 files. Reproducibility: yes. The current-main regex at Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsBest possible solution: Land the narrow tokenizer fix with its regression tests after the exact-head checks and maintainer automerge gates pass. Do we have a high-confidence way to reproduce the issue? Yes. The current-main regex at Is this the best way to solve the issue? Yes. The patch fixes the ambiguous regex at the exact display-label tokenizer, preserves the intended quoted-label behavior with focused tests, and is narrower than replacing this display-only helper with a full shell parser. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 0c33f4e0786e. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source 0, Tests +24. Total +24 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
a9754c3 to
b025a9d
Compare
tokenizeCommand split commands with an ambiguous quoted-span regex where a backslash matched both the escape alternative and the negated class, so an unterminated quote followed by a run of backslashes caused catastrophic backtracking. deriveSessionName runs this on agent-controlled session.command when background sessions are listed/labeled, so a crafted command could hang the event loop. Exclude the backslash from the negated classes to make matching linear, matching the sibling tokenizers that already use the safe form.
b025a9d to
7c55a39
Compare
|
@clawsweeper automerge |
|
🦞🔧 Source: I will update this PR branch, or open a safe credited replacement, if the repair worker finds a narrow CI fix. Automerge progress:
|
|
🦞✅ Source: Why human review is needed: What the maintainer can do as a next step: I added |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
ClawSweeper 🐠 reef update Thanks for the work on this. ClawSweeper opened a replacement PR only because the source branch was not writable from the available bot permissions. branch tides, not contributor blame. Why replacement: ClawSweeper could not update the source PR branch directly; GitHub did not grant sufficient push rights to the bot for that branch.
fish notes: model gpt-5.5, reasoning high; reviewed against 0a38952. |
Summary
tokenizeCommand(src/agents/bash-tools.shared.ts) splits a shell command into tokens with the regex/(?:[^\s"']+|"(?:\\.|[^"])*"|'(?:\\.|[^'])*')+/g. The quoted-span alternatives"(?:\\.|[^"])*"/'(?:\\.|[^'])*'are ambiguous: a backslash matches both\\.and[^"], so a quoted span that never closes forces the engine to try every partition of the backslash run. Runtime is exponential in the number of backslashes after an unterminated quote - classic catastrophic backtracking.deriveSessionName(command), which runs onsession.commandwhenever background bash sessions are listed, labeled, or sent keys (src/agents/bash-tools.process.tsx10,bash-process-references.ts,bash-tools.process-send-keys.ts).session.commandis the agent-issued background command string, so a single crafted background command (e.g. via prompt-injected web/repo/inbound content) pins the Node event loop and stalls the whole agent/gateway process - same impact class as the previously fixed interpreter-heuristic hang (ReDoS in analyzeInterpreterHeuristicsFromUnquoted causes gateway 100% CPU hang #61881).[^"]->[^"\\],[^']->[^'\\]) so a backslash only ever matches the\\.escape alternative. This removes the ambiguity and makes matching linear. It is exactly the form the sibling tokenizers already use (src/agents/sessions/model-registry.ts:219-220,src/agents/tool-display-common.ts:472);tokenizeCommandwas the lone instance still on the unsafe pattern. Well-formed and escaped quoted spans tokenize identically to before.Tests
src/agents/bash-tools.shared.test.ts: added aderiveSessionNameblock - well-formed quoted commands still label as before, backslash-bearing quoted spans still group into one token, and an unterminated quote followed by 50,000 backslashes returns a string in well under 100ms (times out / hangs on the old regex). ExistingresolveSandboxWorkdir/readEnvIntcases unchanged and green.Real behavior proof (required for external PRs)
Behavior addressed: a background bash command whose stored
session.commandcontains an unterminated quote followed by a run of backslashes triggers catastrophic regex backtracking intokenizeCommandviaderiveSessionName, hanging the Node event loop (denial of service) every time the session list / labels / send-keys path derives a name.session.commandis agent-controlled, so injected content can reach it.Real environment tested: Linux, Node 22.19, exercising the actual production
deriveSessionNameexported fromsrc/agents/bash-tools.shared.tsthrough the repo Vitest module loader (no copied regex). The before column ran the unmodifiedorigin/mainsource of that file; the after column ran the patched file; everything else identical.Exact steps or command run after this patch: called
deriveSessionNameon a benign command and onnode "+"\\".repeat(n)for n = 22,26,28,30,32,34, timing each withprocess.hrtime.bigint(), over current headc6829198088with the patch, and (for the before column) over theorigin/mainsource of the same file.Evidence after fix:
Observed result after fix: timing is flat (~0.01ms) across the malicious inputs and benign labeling is unchanged; the event loop is no longer blocked.
What was not tested: a fully wired gateway round-trip that lists a malicious background session through a live channel (the proof drives the exact function that runs on
session.commandrather than issuing a live agent run); the sibling tokenizers inmodel-registry.ts/tool-display-common.ts, which already use the safe form and are unchanged.Before evidence: the same script over the unpatched
origin/mainsource ofbash-tools.shared.ts:Time roughly doubles per +2 backslashes, so n in the 40s reaches seconds and n in the 50s reaches minutes of fully-blocked event loop.