Skip to content

fix(studio): claim upload filenames exclusively - #3786

Merged
jrusso1020 merged 2 commits into
mainfrom
fix/security-upload-collisions
Sep 8, 2026
Merged

fix(studio): claim upload filenames exclusively#3786
jrusso1020 merged 2 commits into
mainfrom
fix/security-upload-collisions

Conversation

@jrusso1020

@jrusso1020 jrusso1020 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Problem and change

Fixes CodeQL #694. Studio chooses an unused upload filename before awaiting the uploaded bytes, so another upload can create that path before the write and have its contents overwritten.

Open the selected filename with exclusive wx creation, then write through and close that descriptor. On EEXIST, retry the existing numbered suffix convention up to the same 10000 boundary. Revalidate containment before each attempt, including after the asynchronous read. Only a successfully written filename is returned or passed to waveform generation.

Preserves extension/dotfile naming, ordinary collision choices, media validation, size limits, skipped/invalid response shapes, and waveform triggering. Unsafe symlink paths remain skipped. This protects destination leaves; existing trusted-project ancestor assumptions and same-inode concurrent mutation remain unchanged. No workflow changes.

Validation

  • All 16 path-safety tests pass; the four new race cases fail against the original implementation.
  • Regression cases cover a competing upload during body read, multiple suffix collisions with dotfiles/compound extensions, and a planted outside symlink.
  • Full Studio-server suite: 537 tests passed across all 38 suites.
  • Studio-server typecheck/build and changed-file lint/format pass.

@miguel-heygen miguel-heygen left a comment

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.

Exact-head security/collision review at 68735f002d1db2534360e4ffc34d8cd67eb6abb6.

  • packages/studio-server/src/routes/files.ts:2201-2226 preserves the existing naming contract and checks both the initial and preselected suffix destination. Dotfiles keep no extension; compound extensions remain attached after the suffix.
  • :2235-2255 makes writeFileSync(..., { flag: "wx" }) the only publication operation. Every EEXIST advances exactly once and re-enters isSafePath before the next attempt; (9999) remains the last attempted suffix under the existing <10000 cap. Non-collision write failures still propagate.
  • The symlink states stay distinct: existing or newly planted outside/dangling leaves fail containment and are skipped; regular/internal collisions reach wx, cannot be followed or overwritten, and advance to the next name. The remaining parent/ancestor swap window is the disclosed trusted-ancestor boundary, not widened by this patch.
  • :2257-2260 records and waveforms only the filename whose exclusive write succeeded. Validation remains before publication and suffixing never changes the extension it validates.
  • All four new write-time race cases pass in the focused local run. A separate direct probe against this head’s isSafePath source confirms live outside and dangling leaves are rejected; the borrowed prebuilt dependency environment was stale for two older assertions, so exact-head CI remains authoritative for the complete 16-case suite.

No blocking findings. Build, Typecheck, runtime contract, lint, format, Studio load/timeline, and regression are green; required Test, Windows, and JavaScript CodeQL were still running at review time and remain merge gates.

Verdict: APPROVE
Reasoning: Exclusive leaf creation closes the check/use overwrite race while preserving collision names, suffix bounds, validation, response, skip, and waveform semantics; each retry revalidates the only pathname property the patch claims to protect.

— Magi

Comment thread packages/studio-server/src/routes/files.ts Fixed
Comment thread packages/studio-server/src/routes/files.ts Dismissed

@miguel-heygen miguel-heygen left a comment

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.

The descriptor lifecycle is correct: fd is block-scoped to a successful openSync, writeFileSync(fd, buffer) is enclosed by finally, closeSync(fd) runs before written can become true, and non-EEXIST failures still propagate. The collision path is also unchanged: only EEXIST advances the suffix once, the containment check still precedes each attempt, and the existing bound remains intact.

One blocker remains: the exact-head CodeQL run completed failure and opened alert #900, again js/file-system-race, now at the new openSync(finalPath, "wx") call. Alert #899 is fixed, but this is a fresh instance of the same query at the same operation. Since this revision explicitly intends to clear the alert without suppression or dismissal, it has not yet achieved that acceptance condition and the native security gate remains red. Please restructure the check/open flow so the query closes while retaining the unsafe-link skip behavior, or explicitly revisit the no-dismissal constraint with a scoped audit of the remaining finding.

Separately, the required Typecheck failure is the known generated-runtime build-order race (runtime-inline.ts was read mid-write, TS1002: Unterminated string literal), not caused by these two files; it needs a retry but is not a code-review blocker here.

Verdict: REQUEST_CHANGES
Reasoning: Descriptor closure and retry behavior are sound, but exact-head CodeQL still raises the same race class as a new blocking alert, so the security fix does not yet clear its stated gate.

— Magi

@miguel-heygen miguel-heygen left a comment

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.

Scoped audit of CodeQL #900 at unchanged head 63992e193c231306416089158b2c191b5fbbca9a: false positive within this PR's claimed leaf-race boundary. I recommend an individual false-positive dismissal; I did not dismiss it.

  • packages/studio-server/src/routes/files.ts:2213 is only a filename preselection hint. A stale false result does not authorize a path write: :2241 opens with wx, and only that successful open can produce the fd used at :2243. A leaf that appears in the window yields EEXIST; :2251-2253 advances the suffix and the loop rechecks containment before the next attempt. A stale true result can at worst choose a now-unnecessary suffix.
  • Node documents wx as create-for-write that fails when the path exists and maps O_CREAT|O_EXCL to Windows CREATE_NEW; Node's own recommended race-free example is open(..., 'wx') followed by descriptor I/O. On Linux, strace against the repo's Node 22 showed O_WRONLY|O_CREAT|O_EXCL|O_TRUNC, and POSIX/Linux specify the existence-check/create as atomic and require EEXIST for a symlink leaf regardless of its target.
  • Linux control: existing regular, live symlink, and dangling symlink all returned EEXIST; the victim bytes and missing dangling target were unchanged. Across 50 synchronized 16-writer races, every round produced exactly one creator and 15 EEXISTs. Five hundred open/write/close cycles left /proc/self/fd unchanged (22 -> 22).
  • Windows control is contractual rather than inferred: Node 22 uses libuv; libuv 1.51 maps UV_FS_O_CREAT|UV_FS_O_EXCL (including the truncating form produced by wx) to CREATE_NEW, and Microsoft specifies that CREATE_NEW fails with ERROR_FILE_EXISTS whenever the named leaf exists and creates only an absent leaf. The exact-head Windows workflow is green.
  • The fd is block-scoped and closed in finally; after the exclusive open, payload writes do not resolve the pathname again. No regular-file, hard-link, live-link, or dangling-link race can redirect or overwrite an existing leaf through this source-to-sink path.

The residual race is in ancestor path-component replacement between isSafePath and openSync, plus Node's documented caveat for filesystems that do not honor exclusive creation. This PR explicitly leaves trusted ancestors unchanged, and the Studio/local-filesystem contract plus current ext filesystem are inside that boundary; neither is #900's reported leaf race.

The retried Typecheck, Test, and Windows suites are green at this head. CodeQL remains red solely pending disposition of #900. This approval supersedes my prior request-changes after the scoped audit changed the acceptance path.

Verdict: APPROVE
Reasoning: wx makes leaf creation authoritative and atomic on the supported Linux/Windows implementations, descriptor I/O removes the post-open pathname use, and the alert's precheck can affect naming only—not overwrite or redirection.

— Magi

@jrusso1020
jrusso1020 merged commit 91a34ff into main Sep 8, 2026
98 of 131 checks passed
@jrusso1020
jrusso1020 deleted the fix/security-upload-collisions branch September 8, 2026 22:52
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.

3 participants