Skip to content

fix(dashboard): prevent symlink-following file disclosure when indexing untrusted repos#476

Open
rafaelfiguereod-stack wants to merge 1 commit into
Egonex-AI:mainfrom
rafaelfiguereod-stack:security/dashboard-symlink-containment
Open

fix(dashboard): prevent symlink-following file disclosure when indexing untrusted repos#476
rafaelfiguereod-stack wants to merge 1 commit into
Egonex-AI:mainfrom
rafaelfiguereod-stack:security/dashboard-symlink-containment

Conversation

@rafaelfiguereod-stack

Copy link
Copy Markdown

Summary

Per the security email thread with @Lum1104 (who kindly invited a PR): the dashboard's /file-content.json reader follows symlinks, so running /understand on an untrusted repo that contains a committed symlink (e.g. src/config.txt -> /home/user/.ssh/id_rsa) can disclose out-of-root files — SSH keys, a sibling project's .env, etc. — in the code viewer.

Root cause

readSourceFile() (packages/dashboard/vite.config.ts) validates the requested path only textually (rejects ..//absolute, checks the graph-derived allowlist) and then calls fs.statSync / fs.readFileSync, both of which follow symlinks. A symlink committed in the indexed repo is textually in-root, is listed by git ls-files, passes statSync().isFile() during indexing (scan-project.mjs), and so becomes an allowlisted graph node — clicking it returns the symlink target's contents.

Fix (2 files)

  • vite.config.tsrealpathSync-resolve the file and re-confirm it stays inside the realpath of the project root before stat/read. Load-bearing fix: closes the read even if a symlink node is already in the allowlist.
  • scan-project.mjs — use lstat instead of stat so symlinks are not followed at index time and never enter the graph/allowlist (matching the recursive-walk fallback / extract-domain-context.py, which already skip symlinks).

Minimal, no new deps, no version bump (left for your release flow). Happy to adjust.

🤖 Generated with Claude Code

…ng untrusted repos

The dashboard /file-content.json reader (packages/dashboard/vite.config.ts
readSourceFile) validated the requested path only textually (rejects ../absolute,
checks the graph-derived allowlist) and then called fs.statSync / fs.readFileSync,
both of which follow symlinks. A symlink committed in an indexed repo
(e.g. src/config.txt -> /home/user/.ssh/id_rsa) is textually in-root, is enumerated
by `git ls-files`, passes statSync().isFile() during indexing, and so becomes an
allowlisted graph node. Opening that file node in the dashboard then returned the
symlink target's contents (SSH keys, a sibling project's .env, etc.) — out-of-root
disclosure when running /understand on an untrusted repo.

- vite.config.ts: realpathSync-resolve absoluteFile and re-confirm it stays inside
  the realpath of the project root before stat/read (load-bearing fix — closes the
  read even if a symlink node is already in the allowlist).
- scan-project.mjs: lstat instead of stat so symlinks are not followed at index
  time and never enter the graph/allowlist, matching the recursive-walk fallback
  which already skips symlinks.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9ae65fadf0

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

Comment on lines +166 to +170
if (
!realRelative ||
realRelative === ".." ||
realRelative.startsWith(`..${path.sep}`) ||
path.isAbsolute(realRelative)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject symlink nodes in the dashboard reader

When users open a graph generated before the scanner change (or otherwise containing an allowlisted symlink node), a link such as src/config.txt -> .env still passes this guard because the resolved target is inside realRoot; the later statSync/readFileSync(absoluteFile) then follows the symlink and serves an ignored file that was never itself graph-allowlisted. Since this function is the load-bearing protection for already-allowlisted symlinks, it should reject symlink paths (or require the resolved target path to be allowlisted) rather than only checking that the real path stays under the project root.

Useful? React with 👍 / 👎.

@rafaelfiguereod-stack

Copy link
Copy Markdown
Author

Hi @Lum1104 — this is the PR for the symlink-following file-disclosure we discussed over email; thanks for inviting it.

It's intentionally minimal (2 files):

  • packages/dashboard/vite.config.tsrealpathSync-resolve the requested file and re-confirm it stays inside the project root before statSync/readFileSync, so an in-root symlink can't be followed out of the project.
  • skills/understand/scan-project.mjslstat (not stat) at index time so symlinks aren't followed and never enter the graph/allowlist, matching the recursive-walk fallback and extract-domain-context.py, which already skip them.

No new deps and no version bump (left for your release flow). Happy to adjust naming, add a regression test, or split it however you prefer.

@tirth8205

Copy link
Copy Markdown
Contributor

Reviewed for the stated focus: does this actually prevent symlink-based file disclosure via /file-content.json, and are there bypasses? Short answer: yes, the hole is genuinely closed, and the parent-directory-symlink case (the one a naive lstat-on-final-component fix would miss) is handled correctly. A couple of minor notes below.

What's correct

  • vite.config.ts (readSourceFile): realpathSync(absoluteFile) resolves every symlink component, so it covers both src/config.txt -> ~/.ssh/id_rsa and the trickier src/sub -> /outside + src/sub/file.txt. Both realFile and realRoot go through realpathSync, so the comparison is canonical-vs-canonical — no case-insensitivity (macOS/Windows) or root-symlink mismatch bypass. The path.relative(realRoot, realFile) guard rejects .., ..${sep}-prefixed, and absolute results (confirmed path.relative returns ../… forms for escapes and "" for equal paths). This is the load-bearing runtime guard and it holds.
  • scan-project.mjs: lstatSync + isSymbolicLink() skip is the right index-time fix. git ls-files does list tracked symlinks (mode 120000), so the previous statSync().isFile() would have followed a symlink-to-file and admitted it as an allowlisted node. This now matches the recursive walker, which already skips symlinks. Good complementary defense-in-depth — the two fixes are independent (runtime guard still closes the read even if a stale/hand-crafted graph contains a symlink node).
  • /file-content.json is the only endpoint that reads arbitrary repo files, so this covers the full attack surface for the reader.

Minor

  1. After the realpath check, statSync/readFileSync operate on absoluteFile, not the validated realFile. In the static committed-symlink model these are equivalent, but it leaves a narrow TOCTOU window between realpathSync(absoluteFile) and readFileSync(absoluteFile) where a component could be swapped to a symlink. Reading realFile would make the path you validated the path you actually read and close that window. Low severity (an active FS-racing local attacker is outside the stated threat model), but it's a one-line tightening.

  2. (nit) realpathSync(projectRoot) is recomputed per request; it could be hoisted since the root is stable per process. Negligible, dev-server only.

Note: no tests were added. A small regression test (symlink → out-of-root target returns the containment rejection; symlink node never enters the allowlist after scan) would lock this in, since the whole point is preventing a silent regression to symlink-following.

Overall: accurate root-cause analysis, the fix actually closes the disclosure, and it correctly handles the parent-symlink case that's easy to miss. I'd consider switching the read to realFile and adding a test, but neither blocks the security objective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants