fix(studio-server): block dangling symlink upload escapes - #3661
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed 12dc6fca9a141c24c39005c43603a75720d8db71. The shared guard now rejects unresolved existing symlinks (packages/core/src/safePath.ts:60), and uploads validate the collision-suffixed destination (packages/studio-server/src/routes/files.ts:2224). Current CI is green.
Blocker — sibling rename writes still escape the project: packages/studio-server/src/routes/files.ts:582–595 (updateReferences, invoked at line 3031) reads and writes every matching text-file entry without checking containment. A pre-existing project/external.html -> outside/victim.html is included by walkFiles. Renaming inside.txt to moved.txt then rewrites inside.txt references in the outside file, even though the rename's own source and destination pass the new guard. I reproduced this with the exact-head helper code: outside bytes changed from reference: inside.txt to reference: moved.txt. This is a pre-existing sibling of the containment contract being hardened here. Resolve/check every scanned file before reading or writing it and add a PATCH regression asserting outside bytes remain unchanged.
Verdict: REQUEST CHANGES
Reasoning: The dangling-upload fix passes the symlink probes, but the sibling rename reference-update path still permits a reproducible outside-project write.
— Magi
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed 28830b3c2e2d2a1db3046c3c73a3d858c3ef130b; my outside-write blocker is resolved.
packages/studio-server/src/routes/files.ts:587 validates every scanned reference file before reading or writing. The new real-PATCH regression checks outside bytes, internal reference updates, moved-file contents, and the update count. I independently reran the exact-head helper probe: the external file stayed unchanged, the internal reference updated once, and all nine previous path cases still passed. This local probe is helper-level; the full HTTP regression was inspected, not rerun locally.
No additional findings in the two-file follow-up. CI has no failing checks at review time, but rerun jobs are still in progress; this approval does not claim CI completion.
— Magi
Verdict: APPROVE
Reasoning: The guard closes the reproduced sibling containment escape and the regression preserves intended in-project rename behavior. Normal required-check gates still apply before merge.
An upload could follow a pre-existing dangling symlink inside a project and create its missing target outside the project. Reject unresolved existing filesystem entries instead of treating them as ordinary missing path segments, and validate the final upload destination after collision renaming.
Builds on #1483 by @calcarazgre646 with current file-route containment coverage, including multipart uploads, binary write preconditions, outside-byte preservation, and explicit Windows skips when symlink creation is unavailable. The new regressions cover both the original filename and the collision-suffixed filename; existing internal symlinks and nested uploads still work.
Validation: reproduced an outside write on current main before the fix; the collision-suffix regression also failed before the final-destination check. All 484 studio-server tests, 14 core path tests, server typecheck, and changed-file lint/format pass. This fixes pre-existing dangling links; filesystem changes racing a check and subsequent operation remain outside the shared path helper's existing guarantee.