Skip to content

Commit e51c5dd

Browse files
committed
Fix benchmark safety review issues
Make the shared benchmark fixture work under Node and Bun by replacing the Deno-only test server with a runtime-neutral node:http bridge. Use the resolved target tier when gating same-origin inbox destinations, and apply the unsafe-target bounds to separate public inbox destinations before signed load can be sent to them. #744 #784 Assisted-by: Codex:gpt-5.5
1 parent 8c48bcd commit e51c5dd

5 files changed

Lines changed: 242 additions & 32 deletions

File tree

packages/cli/src/bench/action.test.ts

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,82 @@ scenarios:
314314
}
315315
});
316316

317+
test("runBench - unsafe public inbox destination needs an explicit CLI target", async () => {
318+
const target = await spawnBenchmarkTarget();
319+
try {
320+
const file = await writeSuite(`version: 1
321+
target: ${target.url.href}
322+
scenarios:
323+
- name: inbox-shared
324+
type: inbox
325+
recipient: "${new URL("/users/alice", target.url).href}"
326+
inbox: "https://prod.example/inbox"
327+
load: { rate: 1/s }
328+
duration: 1ms
329+
`);
330+
let code = -1;
331+
let message = "";
332+
await runBench(
333+
command({
334+
scenario: file,
335+
allowUnsafeTarget: true,
336+
advertiseHost: "127.0.0.1",
337+
}),
338+
{
339+
exit: (c) => {
340+
code = c;
341+
},
342+
writeOutput: () => Promise.resolve(),
343+
log: (m) => {
344+
message = m;
345+
},
346+
},
347+
);
348+
assert.strictEqual(code, 2);
349+
assert.match(message, /--target/);
350+
} finally {
351+
await target.close();
352+
}
353+
});
354+
355+
test("runBench - unsafe public inbox destination needs explicit load", async () => {
356+
const target = await spawnBenchmarkTarget();
357+
try {
358+
const file = await writeSuite(`version: 1
359+
target: ${target.url.href}
360+
scenarios:
361+
- name: inbox-shared
362+
type: inbox
363+
recipient: "${new URL("/users/alice", target.url).href}"
364+
inbox: "https://prod.example/inbox"
365+
duration: 1ms
366+
`);
367+
let code = -1;
368+
let message = "";
369+
await runBench(
370+
command({
371+
scenario: file,
372+
target: target.url.href,
373+
allowUnsafeTarget: true,
374+
advertiseHost: "127.0.0.1",
375+
}),
376+
{
377+
exit: (c) => {
378+
code = c;
379+
},
380+
writeOutput: () => Promise.resolve(),
381+
log: (m) => {
382+
message = m;
383+
},
384+
},
385+
);
386+
assert.strictEqual(code, 2);
387+
assert.match(message, /load/);
388+
} finally {
389+
await target.close();
390+
}
391+
});
392+
317393
test("runBench - malformed expect assertion exits 2 before any load", async () => {
318394
// The expect typo must be caught in preflight, so the run exits 2 (a config
319395
// error) without ever probing the target or sending load.

packages/cli/src/bench/action.ts

Lines changed: 69 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import {
3030
} from "./safety/gate.ts";
3131
import {
3232
classifyResolvedTarget,
33+
classifyTarget,
3334
type ResolveTargetAddresses,
3435
} from "./safety/tiers.ts";
3536
import { runnerFor } from "./scenarios/registry.ts";
@@ -164,13 +165,24 @@ export default async function runBench(
164165

165166
// Gates each resolved inbox destination (which can differ from the suite
166167
// target) before the runner sends load to it.
167-
const assertDestinationAllowed = (url: URL): void =>
168+
const assertDestinationAllowed = (
169+
url: URL,
170+
scenario: ResolvedScenario,
171+
): void => {
168172
assertInboxDestinationAllowed(url, {
169173
targetOrigin: suite.target.origin,
174+
targetTier: tier,
170175
targetBenchmarkMode: probe.benchmarkMode,
171176
allowUnsafe: command.allowUnsafeTarget,
172177
advertised: command.advertiseHost != null,
173178
});
179+
assertPublicDestinationOverrideAllowed(url, scenario, {
180+
targetOrigin: suite.target.origin,
181+
targetBenchmarkMode: probe.benchmarkMode,
182+
allowUnsafe: command.allowUnsafeTarget,
183+
explicitCliTarget: command.target != null,
184+
});
185+
};
174186

175187
if (command.dryRun) {
176188
try {
@@ -229,7 +241,8 @@ export default async function runBench(
229241
allowPrivateAddress,
230242
fleet: fleet ?? null,
231243
fetch: fetchImpl,
232-
assertDestinationAllowed,
244+
assertDestinationAllowed: (url) =>
245+
assertDestinationAllowed(url, scenario),
233246
});
234247
results.push(buildScenarioResult(scenario, measurement));
235248
}
@@ -321,7 +334,10 @@ interface DryRunPlanContext {
321334
readonly documentLoader: DocumentLoader;
322335
readonly contextLoader: DocumentLoader;
323336
readonly allowPrivateAddress: boolean;
324-
readonly assertDestinationAllowed: (url: URL) => void;
337+
readonly assertDestinationAllowed: (
338+
url: URL,
339+
scenario: ResolvedScenario,
340+
) => void;
325341
}
326342

327343
async function renderPlan(
@@ -387,7 +403,9 @@ async function describeInboxDiscoveryPlan(
387403
`inbox ${inbox.href}`,
388404
);
389405
lines.push(
390-
` destination safety: ${describeDestinationSafety(inbox, context)}`,
406+
` destination safety: ${
407+
describeDestinationSafety(inbox, scenario, context)
408+
}`,
391409
);
392410
}
393411
return lines;
@@ -410,10 +428,11 @@ function describeWebFingerPlan(
410428

411429
function describeDestinationSafety(
412430
inbox: URL,
431+
scenario: ResolvedScenario,
413432
context: DryRunPlanContext,
414433
): string {
415434
try {
416-
context.assertDestinationAllowed(inbox);
435+
context.assertDestinationAllowed(inbox, scenario);
417436
return "allowed";
418437
} catch (error) {
419438
if (error instanceof UnsafeTargetError) {
@@ -423,16 +442,55 @@ function describeDestinationSafety(
423442
}
424443
}
425444

445+
interface PublicDestinationOverrideContext {
446+
readonly targetOrigin: string;
447+
readonly targetBenchmarkMode: boolean;
448+
readonly allowUnsafe: boolean;
449+
readonly explicitCliTarget: boolean;
450+
}
451+
452+
function assertPublicDestinationOverrideAllowed(
453+
url: URL,
454+
scenario: ResolvedScenario,
455+
context: PublicDestinationOverrideContext,
456+
): void {
457+
const inheritsTargetGate = url.origin === context.targetOrigin &&
458+
context.targetBenchmarkMode;
459+
if (
460+
classifyTarget(url) !== "public" || inheritsTargetGate ||
461+
!context.allowUnsafe
462+
) {
463+
return;
464+
}
465+
assertUnsafeOverrideAllowed({
466+
tier: "public",
467+
benchmarkMode: false,
468+
allowUnsafe: true,
469+
explicitCliTarget: context.explicitCliTarget,
470+
scenarios: [unsafeOverrideScenario(scenario)],
471+
});
472+
}
473+
426474
function unsafeOverrideScenarios(
427475
suite: Suite,
428476
): Parameters<typeof assertUnsafeOverrideAllowed>[0]["scenarios"] {
429-
const defaultDuration = suite.defaults?.duration != null;
430-
const defaultLoad = hasExplicitLoad(suite.defaults?.load);
431-
return suite.scenarios.map((scenario) => ({
477+
return suite.scenarios.map((scenario) =>
478+
unsafeOverrideScenario(scenario, suite)
479+
);
480+
}
481+
482+
function unsafeOverrideScenario(
483+
scenario: ResolvedScenario | Suite["scenarios"][number],
484+
suite?: Suite,
485+
): Parameters<typeof assertUnsafeOverrideAllowed>[0]["scenarios"][number] {
486+
const defaultDuration = suite?.defaults?.duration != null;
487+
const defaultLoad = hasExplicitLoad(suite?.defaults?.load);
488+
const raw = "raw" in scenario ? scenario.raw : scenario;
489+
return {
432490
name: scenario.name,
433-
explicitDuration: scenario.duration != null || defaultDuration,
434-
explicitLoad: hasExplicitLoad(scenario.load) || defaultLoad,
435-
}));
491+
explicitDuration: raw.duration != null || defaultDuration,
492+
explicitLoad: hasExplicitLoad(raw.load) || defaultLoad,
493+
};
436494
}
437495

438496
function hasExplicitLoad(load: LoadConfig | undefined): boolean {

packages/cli/src/bench/safety/gate.test.ts

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@ function destContext(
146146
) {
147147
return {
148148
targetOrigin: "http://127.0.0.1:3000",
149+
targetTier: "loopback" as const,
149150
targetBenchmarkMode: false,
150151
allowUnsafe: false,
151152
advertised: false,
@@ -199,6 +200,22 @@ test("assertInboxDestinationAllowed - an inbox on the target origin inherits its
199200
);
200201
});
201202

203+
test("assertInboxDestinationAllowed - same-origin inbox uses the resolved target tier", () => {
204+
// The target hostname may be syntactically public but DNS-resolved private.
205+
// The discovered same-origin inbox should inherit that resolved tier instead
206+
// of being reclassified from the hostname string.
207+
assert.doesNotThrow(() =>
208+
assertInboxDestinationAllowed(
209+
new URL("https://staging.example/inbox"),
210+
destContext({
211+
targetOrigin: "https://staging.example",
212+
targetTier: "private",
213+
advertised: true,
214+
}),
215+
)
216+
);
217+
});
218+
202219
test("assertInboxDestinationAllowed - same host, different scheme does not inherit", () => {
203220
// The target is https (its benchmark-mode probe covered port 443); an http
204221
// inbox on the same hostname is a different service (port 80), so it must not
@@ -225,7 +242,10 @@ test("assertInboxDestinationAllowed - a non-loopback inbox needs an advertised h
225242
() =>
226243
assertInboxDestinationAllowed(
227244
new URL("http://10.0.0.5:8000/inbox"),
228-
destContext({ targetOrigin: "http://10.0.0.5:8000" }),
245+
destContext({
246+
targetOrigin: "http://10.0.0.5:8000",
247+
targetTier: "private",
248+
}),
229249
),
230250
(error: unknown) =>
231251
error instanceof UnsafeTargetError &&
@@ -234,7 +254,11 @@ test("assertInboxDestinationAllowed - a non-loopback inbox needs an advertised h
234254
assert.doesNotThrow(() =>
235255
assertInboxDestinationAllowed(
236256
new URL("http://10.0.0.5:8000/inbox"),
237-
destContext({ targetOrigin: "http://10.0.0.5:8000", advertised: true }),
257+
destContext({
258+
targetOrigin: "http://10.0.0.5:8000",
259+
targetTier: "private",
260+
advertised: true,
261+
}),
238262
)
239263
);
240264
});

packages/cli/src/bench/safety/gate.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,8 @@ export interface InboxDestinationGateContext {
122122
* (e.g. an `http://host` inbox does not inherit an `https://host` target).
123123
*/
124124
readonly targetOrigin: string;
125+
/** The resolved target tier used by the main safety gate. */
126+
readonly targetTier: TargetTier;
125127
/** Whether the gated target advertises benchmark mode. */
126128
readonly targetBenchmarkMode: boolean;
127129
/** Whether `--allow-unsafe-target` was given. */
@@ -150,8 +152,9 @@ export function assertInboxDestinationAllowed(
150152
url: URL,
151153
context: InboxDestinationGateContext,
152154
): void {
153-
const tier = classifyTarget(url);
154-
const inheritsTargetGate = url.origin === context.targetOrigin &&
155+
const sameOrigin = url.origin === context.targetOrigin;
156+
const tier = sameOrigin ? context.targetTier : classifyTarget(url);
157+
const inheritsTargetGate = sameOrigin &&
155158
context.targetBenchmarkMode;
156159
if (tier === "public" && !inheritsTargetGate && !context.allowUnsafe) {
157160
throw new UnsafeTargetError(

0 commit comments

Comments
 (0)