-
Notifications
You must be signed in to change notification settings - Fork 11
fix(tools): add executable path validation to prevent directory traversal #603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
b86ad48
db61fcf
1053600
bce6448
e5b43f0
286d1c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ import { expect } from 'chai' | |
| import esmock from 'esmock' | ||
| import { afterEach } from 'mocha' | ||
|
|
||
| import { getCustom, getCustomPath} from "../src/tools.js" | ||
| import { getCustom, getCustomPath } from "../src/tools.js" | ||
|
|
||
|
|
||
| /** | ||
|
|
@@ -65,6 +65,111 @@ suite('testing the various tools and utility functions', () => { | |
|
|
||
| }) | ||
|
|
||
| suite('test getCustomPath executable path validation', () => { | ||
| afterEach(() => delete process.env['TRUSTIFY_DA_DUMMY_PATH']) | ||
|
|
||
| /** Verifies that bare command names pass validation (resolved via OS PATH lookup). */ | ||
| test('allows bare command names without path separators', () => { | ||
| const commands = ['mvn', 'npm', 'go', 'cargo', 'pip3'] | ||
| const saved = {} | ||
| for (const cmd of commands) { | ||
| const envKey = `TRUSTIFY_DA_${cmd.toUpperCase()}_PATH` | ||
| if (envKey in process.env) { | ||
| saved[envKey] = process.env[envKey] | ||
| delete process.env[envKey] | ||
| } | ||
| } | ||
| try { | ||
| for (const cmd of commands) { | ||
| expect(getCustomPath(cmd)).to.equal(cmd) | ||
| } | ||
| } finally { | ||
| for (const [key, val] of Object.entries(saved)) { | ||
| process.env[key] = val | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| /** 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') | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit — Redundant delete This
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [sdlc-workflow/verify-pr] Classified as nit — minor cleanup feedback about a redundant |
||
|
|
||
| process.env['TRUSTIFY_DA_DUMMY_PATH'] = '/usr/local/bin/npm' | ||
| expect(getCustomPath('dummy')).to.equal('/usr/local/bin/npm') | ||
| }) | ||
|
|
||
| /** Reproducer: relative path with traversal segments must be rejected. */ | ||
| test('rejects relative paths containing ".." traversal segments', () => { | ||
| process.env['TRUSTIFY_DA_DUMMY_PATH'] = '../../etc/malicious' | ||
| expect(() => getCustomPath('dummy')).to.throw( | ||
| Error, 'path contains directory traversal segment (..)' | ||
| ) | ||
| }) | ||
|
|
||
| /** Verifies that absolute paths with embedded traversal segments are rejected. */ | ||
| test('rejects absolute paths containing ".." traversal segments', () => { | ||
| process.env['TRUSTIFY_DA_DUMMY_PATH'] = '/usr/bin/../../../tmp/evil' | ||
| expect(() => getCustomPath('dummy')).to.throw( | ||
| Error, 'path contains directory traversal segment (..)' | ||
| ) | ||
| }) | ||
|
|
||
| /** Verifies that paths starting with "./" (workspace-relative) are rejected. */ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question (testing): Consider adding a positive test for allowed relative paths without './' to document intended behavior The validation now rejects
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [sdlc-workflow/verify-pr] Classified as question — asks whether allowing relative paths without |
||
| test('rejects paths starting with "./"', () => { | ||
| process.env['TRUSTIFY_DA_DUMMY_PATH'] = './malicious.sh' | ||
| expect(() => getCustomPath('dummy')).to.throw( | ||
| Error, "relative paths starting with './' are not allowed" | ||
| ) | ||
| }) | ||
|
|
||
| /** 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 (..)' | ||
| ) | ||
| }) | ||
|
sourcery-ai[bot] marked this conversation as resolved.
|
||
|
|
||
| /** Verifies that a bare '..' without path separators is rejected. */ | ||
| test('rejects bare ".." without path separators', () => { | ||
| process.env['TRUSTIFY_DA_DUMMY_PATH'] = '..' | ||
| expect(() => getCustomPath('dummy')).to.throw( | ||
| Error, 'path contains directory traversal segment (..)' | ||
| ) | ||
| }) | ||
|
|
||
| /** Verifies that relative paths with separators (e.g. subdir/binary) are rejected. */ | ||
| test('rejects relative paths with separators', () => { | ||
| process.env['TRUSTIFY_DA_DUMMY_PATH'] = 'subdir/binary' | ||
| expect(() => getCustomPath('dummy')).to.throw( | ||
| Error, 'relative paths are not allowed, use an absolute path or a bare command name' | ||
| ) | ||
| }) | ||
|
|
||
| /** Verifies that rejected paths include the offending path in the error message. */ | ||
| test('error message includes the rejected path', () => { | ||
| process.env['TRUSTIFY_DA_DUMMY_PATH'] = '../../sneaky/script' | ||
| expect(() => getCustomPath('dummy')).to.throw('../../sneaky/script') | ||
| }) | ||
| }) | ||
|
|
||
| suite('test resolveBinary wrapper path regression', () => { | ||
| /** Verifies that resolveBinary with a wrapper path bypasses getCustomPath validation. */ | ||
| test('wrapper path from traverseForWrapper is not subject to path validation', async () => { | ||
| // Given: a mocked traverseForWrapper that returns a workspace-relative wrapper path | ||
| const tools = await esmock('../src/tools.js', {}, { | ||
| 'node:fs': { | ||
| accessSync: () => undefined | ||
| } | ||
| }) | ||
|
|
||
| // When: resolveBinary finds a wrapper, it returns it directly without validation | ||
| const result = tools.resolveBinary('mvn', 'mvnw', '/workspace/project') | ||
| expect(result).to.equal('/workspace/project/mvnw') | ||
| }) | ||
| }) | ||
|
|
||
| suite('test the handleSpacesInPath utility function', () => { | ||
|
|
||
| test('Windows Path with spaces', async () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit — Relative paths without
./prefix still pass (PLAUSIBLE)subdir/maliciouscontains/(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.