Skip to content

Commit 7fdc97c

Browse files
committed
fix: close reviewer #2 findings on controlled egress engine
- run the gateway production image on node:22-slim instead of grafting an unpinned node binary (no shared-library closure) into the bun image - cover the shipped Node runtime in CI: bundle the fetch engine the way the image bundles it and re-run the real DNS/TLS/pinned-IP boundary fixture under Node (caught a real Bun/Node outbound-header divergence) - keep chunked /tool-call bodies working: only enforce body/Content-Length equality when a Content-Length was actually declared (required on /external-fetch per W730 5.2, optional on /tool-call as before) - map an unreadable egress-grant file to the stable FETCH_FAILED code instead of an unhandled PermissionError traceback
1 parent 4985662 commit 7fdc97c

8 files changed

Lines changed: 208 additions & 20 deletions

‎.github/workflows/ci.yml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,5 +95,13 @@ jobs:
9595
- name: Install dependencies
9696
run: bun ci
9797

98+
- name: Assert Node runtime for gateway parity tests
99+
# service/src/external-fetch-runtime.test.ts bundles the controlled-egress
100+
# fetch engine the way service/Dockerfile.egress-gateway does and runs it
101+
# under Node, which is what the shipped gateway image executes. It
102+
# self-skips when `node` is absent, so assert the runtime here rather
103+
# than letting the production-runtime coverage vanish silently.
104+
run: node --version
105+
98106
- name: Bun tests
99107
run: bun run test

‎api/helpers/sandbox_fetch.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ def _read_grant() -> str:
151151
try:
152152
with open(_GRANT_FILE, "r", encoding="utf-8") as grant_file:
153153
grant = grant_file.read(16_385)
154-
except FileNotFoundError:
154+
except OSError:
155155
grant = os.environ.get(_GRANT_ENV, "")
156156
if not grant.strip() or len(grant) > 16_384:
157157
raise SandboxFetchError("FETCH_FAILED")

‎api/src/sandbox-fetch-helper.test.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,4 +293,34 @@ describe('sandbox_fetch typed helper', () => {
293293
});
294294
expect(fs.readFileSync(output, 'utf8')).toBe('pdf fixture');
295295
});
296+
297+
test('maps an unreadable grant file to the stable FETCH_FAILED code', async () => {
298+
const root = workspace();
299+
const victim = path.join(root, 'egress-grant');
300+
fs.writeFileSync(victim, 'opaque-grant', { mode: 0o000 });
301+
const code = [
302+
'import sandbox_fetch, sys',
303+
`sandbox_fetch._GRANT_FILE = ${JSON.stringify(victim)}`,
304+
`sys.argv = ["sandbox_fetch.py", "--output", ${JSON.stringify(path.join(root, 'out.pdf'))}, "--url-stdin"]`,
305+
'raise SystemExit(sandbox_fetch._main())',
306+
].join(';');
307+
const child = Bun.spawn(['python3', '-c', code], {
308+
env: { ...process.env, PYTHONPATH: HELPER_DIR, SANDBOX_EGRESS_GRANT: undefined },
309+
stdin: 'pipe',
310+
stdout: 'pipe',
311+
stderr: 'pipe',
312+
});
313+
child.stdin.write('https://allowed.test/success');
314+
child.stdin.end();
315+
const [status, stdout, stderr] = await Promise.all([
316+
child.exited,
317+
new Response(child.stdout).text(),
318+
new Response(child.stderr).text(),
319+
]);
320+
expect(status).toBe(1);
321+
expect(stdout).toBe('');
322+
expect(stderr.trim()).toBe('FETCH_FAILED');
323+
expect(stderr).not.toContain('Traceback');
324+
expect(relayCalls).toBe(0);
325+
});
296326
});

‎api/src/tool-call-socket-proxy.test.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,45 @@ describe('tool-call socket proxy', () => {
119119
await upstream.close();
120120
});
121121

122+
test('preserves chunked tool-call requests without Content-Length', async () => {
123+
const upstream = await startUpstream();
124+
const socketPath = makeSocketPath();
125+
const proxy = await startToolCallSocketProxy({
126+
socketPath,
127+
rawTarget: upstream.url,
128+
log: { log() {}, warn() {}, error() {} },
129+
});
130+
handles.push(proxy);
131+
132+
const client = await createRawSocket(socketPath);
133+
const responseChunks: Buffer[] = [];
134+
const closed = Promise.withResolvers<void>();
135+
client.on('data', chunk => responseChunks.push(Buffer.from(chunk)));
136+
client.once('close', closed.resolve);
137+
const body = '{"tool_name":"safe","input":{}}';
138+
client.write([
139+
'POST /tool-call HTTP/1.1',
140+
'Host: local',
141+
'Transfer-Encoding: chunked',
142+
'X-Execution-ID: test-exec-id',
143+
'X-Tool-Call-ID: test-call-001',
144+
'X-Callback-Token: test-callback-token',
145+
'Connection: close',
146+
'',
147+
body.length.toString(16),
148+
body,
149+
'0',
150+
'',
151+
'',
152+
].join('\r\n'));
153+
await closed.promise;
154+
155+
const response = Buffer.concat(responseChunks).toString('utf8');
156+
expect(response).toContain('HTTP/1.1 200 OK');
157+
expect(response).toContain('tool_name');
158+
await upstream.close();
159+
});
160+
122161
test('destroys idle raw connections so socket probes cannot pin FDs', async () => {
123162
const upstream = await startUpstream();
124163
const socketPath = makeSocketPath();

‎api/src/tool-call-socket-proxy.ts‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -372,14 +372,22 @@ export async function startToolCallSocketProxy(
372372
}
373373

374374
const requestBodyLimit = isExternalFetch ? 16_384 : maxBodyBytes;
375-
const declaredCL = Number(req.headers['content-length'] ?? (isExternalFetch ? Number.NaN : 0));
376-
if (!Number.isSafeInteger(declaredCL) || declaredCL < 0) {
375+
const declaredHeaderValue = req.headers['content-length'];
376+
/* Content-Length is contractually required on /external-fetch (W730 §5.2 —
377+
* no transfer-encoding/content-length ambiguity), so an absent header is a
378+
* 411 there. /tool-call has always accepted a chunked body with no
379+
* Content-Length, so absence stays legal and the equality check below is
380+
* skipped rather than comparing a real body against an implied 0. */
381+
const declaredCL = declaredHeaderValue === undefined && !isExternalFetch
382+
? undefined
383+
: Number(declaredHeaderValue);
384+
if (declaredCL !== undefined && (!Number.isSafeInteger(declaredCL) || declaredCL < 0)) {
377385
req.resume();
378386
res.writeHead(411, { 'Content-Type': 'text/plain', Connection: 'close' });
379387
res.end('Content-Length is required');
380388
return;
381389
}
382-
if (declaredCL > requestBodyLimit) {
390+
if (declaredCL !== undefined && declaredCL > requestBodyLimit) {
383391
req.resume();
384392
res.writeHead(413, { 'Content-Type': 'text/plain', Connection: 'close' });
385393
res.end('request body too large');
@@ -452,7 +460,7 @@ export async function startToolCallSocketProxy(
452460
req.on('end', () => {
453461
if (rejected) return;
454462
const body = bodyChunks.length === 1 ? bodyChunks[0] : Buffer.concat(bodyChunks);
455-
if (body.length !== declaredCL) {
463+
if (declaredCL !== undefined && body.length !== declaredCL) {
456464
res.writeHead(400, { 'Content-Type': 'text/plain', Connection: 'close' });
457465
res.end('request body length mismatch');
458466
return;

‎service/Dockerfile.egress-gateway‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,17 @@ COPY service/src ./src
1313
COPY shared /shared
1414
RUN bun build ./src/egress-gateway.ts --target=node --format=cjs --outfile=./egress-gateway.cjs
1515

16-
FROM oven/bun:1.3.14 AS production
16+
# The gateway runs the pre-bundled CJS artifact under Node, not Bun: the
17+
# /external-fetch route emits an HTTP response trailer
18+
# (X-CodeAPI-Egress-Outcome) and pins the connection IP through
19+
# https.request({ lookup }), and Bun's node:http/node:https compat layer
20+
# does not carry either faithfully. The production stage therefore bases on
21+
# the Node image directly instead of grafting a node binary (and no shared
22+
# library closure) into the bun image.
23+
FROM node:22-slim AS production
1724
ENV NODE_ENV=production
1825
WORKDIR /app
1926
RUN apt-get update && apt-get install -y --no-install-recommends curl && rm -rf /var/lib/apt/lists/*
20-
COPY --from=node:22-slim /usr/local/bin/node /usr/local/bin/node
2127
COPY --from=build /app/egress-gateway.cjs ./egress-gateway.cjs
2228
EXPOSE 3190
2329
HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \

‎service/src/external-fetch-boundary.fixture.ts‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
import assert from 'node:assert/strict';
2+
import { spawnSync } from 'node:child_process';
3+
import { setTimeout as delay } from 'node:timers/promises';
24
import { Resolver } from 'node:dns/promises';
35
import dgram from 'node:dgram';
46
import fs from 'node:fs';
@@ -13,13 +15,14 @@ const HOST = 'allowed.test';
1315
const TEST_ADDRESS = '93.184.216.34';
1416
const certDir = process.env.EXTERNAL_FETCH_TLS_FIXTURE_DIR ?? '';
1517

18+
/* Runtime-agnostic on purpose: this fixture runs under Bun
19+
* (external-fetch-boundary.test.ts) and, bundled the way the gateway image
20+
* bundles it, under the production Node runtime
21+
* (external-fetch-runtime.test.ts). Neither helper may use Bun globals. */
1622
async function run(command: string[]): Promise<void> {
17-
const process = Bun.spawn(command, { stdout: 'pipe', stderr: 'pipe' });
18-
const status = await process.exited;
19-
if (status !== 0)
20-
throw new Error(
21-
`${command[0]} failed: ${await new Response(process.stderr).text()}`,
22-
);
23+
const result = spawnSync(command[0] as string, command.slice(1), { encoding: 'utf8' });
24+
if (result.status !== 0)
25+
throw new Error(`${command[0]} failed: ${result.stderr ?? ''}`);
2326
}
2427

2528
function readDnsQuestion(packet: Buffer): { name: string; end: number } {
@@ -207,12 +210,14 @@ async function main(): Promise<void> {
207210
assert.equal(opened.redirects, 0);
208211
assert.equal(opened.contentType, 'application/pdf');
209212
assert.equal(requests[0]?.method, 'GET');
210-
assert.deepEqual(requests[0]?.headers.sort(), [
211-
'accept',
212-
'accept-encoding',
213-
'host',
214-
'user-agent',
215-
]);
213+
/* `connection` is added by the HTTP client itself (agent: false, i.e. a
214+
* one-shot connection) and Bun and Node differ on whether it appears on
215+
* the wire. It carries no caller-controlled data, so the policy-relevant
216+
* assertion is that NOTHING else reaches the origin. */
217+
assert.deepEqual(
218+
requests[0]?.headers.filter(header => header !== 'connection').sort(),
219+
['accept', 'accept-encoding', 'host', 'user-agent'],
220+
);
216221

217222
const disconnectOpened = await openExternalFetch({
218223
url: `https://${HOST}/success`,
@@ -226,7 +231,7 @@ async function main(): Promise<void> {
226231
() => 'unexpected-success',
227232
error => error instanceof ExternalFetchError ? error.code : 'unexpected-error',
228233
),
229-
Bun.sleep(250).then(() => 'hung'),
234+
delay(250).then(() => 'hung'),
230235
]);
231236
assert.equal(disconnectOutcome, 'FETCH_FAILED');
232237

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
/**
2+
* Runtime parity test for the controlled-egress fetch engine.
3+
*
4+
* `external-fetch-boundary.test.ts` runs the same real-DNS/real-TLS boundary
5+
* fixture under Bun. Production does NOT run Bun: the gateway ships as a
6+
* pre-bundled CJS artifact executed by Node (service/Dockerfile.egress-gateway
7+
* `production` stage), because the /external-fetch route depends on two
8+
* behaviours Bun's node:http/node:https compat layer does not carry
9+
* faithfully — HTTP response trailers and the connection-pinning
10+
* `https.request({ lookup })` hook that IS the SSRF defence.
11+
*
12+
* So the SSRF-critical path is exercised twice: once under Bun (developer
13+
* runtime) and once here, bundled exactly the way the image bundles it and
14+
* executed by Node, so a Bun/Node divergence in DNS pinning, redirect
15+
* revalidation, header allow-listing, timeouts, or streaming ceilings fails a
16+
* check instead of shipping.
17+
*
18+
* Skipped automatically when `node` isn't on PATH or the platform isn't Linux
19+
* (the fixture needs `unshare --net` to own its loopback addresses).
20+
*/
21+
import { afterAll, expect, test } from 'bun:test';
22+
import { spawnSync } from 'node:child_process';
23+
import fs from 'node:fs';
24+
import os from 'node:os';
25+
import path from 'node:path';
26+
27+
const NODE_AVAILABLE = (() => {
28+
try {
29+
return spawnSync('node', ['--version'], { stdio: 'ignore' }).status === 0;
30+
} catch {
31+
return false;
32+
}
33+
})();
34+
const SHOULD_RUN = NODE_AVAILABLE && process.platform === 'linux';
35+
const testIfRuntime = SHOULD_RUN ? test : test.skip;
36+
37+
const workdir = fs.mkdtempSync(
38+
path.join(os.tmpdir(), 'external-fetch-runtime-'),
39+
);
40+
41+
afterAll(() => {
42+
fs.rmSync(workdir, { recursive: true, force: true });
43+
});
44+
45+
testIfRuntime(
46+
'crosses the same real boundaries when bundled and run under the production Node runtime',
47+
async () => {
48+
const serviceDir = path.resolve(__dirname, '..');
49+
const fixture = path.resolve(
50+
__dirname,
51+
'external-fetch-boundary.fixture.ts',
52+
);
53+
const fixtureDir = path.resolve(serviceDir, 'test-fixtures/external-fetch');
54+
const bundle = path.join(workdir, 'external-fetch-boundary.cjs');
55+
56+
const build = Bun.spawnSync(
57+
[
58+
'bun',
59+
'build',
60+
fixture,
61+
'--target=node',
62+
'--format=cjs',
63+
`--outfile=${bundle}`,
64+
],
65+
{ cwd: serviceDir, stdout: 'pipe', stderr: 'pipe' },
66+
);
67+
expect(build.exitCode, build.stderr.toString()).toBe(0);
68+
expect(fs.existsSync(bundle)).toBe(true);
69+
70+
const child = Bun.spawn(
71+
['unshare', '--user', '--map-root-user', '--net', 'node', bundle],
72+
{
73+
cwd: serviceDir,
74+
env: {
75+
...process.env,
76+
NODE_EXTRA_CA_CERTS: path.join(fixtureDir, 'cert.pem'),
77+
EXTERNAL_FETCH_TLS_FIXTURE_DIR: fixtureDir,
78+
},
79+
stdout: 'pipe',
80+
stderr: 'pipe',
81+
},
82+
);
83+
const [status, stdout, stderr] = await Promise.all([
84+
child.exited,
85+
new Response(child.stdout).text(),
86+
new Response(child.stderr).text(),
87+
]);
88+
expect(status, stderr).toBe(0);
89+
expect(stdout).toContain('EXTERNAL_FETCH_TLS_BOUNDARY_OK');
90+
},
91+
60_000,
92+
);

0 commit comments

Comments
 (0)