Skip to content

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

Open
tuanaiseo wants to merge 1 commit intoheygen-com:mainfrom
tuanaiseo:contribai/fix/security/path-safety-check-is-vulnerable-to-symli
Open

Security: Path safety check is vulnerable to symlink-based directory escape#465
tuanaiseo wants to merge 1 commit intoheygen-com:mainfrom
tuanaiseo:contribai/fix/security/path-safety-check-is-vulnerable-to-symli

Conversation

@tuanaiseo
Copy link
Copy Markdown

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

`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.

Affected files: safePath.ts

Signed-off-by: tuanaiseo <221258316+tuanaiseo@users.noreply.github.com>
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

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

Left one note on coverage.

import { readdirSync, realpathSync } from "node:fs";

/** Reject paths that escape the project directory. */
export function isSafePath(base: string, resolved: string): boolean {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

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

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.

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