Repository navigation
Fix ValidatePath symlink escape vulnerability in tool sandbox - #41
kriserickson merged 6 commits into
Conversation
Co-authored-by: kriserickson <325934+kriserickson@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22782213a8
ℹ️ 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".
| if resolvedClean != cwdClean && !strings.HasPrefix(resolvedClean, cwdClean+string(os.PathSeparator)) { | ||
| return "", fmt.Errorf("path %q is outside the working directory", path) |
There was a problem hiding this comment.
When cwd itself is a symlink (e.g. user cds into /tmp/link where /tmp/link -> /tmp/real), filepath.EvalSymlinks(abs) returns /tmp/real/... but the new containment check still compares that against the lexical cwdClean (/tmp/link). This makes valid in-project paths fail as “outside the working directory,” breaking read_file/list_directory for symlinked working directories. Repro: ValidatePath("f.txt", "/tmp/link") now errors even though f.txt is inside the project; compare against a symlink-resolved cwd (or both forms) before rejecting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR hardens the tool sandbox’s path validation to prevent symlink-based escapes from the working directory and adjusts tests to validate the new behavior and reduce environment-test flakiness.
Changes:
- Update
ValidatePathto resolve symlinks and re-check cwd containment (and blocked patterns) against the real target. - Add a symlink-based regression test for
ValidatePath, and update env filtering expectations (DATABASE_URLredaction). - Rework the environment tool test to avoid reliance on truncated tool output by testing
FilterEnvironmentwith controlled input.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
internal/tools/safety.go |
Adds symlink resolution to ValidatePath to prevent escaping cwd via symlinks. |
internal/tools/safety_test.go |
Adds a symlink escape test and updates expected environment redaction behavior. |
internal/tools/tools_test.go |
Makes TestExecute_Environment less flaky by avoiding assertions dependent on truncated output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| output, err := Execute("environment", nil, shell.Info{}) | ||
| if err != nil { | ||
| t.Fatalf("Execute error: %v", err) | ||
| } | ||
| if !strings.Contains(output, "TEST_SAFE_VAR=visible") { | ||
| t.Error("expected safe var to be visible") | ||
| if output == "" { | ||
| t.Error("expected non-empty environment output") | ||
| } | ||
| // Sensitive values must never appear in the raw form | ||
| if strings.Contains(output, "should_be_hidden") { | ||
| t.Error("expected API_KEY value to be redacted") | ||
| } |
There was a problem hiding this comment.
TestExecute_Environment still asserts redaction by scanning the (potentially truncated) tool output for the raw secret. That can be a false negative if the relevant line is truncated out, so the test may pass even if redaction regresses. Consider making the tool output deterministic by temporarily saving os.Environ(), calling os.Clearenv(), setting only a few vars for the duration of this test, and restoring the original environment in a t.Cleanup (or refactor execEnvironment to accept an env slice so it can be tested without relying on process env size/order).
| // Resolve symlinks so that a link inside cwd pointing outside is caught. | ||
| // If EvalSymlinks fails (e.g. the path does not exist yet), fall back to | ||
| // the lexically cleaned path: no symlink can exist for a non-existent path, | ||
| // so the lexical containment check above is sufficient in that case. | ||
| if resolved, err := filepath.EvalSymlinks(abs); err == nil { | ||
| resolvedClean := filepath.Clean(resolved) | ||
| if resolvedClean != cwdClean && !strings.HasPrefix(resolvedClean, cwdClean+string(os.PathSeparator)) { | ||
| return "", fmt.Errorf("path %q is outside the working directory", path) | ||
| } | ||
| resolvedRel, relErr := filepath.Rel(cwdClean, resolvedClean) | ||
| if relErr != nil { | ||
| return "", fmt.Errorf("cannot compute relative path: %w", relErr) | ||
| } | ||
| if isBlocked(resolvedRel) { | ||
| return "", fmt.Errorf("access to %q is blocked for security", path) | ||
| } | ||
| return resolvedClean, nil | ||
| } |
There was a problem hiding this comment.
When EvalSymlinks(abs) succeeds, the function returns before checking blocked patterns against the lexical relative path. This allows bypassing name-based blocks by creating a blocked-name symlink (e.g., .ssh or .env) inside cwd that points to an allowed file/dir, since only the resolved target is checked. Consider always running isBlocked(relLexical) first (based on the requested path under cwd), and then additionally checking isBlocked(resolvedRel) after symlink resolution.
| // If EvalSymlinks fails (e.g. the path does not exist yet), fall back to | ||
| // the lexically cleaned path: no symlink can exist for a non-existent path, | ||
| // so the lexical containment check above is sufficient in that case. | ||
| if resolved, err := filepath.EvalSymlinks(abs); err == nil { | ||
| resolvedClean := filepath.Clean(resolved) | ||
| if resolvedClean != cwdClean && !strings.HasPrefix(resolvedClean, cwdClean+string(os.PathSeparator)) { | ||
| return "", fmt.Errorf("path %q is outside the working directory", path) | ||
| } | ||
| resolvedRel, relErr := filepath.Rel(cwdClean, resolvedClean) | ||
| if relErr != nil { | ||
| return "", fmt.Errorf("cannot compute relative path: %w", relErr) | ||
| } | ||
| if isBlocked(resolvedRel) { | ||
| return "", fmt.Errorf("access to %q is blocked for security", path) | ||
| } | ||
| return resolvedClean, nil | ||
| } |
There was a problem hiding this comment.
The comment/logic that falls back to the lexical path when filepath.EvalSymlinks fails is not safe as written. A symlink can exist even if its target does not (dangling symlink), and EvalSymlinks can also fail for reasons other than non-existence (permissions, broken components), which would re-enable the original symlink-escape behavior by returning abs. Consider: (1) handling os.IsNotExist(err) separately, (2) using os.Lstat to detect whether any existing path component is a symlink, and/or (3) denying access when symlink resolution fails (since read/list operations require an existing target anyway).
| if abs != cwdClean && !strings.HasPrefix(abs, cwdClean+string(os.PathSeparator)) { | ||
| return "", fmt.Errorf("path %q is outside the working directory", path) |
There was a problem hiding this comment.
The strings.HasPrefix(abs, cwdClean+string(os.PathSeparator)) containment check breaks when cwdClean is a filesystem root (e.g. / on Unix or C:\ on Windows), because appending an extra separator changes the prefix (// or C:\\) and valid in-cwd paths will be rejected. Consider switching to a filepath.Rel-based check (reject if rel starts with ..) or special-casing volume/root paths when building the prefix.
|
|
||
| // Create a symlink inside cwd pointing to the file outside cwd | ||
| symlink := filepath.Join(cwd, "link.txt") | ||
| if err := os.Symlink(secretFile, symlink); err != nil { |
There was a problem hiding this comment.
os.Symlink is not reliably available on all platforms/CI setups (notably Windows often requires admin or Developer Mode). As written, this test will hard-fail in those environments. Consider skipping the test when symlinks aren’t supported (e.g., gate on runtime.GOOS == "windows" or errors.Is(err, syscall.EPERM) / os.IsPermission(err) from os.Symlink).
| if err := os.Symlink(secretFile, symlink); err != nil { | |
| if err := os.Symlink(secretFile, symlink); err != nil { | |
| if os.IsPermission(err) { | |
| t.Skipf("skipping symlink test: symlinks not permitted on this platform/CI: %v", err) | |
| } |
…-a6a4-be63004e459c
…ling in execCheckCommand
…erShell; update cwd handling in TestValidatePath for cross-platform compatibility; set USERPROFILE in memory tests; improve error handling in TestExecute_CheckCommand_NotFound
ValidatePathused only lexical path normalization (filepath.Clean), so a symlink insidecwdpointing outside it (e.g.cwd/link → /etc) would pass the containment check and allowread_file/list_directoryto access files outside the project tree.Changes
internal/tools/safety.go— After lexical normalization, callfilepath.EvalSymlinkson the resolved path and re-run both the cwd boundary check and blocked-pattern check on the real target. Falls back to the lexical path ifEvalSymlinksfails (path doesn't exist; no symlink can exist for a non-existent path so lexical is sufficient).internal/tools/safety_test.go— AddedTestValidatePath_Symlinkusing real temp dirs and a symlink to verify the escape is caught. UpdatedTestFilterEnvironmentto expectDATABASE_URL=[REDACTED](consistent withDATABASEalready being insensitiveEnvKeys).internal/tools/tools_test.go— FixedTestExecute_Environmentwhich was flaky due to output truncation: the real environment exceeds the 4096-byte tool output limit, so vars appended byt.Setenvwere silently dropped. Filtering assertions now useFilterEnvironmentdirectly with controlled input.💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.