feat(allowedpaths): support cross-root symlinks by matt-dz · Pull Request #169 · DataDog/rshell

matt-dz · 2026-04-02T15:51:41Z

Summary

When a symlink in one AllowedPaths root points to a file/directory in another allowed root, the read now succeeds instead of failing with "path escapes from parent"
On os.Root escape errors, the sandbox walks path components to find the escaping symlink, resolves its target, and retries through the matching allowed root
Handles both leaf symlinks and intermediate directory symlinks
Circular symlinks are detected and rejected after 10 hops

Closes #167

Test plan

9 new unit tests covering: leaf symlinks, directory symlinks, relative targets, intermediate directories, escape-to-outside blocking, circular loops, chain depth limits, and the exact issue feat(allowedpaths): support symlinks whose targets resolve within a different allowed-path root #167 scenario
3 new end-to-end scenario tests: cross-root read, cross-root directory traversal, escape-to-outside blocking
Full test suite passes (allowedpaths, interp, tests, analysis)

🤖 Generated with Claude Code

…ifferent allowed root When an os.Root operation fails with "path escapes from parent", the sandbox now walks the relative path component by component to find the escaping symlink, reads its target, resolves it to an absolute path, and retries through whichever allowed root matches. This handles both leaf symlinks (e.g. link.txt -> /other-root/file.txt) and intermediate directory symlinks (e.g. pods/ -> /other-root/). A depth cap of 10 hops prevents infinite loops from circular symlinks. Closes #167 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-04-02T15:53:49Z

@codex conduct a comprehensive security and code review. beware that TOCTOU vulnerabilities are not a large deal because if a user can swap symlinks they likely already have root access to the system.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3988c28080

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

chatgpt-codex-connector · 2026-04-02T15:59:48Z

+	if err != nil && isPathEscapeError(err) {
+		if r, rel, ok := s.resolveFollowingSymlinks(absPath); ok {
+			f, err = r.Open(rel)
+		}


Keep loop detection working for Windows cross-root directories

This new fallback makes cross-root directory symlinks traversable, but Windows loop detection still depends on allowedpaths.FileIdentity, which uses sandbox.resolve(absPath) and cannot resolve those same cross-root symlink paths. In find -L, that drops to path-string cycle checks (builtins/find/find.go), which do not catch A/link -> B and B/link -> A because each descent has a new path, so traversal can grow until timeout instead of terminating on loop detection.

Useful? React with 👍 / 👎.

[Claude Opus 4.6] Valid concern, but this is a pre-existing limitation in FileIdentity on Windows, not introduced by this PR. The fix would be to update portable_windows.go:FileIdentity to fall back to resolveFollowingSymlinks when resolve fails, but that is Windows-only code we cannot test on this macOS machine. Recommend a follow-up PR with Windows CI coverage.

… findings - resolveFollowingSymlinks now delegates to resolveRootFollowingSymlinks instead of duplicating the component-walking logic - Remove unused loop variable (for i, comp -> for i := range) - Fix double negative in test assertion (assert.False x != 0 -> assert.Zero) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

When Lstat fails on a component during symlink resolution (e.g. the target file doesn't exist), return the current root and rel instead of false. This lets the caller retry the operation and get the real error (ENOENT) instead of the original "path escapes from parent". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-04-02T16:11:07Z

@codex conduct a comprehensive security and code review. beware that TOCTOU vulnerabilities are not a large deal because if a user can swap symlinks they likely already have root access to the system. additionally, only focus on reviewing issues on this PR, if there is a pre-existing issue, now is not the time to bring it up.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: feab3b26cb

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

The loop needs N+1 iterations for N symlink hops: N iterations to resolve each symlink, plus 1 final iteration to confirm the resolved path has no more symlinks and return success. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-04-02T16:21:36Z

@codex conduct a comprehensive security and code review. beware that TOCTOU vulnerabilities are not a large deal because if a user can swap symlinks they likely already have root access to the system. additionally, only focus on reviewing issues on this PR, if there is a pre-existing issue, now is not the time to bring it up.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bc717470d2

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

…llback resolveRootFollowingSymlinks now accepts a preserveLast flag. When true, the final path component is not resolved even if it is a symlink. Lstat and Readlink pass true so they operate on the symlink itself rather than its target. All other callers pass false for full resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-04-02T18:05:40Z

@codex conduct a comprehensive security and code review. beware that TOCTOU vulnerabilities are not a large deal because if a user can swap symlinks they likely already have root access to the system. additionally, only focus on reviewing issues on this PR, if there is a pre-existing issue, now is not the time to bring it up.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7f891cfcee

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

filepath.Dir on a trailing-slash path (e.g. "/dir/link/") strips only the slash, producing the wrong parent for relative symlink resolution. Clean absPath at entry to resolveRootFollowingSymlinks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz · 2026-04-02T18:17:25Z

@codex conduct a comprehensive security and code review. beware that TOCTOU vulnerabilities are not a large deal because if a user can swap symlinks they likely already have root access to the system. additionally, only focus on reviewing issues on this PR, if there is a pre-existing issue, now is not the time to bring it up.

chatgpt-codex-connector · 2026-04-02T18:20:48Z

Security & Code Review (PR: `feat(allowedpaths): support cross-root symlinks`)

I reviewed the current PR state (including the follow-up fixes through b7c9fbf) and focused only on issues introduced by this PR’s changes.

Findings

1) P2 – Trailing-slash semantics regression in `Lstat`/`Readlink` cross-root fallback

When cross-root fallback is triggered, resolveRootFollowingSymlinks unconditionally normalizes the input with filepath.Clean(absPath), which strips trailing separators. That changes path semantics for Lstat and Readlink in cases where trailing slash should force directory interpretation (and commonly return ENOTDIR). allowedpaths/sandbox.goL139-L143

Lstat/Readlink now call that fallback with preserveLast=true, but after trailing slash removal they can incorrectly succeed on a terminal symlink path that should fail when called as .../link/. allowedpaths/sandbox.goL588-L600 allowedpaths/sandbox.goL615-L627

Why this is PR-scoped:
This behavior was introduced by the new cross-root resolution/fallback path and then reinforced by the trailing-slash cleanup fix in this PR series.

Suggested fix direction:
Preserve whether the original input had a trailing separator for operations where that matters (Lstat, Readlink), and enforce that semantics after cross-root root/rel resolution (or avoid stripping it before semantics checks).

I did not flag TOCTOU-style concerns per your guidance, and I did not raise unrelated pre-existing issues.

Checks run

✅ go test ./allowedpaths -run CrossRoot -count=1
✅ go test ./tests -run 'TestShellScenarios/(cross_root_symlink_read|cross_root_symlink_dir|cross_root_symlink_outside_blocked)' -count=1
✅ go run /tmp/repro_lstat.go (custom repro: sb.Lstat("bridge/leaf.txt/", dir2) returned success + symlink mode)
✅ go run /tmp/repro_native.go (baseline: native os.Lstat(".../l/") and os.Readlink(".../l/") return not a directory)

View task →

matt-dz · 2026-04-02T18:25:52Z

@codex conduct a comprehensive security and code review. beware that TOCTOU vulnerabilities are not a large deal because if a user can swap symlinks they likely already have root access to the system. additionally, only focus on reviewing issues on this PR, if there is a pre-existing issue, now is not the time to bring it up.

chatgpt-codex-connector · 2026-04-02T18:32:54Z

Codex Review: Didn't find any major issues. Can't wait for the next one!

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

thieman · 2026-04-02T19:07:48Z

+const (
+	modeRead    = 0x04
+	modeWrite   = 0x02
+	modeExecute = 0x01


Nit: are there constants for these in fs or another package?

[Claude Opus 4.6] Checked — Go does not export POSIX R_OK/W_OK/X_OK constants. syscall has them only in test files as unexported _R_OK etc. Our private modeRead/modeWrite/modeExecute constants are the idiomatic approach.

- Add TestIsPathEscapeError regression test to catch Go stdlib changes - Extract openWithSymlinkFallback helper to deduplicate the repeated escape-check-and-retry pattern in readDirN, OpenDir, IsDirEmpty, and ReadDirLimited Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…function Remove the duplicate resolve that returned *os.Root. The single resolve now returns *root, and callers access .root where they need the os.Root handle. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

FileIdentity in portable_windows.go used the old resolve() that returned *os.Root. Updated to use .root on the returned *root entry. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

chatgpt-codex-connector Bot reviewed Apr 2, 2026

View reviewed changes

matt-dz and others added 2 commits April 2, 2026 12:03

chatgpt-codex-connector Bot reviewed Apr 2, 2026

View reviewed changes

Comment thread allowedpaths/sandbox.go Outdated

chatgpt-codex-connector Bot reviewed Apr 2, 2026

View reviewed changes

Comment thread allowedpaths/sandbox.go Outdated

Comment thread allowedpaths/sandbox.go Outdated

chatgpt-codex-connector Bot reviewed Apr 2, 2026

View reviewed changes

Comment thread allowedpaths/sandbox.go

matt-dz marked this pull request as ready for review April 2, 2026 18:33

matt-dz requested review from AlexandreYang, astuyve, julesmcrt, thieman and val06 as code owners April 2, 2026 18:33

thieman reviewed Apr 2, 2026

View reviewed changes

Comment thread allowedpaths/sandbox.go

thieman reviewed Apr 2, 2026

View reviewed changes

Comment thread allowedpaths/sandbox.go

thieman reviewed Apr 2, 2026

View reviewed changes

Comment thread allowedpaths/sandbox.go Outdated

matt-dz and others added 2 commits April 2, 2026 15:32

fix(allowedpaths): fix Windows build after resolve return type change

1ee336a

FileIdentity in portable_windows.go used the old resolve() that returned *os.Root. Updated to use .root on the returned *root entry. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

matt-dz added the verified/allowed_symbols label Apr 2, 2026

matt-dz enabled auto-merge April 2, 2026 19:53

thieman approved these changes Apr 2, 2026

View reviewed changes

matt-dz added this pull request to the merge queue Apr 2, 2026

Merged via the queue into main with commit 735a97d Apr 2, 2026
34 of 35 checks passed

matt-dz deleted the matt-dz/cross-root-symlinks branch April 2, 2026 19:59

Conversation

matt-dz commented Apr 2, 2026

Summary

Test plan

Uh oh!

matt-dz commented Apr 2, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

matt-dz Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

matt-dz commented Apr 2, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

matt-dz commented Apr 2, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

matt-dz commented Apr 2, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

matt-dz commented Apr 2, 2026

Uh oh!

chatgpt-codex-connector Bot commented Apr 2, 2026

Security & Code Review (PR: feat(allowedpaths): support cross-root symlinks)

Findings

1) P2 – Trailing-slash semantics regression in Lstat/Readlink cross-root fallback

Checks run

Uh oh!

matt-dz commented Apr 2, 2026

Uh oh!

chatgpt-codex-connector Bot commented Apr 2, 2026

Uh oh!

Uh oh!

Uh oh!

thieman Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

matt-dz Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Security & Code Review (PR: `feat(allowedpaths): support cross-root symlinks`)

1) P2 – Trailing-slash semantics regression in `Lstat`/`Readlink` cross-root fallback