From 628f3c0a53b9a385e12ef032133798d12fb8edf4 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 18 Sep 2026 11:37:10 +0200 Subject: [PATCH] fix(sdk): sanitize malformed-success decode errors Omit decoder causes at the successful HTTP and SSE payload boundary while retaining safe metadata and preserving body-read, server, authentication, transport, and cancellation causes. Relates to #1694 Co-authored-by: Codex Signed-off-by: Samuele Verzi --- docs/acceptance/README.md | 3 +- .../sdk-malformed-success-decoding.md | 4 +- docs/architecture.md | 7 +- docs/design/IMPLEMENTATION-NOTES.md | 9 +- sdk/typescript/src/errors.ts | 12 +- sdk/typescript/src/http.ts | 40 +- .../test/http-decoding-errors.test.ts | 367 ++++++++++++++++++ user-docs/building/typescript-sdk/connect.md | 9 +- .../reference/typescript-sdk-api/core.md | 2 +- 9 files changed, 425 insertions(+), 28 deletions(-) create mode 100644 sdk/typescript/test/http-decoding-errors.test.ts diff --git a/docs/acceptance/README.md b/docs/acceptance/README.md index 88e84773f7..3215667850 100644 --- a/docs/acceptance/README.md +++ b/docs/acceptance/README.md @@ -342,7 +342,8 @@ PR after verification. There is no cleanup or status-only PR. - [TypeScript SDK malformed-success decoding](sdk-malformed-success-decoding.md) - cause-free protocol errors for malformed successful unary HTTP and ordinary SSE payloads, retaining safe status and request-ID metadata while preserving server, transport, and - cancellation causes. Status: proposed. + cancellation causes. Status: in-progress stacked implementation candidate; the local full + race suite is host-linker-blocked and remains for CI. - [Canonical Shell command tool](canonical-shell-command-tool.md) — canonical `Shell` and `ShellStatus` model-facing names, safe legacy `Bash` input normalization, and diff --git a/docs/acceptance/sdk-malformed-success-decoding.md b/docs/acceptance/sdk-malformed-success-decoding.md index 4a4f369cdf..fbb76423ec 100644 --- a/docs/acceptance/sdk-malformed-success-decoding.md +++ b/docs/acceptance/sdk-malformed-success-decoding.md @@ -4,12 +4,12 @@ **Work classification:** Architectural - this changes the durable public diagnostic and security policy for `ProtocolError` at the TypeScript SDK's HTTP successful-response boundary. **Decision record:** [ADR 0348](../adr/0348-typescript-sdk-malformed-success-decoding.md) **Phase:** TypeScript SDK HTTP transport hardening -**Status:** proposed, 2026-09-18. Drafted from issue #1694 with no unresolved human decisions. +**Status:** in-progress, 2026-09-18. The stacked implementation candidate satisfies AC1.1-AC2.4 and all applicable local gates; the host Xcode/macOS 27 linker blocks the full CGO race suite, so CI must supply that final proof. The contract becomes authoritative only after the Plan / Interface and Implementation PRs merge in order. **Delivery:** Split, with an explicit checkpoint waiver. The Plan / Interface PR records the security and compatibility boundary; in this session the directing human explicitly instructed the implementation to proceed as the next `gh stack` layer without waiting for the plan to merge. `/plan-orchestrate` is not used because its merged-baseline precondition is intentionally waived. **Expected tasks:** 1 **Issue:** [stacklok/mecatl#1694](https://github.com/stacklok/mecatl/issues/1694). **Plan PR:** [#1698](https://github.com/stacklok/mecatl/pull/1698) -**Approved baseline:** absent by explicit human exception; the directing human requested two sequential `gh stack` PRs and explicitly said there is no need to wait for the Plan / Interface PR to merge. +**Approved baseline:** `b6e05820685a85bf3cf027ed42da4e269060e6ed`, the exact Plan / Interface commit used under the explicit human stacking exception; it is not merged authority. The directing human requested two sequential `gh stack` PRs and explicitly said there is no need to wait for the plan to merge. The TypeScript SDK rejects malformed successful unary HTTP responses and ordinary SSE data frames without retaining runtime-dependent decoder exceptions. diff --git a/docs/architecture.md b/docs/architecture.md index cdcedfe382..5ccc890af4 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -332,7 +332,12 @@ protobuf-es decoding, the HTTP transport recursively follows the output descript stdlib-JSON `{seconds,nanos}` objects only at `google.protobuf.Timestamp` and `google.protobuf.Duration` fields. ProtoJSON strings and `null` pass through, malformed objects fail as typed HTTP protocol errors, and normalization uses a detached value so `getRawJson()` -retains the original unary or SSE data. UDS dials by +retains the original unary or SSE data. JSON, well-known-type, and protobuf decode failures for +successful unary responses and ordinary SSE data frames expose only generic SDK messages, HTTP +status, and an available request ID; they omit decoder causes so rejected response text cannot +escape through runtime-specific exception messages. Body acquisition, server errors, +authentication, network, cancellation, and SSE reader failures stay outside that cause-free +boundary. UDS dials by supplying connect-node's HTTP/2 node connection option for the socket path, never a `unix://` base URL. Unit tests inject transports; `sdk/typescript/e2e/` separately builds and spawns the same checkout's `mecated` with the offline mock provider to diff --git a/docs/design/IMPLEMENTATION-NOTES.md b/docs/design/IMPLEMENTATION-NOTES.md index 5d5d042560..f523fddabc 100644 --- a/docs/design/IMPLEMENTATION-NOTES.md +++ b/docs/design/IMPLEMENTATION-NOTES.md @@ -8561,7 +8561,14 @@ numbers; and protobuf range and sign rules fail closed. An existing string or `n protobuf-es unchanged. The walk produces a detached JSON value and leaves unknown fields intact; `sdk/typescript/src/http.ts` registers the original parsed unary response, SSE envelope, and nested event with `getRawJson()` before yielding the decoded message. Any conversion or protobuf-es decode -failure becomes the existing `ProtocolError` with HTTP transport identity. +failure becomes the existing `ProtocolError` with HTTP transport identity. The shared +`malformedSuccess` constructor retains only the generic message, status, request ID, and transport; +the JSON/protobuf exception is deliberately absent. Unary handling acquires `response.text()` in a +separate caused-error block before `JSON.parse`, so a post-header body-stream failure keeps its +existing `ProtocolError.cause`. SSE `reader.read()` failures remain outside both decoder catches. +Non-2xx and valid `event: error` frames still use `errorFromProblem`, and credential, fetch, abort, +control, and stream lifecycle paths are unchanged. [ADR 0348](../adr/0348-typescript-sdk-malformed-success-decoding.md) +records this cause boundary. `sdk/typescript/src/raw.ts` enforces API-major compatibility before all non-compatibility RPCs; the ergonomic client also probes status and maps transport/auth/incompatibility states without diff --git a/sdk/typescript/src/errors.ts b/sdk/typescript/src/errors.ts index 9cf0568230..0520a1faae 100644 --- a/sdk/typescript/src/errors.ts +++ b/sdk/typescript/src/errors.ts @@ -208,7 +208,17 @@ export class AuthenticationError extends MecatlError { } } -/** A transport response violated the SDK's protocol contract. @public */ +/** + * A transport response violated the SDK's protocol contract. + * + * Malformed successful HTTP responses and ordinary SSE data frames omit the + * underlying JSON or protobuf decoder cause. They retain safe correlation + * metadata such as HTTP status and a response request ID when available. + * Server errors, authentication failures, and transport or body-read failures + * keep their separately defined cause behavior. + * + * @public + */ export class ProtocolError extends MecatlError { constructor(message: string, options: Omit) { super(message, { ...options, code: "protocol" }); diff --git a/sdk/typescript/src/http.ts b/sdk/typescript/src/http.ts index 06ffdfe2a7..a4fb689561 100644 --- a/sdk/typescript/src/http.ts +++ b/sdk/typescript/src/http.ts @@ -47,6 +47,14 @@ function record(value: JsonValue): JsonRecord { return value as JsonRecord; } +function malformedSuccess(message: string, response: Response): ProtocolError { + return new ProtocolError(message, { + requestId: response.headers.get("x-request-id") ?? undefined, + status: response.status, + transport: "http", + }); +} + function permissionMode(value: JsonValue | undefined): string { switch (value) { case 2: @@ -288,8 +296,9 @@ class HttpTransport implements Transport { if (!response.ok) await this.#problem(response); let raw: JsonValue = {}; if (response.status !== 204) { + let body: string; try { - raw = (await response.json()) as JsonValue; + body = await response.text(); } catch (cause) { throw new ProtocolError("The mecatl server returned invalid JSON", { cause, @@ -297,6 +306,11 @@ class HttpTransport implements Transport { transport: "http", }); } + try { + raw = JSON.parse(body) as JsonValue; + } catch { + throw malformedSuccess("The mecatl server returned invalid JSON", response); + } } let normalized: JsonValue; let message: MessageShape; @@ -306,12 +320,8 @@ class HttpTransport implements Transport { normalizeUnaryResponse(resolved.classification, normalizeMethodResponse(method.name, raw)), ); message = fromJson(method.output, normalized, { ignoreUnknownFields: true }); - } catch (cause) { - throw new ProtocolError("The mecatl server returned an invalid response", { - cause, - status: response.status, - transport: "http", - }); + } catch { + throw malformedSuccess("The mecatl server returned an invalid response", response); } registerRawJson(message, raw); if (method.name === "GetCompatibilityInfo") { @@ -497,12 +507,8 @@ class HttpTransport implements Transport { let raw: JsonValue; try { raw = JSON.parse(next.value.data) as JsonValue; - } catch (cause) { - throw new ProtocolError("The mecatl SSE stream contained invalid JSON", { - cause, - status: response.status, - transport: "http", - }); + } catch { + throw malformedSuccess("The mecatl SSE stream contained invalid JSON", response); } if (next.value.event === "error") { throw errorFromProblem( @@ -516,12 +522,8 @@ class HttpTransport implements Transport { try { normalized = normalizeHttpWktJson(output, wrapEvent ? { event: raw } : raw); message = fromJson(output, normalized, { ignoreUnknownFields: true }); - } catch (cause) { - throw new ProtocolError("The mecatl SSE stream contained an invalid event", { - cause, - status: response.status, - transport: "http", - }); + } catch { + throw malformedSuccess("The mecatl SSE stream contained an invalid event", response); } registerRawJson(message, raw); if (wrapEvent) { diff --git a/sdk/typescript/test/http-decoding-errors.test.ts b/sdk/typescript/test/http-decoding-errors.test.ts new file mode 100644 index 0000000000..75e7d79280 --- /dev/null +++ b/sdk/typescript/test/http-decoding-errors.test.ts @@ -0,0 +1,367 @@ +import { describe, expect, it } from "vitest"; + +import { HarnessService } from "../src/gen/mecatl/v1/harness_pb.js"; +import { ScheduleService } from "../src/gen/mecatl/v1/schedule_pb.js"; +import { + AuthenticationError, + CursorExpiredError, + createHttpTransport, + ProtocolError, + ServerError, + TransportError, +} from "../src/index.js"; + +const baseUrl = "http://mecatl.test"; +const validWatchFrame = { + cursor: "before-malformed", + event: { seq: 1, text: "before", type: "message.delta" }, + phase: "replay", +}; +const laterWatchFrame = { + cursor: "after-malformed", + event: { seq: 3, text: "must not be yielded", type: "message.delta" }, + phase: "live", +}; + +async function* one(value: T): AsyncIterable { + yield value; +} + +async function caught(operation: Promise): Promise { + try { + await operation; + } catch (error) { + return error; + } + throw new Error("expected operation to reject"); +} + +function data(value: unknown): string { + return `data: ${JSON.stringify(value)}\n\n`; +} + +function sse(body: string, requestId?: string): Response { + return new Response(body, { + headers: { + "content-type": "text/event-stream", + ...(requestId === undefined ? {} : { "x-request-id": requestId }), + }, + }); +} + +async function unary(fetch: typeof globalThis.fetch): Promise { + return createHttpTransport({ baseUrl, fetch }).unary( + ScheduleService.method.getSchedule, + undefined, + undefined, + undefined, + { name: "nightly" }, + ); +} + +async function watch(fetch: typeof globalThis.fetch) { + return createHttpTransport({ baseUrl, fetch }).stream( + HarnessService.method.watchSessionEvents, + undefined, + undefined, + undefined, + one({ sessionId: "session-1" }), + ); +} + +function expectSafeProtocol( + error: unknown, + expected: { + readonly canary: string; + readonly message: string; + readonly requestId?: string | undefined; + }, +): ProtocolError { + expect(error).toBeInstanceOf(ProtocolError); + const protocol = error as ProtocolError; + expect(Object.hasOwn(protocol, "cause")).toBe(false); + expect(protocol.cause).toBeUndefined(); + expect(protocol).toMatchObject({ + code: "protocol", + message: expected.message, + requestId: expected.requestId, + status: 200, + transport: "http", + }); + expect(`${protocol.message} ${JSON.stringify(protocol.toJSON())}`).not.toContain(expected.canary); + return protocol; +} + +describe("HTTP malformed-success decoding", () => { + it("malformed successful HTTP and SSE payloads omit decoder causes", async () => { + const unaryJsonCanary = "UNARY_JSON_PRESENTATION_URL_CANARY"; + const unaryJsonError = await caught( + unary( + async () => + new Response(`not-json-${unaryJsonCanary}`, { + headers: { "x-request-id": "unary-json-request" }, + }), + ), + ); + expectSafeProtocol(unaryJsonError, { + canary: unaryJsonCanary, + message: "The mecatl server returned invalid JSON", + requestId: "unary-json-request", + }); + + const unaryWktCanary = "UNARY_WKT_PRESENTATION_URL_CANARY"; + const unaryWktError = await caught( + unary(async () => + Response.json({ + schedule: { spec: { created_at: { seconds: unaryWktCanary } } }, + }), + ), + ); + expectSafeProtocol(unaryWktError, { + canary: unaryWktCanary, + message: "The mecatl server returned an invalid response", + }); + + const unaryProtoCanary = "UNARY_PROTO_PRESENTATION_URL_CANARY"; + const unaryProtoError = await caught( + unary(async () => + Response.json({ + schedule: { spec: { max_fires: unaryProtoCanary } }, + }), + ), + ); + expectSafeProtocol(unaryProtoError, { + canary: unaryProtoCanary, + message: "The mecatl server returned an invalid response", + }); + + const sseJsonCanary = "SSE_JSON_PRESENTATION_URL_CANARY"; + const invalidJsonStream = await watch(async () => + sse( + `${data(validWatchFrame)}data: {"canary":"${sseJsonCanary}"\n\n${data(laterWatchFrame)}`, + "sse-json-request", + ), + ); + const invalidJsonIterator = invalidJsonStream.message[Symbol.asyncIterator](); + await expect(invalidJsonIterator.next()).resolves.toMatchObject({ + done: false, + value: { cursor: "before-malformed" }, + }); + expectSafeProtocol(await caught(invalidJsonIterator.next()), { + canary: sseJsonCanary, + message: "The mecatl SSE stream contained invalid JSON", + requestId: "sse-json-request", + }); + await expect(invalidJsonIterator.next()).resolves.toEqual({ done: true, value: undefined }); + + const sseWktCanary = "SSE_WKT_PRESENTATION_URL_CANARY"; + const invalidWktEvent = { + cursor: "malformed", + event: { + authorization: { + authorization_id: "authorization-1", + expires_at: { seconds: sseWktCanary }, + status: "pending", + }, + run_id: "run-1", + type: "authorization.required", + }, + phase: "live", + }; + const invalidWktStream = await watch(async () => + sse(`${data(validWatchFrame)}${data(invalidWktEvent)}${data(laterWatchFrame)}`), + ); + const invalidWktIterator = invalidWktStream.message[Symbol.asyncIterator](); + await expect(invalidWktIterator.next()).resolves.toMatchObject({ + done: false, + value: { cursor: "before-malformed" }, + }); + expectSafeProtocol(await caught(invalidWktIterator.next()), { + canary: sseWktCanary, + message: "The mecatl SSE stream contained an invalid event", + }); + await expect(invalidWktIterator.next()).resolves.toEqual({ done: true, value: undefined }); + + const sseProtoCanary = "SSE_PROTO_PRESENTATION_URL_CANARY"; + const invalidProtoEvent = { + cursor: "malformed", + event: { seq: sseProtoCanary, text: "invalid seq", type: "message.delta" }, + phase: "live", + }; + const invalidProtoStream = await watch(async () => + sse(`${data(validWatchFrame)}${data(invalidProtoEvent)}${data(laterWatchFrame)}`), + ); + const invalidProtoIterator = invalidProtoStream.message[Symbol.asyncIterator](); + await expect(invalidProtoIterator.next()).resolves.toMatchObject({ + done: false, + value: { cursor: "before-malformed" }, + }); + expectSafeProtocol(await caught(invalidProtoIterator.next()), { + canary: sseProtoCanary, + message: "The mecatl SSE stream contained an invalid event", + }); + await expect(invalidProtoIterator.next()).resolves.toEqual({ done: true, value: undefined }); + }); + + it("server and authentication errors retain their types and causes", async () => { + const problem = { + code: "session_not_found", + detail: "session missing", + status: 404, + type: "https://mecatl.stacklok.com/problems/session_not_found", + }; + const serverError = await caught( + unary(async () => + Response.json(problem, { + headers: { "content-type": "application/problem+json", "x-request-id": "server-404" }, + status: 404, + }), + ), + ); + expect(serverError).toBeInstanceOf(ServerError); + expect(serverError).toMatchObject({ + cause: problem, + code: "session_not_found", + message: "session missing", + requestId: "server-404", + status: 404, + transport: "http", + }); + + const eventProblem = { + code: "cursor_expired", + detail: "cursor expired", + status: 412, + type: "https://mecatl.stacklok.com/problems/cursor_expired", + }; + const errorStream = await watch(async () => + sse(`event: error\n${data(eventProblem)}`, "sse-error-request"), + ); + const eventError = await caught(errorStream.message[Symbol.asyncIterator]().next()); + expect(eventError).toBeInstanceOf(CursorExpiredError); + expect(eventError).toMatchObject({ + cause: eventProblem, + code: "cursor_expired", + message: "cursor expired", + requestId: "sse-error-request", + status: 200, + transport: "http", + }); + + const credentialCause = new Error("credential source unavailable"); + const credentialError = await caught( + createHttpTransport({ + baseUrl, + credentialProvider: async () => { + throw credentialCause; + }, + fetch: async () => { + throw new Error("fetch must not run"); + }, + }).unary(ScheduleService.method.getSchedule, undefined, undefined, undefined, { + name: "nightly", + }), + ); + expect(credentialError).toBeInstanceOf(AuthenticationError); + expect(credentialError).toMatchObject({ + cause: credentialCause, + code: "authentication", + message: "The credential provider failed", + transport: "http", + }); + + const authenticationProblem = { detail: "upstream detail is not the SDK message" }; + const authenticationError = await caught( + unary(async () => + Response.json(authenticationProblem, { + headers: { "x-request-id": "authentication-401" }, + status: 401, + }), + ), + ); + expect(authenticationError).toBeInstanceOf(AuthenticationError); + expect(authenticationError).toMatchObject({ + cause: authenticationProblem, + code: "authentication", + message: "Authentication failed", + requestId: "authentication-401", + status: 401, + transport: "http", + }); + }); + + it("HTTP transport body-read and cancellation failures retain their causes", async () => { + const networkCause = new TypeError("connection reset"); + const networkError = await caught( + unary(async () => { + throw networkCause; + }), + ); + expect(networkError).toBeInstanceOf(TransportError); + expect(networkError).toMatchObject({ cause: networkCause, transport: "http" }); + + const bodyReadCause = new Error("response body stream failed"); + const bodyReadError = await caught( + unary( + async () => + new Response( + new ReadableStream({ + start(controller) { + controller.error(bodyReadCause); + }, + }), + ), + ), + ); + expect(bodyReadError).toBeInstanceOf(ProtocolError); + expect(bodyReadError).toMatchObject({ + cause: bodyReadCause, + message: "The mecatl server returned invalid JSON", + status: 200, + transport: "http", + }); + + const abortCause = new Error("caller cancelled"); + const controller = new AbortController(); + controller.abort(abortCause); + const abortError = await caught( + createHttpTransport({ + baseUrl, + fetch: async (_input, init) => { + if (init?.signal?.aborted === true) throw init.signal.reason; + throw new Error("expected an aborted signal"); + }, + }).unary(ScheduleService.method.getSchedule, controller.signal, undefined, undefined, { + name: "nightly", + }), + ); + expect(abortError).toBeInstanceOf(TransportError); + expect(abortError).toMatchObject({ cause: abortCause, transport: "http" }); + + const readerCause = new Error("SSE reader failed"); + const encoder = new TextEncoder(); + let reads = 0; + const readerFailureStream = await watch( + async () => + new Response( + new ReadableStream({ + pull(streamController) { + if (reads === 0) { + reads += 1; + streamController.enqueue(encoder.encode(data(validWatchFrame))); + return; + } + streamController.error(readerCause); + }, + }), + { headers: { "content-type": "text/event-stream" } }, + ), + ); + const readerFailureIterator = readerFailureStream.message[Symbol.asyncIterator](); + await expect(readerFailureIterator.next()).resolves.toMatchObject({ + done: false, + value: { cursor: "before-malformed" }, + }); + await expect(readerFailureIterator.next()).rejects.toBe(readerCause); + }); +}); diff --git a/user-docs/building/typescript-sdk/connect.md b/user-docs/building/typescript-sdk/connect.md index 7443b76ad9..ee9528b24b 100644 --- a/user-docs/building/typescript-sdk/connect.md +++ b/user-docs/building/typescript-sdk/connect.md @@ -123,8 +123,13 @@ keep their original meaning. Decoded messages contain the standard protobuf-es values. `getRawJson()` still returns the original parsed HTTP value, including object-form timestamps and durations and unknown fields. A malformed or out-of-range value raises a -`ProtocolError` with `transport` set to `"http"`. Use the decoded values directly -and handle this error as a response-protocol failure. +[`ProtocolError`](/reference/typescript-sdk-api/core.md#api-protocolerror-class) +with `transport` set to `"http"`. Malformed successful unary responses and +ordinary SSE data frames do not attach the JSON or protobuf decoder as the +error's `cause`, because a runtime decoder message can contain response text. +Use `code`, `status`, and `requestId` for logging and correlation. Server, +authentication, network, and response-body read failures keep their documented +cause behavior. ## Observe connection status diff --git a/user-docs/reference/typescript-sdk-api/core.md b/user-docs/reference/typescript-sdk-api/core.md index f642997055..f8bee9b591 100644 --- a/user-docs/reference/typescript-sdk-api/core.md +++ b/user-docs/reference/typescript-sdk-api/core.md @@ -553,7 +553,7 @@ readonly reason: PromptValidationReason; ProtocolError -A transport response violated the SDK's protocol contract. +A transport response violated the SDK's protocol contract. Malformed successful HTTP responses and ordinary SSE data frames omit the underlying JSON or protobuf decoder cause. They retain safe correlation metadata such as HTTP status and a response request ID when available. Server errors, authentication failures, and transport or body-read failures keep their separately defined cause behavior. ```ts export declare class ProtocolError extends MecatlError