fix(sandbox): reject host symlinks during seed imports#793
Merged
Conversation
GocciaSandboxRunner --seed and --seed-config host imports read files through TFileStream, which dereferences symlinks, so a symlink inside a seeded directory (or named directly as a seed) copied bytes from outside the requested seed root into the sandbox VFS — contrary to the sandbox-first, reduced-attack-surface posture in VISION.md. Reject host symlinks at the seed leaf and every descendant of a seeded directory via a cross-platform HostPathIsSymlink helper in the shared FileUtils unit (POSIX lstat+S_ISLNK; Windows reparse points/junctions via faSymLink), failing closed with an error that names the path. Trailing separators are stripped before the check so a trailing slash cannot make lstat follow a symlinked leaf. Ancestor symlinks in the host-supplied path (e.g. macOS /var) and full real-path containment are documented non-goals. Adds ADR 0070, a docs/build-system.md note, and POSIX-guarded CLI regressions covering all four leak vectors. Closes #786 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a cross-platform symlink check, uses it to reject symlinked host seed inputs in SandboxRunner, and adds regression tests plus ADR/build-system documentation for the rejection policy. ChangesSymlink rejection in sandbox seed imports
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Suite TimingTest Runner (interpreted: 10,821 passed; bytecode: 10,821 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 430; bytecode: 430)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results430 benchmarks Interpreted: 🟢 31 improved · 🔴 32 regressed · 367 unchanged · avg -0.0% arraybuffer.js — Interp: 🟢 1, 13 unch. · avg +0.9% · Bytecode: 🔴 13, 1 unch. · avg -19.7%
arrays.js — Interp: 19 unch. · avg -0.8% · Bytecode: 🔴 19 · avg -21.7%
async-await.js — Interp: 6 unch. · avg +0.2% · Bytecode: 🔴 6 · avg -24.1%
async-generators.js — Interp: 2 unch. · avg -7.1% · Bytecode: 🔴 1, 1 unch. · avg -24.0%
atomics.js — Interp: 🔴 2, 4 unch. · avg -1.0% · Bytecode: 🔴 6 · avg -20.9%
base64.js — Interp: 🔴 1, 9 unch. · avg -2.4% · Bytecode: 🔴 10 · avg -22.9%
classes.js — Interp: 🟢 1, 🔴 1, 29 unch. · avg +0.5% · Bytecode: 🔴 25, 6 unch. · avg -20.7%
closures.js — Interp: 🔴 1, 10 unch. · avg +1.3% · Bytecode: 🔴 11 · avg -23.2%
collections.js — Interp: 🔴 1, 11 unch. · avg +0.1% · Bytecode: 🔴 11, 1 unch. · avg -23.3%
csv.js — Interp: 🟢 2, 🔴 1, 10 unch. · avg -2.0% · Bytecode: 🔴 10, 3 unch. · avg -20.4%
destructuring.js — Interp: 🟢 1, 🔴 2, 19 unch. · avg -0.3% · Bytecode: 🔴 21, 1 unch. · avg -20.9%
fibonacci.js — Interp: 🔴 1, 7 unch. · avg -0.3% · Bytecode: 🔴 7, 1 unch. · avg -22.8%
float16array.js — Interp: 🟢 10, 🔴 1, 21 unch. · avg +3.6% · Bytecode: 🔴 28, 4 unch. · avg -23.4%
for-of.js — Interp: 🔴 1, 6 unch. · avg +0.2% · Bytecode: 🔴 6, 1 unch. · avg -21.8%
generators.js — Interp: 🟢 1, 3 unch. · avg +3.2% · Bytecode: 🔴 4 · avg -24.5%
intl.js — Interp: 6 unch. · avg +1.2% · Bytecode: 🔴 6 · avg -21.3%
iterators.js — Interp: 🟢 1, 🔴 1, 40 unch. · avg -0.1% · Bytecode: 🔴 35, 7 unch. · avg -20.7%
json.js — Interp: 🟢 2, 18 unch. · avg +1.5% · Bytecode: 🔴 19, 1 unch. · avg -20.9%
jsx.jsx — Interp: 🔴 1, 20 unch. · avg -3.0% · Bytecode: 🔴 18, 3 unch. · avg -21.7%
modules.js — Interp: 9 unch. · avg +3.7% · Bytecode: 🔴 9 · avg -22.8%
numbers.js — Interp: 11 unch. · avg -0.2% · Bytecode: 🔴 10, 1 unch. · avg -24.5%
objects.js — Interp: 7 unch. · avg +3.2% · Bytecode: 🔴 6, 1 unch. · avg -19.4%
promises.js — Interp: 🔴 2, 10 unch. · avg -3.3% · Bytecode: 🔴 6, 6 unch. · avg -17.7%
property-access.js — Interp: 🔴 1, 4 unch. · avg +1.3% · Bytecode: 🔴 3, 2 unch. · avg -23.0%
regexp.js — Interp: 11 unch. · avg -0.4% · Bytecode: 🔴 9, 2 unch. · avg -19.2%
strings.js — Interp: 🟢 1, 🔴 1, 17 unch. · avg -0.2% · Bytecode: 🔴 15, 4 unch. · avg -21.8%
temporal.js — Interp: 🟢 1, 5 unch. · avg +3.8% · Bytecode: 🔴 6 · avg -26.4%
tsv.js — Interp: 🟢 1, 🔴 1, 7 unch. · avg -3.2% · Bytecode: 🔴 9 · avg -18.8%
typed-arrays.js — Interp: 🟢 7, 🔴 1, 14 unch. · avg +7.5% · Bytecode: 🔴 21, 1 unch. · avg -34.5%
uint8array-encoding.js — Interp: 🟢 2, 🔴 6, 10 unch. · avg -4.9% · Bytecode: 🟢 4, 🔴 11, 3 unch. · avg -8.1%
weak-collections.js — Interp: 🔴 6, 9 unch. · avg -9.3% · Bytecode: 🟢 3, 🔴 10, 2 unch. · avg -6.6%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/test-cli-apps.ts`:
- Line 2745: Add Windows regression coverage for the new symlink policy by
updating the symlink-related tests in test-cli-apps.ts so the win32 branch is
exercised instead of fully skipped. Keep the existing non-Windows assertions,
and add at least one Windows-specific case using a junction created with
symlinkSync(..., "junction") that verifies the same rejection behavior as the
other symlink tests. Use the existing test blocks around the affected symlink
policy checks to locate the right place and ensure the new Windows case covers
the same security contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d09da77-4967-4209-bc05-988ca6b587e1
📒 Files selected for processing (6)
docs/adr/0070-reject-symlinks-in-sandbox-seed-imports.mddocs/adr/README.mddocs/build-system.mdscripts/test-cli-apps.tssource/app/GocciaSandboxRunner.dprsource/shared/FileUtils.pas
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+35 / -2)Newly failing (2):
Newly passing (35):
Steady-state failures are non-blocking; regressions vs the cached main baseline (lower total pass count, or any PASS → non-PASS transition) fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
…-8008af # Conflicts: # docs/adr/README.md
The four POSIX symlink seed tests skip on win32 (file symlinks need elevation on CI runners), leaving the Windows branch of the guard unverified. Add a win32-only case that seeds a directory junction (a reparse point that needs no elevation) and asserts the same rejection and no-leak contract, exercising the FileGetAttr + faSymLink path on the windows-latest CI runners. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GocciaSandboxRunnerhost seed imports (--seedand--seed-configfrom) read files throughTFileStream, which dereferences symlinks. A symlink inside a seeded directory — or named directly as a seed — copied bytes from outside the requested seed root into the sandbox VFS, contrary to the sandbox-first, reduced-attack-surface posture inVISION.md. The VFS itself deliberately does not support symlinks.HostPathIsSymlinkhelper to the sharedFileUtilsunit (POSIXlstat+S_ISLNK; Windows reparse points / junctions viafaSymLink) and rejects host symlinks at two seams inGocciaSandboxRunner.dpr: the seed leaf (SeedHostPath) and every descendant of a seeded directory (ImportDirectoryContents). On encounter it fails closed — aborts with exit code 1 and an error naming the path:Seed path is a symlink (not supported): <path>.lstatfollows a final symlink when the path ends in/(so--seed=linkdir/is rejected just like--seed=linkdir). This bypass was caught by the pre-handoff/code-reviewpass and is covered by a dedicated regression test./var→/private/var, so rejecting them would refuse ordinary temp paths); and full real-path containment.FileGetAttr+faSymLink(reparse points / junctions). File-symlink tests need elevation onwindows-latest, so they are POSIX-guarded; a separatewin32-only test seeds a directory junction (a reparse point that needs no elevation) and asserts the same rejection, so the Windows branch is exercised by thecliCI job onwindows-latest. macOS validated empirically; Linux uses the same POSIX path.Closes #786
Testing
Details:
./build.pas sandboxrunner+ reproduced all four leak vectors → each rejects with exit 1, names the path, and leaks nothing: nested symlink in a seeded dir, direct symlink as--seed,--seed-configfromdir with a symlink, and a trailing slash on a symlinked dir. The positive case (normal dir/file seed, including a trailing slash) still imports.bun run scripts/test-cli-apps.ts— all pass, including 5 newGocciaSandboxRunnersymlink/junction regressions (4 POSIX-guarded + 1win32-only junction; 13 SandboxRunner cases total; existing seed tests unchanged).origin/main:GocciaTestRunner testsand--mode=bytecodeboth 10,821 passing (100%, no failed tests)../format.pas --checkclean; pre-commit hooks (markdownlint, doc links/symbols, format) green.Reviewer notes
origin/main. This ADR was renumbered 0070 → 0071 because fix(engine): deduplicate string-to-number across runtime and compiler #792 landed0070-shared-string-to-number.mdfirst; thedocs/adr/README.mdindex and the ADR heading were updated to match.ZonedDateTime/hoursInDayDST deltas before the merge; these were baseline drift from fix(engine): deduplicate string-to-number across runtime and compiler #792's string-to-number engine change (which the pre-merge branch lacked) and are causally unrelated to this diff (no Temporal/Intl unit importsFileUtils; the change touches zero engine code). The merge brings fix(engine): deduplicate string-to-number across runtime and compiler #792 in.No
tests/*.jswas added: this is a CLI host-app security guard with no JS-language API surface, sodocs/testing.mdroutes coverage toscripts/test-cli-apps.tsalongside the existing sandbox seed tests.