Fix path traversal in profile operations#55
Conversation
Reject profile names containing path separators or traversal sequences to prevent arbitrary filesystem delete/create/symlink. Fixes Microck#54
|
@AAtomical is attempting to deploy a commit to the Projects Team on Vercel. A member of the Team first needs to authorize it. |
|
This PR prevents profile operations from acting on path-traversal names by rejecting separators and
WalkthroughAdded 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 `@server/profile-manager.js`:
- Around line 9-14: Update safeName to reject "." and enforce that the resolved
profile path is a direct child of PROFILES_DIR before deleteProfile or
activateProfile performs filesystem operations; add a regression test covering
"." and path containment.
- Around line 68-69: Require authentication and authorization for profile
mutations before invoking deleteProfile or the activation operation. Update the
DELETE /api/profiles/:name and POST /api/profiles/:name/activate handlers to
validate the requester’s permission to manage the supplied profile, while
preserving safeName validation and rejecting unauthorized requests before any
deletion or OPENCODE_DIR change occurs.
🪄 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: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: cfeb784a-0b64-41d0-b275-fa194984f601
📒 Files selected for processing (1)
server/profile-manager.js
| function safeName(name) { | ||
| if (!name || name.includes('/') || name.includes('\\') || name.includes('..')) { | ||
| throw new Error('Invalid profile name'); | ||
| } | ||
| return name; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Path Traversal (CWE-22): Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Reachability: External
Reject . and enforce path containment.
safeName('.') passes validation, but path.join(PROFILES_DIR, '.') resolves to PROFILES_DIR. Consequently, deleteProfile('.') recursively deletes the entire profiles directory, and activateProfile('.') can point OPENCODE_DIR at the profiles root. Reject . and verify the resolved target is a direct child before filesystem operations; add a regression test.
🤖 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 `@server/profile-manager.js` around lines 9 - 14, Update safeName to reject "."
and enforce that the resolved profile path is a direct child of PROFILES_DIR
before deleteProfile or activateProfile performs filesystem operations; add a
regression test covering "." and path containment.
| function deleteProfile(name) { | ||
| safeName(name); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Authorization Bypass (CWE-862): Missing Authorization
Reachability: External
Require authorization before profile deletion and activation.
The supplied DELETE /api/profiles/:name and POST /api/profiles/:name/activate handlers pass request-controlled names directly to these methods without an ownership or permission check. safeName prevents traversal but still lets any unauthenticated caller delete another non-active profile or switch the global OPENCODE_DIR. Enforce authentication and authorization before invoking these operations.
Also applies to: 81-82
🤖 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 `@server/profile-manager.js` around lines 68 - 69, Require authentication and
authorization for profile mutations before invoking deleteProfile or the
activation operation. Update the DELETE /api/profiles/:name and POST
/api/profiles/:name/activate handlers to validate the requester’s permission to
manage the supplied profile, while preserving safeName validation and rejecting
unauthorized requests before any deletion or OPENCODE_DIR change occurs.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function safeName(name) { | ||
| if (!name || name.includes('/') || name.includes('\\') || name.includes('..')) { | ||
| throw new Error('Invalid profile name'); | ||
| } | ||
| return name; | ||
| } |
There was a problem hiding this comment.
safeName('.') currently passes, so deleteProfile('.') builds path.join(PROFILES_DIR, '.') and then fs.rmSync removes the entire profiles directory. This leaves the delete path with a concrete filesystem escape despite rejecting separators and ..; reject . before using the name in createProfile, deleteProfile, or activateProfile.
Artifacts
Repro: Node.js harness that invokes deleteProfile dot with an isolated HOME
- Contains supporting evidence from the run (text/javascript; charset=utf-8).
Repro: verbose harness output showing profiles root deletion
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/profile-manager.js
Line: 9-14
Comment:
**Reject dot profile names**
`safeName('.')` currently passes, so `deleteProfile('.')` builds `path.join(PROFILES_DIR, '.')` and then `fs.rmSync` removes the entire profiles directory. This leaves the delete path with a concrete filesystem escape despite rejecting separators and `..`; reject `.` before using the name in `createProfile`, `deleteProfile`, or `activateProfile`.
How can I resolve this? If you propose a fix, please make it concise.
|
@AAtomical Thanks for the PR! Feel free to fix the issues notified by Greptile and CodeRabbit, afterwards I will merge to main. |
2 participants
Reject profile names containing path separators or traversal sequences to prevent arbitrary filesystem delete/create/symlink.
Fixes #54
Greptile Summary
This PR adds validation before profile filesystem operations. The main changes are:
/,\, or...Confidence Score: 4/5
The change needs one contained validation fix before merging.
The new checks block separators and
.., but.still resolves to the profiles root on the delete path.server/profile-manager.js
What T-Rex did
Important Files Changed
.to target the profiles root on delete.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Caller participant Manager as profile-manager participant Validator as safeName participant FS as filesystem Caller->>Manager: create/delete/activate profile(name) Manager->>Validator: validate name alt name contains separator or traversal Validator-->>Manager: throw Invalid profile name Manager-->>Caller: error else name accepted Validator-->>Manager: return name Manager->>FS: path.join(PROFILES_DIR, name) Manager->>FS: create/remove/symlink profile target Manager-->>Caller: success end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Caller participant Manager as profile-manager participant Validator as safeName participant FS as filesystem Caller->>Manager: create/delete/activate profile(name) Manager->>Validator: validate name alt name contains separator or traversal Validator-->>Manager: throw Invalid profile name Manager-->>Caller: error else name accepted Validator-->>Manager: return name Manager->>FS: path.join(PROFILES_DIR, name) Manager->>FS: create/remove/symlink profile target Manager-->>Caller: success endPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "Fix path traversal in profile operations" | Re-trigger Greptile