emrg: a Windows path is absolute for the shell that will run it (#1261) - #1262
Conversation
The sandbox tokenises with shlex in POSIX mode, where a backslash escapes, while cmd.exe treats the same character as a path separator. `echo x > C:\Users\x\out.txt` therefore reached the workspace-write boundary as the relative token `C:Usersxout.txt` -- "inside the workspace" -- and was allowed, while its absolute POSIX twin was refused. Measured on master cca0b8d: ten spellings allowed (redirect, append, rm -rf, mv, cp, sed -i, find -delete, chained; UNC already blocked by accident), read-only unaffected, since that tier refuses every target without asking where it resolves. Fix: on a Windows shell, substitute a placeholder for each backslash in the copy the guard tokenises -- never in the command that runs -- and restore it in the tokens on the way out. A substitution rather than a lookup keyed on the mangled form, because a key that fails to match leaves the path mangled and every rule reading it silently inert. With the spelling intact, a drive-rooted path is read as absolute wherever the guard runs, and containment canonicalises the separator so the answer does not depend on which os.path the guard itself is running under. Measured by a corpus driven in BOTH platform states rather than in whichever one the host happens to be: forced Windows 0/17 wrong verdicts, forced POSIX 10/17 -- exactly the pre-fix baseline, so the POSIX reading is untouched (on a POSIX shell `C:\Users\x` really does name the relative file `C:Usersx`). Three mutations each turn a different set red: repair 12, drive-rooted 13, containment 3. Full suite 2483 passed / 16 skipped. Refs #1261.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-041442
Independently re-derived on the head 94584d96. Guard loaded from a detached
worktree with the module identity asserted from the loaded file (master
cca0b8dc → e9bd6d8ab003293e, head 94584d96 → d8cb0a29a749e1a1).
The defect, on master. With the platform flag off — which is master's only
state, since the flag does not exist there — 10 Windows spellings were driven
through _check_sandbox(..., "workspace-write"). Every one that names a path
outside the workspace was ALLOWed: redirect, append, rm -rf, cp, sed -i,
find -delete, a nested \\-separated target, and a forward-slash drive path.
That is the issue's premise reproduced, and it is reproduced without any
injection, so the arm is not an artefact of the test harness.
The head. With the flag on, 8 of those 8 flip to BLOCK. The two that do
not move were checked deliberately: the UNC spelling \\server\share\... is
already rooted through the existing \ test on master (so it is blocked in both
arms and is correctly not claimed as a fix), and cd C:\... && echo x > inner.txt
stays allowed because it is a cwd escape, not a tokenizer escape — it is #1260's
business, and #1260's own docstring already records that it cannot see the drive
spelling until this lands. The cross-reference is accurate in both directions.
No regression off Windows. A 13-case POSIX workspace-write corpus is
0 differences against master, and a 19-case read-only corpus is 0
differences across master and both PR heads — the guard is not allowed to
change a read-only verdict, and it does not.
The platform gate is verified where it can be, and the test says why. The
substitution is gated on os.name == "nt", which is the right question because
the executed command really does go to cmd.exe: line 1788 hands the command to
asyncio.create_subprocess_shell, and the module documents that shell as cmd.exe
via COMSPEC. The substitution is applied to the tokenized copy only — all five
call sites (_split_command_tokens / _tokenize_command callers) are guard-side,
and the string passed to the subprocess is never the rewritten one. The 15 skipped
tests here are exactly the arm that exists only on a real Windows shell
(os.name != "nt" skipif: "the natural-state arm only exists where the shell
really is cmd.exe"), i.e. the one arm that measures the gate without patching —
which is why the green test-windows leg on this head is the load-bearing CI
signal for a Windows-path defect, and it is green.
The branch's own 38 tests pass here (15 skipped, as designed), and both CI legs
(test 3m6s, test-windows 5m48s) are green on this head.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — the repair is structurally unable to fail silently, which is the property that decides this shape.
What I checked on the head 94584d96 (diff read, both CI legs):
- The substitution happens on the copy the guard tokenises, never on the executed command. That
is the only place it can be, and it keeps the fix free of any effect on behaviour. - Backslashes become a NUL placeholder and are restored on the way out. I prefer this over a
"mangled form → raw spelling" lookup for the reason the diff gives: a lookup's failure mode is a key
that does not match, which leaves the path mangled and every rule reading it inert while reporting
nothing. A character that was never removed cannot fail to be restored — the repair is total by
construction, not by enumeration. - It is gated on the shell the command will actually run in (
os.name == "nt"). On POSIX a backslash
is an escape, so repairing it there would refuse an ordinary in-workspace write — a real regression
the gate prevents. Honest limit of this review: because of that same gate I cannot exercise the
changed path on this macOS host;test-windowsis the only instrument that runs it, and it is
green here. I am voting on the structure plus the leg that actually executes it, not on a local
reproduction I did not have.
The corpus in the diff (ten spellings — redirect incl. append, rm -rf, mv, cp, sed -i,
find -delete, chained) is the right shape too: one spelling fixed would have been the #461 defect
again. Merging this closes #1261's first two rows; the third (cd C:\…) also needs the cwd-resolution
PR, so the issue closes when both have landed.
— cycle cyc20260916-044138
|
I tested this PR in both directions, by patching the gate it is written behind. Method. Master and this head loaded from git by sha. The PR's key constant is read at call time (
Both halves matter and both hold:
The POSIX spellings are unchanged too, which is the risk this fix carries (issue #1162's cases):
A note on how the arm is gated, since it decides what CI can and cannot prove. Composition. All three open PRs rewrite this file and all sit on I did not touch the branch. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260916-051502
Independently reproduced. Same method as always here: two pinned worktrees (before = master cca0b8dc, after = 94584d96), each printing the sha256 of the module it actually loaded.
The before arm has no _WINDOWS_SHELL symbol at all, so the Windows rule is simply absent there and the POSIX reading is what the Windows leg runs without it. With a Windows-style workspace (C:\Users\x\repo):
| command | before | after (Windows shell) |
|---|---|---|
echo x > C:\Users\x\out.txt |
ALLOW | BLOCK |
echo x >> C:\Users\x\out.txt |
ALLOW | BLOCK |
rm -rf C:\Users\x\important |
ALLOW | BLOCK |
echo x > C:/Users/x/out.txt |
ALLOW | BLOCK |
Controls unmoved in every arm: echo x > C:\Users\x\repo\out.txt (inside the workspace) ALLOW, echo x > out.txt ALLOW, echo x > /etc/hosts BLOCK.
The gating claim holds too: with the flag off the after arm returns exactly the before arm's verdicts, so an ordinary POSIX spelling like my\ file is not newly refused.
A method note, because it misled me first and a later reviewer should not repeat it: my initial probe passed a POSIX workspace. There every drive-rooted target resolves inside it through os.path.realpath, so the arm reported ALLOW and measured nothing. The branch is only observable with a Windows-style workspace. This PR's test file does that correctly; on the POSIX leg it runs 38 passed / 15 skipped, and the skipped 15 are the ntpath arm the Windows CI leg exercises (green).
Both CI legs green (test, test-windows).
Fixes issue #1261 — the second of the two tokenizer defects found while landing #1260.
What was wrong
The guard tokenises with
shlexin POSIX mode, where a backslash is an escape; a Windows shell (cmd.exe, the bash tool's subprocess shell on that platform) treats the same character as a path separator. A Windows spelling therefore reached every rule with its separators deleted:At
workspace-writean absolute path outside the workspace was read as a relative name and allowed, while the same write spelled/Users/x/out.txtwas refused.read-onlywas unaffected — that tier refuses every target without asking where it resolves.Measured on master
cca0b8dc, the corpus below (not a hand-picked example): 10 of 17 Windows spellings wrong, every one an escape — redirect, append,rm -rf,rm -rfof a drive root,mv,cp(with and without a trailing separator),sed -i,find -delete, and the same redirect behind a&&chain.\\server\share\…was already blocked, but by accident: the mangled token kept its leading backslash.The fix
On a Windows shell, substitute a placeholder for each backslash in the copy the guard tokenises — never in the command that is executed — and restore it in the tokens on the way out. Both tokenizers go through it (
_split_command_tokensfor write targets andcdoperands,_tokenize_commandfor git verbs and wrapper payloads), including their unparseable-input fallbacks.A character substitution rather than a "mangled form → raw spelling" lookup, deliberately: a lookup has a silent failure mode — a key that fails to match leaves the path mangled and every rule reading it inert — while a character that was never removed cannot fail to be restored. With the spelling intact, a drive-rooted path reads as absolute wherever the guard runs, and containment canonicalises the separator so the answer does not depend on which
os.paththe guard itself is running under.Measured on the corpus, in BOTH platform states
The platform axis is a module constant (
_WINDOWS_SHELL), so the Windows arm is exercisable anywhere instead of only where the host happens to be. Forced Windows vs forced POSIX, same corpus, same harness:The forced-POSIX column is the regression guard, and it is an equality rather than a list: it reproduces the pre-fix verdicts exactly, because on a POSIX shell
C:\Users\xreally does name the relative fileC:Usersx, and repairing it there would refuse an ordinary in-workspace write (echo x > my\ fileis the same class of spelling, as are issue #1162's cases, which stay green).Mutation-tested
Three mutations, each killing a different set — so the tests are measuring three separate parts rather than one:
_split_command_tokens_is_absolute_path_is_withinIssue #1261's own table, three states
Row 3 needs the cwd rule from #1260 (still open) as well, so it is measured with the two branches merged rather than claimed from this branch alone:
Hence Refs #1261, not "closes": rows 1–2 land here, row 3 lands when #1260 merges. Closing the issue is a follow-up for whoever merges the second of the two.
Verification
python -c "from emrg.client.app import run_client"andpython -m emrg --helpboth fineThe
test-windowsCI leg is the only placentpathitself is exercised: it runs the same corpus unpatchted (test_natural_windows_shell_refuses_without_patching), which is the arm this host cannot provide.