Sanitize peer label before rendering into notification line (SEC-001) - #8
Open
agigante80 wants to merge 2 commits into
Open
Sanitize peer label before rendering into notification line (SEC-001)#8agigante80 wants to merge 2 commits into
agigante80 wants to merge 2 commits into
Conversation
A peer-controlled `label` was interpolated raw into the single-line
stdout notification the receiving session's LLM acts on:
[inter-session msg=<id> from="<name>" "<label>"] <text>
`name` is ASCII-locked and `text` is control-char sanitized, but
`label` passed through neither. `validate_label` permits `"`, `[`, `]`,
so a crafted label could close its quoted field and reconstruct a
second `[inter-session ... from="..."]` header, spoofing the sender
to the receiving agent (which is instructed to act on messages).
Add `shared.sanitize_label_for_display`, which strips control/ANSI and
neutralizes the header-structural characters `"`, `[`, `]`, and route
`from_label` through it in both render sites (client `_format_msg` and
`list` table output). This mirrors the existing server-side `cwd`
sanitization. Legitimate labels (spaces, Unicode, emoji) are
unaffected; regression test added.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WwNCo3qLamBCzhMVAHGuH9
…low-up)
Code-review follow-up to the render-time neutralization: the raw label
was still reachable unsanitized via messages.log — server._log_message
writes from_label verbatim, and json.dumps does not escape `[`/`]`, so a
forged `[inter-session ...]` fragment survived into the log that the
truncated-message flow greps and shows to the receiving agent.
Add LABEL_FORBIDDEN_CHARS = {" ' [ ] "} and reject them in validate_label.
This is a single boundary defense that covers every surface reflecting a
label (notification line, list table, messages.log, future consumers),
instead of relying on each render site to remember to neutralize. The
render-time sanitize_label_for_display stays as belt-and-suspenders for
labels that never passed validation, and now also folds tabs (which
sanitize_for_stdout preserves) so they can't disrupt the list table.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WwNCo3qLamBCzhMVAHGuH9
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
1 participant
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.
Closes #6.
Problem
A peer-controlled
labelwas interpolated raw into the single-line stdout notification the receiving session's LLM is instructed to act on:nameis ASCII-locked byNAME_REandtextgoes throughsanitize_for_stdout, butlabelpassed through neither.validate_labelpermitted"[], so a crafted label could close its quoted field and reconstruct a second, forged[inter-session … from="…"]header — spoofing the sender. The same raw label was also written tomessages.log(_log_message), which the truncated-message flow greps and shows to the agent (andjson.dumpsdoes not escape[/]).Note:
cwd— another peer-controlled string rendered on the same surfaces — is already sanitized server-side before storage (server.py, with a "terminal-escape injection by a hostile peer" comment).labelwas the same pattern, missed.Fix
LABEL_FORBIDDEN_CHARS = {" ' [ ] "}and reject them invalidate_label. One check covers every surface that reflects a label — notification line,listtable,messages.log, and any future consumer — instead of relying on each render site to remember to escape.sanitize_label_for_displaystrips control/ANSI, folds tabs to spaces (so a tab can't disrupt thelisttable's fixed-width columns), and neutralizes"[]to safe look-alikes. Applied in_format_msg(client.py) and thelistrenderer (list.py) for any label that never passed validation.Legitimate labels (spaces, Unicode, emoji) are unaffected.
Tests
tests/test_shared.py—validate_labelrejects"[]and a full forged-header string.tests/test_client.py—_format_msgwith a malicious label yields only one[inter-sessiontoken and no forgedfrom="…".Full suite: 197 passed. (The 4 failing
test_helpers/test_clienttwo-listener tests are a pre-existing server-election race, unrelated to this change.)Companion
SEC-002 (#7) — a reaction-policy note that only the leading notification header is authoritative — is submitted as a separate PR.
Found via a security review of the runtime source.