Skip to content

Commit 4426f29

Browse files
committed
fix: address CodeRabbit review on download_job_artifacts traversal fix
- 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
1 parent 96c0401 commit 4426f29

2 files changed

Lines changed: 58 additions & 18 deletions

File tree

index.ts

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7466,6 +7466,19 @@ async function downloadJobArtifacts(
74667466
jobId: string,
74677467
localPath?: string
74687468
): Promise<string> {
7469+
if (localPath) {
7470+
const normalizedLocalPath = path.normalize(localPath);
7471+
if (
7472+
path.isAbsolute(normalizedLocalPath) ||
7473+
path.parse(normalizedLocalPath).root !== "" ||
7474+
normalizedLocalPath === ".." ||
7475+
normalizedLocalPath.startsWith(".." + path.sep) ||
7476+
normalizedLocalPath.includes(path.sep + ".." + path.sep)
7477+
) {
7478+
throw new Error("Invalid local_path: directory traversal is not allowed.");
7479+
}
7480+
}
7481+
74697482
projectId = decodeURIComponent(projectId);
74707483
const effectiveProjectId = getEffectiveProjectId(projectId);
74717484
@@ -7486,21 +7499,7 @@ async function downloadJobArtifacts(
74867499
await handleGitLabError(response);
74877500
74887501
const filename = `artifacts_job_${encodeGitLabPathSegment(jobId)}.zip`;
7489-
let savePath: string;
7490-
if (localPath) {
7491-
const normalizedLocalPath = path.normalize(localPath);
7492-
if (
7493-
path.isAbsolute(normalizedLocalPath) ||
7494-
normalizedLocalPath === ".." ||
7495-
normalizedLocalPath.startsWith(".." + path.sep) ||
7496-
normalizedLocalPath.includes(path.sep + ".." + path.sep)
7497-
) {
7498-
throw new Error("Invalid local_path: directory traversal is not allowed.");
7499-
}
7500-
savePath = path.join(normalizedLocalPath, filename);
7501-
} else {
7502-
savePath = filename;
7503-
}
7502+
const savePath = localPath ? path.join(path.normalize(localPath), filename) : filename;
75047503
fs.mkdirSync(path.dirname(savePath), { recursive: true });
75057504
75067505
if (!response.body) {

test/test-job-artifacts.ts

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import { spawn } from 'child_process';
44
import { MockGitLabServer, findMockServerPort } from './utils/mock-gitlab-server.js';
55
import fs from 'node:fs';
66
import path from 'node:path';
7-
import os from 'node:os';
87

98
const MOCK_TOKEN = 'glpat-mock-token-12345';
109
const TEST_PROJECT_ID = '123';
@@ -143,8 +142,12 @@ describe('job artifacts tools', () => {
143142
await mockGitLab.start();
144143
mockGitLabUrl = mockGitLab.getUrl();
145144

146-
// Create a temp directory for download tests
147-
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitlab-mcp-test-'));
145+
// Create a temp directory for download tests. Must be relative to the
146+
// process cwd — download_job_artifacts rejects absolute local_path
147+
// values as directory traversal (see downloadJobArtifacts/downloadAttachment).
148+
tmpDir = `gitlab-mcp-test-artifacts-${process.pid}`;
149+
fs.rmSync(tmpDir, { recursive: true, force: true });
150+
fs.mkdirSync(tmpDir, { recursive: true });
148151
});
149152

150153
after(async () => {
@@ -210,6 +213,44 @@ describe('job artifacts tools', () => {
210213
assert.ok(fs.existsSync(nestedLocalPath), `Directory should be created at ${nestedLocalPath}`);
211214
});
212215

216+
test('download_job_artifacts rejects local_path directory traversal', async () => {
217+
try {
218+
await callTool(
219+
'download_job_artifacts',
220+
{ project_id: TEST_PROJECT_ID, job_id: TEST_JOB_ID, local_path: '../../../tmp' },
221+
{
222+
GITLAB_API_URL: `${mockGitLabUrl}/api/v4`,
223+
GITLAB_PERSONAL_ACCESS_TOKEN: MOCK_TOKEN,
224+
}
225+
);
226+
assert.fail('Expected download_job_artifacts to reject a traversal local_path');
227+
} catch (error: any) {
228+
assert.ok(
229+
typeof error?.message === 'string' && error.message.toLowerCase().includes('traversal'),
230+
`Expected a traversal error, got: ${error?.message}`
231+
);
232+
}
233+
});
234+
235+
test('download_job_artifacts rejects absolute local_path', async () => {
236+
try {
237+
await callTool(
238+
'download_job_artifacts',
239+
{ project_id: TEST_PROJECT_ID, job_id: TEST_JOB_ID, local_path: '/tmp' },
240+
{
241+
GITLAB_API_URL: `${mockGitLabUrl}/api/v4`,
242+
GITLAB_PERSONAL_ACCESS_TOKEN: MOCK_TOKEN,
243+
}
244+
);
245+
assert.fail('Expected download_job_artifacts to reject an absolute local_path');
246+
} catch (error: any) {
247+
assert.ok(
248+
typeof error?.message === 'string' && error.message.toLowerCase().includes('traversal'),
249+
`Expected a traversal error, got: ${error?.message}`
250+
);
251+
}
252+
});
253+
213254
test('get_job_artifact_file returns file content', async () => {
214255
const result = await callTool(
215256
'get_job_artifact_file',

0 commit comments

Comments
 (0)