fix(tools): add executable path validation to prevent directory traversal - #603
fix(tools): add executable path validation to prevent directory traversal#603a-oren wants to merge 6 commits into
Conversation
…rsal Adds validateExecutablePath() to reject user-configured executable paths containing '..' traversal segments or './' relative prefixes before they reach execFileSync(). Bare command names and absolute paths are allowed. Wrapper paths from traverseForWrapper() are unaffected. Implements TC-5485 Assisted-by: Claude Code
Reviewer's GuideAdds centralized executable path validation to getCustomPath() to block directory traversal and relative ./ paths, and backs it with targeted tests including a regression test ensuring wrapper resolution remains unaffected. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="test/tools.test.js" line_range="105" />
<code_context>
+ )
+ })
+
+ /** Verifies that paths starting with "./" (workspace-relative) are rejected. */
+ test('rejects paths starting with "./"', () => {
+ process.env['TRUSTIFY_DA_DUMMY_PATH'] = './malicious.sh'
</code_context>
<issue_to_address>
**question (testing):** Consider adding a positive test for allowed relative paths without './' to document intended behavior
The validation now rejects `./`-prefixed paths but still permits other relative paths without `./` or `..` (e.g. `bin/mvn`, `tools/mvnw`). If that’s the desired behavior, please add a test confirming that `getCustomPath` accepts these paths so the distinction is documented and future changes don’t accidentally alter it.
</issue_to_address>
### Comment 2
<location path="test/tools.test.js" line_range="113-119" />
<code_context>
+ )
+ })
+
+ /** Verifies that traversal paths supplied via opts are also rejected. */
+ test('rejects traversal paths provided via opts', () => {
+ const opts = { 'TRUSTIFY_DA_DUMMY_PATH': '../../tmp/evil' }
</code_context>
<issue_to_address>
**suggestion (testing):** Also cover './' rejection when the path is supplied via opts, not only via process.env
You already verify traversal rejection via `opts`. To align coverage with the env-based `'./'` test, please add a similar case using `const opts = { TRUSTIFY_DA_DUMMY_PATH: './malicious.sh' }` and assert that `getCustomPath('dummy', opts)` throws the expected `'./'` error. This confirms both env and opts inputs share the same validation behavior.
```suggestion
/** Verifies that traversal paths supplied via opts are also rejected. */
test('rejects traversal paths provided via opts', () => {
const opts = { 'TRUSTIFY_DA_DUMMY_PATH': '../../tmp/evil' }
expect(() => getCustomPath('dummy', opts)).to.throw(
Error, 'path contains directory traversal segment (..)'
)
})
/** Verifies that paths starting with "./" provided via opts are rejected. */
test('rejects "./" paths provided via opts', () => {
const opts = { TRUSTIFY_DA_DUMMY_PATH: './malicious.sh' }
expect(() => getCustomPath('dummy', opts)).to.throw(
Error, "relative paths starting with './' are not allowed"
)
})
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ) | ||
| }) | ||
|
|
||
| /** Verifies that paths starting with "./" (workspace-relative) are rejected. */ |
There was a problem hiding this comment.
question (testing): Consider adding a positive test for allowed relative paths without './' to document intended behavior
The validation now rejects ./-prefixed paths but still permits other relative paths without ./ or .. (e.g. bin/mvn, tools/mvnw). If that’s the desired behavior, please add a test confirming that getCustomPath accepts these paths so the distinction is documented and future changes don’t accidentally alter it.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as question — asks whether allowing relative paths without ./ or .. (e.g., bin/mvn) is intentional behavior. This is a valid design question for the PR author to clarify, but does not require a code change. No sub-task created.
The CI runner has TRUSTIFY_DA_PIP3_PATH set, causing getCustomPath('pip3')
to return the env var value instead of the bare name. Save and restore any
matching env vars during the test to isolate from the CI environment.
Implements TC-5485
Assisted-by: Claude Code
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #603 +/- ##
==========================================
+ Coverage 90.95% 91.32% +0.37%
==========================================
Files 41 43 +2
Lines 8984 9592 +608
Branches 1573 1727 +154
==========================================
+ Hits 8171 8760 +589
- Misses 813 832 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Verification Report for TC-5485 (commit db61fcf)
Overall: PASSAll checks pass. The implementation adds This comment was AI-generated by sdlc-workflow/verify-pr v0.13.2. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ruromero
left a comment
ruromero
left a comment
There was a problem hiding this comment.
Security review of the path traversal fix. Found a confirmed bypass where bare .. evades validation, plus minor issues.
| */ | ||
| function validateExecutablePath(binPath) { | ||
| if (!binPath.includes('/') && !binPath.includes('\\')) { | ||
| return binPath |
There was a problem hiding this comment.
Bug — Bare .. bypasses all validation (CONFIRMED)
The early-return short-circuit lets a bare .. (no slashes) pass unchecked. The check !binPath.includes('/') && !binPath.includes('\\') is true for .., so it returns immediately — the segments.includes('..') check on line 65 never runs.
Reproducer: TRUSTIFY_DA_MVN_PATH=..
Suggested fix: Remove the early return entirely. '..'.split(/[\/\\]/) produces ['..'], which includes('..') catches, while bare names like mvn still pass all subsequent checks.
Secondary concern: validateExecutablePath(null) would throw a TypeError (.includes() on null) rather than a descriptive validation error. Current callers always pass string defaults via getCustom so this is not reachable today, but the function is fragile if upstream call patterns change. A typeof binPath !== 'string' guard at the top would harden it.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — sub-task TC-5619 created to address this feedback.
| return binPath | ||
| } | ||
|
|
||
| if (binPath.startsWith('./') || binPath.startsWith('.\\')) { |
There was a problem hiding this comment.
Nit — Relative paths without ./ prefix still pass (PLAUSIBLE)
subdir/malicious contains / (no early return), does not start with ./, and splits into segments without .. — so validation passes. While this doesn't enable upward traversal (the CVE target), it allows execution of binaries resolved relative to cwd, which may not match the intent of blocking ./-prefixed paths.
Worth considering whether any path that isn't absolute and isn't a bare command name should be rejected.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as nit — advisory observation about relative paths without ./ prefix. The reviewer notes this is PLAUSIBLE but uses "Worth considering" language, not a required change. No sub-task created.
| /** Verifies that valid absolute paths are accepted. */ | ||
| test('allows valid absolute paths', () => { | ||
| process.env['TRUSTIFY_DA_DUMMY_PATH'] = '/usr/bin/mvn' | ||
| expect(getCustomPath('dummy')).to.equal('/usr/bin/mvn') |
There was a problem hiding this comment.
Nit — Redundant delete
This delete process.env['TRUSTIFY_DA_DUMMY_PATH'] is unnecessary — the next line overwrites it with a new value, and afterEach already handles cleanup.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as nit — minor cleanup feedback about a redundant delete call that is handled by afterEach. No sub-task created.
Verification: Path traversal validation in
|
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review — Classified as suggestion — meta-summary of 2 inline issues (question about relative path testing + suggestion about opts-based |
Verification Report for TC-5485 (commit db61fcf)
Overall: WARNThe implementation correctly adds This comment was AI-generated by sdlc-workflow/verify-pr v0.13.8. |
Remove the early-return optimization that short-circuited validation for
inputs without path separators. `..` contains no `/` or `\` and was
incorrectly treated as a safe bare command name, skipping the
segments.includes('..') check entirely. Without the early return,
'..'.split(/[/\\]/) correctly produces ['..'] which the existing segment
check catches.
Implements TC-5619
Assisted-by: Claude Code
…ation Tighten validateExecutablePath to reject any path that contains a separator but is not absolute (e.g. subdir/binary, bin/mvn). Custom executable paths must be either a bare command name resolved via PATH or an explicit absolute path — relative paths with directory components could resolve to workspace-internal files. Implements TC-5619 Assisted-by: Claude Code
The next line immediately overwrites the env var, and afterEach handles cleanup. Addresses reviewer nit. Implements TC-5619 Assisted-by: Claude Code
Add a typeof check at the top of validateExecutablePath to throw a descriptive error instead of a TypeError if null, undefined, or a non-string value is passed. Not reachable from current callers but hardens the function against future call-site changes. Implements TC-5619 Assisted-by: Claude Code
3 participants
Summary
Adds defense-in-depth path validation for CVE-2026-18389. The new
validateExecutablePath()function insrc/tools.jsrejects user-configured executable paths that contain..traversal segments or./relative prefixes before they reachexecFileSync(). Bare command names and valid absolute paths are allowed. Wrapper paths resolved bytraverseForWrapper()are intentionally unaffected — they are protected by Workspace Trust gating in the VS Code extension...(without path separators) to bypass validation...is now correctly caught by the segment check.Changes
src/tools.js: AddedvalidateExecutablePath(binPath)function and integrated it intogetCustomPath()as the validation choke point. Removed early-return optimization that bypassed validation for bare...test/tools.test.js: Added 9 tests covering bare names, absolute paths, traversal rejection,./rejection, bare..rejection, opts-based paths, error messages, and wrapper path regression.Test plan
npm test— 576 passing (9 new), 17 failing (pre-existing, poetry not installed)npm run lint— 0 errorsresolveBinary()→traverseForWrapper()bypasses validation..is now correctly rejectedImplements TC-5485
Implements TC-5619