Skip to content

Commit fc897f3

Browse files
committed
feat: US-112 - Add permission check to httpServerClose
1 parent f357548 commit fc897f3

2 files changed

Lines changed: 95 additions & 4 deletions

File tree

packages/secure-exec-node/src/bridge-setup.ts

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -675,6 +675,9 @@ export async function setupRequire(
675675
},
676676
);
677677

678+
// Track server IDs created in this context for ownership validation
679+
const ownedHttpServers = new Set<number>();
680+
678681
// Lazy dispatcher reference for in-sandbox HTTP server callbacks
679682
let httpServerDispatchRef: ivm.Reference<
680683
(serverId: number, requestJson: string) => Promise<string>
@@ -730,6 +733,7 @@ export async function setupRequire(
730733
}>("network.httpServer response", String(responseJson), jsonPayloadLimit);
731734
},
732735
});
736+
ownedHttpServers.add(options.serverId);
733737
deps.activeHttpServerIds.add(options.serverId);
734738
return JSON.stringify(result);
735739
})();
@@ -738,14 +742,22 @@ export async function setupRequire(
738742

739743
// Reference for closing an in-sandbox HTTP server
740744
const networkHttpServerCloseRef = new ivm.Reference(
741-
async (serverId: number): Promise<void> => {
745+
(serverId: number): Promise<void> => {
742746
if (!adapter.httpServerClose) {
743747
throw new Error(
744748
"http.createServer close requires NetworkAdapter.httpServerClose support",
745749
);
746750
}
747-
await adapter.httpServerClose(serverId);
748-
deps.activeHttpServerIds.delete(serverId);
751+
// Ownership check: only allow closing servers created in this context
752+
if (!ownedHttpServers.has(serverId)) {
753+
throw new Error(
754+
`Cannot close server ${serverId}: not owned by this execution context`,
755+
);
756+
}
757+
return adapter.httpServerClose(serverId).then(() => {
758+
ownedHttpServers.delete(serverId);
759+
deps.activeHttpServerIds.delete(serverId);
760+
});
749761
},
750762
);
751763

packages/secure-exec/tests/runtime-driver/node/bridge-hardening.test.ts

Lines changed: 80 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import ivm from "isolated-vm";
22
import { afterEach, describe, expect, it } from "vitest";
3-
import { allowAllFs, allowAllChildProcess, createInMemoryFileSystem, createDefaultNetworkAdapter } from "../../../src/index.js";
3+
import { allowAllFs, allowAllChildProcess, allowAllNetwork, createInMemoryFileSystem, createDefaultNetworkAdapter } from "../../../src/index.js";
44
import type { NodeRuntime } from "../../../src/index.js";
55
import { createTestNodeRuntime } from "../../test-utils.js";
66

@@ -362,6 +362,85 @@ describe("bridge-side resource hardening", () => {
362362
});
363363
});
364364

365+
// -------------------------------------------------------------------
366+
// HTTP server ownership — close only servers created in this context
367+
// -------------------------------------------------------------------
368+
369+
describe("HTTP server ownership", () => {
370+
it("sandbox can close a server it created", async () => {
371+
const adapter = createDefaultNetworkAdapter();
372+
const capture = createConsoleCapture();
373+
374+
proc = createTestNodeRuntime({
375+
permissions: { ...allowAllNetwork },
376+
networkAdapter: adapter,
377+
onStdio: capture.onStdio,
378+
});
379+
380+
const result = await proc.exec(`
381+
const http = require('http');
382+
const server = http.createServer((req, res) => {
383+
res.writeHead(200);
384+
res.end('ok');
385+
});
386+
387+
(async () => {
388+
await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve));
389+
await new Promise((resolve, reject) => {
390+
server.close((err) => err ? reject(err) : resolve());
391+
});
392+
console.log('close:ok');
393+
})();
394+
`);
395+
396+
expect(result.code).toBe(0);
397+
expect(capture.stdout()).toContain("close:ok");
398+
});
399+
400+
it("sandbox cannot close a server it did not create", async () => {
401+
const adapter = createDefaultNetworkAdapter();
402+
const capture = createConsoleCapture();
403+
404+
// Pre-register a server in the adapter that was NOT created by this context
405+
await adapter.httpServerListen!({
406+
serverId: 42,
407+
port: 0,
408+
hostname: "127.0.0.1",
409+
onRequest: async () => ({ status: 200 }),
410+
});
411+
412+
proc = createTestNodeRuntime({
413+
permissions: { ...allowAllNetwork },
414+
networkAdapter: adapter,
415+
onStdio: capture.onStdio,
416+
});
417+
418+
const result = await proc.exec(`
419+
const http = require('http');
420+
421+
(async () => {
422+
try {
423+
// Attempt to call the host bridge reference directly with an unowned serverId
424+
await _networkHttpServerCloseRaw.apply(
425+
undefined, [42], { result: { promise: true } }
426+
);
427+
console.log('close:unexpected');
428+
} catch (e) {
429+
console.log('close:denied');
430+
console.log('error:' + e.message);
431+
}
432+
})();
433+
`);
434+
435+
expect(capture.stdout()).toContain("close:denied");
436+
expect(capture.stdout()).toContain("not owned by this execution context");
437+
expect(capture.stdout()).not.toContain("close:unexpected");
438+
439+
// Clean up the externally-created server
440+
await adapter.httpServerClose!(42);
441+
});
442+
});
443+
365444
// -------------------------------------------------------------------
366445
// Module cache isolation across __unsafeCreateContext calls
367446
// -------------------------------------------------------------------

0 commit comments

Comments
 (0)