Skip to content

fix(studio): contain project IDs across client and server routes - #3808

Merged
jrusso1020 merged 3 commits into
mainfrom
fix/security-studio-project-urls
Sep 9, 2026
Merged

fix(studio): contain project IDs across client and server routes#3808
jrusso1020 merged 3 commits into
mainfrom
fix/security-studio-project-urls

Conversation

@jrusso1020

Copy link
Copy Markdown
Collaborator

Studio decoded project IDs from URL hashes and interpolated them directly into project API and resource URLs. Encoding alone was insufficient: Hono decodes encoded slashes, and the Vite adapter could resolve a traversal ID or session mapping outside the project directory.

This change validates decoded project IDs at hash parsing and URL construction, migrates 43 raw project URL constructions to the existing builder, and checks Vite project/session paths before filesystem access. The composition-file loader also encodes its file suffix. Names containing spaces, Unicode, percent signs, #, and ?, session aliases, and intentionally linked project directories remain supported. Exact dot segments, separators, and control characters are rejected.

Targets 17 findings sharing the same two hash sources: #869, #866, #854, #789, #753, #729, #728, #681, #595, #594, #490, #416, #289, #288, #287, #285, #284. The same fix covers non-alert resource-URL siblings. The source-element probe was extracted to keep the touched Studio file under its 600-line gate.

Validation: 4,790 Studio tests pass (18 todo); full workspace build, Studio typecheck, lint/format, and signed commit hooks pass. Ten new routing/real-Hono regression cases fail against the previous implementation. Tests cover encoded traversal, unsafe session mappings, valid names, aliases, and linked projects. Two existing encoding fixtures now use a valid # name instead of a slash-containing ID. An initial WebMCP teardown error did not recur in the isolated test or the complete rerun. No workflow changes or alert dismissals.

@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 main security shape is right at exact head 14acadb5: the 43 previously raw client project-segment constructions now use buildProjectApiPath, #729 additionally encodes the composition-file suffix (packages/studio/src/hooks/useCompositionContentLoader.ts:27), and the Vite resolver validates both direct IDs and session mappings before filesystem access (packages/studio/vite.adapter.ts:206-234). A diff grep finds no remaining raw /api/projects/${projectId}-style construction; existing already-encoded call sites are unchanged. The final JavaScript CodeQL run passed, and refs/pull/3808/merge has zero open alerts, so all 17 named source flows are closed.

Blocker — the portable project-ID validator still admits Win32 drive-relative path syntax. isValidProjectId rejects separators and dot segments but accepts : at packages/studio/src/utils/projectRouting.ts:9-17. On Windows, that is not an ordinary segment:

isValidProjectId("C:")      = true
win32.resolve("C:\\hf\\data", "C:")      = "C:\\hf\\data"
isValidProjectId("C:demo")  = true
win32.resolve("C:\\hf\\data", "C:demo")  = "C:\\hf\\data\\demo"

Both results pass the new lexical isPathWithin check. The client emits /api/projects/C%3A...; Hono decodes the route parameter back to C: / C:demo, so a crafted hash can alias the entire data root or another project on the server drive. Reject colon-bearing IDs in the shared validator and pin C: plus C:demo with a Win32-resolution regression (and the real Hono route where practical). This is the same path-identity boundary as slash/backslash, not a separate hardening wishlist.

Blocker — required Windows CI is red on a PR-introduced fixture. packages/studio/vite.adapter.projects.test.ts:42-43 attempts to create "..Mañana #1? 50%"; ? is not a legal Windows filename, so Studio shard 2 fails with EINVAL and the required Windows aggregator is red. Use a filesystem-portable allowed name for the real-directory/session/symlink test, and keep ? preservation in the string-level routing tests where no directory must exist.

Important — server discovery can publish IDs the new client contract refuses. listProjects returns every qualifying directory name at packages/studio/vite.adapter.ts:186-200 without isValidProjectId, while useServerConnection stores the first ID before buildProjectHash(first.id) can throw (packages/studio/src/hooks/useServerConnection.ts:48-52). On POSIX, a directory containing \\ or a control character can therefore be listed, partially selected, then strand Studio in retry/wait state. Filter invalid names at project-list egress (and add a negative fixture) so discovery and resolution share one authority.

The null-project guards do not issue /undefined requests, and the normal App route still holds the editor behind the splash until a project exists. The extracted probeSourceElement owns no resources and preserves its fail-open response semantics. Fallow's three complexity findings and three clone groups are all inherited—the audit explicitly reports zero introduced issues—so they are non-blocking.

Local verification: the 20 project-routing tests pass. The adapter/Player subset could not be collected in this review worktree because compatible built workspace subpath artifacts were absent; hosted Linux/full-Studio evidence covers them, while the hosted Windows failure above is definitive.

Verdict: REQUEST CHANGES
Reasoning: All 17 reported flows are closed, but Win32 drive-relative IDs still bypass the claimed project-segment boundary, and a PR-added invalid Windows fixture keeps a required check red.

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

Exact-head re-review at 69c1ec08: the only change from 14acadb5 is packages/studio/vite.adapter.projects.test.ts:42, replacing the Windows-illegal ? in the real-directory fixture. That correctly addresses the required Windows failure while the separate string-level ? routing coverage remains.

The production blockers from review 5151992240 are unchanged:

  • packages/studio/src/utils/projectRouting.ts:9-17 still accepts Win32 drive-relative IDs. C: aliases the data root and C:demo aliases its demo child under win32.resolve; both pass the current isPathWithin check after Hono decodes %3A.
  • packages/studio/vite.adapter.ts:186-200 still publishes qualifying invalid directory names, while packages/studio/src/hooks/useServerConnection.ts:48-52 stores the first ID before buildProjectHash can reject it. Filter discovery through the same validator.

The 17 CodeQL flows were already clear before this test-only push; the new scan and Windows rerun are still pending. Fallow findings remain inherited and non-blocking.

Verdict: REQUEST CHANGES
Reasoning: The CI-only fixture correction is valid, but it does not change the remaining Win32 drive-relative alias or discovery-contract gaps.

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

Both remaining findings are cleared at exact head dfe08c5b.

  • packages/studio/src/utils/projectRouting.ts:9-17 now rejects colon-bearing IDs at the same shared boundary as separators/dot segments. The regressions cover hash parsing, API/hash construction, the real Hono decode path, and the exact win32.resolve alias behavior for C: and C:demo (packages/studio/vite.adapter.projects.test.ts:32-60).
  • packages/studio/vite.adapter.ts:186-193 now filters discovery through isValidProjectId, so the project list and resolver cannot disagree. The POSIX-only fixture proves invalid directory names are not published without pretending those names can be created on Windows.
  • The portable filesystem fixture remains fixed, while ? preservation stays pinned in string-level routing coverage.

The final JavaScript CodeQL run passes and the PR merge ref has zero open alerts, confirming all 17 requested flows remain closed. Windows studio/core/engine/CLI and render lanes are green, including the new platform-sensitive controls. Fallow still reports only the six inherited findings and zero introduced issues.

The top-level Test aggregator is still pending; merge must wait for it. No code or security blocker remains in this review.

Verdict: APPROVE
Reasoning: Project-ID validation, server discovery, Hono decoding, and Win32 path semantics now share one fail-closed contract, with all 17 CodeQL flows and the relevant Windows lanes green.

— Magi

@jrusso1020
jrusso1020 merged commit f54ba56 into main Sep 9, 2026
71 of 72 checks passed
@jrusso1020
jrusso1020 deleted the fix/security-studio-project-urls branch September 9, 2026 09:27
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