Sanitize an unregistered attribution key in every violation producer - #513
Open
ronleizrowice-ant wants to merge 2 commits into
Open
Conversation
…ucer resolveCommandText returned an unknown key verbatim, so a violation attributed through the observe socket or the macOS log tag could carry control characters the proxy path already collapsed. All three producers now resolve through the same registry-or-sanitize function, and an invocation whose commandId equals its text registers that text so its violations still report the raw multi-line command. Remote-Dev: homespace
With the unknown-key fallback sanitizing, the trusted/untrusted split is simplest as: a key this process wrapped resolves to the embedder's text, anything else is forged and gets control characters collapsed. So drop the "absent id skips registration" special case and key the registry by commandId ?? command, the same key every platform wrapper encodes. An un-keyed multi-line command now reports (and matches ignoreViolations against) its raw text, past the 100-character key, like a keyed one.
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Registering un-keyed commands under the truncated attribution key creates collisions: two commands that share the first 100 characters map to the same key, and the later registration overwrites the earlier trusted text. A delayed violation from the first invocation can then be attributed (and filtered by ignoreViolations) using the second command. Could un-keyed entries use a collision-resistant generated ID, or avoid treating a truncated command prefix as a trusted registry key?
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
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.
Summary
resolveCommandTextreturned an unregistered attribution key verbatim, while the proxy path's own fallback already collapsed control characters. The key reaches the violation store over carriers the sandboxed process can write to (the observe socket's event field, the macOS log tag, the proxy username), so an unregistered key could put newlines or escape sequences into the<sandbox_violations>block through two of the three producers.All three producers now resolve through one function, and the rule is uniform: every wrapped invocation registers its command text under its attribution key (the
commandIdwhen the embedder passes one, else the command itself — the same key the platform wrappers encode), andresolveCommandTextreturns that registered text unchanged for a key this process wrapped and control-character-collapsed text for anything else. Previously an invocation with nocommandId, or one whosecommandIdequalled its text, skipped registration as redundant; with a sanitizing fallback that would have turned its own multi-line command into a single line in the report and forignoreViolationsmatching, so those now register like any other.This is the other half of what Claude Code currently carries as a dist patch on 0.0.73 (the first half is the ancestor pinning in #485 / its rebase); with both merged that patch can be dropped.
Test plan
New
test/sandbox/command-text-registry.test.ts: registered id → verbatim text (control characters kept); id equal to text registers; an un-keyed invocation registers under its own command and resolves to the full text past the 100-character key; a key no invocation registered is control-stripped.registerCommandText/resolveCommandTextare exported for the test, following the existing "Exported for testing" precedent insandbox-utils.ts.Driven end-to-end through the package entry on macOS (seatbelt log monitor) and Linux (seccomp observer + proxy): a multi-line command hitting a write deny reports its raw multi-line text in the violation's
commandwith nocommandId, withcommandIdequal to the command, and with a distinctcommandId+commandText; a proxy request from inside the sandbox with a forgedsrt.<base64>username is recorded with the forged key's control characters collapsed.