Skip to content

fix(sandbox): reject Windows drive-letter host paths in the host-bash guard - #7089

Closed
BlueX888 wants to merge 1 commit into
bytedance:mainfrom
BlueX888:fix/host-bash-windows-drive-guard
Closed

BlueX888 wants to merge 1 commit into
bytedance:mainfrom
BlueX888:fix/host-bash-windows-drive-guard

Conversation

@BlueX888

@BlueX888 BlueX888 commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Why

On a Windows host with sandbox.allow_host_bash: true, the local host-bash guard forwards any command that spells a host path with a drive letter to PowerShell, cmd.exe or Git Bash. On main at ce0fa4c1:

  • cat C:/Windows/win.ini and cat 'C:\Windows\win.ini' run against the host file, no error
  • cat C:/ and cat C:\ — the bare drive roots are forwarded too
  • cd C:/Windows/System32 && cat drivers/etc/hosts — the model-issued cd overrides the cd <workspace> && anchor, so the relative read lands in a host directory

_ABSOLUTE_PATH_PATTERN needs a / that is not preceded by :, so a drive path never matches, and _validate_local_bash_cwd_target only inspects targets starting with /, $ or ~.

What changed

On a Windows host only, the guard now scans the raw command for the spellings that are unambiguously host paths and rejects them with the existing "Unsafe absolute paths" / "Unsafe working directory change" errors:

  • drive paths C:/…, C:\…, C://… and C:/, found wherever they appear, including inside an interpreted script (python -c "open('C:/Windows/win.ini')", powershell -Command "Get-Content C:\Windows\win.ini") and after an attached short option (curl -oC:/Windows/evil.exe, git -CC:/Windows)
  • Win32 device and extended-length prefixes (\\?\C:\…, \\.\PhysicalDrive0)
  • UNC paths (\\server\share\…)
  • variable expansions (%WINDIR%\win.ini, $env:SystemRoot\win.ini, $HOME\.ssh\id_rsa)
  • a working-directory target that is a drive (cd C:, cd C:/Windows, cd C:Windows), a backslash root (cd \Windows) or a variable (%WINDIR%, $env:WINDIR); cmd chdir and PowerShell Set-Location / Push-Location / sl are handled as working-directory commands too
  • unquoted ..\..\ arguments, whose backslashes the token splitter drops

Arguments that are text stay text: grep 'a:b', echo a:b, grep -E '\bword\b', printf '\t%s\n', rg -g '*.py' '\Aclass' ., date +%H:%M, %-formats and a quoted ..\ inside a pattern (rg 'foo|..\bar' .) pass unchanged. POSIX behaviour is unchanged: each of those spellings is an ordinary relative name there.

Kept out of this cut on purpose, because each needs per-tool argument tables to stay clear of pattern text: a single-backslash root as a read argument (cat \Windows\win.ini), a bare drive-relative argument (cat C:Windows\win.ini), a /-rooted option or PowerShell provider value (-LiteralPath:/Windows/win.ini, FileSystem::/Windows/win.ini, curl -o/Windows/win.ini), cmd ^ decoding, short-option clusters, and a cwd command inside a quoted script string (powershell -Command "cd \Windows"). A quoted argument stays text, so rg 'then cd \Windows' docs passes.

Surface area

  • Sandbox — the host-bash guard in backend/packages/harness/deerflow/sandbox/tools.py
  • Frontend UI
  • Backend API
  • Agents / LangGraph
  • Skills
  • Dependencies
  • Default behavior change — only commands a Windows host previously let through are rejected; no opt-in or default changes
  • Docs / tests / CI only

Bug fix verification

  • Test path that reproduces the bug: backend/tests/test_sandbox_tools_windows_drive_paths.py
  • Did it go red on main and green on this branch? yes — on main at ce0fa4c1 the file is 35 failed / 20 passed, every failure DID NOT RAISE PermissionError; on this branch 55 passed.

Validation

Ran in backend/:

  • pytest tests/test_sandbox_tools_windows_drive_paths.py tests/test_sandbox_tools_security.py -q — 371 passed
  • ruff check and ruff format --check on both changed files — clean

The full make test shard matrix needs the CI Postgres/Redis services, so CI covers it.

AI assistance

Tool(s) used: DeepSeek Harness (agentic coding harness)

How you used it: Scanned the existing guard and its POSIX path rules, wrote the Windows scan and its regression tests, and ran the red/green and lint checks above.

  • I've read and understand every line of this change and take responsibility for it — it's not unreviewed AI output.

@github-actions github-actions Bot added area:sandbox Sandboxed execution and docker/ risk:high High risk: backend API, agents, sandbox, auth, deps, CI size/M PR changes 100-300 lines labels Oct 19, 2026
@WillemJiang
WillemJiang requested a balanced review from Copilot October 19, 2026 01:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Two drive-path bypasses remain, and CI skips all new rejection tests.

3 open issues
What changed in this PR

Strengthens DeerFlow’s opt-in host-bash guard against Windows drive-letter paths while preserving POSIX behavior.

Changes:

  • Adds Windows-specific checks for drive paths, bare drive references, and working-directory changes.
  • Adds Windows rejection tests and POSIX compatibility tests.
File Description
backend/​tests/​test_sandbox_tools_windows_drive_paths.py Adds drive-path rejection and compatibility tests.
backend/​packages/​harness/​deerflow/​sandbox/​tools.py Adds Windows drive-path validation and documents its behavior.

Review effort: Balanced

Comment on lines +1570 to +1571
if _is_in_spans(match.start(), url_spans):
continue
Comment on lines +1582 to +1583
if not _WINDOWS_DRIVE_SPEC_PATTERN.fullmatch(token):
continue
"outputs_path": "/tmp/deer-flow/threads/t1/user-data/outputs",
}

_windows_only = pytest.mark.skipif(os.name != "nt", reason="drive-letter spellings are absolute host paths only on a Windows host")
@BlueX888

Copy link
Copy Markdown
Contributor Author

All three findings are addressed in 068c21b6 (pushed on top of the reviewed head):

  1. Doubled-separator drive URLs. The drive scans now use a strict containment test (_is_embedded_in_spans): a drive match inherits the non-file-URL exemption only when it is strictly inside a larger URL, never when it starts the URL span. type C://Windows/win.ini (and quoted/triple-separator/lowercase variants) is rejected, while a drive-like string embedded in a real URL (e.g. https://example.com/?next=C:/Windows/win.ini) remains allowed. A doubled-separator regression case is included.

  2. Drive-relative arguments. The token scan changed from an exact X: match to an anchored X:... match, so type C:Windows/win.ini and type C:Windows\win.ini are rejected in both scans (including quoted, tab/newline-separated and line-start forms, and forms where shlex strips unquoted backslashes). Anchoring preserves flag-with-colon tokens (-F:, -d:-style) and sed scripts (s/foo:/bar:/); cd/pushd drive-relative targets were already covered by the prefix match.

  3. Tests never running on Linux CI. The skipif(os.name != "nt") markers are gone; windows_host / posix_host fixtures monkeypatch sandbox_tools._is_windows(), so all 21 tests now execute on every host (previously 13 of 15 were skipped on the Ubuntu jobs) while the POSIX-specific checks stay separate. -rs confirms 21 passed, 0 skipped.

Evidence: the new cases fail at the base (16 failed, 5 passed at current main), pass at the new head (21 passed, 0 skipped); test_sandbox_tools_security.py 227 passed; the local-bash trio's three C:/Users/admin vs Admin case-mismatch failures are identical at base (pre-existing, unrelated); ruff clean.

One residual noted by an independent adversarial pass (pre-existing at the reviewed head, not introduced here): the token-occurrence loop's URL exemption can be satisfied by a decoy non-file URL containing the same text (curl 'https://x/?p=C:Windows/win.ini'; type C\:Windows\win.ini). Dropping that exemption for bare X: tokens (they are never URL text in the shlex stream) is a cheap hardening — say the word and I'll fold it in; otherwise it stays documented as a known corner.

@BlueX888

Copy link
Copy Markdown
Contributor Author

Folded the decoy-URL hardening in as well (commit 3fff21e0): a bare X:-prefixed drive word is now rejected unconditionally after the allowed-path filter, so a decoy non-file URL can no longer supply the only verbatim occurrence that exempted the real drive-relative path. Two regression cases cover both decoy spellings (...?p=C:Windows/win.ini and ...?p=C:Windowswin.ini, each with type C\:Windows\win.ini). The PR body now reflects the 22-test suite. Hosted workflows have not run on this head yet — they still need maintainer approval for fork PRs.

@willem-bd willem-bd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh adversarial review at head 3fff21e. I empirically drove validate_local_bash_command_paths at head (validator invoked directly with _is_windows() patched, matching the PR's own fixtures). Every variant the PR claims to reject is rejected: C:/... and C:\... (quoted and unquoted), the bare roots C:/, C:\, the bare drive C:, cd/pushd targets including cd C: and the cd-then-relative-read case, drive-relative C:Windows/... and C:Windows\... (including the decoy-URL form), the C:// doubling, lowercase drives, 8.3 short names, and Win32-namespaced \\?\C:\...; the -F:/s/// carve-outs, URL-embedded drive text, and the POSIX spellings stay allowed, and the new test file passes 22/22 at head. The #6930 non-ASCII prose exemption does not swallow drive paths: the token scan fires on any word starting with X:, so benign text like python -c "print(f'C:/端口{port}')" stays allowed while cat C:/文档/x is rejected. Three same-class Windows host-path spellings remain uncovered — backslash UNC, drive-root backslash form, and cmd/PowerShell env-var expansion — detailed inline.

# at all. The ``*`` tail lets a bare drive root (``C:/``, ``C:\``) match too.
# On POSIX the same spelling is a relative path under the workspace, so the
# drive scans stay Windows-only (validate_local_bash_command_paths).
_WINDOWS_DRIVE_PATH_PATTERN = re.compile(r"(?<!\w)[A-Za-z]:[\\/][^\s\"'`;&|<>()]*")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified at head that this scan (plus the token scan) catches C:/, C:\, bare roots, drive-relative forms, lowercase drives, 8.3 names and C://..., and that //server/share/x is rejected by the pre-existing /-prefixed scan. One same-class spelling still slips every scan: the backslash UNC form. cat '\\server\share\secret.txt', cd '\\server\share' and pushd '\\server\share' are all allowed on a Windows host — there is no / for the absolute-path scan, no X: prefix for the drive scans, and the dotdot token check does not apply — while the equivalent forward-slash spelling //server/share/x is rejected, so coverage is asymmetric across the two UNC spellings. A UNC path is an absolute host path exactly like the drive forms this PR adds (no colon needed), so it merits the same Windows-only treatment (e.g. reject a quoted word that starts with \\, mirroring the X:-prefix argument that no relative workspace path begins with a path-root marker), plus a regression case in test_sandbox_tools_windows_drive_paths.py.

# added by _apply_cwd_prefix). The ``*`` tail catches the bare drive root
# itself, and a standalone ``C:`` word names the drive's current directory
# there (``dir C:`` lists it) — no Windows filename may contain a colon,
# so neither spelling can be a relative workspace path. A match that

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same-family gap: a word beginning with a backslash is also never a relative workspace path on a Windows host, but it has no colon so the new X:-anchored scans cannot see it. Verified at head: cat '\Windows\win.ini' and cd '\Windows' && cat win.ini both pass the guard, yet PowerShell resolves a leading-\ path against the root of the current drive — with the workspace on C: that read lands on C:\Windows\win.ini, i.e. the same host read as the cd-override example in the PR description. The comment's own argument ("no Windows filename may contain a colon, so such a word is never a relative workspace path") applies equally to a leading-\ word on PowerShell/cmd hosts; a Windows-only check for a token starting with \ (or folding it into the drive scans) would close it.

# the same text also occurs inside a URL would let a decoy URL excuse
# the real drive-relative path:
# curl 'https://x/?p=C:Windowswin.ini'; type C\:Windows\win.ini
for token in tokens:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR's threat model counts PowerShell and cmd.exe as resolving shells (it uses their C: semantics to justify the drive scans), and on those shells env-var spellings expand at execution time where every scan here is blind. Verified allowed at head: cat %WINDIR%\win.ini, cat %SYSTEMDRIVE%\Windows\win.ini (cmd.exe) and cat $env:SystemRoot\win.ini (PowerShell) — none contain / or an X: prefix, and the $NAME/ scan only fires on the bash spelling ($SYSTEMDRIVE/Windows/win.ini is already rejected for exactly the "runtime value is unknown" reason in the docstring). If cmd/PowerShell are in scope for the drive scans, these are the same class: either reject path-like %NAME% / $env:NAME words on a Windows host or state explicitly in the docstring that shell-native expansions remain out of scope, so the next reviewer does not re-derive the boundary.

@github-actions github-actions Bot added size/XL PR changes 700+ lines and removed size/M PR changes 100-300 lines labels Oct 19, 2026
@BlueX888

Copy link
Copy Markdown
Contributor Author

Thanks for the fresh adversarial pass — all three same-class spellings you found are now closed in 8b1a2aca (pushed on top of 3fff21e0):

  • Backslash UNC (\\server\share\...) as read arguments and cd/pushd targets, quoted and unquoted.
  • Drive-root-relative backslash (\Windows\win.ini, cd '\Windows' && cat win.ini), including case variants.
  • cmd.exe / PowerShell env expansion (%WINDIR%\win.ini, %SYSTEMDRIVE%\Windows\win.ini, $env:SystemRoot\win.ini, ${env:...}).

While hardening those I also closed the same-class variants the follow-up review surfaced: concatenated cwd targets (cd %HOMEDRIVE%%HOMEPATH% && type .ssh\id_rsa), option values classified by option name (path options such as -Path:/-OutFile:/--output=/--file= and assignments stay strict; pattern-valued options such as --regexp/-e/--include/--exclude/-F may carry escape text), attached short-option values (curl -o\Users\Public\evil.exe, sort -o\recovery), short-component rooted paths (\recovery\x, \temp\x), and assertion-shaped root names (\Boot\BCD, \EFI\BOOT, \Bootmgr, \BootNXT, \temp\BCD, \recovery\BCD).

The escape/format allow-list you called out stays green: grep '\d', printf '\t%s\n', python -c "print('\n')", awk '{print "\t"}', cut -d'\t', tr '\n' ' ', \Afoo, \Bfoo, \Qfoo\E, printf '\efoo', rsync --exclude=\*.tar.gz, %s:~%s templates, grep -F '$HOME\', --regexp=\w\+, awk -v re=\bfoo, X=\w\+; the POSIX pins and all prior drive/UNC/env/option rejections hold.

At 8b1a2aca: the drive-path suite is 137 passed, the test file is red on current main with the guard absent (93 failed / 44 passed), and test_sandbox_tools_security.py is 227 passed, 4 skipped; ruff clean. Hosted CI will run once the workflows are approved for the new head.

One follow-up is in progress from the latest internal pass: two pattern-argument edges — an assertion followed by another regex escape (grep '\Afoo\w+') and assertion-shaped names passed as file arguments of pattern commands (grep -a . '\Bootmgr', sed -n p '\Bootmgr') — are being tightened so the allow-list and the rooted-name rejection don't interfere. I'll push that as a small follow-up commit rather than hold this update.

@BlueX888
BlueX888 force-pushed the fix/host-bash-windows-drive-guard branch from 8b1a2ac to f12eb59 Compare October 19, 2026 10:30
@BlueX888

Copy link
Copy Markdown
Contributor Author

Pushed the follow-up series for the remaining same-family gaps, rebased onto current main (fee0cd0c). New head: f12eb59d.

  • 19c6a184 — root-name assignments are rooted again (X=\Boot, X=\EFI, X=\Bootmgr, X=\BootNXT, chained set/dir forms); combined short options are scanned letter by letter so a value-taking letter consumes the rest of the cluster (grep -iefoo, sed -ne's/x/y/'); wrapper option values are consumed (xargs -n/-I/-L/-s/-P, env -u/-C, nice -n, time -f/-o, stdbuf -i/-o/-e).
  • dd34d25b — the known-root assignment check now covers the \e escape path and strips trailing extensions (X=\efi, X=\Boot.ini, X=\Bootmgr.exe); command-name resolution is re-run after find -exec, so -exec env grep, -exec nice -n 5 grep and -exec xargs -n 1 grep reach the pattern scan.
  • 6ae26b1c — dot-bearing known roots are matched by stripping trailing extensions one at a time, so X=\$Recycle.Bin, set X=\$Recycle.Bin & call dir %^X% and X=\$Recycle.Bin.txt are rejected while \Bfoo/\Afoo remain regex assignments.

Every case has a regression test in backend/tests/test_sandbox_tools_windows_drive_paths.py. At the new head: 218 passed (drive-path), 300 passed / 4 skipped (security suite), ruff clean. Note the workflows on the force-pushed head sit in action_required until a maintainer approves the run, so the checks may stay pending until then.

@willem-bd willem-bd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head f12eb59d. The coverage added since 3fff21e0 is real: the new suite passes 218/218 with no skips, and I re-verified that the previously reported backslash-UNC, \Windows and %WINDIR%\/$env:... forms are now rejected. Five same-class gaps remain at this head, each reproduced by driving validate_local_bash_command_paths directly with _is_windows() patched: (1) -Path:/Windows/win.ini (and FileSystem::/Windows/win.ini) is allowed because a / preceded by : is skipped by _ABSOLUTE_PATH_PATTERN and the new option scan only accepts a backslash root; (2) the new standalone env-var cwd check is only reached for the literal lowercase cd/pushd, so CD %WINDIR%, chdir %WINDIR% and Set-Location $env:WINDIR change into a host directory; (3) unquoted ..\..\ traversal (shlex strips the backslashes) still escapes the workspace; (4) cmd.exe ^ escaping (C^:/Windows) hides drive paths from every scan; (5) the X: token scan is context-free and rejects legitimate pattern arguments such as grep 'a:b' file. Findings 1-3 are host/workspace escapes; 4-5 are lower severity. The test suite is strong but has no case for any of these. Nothing here duplicates the existing review threads.

_WINDOWS_OPTION_ROOTED_PATH_PATTERN = re.compile(
r"(?:^|(?<=[\s;&|()<>`\"']))"
r"(?:(?P<option>-{1,2}[A-Za-z0-9][A-Za-z0-9_-]*)[:=]|(?P<drive>[A-Za-z]):|(?P<assign>[A-Za-z_][A-Za-z0-9_]*)=)"
r"['\"]?(?P<path>\\\\?[^\s\"'`;&|<>()]+)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option values rooted with / are still invisible to the guard. This path group requires a leading backslash, and the pre-existing _ABSOLUTE_PATH_PATTERN deliberately skips a / preceded by : ((?<![\w:])), so a PowerShell parameter value is never scanned. Verified at this head (validator driven directly with _is_windows() patched): Get-Content -Path:/Windows/win.ini is allowed, while PowerShell resolves the leading / against the root of the current drive (Path.GetFullPath('/Windows') -> C:\Windows), i.e. the same host read as the rejected -Path:\Windows\win.ini. Get-Content FileSystem::/Windows/win.ini is allowed for the same reason. Quoting flips the result (-Path:'/Windows/win.ini' is rejected because the / is then preceded by a quote), which is exactly why the unquoted form is easy to miss. Please let this scan accept a / root as well, running it through _is_allowed_local_bash_absolute_path so /mnt/user-data/... stays legal, and add -Path: plus provider-qualified regression cases.

# even without a trailing separator. Search rather than full-match: a
# concatenation such as ``%HOMEDRIVE%%HOMEPATH%`` is just as unsafe and
# would otherwise slip past a single-variable pattern.
if _is_windows() and _WINDOWS_ENV_VAR_TARGET_PATTERN.search(target):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These Windows cwd checks only run when the token's command name is exactly cd or pushd (_LOCAL_BASH_CWD_COMMANDS, exact str membership in _validate_local_bash_shell_tokens). The shells this PR treats as in scope do not work that way: cmd.exe command names are case-insensitive and chdir is a synonym, and PowerShell provides Set-Location/Push-Location (aliased as cd/sl). Verified allowed at this head: CD %WINDIR% && type win.ini, Cd %WINDIR%, chdir %WINDIR% && type win.ini, PUSHD %USERPROFILE%, Set-Location $env:WINDIR && type win.ini, and Push-Location $env:WINDIR — each changes into a host directory and then reads a relative file there, which is the exact scenario the standalone-variable target check was added for. (The drive/backslash spellings are still caught by the path scans, so only the variable form leaks here.) Please match cwd commands case-insensitively and include chdir/Set-Location/Push-Location on the Windows path, or apply the standalone env-var/backslash check to any command's arguments.

# hide behind the shell lexer. A URL may legitimately contain the same
# text, so keep the URL span exemption.
raw_windows_paths: list[tuple[str, int]] = []
for match in _WINDOWS_ROOTED_BACKSLASH_PATH_PATTERN.finditer(command):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The raw scans only consider words that start with a backslash, so the backslash traversal spelling still slips when unquoted. Verified allowed at this head: type ..\..\secret.txt, Get-Content ..\..\secret.txt, cd ..\..\ && type secret.txt, and type ..\..\..\..\Users\Public\secret.txt, while the quoted type '..\..\secret.txt' is rejected. The cause is the same lexer problem this raw scan exists to solve: shlex strips unquoted backslashes (..\..\secret.txt becomes ....secret.txt), so the pre-existing _DOTDOT_PATH_SEGMENT_PATTERN — which already includes \\ — never sees the segments. This is pre-existing rather than introduced by the PR, but it is the same Windows workspace-escape class and the raw-command walk here is the natural place to close it: scan raw words for (?:^|[\s;&|()<>"'])..[\/](or run the dotdot check against the raw word) and add the unquotedcd`/read cases.

if command_name is not None:
raise PermissionError(f"Unsafe working directory change in command: {command_name} {raw_path}. Use paths under {VIRTUAL_PATH_PREFIX}")
unsafe_paths.append(raw_path)
for match in _WINDOWS_DRIVE_PATH_PATTERN.finditer(command):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmd.exe removes ^ before parsing, but every scan here inspects the literal text, so the caret hides a drive path: type C^:/Windows/win.ini, type C^:\Windows\win.ini and cd C^:/Windows && type win.ini are all allowed at this head (verified), while cmd executes them as C:/Windows/win.ini / C:\Windows\win.ini. If cmd.exe is in the threat model (the PR relies on its C: semantics and %NAME% expansion to justify the drive scans), handle ^ in the raw-command scan; otherwise please state in the docstring that cmd preprocessing (^, and !VAR! under delayed expansion) remains out of scope, so this boundary is explicit rather than an oversight.

# the real drive-relative path:
# curl 'https://x/?p=C:Windowswin.ini'; type C\:Windows\win.ini
for token in tokens:
if not _WINDOWS_DRIVE_SPEC_PATTERN.match(token):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This token scan rejects any word whose second character is :, regardless of argument context, so legitimate non-path arguments now fail on Windows. Verified at this head: echo a:b, grep 'a:b' file.txt, grep -e 'a:b' file, and Get-ChildItem -Filter 'a:*' all raise Unsafe absolute paths in command: a:b. The comment's premise — no Windows filename may contain a colon — only establishes that such a word cannot be a relative path; it does not make every such word a path. The PR already classifies argument context via _windows_argument_context for the backslash scan, so reusing it here would keep type a:b rejected while allowing the actual pattern argument of a regex tool (and an exemption for PowerShell -Filter-style pattern values if those matter). Please add allow-list regressions for the non-path forms.

@willem-bd willem-bd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at new head f12eb59 (PR was force-pushed; now +1608/-1 over two files). All three findings from the previous review (5566461327) are FIXED, verified empirically at head by driving validate_local_bash_command_paths with the host gate patched, using the same probe shapes as before:

  1. Backslash UNC (cat '\\server\share\secret.txt' quoted and unquoted, cd/pushd '\\server\share') — now rejected via the new raw-command backslash scan, which also defeats shlex stripping unquoted backslashes (cat \\server\share\secret.txt unquoted is caught in the raw text).
  2. Drive-root form (cat '\Windows\win.ini', cd '\Windows' && cat win.ini) — rejected for both quoted and unquoted spellings, plus a token fallback for non-verbatim shells escapes.
  3. Shell-native expansions (cat %WINDIR%\win.ini, cat %SYSTEMDRIVE%\Windows\win.ini, cat $env:SystemRoot\win.ini, cd %WINDIR%) — rejected, including cmd substring expansion (%WINDIR:~0,3%), concatenated targets (cd %HOMEDRIVE%%HOMEPATH%), and bash-spelling $HOME\.ssh\id_rsa.

The expanded test file passes 218/218 at head and the pre-existing test_sandbox_tools_security.py passes 304/304 (no POSIX regression). The new escape-vs-root heuristics hold up against the family's known evasion shapes: \\?\C:\Windows\win.ini and \\.\PhysicalDrive0 stay rejected, the decoy-URL drive-relative case stays rejected (curl 'https://x/?p=C:Windowswin.ini'; type C\:Windows\win.ini), PowerShell subexpression $($env:SystemRoot)\win.ini is caught, and the -F:/s/// carve-outs stay allowed.

One new same-class gap found, detailed inline: the raw scan's word-boundary class omits , and =, which cmd.exe treats as standard argument delimiters, so a rooted backslash word after one still reaches the host while the identical word without the delimiter is rejected.

# token stream. Each match still goes through
# _is_windows_rooted_backslash_word() so quote-initial escape arguments are
# not mistaken for roots.
_WINDOWS_ROOTED_BACKSLASH_PATH_PATTERN = re.compile(r"(?:^|(?<=[\s;&|()<>`\"']))\\\\?[^\s\"'`;&|<>()]+")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified at head that the three spellings from the previous review (UNC, \Windows-rooted, %VAR%/$env:NAME) are now rejected. One same-class gap remains in this lookbehind class: it omits , and = — both standard cmd.exe argument delimiters — so a rooted backslash word following one is invisible to every scan. Confirmed allowed at head while the same word without the delimiter is rejected:

  • type ,\Windows\win.ini / type =\Windows\win.ini (host read of C:\Windows\win.ini on cmd.exe)
  • cd ,\Windows && type win.ini / pushd =\Windows && type win.ini (the cd-override shape)
  • type ,\\server\share\secret.txt (UNC) and type ,\Boot\BCD (the exact word this PR's own tests reject without the comma)

PowerShell reaches the same word too (unary comma builds a one-element array argument). Meanwhile type ,C:\Windows\win.ini is rejected, because the drive scan has no word-boundary lookbehind — so coverage is asymmetric between the two root spellings, mirroring the forward/backslash UNC asymmetry flagged last round.

Adding , and = to this class (and to the identical lookbehind in _WINDOWS_OPTION_ROOTED_PATH_PATTERN and _WINDOWS_ATTACHED_SHORT_OPTION_PATH_PATTERN), with regression cases in test_sandbox_tools_windows_drive_paths.py, would close it. !WINDIR! (delayed expansion) and %~dp0 (batch params) are analogous but non-default execution contexts — lower priority.

@BlueX888

Copy link
Copy Markdown
Contributor Author

Addressed all six findings in c394a011 (three commits on top of the reviewed head; no rebase).

  1. /-rooted option and provider values. -Path:/Windows/win.ini and FileSystem::/Windows/win.ini are re-checked as absolute paths through the same allowlist, so /mnt/user-data/... stays legal. The same scan now covers module-qualified providers (Microsoft.PowerShell.Core\FileSystem::/Windows/win.ini), the backslash provider form (FileSystem::\Windows\win.ini), and the attached short-option write spelling (curl -o/Windows/win.ini, including clusters such as -sSo/Windows/win.ini).
  2. cwd spellings. Command names are matched case-insensitively and chdir, Set-Location and Push-Location (plus sl) are recognized, so CD %WINDIR%, chdir %WINDIR%, PUSHD %USERPROFILE% and Set-Location $env:WINDIR report the working-directory error.
  3. Unquoted backslash traversal. The raw scan rejects a ..\ segment at a word boundary or after a separator (type ..\..\secret.txt, Get-Content ..\..\secret.txt, cd ..\..\, type foo\..\..\secret.txt), while regex text such as grep -E 'a|..\d' stays allowed.
  4. cmd.exe ^. Decoding is context-aware: C^:/Windows, C:^/Windows, C^:\Windows and ^\Windows are decoded, while grep '^C:', grep '^\Windows' and printed text keep the caret. !VAR! (delayed expansion) is documented as out of scope.
  5. Colon words in pattern context. The drive-relative scan is position-aware, so a duplicate decoy (echo a:b && type a:b) can no longer lend its text context to the real path. Pattern arguments, PowerShell -Filter/-Include/-Exclude/-Pattern values and printed arguments are allowed; type a:b, a later file positional and -Path a:b stay rejected.
  6. ,/= argument delimiters. Added to all three lookbehinds, with regressions for type ,\Windows\win.ini, type =\Windows\win.ini, cd ,\Windows, pushd =\Windows, type ,\server\share\secret.txt, type ,\Boot\BCD and the ,a:b drive-relative form. Concatenated escape text after a delimiter (printf '%s,%d\n') is recognized as text, but type \d\n is treated as a rooted path again.

Tests: the Windows guard suite is 300 tests (was 218), all passing; the pre-existing test_sandbox_tools_security.py suite passes 300/304 (4 platform-gated skips). Implementer and independent reviewer green receipts are bound to c394a011.

@BlueX888

Copy link
Copy Markdown
Contributor Author

Verified at c394a011: this is closed. _is_local_bash_cwd_command matches command names case-insensitively on Windows and recognizes chdir, Set-Location, Push-Location and sl alongside cd/pushd (commit 5be51d76), so CD %WINDIR% && type win.ini, Cd %WINDIR%, chdir %WINDIR% && type win.ini, PUSHD %USERPROFILE%, Set-Location $env:WINDIR && type win.ini and Push-Location $env:WINDIR all raise Unsafe working directory change, while chdir /mnt/user-data/workspace and Set-Location /mnt/user-data/workspace stay allowed. Regression: test_validate_local_bash_command_paths_blocks_case_insensitive_and_alias_cwd_targets.

Re-ran on c394a011: test_sandbox_tools_windows_drive_paths.py 300 passed; test_sandbox_tools_security.py 300 passed, 4 skipped.

@willem-bd willem-bd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 45c06c2. The earlier Windows-path follow-ups are covered by the current tests. Two remaining P2 findings are detailed inline: a residual PowerShell cwd-target gap and a new ripgrep compatibility regression.

Validation: the Windows-path and existing host-bash security suites passed all 604 tests. Eight additional probes expose the two findings (four cases each). The four ripgrep commands execute successfully against temporary files and pass with the base guard function substituted, but the head rejects them. Ruff lint/format, diff and guidance checks passed; current CI checks were green. Windows validator checks used the patched host gate, matching the PR's own fixtures; no native Windows or PowerShell execution was performed. PowerShell parameter semantics were checked against Microsoft's documentation. No production source files were changed during this review.

Comment on lines +2506 to +2507
if _is_windows() and _WINDOWS_ENV_VAR_TARGET_PATTERN.search(target):
raise PermissionError(f"Unsafe working directory change in command: {command_name} {target}. Use paths under {VIRTUAL_PATH_PREFIX}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Consume PowerShell option values before checking the cwd target

These commands still pass validate_local_bash_command_paths with the Windows host gate enabled:

  • Set-Location -ErrorAction Stop $env:WINDIR; Get-Content win.ini
  • Set-Location -ErrorAction Stop -Path $env:WINDIR; Get-Content win.ini
  • Push-Location -StackName saved $env:WINDIR; Get-Content win.ini
  • cd -ErrorAction SilentlyContinue $env:WINDIR; Get-Content win.ini

The existing _next_cd_target skips an option token but does not consume its value, so this check receives Stop, saved, or SilentlyContinue instead of the actual directory. The raw scans also miss a standalone $env:WINDIR because it has no trailing separator. Consequently, recognizing the PowerShell cwd aliases does not close the environment-variable cwd case when a value-taking parameter precedes the target.

PowerShell binds those strings to their named parameters before binding the directory argument; see Microsoft's Set-Location and Push-Location syntax. On a PowerShell host shell, that leaves the same cwd override and subsequent relative host-file read the PR intends to reject. This is a residual coverage gap in the best-effort guard, not a claim that host bash provides filesystem isolation.

Please consume PowerShell value-taking parameters, including common parameters, before validating the actual Path argument and add regression cases. The validator admission is empirically verified; native Windows/PowerShell execution was not tested.

Comment on lines +2246 to +2247
if name in _WINDOWS_GREP_LIKE_COMMANDS:
return "grep"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve ripgrep-specific option semantics

Mapping rg to grep's option table loses value-taking ripgrep options such as --glob/-g and --type/-t. All of these valid workspace searches are rejected on the Windows validator path:

  • rg --glob '*.py' '\Aclass' .
  • rg -g '*.py' '\Aclass' .
  • rg --type py '\Aclass' .
  • rg -t py '\Aclass' .

The parser treats the option as a flag, mistakes its value (*.py or py) for the search pattern, and then classifies the actual regex as a path. The rooted-backslash check consequently reports the regex as an unsafe host path.

I executed each command successfully against a temporary example.py file before invoking the validator. All four pass when substituting the base revision's guard function and fail at this head, confirming an introduced compatibility regression. Please retain tool-specific value-taking option semantics and add allow-list regressions for ripgrep patterns following glob/type options.

@BlueX888
BlueX888 force-pushed the fix/host-bash-windows-drive-guard branch from 45c06c2 to c59d875 Compare October 22, 2026 10:08
@BlueX888

Copy link
Copy Markdown
Contributor Author

Both P2s from this review are addressed at c59d8750, now the head of fix/host-bash-windows-drive-guard (rebased onto current main at 29ba806fc).

(a) PowerShell cwd targets. _next_cd_target now consumes PowerShell named/common parameters on a Windows host, so a value-taking parameter's value is never validated as the directory, and -Path/-LiteralPath (including the -Path: attached form) name the target explicitly. All four reported commands now report the working-directory error, while Set-Location -Path /mnt/user-data/workspace and Push-Location -StackName saved /mnt/user-data/workspace stay allowed.

(b) ripgrep option semantics. rg no longer inherits grep's option table: -g/--glob/--iglob and -t/--type/-T/--type-not take glob/type values that are not the search pattern, and -E is --encoding. The four reported searches pass, while a host path in the same command is still rejected (rg -g '*.py' '\Aclass' C:/Windows, rg -t py '\Aclass' \Windows\win.ini).

The delta against the reviewed head 45c06c2c is additive and touches only backend/packages/harness/deerflow/sandbox/tools.py and backend/tests/test_sandbox_tools_windows_drive_paths.py; no existing rejection was removed or relaxed (the pre-existing host-bash security suite is unchanged in result).

Tests actually run (Windows host, backend uv project; the Windows branch of the validator is selected by the suites' _is_windows fixture, no native PowerShell shell was invoked):

  • uv run --frozen --project backend python -m pytest backend/tests/test_sandbox_tools_windows_drive_paths.py -q — 337 passed, clean tree at c59d8750
  • uv run --frozen --project backend python -m pytest backend/tests/test_sandbox_tools_security.py -q — 300 passed, 4 skipped
  • Reviewer-authored regression set (independent of the branch's own tests, 16 cases covering both findings plus allow-list and no-relaxation controls): at current main 29ba806fc — 10 failed / 6 passed, every failure DID NOT RAISE PermissionError; at the reviewed head 45c06c2c — 8 failed / 8 passed, where the four PowerShell commands are still allowed and the four ripgrep searches are rejected with Unsafe absolute paths in command: \Aclass; at c59d8750 — 16 passed.

@WillemJiang WillemJiang added this to the 2.2.0 milestone Oct 22, 2026
@WillemJiang

Copy link
Copy Markdown
Collaborator

@BlueX888, the PR is so big for the feature we want. Could you reduce its size?

@WillemJiang WillemJiang removed this from the 2.2.0 milestone Oct 23, 2026
@BlueX888

Copy link
Copy Markdown
Contributor Author

Thanks — yes, I can. Two things are tangled together here, so let me separate them.

The 18 commits are one topic (fix(sandbox), Windows host-path rejection in the host-bash guard), not 18 features. They are iterations: while closing each newly found bypass the series rewrote ~445 of its own added lines. I can squash that to a single commit without losing any coverage, so the diff stays +2407/−3 in the same 2 files.

The line count is the real cost: ~1200 lines in tools.py, most of it a shell-grammar parser (grep/rg option tables, cmd ^ decoding, PowerShell named parameters, wrapper following), plus a 1204-line test file (337 cases). Those exist so the guard does not reject legitimate commands such as grep -e 'a:b' or rg --glob '*.py', so removing them brings back false positives.

There is one clean cut I can make on request: land the core first — ad2509c8..e089204a, +292/−1, 22 tests, green on its own — and the parser-fidelity tail as a follow-up.

Which do you prefer: squash only, or core-first split?

@WillemJiang

Copy link
Copy Markdown
Collaborator

@BlueX888 please make a clean cut to do the quick fix.

@BlueX888
BlueX888 force-pushed the fix/host-bash-windows-drive-guard branch from c59d875 to e49e553 Compare October 23, 2026 15:13
@BlueX888

Copy link
Copy Markdown
Contributor Author

Reduced to +1149 / −3 in the same two files (was +2407 / −3) and rebased onto
current main, force-pushed to this branch.

What went. The escape-vs-root oracle (14 regexes plus a 107-line decision
function), the cmd/PowerShell option-arity and wrapper tables, a re-implemented
raw lexer, and the invented drive-root name list. None of it came from a review
finding: it existed because the patch scanned raw text with lookbehind-anchored
regexes and then had to win back the false positives that produced. The guard now
judges quote-aware raw shell words, so a word's argument position answers the
escape question and most of that machinery disappears. The test file drops from
337 cases to 200 — one representative per behaviour class instead of a
permutation matrix.

What stayed, because you asked for it. Every case from your four review
rounds still rejects, each red on main and green here: drive letters in all
spellings and as cd/pushd/Set-Location targets; backslash UNC and
\Windows\win.ini; %WINDIR%, $env:SystemRoot, $HOME; -Path:/Windows and
FileSystem::/Windows (still through the allowlist, so /mnt/user-data/...
stays legal); ,/= delimiters; unquoted ..\..\; cmd ^; \\?\… and
\\.\…; drive paths inside python -c / perl -e / node -e / bash -c /
powershell -Command strings; C\:Windows\win.ini and the decoy
curl 'https://x/?p=C:Windowswin.ini'; type C\:Windows\win.ini.

Option arity is now per tool. The single shared "takes a value" set is gone;
each tool's arity comes from its own --help. That matters: -l and -o are
flags for grep and ripgrep, so grep -l foo \Windows\win.ini and
rg -o foo \Windows\win.ini reject — under one shared set they silently became
pattern text. sed/gawk -l still take a value; ripgrep's -g/-t/-T
filters are ripgrep-only.

What still doesn't. The false positives you reported are gone
(grep 'a:b', echo a:b, -Filter 'a:*', rg -g '*.py' '\Aclass' .,
grep -E '\bword\b'), while a host path in the same command still rejects. Three
deltas remain and I'd rather name them than hide them: grep -E '\C',
sed -E 's/(a)\1/b/' and echo '\1/b/' stay allowed, because rejecting them
needs exactly the per-tool escape tables this PR drops. sed 's/C:/X/' rejects,
matching the current head. !NAME! delayed expansion, wrapper flags and
short-option clusters are still not modelled.

Measured locally (Windows, backend uv project; the Windows branch is selected
by the fixtures, no native PowerShell was run):

  • pytest tests/test_sandbox_tools_windows_drive_paths.py — 200 passed at this
    head; on current main the same file is 129 failed / 71 passed, every failure
    DID NOT RAISE PermissionError.
  • pytest tests/test_sandbox_tools_security.py tests/test_skill_request_scoped_secrets.py
    — 450 passed, 5 skipped.
  • Sandbox guard sweep (5 files): 619 passed, 7 skipped, and the same 15
    pre-existing failures main has on this host.
  • ruff check . and ruff format --check . clean.

I also ran this revision against the current head on a 1440-case differential
(15 corpora: your review cases, the repo's own guard tests, curated and generated
variants). It rejects everything the head rejects that is a real host path —
\Windows\win.ini, \Users\Public\secret.txt, \Windows\System32\config\SAM,
\bootmgr, \pagefile.sys — and it closes one gap the head still has:
type \$Recycle.Bin is allowed by the head and rejected here. Every short option
in grep's and ripgrep's own help was swept to confirm no arity error can hide a
path in a pattern slot.

Full make test needs the CI Postgres/Redis services, so that one is CI's. If
you'd rather review the reduction as a fresh PR instead of a force-push here,
say so and I'll open one and close this.

@BlueX888
BlueX888 force-pushed the fix/host-bash-windows-drive-guard branch 2 times, most recently from cf0f97e to 31a946e Compare October 25, 2026 20:45
@github-actions github-actions Bot added size/M PR changes 100-300 lines and removed size/XL PR changes 700+ lines labels Oct 25, 2026
@BlueX888

Copy link
Copy Markdown
Contributor Author

Cut to the quick fix: +296 / −3, one commit, two files (sandbox/tools.py plus the new test file), down from +1149 / −3 on the previous head.

The Windows branch of the guard now rejects drive paths (C:/…, C:\…, C://…, C:/, also inside python -c / powershell -Command strings and after attached options such as curl -oC:/…), Win32 device and extended-length paths, UNC paths, %WINDIR%\… / $env:…\… / $HOME\… expansions, ..\… arguments, and a working-directory target that is a drive, a backslash root or a variable — including behind a PowerShell parameter (Set-Location -ErrorAction Stop $env:WINDIR). Text arguments still pass (grep 'a:b', printf '\t%s\n', rg 'foo|..\bar' .). POSIX behaviour is unchanged.

Left out of this cut, because each needs per-tool argument tables to keep pattern text allowed: a single-backslash read root (cat \Windows\win.ini), a bare drive-relative argument (cat C:Windows\win.ini), /-rooted option or provider values (-LiteralPath:/Windows/…, FileSystem::/Windows/…, curl -o/Windows/…), cmd ^ decoding, short-option clusters, and a cwd command inside a quoted script string.

backend/tests/test_sandbox_tools_windows_drive_paths.py is 55 cases: red on main at ce0fa4c1 (35 failed / 20 passed, all DID NOT RAISE PermissionError), green here (55 passed); with test_sandbox_tools_security.py the pair is 371 passed. ruff check and ruff format --check are clean.

@BlueX888

Copy link
Copy Markdown
Contributor Author

-Path: behaves differently from the other /-rooted forms the body lists as left out. At 31a946e5, Get-Content FileSystem::/Windows/win.ini and Get-Content -LiteralPath:/Windows/win.ini pass, while Get-Content -Path:/Windows/win.ini is rejected by the attached-option scan: _WINDOWS_ATTACHED_DRIVE_PATTERN takes the option's last letter for a drive, so the error names h:/Windows/win.ini.

The same match rejects an allowed path: Get-Content -Path:/mnt/user-data/workspace/notes.txt fails with h:/mnt/user-data/workspace/notes.txt, while main at ce0fa4c1 allows both commands. Distinguishing -Path: from -oC: needs the per-tool option tables this cut removes, so the pattern stays as it is; the alternatives are dropping the attached-option scan (losing the curl -oC: case) or restoring the tables.

The local host-bash guard validates POSIX absolute paths only, so on a
Windows host a drive path (``C:/Windows/win.ini``, ``cat C:/``), a Win32
device or extended-length path, a UNC path and a cmd/PowerShell variable
expansion reach PowerShell, cmd.exe or Git Bash unchanged.

Scan the raw command for those unambiguous spellings on Windows, including
a drive path after an attached short option, and reject a working-directory
target that names a drive, a backslash root or a variable - also behind a
PowerShell parameter such as ``-ErrorAction Stop``. A single-backslash root
as a read argument, a bare drive-relative argument inside a script string
and a ``/``-rooted option value stay out of scope: telling them apart from
pattern text needs per-tool argument tables.
@BlueX888
BlueX888 force-pushed the fix/host-bash-windows-drive-guard branch from 31a946e to d9177ae Compare October 25, 2026 23:58
@BlueX888

Copy link
Copy Markdown
Contributor Author

At d9177aed the three caret spellings still pass: type C^:/Windows/win.ini, type C^:\Windows\win.ini, cd C^:/Windows && type win.ini. The scans read literal text and cmd removes ^ before parsing, so the drive path only exists after preprocessing.

Took the second branch of this comment: validate_local_bash_command_paths now states that cmd.exe ^ escapes and delayed !VAR! expansion are out of scope, so the boundary is in the docstring rather than implicit. Caret decoding was in the c394a011 series and went with the parser layer removed by the size cut (#7089 (comment)); the PR body lists it among the spellings kept out of this cut.

Verified by driving validate_local_bash_command_paths directly with _is_windows() patched: the three forms above and grep '^C:' are allowed, while C:/..., C:\... and cd C:/Windows && type win.ini are rejected. pytest tests/test_sandbox_tools_windows_drive_paths.py tests/test_sandbox_tools_security.py -q passes (371) at d9177aed.

@BlueX888

Copy link
Copy Markdown
Contributor Author

At d9177aed the two patterns this comment names (_WINDOWS_OPTION_ROOTED_PATH_PATTERN, _WINDOWS_ATTACHED_SHORT_OPTION_PATH_PATTERN) are not in the branch: the cut removed the word-level scan they belonged to. The surviving raw cwd scan, _WINDOWS_CWD_ROOT_PATTERN, still misses a delimiter before a backslash root:

  • allowed: cd ,\Windows && type win.ini, pushd =\Windows && type win.ini, Set-Location ,\Windows; type win.ini
  • rejected: cd \Windows && type win.ini, cd ,C:\Windows && type win.ini, type ,\\server\share\secret.txt

Those results come from driving validate_local_bash_command_paths with _is_windows() patched, as the new test file does. The drive and UNC scans have no word-boundary lookbehind, so the delimiter only helps the backslash-root spelling. type ,\Windows\win.ini is allowed too, but a single-backslash read root is already stated out of scope in _windows_host_paths; separating it from regex text needs per-tool argument tables.

Closing the cwd case is one class change in _WINDOWS_CWD_ROOT_PATTERN plus a regression case. The diff is at 299 changed lines, so the new case is line 300 and the size bot relabels the PR size/L, against the cut WillemJiang asked for. It goes in the follow-up with the other excluded spellings (^ decoding, read roots, /-rooted option values) after this lands.

@BlueX888

Copy link
Copy Markdown
Contributor Author

At d9177aed all four commands raise the working-directory error:

  • Set-Location -ErrorAction Stop $env:WINDIR; Get-Content win.ini
  • Set-Location -ErrorAction Stop -Path $env:WINDIR; Get-Content win.ini
  • Push-Location -StackName saved $env:WINDIR; Get-Content win.ini
  • cd -ErrorAction SilentlyContinue $env:WINDIR; Get-Content win.ini

These results come from driving validate_local_bash_command_paths with _is_windows() patched, the fixture shape the new test file uses. _next_cd_target consumes a non-path parameter's value, skips -Path/-LiteralPath/-LP so the argument after them is the validated target, and the variable then hits the cwd-target check. Set-Location -ErrorAction Stop /mnt/user-data/workspace and Push-Location -StackName saved /mnt/user-data/workspace stay allowed. Three of the four are regression cases in backend/tests/test_sandbox_tools_windows_drive_paths.py:58-61; the -Path form reports the same error from the same branch and has no separate case. pytest tests/test_sandbox_tools_windows_drive_paths.py tests/test_sandbox_tools_security.py -q -> 371 passed.

One shape still passes: a value-taking parameter whose value starts with / is not consumed, so Push-Location -StackName /mnt/user-data/workspace $env:WINDIR is allowed and PowerShell binds $env:WINDIR as the Path. Consuming that value without also consuming the target of a switch such as -PassThru needs per-parameter argument tables, which the cut to this diff removed; it belongs with the other argument-table spellings in the follow-up rather than in this diff.

@BlueX888

Copy link
Copy Markdown
Contributor Author

At d9177aed all four commands pass:

  • rg --glob '*.py' '\Aclass' .
  • rg -g '*.py' '\Aclass' .
  • rg --type py '\Aclass' .
  • rg -t py '\Aclass' .

The rg-to-grep option table is not in this diff: the cut to +296/−3 removed the per-tool tables, so a non-cwd argument is not parsed for options at all. The remaining Windows scans reject only unambiguous host-path spellings and treat a quoted or single-backslash word as text, so the regex is no longer classified as a path.

rg -g '*.py' '\Aclass' . is already an allow-list case in backend/tests/test_sandbox_tools_windows_drive_paths.py:77. The --glob, --type and -t spellings take the same code path, since no per-command option table is left for any of them, and three more entries would take the diff to 302 changed lines, over the size/M boundary the cut request set, so the representative case stays.

Results come from driving validate_local_bash_command_paths with _is_windows() patched, the fixture shape the test file uses. pytest tests/test_sandbox_tools_windows_drive_paths.py tests/test_sandbox_tools_security.py -q -> 371 passed.

@BlueX888

Copy link
Copy Markdown
Contributor Author

@willem-bd Could you approve the pending code workflows for the current head (d9177ae)? Five workflows are at action_required with zero jobs created, so unit tests and lint have not run on this revision yet.

On scope: after the trim, are the two residual boundaries I described in the threads acceptable for this PR, or would you prefer them in a follow-up?

  • The token scan still over-blocks some legitimate non-path arguments (words whose second character is :), and the surviving raw cwd scan still misses a delimiter before a backslash-rooted path (e.g. cd ,\Windows ...).
  • The cut removed the per-tool option tables, so option values for rg / PowerShell commands are no longer parsed.

I don't want to expand the diff further without your call on scope.

@BlueX888 BlueX888 closed this Nov 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:sandbox Sandboxed execution and docker/ risk:high High risk: backend API, agents, sandbox, auth, deps, CI size/M PR changes 100-300 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants