Repository navigation
HIGH security: confine the file IPC to the workspace when symlinks are involved - #24
Merged
Merged
Conversation
A symlink inside the selected workspace let the file IPC read and overwrite files outside it. `<workspace>/notes.txt -> ../elsewhere/target` reads as an ordinary in-workspace relative name, and the read/write/list call then reached the external target. Root cause: the shared containment guard was lexical only. `resolve`, `normalize` and `relative` are string math and know nothing about symlinks, while the `readFile`/`writeFile`/`readdir` that runs afterwards is resolved by the kernel, which does follow them. Every consumer of the guard was affected — the text read, the binary read, the write, the directory listing, and both git path operations — because they all share it. The guard becomes an async `safeResolve` that all of them call, in three layers: 1. canonicalize the workspace root and the target with `realpath` and re-check containment on the canonical paths. The not-yet-existing tail of a path is re-attached to its deepest existing ancestor, so creating a new file in a real workspace directory still works. 2. reject any path component that is a symlink, via an `lstat` walk. Not redundant with (1): `realpath` throws ENOENT on a dangling symlink, so canonicalization alone treats it as a name to be created and the write follows the link and creates the file at its external target. 3. open reads and writes with `O_NOFOLLOW` (plus `O_NONBLOCK`, so a FIFO cannot park the open and with it the IPC call), so containment does not rest on the path still meaning the same thing between the check and the open. Those flags are POSIX-only, which is why (2) is a check in its own right and not merely a pre-filter. Layer 3 is why the git diff's working-side read had to change shape rather than just gain a call to the guard. It was a plain `stat` + `readFile` on the cleared path, which is a second and third resolution of a name the kernel is free to look up differently — anything that swaps that final component for a symlink after the check is what the read actually gets, and the external file then travels to the renderer as diff text. It now opens through the shared `openForRead` and stats through the returned handle, like every other read of a confined path, so the flags are stated in exactly one place. The boundary is the WORKSPACE, not symlinks. A link whose target exists and is itself inside the workspace is followed, and what comes back is the canonical path of that target: it reaches nothing the caller could not already reach by the target's real name, and refusing it would make an ordinary `node_modules` or monorepo checkout unbrowsable for no gain. What is refused is a link that leaves the workspace, and a dangling link, whose target does not exist yet and so cannot be shown to land inside it. Both directions of that are pinned by tests, so the policy cannot be flipped by accident later. The lexical form is no longer exported — a caller reaching for it would reintroduce exactly this bug.
9 of 13 tasks
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.
What & why
The file IPC that backs the in-app editor confined paths to the selected
workspace with a purely lexical check (resolve + normalize + relative), and
then let Node open the result. A symlink placed inside the workspace but
pointing outside it — the kind of thing an agent-generated repo or a cloned
repo routinely contains — is lexically in-bounds, so the guard passed it and the
open followed the link to its external target. That allowed reading any file the
user can read and overwriting any file the user can write, from inside the
sandbox.
getDiff's working-tree read had the same exposure with an additionalfinal-component TOCTOU window.
The fix canonicalizes the workspace root and the target with
realpath,re-checks containment on the canonical paths, rejects any symlink component that
survives canonicalization (this is what catches a dangling link, which
realpathcannot resolve), and opens withO_NOFOLLOW. Reads are centralizedthrough one
openForReadhelper so the flags cannot drift between sinks, andgetDiffnow stats through the open handle rather than re-resolving the path.In-workspace symlinks whose target stays in-workspace are still followed (so a
node_modulessymlink remains browsable); only out-of-workspace targets anddangling links are refused.
Type of change
Evidence
A self-contained PoC driver builds a throwaway workspace, loads the real
src/main/fs.ts+src/main/git.ts, runs 7 symlink-escape attacks plus 3controls, and cleans up — no real external file is ever touched.
Before
After
How I tested it
npm run typecheck— 0 errors (node + web)npm run test:focused— 591 of 591 (9 new containment tests, incl. adeterministic check-to-open TOCTOU test for the
getDiffread that shadowsgitonPATHto swap the final component mid-call)npm run build— succeedsDiscord (optional)
Discord:
Checklist
npm run typecheckpasses.npm run test:focusedpasses.npm run buildsucceeds.commented-out code, or unrelated formatting churn in it.
DESIGN.md/tokens.ts— no ad-hoc colors,spacing, or fonts. (No UI change.)