Repository navigation
CRW-583: join Path parts and decode string escapes in the shell write-destination reader - #555
Merged
Merged
Conversation
…erals Red on origin/dev 7e5c55d: the shell write-destination reader names the first part of a multi-part Path call and leaves Python and JavaScript string escapes undecoded.
…tinations The memory write gate reads ShellWriteDestinations. A Path call with several literal parts now names posixpath.join of them, a Python literal is decoded as Python reads it, and a JavaScript string or template literal is read as JavaScript reads it. Each new destination is appended after the oracle's answer, which stays first and unchanged. The one recorded case whose escape now decodes is tagged intentionally-changed.
The CRW-517 Path-join line becomes port: fixed, the wrapper line gains the reason Sudo is lowercased, and a new section holds the escape line and the follow-ups that stay kept.
A fresh review found three gaps. A JavaScript \u{d83d}\u{de00} pair became two replacement characters, so the real path was not named. A Python surrogate escape (\udcc3) is a raw byte under os.fsencode and was also replaced; every other Python surrogate cannot be encoded and now names nothing. The Path join copied the accumulated path for each part, which is quadratic in the number of parts; it now grows one buffer. Tests for each were red on 466476c.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…iteral
A Path call mixing literal and dynamic parts (Path('/m', name)) named nothing after the join change; the memory write gate then had no destination for a write under a protected directory. The tests expect the literal prefix the write lands under. Red on ed94590.
…ot a literal
An argument that is no literal leaves the rest of the path unknown, so the call names the join of the literal parts before it (Path('/m', name) is /m) until an absolute literal part starts the path over. This keeps every destination the earlier reading named for such a call.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed9459085f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
open(path, '\N{LATIN SMALL LETTER W}') writes, but the literal is rejected as a whole, so the write was no longer named. Red on 0bda818.
A \N{name} escape cannot be decoded without the Unicode name table, so a mode written with one is read as writing and the call is still named. The earlier reading classified it as writing only when the escape text happened to hold a lowercase w, a, x or +.
…a dynamic part A fresh review showed the join restarting at an absolute f-string with a field and dropping the literal base the earlier reading named. The tests expect that base, and the first literal argument, to stay named for such a call. Red on 2bb351f.
A Path call with an argument that is no literal, or an f-string with a field, now also names its first argument when that is a literal, as the earlier reading did, so the join never names fewer destinations than before. Calls whose arguments are all literals are unchanged.
…side the decoded one
A fresh review showed the decoded reading replacing the earlier one: an open call in keyword form with a \N{name} escape or a NUL named nothing, and the first argument of a Path call with a dynamic part was named in its decoded form, which can normalize out of a protected directory. The tests expect both readings. Red on e7821dd.
The decoded reading added to the earlier one but also replaced it in an open call and in the first argument of a Path call with a dynamic part, so a path the earlier reading named under a protected directory (a keyword-form open with a \N{name} or NUL escape, an f-string whose decoded text normalizes elsewhere) was lost. Both readings are named now; an all-literal Path call still names only its join.
…d keeping a single Path argument A fresh review found the earlier reading's mode (the x of a hex escape) admitting the decoded path of a read, and a single-argument Path call losing the earlier reading of its escape. Red on d646401.
…ment Each reading of an open call now names its path only when its own mode writes (a mode written with a named escape counts for both), and a Path call with one argument keeps the earlier reading of it, so the decoded reading adds to the earlier one without making a read look like a write or dropping the earlier path.
The coordinator refreshed the base. The only conflict was in docs/port-cxc/known-defects.md, where both sides appended lines at the same place (1 block(s)); both sets are kept, dev's lines first, then this branch's.
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.
Summary
The shell write-destination reader (
ShellWriteDestinations) feeds the fail-closed memory write gate: the gate blocks a command when any destination it names is protected. Two kinds of write that reach a protected path were not named. APathcall with several literal parts was read as its first part or as the text between its first and last quote (Path('/tmp/b', '/m/a')gave/tmp/b', '/m/aand/tmp/b, never/m/a), and a string literal written with escapes was named letter for letter (open("\x2fm/a", "w")gave\x2fm/a; node'swriteFileSync("\x2fm/a", ...)the same). The oracle misses these too. Because the reader is a gate input, this PR fixes them the way the earlier shell-write port fixed its other misses: the real path is appended after the oracle's answer, which stays first and unchanged. The decoded reading is added to the earlier reading of a literal and never replaces it, with one required exception: aPathcall of several literal arguments names its join, not its first part.Expected behaviour and acceptance criteria
Path(...).write_text/.write_bytesnamesposixpath.joinof its arguments when every non-blank argument is a string literal (a part starting with/discards the parts before it, nothing else is normalized, a trailing comma leaves a blank).Path('/tmp/b', '/m/a')andPath('/m', 'a')name/m/a;Path('/m/a', '/tmp/b',)names/tmp/band not/m/a.Path('/m', name)names/m,Path('/w', name, '/m', 'x')names/m/xand/w,Path(name, 'a')names nothing. A call with a single argument keeps the earlier reading of it as well. Deviation from the stated criteria: the assigned answer was thatPath('/m', name)names nothing. Devin's security finding and a Codex P1 on this PR both showed that this drops a destination the previous reading named (/m, the directory the write lands under), so a write under a protected directory through a dynamic name would reach the gate with no destination. The prefix reading fixes that and keeps every destination the previous reading named; it is one function (shellWriteEscapePath) and one test table, and is easy to revert if the nothing-answer is wanted.\\ \' \" \a \b \f \n \r \t \v, octal of one to three digits,\xhh, and in a str literal\uXXXXand\UXXXXXXXX. An unknown escape keeps its backslash; a raw literal stays literal;\N{...}, a short or oversized\x/\u/\Uand a NUL make it no literal for the decoded reading. In a bytes literal\u,\Uand\Nare plain text, an escape is a byte and octal wraps at eight bits. In a str literal a surrogate escape U+DC80 to U+DCFF is the byte thatos.fsencodemakes of it (so\udcc3\udca9isé) and any other surrogate names nothing.open(...)names the path of each reading, the earlier one (which kept every escape but a backslash before the quote or another backslash as written) and the decoded one, each when its own mode writes; the decoded reading also reads the mode, soopen(p, '\167')counts as a write. A mode written with a\N{name}escape cannot be decoded (it needs the Unicode name table), so it counts as writing for both readings (a read written that way is now named: the gate fails closed).writeFile*,appendFile*orcreateWriteStreamis read as JavaScript reads it (\xhh \uXXXX \u{...},\0, legacy octal,\b \f \n \r \t \v, line continuation, any other\casc, surrogates joined as UTF-16 units) and the decoded value is appended when the result does not already hold it. A template with an unescaped dollar-brace placeholder has no decoded value (its raw text, which the earlier port already reported, stays).plugins/crw/wiring/hooks,plugins/crw/skills, any.codex-plugindirectory),go.mod, or any file of another change; no CGO, no Node at run time, noinit()or package-level initialization; every new package-level name is prefixedshellWriteEscape.Oracle sources ported
CXC v0.2.40 (commit 3c1459ac),
plugins/codexclaw/components/pabcd-state/src/shell-write-destinations.tslines 505-549 (pythonNodeWriteDestinations,scriptWriteDestinations: theopen,PathandwriteFile*patterns). The oracle's answer is produced exactly as before; this PR changes only the additive reading ininternal/pabcd/hook/shellwrite_verbs.go.What changed
internal/pabcd/hook/shellwrite_verbs.go:shellVerbLiteraldecodes the body at the closing quote (shellWriteEscapePython,shellWriteEscapeDigits; the scan isshellWriteEscapeLiteral, whoseearlierswitch gives the previous reading throughshellWriteEscapeUnquote);shellVerbOpenCallnames the path of each reading; thePathframe ofshellVerbOpenWritescallsshellWriteEscapePath(one linear buffer;shellWriteEscapeFieldfinds an f-string field); the hardened reading ofshellVerbScriptWritesaddsshellWriteEscapeJSWrites, an escape-aware reader of the JavaScript literal (RE2, linear) whose bodyshellWriteEscapeJSdecodes. The oracle's own lazy.*?patterns stay as they are: their dot stops at an escaped quote and never crosses a line terminator, so a path hidden by an escape or a line continuation is not in their raw text.internal/pabcd/hook/shellwrite_escape_test.go(new): the cases of the change throughShellWriteDestinations(the oracle's answer,shellVerbOracleof the tokens, must be a prefix of each result), a decode table for Python literals and one for JavaScript literals, the unit reading ofshellVerbScriptWrites, and a linearity test for thePathjoin.internal/pabcd/hook/testdata/shellwrite/{record-shellwrite-verbs.mjs,verbs-oracle.json}: the one recorded case whose escape now decodes,python3 -c "open('/m/a\nb','w')"(a real newline joins the oracle's raw/m/a\nb), is tagged intentionally-changed with its reason. The recorder gained the matching addition; re-recording with Node 24 against the v0.2.40 build into a scratch file reproduced the checked-in file except for that entry (342 commands and 353 function answers, the oracle's outputs unchanged). This is the one edit outside the shell-write source, test and documentation files.docs/port-cxc/known-defects.md: the Path-join line of the shell-write section is narrowed to the multi-argument call and markedport: fixed; its wrapper line gains the reasonSudois lowercased (on a case-insensitive file system, the macOS default,Sudoruns sudo, so reporting its write is the fail-closed choice); a new section at the end holds the escape line (port: fixed) and twoport: keptlines.Corpus fixtures (contract/fixtures/cxc)
Two fixtures reach this reader, both through the memory write gate leg of the PreToolUse hook (
pre-tool-use-guarding-memory-write, which callsShellWriteDestinationsinmemorygate.go) and replayed byTestDomain/cxc:>passes)The memory write gate port's notes file already claims both as identical, and the replayer refuses a second claim on a fixture, so this PR adds no
contract/notes/cxc/CRW-583.json: it makes no fixture newly pass. The replay stays green (12 memory-write fixtures replayed).Evidence
origin/dev7e5c55d 42 subtests fail by assertion (no compile error) and reproduce the report:Path('/tmp/b', '/m/a')gives["/tmp/b', '/m/a" "/tmp/b"],open("\x2fm/a","w")gives the raw text only. Every later fix (surrogates, linear join, dynamic Path parts, named-escape modes, the earlier reading kept beside the decoded one) also starts with a test commit that is red before the fix; the red outputs are kept with the receipts.go test -count=1 ./internal/pabcd/hook/ok;go test -count=1 ./internal/contracttest -run TestDomain/cxcok;make lint(vet, staticcheck, gofmt) ok;GOOS=darwin go vet ./...ok;CGO_ENABLED=0 go build ./...ok;go run -tags dev ./cmd/crw-dev ci validate,ci contractsok andci pluginok with the digest unchanged (0236789ec4aa1c59, the same as on dev);git diff --check origin/dev...HEADok; gitleaks over the branch: no leaks; no blob over 1 MB. The branch conflicts with newer dev only indocs/port-cxc/known-defects.md(both sides append at the end), so CI was dispatched on the head by hand; a merge of the head with dev builds and passes the hook tests.Pathparts hide protected writes) and the Codex Code Review two P1 findings (the same one, and a mode written as a named Unicode escape no longer classified as a write); all are fixed in commits after the first head, tests first, answered on their threads and resolved. The Codex Security Review was skipped by its usage limit.Size
Counted diff 579 lines, generated data (the recorded JSON, 2 lines) excluded: implementation 324, tests 240, recorder 3, documentation 12.
Defects found
Pathcall and string escapes: fixed (security), as above.\N{name}(valid Python that needs the Unicode name table) and the/operator formPath("/m") / "a"stay unresolved:port: keptfollow-ups (the earlier reading and the oracle's raw text are still named).Pathcall of several literal arguments no longer names its first part (required); a literal Python rejects or whose value holds NUL has no decoded path (the earlier reading is still named).ShellWriteDestinationsmerges the oracle's and the added answers per segment, so across segments an addition of an earlier segment precedes the oracle's answer for a later one (sudo -n tee /m/a; tee /m/banswers/m/a,/m/b; the oracle/m/b, on origin/dev as well). The memory gate returns on the first protected destination, so order does not change a verdict.Out of scope
shellwrite.goand the gate (memorygate.go), implicit adjacent-literal concatenation and f-string placeholders as values (unchanged),os.openand other write APIs, heredoc-fed programs, and every other shell-write follow-up of the earlier port. The known-defects file may show a merge conflict with newer dev: both sides append at the end of the file.