fix(browser): admit query expansion and guard saved-value publication (#10052)

fix(browser): admit query expansion and active response saves

docs/guides/web/browser-scenario-contract.md

@@ -531,7 +531,10 @@ id the application assigns:
   start origin as the browser spells it, so a path may name `My List` or
   `My%20List` (`{*}` matches one segment, and the query is ignored), and fails
   the step unless a 2xx JSON response arrives before its `timeout_ms`; one still
-  being read when the step times out or is cancelled saves nothing. A pointer
+  being read when the step times out or is cancelled saves nothing. Saved values
+  are committed only while the step remains active, after response reading and
+  any request-route cleanup succeed. Cancellation after that commit retains
+  them; a physical request or cleanup failure keeps its original reason. A pointer
   that names no string or number fails the step, so later steps never send a
   missing value, as does a response over 4,000,000 characters: a request step
   stops reading and aborts its fetch there. A UI step reads a body only when
@@ -592,15 +595,17 @@ id the application assigns:
   partial failures without retaining an image to scrub later. Other requested
   artifacts remain available; affected screenshot/aggregate completeness is
   partial and is not eligible for equality.
-- Saved URLs and expanded request JSON are admitted before building their expanded strings or
-  serializing. Its allowance is the smaller of Node's string limit and
-  one sixteenth of current V8 available heap bytes, allowing for the expanded
-  tree, serialization, Playwright transport and browser-side copies. Size
+- Saved URLs, structured queries, and expanded request JSON are admitted before
+  building their expanded strings or serializing. Their allowance is the smaller
+  of Node's string limit and one sixteenth of current V8 available heap bytes,
+  allowing for the expanded tree, serialization, Playwright transport and
+  browser-side copies. Size
   accounting includes JSON escaping and repeated references; each distinct
   saved value's size is measured once per request. An oversized request fails
   before browser dispatch; the complete request envelope, including repeated
-  header values, is checked as well. URL encoding uses a conservative
-  nine-characters-per-UTF-16-unit allowance before encoding a saved value.
+  header values, is checked as well. URI-component and form-query sizes are
+  counted with their respective escaping rules before encoding; repeated equal
+  components reuse their measured size.
   Reduce the payload/repetitions, or raise the parent
   Node heap and restart; this is a conservative allocation estimate, not a
   measurement or guarantee of browser/process-family memory.

src/browser/BrowserScenarioSecrets.ts

@@ -1,3 +1,7 @@
+import {
+  scenarioSerializationAllowance,
+  scenarioQueryLength,
+} from "./BrowserScenarioSerialization.js";
 import type { AnalysisError } from "../domain/analysisErrorBase.js";
 import { redactSensitiveFailure } from "../domain/explicitSensitiveFailure.js";
 import { AnalysisInputError } from "../domain/analysisErrorCore.js";
@@ -666,8 +670,28 @@ export class BrowserScenarioSecrets {
 
   url(destination: BrowserScenarioUrl): string {
     const url = new URL(destination.url);
-    for (const { name, value } of destination.query)
-      url.searchParams.append(name, this.value(value));
+    if (destination.query.length > 0) {
+      const query: [string, string][] = destination.query.map(
+        ({ name, value }) => [name, this.value(value)],
+      );
+      const entries = [...url.searchParams, ...query];
+      // Appending form entries reserializes the literal query too; fragments remain unchanged.
+      const queryStart = url.href.indexOf("?");
+      const hashStart = url.href.indexOf("#");
+      const existingQueryLength =
+        queryStart >= 0 && (hashStart < 0 || queryStart < hashStart)
+          ? (hashStart < 0 ? url.href.length : hashStart) - queryStart
+          : 0;
+      const budget =
+        scenarioSerializationAllowance() -
+        (url.href.length - existingQueryLength) -
+        1;
+      if (scenarioQueryLength(entries, budget, "form") > budget)
+        throw new Error(
+          "Expanded scenario URL exceeds the current heap/string serialization allowance; reduce repeated values or raise the parent Node heap and restart.",
+        );
+      for (const [name, value] of query) url.searchParams.append(name, value);
+    }
     return url.href;
   }
 

src/browser/BrowserScenarioSerialization.ts

@@ -0,0 +1,65 @@
+import { constants as bufferConstants } from "node:buffer";
+import { getHeapStatistics } from "node:v8";
+
+/** Allow for expanded values, stringify, Playwright transport and browser-side copies. */
+const SCENARIO_ALLOCATION_FACTOR = 16;
+export const scenarioSerializationAllowance = (): number =>
+  Math.min(
+    bufferConstants.MAX_STRING_LENGTH,
+    Math.floor(
+      getHeapStatistics().total_available_size / SCENARIO_ALLOCATION_FACTOR,
+    ),
+  );
+
+type QueryEncoding = "uri-component" | "form";
+const URI_KEPT = /^[\w.!~*'()-]$/u;
+const FORM_KEPT = /^[\w *.-]$/u;
+
+/** Count the selected encoder's ASCII output without allocating an encoded copy. */
+const componentLength = (
+  text: string,
+  limit: number,
+  encoding: QueryEncoding,
+): number => {
+  let length = 0;
+  for (const character of text) {
+    const point = character.codePointAt(0)!;
+    if (point >= 0xd800 && point <= 0xdfff && encoding === "uri-component")
+      throw new URIError("URI malformed");
+    // URLSearchParams replaces an unpaired UTF-16 unit with U+FFFD (three UTF-8 bytes).
+    length += (encoding === "form" ? FORM_KEPT : URI_KEPT).test(character)
+      ? 1
+      : 3 * (point < 0x80 ? 1 : point < 0x800 ? 2 : point < 0x10000 ? 3 : 4);
+    if (length > limit) return length;
+  }
+  return length;
+};
+
+export const uriComponentLength = (text: string, limit: number): number =>
+  componentLength(text, limit, "uri-component");
+
+/** Query separators and encoded entries; a result above the budget establishes overflow only. */
+export const scenarioQueryLength = (
+  entries: readonly (readonly [string, string])[],
+  budget: number,
+  encoding: QueryEncoding,
+): number => {
+  let rawLength = Math.max(0, entries.length * 2 - 1);
+  for (const [name, value] of entries) {
+    rawLength += name.length + value.length;
+    if (rawLength > budget) return rawLength;
+  }
+  let length = Math.max(0, entries.length * 2 - 1);
+  const sizes = new Map<string, number>();
+  for (const entry of entries)
+    for (const text of entry) {
+      let size = sizes.get(text);
+      if (size === undefined) {
+        size = componentLength(text, budget - length, encoding);
+        sizes.set(text, size);
+      }
+      length += size;
+      if (length > budget) return length;
+    }
+  return length;
+};

src/browser/PlaywrightScenarioRequest.test.ts

@@ -390,6 +390,42 @@ describe("request route cleanup", () => {
     expect(save).not.toHaveBeenCalled();
     expect(ready).not.toHaveBeenCalled();
   });
+
+  it.each([
+    { status: 200, end: "cancellation" },
+    { status: 503, end: "cancellation" },
+    { status: 200, end: "deadline" },
+    { status: 503, end: "deadline" },
+  ])(
+    "publishes nothing after $end during final route cleanup (status $status)",
+    async ({ status, end }) => {
+      stubFetch(status, ['{"id":"late-object"}']);
+      const parsed = scenario([
+        requestStep({
+          expected_status: [200],
+          timeout_ms: 5000,
+          save: [{ name: "id", json_pointer: "/id" }],
+        }),
+      ]);
+      const page = fakePage({});
+      const secrets = resolved(parsed);
+      const cancel = new AbortController();
+      const began = Date.now();
+      const unroute = vi.spyOn(page, "unroute").mockImplementation(async () => {
+        if (end === "cancellation")
+          cancel.abort("cancelled during final route cleanup");
+        else {
+          const now = vi.spyOn(Date, "now").mockReturnValue(began + 5001);
+          onTestFinished(() => now.mockRestore());
+        }
+      });
+      await expect(
+        perform(page, parsed.actions[0]!, secrets, cancel.signal),
+      ).rejects.toThrow(status === 503 ? "answered 503" : "ended before");
+      expect(() => secrets.savedValue("id")).toThrow("No earlier step saved");
+      expect(unroute).toHaveBeenCalledOnce();
+    },
+  );
 });
 
 describe("request page boundaries", () => {
@@ -513,12 +549,14 @@ describe("request step confinement", () => {
     const parsed = scenario([
       requestStep({
         path: "/api/items?flag&a=b%20c&s=x;y",
-        query: [{ name: "q", value: { source: "literal", value: "a b" } }],
+        query: [
+          { name: "q", value: { source: "literal", value: "a b私😀!~'" } },
+        ],
       }),
     ]);
     await perform(fakePage({}), parsed.actions[0]!, resolved(parsed));
     expect(calls[0]!.url).toBe(
-      `${ORIGIN}/api/items?flag&a=b%20c&s=x;y&q=a%20b`,
+      `${ORIGIN}/api/items?flag&a=b%20c&s=x;y&q=a%20b%E7%A7%81%F0%9F%98%80!~'`,
     );
     for (const [path, sent] of [
       ["/api/items?", "/api/items?q=1"],
@@ -1700,7 +1738,7 @@ describe("saving from a UI step's response", () => {
     await expect(
       perform(uiPage([]).page, parsed.actions[0]!, resolved(parsed)),
     ).rejects.toThrow(
-      "no POST /api/items response arrived before the step's timeout",
+      "no POST /api/items response could be read, so nothing was saved: Timeout exceeded",
     );
     await expect(
       perform(
@@ -1721,6 +1759,72 @@ describe("saving from a UI step's response", () => {
   });
 });
 
+it.each([false, true])(
+  "retains the physical response-wait failure despite late cancellation %s and removes its request watcher",
+  async (cancelAtCompletion) => {
+    const parsed = clickSaving("/api/items");
+    const secrets = resolved(parsed);
+    const { page, listeners } = uiPage([]);
+    const primary = new Error(
+      "page.waitForResponse: Target page has been closed",
+    );
+    vi.spyOn(page, "waitForResponse").mockRejectedValue(primary);
+    const cancel = new AbortController();
+    if (cancelAtCompletion) {
+      const remove = cancel.signal.removeEventListener.bind(cancel.signal);
+      const removing = vi
+        .spyOn(cancel.signal, "removeEventListener")
+        .mockImplementation((type, listener, options) => {
+          remove(type, listener, options);
+          cancel.abort("cancelled after the physical wait failed");
+        });
+      onTestFinished(() => removing.mockRestore());
+    }
+    const failure = await perform(
+      page,
+      parsed.actions[0]!,
+      secrets,
+      cancel.signal,
+    ).catch((cause: unknown) => cause);
+    expect(failure).toMatchObject({ cause: primary });
+    expect((failure as Error).message).toContain(primary.message);
+    expect(() => secrets.savedValue("item")).toThrow("No earlier step saved");
+    expect(listeners.size).toBe(0);
+  },
+);
+
+it.each(["request", "click"])(
+  "retains saved values when cancellation follows completed %s publication",
+  async (kind) => {
+    onTestFinished(() => {
+      vi.unstubAllGlobals();
+    });
+    const parsed =
+      kind === "request"
+        ? scenario([
+            requestStep({ save: [{ name: "item", json_pointer: "/id" }] }),
+          ])
+        : clickSaving("/api/items");
+    stubFetch(200, ['{"id":"committed-object"}']);
+    const page =
+      kind === "request"
+        ? fakePage({})
+        : uiPage([
+            {
+              method: "POST",
+              url: `${ORIGIN}/api/items`,
+              status: 201,
+              body: '{"id":"committed-object"}',
+            },
+          ]).page;
+    const secrets = resolved(parsed);
+    const cancel = new AbortController();
+    await perform(page, parsed.actions[0]!, secrets, cancel.signal);
+    cancel.abort("cancelled after publication");
+    expect(secrets.savedValue("item")).toBe("committed-object");
+  },
+);
+
 describe("saving from a compressed or cached UI response", () => {
   it.each([
     { encoding: "gzip", size: 32 },
@@ -1842,6 +1946,35 @@ describe("saved response limits and deadlines", () => {
     expect(listeners.size).toBe(0);
   });
 
+  it("saves nothing when cancellation arrives as the body wait releases its listener", async () => {
+    const parsed = clickSaving("/api/items");
+    const secrets = resolved(parsed);
+    const { page, listeners } = uiPage([
+      {
+        method: "POST",
+        url: `${ORIGIN}/api/items`,
+        status: 201,
+        body: '{"id":"late-object"}',
+      },
+    ]);
+    const cancel = new AbortController();
+    const remove = cancel.signal.removeEventListener.bind(cancel.signal);
+    let waitsReleased = 0;
+    const removeListener = vi
+      .spyOn(cancel.signal, "removeEventListener")
+      .mockImplementation((type, listener, options) => {
+        remove(type, listener, options);
+        if (++waitsReleased === 2)
+          cancel.abort("cancelled at body wait completion");
+      });
+    onTestFinished(() => removeListener.mockRestore());
+    await expect(
+      perform(page, parsed.actions[0]!, secrets, cancel.signal),
+    ).rejects.toThrow("ended before");
+    expect(() => secrets.savedValue("item")).toThrow("No earlier step saved");
+    expect(listeners.size).toBe(0);
+  });
+
   it("reads a response whose received bytes exceed the character limit but whose text is within it", async () => {
     const parsed = clickSaving("/api/items");
     const secrets = resolved(parsed);
@@ -1911,6 +2044,92 @@ it.each(["array", "embedded", "headers"])(
   },
 );
 
+it.each(["x".repeat(3_000_000), "私".repeat(100_000)])(
+  "refuses repeated query expansion before encoding or browser dispatch (case %#)",
+  async (value) => {
+    onTestFinished(() => {
+      vi.unstubAllGlobals();
+    });
+    const parsed = scenario([
+      requestStep({
+        query: Array.from({ length: 1000 }, (_, index) => ({
+          name: `repeated-${index}`,
+          value: { source: "literal", value },
+        })),
+      }),
+    ]);
+    const secrets = resolved(parsed);
+    const page = { ...fakePage({}), evaluate: vi.fn() } as unknown as Page;
+    const encode = vi
+      .spyOn(globalThis, "encodeURIComponent")
+      .mockImplementation(() => {
+        throw new Error("Query encoding ran before expansion admission");
+      });
+    onTestFinished(() => encode.mockRestore());
+    await expect(perform(page, parsed.actions[0]!, secrets)).rejects.toThrow(
+      "serialization allowance",
+    );
+    expect(encode).not.toHaveBeenCalled();
+    expect(page.evaluate).not.toHaveBeenCalled();
+  },
+);
+
+it("refuses repeated goto query expansion before form encoding or browser navigation", async () => {
+  const value = "x".repeat(3_000_000);
+  const parsed = scenario([
+    {
+      ...gotoStep(`${ORIGIN}/items?literal=a%20b#kept`),
+      destination: {
+        url: `${ORIGIN}/items?literal=a%20b#kept`,
+        query: Array.from({ length: 1000 }, (_, index) => ({
+          name: `repeated-${index}`,
+          value: { source: "literal", value },
+        })),
+      },
+    },
+  ]);
+  const secrets = resolved(parsed);
+  const goto = vi.fn(async () => null);
+  const append = vi
+    .spyOn(URLSearchParams.prototype, "append")
+    .mockImplementation(() => {
+      throw new Error("Form encoding ran before expansion admission");
+    });
+  onTestFinished(() => append.mockRestore());
+  await expect(
+    perform({ goto } as unknown as Page, parsed.actions[0]!, secrets),
+  ).rejects.toThrow("serialization allowance");
+  expect(append).not.toHaveBeenCalled();
+  expect(goto).not.toHaveBeenCalled();
+});
+
+it("preserves goto form query normalization, Unicode, replacement characters and the fragment", async () => {
+  const parsed = scenario([
+    {
+      ...gotoStep(`${ORIGIN}/items?literal=a%20b#kept`),
+      destination: {
+        url: `${ORIGIN}/items?literal=a%20b#kept`,
+        query: [
+          {
+            name: "q",
+            value: { source: "literal", value: "a b私😀!~'\ud800" },
+          },
+        ],
+      },
+    },
+  ]);
+  const goto = vi.fn(async () => null);
+  await perform(
+    { goto } as unknown as Page,
+    parsed.actions[0]!,
+    resolved(parsed),
+  );
+  expect(goto).toHaveBeenCalledWith(
+    `${ORIGIN}/items?literal=a+b&q=a+b%E7%A7%81%F0%9F%98%80%21%7E%27%EF%BF%BD#kept`,
+    expect.anything(),
+  );
+});
+
 it("refuses repeated saved URL expansion before browser navigation", async () => {
   const parsed = scenario([
     requestStep({ save: [{ name: "large", json_pointer: "/value" }] }),

src/browser/PlaywrightScenarioRequest.ts

@@ -1,5 +1,3 @@
-import { constants as bufferConstants } from "node:buffer";
-import { getHeapStatistics } from "node:v8";
 import { jsonStringByteLength } from "../domain/jsonStringByteLength.js";
 import { jsonValueByteLength } from "../domain/jsonValueByteLength.js";
 import type { JsonValue } from "../domain/jsonValue.js";
@@ -24,6 +22,11 @@ import {
   type BrowserScenarioSaveResponse,
 } from "../domain/browserScenarioValues.js";
 import type { BrowserScenarioSecrets } from "./BrowserScenarioSecrets.js";
+import {
+  scenarioSerializationAllowance,
+  scenarioQueryLength,
+  uriComponentLength,
+} from "./BrowserScenarioSerialization.js";
 
 type RequestAction = Extract<BrowserScenarioAction, { action: "request" }>;
 
@@ -160,18 +163,12 @@ const headerValue = async (
   return readPageValue(step, value, label);
 };
 
-/** Append declared query entries after any literal query the path carries, leaving that query as written. */
+/** Append admitted query entries after any literal query the path carries, leaving that query as written. */
 const withQuery = (
   url: URL,
   entries: readonly (readonly [string, string])[],
 ): string => {
   if (entries.length === 0) return url.href;
-  const added = entries
-    .map(
-      ([name, value]) =>
-        `${encodeURIComponent(name)}=${encodeURIComponent(value)}`,
-    )
-    .join("&");
   // fetch drops the fragment; cut it at its "#" even when nothing follows it.
   const hash = url.href.indexOf("#");
   const base = hash < 0 ? url.href : url.href.slice(0, hash);
@@ -180,6 +177,16 @@ const withQuery = (
     : base.endsWith("?") || base.endsWith("&")
       ? ""
       : "&";
+  const budget =
+    scenarioSerializationAllowance() - base.length - separator.length;
+  if (scenarioQueryLength(entries, budget, "uri-component") > budget)
+    throw requestEnvelopeError();
+  const added = entries
+    .map(
+      ([name, value]) =>
+        `${encodeURIComponent(name)}=${encodeURIComponent(value)}`,
+    )
+    .join("&");
   return `${base}${separator}${added}`;
 };
 
@@ -315,7 +322,7 @@ export const performScenarioRequest = async (input: {
   );
   const step = { page, url, stepId: action.step_id, secrets };
   const headers: [string, string][] = [];
-  const envelopeLimit = requestSerializationAllowance();
+  const envelopeLimit = scenarioSerializationAllowance();
   let headerBytes = 2;
   const headerValues = new Map<string, number>();
   const appendHeader = (name: string, value: string): void => {
@@ -466,6 +473,7 @@ export const performScenarioRequest = async (input: {
     }
   }
   if (action.save.length > 0) {
+    assertResponseSaveActive(action.step_id, deadline, signal);
     saveValues(action.step_id, response.body, action.save, secrets);
     if (observedRequest !== undefined && !ambiguousRequest)
       secrets.markResponseSecretsReady(observedRequest);
@@ -481,7 +489,7 @@ export const withSavedPlaceholders = (
   secrets: BrowserScenarioSecrets,
   placeholder: RegExp = SAVED_PLACEHOLDER,
 ): string => {
-  const limit = requestSerializationAllowance();
+  const limit = scenarioSerializationAllowance();
   let expandedUnits = text.length;
   if (expandedUnits > limit)
     throw new ScenarioRequestError(
@@ -493,8 +501,7 @@ export const withSavedPlaceholders = (
       let replacement = encoded.get(name);
       if (replacement === undefined) {
         const raw = String(secrets.savedValue(name));
-        // A UTF-16 unit needs at most nine ASCII characters under URI encoding.
-        if (raw.length * 9 > limit)
+        if (uriComponentLength(raw, limit) > limit)
           throw new ScenarioRequestError(
             "Saved URL encoding exceeds the current heap/string serialization allowance.",
           );
@@ -553,6 +560,21 @@ const namesResponse = (
   );
 };
 
+/** Publication must still belong to the step after its last asynchronous boundary. */
+const assertResponseSaveActive = (
+  stepId: string,
+  deadline: number | undefined,
+  signal: AbortSignal | undefined,
+): void => {
+  if (
+    signal?.aborted === true ||
+    (deadline !== undefined && Date.now() >= deadline)
+  )
+    throw new ScenarioRequestError(
+      `Step ${stepId} ended before its saved response was read, so nothing was saved.`,
+    );
+};
+
 /** Settle as the work does, or fail the step once its deadline passes or it is cancelled, so late work saves nothing. */
 const beforeStepEnds = <T>(
   work: Promise<T>,
@@ -647,9 +669,11 @@ export const awaitSavedResponse = (
     let response: Awaited<typeof waiting>;
     try {
       response = await beforeStepEnds(waiting, stepId, deadline, signal);
-    } catch {
+    } catch (cause: unknown) {
+      if (cause instanceof ScenarioRequestError) throw cause;
       throw new ScenarioRequestError(
-        `Step ${stepId}: no ${spec.method} ${spec.path} response arrived before the step's timeout, so nothing was saved.`,
+        `Step ${stepId}: no ${spec.method} ${spec.path} response could be read, so nothing was saved: ${cause instanceof Error ? cause.message : String(cause)}`,
+        { cause },
       );
     }
     const assertUnambiguous = (): void => {
@@ -672,6 +696,7 @@ export const awaitSavedResponse = (
     if (!body.ok)
       throw new ScenarioRequestError(`Step ${stepId}: ${body.error.message}`);
     assertUnambiguous();
+    assertResponseSaveActive(stepId, deadline, signal);
     saveValues(stepId, body.value, spec.save, secrets);
     secrets.markResponseSecretsReady(response.request());
   };
@@ -683,22 +708,12 @@ const requestEnvelopeError = (): ScenarioRequestError =>
     "Expanded request envelope exceeds the current heap/string serialization allowance; reduce repeated values or raise the parent Node heap and restart.",
   );
 
-/** Allow for the expanded tree, stringify, Playwright transport and browser-side copies. */
-const REQUEST_JSON_ALLOCATION_FACTOR = 16;
-const requestSerializationAllowance = (): number =>
-  Math.min(
-    bufferConstants.MAX_STRING_LENGTH,
-    Math.floor(
-      getHeapStatistics().total_available_size / REQUEST_JSON_ALLOCATION_FACTOR,
-    ),
-  );
-
 /** Count serialization expansion without allocating substituted strings or their serialized copies. */
 const admitSavedJson = (
   value: JsonValue,
   secrets: BrowserScenarioSecrets,
 ): void => {
-  const limit = requestSerializationAllowance();
+  const limit = scenarioSerializationAllowance();
   let bytes = 0;
   const savedSizes = new Map<string, number>();
   const charge = (size: number): void => {