Security: Path safety check is vulnerable to symlink-based directory escape

[tuanaiseo]

Problem

isSafePath only verifies string prefix containment after resolve(). This does not account for symlinks inside the project directory that point outside the base path. Downstream file reads/writes that rely on this helper can be tricked into accessing files outside the intended project root via symlink traversal.

Severity: medium
File: packages/core/src/studio-api/helpers/safePath.ts

Solution

Use realpath (or realpathSync) on both base and target before comparison, and reject targets whose real path is outside the base real path. For write paths, also open files with flags that reduce symlink-follow risks where possible.

Changes

• packages/core/src/studio-api/helpers/safePath.ts (modified)

Testing

• Existing tests pass
• Manual review completed
• No new warnings/errors introduced

[miguel-heygen]

Left one note on coverage.

[miguel-heygen]

This closes a real escape, but I would still want one focused regression test here. isSafePath() is the guard for both the Studio file routes and preview asset serving, so the PR should prove a symlink inside the project cannot read or write outside the project root, while a normal in-project create still works. Otherwise this is easy to reopen later.

[miguel-heygen]

This fixes a real issue, but I want one targeted regression test before approval. isSafePath() guards the Studio file routes and preview asset serving, and this is easy to reopen later without coverage.

[vanceingalls]

First review at d1c33285. Existing review: @miguel-heygen REQUEST_CHANGES (2026-04-23) — pending a regression test.

Audited

• packages/core/src/studio-api/helpers/safePath.ts:1-30 (the realpath check)
• Miguel's prior review (asks for a targeted regression test)

Strengths

• The bug is real. isSafePath currently does string-prefix containment after resolve() only. A symlink inside the project dir pointing outside the base (e.g. <project>/escape -> /etc/) bypasses the check — resolve() doesn't follow symlinks, so the resolved string still has the project prefix even though the OS will follow the symlink at I/O time.
• realpath is the canonical fix. Following the symlinks before comparison is what isSafePath should always have done.
• The PR keeps the prior resolve() containment check as a fast path before falling through to the slower realpath call. Good — realpath has filesystem overhead, and most legitimate paths are non-symlinked.

Important — Miguel's regression test ask is correct and still unaddressed

Miguel's review (2026-04-23) asked for one targeted regression test:

"I want one targeted regression test before approval. isSafePath() guards the Studio file routes and preview asset serving, and this is easy to reopen later without coverage."

No commits since 2026-04-23 (20 days). Concretely the test should:

1. Create a temp dir as the "base"
2. Create a symlink inside the temp dir pointing outside (e.g. to /tmp or to /etc/hostname)
3. Assert isSafePath(base, symlinkPath) returns false (the new behavior) — the old behavior returned true
4. Also assert the non-symlink negative case (isSafePath(base, "../../etc/passwd")) still returns false
5. And the positive case (isSafePath(base, "subdir/file.txt")) returns true

A 30-line test file (safePath.test.ts) would unblock this PR.

Important — Rule 2: every caller of isSafePath benefits, but verify the contract holds

grep -rn "isSafePath" packages/core/src/ should surface all consumers. Each should:

• Pass paths AFTER any user-controlled URL decode/transform
• Trust the return value as authoritative (no inline .. checks downstream — those become dead code, or worse, mask the helper's failure)

#621 (already approved) adds an inline .. check at one call site. After this PR lands, that inline check becomes redundant — and #620's double-decode fix interacts with isSafePath the same way. Worth a follow-up to centralize all path-safety logic in safePath.ts and delete inline guards across the codebase. James flagged the same idea on #621.

Nit

• Consider realpathSync.native (vs realpathSync) for the perf win on hot paths — Node's native variant uses the libuv syscall directly without the JS-level normalization. Studio file-serving is hot enough that this might be worth the micro-opt.
• Error handling: if realpathSync throws (e.g. file doesn't exist yet for a write path), the current code reacts how? The diff doesn't show — verify the failure mode for "path that doesn't exist yet but is intended to be written" doesn't accidentally permit-by-default.

Verdict

Verdict: COMMENT
Reasoning: Fix is the right shape (symlink-escape is a real isSafePath bypass). Blocker is Miguel's regression-test ask from 2026-04-23 (20 days unaddressed). External-contributor PR — Vance check before merging once test lands.

— Vai

[jrusso1020]

Closing in favor of #1397, which implements the same fix correctly and with test coverage.

The vulnerability you identified is real: path.resolve() doesn't dereference symlinks, so a symlink inside the project dir pointing outside it bypasses the prefix check. #1397 takes the same realpath-both-sides approach, plus:

• Adds a dedicated safePath.test.ts (10 cases, including the symlink-escape cases that fail against the current implementation) — this PR shipped no tests for a security-critical helper.
• Drops the unimplemented claim in this PR's description about opening write paths with symlink-resistant flags (O_NOFOLLOW); the diff here only touched isSafePath, not the readFileSync/writeFileSync call sites. That hardening is a separate change and out of scope.
• Documents the residual TOCTOU race (a symlink swapped in between the check and the fs call) that neither approach can close on its own.

Thanks for flagging the issue.