Skip to content

Commit fa86236

Browse files
committed
fix: secure routed browser command boundaries
1 parent b431b7b commit fa86236

14 files changed

Lines changed: 284 additions & 83 deletions

.github/workflows/agent-task-contracts.yml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@ on:
1414
- "fixtures/agent-task-runtime-paths-run-29305012941.json"
1515
- "package-lock.json"
1616
- "package.json"
17+
- "packages/runtime-playground/src/**"
18+
- "tests/browser-*.test.ts"
19+
- "tests/editor-*.test.ts"
1720
- "tests/agent-task-*.test.ts"
1821
- "tests/runtime-sources-materialization.test.ts"
1922
- "tests/runtime-sources-playground-integration.test.ts"
@@ -58,6 +61,9 @@ on:
5861
- "fixtures/agent-task-runtime-paths-run-29305012941.json"
5962
- "package-lock.json"
6063
- "package.json"
64+
- "packages/runtime-playground/src/**"
65+
- "tests/browser-*.test.ts"
66+
- "tests/editor-*.test.ts"
6167
- "tests/agent-task-*.test.ts"
6268
- "tests/runtime-sources-materialization.test.ts"
6369
- "tests/runtime-sources-playground-integration.test.ts"
@@ -130,6 +136,7 @@ jobs:
130136
- run: npm run test:runtime-command-artifact-bounds
131137
- run: npm run test:redaction
132138
- run: npm run test:browser-preview-routing
139+
- run: npm run test:browser-routed-command-security
133140
- run: npm run test:production-boundary-enforcement
134141
- run: npm run test:runtime-tool-policy
135142

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@
109109
"smoke": "tsx scripts/run-smoke.ts",
110110
"test:redaction": "tsx tests/redaction.test.ts",
111111
"test:browser-preview-routing": "tsx --test tests/browser-preview-routing.test.ts",
112+
"test:browser-routed-command-security": "tsx --test tests/browser-routed-command-security.test.ts",
112113
"test:cloudflare-runtime": "node --test tests/cloudflare-d1-provisioner.test.mjs && tsx tests/cloudflare-site-context.test.ts && tsx tests/cloudflare-coordinator-site-partitioning.test.ts && tsx tests/cloudflare-d1-operation-repository.test.ts && tsx tests/cloudflare-provisioning-api.test.ts && tsx tests/cloudflare-runtime.test.ts && node ./node_modules/typescript/bin/tsc -p packages/runtime-cloudflare --noEmit",
113114
"test:cloudflare-administrator-claim": "tsx tests/cloudflare-provisioning-api.test.ts",
114115
"test:cloudflare-wordpress-auth": "tsx tests/cloudflare-wordpress-auth.test.ts",

packages/runtime-playground/src/browser-actions-runner.ts

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@ import { browserAssertionsSummary, browserStepRecord, executeBrowserInteractionS
1212
import { browserCommandLivenessPolicy, isBrowserCommandLivenessError, withBrowserCommandLiveness } from "./browser-liveness.js"
1313
import { serializeBrowserError } from "./browser-metrics.js"
1414
import { executeBrowserObservationAssertion } from "./browser-observation-assertions.js"
15-
import { browserPreviewNetworkPolicyIsActive, browserPreviewNetworkPolicySummary, browserPreviewNeedsContextRouting, browserPreviewReadinessError, browserPreviewTopology, resolveBrowserPreviewUrl, routeBrowserPreviewContextNetwork } from "./browser-preview-routing.js"
15+
import { browserPreviewNetworkPolicyIsActive, browserPreviewNetworkPolicySummary, browserPreviewNeedsContextRouting, browserPreviewReadinessError, browserPreviewTopology, closeBrowserAndDrainPreviewRoutes, createBrowserPreviewRouteTracker, resolveBrowserPreviewUrl, routeBrowserPreviewContextNetwork } from "./browser-preview-routing.js"
16+
import { browserCommandResult } from "./browser-result-sanitization.js"
1617
import { BROWSER_PROBE_STATE_INIT_SCRIPT, browserProbeReplayability, browserProbeViewport } from "./browser-probe.js"
1718
import { runBrowserProbeCommand, type BrowserProbeRunPlan } from "./browser-probe-runner.js"
1819
import { browserActionTargetUrls, browserAuthRequest, browserProbeWaterfallArtifact, browserProbeWebSocketArtifact, browserProbeWebSocketSummary, browserRedirectDiagnosticsArtifact, browserRequestCoverageArtifact, browserStorageStateAuthSummary, browserStorageStateImportFromArgs, browserWordPressDiagnosticsArtifact, createBrowserProbeProgressTracker, fileSha256, installBrowserWordPressDiagnostics, installWordPressAdminAuthCookies, livenessRemainingWallTimeMs, normalizeBrowserProbeScriptCheckpoint, type BrowserCommandProgressEvent, type BrowserStorageStateImport } from "./browser-probe-support.js"
@@ -133,6 +134,7 @@ export async function runBrowserActionsCommand({
133134
const browser = session?.browser ?? await launchChromiumBrowser()
134135
const topology = browserPreviewTopology(args, runtimeSpec, server.serverUrl, server.previewProxyDiagnostics?.targetOrigin)
135136
const { preview, networkPolicy } = topology
137+
const routeTracker = createBrowserPreviewRouteTracker()
136138
let requestedUrl = initialUrl ? topology.resolveUrl(initialUrl) : preview.effectiveOrigin
137139
let finalUrl = requestedUrl
138140
let htmlSha256: string | undefined
@@ -173,7 +175,7 @@ export async function runBrowserActionsCommand({
173175
}) : undefined)
174176
const context = environmentRuntime?.context ?? null
175177
if (context && !session) {
176-
await routeBrowserPreviewContextNetwork(context, networkPolicy, topology.origins.localProxyOrigin)
178+
await routeBrowserPreviewContextNetwork(context, networkPolicy, topology.origins.localProxyOrigin, routeTracker)
177179
}
178180
const page = activePage = environmentRuntime?.page ?? await browser.newPage()
179181
if (onProgress) {
@@ -437,11 +439,12 @@ export async function runBrowserActionsCommand({
437439
pendingError = error instanceof Error ? error : new Error(String(error))
438440
errors.push(serializeBrowserError("probe-error", error))
439441
} finally {
440-
await settleBrowserNetworkTasks(networkTasks, livenessPolicy.networkSettleTimeoutMs)
442+
await settleBrowserNetworkTasks(networkTasks, livenessPolicy.networkSettleTimeoutMs).catch((error) => errors.push(serializeBrowserError("probe-error", error)))
441443
if (activePage && resolvedEnvironment) environmentEvidence = await observePlaywrightBrowserEnvironment(activePage, requestedEnvironment, resolvedEnvironment).catch(() => environmentEvidence)
442-
if (!session) {
443-
await environmentRuntime?.close().catch(() => undefined)
444-
await browser.close()
444+
const cleanupBrowser = session ? { close: async () => {} } : { close: async () => { await environmentRuntime?.close(); await browser.close() } }
445+
for (const routeError of await closeBrowserAndDrainPreviewRoutes(cleanupBrowser, routeTracker)) {
446+
errors.push(serializeBrowserError("probe-error", routeError))
447+
pendingError ??= routeError
445448
}
446449
if (capture.has("steps")) {
447450
await artifactSession.writeJsonLines("steps", "steps.jsonl", stepRecords)
@@ -580,9 +583,7 @@ export async function runBrowserActionsCommand({
580583
throw new Error("wordpress.browser-actions did not produce a browser artifact")
581584
}
582585

583-
return {
584-
artifact,
585-
output: `${JSON.stringify({
586+
return browserCommandResult(artifact, {
586587
command: "wordpress.browser-actions",
587588
requestedUrl,
588589
preview,
@@ -591,8 +592,7 @@ export async function runBrowserActionsCommand({
591592
files: artifact.files,
592593
summary: artifact.summary,
593594
steps: stepRecords,
594-
}, null, 2)}\n`,
595-
}
595+
})
596596
}
597597

598598
export function browserToolVerifierUnsupportedResult(step: BrowserInteractionStep, index: number, startedAt: string): BrowserToolVerifierResult {
@@ -1067,17 +1067,14 @@ export async function runBrowserScenarioCommand({
10671067
throw new BrowserCommandArtifactError(`wordpress.browser-scenario failed: ${pendingError.message}`, artifact)
10681068
}
10691069

1070-
return {
1071-
artifact,
1072-
output: `${JSON.stringify({
1070+
return browserCommandResult(artifact, {
10731071
command: "wordpress.browser-scenario",
10741072
requestedUrl: artifact.requestedUrl,
10751073
finalUrl,
10761074
files: artifact.files,
10771075
summary: artifact.summary,
10781076
scenario: scenarioSummary,
1079-
}, null, 2)}\n`,
1080-
}
1077+
})
10811078
}
10821079

10831080
async function browserScenarioFromArgs(args: string[]): Promise<BrowserScenarioInput> {

packages/runtime-playground/src/browser-artifact-session.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { basename, join } from "node:path"
33
import type { ArtifactProvenanceMetadata } from "@automattic/wp-codebox-core"
44
import { ArtifactBundleWriter } from "./artifact-bundle-writer.js"
55
import { browserArtifactFileManifest, type BrowserArtifactFiles } from "./browser-artifacts.js"
6+
import { sanitizeBrowserResultValue } from "./browser-result-sanitization.js"
67

78
export class BrowserArtifactSession {
89
readonly writer: ArtifactBundleWriter
@@ -32,11 +33,11 @@ export class BrowserArtifactSession {
3233
}
3334

3435
async writeJson(key: keyof BrowserArtifactFiles, fileName: string, value: unknown): Promise<void> {
35-
await this.writer.writeJson(this.path(fileName), value, this.manifestWithoutContentType(key))
36+
await this.writer.writeJson(this.path(fileName), sanitizeBrowserResultValue(value), this.manifestWithoutContentType(key))
3637
}
3738

3839
async writeJsonLines(key: keyof BrowserArtifactFiles, fileName: string, records: unknown[]): Promise<void> {
39-
await this.writer.writeJsonLines(this.path(fileName), records, this.manifestWithoutContentType(key))
40+
await this.writer.writeJsonLines(this.path(fileName), sanitizeBrowserResultValue(records), this.manifestWithoutContentType(key))
4041
}
4142

4243
async writeGenerated(key: keyof BrowserArtifactFiles, fileName: string, write: (absolutePath: string) => Promise<void>): Promise<void> {

packages/runtime-playground/src/browser-command-artifact-error.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,14 @@
11
import type { BrowserArtifact } from "./browser-artifacts.js"
2+
import { sanitizeBrowserArtifact, sanitizeBrowserResultValue } from "./browser-result-sanitization.js"
23

34
export class BrowserCommandArtifactError extends Error {
4-
constructor(message: string, readonly artifact: BrowserArtifact, readonly artifactRoot?: string) {
5-
super(message)
5+
readonly artifact: BrowserArtifact
6+
7+
constructor(message: string, artifact: BrowserArtifact, readonly artifactRoot?: string) {
8+
super(sanitizeBrowserResultValue(message, "message"))
69
this.name = "BrowserCommandArtifactError"
10+
Object.assign(artifact, sanitizeBrowserArtifact(artifact))
11+
this.artifact = artifact
712
}
813
}
914

packages/runtime-playground/src/browser-multi-actor-scenario-runner.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@ import { BrowserCommandArtifactError } from "./browser-command-artifact-error.js
66
import { attachBrowserCaptureListeners, launchChromiumBrowser, settleBrowserNetworkTasks } from "./browser-capture-session.js"
77
import { executeBrowserInteractionStep } from "./browser-interactions.js"
88
import { browserProbeReplayability } from "./browser-probe.js"
9-
import { browserPreviewReadinessError, browserPreviewTopology, routeBrowserPreviewContextNetwork } from "./browser-preview-routing.js"
9+
import { browserPreviewReadinessError, browserPreviewTopology, closeBrowserAndDrainPreviewRoutes, createBrowserPreviewRouteTracker, routeBrowserPreviewContextNetwork } from "./browser-preview-routing.js"
10+
import { browserCommandResult } from "./browser-result-sanitization.js"
1011
import { installWordPressAdminAuthCookies } from "./browser-probe-support.js"
1112
import { bootstrapPhpCode } from "./php-bootstrap.js"
1213
import { assertPlaygroundResponseOk, type PlaygroundRunResponse } from "./playground-command-errors.js"
@@ -33,6 +34,7 @@ export async function runBrowserMultiActorScenarioCommand(input: {
3334
const browserArgs = routeHost && !scenario.browserArgs.some((arg) => arg.startsWith("route-host=")) ? [...scenario.browserArgs, `route-host=${routeHost}`] : scenario.browserArgs
3435
const topology = browserPreviewTopology(browserArgs, runtimeSpec, server.serverUrl, server.previewProxyDiagnostics?.targetOrigin)
3536
const browser = await launchChromiumBrowser()
37+
const routeTracker = createBrowserPreviewRouteTracker()
3638
const evidence: Record<string, ActorEvidence> = {}
3739
let result: BrowserMultiActorScenarioResult | undefined
3840
let failure: Error | undefined
@@ -56,7 +58,7 @@ export async function runBrowserMultiActorScenarioCommand(input: {
5658
const userId = await actorUserId(actor.name, session.user.userId, session.user, runtimeSpec, runPlaygroundCommand, server)
5759
const environmentRuntime = await createPlaywrightBrowserEnvironmentContext(browser, resolvedEnvironment, { contextOptions: topology.contextOptions() })
5860
const context = environmentRuntime.context
59-
await routeBrowserPreviewContextNetwork(context, topology.networkPolicy, topology.origins.localProxyOrigin)
61+
await routeBrowserPreviewContextNetwork(context, topology.networkPolicy, topology.origins.localProxyOrigin, routeTracker)
6062
const page = environmentRuntime.page
6163
await context.tracing.start({ screenshots: true, snapshots: true })
6264
await installWordPressAdminAuthCookies({ command: "wordpress.browser-scenario", cookieUrls: topology.authCookieUrls([topology.resolveUrl(scenario.url)]), page, runPlaygroundCommand, runtimeSpec, server, userId })
@@ -73,7 +75,8 @@ export async function runBrowserMultiActorScenarioCommand(input: {
7375
failure = error instanceof Error ? error : new Error(String(error))
7476
if (error instanceof BrowserMultiActorScenarioError) result = error.result
7577
} finally {
76-
await browser.close()
78+
const routeErrors = await closeBrowserAndDrainPreviewRoutes(browser, routeTracker)
79+
failure ??= routeErrors[0]
7780
}
7881

7982
const replay = result?.replay ?? { schema: "wp-codebox/browser-multi-actor-replay/v1", seed: scenario.seed, scenario, schedule: [] }
@@ -91,7 +94,7 @@ export async function runBrowserMultiActorScenarioCommand(input: {
9194
const traces = Object.values(evidence).map((actor) => actor.files.trace).filter((path): path is string => Boolean(path))
9295
const artifact = { artifactType: "scenario" as const, requestedUrl: target, url: target, preview: topology.preview, ...topology.origins, files: { summary: "files/browser/multi-actor-scenario-summary.json", steps: "files/browser/multi-actor-events.json", network: "files/browser/multi-actor-network.json", requestCoverage: "files/browser/multi-actor-request-coverage.json", waterfall: "files/browser/multi-actor-waterfall.json", ...(traces.length > 0 ? { traces } : {}) }, summary: { actions: scenario.actions.length, steps: scenario.actions.length, consoleMessages: Object.values(evidence).reduce((total, actor) => total + actor.console.length, 0), errors: Object.values(evidence).reduce((total, actor) => total + actor.errors.length, 0), finalUrl: target, htmlSnapshot: false, networkEvents: network.length, replayability: browserProbeReplayability(captures), screenshot: captures.has("screenshot"), viewport: null, environment: Object.values(evidence)[0]?.environment, multiActor: { seed: scenario.seed, finalState: result?.finalState ?? "failed", actors: Object.keys(evidence), replay: "files/browser/multi-actor-replay.json" } } } satisfies BrowserArtifact
9396
if (failure) throw new BrowserCommandArtifactError(`wordpress.browser-scenario failed: ${failure.message}`, artifact)
94-
return { artifact, output: `${JSON.stringify({ command: "wordpress.browser-scenario", files: artifact.files, summary: artifact.summary, scenario: summary }, null, 2)}\n` }
97+
return browserCommandResult(artifact, { command: "wordpress.browser-scenario", files: artifact.files, summary: artifact.summary, scenario: summary })
9598
}
9699

97100
export async function navigateBrowserMultiActorPages(

packages/runtime-playground/src/browser-preview-routing.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,22 @@ export async function drainBrowserPreviewRouteTracker(tracker: BrowserPreviewRou
350350
}
351351
}
352352

353+
export async function closeBrowserAndDrainPreviewRoutes(browser: Pick<import("playwright").Browser, "close">, tracker: BrowserPreviewRouteTracker): Promise<Error[]> {
354+
const errors: Error[] = []
355+
try {
356+
await browser.close()
357+
} catch (error) {
358+
errors.push(browserPreviewLifecycleError("browser-close", error))
359+
} finally {
360+
try {
361+
await drainBrowserPreviewRouteTracker(tracker)
362+
} catch (error) {
363+
errors.push(browserPreviewLifecycleError("route-drain", error))
364+
}
365+
}
366+
return errors
367+
}
368+
353369
function browserPreviewMode(args: string[], publicOrigin: string | undefined): BrowserProbePreviewMode {
354370
const raw = argValue(args, "preview-mode")?.trim() || (publicOrigin ? "public" : "local")
355371
if (raw === "local" || raw === "public" || raw === "secure") {
@@ -641,6 +657,13 @@ function browserPreviewRouteRequestSummary(route: Route): { method: string; reso
641657
}
642658
}
643659

660+
function browserPreviewLifecycleError(operation: string, error: unknown): Error {
661+
const cause = sanitizeBrowserPreviewRouteError(error).message.replace(/[\r\n]+/g, " ")
662+
const diagnostic = new Error(`wordpress.browser-probe route lifecycle failed: operation=${operation} cause=${cause}`)
663+
diagnostic.name = "BrowserPreviewRouteLifecycleError"
664+
return diagnostic
665+
}
666+
644667
function browserPreviewRouteFetchExhaustedError(route: Route, requestUrl: URL, attempts: number, error: unknown): Error {
645668
const method = route.request().method()
646669
const resourceType = route.request().resourceType()

packages/runtime-playground/src/browser-probe-runner.ts

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { attachBrowserCaptureListeners, chromiumBrowserMetadata, launchChromiumB
66
import { browserCommandLivenessPolicy, isBrowserCommandLivenessError, withBrowserCommandLiveness } from "./browser-liveness.js"
77
import { browserProbeLifecycleArtifact, browserProbeLifecycleInitScript, collectBrowserProbeLifecycle } from "./browser-lifecycle.js"
88
import { browserProbeBenchMetrics, serializeBrowserError } from "./browser-metrics.js"
9-
import { browserPreviewNetworkPolicyIsActive, browserPreviewNetworkPolicySummary, browserPreviewNeedsContextRouting, browserPreviewReadinessError, browserPreviewSecureContextError, browserPreviewTopology, createBrowserPreviewRouteTracker, drainBrowserPreviewRouteTracker, routeBrowserPreviewContextNetwork, routeBrowserPreviewPageNetwork } from "./browser-preview-routing.js"
9+
import { browserPreviewNetworkPolicyIsActive, browserPreviewNetworkPolicySummary, browserPreviewNeedsContextRouting, browserPreviewReadinessError, browserPreviewSecureContextError, browserPreviewTopology, closeBrowserAndDrainPreviewRoutes, createBrowserPreviewRouteTracker, drainBrowserPreviewRouteTracker, routeBrowserPreviewContextNetwork, routeBrowserPreviewPageNetwork } from "./browser-preview-routing.js"
1010
import { BROWSER_PROBE_PERFORMANCE_INIT_SCRIPT, BROWSER_PROBE_STATE_INIT_SCRIPT, browserProbeAssertionsFromArgs, browserProbeCheckpoint, browserProbeMemoryArtifact, browserProbePendingCheckpoints, browserProbePerformanceArtifact, browserProbeViewport, executeBrowserProbeAssertions, navigateBrowserProbe } from "./browser-probe.js"
1111
import { argValue, commaListArg, durationArg, strictBooleanArg, viewportArg } from "./commands.js"
1212
import type { PlaygroundRunResponse } from "./playground-command-errors.js"
@@ -497,21 +497,28 @@ export async function runSingleBrowserProbeCommand({
497497
}
498498
}
499499
}
500-
await settleBrowserNetworkTasks(networkTasks, livenessPolicy.networkSettleTimeoutMs)
501-
await geolocationPermissionCleanup?.()
502-
if (!session) {
503-
await environmentRuntime?.close().catch(() => undefined)
504-
await browser.close()
500+
try {
501+
await settleBrowserNetworkTasks(networkTasks, livenessPolicy.networkSettleTimeoutMs)
502+
} catch (error) {
503+
errors.push(serializeBrowserError("probe-error", error))
505504
}
506505
try {
507-
await drainBrowserPreviewRouteTracker(routeTracker)
506+
await geolocationPermissionCleanup?.()
508507
} catch (error) {
509-
const routeError = redactError(error, { redactAllUrlQueryValues: true, redactUrlHash: true, redactQueryAssignments: true })
510-
if (!pendingError && runPlan.routeHostDrain === "required") {
508+
const cleanupError = redactError(error, { redactAllUrlQueryValues: true, redactUrlHash: true, redactQueryAssignments: true })
509+
if (!pendingError) {
510+
pendingError = cleanupError
511+
progress.fail("probe-error", cleanupError)
512+
}
513+
errors.push(serializeBrowserError("probe-error", error))
514+
}
515+
for (const routeError of await closeBrowserAndDrainPreviewRoutes(browser, routeTracker)) {
516+
const browserCloseFailed = routeError.message.includes("operation=browser-close")
517+
if (!pendingError && (browserCloseFailed || runPlan.routeHostDrain === "required")) {
511518
pendingError = routeError
512519
progress.fail("probe-error", routeError)
513520
}
514-
errors.push(serializeBrowserError("probe-error", error))
521+
errors.push(serializeBrowserError("probe-error", routeError))
515522
}
516523
if (captureSelection.console) {
517524
await artifactSession.writeJsonLines("console", "console.jsonl", consoleMessages)

0 commit comments

Comments
 (0)