fix(pabcd-state): close the Windows memory-write-gate bypasses (L6, #135, #136, #141) - #160
Conversation
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. |
![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: ff9fe477e5
ℹ️ 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".
| } else if (verb === "copy-item" || verb === "copy" || verb === "move-item") { | ||
| dests.push(...powershellWriteDestinations(args, true)); |
There was a problem hiding this comment.
When the Windows shell is PowerShell, cp and mv are aliases for Copy-Item and Move-Item, but they are handled by the earlier POSIX branch instead of this new copyLike branch. Thus cp -Destination ~/.codex/memories/n.md -Path ./n.md treats both parameter operands as positional and returns the last one (./n.md), allowing the actual memory destination through the gate without authorization. The amended contract explicitly requires cp/mv to use the -Destination operand in devlog/_plan/260911_memory_recall_sweep/060_wp7_memory_write_gate.md:727-731; include these aliases in the named-destination handling.
Useful? React with 👍 / 👎.
| if (verb === "cp" || verb === "mv") return cpMvDestinations(args); | ||
| if (verb === "perl" || verb === "ruby") return interpInPlaceDestinations(args); | ||
| return []; | ||
| const dests = [...dotnetWriteDestinations(segment)]; |
There was a problem hiding this comment.
The new .NET scanner runs against the entire raw segment rather than the parsed executable expression, so read-only commands such as Write-Output '[IO.File]::WriteAllText("~/.codex/memories/x.md","x")' or rg '[IO.File]::WriteAllText("~/.codex/memories/x.md",...)' file.ps1 return a memory destination and are denied. This contradicts the parser's quote-aware contract and may also consume a one-shot grant without performing a write; restrict the scan to an executable .NET expression or otherwise exclude quoted data operands.
Useful? React with 👍 / 👎.
| function normalizeVerb(verb: string): string { | ||
| return verb.replace(/\.(exe|cmd|bat)$/i, "").toLowerCase(); |
There was a problem hiding this comment.
Stripping .exe makes an explicit sc.exe invocation indistinguishable from PowerShell's sc alias. For example, sc.exe create svc binPath= C:\Users\me\.codex\memories\svc.exe only registers a service executable path, but the file-shaped operand activates the Set-Content handling and the gate falsely denies it as a memory write. The repository plan itself records the sc.exe Service Control collision at devlog/_plan/260911_memory_recall_sweep/060_wp7_memory_write_gate.md:587; preserve this distinction and recognize only bare sc as the write alias.
Useful? React with 👍 / 👎.
|
✅ Target branch corrected This pull request now targets The |
github-actions
Bot
changed the title
The peer session merged its Windows sweep into dev mid-layer, moving origin/dev from a267b39 to d4bef1f and shifting bin/cxc.mjs, which this layer edits. The chain was rebased at P with a clean tree rather than after implementing the largest layer: 31 commits replayed with no conflicts, all six branches force-pushed with lease, ancestry verified layer by layer, and the full suite still shows exactly the two known environmental failures across the peer's seven commits and this chain's twenty-four. The revalidation otherwise found every BEFORE fence exact, confirmed the grant is keyed by a cwd the success message never prints, and restated the PowerShell switch-parameter trap that would let a real memory write through the gate.
The reviewer hunted bypasses instead of reviewing prose and found twelve command lines that would write into the memories directory and pass the gate, including PowerShell unique-prefix abbreviations like -LP and -Fo, the aliases sc and ni, Add-Content, slash switches, py -c, node --eval, glued node -e and [IO.File]::WriteAllText. Chasing those flag by flag is unbounded because PowerShell accepts any unambiguous prefix, so the design changes: for a write cmdlet the parser over-collects every non-flag operand and lets the memory-path check narrow, a flag never consumes the next token unless it is on a short value-taking allowlist matched by prefix, and only the copy and move family stays destination-only so copying out of memories keeps working. The tests now pin the abbreviations and aliases rather than full cmdlet names, the Korean widening drops the object particles so 메모리를 저장해 stays false, and #141 is proven through the real bins rather than dist/cli.js.
…136, #141) The gate is attached to Bash as well as the memory tool, but on Windows it recognised almost nothing an agent would actually type. absolutize only expanded ~/, so a backslash tilde or a USERPROFILE variable escaped the check entirely, and the destination parser knew only POSIX verbs, so every PowerShell write returned no destination and passed. Parsing PowerShell like POSIX was itself the bug: PowerShell accepts any unambiguous prefix of a parameter name, so an audit found -LP, -Fo, -Fi, -Dest and unlisted switches all steered the parser onto the wrong token. A write cmdlet now over-collects every non-flag operand and the memory-path check narrows, which makes the whole class of flag-naming tricks irrelevant; only the copy and move family stays destination-only so copying a file out of memories keeps working. Add-Content, the sc/ni/ac aliases, py, node --eval and the .NET WriteAllText statics are covered. For #135 the grant tells you which cwd it was written to and the deny names the cwd it must be issued from, and 메모리 forms now trigger the gate while 메모리를 저장해 does not. For #141 --help is answered on both real entrypoints with no grant recorded and --session=<id> is accepted.
9fdbe52 to
3212521
Compare
ff9fe47 to
b362256
Compare
The ubuntu CI lane measures the suite and inventory.mjs --check --tests fails when the README badge disagrees. Every layer in this stack adds tests, so every layer publishes its own cumulative total. The diff-derived estimate for this layer was 3132; the measured value reported by CI is 3133, and the measured value is what governs.
b362256 to
daa6b2e
Compare
github-actions
Bot
changed the title
2 participants
The gate is attached to
Bashandapply_patchas well as the memory tool, but on Windows it recognised almost nothing an agent would type:absolutizeexpanded only~/, and the destination parser knew only POSIX verbs. Parsing PowerShell like POSIX was itself the bug — PowerShell accepts any unambiguous prefix of a parameter name, so-LP,-Fo,-Fi,-Destand unlisted switches all steered the parser onto the wrong token. A write cmdlet now over-collects every non-flag operand and the memory-path check narrows; only the copy/move family stays destination-only so copying a file out of memories keeps working. Aliases,Add-Content,py,node --evaland the[IO.File]::WriteAll*statics are covered.Measured with a direct probe over the twelve audited bypasses plus four must-allow controls: parent source 16 misses, this layer 0.
Closes #135
Closes #136
Closes #141
Stack (merge bottom-up)
codex/memory-recall-roadmapcodex/fix-recall-cwd-normalizationcodex/fix-memory-search-semanticscodex/fix-recall-cli-arg-hygienecodex/fix-chat-index-freshnesscodex/fix-recall-intent-regexcodex/fix-memory-write-gateYou are here: L6. Base is
codex/fix-recall-intent-regex. Review this PR's diff only — it is already scoped to this layer.Review focus
the destination parser. This is the security boundary of the stack.
Evidence
Every layer was planned to diff level before any code, audited by an independent
xai/grok-4.6reviewer, and implemented only after the audit's blockers were folded. Each new test was observed failing on the parent tip before it was shown passing; the per-layer receipt indevlog/_plan/260911_memory_recall_sweep/records the exact red output.Local gate:
npm run buildexit 0, andnpm testshowing exactly the two pre-existing environmental failures recorded in002_host_verification_baseline.md(hook-benchhardcodescwd: "/tmp", andcxc map --helpneedspy) and no third.Note on the target-branch check
Enforce PR target branchrequiresdev. Layers L1-L6 legitimately target the layer below, so that workflow will flag them. Do not retarget them todev— that would dissolve the stack. Merge bottom-up; each merge retargets the next child.