Skip to content

Commit 659fe06

Browse files
committed
fix: harden artifact ingestion boundaries
1 parent 13bc70f commit 659fe06

34 files changed

Lines changed: 2613 additions & 729 deletions

.github/workflows/publish-mcp-registry.yml

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ jobs:
2424
name: Publish MCP Registry
2525
runs-on: ubuntu-latest
2626
timeout-minutes: 10
27+
env:
28+
MCP_PUBLISHER_VERSION: v1.8.1
29+
MCP_PUBLISHER_ASSET: mcp-publisher_linux_amd64.tar.gz
30+
MCP_PUBLISHER_SHA256: a06c9096dcb9727c13555b6be26c7effa707b01f06a4c561ba7a3635443cf2cc
2731
steps:
2832
- name: Checkout
2933
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
@@ -64,7 +68,14 @@ jobs:
6468
- name: Install mcp-publisher
6569
run: |
6670
set -euo pipefail
67-
curl -L "https://github.com/modelcontextprotocol/registry/releases/latest/download/mcp-publisher_$(uname -s | tr '[:upper:]' '[:lower:]')_$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/').tar.gz" | tar xz mcp-publisher
71+
PUBLISHER_ARCHIVE="$RUNNER_TEMP/$MCP_PUBLISHER_ASSET"
72+
curl --fail --show-error --location --retry 3 \
73+
--output "$PUBLISHER_ARCHIVE" \
74+
"https://github.com/modelcontextprotocol/registry/releases/download/${MCP_PUBLISHER_VERSION}/${MCP_PUBLISHER_ASSET}"
75+
printf '%s %s\n' "$MCP_PUBLISHER_SHA256" "$PUBLISHER_ARCHIVE" | sha256sum --check --strict
76+
tar -xzf "$PUBLISHER_ARCHIVE" -C "$RUNNER_TEMP" mcp-publisher
77+
install -m 0755 "$RUNNER_TEMP/mcp-publisher" ./mcp-publisher
78+
./mcp-publisher --version
6879
shell: bash
6980

7081
- name: Authenticate to MCP Registry

package.json

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -245,25 +245,31 @@
245245
],
246246
"dependencies": {
247247
"@limrun/api": "^0.24.5",
248-
"yaml": "^2.9.0"
248+
"ipaddr.js": "^2.5.0",
249+
"tar-stream": "^3.2.0",
250+
"undici": "7.29.0",
251+
"yaml": "^2.9.0",
252+
"yauzl": "^3.4.0"
249253
},
250254
"devDependencies": {
251255
"@agent-device/ad-replay": "workspace:*",
252256
"@agent-device/ad-script": "workspace:*",
253257
"@agent-device/contracts": "workspace:*",
254258
"@agent-device/kernel": "workspace:*",
255-
"@agent-device/selectors": "workspace:*",
256259
"@agent-device/maestro": "workspace:*",
257260
"@agent-device/provider-limrun": "workspace:*",
258261
"@agent-device/provider-webdriver": "workspace:*",
259262
"@agent-device/replay-test": "workspace:*",
263+
"@agent-device/selectors": "workspace:*",
260264
"@agent-device/xml": "workspace:*",
261265
"@arethetypeswrong/cli": "^0.18.5",
262266
"@chenglou/freerange": "^0.0.1",
263267
"@stryker-mutator/core": "9.6.1",
264268
"@stryker-mutator/vitest-runner": "9.6.1",
265269
"@types/node": "^22.19.21",
266270
"@types/pngjs": "^6.0.5",
271+
"@types/tar-stream": "^3.1.4",
272+
"@types/yauzl": "^2.10.3",
267273
"@vitest/coverage-v8": "4.1.8",
268274
"fallow": "^2.95.0",
269275
"fast-check": "^4.9.0",

pnpm-lock.yaml

Lines changed: 292 additions & 106 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/__tests__/npm-package-scripts.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ const packagedCliWorkflow = fs.readFileSync(
1616
path.join(repoRoot, '.github', 'workflows', 'ci.yml'),
1717
'utf8',
1818
);
19+
const mcpRegistryWorkflow = fs.readFileSync(
20+
path.join(repoRoot, '.github', 'workflows', 'publish-mcp-registry.yml'),
21+
'utf8',
22+
);
1923

2024
function script(name: string): string {
2125
const value = packageJson.scripts[name];
@@ -110,3 +114,39 @@ test('publishing cannot skip the package gate', () => {
110114
/run: node --experimental-strip-types scripts\/check-package\.ts/,
111115
);
112116
});
117+
118+
test('MCP registry publishing verifies an immutable publisher before OIDC login', () => {
119+
assert.match(mcpRegistryWorkflow, /MCP_PUBLISHER_VERSION: v1\.8\.1/);
120+
assert.match(mcpRegistryWorkflow, /MCP_PUBLISHER_ASSET: mcp-publisher_linux_amd64\.tar\.gz/);
121+
assert.match(
122+
mcpRegistryWorkflow,
123+
/MCP_PUBLISHER_SHA256: a06c9096dcb9727c13555b6be26c7effa707b01f06a4c561ba7a3635443cf2cc/,
124+
);
125+
assert.doesNotMatch(mcpRegistryWorkflow, /releases\/latest/);
126+
assert.doesNotMatch(mcpRegistryWorkflow, /curl[^\n]*\|[^\n]*tar/);
127+
128+
const downloadIndex = mcpRegistryWorkflow.indexOf(
129+
'/releases/download/${MCP_PUBLISHER_VERSION}/${MCP_PUBLISHER_ASSET}',
130+
);
131+
const verificationIndex = mcpRegistryWorkflow.indexOf('sha256sum --check --strict');
132+
const extractionIndex = mcpRegistryWorkflow.indexOf('tar -xzf');
133+
const smokeCheckIndex = mcpRegistryWorkflow.indexOf('./mcp-publisher --version');
134+
const loginIndex = mcpRegistryWorkflow.indexOf('./mcp-publisher login github-oidc');
135+
const publishIndex = mcpRegistryWorkflow.indexOf('./mcp-publisher publish server.json');
136+
137+
for (const [label, index] of [
138+
['versioned download', downloadIndex],
139+
['checksum verification', verificationIndex],
140+
['archive extraction', extractionIndex],
141+
['version smoke check', smokeCheckIndex],
142+
['OIDC login', loginIndex],
143+
['registry publish', publishIndex],
144+
] as const) {
145+
assert.notEqual(index, -1, `workflow must contain ${label}`);
146+
}
147+
assert.ok(downloadIndex < verificationIndex, 'download must precede checksum verification');
148+
assert.ok(verificationIndex < extractionIndex, 'verification must precede extraction');
149+
assert.ok(extractionIndex < smokeCheckIndex, 'installation must precede the version smoke check');
150+
assert.ok(smokeCheckIndex < loginIndex, 'version smoke check must precede OIDC login');
151+
assert.ok(loginIndex < publishIndex, 'OIDC login must precede publish');
152+
});
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import assert from 'node:assert/strict';
2+
import { test } from 'vitest';
3+
import { parseUploadContentLength, parseUploadContentRange } from '../resumable-upload-range.ts';
4+
5+
test('content ranges are bounded by the declared upload size', () => {
6+
assert.deepEqual(parseUploadContentRange('bytes 2-4/5', 5), {
7+
start: 2,
8+
end: 4,
9+
size: 5,
10+
span: 3,
11+
});
12+
for (const value of ['bytes 0-5/5', 'bytes 5-5/5', 'bytes 0-0/0']) {
13+
assert.throws(() => parseUploadContentRange(value, Number(value.split('/')[1])));
14+
}
15+
});
16+
17+
test('content range and length numbers use decimal safe-integer grammar', () => {
18+
for (const value of ['+1', '1e3', '0x10', '1.5', '-1', '9007199254740992']) {
19+
assert.throws(() => parseUploadContentLength(value), value);
20+
}
21+
assert.equal(parseUploadContentLength('0'), 0);
22+
assert.equal(parseUploadContentLength('123'), 123);
23+
assert.equal(parseUploadContentLength(undefined), undefined);
24+
});

src/daemon/__tests__/resumable-upload.test.ts

Lines changed: 105 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,15 @@
1-
import { test } from 'vitest';
1+
import { test, vi } from 'vitest';
22
import assert from 'node:assert/strict';
3+
import crypto from 'node:crypto';
4+
import fs from 'node:fs';
5+
import { PassThrough, Readable } from 'node:stream';
6+
import type { IncomingMessage } from 'node:http';
37
import { AppError } from '@agent-device/kernel/errors';
4-
import { finalizeResumableUpload } from '../resumable-upload.ts';
8+
import {
9+
beginResumableUpload,
10+
finalizeResumableUpload,
11+
receiveResumableUploadChunk,
12+
} from '../resumable-upload.ts';
513

614
test('finalizing an unknown upload reports expiry with a recovery hint', async () => {
715
const error = await finalizeResumableUpload('missing-upload-id').then(
@@ -15,3 +23,98 @@ test('finalizing an unknown upload reports expiry with a recovery hint', async (
1523
assert.equal(appError.details?.reason, 'RESOURCE_EXPIRED');
1624
assert.equal(typeof appError.details?.hint, 'string');
1725
});
26+
27+
test('oversized ranged chunks roll back atomically and can be retried and finalized', async () => {
28+
const bytes = Buffer.from('ABCDE');
29+
const uploadId = beginUpload(bytes).uploadId;
30+
await assert.rejects(
31+
receiveResumableUploadChunk({
32+
uploadId,
33+
req: request(Buffer.from('ABC'), { 'content-range': 'bytes 0-1/5' }),
34+
}),
35+
/permitted byte range/i,
36+
);
37+
assert.deepEqual(
38+
await receiveResumableUploadChunk({
39+
uploadId,
40+
req: request(Buffer.from('AB'), { 'content-range': 'bytes 0-1/5' }),
41+
}),
42+
{ complete: false, offset: 2 },
43+
);
44+
await receiveResumableUploadChunk({
45+
uploadId,
46+
req: request(Buffer.from('CDE'), { 'content-range': 'bytes 2-4/5' }),
47+
});
48+
const finalized = await finalizeResumableUpload(uploadId);
49+
try {
50+
assert.equal(fs.readFileSync(finalized.artifactPath, 'utf8'), 'ABCDE');
51+
} finally {
52+
fs.rmSync(finalized.tempDir, { recursive: true, force: true });
53+
}
54+
});
55+
56+
test('an early finalize keeps the upload resumable', async () => {
57+
const bytes = Buffer.from('resume');
58+
const uploadId = beginUpload(bytes).uploadId;
59+
await assert.rejects(finalizeResumableUpload(uploadId), /incomplete/i);
60+
await receiveResumableUploadChunk({ uploadId, req: request(bytes) });
61+
const finalized = await finalizeResumableUpload(uploadId);
62+
fs.rmSync(finalized.tempDir, { recursive: true, force: true });
63+
});
64+
65+
test('per-ticket operations serialize while an earlier chunk is paused', async () => {
66+
const bytes = Buffer.from('ABCD');
67+
const uploadId = beginUpload(bytes).uploadId;
68+
const first = requestStream({ 'content-range': 'bytes 0-1/4' });
69+
const second = requestStream({ 'content-range': 'bytes 2-3/4' });
70+
const firstResult = receiveResumableUploadChunk({ uploadId, req: first });
71+
const secondResult = receiveResumableUploadChunk({ uploadId, req: second });
72+
second.end('CD');
73+
let secondSettled = false;
74+
void secondResult.finally(() => {
75+
secondSettled = true;
76+
});
77+
await Promise.resolve();
78+
assert.equal(secondSettled, false);
79+
first.end('AB');
80+
assert.deepEqual(await firstResult, { complete: false, offset: 2 });
81+
assert.deepEqual(await secondResult, { complete: true, offset: 4 });
82+
const finalized = await finalizeResumableUpload(uploadId);
83+
fs.rmSync(finalized.tempDir, { recursive: true, force: true });
84+
});
85+
86+
test('expiry aborts an active receive and invalidates the ticket after rollback', async () => {
87+
vi.useFakeTimers();
88+
try {
89+
const bytes = Buffer.from('AB');
90+
const uploadId = beginUpload(bytes).uploadId;
91+
const body = requestStream();
92+
const receiving = receiveResumableUploadChunk({ uploadId, req: body });
93+
body.write('A');
94+
await vi.advanceTimersByTimeAsync(5 * 60 * 1000);
95+
await assert.rejects(receiving, /expired/i);
96+
await assert.rejects(finalizeResumableUpload(uploadId), /not found or expired/i);
97+
} finally {
98+
vi.useRealTimers();
99+
}
100+
});
101+
102+
function beginUpload(bytes: Buffer): ReturnType<typeof beginResumableUpload> {
103+
return beginResumableUpload({
104+
baseUrl: 'http://127.0.0.1:1234',
105+
tokenHeaders: {},
106+
uploadAttemptId: crypto.randomUUID(),
107+
sha256: crypto.createHash('sha256').update(bytes).digest('hex'),
108+
fileName: 'artifact.bin',
109+
sizeBytes: bytes.length,
110+
artifactType: 'file',
111+
});
112+
}
113+
114+
function request(body: Buffer, headers: Record<string, string> = {}): IncomingMessage {
115+
return Object.assign(Readable.from(body), { headers }) as IncomingMessage;
116+
}
117+
118+
function requestStream(headers: Record<string, string> = {}): PassThrough & IncomingMessage {
119+
return Object.assign(new PassThrough(), { headers }) as PassThrough & IncomingMessage;
120+
}

src/daemon/__tests__/upload.test.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import path from 'node:path';
55
import { Readable } from 'node:stream';
66
import type { IncomingMessage } from 'node:http';
77
import { receiveUpload } from '../upload.ts';
8-
import { streamReadableToFile } from '../artifact-download.ts';
8+
import { streamReadableToFile, validateArtifactContentLength } from '../artifact-download.ts';
99
import { runCmdSync } from '../../utils/exec.ts';
1010
import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts';
1111

@@ -23,6 +23,13 @@ test('receiveUpload rejects uploads that exceed the configured content-length li
2323
await assert.rejects(async () => await receiveUpload(req), /Upload exceeds maximum size/i);
2424
});
2525

26+
test('artifact content-length accepts decimal integers only', () => {
27+
for (const value of ['+1', '1e3', '0x10', '1.5', '-1']) {
28+
assert.throws(() => validateArtifactContentLength(value), value);
29+
}
30+
assert.doesNotThrow(() => validateArtifactContentLength('0'));
31+
});
32+
2633
test('receiveUpload rejects app bundle archives containing symlinks', async () => {
2734
const tempRoot = mkdtempForTestSync('agent-device-upload-archive-');
2835
const appDir = path.join(tempRoot, 'Sample.app');

0 commit comments

Comments
 (0)