Fix path-injection in /paths/compare by confining inputs to the sites root - #4267
Merged
Conversation
… alerts Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
📊 Performance Test ResultsComparing ac6b06f vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
epeicher
approved these changes
Jul 23, 2026
epeicher
left a comment
epeicher
left a comment
Contributor
There was a problem hiding this comment.
I have tested and reviewed the changes, and they LGTM!
1 task
wojtekn
added a commit
that referenced
this pull request
Jul 28, 2026
…solve (#4324) ## Related issues - Related to code scanning alerts [#207](https://github.com/Automattic/studio/security/code-scanning/207) and [#208](https://github.com/Automattic/studio/security/code-scanning/208) - Follow-up to #4267, which added a sound runtime guard but left both CodeQL alerts open ## How AI was used in this PR Claude Code confirmed via the GitHub API that both alerts were still `open` on the latest `trunk` scan (which already contains #4267), traced why CodeQL's taint tracker did not treat the previous `isPathWithin` guard as a barrier, designed the restructured barrier pattern, wrote the helper and tests, and ran lint/typecheck/tests. I reviewed the approach and the diff. ## Proposed Changes PR #4267 confined the unauthenticated, loopback-only `POST /api/paths/compare` inputs to the sites root, which is a correct runtime mitigation — but the two high-severity `js/path-injection` alerts stayed **open** after it merged. CodeQL does not recognize the standalone `isPathWithin(...)` boolean check as a sanitizer: the untrusted values that actually reached `fs.statSync` (via `path.resolve` inside `arePathsEqual`) were the *raw* request values, so the taint flow `req.body → path1/path2 → statSync` was never broken in CodeQL's model. This restructures the endpoint into the resolve → confine-to-root → use-the-returned-value pattern that CodeQL treats as a barrier: - A new `confineToRoot( root, candidate )` helper resolves the candidate against the root, checks it is the root or a descendant, and **returns the normalized absolute path** (or `null` when it escapes). `isPathWithin` now delegates to it. - The `/paths/compare` handler passes only the confined, resolved paths into `arePathsEqual` — the raw, untrusted request values never reach any filesystem call. - As additional hardening, relative candidates now resolve against the sites root rather than the process cwd. No user-visible behavior change for legitimate use: the UI only ever compares real site paths under the sites root, which still compare correctly. The shared `arePathsEqual` helper is intentionally left untouched, since its other callers (CLI, desktop IPC, AI sessions) legitimately compare arbitrary absolute site paths that come from trusted local config. > Note: CodeQL can only re-confirm the alerts as resolved on the next scan after this lands on `trunk`. If the model still flags it, the fallback is a CodeQL sanitizer model or a justified dismissal — but the restructured flow is expected to clear it at the source. ## Testing Instructions - `npm test -- packages/common/lib/tests/fs-utils.test.ts` — new `confineToRoot` tests cover the root itself, descendants, in-root `..` normalization, `../` escapes, the sibling-prefix edge case (`studio-sites-other` must not count as inside `studio-sites`), and relative-candidate resolution against the root. Existing `isPathWithin` tests still pass. - `npm run typecheck` passes across all workspaces. - Manual: `POST /api/paths/compare` with two real site paths under the sites root returns `{ equal: true }` when they match; a body with a `../` traversal path returns `{ equal: false }`. ## Pre-merge Checklist - [x] Have you checked for TypeScript, React or other console errors? --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issues
How AI was used in this PR
Claude Code fetched both CodeQL alerts, traced the taint flow, proposed the "confine to a safe root" mitigation, wrote the helper and tests, and ran lint/typecheck/tests. I reviewed the approach and the diff.
Proposed Changes
Two high-severity CodeQL
js/path-injectionalerts flagged the unauthenticated, loopback-onlyPOST /api/paths/compareendpoint in the local server: it passed request-bodypath1/path2straight intoarePathsEqual, which callsfs.statSync(path.resolve(...)). A crafted../payload could probe arbitrary filesystem paths outside the sites directory.This confines both operands to the server's
sitesRootbefore they reach any filesystem call. Anything outsidesitesRootcannot be a Studio site, so a non-match is the correct answer and the untrusted value never reachesstatSyncunguarded. The sharedarePathsEqualhelper is intentionally left untouched, since its other callers (CLI, desktop IPC) legitimately compare arbitrary absolute site paths that come from trusted local config.No user-visible behavior change for legitimate use: the UI only ever compares real site paths under
sitesRoot.Testing Instructions
npm test -- packages/common/lib/tests/fs-utils.test.ts— newisPathWithintests cover the root itself, descendants,../escapes, and the sibling-prefix edge case (sites-othermust not count as insidesites).npm run typecheckpasses across all workspaces.POST /api/paths/comparewith two real site paths under the sites root returns{ equal: true }when they match; a body with a../traversal path returns{ equal: false }.Pre-merge Checklist