Skip to content

fix(studio): resolve bare git on Windows sandbox PATH - #7323

Merged
danielhanchen merged 21 commits into
unslothai:mainfrom
Souravrajvi0:fix/7317-windows-git-path
Jul 24, 2026
Merged

fix(studio): resolve bare git on Windows sandbox PATH#7323
danielhanchen merged 21 commits into
unslothai:mainfrom
Souravrajvi0:fix/7317-windows-git-path

Conversation

@Souravrajvi0

Copy link
Copy Markdown
Contributor

Fixes #7317

Problem

On Windows, Studio's sandboxed terminal tool fails with 'git' is not recognized as an internal or external command even when Git works in every normal cmd/PowerShell on the machine (including the one Studio was launched from). Bare git only works after the model is told to use the 8.3 path C:\PROGRA~1\Git\cmd\git.exe.

_build_safe_env() rebuilds PATH from scratch as Studio venv + System32 only. User-installed Git lives under C:\Program Files\Git\cmd, so it never enters that PATH. Unix sandboxes already include /usr/bin, which is why system git worked there.

Fix

  • studio/backend/core/inference/tools.py: After the curated interpreter / venv / system dirs, append absolute entries from the parent PATH (strip quotes; skip empty / . / relative entries so cwd PATH hijacks stay blocked). On Windows, also inherit PATHEXT so git maps to git.exe like a normal shell.
  • studio/backend/tests/test_sandbox_tools.py: Cover host PATH append order, relative-entry filtering, and quoted Windows-style PATH entries.

Verification

PYTHONPATH=studio/backend python -m pytest studio/backend/tests/test_sandbox_tools.py::TestSandboxEnvIsolation -q
PYTHONPATH=studio/backend python -m pytest studio/backend/tests/test_bypass_permissions.py -q -k 'safe_env or bypass_env or PATH or secret'

8/8 sandbox env tests pass; 32 related bypass/env tests pass.

Compatibility

  • Curated PATH prefix is unchanged: Studio python/pip still win over host copies.
  • Credential whitelist is unchanged (still no HF_TOKEN / USERPROFILE / etc. in the sandbox env).
  • Full-access / bypass mode already inherited host PATH; unchanged.
  • Linux/macOS get the same absolute host-PATH append; relative entries remain excluded.

Souravrajvi0 and others added 2 commits July 22, 2026 07:29
Sandboxed terminal tools rebuilt PATH as venv + System32 only, so
user-installed Git under Program Files never resolved by bare name.
Append absolute host PATH dirs after the curated prefix and inherit
PATHEXT on Windows (unslothai#7317).
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4cf3265e2b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread studio/backend/core/inference/tools.py Outdated
entry = entry.strip().strip('"')
if not entry or entry in (".",) or not os.path.isabs(entry):
continue
path_entries.append(entry)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep auto-approved commands off inherited PATH

When the Studio process is launched with a user-writable absolute PATH entry (for example a venv, node_modules/.bin, or ~/bin) and the curated dirs lack an auto-safe command such as jq or rg, appending every host PATH directory lets the sandbox resolve and run that host executable. The terminal safety gate approves these commands by basename before execution, so this can turn an auto-approved read-only command into arbitrary code from the operator's PATH; restrict inheritance to trusted/tool-specific directories or gate it to the Windows Git case.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in f885a3d.

Host PATH inheritance is now gated to Windows Git install directories only (\Git\cmd, \Git\bin, \Git\mingw64\bin). User-writable absolute PATH entries (venv Scripts, node_modules/.bin, custom bins) are no longer appended, so they cannot shadow auto-safe terminal commands like jq/rg.

Linux/macOS curated PATH (/usr/bin, etc.) already covers system git, so no host inheritance there.

Tests: TestSandboxEnvIsolation (9 passed), including new test_user_writable_host_path_not_inherited.

…nslothai#7323)

Only append Git-for-Windows install directories from the host PATH on
Windows, instead of every absolute entry. This fixes bare `git` resolution
(unslothai#7317) without letting user-writable dirs (venv, node_modules/.bin)
shadow auto-safe terminal commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f885a3d036

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread studio/backend/core/inference/tools.py Outdated
# list when present so bare tool names resolve like a normal shell.
pathext = os.environ.get("PATHEXT")
if pathext:
env["PATHEXT"] = pathext

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep PATHEXT from enabling cwd command hijacks

On Windows, when the sandbox workdir already contains rg.bat/jq.cmd/another safe-command stub, inheriting the host PATHEXT lets cmd /c rg ... resolve that current-directory script before the curated PATH; Microsoft documents both PATHEXT-based extension lookup and that the current directory is searched before PATH, so an auto-approved read-only basename can become arbitrary code execution. Use a fixed minimal extension set plus NoDefaultCurrentDirectoryInExePath, or resolve trusted tool paths explicitly, instead of copying the operator's PATHEXT.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Completed in c51a7a4: sandbox env now also sets NoDefaultCurrentDirectoryInExePath=1 so cmd/CreateProcess skip cwd for bare names.

Use a fixed .EXE;.COM list instead of inheriting the host PATHEXT so
cmd cannot resolve auto-approved bare names from workdir .BAT/.CMD stubs.
@Souravrajvi0
Souravrajvi0 force-pushed the fix/7317-windows-git-path branch from 967a556 to 0bdf41e Compare July 23, 2026 04:36
@Souravrajvi0

Copy link
Copy Markdown
Contributor Author

Fixed in 0bdf41e.

P2 — Keep PATHEXT from enabling cwd command hijacks: Valid. Sandbox env now pins PATHEXT=.EXE;.COM instead of inheriting the host list so workdir .BAT/.CMD stubs cannot hijack auto-approved bare names.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0bdf41efa8

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread studio/backend/core/inference/tools.py Outdated
# without appending user-writable dirs (venv, node_modules/.bin) that could
# shadow auto-safe terminal commands (#7317).
_WINDOWS_GIT_PATH_RE = re.compile(
r"[\\/]git[\\/](?:cmd|bin|mingw64[\\/]bin)(?:[\\/]|$)",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Restrict inherited Git dirs to trusted installs

When Studio runs on Windows with a user-writable host PATH entry that merely ends in \Git\cmd or \Git\mingw64\bin (for example C:\Users\alice\scratch\Git\cmd), this suffix-only allowlist appends that directory to the sandbox PATH. If the curated dirs do not contain an auto-approved command such as rg, a malicious rg.exe in that user-controlled directory can still be selected after the terminal safety gate approves by basename. Fresh evidence relative to the earlier PATH comment is that the new allowlist still trusts any suffix-matching Git path rather than a trusted install root.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed in c51a7a4: the suffix regex is gone; we now append only the directory of the git the host shell resolves via shutil.which, so user-writable Git-named dirs are never trusted.

@Souravrajvi0
Souravrajvi0 marked this pull request as draft July 23, 2026 07:34
@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2408fad677

ℹ️ 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".

Comment thread studio/backend/core/inference/tools.py Outdated
Comment on lines +2534 to +2538
git_exe = shutil.which("git")
if git_exe:
git_dir = os.path.dirname(git_exe)
if os.path.isabs(git_dir):
path_entries.append(git_dir)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve resolved Git wrapper extensions

When the host's working git is a .cmd or .bat wrapper/shim, shutil.which("git") succeeds using the host PATHEXT and this code appends its directory, but the sandbox then restricts PATHEXT to .EXE;.COM. The cmd /c terminal therefore cannot resolve bare git, so the Windows failure remains for those valid host setups. Either preserve the extension needed by the resolved executable or explicitly handle non-EXE Git launchers.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed in d96d12c: the resolved git launcher's extension (e.g. .CMD) is appended to the restricted PATHEXT; cwd lookup stays disabled via NoDefaultCurrentDirectoryInExePath.

danielhanchen added a commit to danielhanchen/unsloth-staging-2 that referenced this pull request Jul 23, 2026
@Souravrajvi0
Souravrajvi0 marked this pull request as ready for review July 23, 2026 10:22
@Souravrajvi0

Copy link
Copy Markdown
Contributor Author

Ready for review — undrafted after @danielhanchen's commits on ``. Maintainer changes incorporated; awaiting CI + review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2408fad677

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread studio/backend/core/inference/tools.py Outdated
if git_exe:
git_dir = os.path.dirname(git_exe)
if os.path.isabs(git_dir):
path_entries.append(git_dir)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Constrain the inherited Git directory to trusted installs

When Git is installed through a per-user manager such as Scoop, shutil.which("git") can resolve to a user-writable shims directory, and this appends that whole directory to the sandbox PATH. In auto mode, if the curated Windows dirs do not contain a safe basename like rg or jq, the safety gate still approves the basename but cmd can execute the copy from that inherited user-writable directory, re-opening arbitrary-code execution from outside the curated PATH. Fresh evidence relative to the earlier suffix-allowlist comment is that the current code trusts the shutil.which directory solely because it is absolute, with no trusted-root or ACL check.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

shutil.which returns the same git the operator's own shell runs. If that directory is attacker-writable, the host user's every git invocation is already compromised; the sandbox cannot meaningfully out-trust the host PATH for the tool the fix exists to expose. ACL checks here would add platform-specific complexity without changing that trust boundary.

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ad481c04b

ℹ️ 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".

Comment thread studio/backend/core/inference/tools.py Outdated
Comment on lines +2535 to +2539
git_exe = shutil.which("git")
if git_exe:
git_dir = os.path.dirname(git_exe)
if os.path.isabs(git_dir):
path_entries.append(git_dir)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Do not trust Git resolved from arbitrary host PATH directories

shutil.which("git") can resolve a launcher from a user-writable host PATH entry (for example, the Scoop shim directory explicitly supported by the new test), but its entire directory is then added to the sandbox PATH. An attacker who can place rg.exe (or another auto-approved utility) beside that launcher can have an auto-approved command such as rg execute arbitrary code without confirmation; the restricted PATHEXT and disabled cwd lookup do not protect against an .exe already in this newly trusted directory. Restrict the inherited location to a trusted Git installation or validate that the resolved directory cannot be user-writable.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch, and correct that the auto-approve gate makes this reachable. Fixed in ccec25e1f: the inherited git directory is now accepted only when it sits under a system install root (Program Files / ProgramW6432 / SystemRoot). Per-user managers like Scoop shims are refused, so an attacker cannot drop rg.exe beside a user-writable git launcher; those users can still run git in full-access mode.

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 98e6124c1e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

norm = os.path.normcase(os.path.normpath(path))
for root in roots:
root_norm = os.path.normcase(os.path.normpath(root))
if norm == root_norm or norm.startswith(root_norm + os.sep):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Limit trusted Windows roots to non-writable dirs

When the host PATH resolves git from a writable directory under %SystemRoot% such as C:\Windows\Temp, this prefix check treats that directory as trusted and appends it to the sandbox PATH. Standard users can create files in Windows\Temp, so a sibling rg.exe/jq.exe there can still be selected after the auto-approval gate approves the basename; the fresh case here is the recursive trust of all %SystemRoot% subdirectories rather than arbitrary user PATH suffixes. Restrict this to specific non-user-writable install locations or explicitly exclude writable Windows subdirectories.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed in 00ebff73e: SystemRoot is no longer a trusted root (Git does not install there and it holds world-writable Windows\Temp). Only the Program Files roots are trusted now.

Comment thread studio/backend/core/inference/tools.py Outdated
git_exe = shutil.which("git")
if git_exe:
git_dir = os.path.dirname(git_exe)
if os.path.isabs(git_dir) and _is_trusted_windows_program_dir(git_dir):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Expand short Program Files paths before trusting Git

On Windows hosts whose PATH contains Git through the DOS 8.3 alias, for example C:\PROGRA~1\Git\cmd, shutil.which("git") returns that short path but this trust check compares it textually against the long %ProgramFiles% roots. The directory is therefore refused and bare git remains unavailable in the sandbox even though it is the normal system Git install this change is meant to support; expand/canonicalize short names before applying the trusted-root check.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed in 00ebff73e: both the resolved git dir and the roots are canonicalized with realpath before comparison, so an 8.3 short alias like C:\PROGRA~1\Git\cmd matches Program Files and bare git stays available.

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 812ae238f9

ℹ️ 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".

Comment thread studio/backend/core/inference/tools.py Outdated
Comment on lines +2511 to +2514
for var in ("ProgramFiles", "ProgramFiles(x86)", "ProgramW6432"):
val = os.environ.get(var)
if val:
roots.append(val)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Do not trust overrideable ProgramFiles environment values

When Studio is launched with ProgramFiles (or the other checked variables) overridden to a user-writable directory, that directory is treated as a trusted install root. A fake git.exe there will be returned by shutil.which, its directory will be appended to the sandbox PATH, and an auto-approved bare command such as rg can then resolve to another attacker-controlled executable placed beside it. This defeats the user-writable-PATH exclusion; validate the actual directory ACL/known-folder location rather than trusting an inherited environment value.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed in 5f1aa1ca3: the Program Files roots are now resolved via the Windows known-folder API (SHGetKnownFolderPath), so an overridden %ProgramFiles% env value no longer moves the trust boundary. Env vars are only a fallback when the API is unavailable.

Comment thread studio/backend/core/inference/tools.py Outdated
Comment on lines +2569 to +2570
if os.path.isabs(git_dir) and _is_trusted_windows_program_dir(git_dir):
path_entries.append(git_dir)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Append the canonical Git directory to PATH

The trust check resolves symlinks with realpath, but the PATH entry retains the unresolved git_dir. A user-controlled junction in a host PATH can initially point at Program Files so it passes this check, then be retargeted to a writable directory before a sandboxed auto-approved command executes; commands such as rg will then resolve from that attacker-controlled directory. Append the canonical path used for the trust decision (or otherwise retain an immutable trusted target) instead.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed in 5f1aa1ca3: the canonical realpath of the trusted dir is appended to PATH, not the raw entry, so a junction that passed the check cannot be retargeted to a writable dir before a sandboxed command runs.

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 080286bab5

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread studio/backend/core/inference/tools.py Outdated
Comment on lines +2530 to +2534
if not roots:
for var in ("ProgramFiles", "ProgramFiles(x86)", "ProgramW6432"):
val = os.environ.get(var)
if val:
roots.append(val)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Include the native Program Files root for 32-bit Windows

On a 32-bit Python/Studio process running on 64-bit Windows, the known-folder API maps both FOLDERID_ProgramFiles and FOLDERID_ProgramFilesX86 to C:\Program Files (x86) (Microsoft table), so roots is non-empty and this env/fixed-root fallback never adds the native C:\Program Files root. In that environment a normal 64-bit Git install at C:\Program Files\Git\cmd is rejected by _is_trusted_windows_program_dir, leaving bare git unavailable in the sandbox; query a trusted native Program Files root as well instead of only falling back when no roots were found.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed in 58a650acb: FOLDERID_ProgramFilesX64 is now queried alongside the other two, so a 32-bit process still trusts the native C:\Program Files (where the first two ids both map to x86). The env fallback also already includes ProgramW6432.

Comment on lines +318 to +320
prog = tmp_path / "Program Files"
monkeypatch.setenv("ProgramFiles", str(prog))
git_dir = prog / "Git" / "cmd"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Stub known-folder roots in Windows-specific env tests

When these tests run on an actual Windows runner, setting %ProgramFiles% here no longer makes tmp_path/Program Files trusted because _windows_program_roots() now uses SHGetKnownFolderPath before consulting the environment. That means the fake git.exe under tmp_path is rejected and the assertion below fails even though the production code is behaving as designed; monkeypatch _windows_program_roots in the positive tests the same way the later known-folder test does.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch. Fixed in 58a650acb: the positive Windows tests now monkeypatch _windows_program_roots instead of setting %ProgramFiles%, so they hold on a real Windows runner where SHGetKnownFolderPath is consulted first.

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0df5795ca3

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread studio/backend/core/inference/tools.py Outdated
Comment on lines +2603 to +2606
git_exe = shutil.which("git")
if git_exe:
git_dir = os.path.dirname(git_exe)
if os.path.isabs(git_dir) and _is_trusted_windows_program_dir(git_dir):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Continue past untrusted git shims

When a Windows PATH contains a user-writable git shim before a normal C:\Program Files\Git\cmd entry, shutil.which("git") returns only that first shim; this check rejects it and then stops, so the trusted Program Files Git later on PATH is never appended and cmd /c git ... remains not recognized. Fresh evidence versus the earlier trust-boundary comments is that the final code skips untrusted first matches without scanning for a later trusted match; iterate the PATH/PATHEXT candidates until a trusted Git dir is found.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed in bf1eb19: git resolution now scans all PATH/PATHEXT candidates for a trusted Program Files launcher instead of stopping at shutil.which's first (possibly untrusted shim) match.

Comment thread studio/backend/core/inference/tools.py Outdated
Comment on lines +2533 to +2537
if not roots:
for var in ("ProgramFiles", "ProgramFiles(x86)", "ProgramW6432"):
val = os.environ.get(var)
if val:
roots.append(val)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Add the native Program Files root after x86 matches

On a 32-bit Studio process running on 64-bit Windows, the first known-folder calls can populate roots with only C:\Program Files (x86), while Microsoft documents FOLDERID_ProgramFilesX64 as unsupported for 32-bit applications on 64-bit Windows, so this if not roots fallback never adds the native C:\Program Files root. Fresh evidence versus the earlier 32-bit-root thread is that the current fix relies on the unsupported X64 known-folder ID; derive and add a trusted native root even when the API returned only x86 roots, otherwise a normal 64-bit Git install is still rejected.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed in bf1eb19: since FOLDERID_ProgramFilesX64 is unsupported for 32-bit apps, the native root is now derived by stripping the ' (x86)' suffix from the trusted x86 root, so a 64-bit Git install is trusted without relying on that id.

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1bb316460

ℹ️ 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".

Comment thread studio/backend/core/inference/tools.py Outdated
Comment on lines +2533 to +2537
if not roots:
for var in ("ProgramFiles", "ProgramFiles(x86)", "ProgramW6432"):
val = os.environ.get(var)
if val:
roots.append(val)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep fallback Program Files roots out of the trust boundary

When the known-folder API is unavailable or raises, this fallback treats the parent-controlled ProgramFiles variables as admin-managed roots. A user who launches Studio with ProgramFiles pointing at a writable directory and a git.exe there can make that directory enter the sandbox PATH; subsequently, an auto-approved bare command such as rg can resolve to an attacker-controlled sibling in that directory. Fail closed or use a non-environment-derived trusted root in this path.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed in cbd15ce70: the known-folder-unavailable fallback no longer reads %ProgramFiles%/%ProgramW6432%; it uses fixed %SystemDrive%\Program Files roots (SystemDrive is not a viable override, since retargeting it breaks the OS), so an overridden ProgramFiles cannot relocate the trust boundary.

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 877895dcc0

ℹ️ 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".

Comment thread studio/backend/core/inference/tools.py Outdated
Comment on lines +2537 to +2538
drive = os.environ.get("SystemDrive", "C:")
roots = [drive + r"\Program Files", drive + r"\Program Files (x86)"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Do not derive trusted Program Files from SystemDrive

When the known-folder API is unavailable, SystemDrive is still just an inherited, overrideable environment variable; setting it to a writable drive makes that drive’s Program Files directory a trusted root. If the parent PATH then resolves git from that location, this code appends the attacker-controlled directory to the sandbox PATH, allowing a colocated executable such as an auto-approved rg.exe to run. Use a non-environment-derived OS API/fixed system root for this fallback, or refuse to add Git when the trusted root cannot be resolved.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed in 4bac1aa. Correct: %SystemDrive% is itself a caller-overrideable environment variable, so deriving a fallback trusted root from it still lets an attacker relocate the trust boundary onto a writable drive. The resolver now fails closed: when the known-folder API (SHGetKnownFolderPath) is unavailable it returns no roots at all, so Git is not appended to the sandbox PATH. On any real Windows host shell32 is present, so this only affects broken or non-Windows environments where the sandbox git-PATH feature is not needed.

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 4bac1aa319

ℹ️ 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".

@danielhanchen

Copy link
Copy Markdown
Member

Reviewed and verified. In the studio sandbox, bare git on Windows was not resolvable because the sandbox PATH was stripped, so git backed tools failed. This resolves a trusted git into the sandbox PATH while keeping the trust boundary tight. Over review we hardened it so the trusted Program Files root is resolved only from the Windows known folder API (SHGetKnownFolderPath), never from caller overrideable environment variables including ProgramFiles, ProgramW6432, and SystemDrive, and it fails closed when that API is unavailable so no untrusted directory can ever enter the sandbox PATH. Covered with 145 sandbox tests including case sensitivity, PATHEXT, 8.3 short paths, and untrusted shim rejection. Staging CI confirmed the relevant Linux, Mac, and Windows jobs green. Codex review converged and approved. Good to merge.

@danielhanchen
danielhanchen merged commit f5a0c22 into unslothai:main Jul 24, 2026
28 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Command executor on Windows can't use git despite it being installed

2 participants