fix: path traversal in download_job_artifacts local_path - #644
fix: path traversal in download_job_artifacts local_path#644brennanneoh wants to merge 3 commits into
Conversation
downloadJobArtifacts() joined the caller-controlled local_path argument (download_job_artifacts MCP tool) directly into the filesystem save path with no validation, allowing an absolute path or ../ traversal sequence to write the artifact zip anywhere the process has permission to write. Apply the same normalize/validate check already used by downloadAttachment() for its localPath parameter: reject absolute paths, "..", and any path escaping the intended base directory.
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesArtifact path security
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 3
🤖 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 `@index.ts`:
- Around line 7490-7500: Update the artifact download tests to pass relative
fixture directories below the process working directory instead of the absolute
tmpDir paths. Replace both local_path values based on tmpDir, including the
artifacts/run-42 case, while preserving their intended directory structure and
download assertions.
- Around line 7490-7498: Move the localPath validation block in
downloadJobArtifacts before constructing the artifact URL and issuing the
authenticated GitLab fetch. Preserve the existing traversal checks and error
message, and only proceed to handleGitLabError and streamPipeline after
localPath has been validated.
- Around line 7491-7500: Update the local path validation around
normalizedLocalPath to reject drive-relative Windows paths by parsing the
normalized path and rejecting any path with a non-empty parsed root, including
values such as C:foo and C:..\\outside. Preserve the existing absolute-path and
traversal checks before constructing savePath with path.join.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 389289d6-53fe-4a5d-9ee3-6754657c22ee
📒 Files selected for processing (1)
index.ts
📜 Review details
🔇 Additional comments (1)
index.ts (1)
7491-7500: 🔒 Security & PrivacyVerify the symlink threat model.
This check validates only lexical path components. A relative
localPathsuch aslinkcan name a symlink below the working directory. The later filesystem calls follow that symlink, so the archive can still be written outside the intended base without an absolute path or... If callers are untrusted, canonicalize the parent directory against a fixed base and use no-follow or atomic file creation to avoid a race.
- validate local_path before the GitLab fetch, not after - reject Windows drive-relative paths (e.g. "C:foo") via path.parse().root - update job-artifact tests to use relative fixture dirs, add traversal and absolute-path rejection test cases
|
Pushed 4426f29 addressing the 3 actionable comments:
On the symlink note: agreed it's a real gap, but it's pre-existing in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/test-job-artifacts.ts`:
- Around line 226-230: Restructure the traversal test around the
download_job_artifacts call so a successful call cannot be caught as the
expected rejection. Use assert.rejects or capture only the callTool error in the
try/catch, then perform the rejection/message assertion outside the catch while
preserving the traversal-message validation.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 1c3a1bec-5da0-42f5-bf33-a4480e881c96
📒 Files selected for processing (2)
index.tstest/test-job-artifacts.ts
📜 Review details
🔇 Additional comments (4)
index.ts (2)
7469-7481: LGTM!
7502-7502: LGTM!test/test-job-artifacts.ts (2)
145-150: LGTM!
235-253: LGTM!
assert.fail() inside the same try block as the awaited call could be swallowed by its own catch, silently passing on an unexpected success. Capture the error outside the try/catch and assert on it there.
|
Pushed bbd734f: fixed the flagged issue in the two new traversal-rejection tests. `assert.fail()` was inside the same `try` its own `catch` handled, so an unexpected success would have been silently swallowed as a pass. Now the error is captured outside the `try/catch` and asserted on there, so a non-throwing call correctly fails the test. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/test-job-artifacts.ts (1)
237-256: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for Windows drive-relative paths.
The absolute-path test covers only the host-native absolute-path branch. It does not exercise
path.parse(normalizedLocalPath).rootwith a value such asC:artifacts. If Windows runtimes are supported, add this case to a Windows test job or verify that an existing test covers it. Node.js path parsing is platform-specific. (nodejs.org)🤖 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 `@test/test-job-artifacts.ts` around lines 237 - 256, Add coverage for the Windows drive-relative path case in the download_job_artifacts tests, using a path such as C:artifacts and running it in a Windows-specific test job or environment. Verify that callTool rejects it with the same traversal error, and avoid relying on the host-native absolute-path test to cover platform-specific path.parse behavior.
🤖 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.
Outside diff comments:
In `@test/test-job-artifacts.ts`:
- Around line 237-256: Add coverage for the Windows drive-relative path case in
the download_job_artifacts tests, using a path such as C:artifacts and running
it in a Windows-specific test job or environment. Verify that callTool rejects
it with the same traversal error, and avoid relying on the host-native
absolute-path test to cover platform-specific path.parse behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ac6d3d37-c8c1-4db3-8e38-7b787666ff72
📒 Files selected for processing (1)
test/test-job-artifacts.ts
📜 Review details
🔇 Additional comments (2)
test/test-job-artifacts.ts (2)
7-7: LGTM!Also applies to: 145-150
216-235: LGTM!
1 participant
Summary
downloadJobArtifacts()(index.ts, backing thedownload_job_artifactstool) joined the caller-suppliedlocal_pathinto the filesystem save path with no validation, allowing an absolute path or../traversal to write the downloaded artifact zip anywhere the process has write permission.downloadAttachment()function already validates its ownlocal_paththe same way; this bringsdownloadJobArtifactsin line with that existing, hardened pattern (reject absolute paths and..traversal).Why
Security issue: an MCP client (or anything able to issue tool calls in local/stdio mode) could pass
local_path: "/etc/cron.d"orlocal_path: "../../../../home/user/.ssh"todownload_job_artifactsand get an arbitrary file write on the host.Reported privately to the maintainer alongside a second, unrelated finding before opening this PR.
How tested
npx tsc --noEmitnpm run test:mock(all suites pass; added two new tests totest/test-job-artifacts.tscovering traversal and absolute-path rejection, and updated two existing tests that relied on an absolutelocal_pathto use a relative one, matching the contract already established bydownloadAttachment's test suite)npm run test:consumer-smokeBreaking changes
local_pathfordownload_job_artifactsmust now be a relative, non-traversal path — matching the existing contract fordownload_attachment'slocal_path. Absolute paths or..segments now return an error instead of silently writing outside the intended location.