Repository navigation
fix(zen): confine load targets to the workspace and its dependencies - #1559
Conversation
Absolute paths, package:// URIs, @stdlib paths with absolute segments, and symlinks could resolve to any host file, and parse diagnostics then echoed its contents. resolve() is the one point every load, File(), and Path() passes through, so canonicalize the target there and reject it unless it lies in the workspace, its stdlib, or a resolved package root. Amp-Thread-ID: https://ampcode.com/threads/T-67f94b14-128e-4596-b9ab-2be59ef8d30a Co-authored-by: Amp <amp@ampcode.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let canonical = context.file_provider.canonicalize(&resolved_path)?; | ||
| if !self | ||
| .resolution | ||
| .contains_path(context.file_provider, &canonical) | ||
| { |
There was a problem hiding this comment.
🟥 Retargeted symlink leaks host files
After resolve() caches a safe symlink target, retargeting that symlink outside the workspace leaves containment using the old path. read_file() follows its new target and can return host file contents in diagnostics.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Not changing this. Exploiting it needs the symlink retargeted mid-evaluation, after its first canonicalization. A build evaluates a fixed checkout in one process, and Zen code has no way to create or retarget symlinks. In the long-lived LSP only the local user can change their own symlinks, and they can already read those files. Re-checking on every read would add cost without closing a reachable path.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 17f850b. Configure here.
| .resolution | ||
| .values() | ||
| .any(|map| map.package_for_file(path).is_some()) | ||
| } |
There was a problem hiding this comment.
Empty root rejects every load
Medium Severity
contains_path treats an empty workspace root as contained-in-nothing, then the new resolve() gate rejects every spec. ResolutionResult::empty() uses that empty root, including the LSP fallback when dependency resolution fails, so prelude @stdlib loads and other resolve() calls now fail instead of degrading.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 17f850b. Configure here.
There was a problem hiding this comment.
Keeping this. ResolutionResult::empty() has no package scopes, so relative, @stdlib, and package-URL loads already failed in that fallback with "current file not in any package". Only absolute paths and package:// URIs got through. With no workspace to contain them, rejecting those too is the intended behavior.
The frozen resolution already lists the canonical stdlib root, captured right after pcb relinks .pcb/stdlib to the toolchain, so the separate stdlib prefix check was redundant. Amp-Thread-ID: https://ampcode.com/threads/T-67f94b14-128e-4596-b9ab-2be59ef8d30a Co-authored-by: Amp <amp@ampcode.com>
Why
load(),File(), andPath()could resolve to files anywhere on the host through absolute paths,@stdlib//abs/path,package://URIs with.., or symlinks. When such a file failed to parse, the diagnostic echoed its contents.Changes
resolve()now canonicalizes every resolved target and rejects it unless it lies inside the workspace, its stdlib, or a resolved dependency (including[patch]path deps).Validation
load_cannot_read_files_outside_workspacecovers absolute paths,@stdlib//abs,package://traversal, and symlinks, and checks that in-workspace absolute loads still work. It fails without the fix.@stdlib/..., and an out-of-workspace[patch]path dependency still load.cargo nextest run --workspacepasses; clippy is clean.Note
High Risk
Security fix in core file-resolution for Zen evaluation; behavior change for any project that relied on out-of-workspace paths, but intended tightening with broad test coverage.
Overview
Closes a path where
load(),File(), andPath()could read arbitrary host files via absolute paths,@stdlibabuse,package://traversal, or symlinks—and where parse failures could leak file contents in diagnostics.resolve()now canonicalizes every resolved target and rejects it unless it lies under the workspace root or a resolved package (stdlib and deps included), via newcontains_path. The check runs before the existence/read path so rejected specs do not confirm whether a file exists.Docs and changelog describe the rule; a Unix integration test covers the main escape vectors and confirms in-workspace absolute loads still work.
Reviewed by Cursor Bugbot for commit 6cca407. Bugbot is set up for automated code reviews on this repo. Configure here.