diff --git a/docs/deviations.md b/docs/deviations.md index d0fab78..ea1d822 100644 --- a/docs/deviations.md +++ b/docs/deviations.md @@ -502,6 +502,7 @@ frozen tree and is amended only deliberately, by hand. When §10 is next amended | **`HTTP-35`'s timeout check is read as the FULL range `AbortSignal.timeout()` accepts, not the lower bound the requirement enumerates.** `HTTP-35` says the options builder "MUST reject a non-null timeout that is zero or negative". `RequestOptionsBuilder.timeoutMs` rejects three more classes: non-finite (shipped unledgered before this audit), non-integer, and anything above `2**32 - 1`. **Strictly stricter than the letter, and deliberately so.** The field has exactly one consumer — `composeSignal` hands it to `AbortSignal.timeout()` — so a value this setter admits and that function refuses is `HTTP-35`'s own failure mode with the seam moved: the error surfaces inside a transport, as an unwrapped platform `RangeError`, one frame away from the call that supplied it. The earlier reading accepted `1.5` and argued in TSDoc that "a timeout is a duration and a fractional millisecond is meaningful"; no consumer of the field can express one. **The range checked is Node's, and that is the point:** `AbortSignal.timeout(1.5)` and `AbortSignal.timeout(2 ** 32)` raise `RangeError` on Node and are ACCEPTED on Bun, and a negative delay is `RangeError` on Node against `TypeError` on Bun (measured 2026-09-05), so leaving the check to the runtime would make an SDK-level contract depend on which runtime the caller happens to be on. *Rejected:* rounding with `Math.ceil` and clamping inside `composeSignal`, which hides the caller's mistake in the one place `HTTP-35` exists to surface it. `composeSignal` is documented as still able to raise, because a transport's own `defaultTimeoutMs` construction option bypasses this setter and is not validated by core — recorded for #81/#82, not fixed here | audit #67 / #76 | 2026-09-05 | `docs/product-spec/04-core-http-domain-model.md:48` is `HTTP-35`'s wording. `packages/core/src/http/request-options.ts:12` (`MAX_TIMEOUT_MS`) and `:204-214` (the check and the rewritten TSDoc paragraph); `packages/core/src/seams/transport.ts:86-92` is `composeSignal`'s new `@throws`, which states the two-runtime divergence rather than naming one error class. Pinned by "rejects a fractional timeout, which no transport deadline can honor" (`packages/core/src/http/request-options.test.ts:128`, the FLIPPED case — it pinned acceptance until this audit), "rejects a timeout above AbortSignal.timeout()'s ceiling of 2**32 - 1" (`:134`), "accepts the ceiling itself" (`:143`) and the `every accepted timeout is an integer in 1..2**32 - 1` property (`:157`); the Node half is `composeSignal timeout range on Node (HTTP-35)` in `tests/node-conformance/seams.test.mjs:105`, which cannot live in `bun test` because Bun accepts both rejected values | not yet in §10 | | **`HTTP-31`'s "falls back to raw text rather than throwing" is satisfied for an unpaired surrogate by SUBSTITUTING U+FFFD, not by keeping the raw text.** `HTTP-31` (MUST) makes `QueryParams.parse` lenient and enumerates the lenient cases, ending with "malformed percent-encoding falling back to raw text rather than throwing". An unpaired surrogate is a fourth kind of malformed input the enumeration does not name, and the fallback it prescribes is not available for it: the raw text has no UTF-8 form, so keeping it produces a `QueryParams` whose `encode()` throws `URIError` — the throw merely deferred out of `parse` and into an accessor that documents no throw at all. **The port repairs instead.** `parse` runs `toWellFormed()` over each decoded name and value, so every instance it returns is encodable, which is what "parsing MUST invert encode" needs to mean. The strict half of the rule is unaffected and is where `#76` puts the rejection: `QueryParamsBuilder.add` throws `UrlConstructionError` for the same input, and `substitutePathParams` throws `OperationAssemblyError`. That asymmetry is not new to the query model — it is exactly the outbound/inbound split `Headers` already draws for `HTTP-18` against `HTTP-19`, applied to the one requirement pair that needs it here. Substitution matches the platform rather than inventing a policy: `new URL('https://x/?a=\uD800').search` is `?a=%EF%BF%BD` (measured 2026-09-05). *Rejected:* letting `parse` throw the builder's error, which breaks a MUST. *Rejected:* dropping the offending parameter, which loses a name the caller may be matching on | audit #67 / #76 | 2026-09-05 | `docs/product-spec/04-core-http-domain-model.md:42` carries `HTTP-31`'s wording (shared with `HTTP-30`). `packages/core/src/http/rfc3986.ts:17-18` are the two patterns, `:31` `hasLoneSurrogate` (strict) and `:44` `toWellFormed` (lenient) — one rule, two entry points, so no caller can pick the wrong one; `packages/core/src/http/query-params.ts:144-150` is `parse`'s repair with the `HTTP-18`/`HTTP-19` comparison stated inline, against `:44-50` and `:240-241` for the strict `add` path; `packages/core/src/seams/operation.ts:139-144` is the path-param half. `/\p{Surrogate}/u` rather than `String.prototype.isWellFormed()` because the latter is ES2024 and `tsconfig.base.json:5-11` pins `lib: ES2023`, though the `engines.node >= 20.3` runtime has it. Pinned by the `lone surrogates are rejected where they are supplied (HTTP-29, HTTP-31)` block in `packages/core/src/http/query-params.test.ts:170` — "parse() stays lenient and substitutes U+FFFD, because HTTP-31 forbids throwing" (`:197`) and the `no anything escapes parse()` property (`:233`) | not yet in §10 | | **`OBS-35`'s "MUST NOT bake in a default config key name" is satisfied by making the key configurable, not by removing the default.** `OBS-35` (SHOULD) asks for a tolerant, layered log-level resolution and adds one MUST: no baked-in default key name. The port ships `CFG_KEY_LOG_LEVEL` (`DEXPACE_LOG_LEVEL`) as `CFG-14`'s well-known key and, until 2026-09-05, read it unconditionally. It is now `LoggingStepSettings.configKey`'s default: a caller names their own key and the resolution is otherwise identical. **Why the default stays.** A required key would mean no caller gets ambient granularity without naming one first, which trades a MUST about *naming* for a worse default experience, and `CFG-14` — which this port also implements — exists precisely to standardise the name. The layered resolution itself is `CFG-1`'s (override → environment → normalised property → default) and is tolerant as the requirement asks. **A second, quieter half:** the process-wide configuration slot starts empty (`CFG-13`), so no key of any name resolves until a host calls `setGlobalConfiguration(defaultConfiguration())`. That is deliberate — defaulting the slot to a configuration that reads `process.env` would make an import-time environment read the SDK's default behaviour — and it is now documented as the required wiring rather than left to be discovered | audit #67 / #80 | 2026-09-05 | `packages/core/src/observability/logging-step.ts:64-79,94-104` (the setting and the resolution); `packages/core/src/config/configuration.ts:311,354-358,368` (`CFG_KEY_LOG_LEVEL`, `defaultConfiguration`, the empty default slot); `docs/sdk-documentation/pipelines.md` "Turning logging on from the environment"; `docs/product-spec/15-instrumentation-and-observability.md:66` (the requirement) | not yet in §10 | +| **`CFG-22`'s SOCKS proxy types are resolved by the configuration layer and supported by neither shipped transport; the refusal is at the transport factory, and `ProxyType` keeps them.** `CFG-22` (MUST) requires the proxy model to carry "the proxy protocol type (HTTP, SOCKS4, SOCKS5)", and the port implements it in full: `ProxyType` is `'http' \| 'socks4' \| 'socks5'`, and `resolveProxyOptions` maps `ALL_PROXY`/`HTTPS_PROXY`'s `socks:`, `socks4:`, `socks4a:`, `socks5:` and `socks5h:` schemes onto it. Nothing can then send over one. `@dexpace/transport-undici` builds undici's `ProxyAgent`, which is an HTTP `CONNECT` tunnel reading its `uri` as a URL, and `@dexpace/transport-fetch` ships no `proxy` option at all because Node's bare global `fetch` exposes no proxy hook outside undici internals. So a configuration that resolves cleanly has no transport that can honour it. **What changed on 2026-09-05 (audit #67 / #81).** Until then the discovery was `new ProxyAgent({uri: 'socks5://…'})` throwing undici's `InvalidArgumentError('Invalid URL protocol: socks5:')` out of a public factory — untyped, undocumented, and outside the SDK's error vocabulary. `undiciTransport()` now refuses `proxy.type !== 'http'` at construction with a `TypeError` naming the type, before any dispatcher is allocated, deliberately outside the `IoError` tree so `retry/classify.ts`'s allow-list makes it non-retryable (RETRY-2). That is `TRANSPORT-30`'s "make the limitation discoverable rather than silently misbehaving" applied at the earliest point that can. **Why `ProxyType` still admits `socks4`/`socks5`.** Narrowing a `@public` union is a breaking change and therefore a release-pass decision, which this run is not taking (the run's release machinery is suspended); and `CFG-22`'s MUST is about the *model*, which would then no longer satisfy it. The honest state is a configuration layer that is complete and a transport layer that is not, which is what this row records. A future transport — a `node:net` SOCKS dialer, or a `ProxyAgent` replacement — closes the gap without a model change. | audit #67 / #81 | 2026-09-05 | `packages/core/src/config/proxy.ts:34` (`ProxyType`), `:372-380` (the scheme map); `packages/transport-undici/src/undici-transport.ts:138,151-158,192` (the supported type, the refusal, and where it runs); `packages/transport-fetch/src/fetch-transport.ts:76-79` (no `proxy` option, and why); `docs/product-spec/16-configuration.md:42` (`CFG-22`); `docs/product-spec/17-transport-adapter-conformance-contract.md:48` (`TRANSPORT-30`) | not yet in §10 | ### Proposed erratum for `PIPE-40` (drafted 2026-09-04, not applied) diff --git a/docs/sdk-documentation/write-a-transport.md b/docs/sdk-documentation/write-a-transport.md index dbf2648..39f88c5 100644 --- a/docs/sdk-documentation/write-a-transport.md +++ b/docs/sdk-documentation/write-a-transport.md @@ -44,7 +44,7 @@ export function echoTransport(): Transport { Note `setInbound`, not `set`: values a server sent are accepted leniently. Using the strict setter on a real server's headers means a response with an obs-text byte in it becomes unreadable. -## Nine rules a real transport must follow +## Eleven rules a real transport must follow The full contract is `docs/product-spec/17-transport-adapter-conformance-contract.md`, thirty `TRANSPORT-N` clauses. These are the ones that are easy to get wrong. @@ -61,24 +61,36 @@ client, so forwarding a caller's copy corrupts framing. `Connection` is in the d `fetch`-class transport and not for an undici-class one — §17 says so explicitly. Log the **name**, never the value, and dedupe per name by default. -**3. Map aborts to exactly two errors** (`TRANSPORT-3`/`TRANSPORT-4`/`TRANSPORT-8`). A timeout is the +**3. Whatever your native client refuses, drop that header — never the request** (`TRANSPORT-12`). +This is the half that is easy to miss, because the refusal happens somewhere you are not looking. +`@dexpace/core` admits every printable ASCII byte in a header name (`http/ascii-validation.ts`), so +`X Custom` is a model-valid name no HTTP client on this platform will carry. Both shipped clients +also refuse `Expect`, `Keep-Alive` and `Upgrade` outright, and undici refuses `Connection` with any +value but `close`/`keep-alive`. Find out *where* your client decides: WHATWG `Headers.append` throws +at construction, which a `try`/`catch` degrades for free; undici validates inside `dispatch`, so +that transport has to ask the question itself before handing the array over. Getting this wrong +does not look like a bug in your transport — it looks like a retryable network failure that burns +the caller's whole retry budget re-proving a permanent misconfiguration. The shared suite has a row +per name. + +**4. Map aborts to exactly two errors** (`TRANSPORT-3`/`TRANSPORT-4`/`TRANSPORT-8`). A timeout is the retryable `TransportFailureError`; a caller abort is the terminal `CancellationError`. A raw `DOMException` must never surface. `isTimeoutSignal(signal)` is how you tell them apart. -**4. An abort after delivery must not close the delivered body** (`SEAM-16`). Both native clients tie +**5. An abort after delivery must not close the delivered body** (`SEAM-16`). Both native clients tie a response body's lifetime to the signal they were given, so dispatch over a **fork** of the signal and detach it at delivery. Get this wrong and a caller who aborts a moment after `send()` resolves finds the body they already own torn out from under them. -**5. The caller owns the response body** (`BODY-15`). Return it live and unread. Do not buffer it, do +**6. The caller owns the response body** (`BODY-15`). Return it live and unread. Do not buffer it, do not close it. -**6. Ownership decides who closes what** (`SEAM-14`). A dispatcher or client the caller supplied is +**7. Ownership decides who closes what** (`SEAM-14`). A dispatcher or client the caller supplied is never touched by your `close()`. One you constructed is yours to close. Make that decision once, at construction, and make supplying both a caller-owned client *and* an option that would build one a construction-time `TypeError` rather than a silent win for one of them. -**7. `close()` must be idempotent, concurrent-safe, and non-blocking** (`TRANSPORT-15`/`TRANSPORT-16`). +**8. `close()` must be idempotent, concurrent-safe, and non-blocking** (`TRANSPORT-15`/`TRANSPORT-16`). No unbounded await — a graceful drain would stall teardown for as long as one in-flight send against a slow peer takes. Destroying is the sanctioned choice; in-flight sends then reject with `CancellationError`, and so does a `send()` issued after `close()`, because it cannot succeed over a @@ -86,11 +98,28 @@ dispatcher that no longer exists and so is not a retryable failure. Declare your (`SEAM-15`) either way: `@dexpace/transport-fetch`'s `close()` is a documented no-op over a runtime global it does not own, and `send()` keeps working after it. -**8. Recognize a file body structurally** (`TRANSPORT-28`). `body.kind === 'file'` widens the body to -`FileBodyDescriptor` — `path`, `start`, `count` — and lets you dispatch straight off the file. Never -`instanceof` against `@dexpace/body-file`: a transport must not depend on it. - -**9. Send a real `User-Agent`** (`NFR-15`), never a placeholder. `getBuildInfo()` supplies the tokens. +**9. Recognize a file body structurally, and still write it through `writeTo`** +(`TRANSPORT-28`, `BODY-13`). `body.kind === 'file'` widens the body to `FileBodyDescriptor` — +`path`, `start`, `count`. Never `instanceof` against `@dexpace/body-file`: a transport must not +depend on it. + +Reading `path` yourself is the trap. It is a shorter path to the wire, and it skips the +descriptor's own `writeTo`, which is where `BODY-13`'s `transferred === count` check lives — the +only thing that can notice a file truncated between `stat` and `send`, because `Content-Length` is +dropped outbound (rule 2) so the framing cannot. `@dexpace/transport-undici` did exactly this until +2026-09-05 and uploaded short files with a 200. Unless your client has a genuine kernel `sendfile` +path, treat a file body as an ordinary `Body` and let `writeTo` produce the bytes; `TRANSPORT-28`'s +zero-copy clause is a SHOULD, and its MUSTs — replayable, and exactly the declared range on the +wire — are the descriptor's to keep, not yours. + +**10. Refuse a proxy you cannot honour, at construction** (`TRANSPORT-30`). `ProxyType` admits +`socks4` and `socks5`, and core resolves both from `ALL_PROXY`, so a configuration can hand you a +proxy your client cannot build. Reject it in the factory with a typed error that names the type, +before you allocate anything — not on the first send, where it arrives as whatever the native +client raises. Keep it outside the `IoError` tree: `retry/classify.ts` is an allow-list, so a +misconfiguration no retry can fix is then non-retryable for free. Declare it in `@throws`. + +**11. Send a real `User-Agent`** (`NFR-15`), never a placeholder. `getBuildInfo()` supplies the tokens. ## Prove it @@ -105,10 +134,13 @@ runTransportConformanceSuite('my-transport', () => myTransport(), { supportsInternalCancel: false, // TRANSPORT-8: a cancel path distinct from a caller abort supportsProxy: false, // TRANSPORT-30 dropsConnectionHeader: true, // TRANSPORT-11: is `Connection` in your drop set? + // TRANSPORT-30, optional: a proxy type your configuration can express and your client cannot + // honour. Omit it and the row asserts `supportsProxy` is false, rather than skipping. + // unsupportedProxy: {type: 'socks5', build: () => myTransport({proxy: socks5Proxy})}, }); ``` -The three capability flags are the only clauses §17 scopes to a subset of transports; everything else +Those capability entries are the only clauses §17 scopes to a subset of transports; everything else runs unconditionally. The suite starts its own fixture server, and a second one on a separate origin for the rows that deliberately leave a connection unusable — a client that reuses a poisoned connection otherwise fails thirty rows downstream, which is a debugging problem of a different order. @@ -120,15 +152,15 @@ The package is `private` and its `exports` name `./src/index.ts`, so it resolves `@dexpace/transport-shared` exists so the algorithm both adapters need exists once. Its exports are `@internal` and it is not a package to install directly, but reading it is the fastest way to see -what a correct implementation of rules 2, 3, 4 and 6 looks like: +what a correct implementation of rules 2, 3, 4, 5 and 7 looks like: | Module | Concern | |---|---| -| `header-mapping.ts` | The outbound drop-and-degrade pass and the lenient inbound copy | +| `header-mapping.ts` | Rules 2 and 3: the outbound drop-and-degrade pass, and the lenient inbound copy | | `drop-log.ts` | Bounded, case-insensitive, drain-to-cap dedup of already-logged drop names | | `abort-mapping.ts` | The single mapping from an aborted signal to `TransportFailureError` or `CancellationError` | | `body-pump.ts` | Turning a `Body` into a request stream the transport owns, plus idempotent teardown for an abandoned producer | -| `signal-fork.ts` | Rule 4's fork-and-detach | +| `signal-fork.ts` | Rule 5's fork-and-detach | ## Package it diff --git a/packages/transport-conformance/src/fixtures.ts b/packages/transport-conformance/src/fixtures.ts index fe20b7f..6088a21 100644 --- a/packages/transport-conformance/src/fixtures.ts +++ b/packages/transport-conformance/src/fixtures.ts @@ -1,11 +1,13 @@ // SPDX-License-Identifier: MIT // packages/transport-conformance/src/fixtures.ts +import {createReadStream} from 'node:fs'; import { createServer, type IncomingMessage, type Server, type ServerResponse, } from 'node:http'; +import type {FileBodyDescriptor} from '@dexpace/core'; /** A running fixture server, addressable by URL and shut down through {@link TestServer.close}. */ export interface TestServer { @@ -168,3 +170,77 @@ export function startFixtureServer(): Promise { }); }); } + +/** What {@link fileBodyFixture} needs beyond the path; mirrors `fileBody()`'s own option bag. */ +export interface FileBodyFixtureOptions { + /** The byte offset the descriptor declares; defaults to 0. */ + readonly start?: number; + /** The byte count the descriptor declares, captured as `fileBody()` captures it from `stat`. */ + readonly count: number; + /** Incremented on every `writeTo` call, so a row can assert the transport used it. */ + readonly writes?: {count: number}; +} + +/** + * A `kind: 'file'` request body over a real path, carrying BODY-13's `transferred === count` check + * itself — the shape `@dexpace/body-file`'s `fileBody()` produces, minus the construction-time + * validation no row here needs. + * + * **Deliberately a stand-in, not the real factory.** `@dexpace/transport-conformance` is `private`, + * resolves unbuilt, and depends on `@dexpace/core` alone; taking `@dexpace/body-file` would put a + * ninth entry in the root `build:deps` chain for one row. A real `fileBody()` crossing a real + * transport already has a home — `tests/node-conformance/transport.test.mjs`, which is the only + * layer that can host it. What a *transport* is answerable for is narrower, and is exactly what this + * exercises: TRANSPORT-28's structural recognition on `kind` alone, and that the declared length is + * honoured by calling the descriptor's own `writeTo` rather than by reading `path` behind its back. + * + * @param path - the file to stream; read fresh on every `writeTo`, as BODY-11 requires. + * @param options - the declared range, and an optional write counter. + * @returns a frozen descriptor a transport must recognise structurally. + */ +export function fileBodyFixture( + path: string, + options: FileBodyFixtureOptions, +): FileBodyDescriptor { + const start = options.start ?? 0; + const {count} = options; + return Object.freeze({ + kind: 'file' as const, + mediaType: 'application/octet-stream', + contentLength: count, + replayable: true, + path, + start, + count, + async writeTo(sink: WritableStream): Promise { + if (options.writes !== undefined) options.writes.count += 1; + const writer = sink.getWriter(); + if (count === 0) { + writer.releaseLock(); + return; + } + const stream = createReadStream(path, {start, end: start + count - 1}); + let transferred = 0; + try { + for await (const chunk of stream) { + const bytes = chunk as Buffer; + await writer.write(new Uint8Array(bytes)); + transferred += bytes.byteLength; + } + if (transferred !== count) { + // BODY-13's exact sentence, and its exact wording in `@dexpace/body-file`: the error names + // transferred-of-total, so a row can assert the numbers rather than only the class. + throw new Error( + `short write: transferred ${String(transferred)} of ${String(count)} bytes`, + ); + } + } catch (error) { + await writer.abort(error); + throw error; + } finally { + stream.destroy(); + writer.releaseLock(); + } + }, + }); +} diff --git a/packages/transport-conformance/src/run-suite.ts b/packages/transport-conformance/src/run-suite.ts index e7a416f..50d66bc 100644 --- a/packages/transport-conformance/src/run-suite.ts +++ b/packages/transport-conformance/src/run-suite.ts @@ -1,21 +1,29 @@ // SPDX-License-Identifier: MIT // packages/transport-conformance/src/run-suite.ts // The single TRANSPORT-N conformance suite, run once per transport package so the two adapters cannot -// drift. Exercises: TRANSPORT-1..9, TRANSPORT-14..17, TRANSPORT-20..21, TRANSPORT-23..27, -// TRANSPORT-29, SEAM-12, SEAM-16, SEAM-30, NFR-15, and AUTH-12/AUTH-25 to the extent a transport is +// drift. Exercises: TRANSPORT-1..9, TRANSPORT-11..17, TRANSPORT-20..21, TRANSPORT-23..29, BODY-13, +// SEAM-12, SEAM-16, SEAM-30, NFR-15, and AUTH-12/AUTH-25 to the extent a transport is // answerable for them (the repeated-challenge-header row). TRANSPORT-10..13's SHARED half -- the one // outbound header pass both adapters call -- is asserted at its source in -// @dexpace/transport-shared, and the rows here cover only what each adapter decides for itself. -// TRANSPORT-18/28's collapses are Deviation Ledger rows; TRANSPORT-30's -// full flow is transport-undici's challenge-handler.test.ts. TRANSPORT-22 is NOT driven from here -- +// @dexpace/transport-shared; the rows here cover what each adapter decides for itself, which since +// audit #67 / #81 includes TRANSPORT-11/12's per-header degrade, because the two adapters had four +// different answers for the same model-valid header and only a shared row could say so. +// TRANSPORT-18's collapse is a Deviation Ledger row, as is TRANSPORT-28's zero-copy SHOULD, whose two +// MUSTs the file-body rows do assert; TRANSPORT-30's +// full proxy-challenge flow is transport-undici's challenge-handler.test.ts, and only its +// unsupported-type refusal is a row here. TRANSPORT-22 is NOT driven from here -- // forcing an adaptation throw needs a per-transport hook into the native response, so each adapter // asserts it against its own (transport-fetch's fetch-transport.test.ts:118, transport-undici's -// undici-transport.test.ts:503). +// undici-transport.test.ts:614). +import {mkdtemp, rm, truncate, writeFile} from 'node:fs/promises'; +import {tmpdir} from 'node:os'; +import {join} from 'node:path'; import {afterAll, beforeAll, describe, expect, test} from 'bun:test'; import { getBuildInfo, getGlobalLogger, Headers, + isIoError, Request, RequestOptions, setGlobalLogger, @@ -24,11 +32,27 @@ import { type Transport, } from '@dexpace/core'; import { + fileBodyFixture, REPEATED_CHALLENGES, startFixtureServer, type TestServer, } from './fixtures.js'; +/** + * A proxy configuration the transport under test cannot honour, so TRANSPORT-30's + * "discoverable rather than silently misbehaving" clause has something to be asserted against. + * + * Supplied by the adapter because only the adapter knows which of `ProxyType`'s values its native + * client refuses — `@dexpace/core` resolves `socks4`/`socks5` from `ALL_PROXY` (CFG-22) and neither + * shipped transport can carry either. + */ +export interface UnsupportedProxy { + /** The `ProxyOptions.type` the native client cannot honour; the refusal must name it. */ + readonly type: string; + /** Builds a transport configured with that type. Expected to throw rather than return one. */ + build(): Transport; +} + /** * The clauses `docs/product-spec/17-transport-adapter-conformance-contract.md` scopes to only one * reference transport, plus the one drop-set entry that legitimately differs between the two. @@ -43,6 +67,12 @@ export interface TransportCapabilities { readonly supportsProxy: boolean; /** TRANSPORT-11: whether `Connection` is in this transport's outbound drop set. */ readonly dropsConnectionHeader: boolean; + /** + * TRANSPORT-30: a proxy type this transport's configuration can express and its native client + * cannot honour. Omit it when the transport takes no proxy at all, or honours every type it can + * be handed — the row then asserts that omission is the truth rather than skipping. + */ + readonly unsupportedProxy?: UnsupportedProxy; } /** What every row below needs: a transport factory, the live fixture origin, and the capability flags. */ @@ -538,6 +568,200 @@ function registerHeaderRows(ctx: SuiteContext): void { }); } +/** + * Headers every model layer in this SDK accepts and at least one shipped native client refuses + * outright, paired with the value that provokes the refusal. + * + * `expect`, `keep-alive` and `upgrade` are `undici`'s three unconditional rejections + * (`lib/core/request.js:398,409` in 6.28.0 — `InvalidArgumentError` for the first two, + * `NotSupportedError` for `expect`), and Node's global `fetch` is undici-backed, so both adapters + * meet them. `X Custom` is the non-token name: `@dexpace/core` admits any printable ASCII byte in + * a header name (`http/ascii-validation.ts:29-33`), while both native layers require RFC 9110 + * `token`. + * + * TRANSPORT-12 is what makes these one table rather than four transport-specific quirks: whatever + * the native client refuses, the transport drops *that header only* and still dispatches. + */ +const NATIVE_REJECTED_HEADERS: readonly (readonly [string, string])[] = [ + ['Expect', '100-continue'], + ['Keep-Alive', 'timeout=5'], + ['Upgrade', 'websocket'], + ['X Custom', 'model-valid, non-token'], +]; + +function registerNativeRejectionRows(ctx: SuiteContext): void { + describe('TRANSPORT-11/12/13: a header the native client refuses degrades to a logged drop', () => { + for (const [name, value] of NATIVE_REJECTED_HEADERS) { + test(`${name} is dropped and logged, and the rest of the request still dispatches`, async () => { + let echoed: Record = {}; + const dropped = await captureDroppedHeaders(async () => { + await withTransport(ctx.makeTransport, async transport => { + const request = Request.newBuilder() + .url(ctx.url('/echo-headers')) + .headers( + Headers.newBuilder() + .set(name, value) + .set('X-Pass-Through', 'survives') + .build(), + ) + .build(); + echoed = await readEchoedHeaders(transport, request); + }); + }); + // The send resolved at all, which is TRANSPORT-12's "the resulting native exception MUST NOT + // escape the send contract"; the two header assertions are its "the bad header is absent, + // the normal header present". + expect(echoed[name.toLowerCase()]).toBeUndefined(); + expect(echoed['x-pass-through']).toBe('survives'); + // TRANSPORT-13: the drop is discoverable by name, never silent. + expect(dropped).toContain(name.toLowerCase()); + }); + } + + test('a Connection value the native client cannot carry is dropped on both transports', async () => { + // `Connection` is the one name whose drop set legitimately differs (TRANSPORT-11's own note, + // and `registerDropSetRows` below), so this row asserts the intersection: `upgrade` is neither + // `close` nor `keep-alive`, undici rejects it outright (`request.js:400-404`), and the WHATWG + // layer forbids the name entirely. Both must drop it, whichever reason applies. + // + // Asserted through the log rather than the echo for the same reason `registerDropSetRows` + // does: both clients set a `Connection` header of their own, so the wire cannot tell a + // forwarded caller header from the client's. + const dropped = await captureDroppedHeaders(async () => { + await withTransport(ctx.makeTransport, async transport => { + const request = Request.newBuilder() + .url(ctx.url('/echo-headers')) + .headers(Headers.newBuilder().set('Connection', 'upgrade').build()) + .build(); + const response = await transport.send(request); + expect(response.status.code).toBe(200); + await response.close(); + }); + }); + expect(dropped).toContain('connection'); + }); + }); +} + +/** + * The declared length of the truncate-after-stat file body, deliberately **below** both shipped + * adapters' 1,000,000-byte materialize bound, so this row drives the buffered path. + * + * The streamed path is asserted in `tests/node-conformance/transport.test.mjs` instead, and that is + * not a preference. Bun 1.3.14's `Readable.fromWeb` leaks the abort reason as two or three unhandled + * rejections when the web readable behind it is aborted mid-pull, which is exactly what a producer + * failure on the streamed path does; `bun:test` then fails whichever row happens to be running. + * Isolated to sixteen lines with no SDK code in them, and clean under `node --test` on both + * transports (measured 2026-09-05, audit #67 / #81). Raising this constant past 1,000,000 will make + * the row red for that reason and no other. + */ +const TRUNCATED_FILE_BYTES = 64; + +/** How far a truncate-after-stat cuts the file back; small enough that no read can be a full one. */ +const TRUNCATED_TO_BYTES = 10; + +/** The intact file-body fixture's size, and the byte range the ranged row asks for inside it. */ +const INTACT_FILE_BYTES = 64; +const INTACT_RANGE = {start: 10, count: 20} as const; + +/** Distinguishable bytes, so a misaligned send fails on content and not merely on length. */ +function fileFixtureBytes(size: number): Uint8Array { + const bytes = new Uint8Array(size); + for (let index = 0; index < size; index += 1) { + bytes[index] = 33 + ((index * 7) % 94); + } + return bytes; +} + +/** Writes `size` fixture bytes to a fresh temporary file, runs `body`, and removes the directory. */ +async function withFixtureFile( + size: number, + body: (path: string) => Promise, +): Promise { + const dir = await mkdtemp(join(tmpdir(), 'dexpace-conformance-file-')); + try { + const path = join(dir, 'payload.bin'); + await writeFile(path, fileFixtureBytes(size)); + return await body(path); + } finally { + await rm(dir, {recursive: true, force: true}); + } +} + +/** + * Every message on `error` and its `cause` chain, joined. + * + * BODY-13's transferred-of-total text surfaces at a different depth per path: the streamed path + * rethrows the producer's own failure as the message, while the buffered path wraps it as a cause + * under a fixed "request body could not be written". Both satisfy the requirement; asserting on the + * chain is what lets one row cover both without pinning either transport's wrapper wording. + */ +function messageChain(error: unknown): string { + const parts: string[] = []; + let current: unknown = error; + for (let depth = 0; depth < 8 && current instanceof Error; depth += 1) { + parts.push(current.message); + current = current.cause; + } + return parts.join(' <- '); +} + +function registerFileBodyRows(ctx: SuiteContext): void { + describe('TRANSPORT-28, BODY-13: a file body is dispatched through its own writeTo', () => { + test('an intact ranged file body puts exactly its declared bytes on the wire', async () => { + await withFixtureFile(INTACT_FILE_BYTES, async path => { + const writes = {count: 0}; + await withTransport(ctx.makeTransport, async transport => { + const response = await transport.send( + Request.newBuilder() + .method('POST') + .url(ctx.url('/echo-body')) + .body(fileBodyFixture(path, {...INTACT_RANGE, writes})) + .build(), + ); + // TRANSPORT-28's "assert exactly that byte range reaches the wire". + expect([...(await response.bytes())]).toEqual([ + ...fileFixtureBytes(INTACT_FILE_BYTES).slice( + INTACT_RANGE.start, + INTACT_RANGE.start + INTACT_RANGE.count, + ), + ]); + }); + // TRANSPORT-17's counterpart for a replayable body: a transport that reads `path` itself + // rather than calling `writeTo` would put the same bytes on the wire and leave this at 0. + expect(writes.count).toBe(1); + }); + }); + + test('a file body truncated after its length was captured fails the send', async () => { + await withFixtureFile(TRUNCATED_FILE_BYTES, async path => { + // The descriptor is built first, exactly as `fileBody()` captures `count` from `stat`, and + // the file shrinks underneath it afterwards. `content-length` is dropped outbound, so the + // wire cannot detect this either — BODY-13's check inside `writeTo` is the only thing that + // can, and a transport that hands the path to its native client never runs it + // (audit #67 / #81, where undici POSTed the ten surviving bytes and resolved 200). + const body = fileBodyFixture(path, {count: TRUNCATED_FILE_BYTES}); + await truncate(path, TRUNCATED_TO_BYTES); + const error = await withTransport(ctx.makeTransport, transport => + rejection( + transport.send( + Request.newBuilder() + .method('POST') + .url(ctx.isolatedUrl('/echo-body')) + .body(body) + .build(), + ), + ), + ); + expect(error).toMatchObject({name: 'TransportFailureError'}); + expect(messageChain(error)).toContain( + `transferred ${String(TRUNCATED_TO_BYTES)} of ${String(TRUNCATED_FILE_BYTES)} bytes`, + ); + }); + }); + }); +} + /** * The fixture's challenge list, recovered from however this transport chose to split it. * @@ -603,6 +827,37 @@ function registerDropSetRows(ctx: SuiteContext): void { }); } +function registerProxyRefusalRows(ctx: SuiteContext): void { + describe('TRANSPORT-30: a proxy the native client cannot honour is refused, not attempted', () => { + test('an unsupported proxy type fails at construction with a typed, naming error', () => { + const unsupported = ctx.capabilities.unsupportedProxy; + if (unsupported === undefined) { + // The row still runs here rather than being skipped, and asserts the only thing left to + // assert: that there is genuinely no proxy surface to mis-set. `@dexpace/transport-fetch` + // is this leg — it ships no `proxy` option at all, deliberately (design doc §6), so + // TRANSPORT-30 has no subject on it and a silent skip would look the same as a gap. + expect(ctx.capabilities.supportsProxy).toBe(false); + return; + } + let thrown: unknown; + try { + // A transport that returns instead of throwing has deferred the failure to the first send, + // where it arrives as whatever the native client raises — the shape TRANSPORT-30 rules out. + void unsupported.build(); + } catch (error) { + thrown = error; + } + expect(thrown).toBeInstanceOf(TypeError); + // Not an `IoError`: `retry/classify.ts` is an allow-list, so a misconfiguration no retry can + // fix is non-retryable for free (RETRY-2). An undici `InvalidArgumentError` escaping raw + // would fail this too, being neither a `TypeError` nor documented. + expect(isIoError(thrown)).toBe(false); + // "Discoverable": the message names the type that was refused, not merely that something was. + expect((thrown as Error).message).toContain(unsupported.type); + }); + }); +} + function registerScopedRows(ctx: SuiteContext): void { if (ctx.capabilities.supportsInternalCancel) { describe('TRANSPORT-8: an internal cancel is told apart from a timeout', () => { @@ -687,8 +942,11 @@ export function runTransportConformanceSuite( registerCancellationRows(ctx); registerLifecycleRows(ctx); registerHeaderRows(ctx); + registerNativeRejectionRows(ctx); + registerFileBodyRows(ctx); registerInboundHeaderRows(ctx); registerDropSetRows(ctx); + registerProxyRefusalRows(ctx); registerScopedRows(ctx); }); } diff --git a/packages/transport-fetch/README.md b/packages/transport-fetch/README.md index 3ef2b5b..cf97160 100644 --- a/packages/transport-fetch/README.md +++ b/packages/transport-fetch/README.md @@ -50,9 +50,21 @@ records the decision and the four reasons the floor does not move instead. - Redirects are **never** followed (`redirect: 'manual'`). The SDK pipeline is the redirect authority (`TRANSPORT-1`/`TRANSPORT-2`). -- `Content-Length`, `Host`, `Transfer-Encoding`, and `Connection` are dropped outbound — the client - computes its own framing — and every drop is logged by name (never by value) through the global - logger, deduped per name by default (`TRANSPORT-11`/`TRANSPORT-13`). +- `Content-Length`, `Host` and `Transfer-Encoding` are dropped outbound because the client computes + its own framing; `Connection`, `Expect`, `Keep-Alive` and `Upgrade` because the layer underneath + refuses them. WHATWG names all four forbidden request headers, but the implementations do not + enforce that list and disagree about what happens instead: on Node the global `fetch` is + undici-backed, so an undropped `Expect`/`Keep-Alive`/`Upgrade` reaches undici's own validation and + fails the send with the **retryable** `TransportFailureError` — a permanent misconfiguration + spending the caller's whole retry budget — while Bun 1.3.14 forwards the first two to the wire and + hangs on the third until something else times the call out. Dropping the name is the one behaviour + `TRANSPORT-12` asks for, + and it matches `@dexpace/transport-undici` (measured 2026-09-05; audit #67 / #81). +- A header name the WHATWG `Headers` layer rejects — `@dexpace/core` admits every printable ASCII + byte in a name, so `X Custom` is model-valid and unsendable — degrades to the same drop, never a + failed send (`TRANSPORT-12`). +- Every drop is logged by name (never by value) through the global logger, deduped per name by + default (`TRANSPORT-11`/`TRANSPORT-13`). - An abort that fires **after** `send()` resolved does not close the delivered body: the caller owns it (`SEAM-16`). Cancellation stays live for the whole in-flight window. - A timeout surfaces as the retryable `TransportFailureError`; a caller abort as the terminal diff --git a/packages/transport-fetch/src/fetch-transport.ts b/packages/transport-fetch/src/fetch-transport.ts index e76236d..2cae39e 100644 --- a/packages/transport-fetch/src/fetch-transport.ts +++ b/packages/transport-fetch/src/fetch-transport.ts @@ -38,15 +38,29 @@ import { const REDIRECT_MODE = 'manual' as const; /** - * TRANSPORT-11's outbound drop set for this transport. `connection` is in it because WHATWG `fetch` - * treats it as a forbidden request header and would strip it silently — dropping it here makes the - * removal observable through the drop log instead. + * TRANSPORT-11's outbound drop set for this transport. + * + * `connection` is in it because WHATWG `fetch` treats it as a forbidden request header and would + * strip it silently — dropping it here makes the removal observable through the drop log instead. + * + * `expect`, `keep-alive` and `upgrade` are in it because the *implementations* do not honour the + * WHATWG forbidden-header list at all. Node's global `fetch` is undici-backed and undici's `Headers` + * deliberately does not implement forbidden names in a non-browser environment, so the three reach + * `lib/core/request.js:398,409` and reject the dispatch; `fetch()` then rejects with a bare + * `TypeError: fetch failed`, which this transport can only classify as the **retryable** + * `TransportFailureError` — a permanent misconfiguration that spends the caller's whole retry budget + * re-proving itself. Bun 1.3.14 diverges again: it forwards `expect` and `keep-alive` to the wire + * and hangs indefinitely on `upgrade`. Measured on both, 2026-09-05, audit #67 / #81. Neither + * outcome is TRANSPORT-12's, and a name in the drop set is the one behaviour that is. */ const FETCH_FORBIDDEN_HEADERS = [ 'content-length', 'host', 'transfer-encoding', 'connection', + 'expect', + 'keep-alive', + 'upgrade', ] as const; /** diff --git a/packages/transport-undici/README.md b/packages/transport-undici/README.md index 5858f98..c67c411 100644 --- a/packages/transport-undici/README.md +++ b/packages/transport-undici/README.md @@ -55,11 +55,18 @@ terminal `CancellationError`, and so does a `send()` issued after `close()` — documented `SEAM-15` post-close mode. It cannot succeed over a dispatcher that no longer exists, so it is not reported as a retryable failure. -## Proxy support and its one real limit +## Proxy support and its two real limits `ProxyOptions` routes here in full: address, Basic credentials, and `NO_PROXY`/`nonProxyHosts` bypass globs, which route over a separate direct `Agent` rather than being tunnelled anyway. +**`type` must be `http`.** undici's `ProxyAgent` is an HTTP `CONNECT` tunnel and reads its `uri` as +a URL, so `socks4`/`socks5` — which `ProxyType` admits and core resolves from `ALL_PROXY` +(`CFG-22`) — are refused at construction with a `TypeError` naming the type. Before 2026-09-05 they +reached `new ProxyAgent({uri: 'socks5://…'})` and escaped this factory as an undici +`InvalidArgumentError`, untyped and undocumented. Neither shipped transport can carry SOCKS; +`docs/deviations.md` records the gap. + **A custom `challengeHandler` cannot be dispatched**, and the limitation is surfaced rather than silently misbehaving (`TRANSPORT-30`): @@ -79,15 +86,28 @@ silently misbehaving (`TRANSPORT-30`): ## Behavior worth knowing -- File bodies (`body.kind === 'file'`, e.g. `@dexpace/body-file`'s `fileBody()`) dispatch straight - off the file honoring `start`/`count`, one fewer userspace copy than the `fetch` transport - (`TRANSPORT-28`; a literal kernel zero-copy path does not exist on Node — see the Deviation - Ledger). Recognition is structural, on `kind` alone: this package does not depend on - `@dexpace/body-file`. +- File bodies (`body.kind === 'file'`, e.g. `@dexpace/body-file`'s `fileBody()`) are written + through the descriptor's own `writeTo`, exactly as in the `fetch` transport — buffered below + 1 MB, streamed above it. Until 2026-09-05 this transport instead handed + `createReadStream(path, {start, end})` to undici: one fewer userspace copy, and no `writeTo`, so + `BODY-13`'s `transferred === count` check never ran and a file truncated between `stat` and + `send` uploaded its remaining bytes and returned 200. `content-length` is dropped outbound, so + the framing could not catch it either. `TRANSPORT-28`'s zero-copy clause is a SHOULD that no + user-space path in either client can honour anyway (Deviation Ledger item 13); its MUSTs — a file + body is replayable, and exactly its declared byte range reaches the wire — are honoured by the + descriptor. Recognition, where it is still needed, stays structural: this package does not depend + on `@dexpace/body-file`. - Redirects are pinned off (`maxRedirections: 0`) even behind a bring-your-own dispatcher that may carry a redirect interceptor. The pipeline is the single redirect authority. - `Connection` is **not** dropped outbound — §17's own note is that an undici-class transport - forwards it. `Content-Length`, `Host`, and `Transfer-Encoding` are. + forwards it — but only with a value undici will carry (`close` or `keep-alive`, matched + case-insensitively). Any other value is dropped, because undici rejects it outright. +- `Content-Length`, `Host` and `Transfer-Encoding` are dropped because undici computes them; + `Expect`, `Keep-Alive` and `Upgrade` because undici refuses them + (`InvalidArgumentError`/`NotSupportedError` out of its own argument validation, before anything + reaches the wire). So is any header name outside RFC 9110 `token` — `@dexpace/core` admits every + printable ASCII byte in a name, undici does not. Every one of these is a drop logged by name + (`TRANSPORT-12`), never a failed send. - Destroying the dispatcher mid-flight surfaces as the terminal `CancellationError`, while a timeout on the same path stays the retryable `TransportFailureError` (`TRANSPORT-8`). - `Response.protocol` is always `HTTP_1_1`: undici's `ResponseData` does not surface the negotiated diff --git a/packages/transport-undici/src/undici-transport.conformance.test.ts b/packages/transport-undici/src/undici-transport.conformance.test.ts index 866b18f..1f5106a 100644 --- a/packages/transport-undici/src/undici-transport.conformance.test.ts +++ b/packages/transport-undici/src/undici-transport.conformance.test.ts @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT // packages/transport-undici/src/undici-transport.conformance.test.ts // Runs the shared TRANSPORT-N suite (@dexpace/transport-conformance) against undiciTransport(). +import {createProxyOptions} from '@dexpace/core'; import {runTransportConformanceSuite} from '@dexpace/transport-conformance'; import {undiciTransport} from './undici-transport.js'; @@ -9,4 +10,17 @@ runTransportConformanceSuite('undiciTransport', () => undiciTransport(), { supportsProxy: true, // TRANSPORT-11's own note: an undici-class transport forwards `Connection` rather than dropping it. dropsConnectionHeader: false, + // TRANSPORT-30: core resolves `ALL_PROXY=socks5://host:1080` to this type (CFG-22), and undici's + // `ProxyAgent` is an HTTP CONNECT tunnel that cannot carry it. + unsupportedProxy: { + type: 'socks5', + build: () => + undiciTransport({ + proxy: createProxyOptions({ + type: 'socks5', + host: '127.0.0.1', + port: 1080, + }), + }), + }, }); diff --git a/packages/transport-undici/src/undici-transport.test.ts b/packages/transport-undici/src/undici-transport.test.ts index 862a718..e43390c 100644 --- a/packages/transport-undici/src/undici-transport.test.ts +++ b/packages/transport-undici/src/undici-transport.test.ts @@ -11,7 +11,7 @@ // TRANSPORT-19 (a header-mapping throw leaves no started body producer stranded), SEAM-30 (so no // producer rejection reaches Node's default unhandledRejection policy) import {createRequire} from 'node:module'; -import {mkdtemp, rm, writeFile} from 'node:fs/promises'; +import {mkdtemp, readFile, rm, writeFile} from 'node:fs/promises'; import {createServer, type Server} from 'node:http'; import {tmpdir} from 'node:os'; import {join} from 'node:path'; @@ -348,25 +348,41 @@ describe('undiciTransport disposal (TRANSPORT-15/16)', () => { }); }); +/** + * A `Dispatcher` that records every `request()` it is handed and answers 200 with an empty body, so + * a row can assert what reached undici's argument validation rather than what reached the wire. + * Returns the recording array; the caller builds the transport around it. + */ +function recordingDispatcher(sink: Dispatcher.RequestOptions[]): Dispatcher { + return { + request: (options: Dispatcher.RequestOptions) => { + sink.push(options); + return Promise.resolve({ + statusCode: 200, + headers: {}, + body: { + destroy: () => undefined, + [Symbol.asyncIterator]: () => ({ + next: () => Promise.resolve({done: true, value: undefined}), + }), + }, + } as unknown as Dispatcher.ResponseData); + }, + close: () => Promise.resolve(), + } as unknown as Dispatcher; +} + +/** The flat `[name, value, ...]` array one recorded dispatch carried. */ +function dispatchedHeaders( + sent: Dispatcher.RequestOptions | undefined, +): string[] { + return (sent?.headers ?? []) as string[]; +} + describe('undiciTransport dispatch', () => { test('TRANSPORT-2/11: redirects are pinned off and Connection is forwarded, not dropped', async () => { const dispatched: Dispatcher.RequestOptions[] = []; - const recorder = { - request: (options: Dispatcher.RequestOptions) => { - dispatched.push(options); - return Promise.resolve({ - statusCode: 200, - headers: {}, - body: { - destroy: () => undefined, - [Symbol.asyncIterator]: () => ({ - next: () => Promise.resolve({done: true, value: undefined}), - }), - }, - } as unknown as Dispatcher.ResponseData); - }, - close: () => Promise.resolve(), - } as unknown as Dispatcher; + const recorder = recordingDispatcher(dispatched); const transport = undiciTransport({dispatcher: recorder}); const request = Request.newBuilder() @@ -383,79 +399,174 @@ describe('undiciTransport dispatch', () => { const sent = dispatched[0]; expect(sent?.maxRedirections).toBe(0); expect(sent?.path).toBe('/anything?q=1'); - const headers = sent?.headers as string[]; + const headers = dispatchedHeaders(sent); expect(headers).toContain('Connection'); expect(headers).not.toContain('Content-Length'); }); }); -describe('undiciTransport body and adaptation paths', () => { - test('TRANSPORT-28: a file body dispatches exactly its declared byte range', async () => { - const dir = await mkdtemp(join(tmpdir(), 'undici-file-body-')); +describe('undiciTransport outbound header degradation (TRANSPORT-11/12)', () => { + test('every header undici refuses is dropped before dispatch, and logged by name', async () => { + const dispatched: Dispatcher.RequestOptions[] = []; + const transport = undiciTransport({ + dispatcher: recordingDispatcher(dispatched), + headerDropLogging: 'all', + }); + const capture = captureDroppedHeaders(); try { - const path = join(dir, 'payload.bin'); - await writeFile(path, 'ABCDEFGH'); - // The structural recognition contract, built by hand: this package must narrow on - // `kind === 'file'` alone, never on an instanceof against @dexpace/body-file, which it - // deliberately does not depend on. - const descriptor: FileBodyDescriptor = { - kind: 'file', - mediaType: 'application/octet-stream', - contentLength: 4, - replayable: true, - path, - start: 2, - count: 4, - writeTo: () => - Promise.reject(new Error('the transport must not call writeTo here')), - }; + const request = Request.newBuilder() + .url(`${origin}/anything`) + .headers( + Headers.newBuilder() + .set('Expect', '100-continue') + .set('Keep-Alive', 'timeout=5') + .set('Upgrade', 'websocket') + .set('X Custom', 'model-valid, non-token') + .set('X-Pass-Through', 'survives') + .build(), + ) + .build(); + await (await transport.send(request)).close(); + await transport.close(); + const headers = dispatchedHeaders(dispatched[0]); + // Asserted against the argument array, not the wire: undici validates in `new Request(...)`, + // so a name that reaches this array is a name that would have thrown (audit #67 / #81). + expect(headers).toEqual(['X-Pass-Through', 'survives']); + for (const name of ['expect', 'keep-alive', 'upgrade', 'x custom']) { + expect(capture.dropped).toContain(name); + } + } finally { + capture.restore(); + } + }); + + test('a Connection value undici cannot carry is dropped, close and keep-alive are not', async () => { + const carried: string[] = []; + const dropped: string[] = []; + for (const value of ['close', 'Keep-Alive', 'upgrade']) { + const dispatched: Dispatcher.RequestOptions[] = []; + const transport = undiciTransport({ + dispatcher: recordingDispatcher(dispatched), + headerDropLogging: 'all', + }); + const capture = captureDroppedHeaders(); + try { + const request = Request.newBuilder() + .url(`${origin}/anything`) + .headers(Headers.newBuilder().set('Connection', value).build()) + .build(); + await (await transport.send(request)).close(); + await transport.close(); + if (dispatchedHeaders(dispatched[0]).includes('Connection')) { + carried.push(value); + } + // `createDropLogger` lower-cases the name it logs, so the field is `connection`. + if (capture.dropped.includes('connection')) dropped.push(value); + } finally { + capture.restore(); + } + } + // `Keep-Alive` mixed-cased on purpose: undici lower-cases the value before comparing + // (`lib/core/request.js:401`), so this transport must too or it would drop a header undici + // would have carried. + expect(carried).toEqual(['close', 'Keep-Alive']); + expect(dropped).toEqual(['upgrade']); + }); +}); + +/** + * A `kind: 'file'` descriptor over a real path whose `writeTo` streams its own declared range and + * counts its calls -- the shape `@dexpace/body-file`'s `fileBody()` produces. Built by hand because + * TRANSPORT-28's recognition contract is structural: this package narrows on `kind` alone and must + * never `instanceof` against a package it does not depend on. + */ +function fileDescriptor( + path: string, + range: {start: number; count: number}, + writes: {count: number}, +): FileBodyDescriptor { + return { + kind: 'file', + mediaType: 'application/octet-stream', + contentLength: range.count, + replayable: true, + path, + start: range.start, + count: range.count, + async writeTo(sink: WritableStream): Promise { + writes.count += 1; + const writer = sink.getWriter(); + if (range.count === 0) { + writer.releaseLock(); + return; + } + const bytes = (await readFile(path)).subarray( + range.start, + range.start + range.count, + ); + await writer.write(new Uint8Array(bytes)); + writer.releaseLock(); + }, + }; +} + +/** Runs `body` against a fresh temporary file holding `ABCDEFGH`, and removes the directory after. */ +async function withPayloadFile( + run: (path: string) => Promise, +): Promise { + const dir = await mkdtemp(join(tmpdir(), 'undici-file-body-')); + try { + const path = join(dir, 'payload.bin'); + await writeFile(path, 'ABCDEFGH'); + return await run(path); + } finally { + await rm(dir, {recursive: true, force: true}); + } +} + +describe('undiciTransport body and adaptation paths', () => { + test('TRANSPORT-28/BODY-13: a file body dispatches its declared range THROUGH its own writeTo', async () => { + await withPayloadFile(async path => { + const writes = {count: 0}; const transport = undiciTransport(); const request = Request.newBuilder() .method('POST') .url(`${origin}/upload`) - .body(descriptor) + .body(fileDescriptor(path, {start: 2, count: 4}, writes)) .build(); received.length = 0; await (await transport.send(request)).close(); await transport.close(); expect(received[0]).toBe('CDEF'); - } finally { - await rm(dir, {recursive: true, force: true}); - } + // The `writes` count is the whole point of the row. Until audit #67 / #81 this transport + // handed `createReadStream(path, {start, end})` to undici and left `writeTo` uncalled, so the + // same four bytes reached the wire with BODY-13's `transferred === count` invariant -- + // the only thing that can see a file truncated after its length was captured -- never run. + expect(writes.count).toBe(1); + }); }); test('a zero-count file body dispatches as an empty body, not a stream error', async () => { - const dir = await mkdtemp(join(tmpdir(), 'undici-empty-file-body-')); - try { - const path = join(dir, 'payload.bin'); - await writeFile(path, 'ABCDEFGH'); - // createReadStream throws ERR_OUT_OF_RANGE the moment `end` falls below `start`, which is what - // `start + count - 1` computes for count 0 -- the empty range needs its own branch. - const descriptor: FileBodyDescriptor = { - kind: 'file', - mediaType: 'application/octet-stream', - contentLength: 0, - replayable: true, - path, - start: 4, - count: 0, - writeTo: () => Promise.resolve(), - }; + await withPayloadFile(async path => { + // `isMaterializable` admits `contentLength === 0`, and `materializeBody` returns an empty + // buffer without opening anything, so the empty range needs no branch of its own. It did when + // the file body went to `createReadStream`, which throws ERR_OUT_OF_RANGE the moment `end` + // (start + count - 1) falls below `start`. + const writes = {count: 0}; const transport = undiciTransport(); received.length = 0; const response = await transport.send( Request.newBuilder() .method('POST') .url(`${origin}/upload`) - .body(descriptor) + .body(fileDescriptor(path, {start: 4, count: 0}, writes)) .build(), ); await response.close(); await transport.close(); expect(received[0]).toBe(''); - } finally { - await rm(dir, {recursive: true, force: true}); - } + expect(writes.count).toBe(1); + }); }); }); @@ -576,6 +687,35 @@ describe('undiciTransport failure classification (TRANSPORT-20)', () => { }); describe('undiciTransport proxy dispatch (TRANSPORT-30)', () => { + test('TRANSPORT-30: a SOCKS proxy is refused at the factory, before any Agent is built', () => { + // Both SOCKS values `ProxyType` admits, because core resolves both from the environment + // (`socks4`/`socks4a` and `socks:`/`socks5`/`socks5h`, `config/proxy.ts:372-380`) and undici's + // `ProxyAgent` carries neither -- it is an HTTP CONNECT tunnel and reads its `uri` as a URL. + const {agents, restore} = captureOwnedAgents(); + try { + for (const type of ['socks4', 'socks5'] as const) { + let thrown: unknown; + try { + undiciTransport({ + proxy: createProxyOptions({type, host: '127.0.0.1', port: 1080}), + }); + } catch (error) { + thrown = error; + } + expect(thrown).toBeInstanceOf(TypeError); + // TRANSPORT-30's "discoverable": the refusal names the type, and it is not the raw + // `InvalidArgumentError: Invalid URL protocol: socks5:` undici used to let escape. + expect((thrown as Error).message).toContain(type); + expect(thrown).not.toBeInstanceOf(IoError); + } + // The check runs before `new undici.Agent(...)`, so a refused construction leaks nothing -- + // there is no transport for the caller to have called `close()` on. + expect(agents.length).toBe(0); + } finally { + restore(); + } + }); + test('a per-request Proxy-Authorization is dropped when a proxy is configured', async () => { // ProxyAgent.dispatch throws InvalidArgumentError on ANY per-request Proxy-Authorization -- a // deliberate undici security fix -- so forwarding one would turn every proxied send into a hard diff --git a/packages/transport-undici/src/undici-transport.ts b/packages/transport-undici/src/undici-transport.ts index 0853169..46ec7bc 100644 --- a/packages/transport-undici/src/undici-transport.ts +++ b/packages/transport-undici/src/undici-transport.ts @@ -1,6 +1,5 @@ // SPDX-License-Identifier: MIT // packages/transport-undici/src/undici-transport.ts -import {createReadStream} from 'node:fs'; import {createRequire} from 'node:module'; import {Readable} from 'node:stream'; import type {ReadableStream as NodeReadableStream} from 'node:stream/web'; @@ -13,8 +12,8 @@ import { Status, TransportFailureError, type Body, - type FileBodyDescriptor, type ProxyOptions, + type ProxyType, type Request, type RequestOptions, type Transport, @@ -52,13 +51,26 @@ const require = createRequire(import.meta.url); const undici = require('undici/index.js') as typeof import('undici'); /** - * TRANSPORT-11's outbound drop set for this transport. `connection` is deliberately absent — §17's - * own note is that an undici-class transport forwards it rather than dropping it. + * TRANSPORT-11's outbound drop set for this transport: the three framing headers undici computes + * itself, plus the three it refuses outright. + * + * `expect`, `keep-alive` and `upgrade` are undici's unconditional rejections + * (`lib/core/request.js:398,409` in 6.28.0 — `InvalidArgumentError` for the first two, + * `NotSupportedError` for `expect`). Until 2026-09-05 they were absent, so a caller who set any of + * them got a failed send out of `undiciTransport().send()` where the fetch twin dispatched + * (audit #67 / #81). TRANSPORT-12 says the header is what gives way, not the request. + * + * `connection` is deliberately absent — §17's own note is that an undici-class transport forwards + * it rather than dropping it. undici carries only `close` and `keep-alive`, so a third value is + * dropped per-request by {@link isUnsendableHeader} rather than by name here. */ const UNDICI_FORBIDDEN_HEADERS: readonly string[] = [ 'content-length', 'host', 'transfer-encoding', + 'expect', + 'keep-alive', + 'upgrade', ]; /** @@ -87,7 +99,13 @@ export interface UndiciTransportOptions { * (SEAM-14); supplying it together with `proxy` is a construction-time error. */ readonly dispatcher?: Dispatcher; - /** Proxy configuration; the transport constructs and owns the resulting `ProxyAgent`. */ + /** + * Proxy configuration; the transport constructs and owns the resulting `ProxyAgent`. + * + * `type` must be `http`. undici's `ProxyAgent` is an HTTP `CONNECT` tunnel, so the `socks4` and + * `socks5` values `ProxyType` also admits — which core resolves from `ALL_PROXY` — are refused at + * construction rather than at the first send (TRANSPORT-30). + */ readonly proxy?: ProxyOptions; /** How dropped header names are logged (TRANSPORT-13); defaults to `'first-per-name'`. */ readonly headerDropLogging?: HeaderDropLogging; @@ -107,6 +125,38 @@ interface DispatcherSet { readonly owned: readonly Dispatcher[]; } +/** + * The one `ProxyOptions.type` this transport can build a dispatcher for. + * + * undici's `ProxyAgent` is an HTTP `CONNECT` tunnel and takes its `uri` as a URL, so a `socks5://` + * or `socks4://` one throws `InvalidArgumentError('Invalid URL protocol: …')` out of its + * constructor. Core resolves `ALL_PROXY=socks5://host:1080` to `type: 'socks5'` quite legitimately + * (`config/proxy.ts:372-380`, CFG-22), so the configuration can express a proxy neither shipped + * transport can honour, and until 2026-09-05 the way you found out was an undici error escaping a + * public factory untyped and undocumented (audit #67 / #81, `docs/deviations.md`). + */ +const SUPPORTED_PROXY_TYPE: ProxyType = 'http'; + +/** + * TRANSPORT-30's "make the limitation discoverable rather than silently misbehaving", applied at + * the earliest point that can: construction, before any dispatcher is allocated. + * + * A `TypeError`, matching {@link selectDispatchers}' other construction-time refusal and + * deliberately outside the `IoError` tree — `retry/classify.ts` is an allow-list, so a + * misconfiguration that no retry can fix is non-retryable for free (RETRY-2). + * + * @param proxy - the configured proxy. + * @throws `TypeError` when `proxy.type` is anything but `http`. + */ +function requireSupportedProxyType(proxy: ProxyOptions): void { + if (proxy.type === SUPPORTED_PROXY_TYPE) return; + throw new TypeError( + `unsupported proxy type \`${proxy.type}\`: undici's ProxyAgent is an HTTP CONNECT tunnel and ` + + 'cannot carry a SOCKS proxy, and @dexpace/transport-fetch has no proxy support at all. ' + + 'Configure an http proxy, or route SOCKS outside this SDK.', + ); +} + /** * The proxy URI plus its Basic credential, kept apart. `formatProxyOptions` is deliberately *not* * used here: it masks credentials as `***:***` for logging, and feeding that to `ProxyAgent` would @@ -137,6 +187,9 @@ function selectDispatchers(options: UndiciTransportOptions): DispatcherSet { const byo = options.dispatcher; return {proxied: byo, direct: byo, owned: []}; } + // Before anything is allocated: a refusal after `new undici.Agent(...)` would leak the direct + // agent on the way out, with no transport for the caller to close it through. + if (options.proxy !== undefined) requireSupportedProxyType(options.proxy); // Agent, not Pool: a Pool is bound to one origin at construction, but a general-purpose Transport // must reach whatever origin each Request names. const direct = new undici.Agent(options.agentOptions); @@ -146,7 +199,52 @@ function selectDispatchers(options: UndiciTransportOptions): DispatcherSet { return {proxied, direct, owned: [proxied, direct]}; } -/** undici's flat `[name, value, name, value, ...]` form -- the only shape that keeps a repeated name repeated (HTTP-14). */ +/** + * RFC 9110 `token`, which is what undici's `isValidHTTPToken` enforces on every header name + * (`lib/core/util.js:547-587`): VCHAR minus the delimiters `"(),/:;<=>?@[\]{}`. Anchored and + * one-or-more, so the empty name undici also rejects fails here too. + * + * `@dexpace/core` is deliberately laxer: `hasForbiddenNameByte` admits every printable ASCII byte + * (`http/ascii-validation.ts:29-33`), so `X Custom` is a model-valid name that no native client on + * this platform will carry. + */ +const RFC9110_TOKEN = /^[!#$%&'*+\-.^_`|~0-9A-Za-z]+$/u; + +/** The only two `Connection` values undici carries; anything else is `InvalidArgumentError`. */ +const FORWARDABLE_CONNECTION_VALUES: ReadonlySet = new Set([ + 'close', + 'keep-alive', +]); + +/** + * Whether undici's own request validation would reject this header, which for TRANSPORT-12 means it + * is dropped rather than allowed to fail the send. + * + * Only the *name* grammar and the `connection` value are checked. undici's value grammar + * (`headerCharRegex`, `lib/core/util.js:598`) admits HTAB, 0x20-0x7E **and** obs-text 0x80-0xFF, + * which is strictly wider than the outbound value rule `mapOutboundHeaders` has already applied + * (HTTP-18), so no value that reaches here can fail there. + * + * @param name - the header name, as the caller spelled it. + * @param value - the header value. + * @returns `true` when the header must be dropped instead of dispatched. + */ +function isUnsendableHeader(name: string, value: string): boolean { + if (!RFC9110_TOKEN.test(name)) return true; + return ( + name.toLowerCase() === 'connection' && + !FORWARDABLE_CONNECTION_VALUES.has(value.toLowerCase()) + ); +} + +/** + * undici's flat `[name, value, name, value, ...]` form -- the only shape that keeps a repeated name + * repeated (HTTP-14) -- with every header undici would refuse degraded to a logged drop. + * + * The per-header guard is the same degrade `@dexpace/transport-fetch` gets for free from wrapping + * `Headers.append` in a `try`/`catch` (`fetch-transport.ts:159-166`): undici validates at dispatch + * rather than at construction, so this transport has to ask the question itself (TRANSPORT-12). + */ function toUndiciHeaders( request: Request, forbidden: readonly string[], @@ -155,8 +253,14 @@ function toUndiciHeaders( const {sent, dropped} = mapOutboundHeaders(request.headers, forbidden, { bodyDerivedMediaType: request.body?.mediaType, }); - logDrops(dropped); - return [...sent.entries()].flat(); + const degraded = [...dropped]; + const flat: string[] = []; + for (const [name, value] of sent.entries()) { + if (isUnsendableHeader(name, value)) degraded.push(name); + else flat.push(name, value); + } + logDrops(degraded); + return flat; } /** @@ -245,33 +349,29 @@ interface PreparedBody { } /** - * TRANSPORT-28's recognition contract, in one named place: a plain string-literal check, never a - * cross-package `instanceof` against `@dexpace/body-file` (which this package does not depend on). - * `Body.kind` is a union on one interface rather than a discriminated union of interfaces, so the - * narrowing has to be spelled out as a predicate. + * Prepares one request body for dispatch: buffer it if it is small and replayable, stream it + * otherwise. Exactly the two decisions the fetch twin makes, in the same order, and deliberately so + * — there is **no** `kind === 'file'` branch here. + * + * There was one until 2026-09-05. It handed `createReadStream(path, {start, end})` to undici, which + * is a genuinely shorter path to the wire — one fewer userspace copy — and it bypassed the + * descriptor's own `writeTo`, so `@dexpace/body-file`'s `transferred === count` invariant never ran + * (BODY-13). `content-length` is dropped outbound, so undici framed the body chunked and the wire + * could not detect the short write either: a file truncated between `stat` and `send` POSTed its + * remaining bytes and resolved 200, where the fetch transport raised `TransportFailureError` + * (audit #67 / #81). A file body is now written through `writeTo` like any other, which is what + * makes BODY-13 hold on both transports and what the shared conformance row asserts. + * + * TRANSPORT-28's SHOULD is not thereby abandoned so much as re-described: no user-space path in + * either client reaches `sendfile(2)`, which `docs/deviations.md` item 13 has recorded since Phase + * 8a, and the clause's MUST — a file body is replayable, and exactly its declared byte range reaches + * the wire — is honoured by the descriptor itself, on both transports, by the same code. + * + * The zero-count case needs no branch either: `isMaterializable` admits `contentLength === 0`, and + * `materializeBody` returns an empty buffer without ever opening a read stream. */ -function isFileBody(body: Body): body is FileBodyDescriptor { - return body.kind === 'file'; -} - async function prepareBody(body: Body | undefined): Promise { if (body === undefined) return {init: null, pump: undefined}; - if (isFileBody(body)) { - // An empty range is not a degenerate read stream: `createReadStream` throws ERR_OUT_OF_RANGE the - // moment `end` (start + count - 1) falls below `start`, so a zero-count file body has to become - // an explicit empty body rather than a stream nobody can open. - if (body.count === 0) return {init: new Uint8Array(0), pump: undefined}; - // TRANSPORT-28: dispatch straight off the file, honoring start/count, rather than routing the - // bytes through a userspace TransformStream first. The closest available approximation of the - // reference's zero-copy path -- see the Deviation Ledger for why a literal one does not exist. - return { - init: createReadStream(body.path, { - start: body.start, - end: body.start + body.count - 1, - }), - pump: undefined, - }; - } if (isMaterializable(body, MAX_MATERIALIZED_BODY_BYTES)) { try { return {init: await materializeBody(body), pump: undefined}; @@ -591,7 +691,9 @@ if (typeof Symbol.asyncDispose === 'symbol') { * * @param options - optional transport settings. * @returns a transport ready to send; release it with `close()`. - * @throws `TypeError` when both `dispatcher` and `proxy` are supplied. + * @throws `TypeError` when both `dispatcher` and `proxy` are supplied, or when `proxy.type` is + * anything but `http` — undici's `ProxyAgent` cannot carry a SOCKS proxy, and neither can + * `@dexpace/transport-fetch`, which has no proxy option at all. * * @public */ diff --git a/tests/node-conformance/transport.test.mjs b/tests/node-conformance/transport.test.mjs index c6e16f6..b481f13 100644 --- a/tests/node-conformance/transport.test.mjs +++ b/tests/node-conformance/transport.test.mjs @@ -12,13 +12,16 @@ // // Exercises: TRANSPORT-1 (redirects not followed), TRANSPORT-4/20 (timeout and no-response classification), // TRANSPORT-17 (a single-use body written once, its bytes on the wire), TRANSPORT-24 (vendor status codes), -// TRANSPORT-28/BODY-11 (a real fileBody() over the wire, whole and ranged), +// TRANSPORT-11/12 (a header the native layer refuses is dropped, not a failed send -- Node's undici-backed +// `fetch` rejects three names Bun's forwards), +// TRANSPORT-28/BODY-11 (a real fileBody() over the wire, whole and ranged), BODY-13 (a truncate-after-stat +// short write fails the send on the streamed path, which only this runtime can assert), // TRANSPORT-25 (the response body is a lazily-read stream and close releases it), TRANSPORT-29/SEAM-12 // (concurrent sends), SEAM-16 (an abort after delivery must not close the delivered body). import assert from 'node:assert/strict'; import {createServer} from 'node:http'; import {after, before, describe, it} from 'node:test'; -import {mkdtemp, rm, writeFile} from 'node:fs/promises'; +import {mkdtemp, rm, truncate, writeFile} from 'node:fs/promises'; import {tmpdir} from 'node:os'; import {join} from 'node:path'; import {createHash} from 'node:crypto'; @@ -140,6 +143,42 @@ describe('the transport adapters on the Node runtime', () => { } }); + it('drops the headers the native layer refuses rather than failing the send (TRANSPORT-11/12)', async () => { + // The one case in this file where Bun and Node disagree about the *outcome*, not merely the + // implementation. `Expect`, `Keep-Alive` and `Upgrade` are undici's three unconditional + // rejections, and Node's global `fetch` is undici-backed, so on this runtime an undropped + // one rejects the send: `TypeError: fetch failed` through transport-fetch, which can only + // be classified as the RETRYABLE TransportFailureError, and a terminal TypeError through + // transport-undici. Bun's `fetch` instead forwards `Expect`/`Keep-Alive` to the wire and + // hangs on `Upgrade`, so the Bun conformance rows for these names prove a weaker claim than + // this one does (audit #67 / #81, measured 2026-09-05). + const transport = makeTransport(); + try { + const response = await transport.send( + Request.newBuilder() + .url(`${origin}/echo`) + .headers( + Headers.newBuilder() + .set('Expect', '100-continue') + .set('Keep-Alive', 'timeout=5') + .set('Upgrade', 'websocket') + .set('X Custom', 'model-valid, non-token') + .set('X-Pass-Through', 'survives') + .build(), + ) + .build(), + RequestOptions.newBuilder().timeoutMs(2_000).build(), + ); + const echoed = JSON.parse(await response.text()); + for (const name of ['expect', 'keep-alive', 'upgrade', 'x custom']) { + assert.equal(echoed.headers[name], undefined, name); + } + assert.equal(echoed.headers['x-pass-through'], 'survives'); + } finally { + await transport.close(); + } + }); + it('surfaces a vendor status with a readable body (TRANSPORT-24)', async () => { const transport = makeTransport(); try { @@ -345,6 +384,62 @@ describe('the transport adapters on the Node runtime', () => { await transport.close(); } }); + + // BODY-13's short-write clause on the STREAMED request-body path, which is this tree's to + // hold for two independent reasons. The shared conformance row + // (`run-suite.ts`'s "a file body truncated after its length was captured") drives the + // buffered path only: above the adapters' 1,000,000-byte materialize bound the producer + // failure aborts a `TransformStream` mid-pull, and Bun 1.3.14's `Readable.fromWeb` leaks + // that abort reason as unhandled rejections, which `bun:test` scores against an unrelated + // row. Node's bridge does not. And this is the only layer where a real `fileBody()` — whose + // `transferred === count` invariant is the thing under test — meets a real transport. + // + // Until audit #67 / #81 the undici transport handed `createReadStream(path, …)` to undici + // and never called `writeTo` at all, so this case reported 200 with ten bytes on the wire + // while transport-fetch raised. `content-length` is dropped outbound, so the framing cannot + // catch it either. + it('fails a send whose file was truncated after its length was captured (BODY-13)', async () => { + const declared = 1_100_000; + const shortDir = await mkdtemp( + join(tmpdir(), 'dexpace-filebody-short-'), + ); + const shortPath = join(shortDir, 'payload.bin'); + const transport = makeTransport(); + try { + await writeFile(shortPath, fixtureBytes(declared)); + const body = fileBody(shortPath); + assert.equal(body.contentLength, declared); + await truncate(shortPath, 10); + await assert.rejects( + transport.send( + Request.newBuilder() + .method('POST') + .url(`${origin}/echo`) + .body(body) + .build(), + ), + error => { + assert.equal(error.name, 'TransportFailureError'); + // BODY-13 names transferred-of-total. The streamed path rethrows the producer's own + // message; the buffered one carries it as a cause, so both are searched. + const chain = []; + for (let at = error; at instanceof Error; at = at.cause) { + chain.push(at.message); + } + assert.ok( + chain.some(message => + message.includes(`transferred 10 of ${declared} bytes`), + ), + `no transferred-of-total in ${JSON.stringify(chain)}`, + ); + return true; + }, + ); + } finally { + await transport.close(); + await rm(shortDir, {recursive: true, force: true}); + } + }); }); }); }