fix(core): block symlink-based path escape in studio-api isSafePath#1397
Conversation
path.resolve() collapses ./.. but does not dereference symlinks, so a symlink living inside the project dir but pointing outside it (e.g. project/link -> /etc) passed the prefix check, letting a downstream read/write/stat follow it to a file outside the project root. The `..` traversal case was already blocked; symlink traversal was the gap. Canonicalize both base and target with realpathSync before comparing. The target may not exist yet (new-file writes), so canonicalize the deepest existing ancestor and re-attach the trailing not-yet-existing segments, which cannot be symlinks at check time. Fail closed if base is unresolvable. Adds safePath.test.ts covering: in-base allow, not-yet-existing write target, `..` escape, existing-file-through-symlink escape, write-target under a symlinked parent, file-symlink escape, in-base symlink allow, symlinked-base canonicalization, and base-missing fail-closed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
miguel-heygen
left a comment
There was a problem hiding this comment.
Missed call site — same vulnerable pattern at render.ts:83
Excellent fix and thorough test suite. The isSafePath implementation and the 10 test cases are sound. One blocker:
blocker — packages/core/src/studio-api/routes/render.ts:83 still uses the old vulnerable startsWith pattern:
const resolved = resolve(project.dir, body.composition);
if (!resolved.startsWith(resolve(project.dir) + sep)) { …body.composition is attacker-controlled (deserialized from c.req.json()). This is the same symlink-escape class this PR closes — resolve() doesn't dereference symlinks, so a project carrying body.composition = "link/secret.hf" where link → /etc still passes this check and the path is forwarded downstream. The PR description scopes the fix to routes/files.ts and routes/preview.ts, but this third call site was missed (Rule 2: every site that satisfies the same contract must be fixed).
Fix: replace with isSafePath(project.dir, resolved):
const resolved = resolve(project.dir, body.composition);
if (!isSafePath(project.dir, resolved)) {
return c.json({ error: "composition path must be within the project directory" }, 400);
}And import isSafePath in render.ts.
Strengths:
safePath.ts— clean incremental ancestor walk with correcttrailing.reverse()to reassemble the final path. Fail-closed on unresolvable base is the right default.safePath.test.ts— covers the TOCTOU-adjacent cases (write target under symlinked parent, file symlink to outside, in-base symlink allowed, macOS/var → /private/varcanonicalization). These are exactly the cases that break naive implementations.
Note: CI is still running (Test, CLI smoke, regression-shards pending). Posting now because the render.ts finding is a blocker-class security issue that can be addressed in parallel.
Verdict: REQUEST CHANGES
Reasoning: Same vulnerability class present at render.ts:83; all other changes are correct and the implementation is solid — this is a one-line fix away from approval.
— Via
Review on #1397 found a third call site with the same vulnerable startsWith pattern. Apply Rule 2: fix every site sharing the contract (gate an attacker-influenced path before a symlink-following fs op). - studio-api routes/render.ts: body.composition (from c.req.json()) was checked with `resolved.startsWith(resolve(project.dir) + sep)`, which doesn't dereference symlinks — an in-project symlink to an external target escaped the project root. Now uses isSafePath(). - cli commands/play.ts: the `/composition/*` server route used `filePath.startsWith(project.dir)` with no trailing-separator guard, so both a sibling dir sharing the prefix (`<dir>-evil`) and symlink escapes passed. Now uses isSafePath() via @hyperframes/core/studio-api (the same lazy-import pattern commands/validate.ts already uses). Tests: render.test.ts gains a "composition path safety" block (in-base allow, `..` reject, in-project-symlink-to-outside reject, in-project symlink staying inside allow). The shared render test adapter now points at a real dir since isSafePath fails closed on an unresolvable base (production project dirs always exist on disk). Not in this change: compiler/htmlBundler.ts has the same class at two sites (safePath helper + inline CSS @import check), but the compiler sits below studio-api in the dependency graph and can't import isSafePath without a backwards edge; that fix needs the helper promoted to a neutral module and is tracked as a follow-up. renderArgs.ts / videoFrameExtractor.ts carry the trailing-sep guard and a local-CLI/engine-internal threat model. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — good catch, confirmed against source and fixed in e7127cd.
I then swept every
Two more sites carry the same class but I deliberately did not fold them into this PR — flagging for your call:
CI was green on the prior commit; re-running now. Ready for re-review. |
…ndler Per review on #1397: extend the symlink-escape fix to the compiler, and remove the duplicated path-safety logic. - Move isSafePath to packages/core/src/safePath.ts (a neutral package-root module). studio-api/helpers/safePath.ts re-exports it for back-compat (keeping walkDir), and it's now exported from the core entrypoint so non-studio-api layers can use it. compiler/ sits below studio-api in the dep graph, so it could not import the helper from its old home without a backwards edge — the promotion removes that constraint. - compiler/htmlBundler.ts: route both containment checks (the safePath helper and the inline CSS @import check) through isSafePath. The bundler reads+inlines these files, so an in-project symlink pointing outside the root would otherwise bake external content into the output. All callers already skip on a null/false result, so nothing is read on rejection. Tests: safePath.test.ts moves with the impl; htmlBundler.test.ts gains a case proving an in-project sub-composition script is inlined while a script reached through an escaping symlink is not (positive control + leak assertion). Deferred (tracked for a dedicated follow-up, see PR thread): the relative()-based isPathInside family (core/compiler/assetPaths, producer/services/fileServer, producer/utils/paths and their callers in the render pipeline) is symlink-blind in the same way, and engine videoFrameExtractor's asset resolver needs a caller-side gate (its http downloads land outside the project root, so a single-root check is wrong). Both are regression-sensitive render-pipeline surfaces that warrant their own focused, well-tested pass. renderArgs.ts is intentionally left: it is filesystem-free by design (injected stat) and its threat model is the user's own --composition CLI arg. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Update — the I also chased the rest of the
One thing worth your eyes: this makes Follow-up stacked PR incoming to route |
…view nits) Addresses Via's non-blocking review notes on #1397: - Wrap every symlinkSync in the new tests with a tryCreateSymlink helper that returns false (and the test early-returns) when creation throws, mirroring the preview.test.ts convention. Non-symlink-privileged Windows runners no longer risk crashing the suite on EPERM. - safePath.ts: `[...trailing].reverse()` instead of mutating `trailing` in place — harmless today (single return) but future-proof against a looping edit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
miguel-heygen
left a comment
There was a problem hiding this comment.
✅ All prior findings addressed
render.ts:83 blocker (mine) — Fixed. body.composition now routed through isSafePath(project.dir, resolved), import added. Four new route tests cover in-base allow, .. reject, in-project symlink escaping → reject, in-project symlink staying inside → allow. resolveProject corrected to return a real tmpdir() so realpathSync doesn't fail closed on /tmp/proj.
Windows tryCreateSymlink convention (Rames #1) — Fixed. All new symlink tests in safePath.test.ts, render.test.ts, and htmlBundler.test.ts use the tryCreateSymlink/early-return pattern.
htmlBundler parallel vuln (Rames #2) — Fixed and done cleanly: isSafePath promoted to packages/core/src/safePath.ts (neutral package-root module), compiler imports it directly without a backwards dep on studio-api. Both the safePath() wrapper and the inline CSS @import check now route through isSafePath. Symlink-escape test added.
trailing.reverse() nit (Rames #3) — Fixed: [...trailing].reverse().
CLI play.ts — Correct sweep of the same HTTP contract. Was also missing the +sep trailing-separator guard. Now uses isSafePath via lazy import (matching commands/validate.ts pattern).
CI — Test failure (projects.test.ts:85) is pre-existing on main (confirmed: same job, same file:line on the main branch run at e2cc134c). The #1400 unblocker is the fix; not caused by this PR.
One nit (not a blocker): htmlBundler.test.ts — security marker mismatch
The test file written to disk is:
writeFileSync(join(external, "secret.js"), `window.__HF_SECRET__="SECRET...AKED";`);But the assertion checks:
expect(bundled).not.toContain("SECRET_MARKER_LEAKED");"SECRET_MARKER_LEAKED" never appears anywhere in the bundle — the string written is "SECRET...AKED". The negative assertion is always trivially true regardless of whether the symlink escape is blocked. The positive control (LOCAL_MARKER_INLINED) is fine. Quick fix: change the file content to use "SECRET_MARKER_LEAKED" as the literal string, or change the assertion to not.toContain("SECRET...AKED").
Strengths:
- Promotion to a package-root module is the right structural call.
compiler/sitting belowstudio-api/in the dep graph made the backwards-import problem real; this resolves it cleanly. - The
isSafePathalgorithm (ancestor-walk + realpath + fail-closed on unresolvable base) is sound. The[...trailing].reverse()defensive copy is good practice. - The intentional-not-fixed scope (producer lexical-symlink feature, engine,
renderArgs.ts) is well-documented with rationale in both the PR body and inline comments.
Verdict: APPROVE
Reasoning: All blockers and nits from prior rounds addressed. Remaining item is a minor test marker mismatch (trivially-true negative assertion) that doesn't affect correctness of the fix — the guard works, the positive control confirms the bundler ran. Happy to see it fixed in #1398 or a fast-follow.
— Via
What
Harden HyperFrames' path-traversal guards against symlink-based directory escape, on the surfaces where that's actually a vulnerability, and remove the duplicated/incomplete containment logic that let it slip through review (#465).
packages/core/src/safePath.ts(new) —isSafePathcanonicalizes withrealpath(the core fix), promoted to a neutral package-root module so every layer can share one implementation.studio-api/helpers/safePath.tsre-exports it for back-compat (keepswalkDir); also exported from the core entrypoint.studio-api/routes/render.ts—body.composition(attacker-controlled JSON) now routed throughisSafePath(raised in review).cli/commands/play.ts— the/composition/*server route now routed throughisSafePath.compiler/htmlBundler.ts— both containment checks (thesafePathhelper + the inline CSS@importcheck) now useisSafePath.studio-api/routes/files.ts+preview.tsget the fix transparently via the shared helper.Supersedes #465 (same vulnerability class for
isSafePath, but no tests and it missed every other call site).Why
path.resolve()collapses./..but does not dereference symlinks. The old checks were string prefix tests (resolved.startsWith(resolve(base) + sep)):../../etc/passwdwas already blocked (resolves outsidebase).project/link -> /etc) was not —resolve(project/link/x)is still lexically underproject/, so the guard passed and a downstreamreadFileSync/statSync/serve/inline followed the link out of the root.The threat is a shared/cloned project carrying a pre-planted symlink: on a surface that serves or bakes file contents, that's an arbitrary-file-read primitive on the host (CWE-59 / CWE-22).
How
isSafePathcanonicalizes both sides withrealpathSyncbefore comparing, walks up to the deepest existing ancestor for not-yet-existing write targets, and fails closed ifbaseis unresolvable. In-project symlinks that stay inside the root are still allowed — only escapes are rejected. Documented residual: TOCTOU between check and fs call (inherent; not claimingO_NOFOLLOW).Scope — why some surfaces are fixed and others intentionally are not
A symlink-escape is a bug only where the path check is the sole boundary (no sandbox) and the surface gives an exfiltration channel. By that test:
Fixed (sole boundary + exfil path, external access never intended):
files/preview/renderand CLIplay— local HTTP servers; a malicious project's own preview HTML canfetch()the symlinked path and POST it out.htmlBundler— inlines content into shareable self-contained HTML, and it already rejected.., so the symlink path was an accidental hole, not a feature.Intentionally NOT changed:
isPathInsidefamily (services/fileServer.ts,services/htmlCompiler.ts,utils/paths.ts, …) — the producer deliberately supports out-of-project assets:toExternalAssetKey/hf-extcopies external files into the render sandbox, andfileServer.tsdocuments "keep this lexical so project symlinks to sibling asset directories behave like preview mode." Realpath-blocking there would break a feature. (Verified: the producer does not importbundleToSingleHtml, so this PR's bundler change cannot affect it.)videoFrameExtractor.resolveProjectRelativeSrc— project-relative<video src>resolution; external assets use the separate absolute-path channel. Low severity (the rendered output stays with the renderer; cloud renders are sandboxed) and the correct fix is a caller-side gate, not a resolver-internal filter. Tracked for a possible stacked follow-up depending on cloud-render isolation.cli/utils/renderArgs.ts— filesystem-free by design (injectedstat); threat model is the user's own--compositionCLI arg.A separate stacked PR will route
preview.ts/render.tsproject-path resolution through the singleresolveProjectPath/resolveWithinProjectchokepoint, so a future route can't reintroduce this by forgetting the check — the structural root cause of why #465 missedrender.ts.Test plan
safePath.test.ts(10 cases) +render.test.ts"composition path safety" (4 cases) +htmlBundler.test.tssymlink-escape case (in-project script inlined, symlinked-external not leaked). Symlink cases fail against the old code.@hyperframes/core(1480 tests) +@hyperframes/cli(744 tests) pass;tsc --noEmit(core + cli + engine),oxlint,oxfmt --checkall clean.🤖 Generated with Claude Code