fix(tools): add executable path validation to prevent directory traversal - #603
Merged
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.
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 91.22% 91.32% +0.10%
==========================================
Files 43 43
Lines 9558 9592 +34
Branches 1717 1727 +10
==========================================
+ Hits 8719 8760 +41
+ Misses 839 832 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Author
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
previously requested changes
Aug 10, 2026
ruromero
left a comment
ruromero
left a comment
Collaborator
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.
Collaborator
Verification: Path traversal validation in
|
Contributor
Author
|
[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 |
Contributor
Author
Contributor
Author
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
Strum355
approved these changes
Aug 20, 2026
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
4 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.
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