Skip to content

fix(security): add path traversal protection to render file serving#621

Open
hobostay wants to merge 1 commit into
heygen-com:mainfrom
hobostay:fix/path-traversal-render-file
Open

fix(security): add path traversal protection to render file serving#621
hobostay wants to merge 1 commit into
heygen-com:mainfrom
hobostay:fix/path-traversal-render-file

Conversation

@hobostay
Copy link
Copy Markdown
Contributor

@hobostay hobostay commented May 5, 2026

Summary

  • Add path traversal protection to the /projects/:id/renders/file/* endpoint
  • The filename extracted from the URL was joined with rendersDir without validation, allowing ../../ sequences to serve arbitrary files

Details

Affected file: packages/core/src/studio-api/routes/render.ts (lines 180-187)

A request to /projects/1/renders/file/../../etc/passwd would resolve to a file outside the renders directory.

Test plan

  • Verify normal render file downloads still work
  • Verify paths containing .. are rejected with 403

🤖 Generated with Claude Code

The /projects/:id/renders/file/* endpoint served files by joining
rendersDir with the URL filename without validating the resolved
path stays within rendersDir. A request to
/renders/file/../../etc/passwd would serve arbitrary files.

Add a ".." check on the filename and verify the resolved path
starts with the renders directory.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

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

Verified — render.ts:186 currently does join(rendersDir, filename) with zero containment check, and existsSync will happily resolve ../../etc/passwd. The two-layer defense (literal .. rejection + startsWith(resolve(rendersDir)) containment) closes it. Worth noting this matches the pattern used in files.ts (isSafePath) — consider extracting to a shared helper in a follow-up, but not blocking. LGTM.

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

Additive review at 6731abfa@jrusso1020 already verified and approved.

Audited

  • packages/core/src/studio-api/routes/render.ts:180-187 (the two-layer defense)
  • Cross-checked against isSafePath in safePath.ts

Strength

James covered the bug — join(rendersDir, filename) with zero containment check + existsSync resolving ../../etc/passwd was a path traversal in the literal sense. The two-layer defense (literal .. rejection + startsWith(resolve(rendersDir)) containment) closes it correctly.

Important — Rule 2 contract audit on the isSafePath ecosystem

Three relevant PRs are open against the same security surface:

  1. This PR (#621) — adds inline path-traversal check at one new call site
  2. #620 — fixes double-decode in files.ts (already approved)
  3. #465 — adds realpath symlink-escape check to isSafePath itself

Each PR fixes a different attack vector at a different call site against the same safePath.ts helper module. The right end-state is:

  • safePath.ts:isSafePath() enforces ALL three guarantees (no .., no double-decode, no symlink escape) in one place
  • All *.ts route handlers use isSafePath() — not inline checks like this PR's :182-187

This PR's inline approach is fine for landing a fix today, but it forks the contract — now there are two definitions of "safe path" (one in safePath.ts, one inline here). Worth a follow-up after all three land: refactor render.ts to import isSafePath and delete the inline guards. Reference: James called the same out — "consider extracting to a shared helper in a follow-up, but not blocking."

Important — auth boundary check?

gh api repos/heygen-com/hyperframes/contents/packages/core/src/studio-api/routes/render.ts to confirm: is the /projects/:id/renders/file/* endpoint behind an auth check? If it's unauthenticated and serves arbitrary user-uploaded project files, the path-traversal fix is the right starting point but the next security gap is that any user can list/fetch any other user's renders. Hopefully out of scope (this is the studio dev server, presumably localhost-only), but worth confirming.

Verdict

Verdict: COMMENT
Reasoning: Path-traversal fix is correct (James verified). Three open PRs (#621, #620, #465) cover related security gaps against the same helper module — recommend a follow-up factoring to centralize all three guarantees in safePath.ts and delete inline checks. External-contributor PR — Vance check before merging.

— Vai

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

hobostay's path-traversal protection on render file serving. @jrusso1020 already APPROVED on this SHA. CLEAN merge state. Vance authorized the stamp.

Verdict: APPROVE
Reasoning: James verified, security-hardening, MergeStateStatus CLEAN.

— Vai

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

Labels

None yet

3 participants