feat: add clone path and related test#752
Open
batleforc wants to merge 1 commit into
Open
Conversation
batleforc
requested review from
RomanNikitenko,
azatsarynnyy,
rgrunber and
vitaliy-guliy
as code owners
Contributor
|
Click here to review and test in web IDE: |
&message=Dev%20cluster%20(for%20maintainers)&logo=eclipseche&color=525C86&labelColor=FDB940)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds an optional Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant CodeWorkspace
participant path
participant FileSystem
CodeWorkspace->>path: resolve(PROJECTS_ROOT, clonePath or name)
CodeWorkspace->>FileSystem: check resolved path exists
CodeWorkspace->>CodeWorkspace: skip if path escapes PROJECTS_ROOT
CodeWorkspace->>CodeWorkspace: add workspace folder with resolved path
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@launcher/src/code-workspace.ts`:
- Around line 156-161: `clonePath` in `code-workspace.ts` is being used directly
in `workspace.folders` path construction, which allows path traversal outside
`env.PROJECTS_ROOT`. Update the workspace-building logic around `pathProject` so
`clonePath` is validated/sanitized to ensure it stays relative to the projects
root before calling `fs.pathExists` or pushing a folder. Use the
`workspace.folders` append flow and the `project.clonePath || project.name`
assignment as the main place to enforce the check, and reject or ignore any
value that resolves outside `env.PROJECTS_ROOT`.
In `@launcher/tests/code-workspace.spec.ts`:
- Around line 525-557: Add a test in CodeWorkspace.generate() for the existing
single-project workspace case where the .code-workspace file already exists
under PROJECTS_ROOT using clonePath, not project.name. Update the
pathExists/readFile setup to return true for ${clonePath}/.code-workspace and
verify generate() looks up that exact location. This should cover the branch
that currently uses the wrong lookup key and ensure no writeFile occurs when the
workspace already exists.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58ef240a-5a03-4d01-8b05-e7b1da04b9a5
📒 Files selected for processing (4)
launcher/src/code-workspace.tslauncher/src/flattened-devfile.tslauncher/tests/_data/flattened.devworkspace.with-clone-path.yamllauncher/tests/code-workspace.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
launcher/src/code-workspace.ts (1)
162-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
constinstead ofletforpathProject.
pathProjectis never reassigned.♻️ Proposed fix
- let pathProject = project.clonePath || project.name; + const pathProject = project.clonePath || project.name;🤖 Prompt for 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. In `@launcher/src/code-workspace.ts` at line 162, Use const instead of let for pathProject in the code-workspace logic because the value is never reassigned. Update the declaration in the code that builds the workspace path (around the pathProject assignment) so it remains immutable and matches the existing naming in this section.
🤖 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 `@launcher/src/code-workspace.ts`:
- Around line 163-167: The workspace-folder path validation in code-workspace.ts
is using a string-prefix check that can be bypassed by paths like sibling
directories with the same prefix. Update the fullPath validation in the
project-iteration logic to use a real path-boundary check based on path.relative
against env.PROJECTS_ROOT, and keep the existing skip/continue behavior when the
resolved path escapes the root.
---
Nitpick comments:
In `@launcher/src/code-workspace.ts`:
- Line 162: Use const instead of let for pathProject in the code-workspace logic
because the value is never reassigned. Update the declaration in the code that
builds the workspace path (around the pathProject assignment) so it remains
immutable and matches the existing naming in this section.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bf1dbc1-438d-44fc-8113-055887b34452
📒 Files selected for processing (4)
launcher/src/code-workspace.tslauncher/src/flattened-devfile.tslauncher/tests/_data/flattened.devworkspace.with-clone-path.yamllauncher/tests/code-workspace.spec.ts
✅ Files skipped from review due to trivial changes (1)
- launcher/tests/_data/flattened.devworkspace.with-clone-path.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- launcher/src/flattened-devfile.ts
- launcher/tests/code-workspace.spec.ts
Comment on lines
+163
to
+167
| const fullPath = path.resolve(env.PROJECTS_ROOT, pathProject); | ||
| if (!fullPath.startsWith(path.resolve(env.PROJECTS_ROOT))) { | ||
| console.log(`> Skipping project ${project.name}: clonePath escapes projects root`); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Path traversal check is bypassable via string-prefix false positives.
fullPath.startsWith(path.resolve(env.PROJECTS_ROOT)) is a string comparison, not a path boundary check. If PROJECTS_ROOT is /projects, a clonePath like ../projects-evil/data resolves to /projects-evil/data, which passes startsWith('/projects') because /projects is a string prefix of /projects-evil. This allows directories outside the intended root to be added as workspace folders.
Use path.relative to ensure the resolved path stays within the root:
🔒 Proposed fix using path.relative
const fullPath = path.resolve(env.PROJECTS_ROOT, pathProject);
- if (!fullPath.startsWith(path.resolve(env.PROJECTS_ROOT))) {
+ const relative = path.relative(path.resolve(env.PROJECTS_ROOT), fullPath);
+ if (relative.startsWith('..') || path.isAbsolute(relative)) {
console.log(`> Skipping project ${project.name}: clonePath escapes projects root`);
continue;
}🤖 Prompt for 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.
In `@launcher/src/code-workspace.ts` around lines 163 - 167, The workspace-folder
path validation in code-workspace.ts is using a string-prefix check that can be
bypassed by paths like sibling directories with the same prefix. Update the
fullPath validation in the project-iteration logic to use a real path-boundary
check based on path.relative against env.PROJECTS_ROOT, and keep the existing
skip/continue behavior when the resolved path escapes the root.
Signed-off-by: Max Batleforc <maxleriche.60@gmail.com> Assisted-by: CodeRabbit review
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
1 participant
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.
What does this PR do?
In code-workspace, either use the clonePath if it exist or the project name to find the folder.
What issues does this PR fix?
eclipse-che/che#23909
How to test this PR?
Does this PR contain changes that override default upstream Code-OSS behavior?
git rebasewere added to the .rebase folderSummary by CodeRabbit
New Features
Bug Fixes
Tests