fix: correct workspace root containment check for root paths - #1111
Open
MacLeod92 wants to merge 2 commits into
Open
fix: correct workspace root containment check for root paths#1111MacLeod92 wants to merge 2 commits into
MacLeod92 wants to merge 2 commits into
Conversation
validateWorkspacePath checked containment via
path.startsWith(`${root}${path.sep}`), which appends a path separator
unconditionally. When WORKSPACES_ROOT resolves to an actual filesystem
root ("/" on Unix, "C:\\" on Windows), the root already ends in a
separator, so the check effectively looked for a double separator that
no real path has — rejecting every path except the root itself, with
"Workspace path must be within the allowed workspace root: /" even for
genuinely contained paths. The same bug existed in the adjacent
symlink-target check.
Replaced both with a shared isPathWithinRoot helper based on
path.relative, which handles root paths, drive letters, and trailing
separators correctly without manual separator bookkeeping.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughChangesPath containment validation
Poem
Merge Risk: ⚪ Minimal · up to The change corrects workspace containment handling for filesystem-root paths and centralizes the check without any actionable merge-blocking risk remaining. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/shared/utils.ts`:
- Around line 211-214: Update isPathWithinRoot so descendant paths whose names
begin with “..” remain valid; reject only a relative path equal to “..” or
starting with “..” followed by path.sep, while preserving the existing
absolute-path and root-path handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e36694e5-ec7e-446e-89dc-15f284e1109e
📒 Files selected for processing (1)
server/shared/utils.ts
…versal
isPathWithinRoot rejected any relative path that starts with the two
characters "..", including a real directory name like "..cache" that
is not parent traversal at all. path.relative('/workspaces',
'/workspaces/..cache') returns "..cache", and the old check treated
that string prefix as an escape attempt, rejecting a path that never
leaves the root.
Addresses a review comment from CodeRabbit on this PR. The fix now
checks for an exact ".." segment or a ".." followed by a path
separator, so only genuine parent traversal is rejected. A descendant
name that merely begins with ".." is now treated as contained, as it
should be.
MacLeod92
force-pushed
the
fix/root-containment-double-separator
branch
from
August 18, 2026 20:54
ce509d3 to
1d2093a
Compare
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
2 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.
Problem
The function
validateWorkspacePathchecks two conditions with a string comparison. It tests whether a path starts with the workspace root plus one separator. This check fails when the workspace root itself resolves to a filesystem root, such as/on Unix orC:\on Windows.A filesystem root already ends in a separator. The check appends a second separator before the comparison. This produces a string like
//orC:\\, which no normal absolute path starts with. As a result, every requested path gets rejected, even paths that are correct and safe.Cause
The environment variable
WORKSPACES_ROOTsets the workspace root. This variable does not pass throughFORBIDDEN_WORKSPACE_PATHS. An operator can setWORKSPACES_ROOTto a drive root on Windows, since the forbidden list blocks specific Windows paths but not the drive root itself. In this case, the broken check runs on every request, with no earlier guard to catch it.Fix
This PR adds a function named
isPathWithinRoot. The function usespath.relativein place of string-prefix matching. It compares the relative path between the root and the target. It reports containment as true when the relative path is empty, or when the relative path does not start with..and is not itself absolute.This PR also removes the two duplicate
ifblocks that ran the broken check. Both call sites, the main containment check and the symlink-target check, now call the shared function.Testing
Single commit,
server/shared/utils.tsonly (+14/-8). This fix has been running in a private build since 2026-07-01 without issues. I confirmed the original check rejects valid paths when the workspace root resolves to/. I confirmed the new function accepts these same paths. I ran the existing test suite. I found no regressions in normal, non-root workspace configurations.Summary by CodeRabbit