diff --git a/docs/deviations.md b/docs/deviations.md index 6bf260b..86426bb 100644 --- a/docs/deviations.md +++ b/docs/deviations.md @@ -482,6 +482,8 @@ frozen tree and is amended only deliberately, by hand. When §10 is next amended | **`XCUT-16`'s replay guard is keyed on whether the hop was guarded, not on whether the replacement looks credentialed.** `XCUT-16` and `AUTH-28` say the guard applies "on any path where a credential will be attached", and carve out "a deliberately credential-free re-issue MAY proceed over any scheme". Deciding which of the two a challenge replacement is cannot be done by reading header names: the step's own `ApiKeyCredentialConfig.headerName` stamps whatever header the caller names, and a `challengeHook` may invent a carrier this step has never been told about. The port therefore reads "a credential will be attached" as a property of the HOP — if the outbound pass ran the HTTPS guard, so does the replay, whatever URL and headers the hook chose. **Strictly wider than the requirement's letter**, and knowingly so: it refuses a downgraded replacement that carries no credential at all, on a hop that is credentialed. The carve-out is preserved where it is observable — a `NO_AUTH` hop is never guarded outbound, and its replay is guarded only when the replacement carries `Authorization` or `Proxy-Authorization`, which is the previous rule kept as a second arm. *Rejected:* deriving the credential-carrying header names from configuration, which misses the hook-invented carrier and is the shape that let the reported leak through | audit #67 / #71 | 2026-09-04 | `docs/product-spec/19-cross-cutting-invariants-and-policies.md:44` is the requirement and its carve-out. `packages/core/src/auth/auth-step.ts:389` sets `OutboundPlan.guarded`; `:564-575` is `guardReplayScheme` and its two arms. Pinned by "a replacement carrying a NON-standard credential header over plaintext is refused" and "a header-free replacement over plaintext is refused too" in `packages/core/src/auth/auth-step.test.ts`, and by the "XCUT-16: a guarded hop stays guarded across a challenge replay" block in `tests/conformance/xcut/security-by-default.conformance.test.ts` | not yet in §10 | | **`ASYNC-21`'s "MUST NOT close the caller-owned source on any termination" is not honoured: the RxJS SSE adapter takes ownership and closes.** `sseEvents$` and `typedSse$` pass `() => stream.close()` as `fromAsyncIterable`'s `release`, and RxJS runs a subscriber's finalizer on *every* termination — unsubscription, end-of-source and a source error alike, which is the complete list the clause names. **Kept, deliberately, on two grounds.** (1) **The clause has no subject on this platform.** It presumes a source whose iterator return leaves the source open; this port's `SseStream` is deliberately not that one. `#iterate`'s `finally` calls `#releaseQuietly()`, so the resource is released whenever the runtime drives `return()` — which `fromAsyncIterable` must do exactly once (`ASYNC-6`), and which a plain `for await` with `break` does too. Removing the callback would change which channel reports a release failure and when the release runs, not whether the caller-owned source ends up closed. (2) **The ordering is load-bearing.** The release runs *ahead of* `iterator.return()` because an async generator's `return()` queues behind a suspended `next()`, and an SSE stream idling between events is parked in exactly that pull — so without the callback an `unsubscribe()` stays pending until the server next sends a byte, holding the socket open indefinitely. Measured: deleting the two `release` arguments turns four cases red — the two suspended-pull ones, as "the teardown did not settle within 500ms", and the two pre-existing idle-unsubscribe assertions — while every exactly-once release count stays green, which is the shape of the claim. Pagination attaches no release for the complementary reason: a `Paginator`'s pulls are bounded HTTP exchanges, never a wait on a server that may never answer. *Rejected:* dropping the callback to match the letter (reintroduces the hang for no change in what closes). *Rejected:* a caller-facing `{ownership}` option (two behaviours to document for a case with one correct answer). The public TSDoc and `packages/rx/README.md` now state the transfer outright — subscribing hands the stream over, do not close it yourself and do not iterate it afterwards — rather than leaving the `ASYNC-21` citation on the doc comment's first line to read as satisfied | audit #67 / #75 | 2026-09-05 | `packages/rx/src/sse.ts:46` and `:73-75` are the two `release` arguments; `packages/rx/src/from-async-iterable.ts:103-108` is the teardown that runs one on every termination; `docs/product-spec/18-asynchronous-runtime-adapter-contract.md:42` is the requirement. Ground 1: `packages/core/src/sse/stream.ts:136-139` (`#iterate`'s `finally` → `#releaseQuietly()`) with `:117-121` (`close()` memoized, `SSE-28`). Ground 2: `packages/rx/src/from-async-iterable.ts:44-48` states the ordering and why. Pinned by the two `resource ownership` blocks in `packages/rx/src/sse.test.ts`, which count the release the OWNED resource sees rather than `SseStream.close()` calls — the facade memoizes, so a facade-level count reads "once" however many paths call it — and by "SSE ownership transfer releases once on Node" in `tests/node-conformance/rx-bridge.test.mjs`. Phase 8b marked `ASYNC-21` ✅ with this clause dropped from its gist (`docs/work/mvp/phase8/phase8b/2026-07-28-phase8b-async-runtime-checklist.md:67`); that is a dated record and is left as written. The other half is `SSE-41`'s own "documented source ownership" clause, which the same checklist marked ✅ (`:74`) on the strength of documentation that named unsubscription only — completed by the TSDoc and README rewrite this row accompanies | not yet in §10 | | **`AUTH-22`'s "emit cnonce/nc/qop only when qop is negotiated" is not applied to `cnonce` for a `-sess` algorithm.** A `-sess` HA1 is `H(H(user:realm:pass):nonce:cnonce)` (RFC 7616 §3.4.2), so the client nonce is an *input to the hash* for `MD5-sess` and `SHA-256-sess` whatever `qop` the challenge offered. The port implemented AUTH-22 to the letter: it drew a fresh cnonce, folded it into HA1, and then omitted it from the header whenever `qop` was absent — a response no server can verify, because it has no way to reconstruct HA1. AUTH-30 bounds the re-challenge replay to one 401, so every such exchange simply failed. **`cnonce` is now emitted for any `-sess` algorithm; `nc` and `qop` stay conditional exactly as AUTH-22 says**, because RFC 2069's response input is `H(HA1:nonce:HA2)` and carries no nonce count, so emitting one would advertise a count the response was not computed over. RFC 7616 §3.4 states the wider rule outright — "cnonce: This parameter MUST be used by all implementations". AUTH-22's clause is RFC 2617's RFC 2069-compatibility form, written before `-sess` existed, and the requirement's own AUTH-15 mandates both `-sess` algorithms, so the two sentences cannot both be followed. *Rejected:* declining a `-sess`-without-`qop` challenge instead, which turns every such server into a guaranteed 401 for no security gain, when the value the server needs has already been computed | audit #67 / #74 | 2026-09-05 | `packages/core/src/auth/digest.ts:345-350` (the `-sess` HA1 that consumes the cnonce) against `:405-408` (`buildHeaderValue`, where the `else if` now emits it); `docs/product-spec/11-authentication.md:18` and `docs/product-spec/appendix-c-consolidated-normative-requirement-index.md:357` are AUTH-22's wording. Pinned by the `digestHandler -sess without qop (AUTH-17/AUTH-22)` block in `packages/core/src/auth/digest.test.ts` — one row asserting the header carries `cnonce` and neither `nc` nor `qop`, one recomputing the response from the header's OWN cnonce so a value drawn twice would fail — and by the `MD5-sess, no qop` vector in the same file | not yet in §10 | +| **`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 | ### Proposed erratum for `PIPE-40` (drafted 2026-09-04, not applied) diff --git a/docs/sdk-documentation/http.md b/docs/sdk-documentation/http.md index b570319..4ef7dc6 100644 --- a/docs/sdk-documentation/http.md +++ b/docs/sdk-documentation/http.md @@ -110,8 +110,9 @@ const options = RequestOptions.newBuilder() `RequestOptions.EMPTY` is the shared no-op instance. A step reads it as `ctx.options`, and a transport receives it as `send()`'s second argument. Both range checks are the **full** range, not only the lower bound: `maxRetries` rejects anything that is not a non-negative integer, and -`timeoutMs` rejects zero, negatives, `Infinity` and `NaN` alike (`HTTP-35`). A fractional -`timeoutMs` is accepted — a timeout is a duration, not a count. +`timeoutMs` rejects zero, negatives, `Infinity`, `NaN`, a fractional value and anything above +`2**32 - 1` (`HTTP-35`) — the range is `AbortSignal.timeout()`'s, the only one a transport can +honor. `auth` on the builder is the **per-call** auth tier, the highest-precedence one; see [`auth.md`](./auth.md). diff --git a/packages/core/src/http/builder.ts b/packages/core/src/http/builder.ts index 5411659..c4bc1e4 100644 --- a/packages/core/src/http/builder.ts +++ b/packages/core/src/http/builder.ts @@ -20,6 +20,22 @@ export interface Builder { build(): T; } +/** + * The one frozen empty list every multi-value accessor returns for an absent name. + * + * Shared, not allocated per miss, and frozen for the same reason the present-name lists are: + * HTTP-5's accessors "MUST NOT let a caller mutate the model through the returned value", and the + * TSDoc on `Headers.getAll` and `QueryParams.getAll` promises a frozen list on every path. Both + * returned a fresh `[]` on a miss, which was neither (audit #67 / #76). Sharing one instance is + * safe precisely because it is frozen — there is no state in it to alias, and no caller can add + * any. + * + * Lives here rather than in either model because both need it and the two models deliberately + * import nothing from each other; this module is already the shared construction helper they both + * import from. + */ +export const EMPTY_VALUE_LIST: readonly string[] = Object.freeze([]); + /** * Returns `value` when present, throwing a field-named error when it is `null` or `undefined`. * diff --git a/packages/core/src/http/errors.ts b/packages/core/src/http/errors.ts index 9a3d055..66c0f83 100644 --- a/packages/core/src/http/errors.ts +++ b/packages/core/src/http/errors.ts @@ -127,16 +127,39 @@ export class MediaTypeParseError extends DexpaceError {} export class ProtocolParseError extends DexpaceError {} /** - * Thrown when a request URL is malformed or not absolute; the message carries the offending input - * and the underlying parse failure is chained as `cause` (HTTP-47). + * Thrown when a URL cannot be constructed from what a caller supplied. + * + * Three cases, all of them "this input has no URL form": + * + * - A request URL that is malformed or not absolute (HTTP-47). The message carries the offending + * input and the underlying parse failure is chained as `cause`. + * - A base URL handed to `buildRequest()` that is malformed, not absolute, or carries a fragment + * (SEAM-27). + * - A query-parameter name or value carrying an unpaired surrogate. Such a string has no UTF-8 + * form, so RFC 3986 percent-encoding is undefined for it and `QueryParams.encode()` could only + * fail; `QueryParamsBuilder.add` rejects it at the call that supplied it instead. `cause` is not + * set on this path — nothing was caught, the input was inspected (HTTP-29, audit #67 / #76). + * `QueryParams.parse` does NOT throw it: HTTP-31 makes parsing lenient, so it substitutes U+FFFD. * * @public */ export class UrlConstructionError extends DexpaceError {} /** - * Thrown when a per-call operational override is out of range — a non-null timeout that is zero or - * negative, or a negative max-retries (HTTP-35). + * Thrown when a per-call operational override is out of range (HTTP-35). + * + * The ranges checked are the FULL ranges, not the lower bounds the requirement's own wording names. + * HTTP-35's point is that an out-of-range override is a loud error at the call site that supplied + * it, never a value reinterpreted downstream, and a value that only *some* consumer refuses is the + * same failure moved one seam away: + * + * - `timeoutMs` must be an integer in `1 .. 2**32 - 1` — the range `AbortSignal.timeout()` accepts, + * which is the only one a transport can honour. Zero, negatives, `Infinity`, `NaN`, a fractional + * millisecond and anything above the ceiling are all rejected. Zero is rejected rather than + * reinterpreted: it means "no timeout" in one transport and is an error in another. + * - `maxRetries` must be a non-negative integer. `0` is accepted and means "disable retries for + * this call", distinct from `undefined`; `Infinity` and `NaN` are rejected because they make a + * retry driver's ceiling test permanently false and its loop unbounded. * * @public */ diff --git a/packages/core/src/http/headers.test.ts b/packages/core/src/http/headers.test.ts index 762158a..618e6bb 100644 --- a/packages/core/src/http/headers.test.ts +++ b/packages/core/src/http/headers.test.ts @@ -9,7 +9,9 @@ // XCUT-15's ingested-collection clause (a builder defensively copies what it is handed, so mutating that // collection after build() cannot alter the built model, and a derived builder never aliases its source), // HTTP-17 (outbound name validation + trim), HTTP-18 (outbound value validation), HTTP-19 (inbound leniency), -// HTTP-20 (no value echo, escaped name), HTTP-21 (typed HeaderName interop) +// HTTP-20 (no value echo, escaped name), HTTP-21 (typed HeaderName interop), +// HTTP-5 again (getAll returns a FROZEN list on both the present-name and the absent-name path), +// HTTP-13 once more (Headers.equals asserted directly, not only through Request.equals) import {describe, expect, test} from 'bun:test'; import fc from 'fast-check'; import {Headers, HeaderName} from './headers.js'; @@ -251,3 +253,119 @@ describe('HeaderName.lowerCased (HTTP-21)', () => { expect(name.raw).toBe('Content-Type'); }); }); + +describe('getAll returns a frozen list on every path (HTTP-5)', () => { + const headers = Headers.newBuilder() + .add('X-Tag', 'a') + .add('X-Tag', 'b') + .build(); + + test('the present-name list is frozen', () => { + // Asserted directly for the first time by audit #67 / #76. `build()` freezes each value list, + // and `getAll` returns that same reference rather than a copy, so the freeze is the whole of + // HTTP-5's "cannot mutate the model through the returned value" on this accessor — if the + // freeze were ever dropped, nothing else would notice. + const values = headers.getAll('X-Tag'); + expect(Object.isFrozen(values)).toBe(true); + expect(() => (values as string[]).push('c')).toThrow(TypeError); + expect(headers.getAll('X-Tag')).toEqual(['a', 'b']); + }); + + test('the absent-name list is frozen too, and is the same shared instance', () => { + // It was a fresh `[]` — unfrozen, against the TSDoc's promise of a frozen list, and a fresh + // allocation on every miss. + const first = headers.getAll('nope'); + const second = headers.getAll('also-nope'); + expect(Object.isFrozen(first)).toBe(true); + expect(first).toBe(second); + expect(() => (first as string[]).push('x')).toThrow(TypeError); + }); +}); + +// Reached only through `Request.equals` until audit #67 / #76. Each case below is a way the +// comparison could be wrong that a Request-level test would not isolate. +function buildHeaders(pairs: readonly (readonly [string, string])[]): Headers { + const builder = Headers.newBuilder(); + for (const [name, value] of pairs) builder.add(name, value); + return builder.build(); +} + +describe('Headers.equals directly (HTTP-13): names and casing', () => { + test('is reflexive and true for an identical construction', () => { + const a = buildHeaders([['X-A', '1']]); + expect(a.equals(a)).toBe(true); + expect(a.equals(buildHeaders([['X-A', '1']]))).toBe(true); + }); + + test('name casing does not participate — HTTP-13 folds names', () => { + expect( + buildHeaders([['X-A', '1']]).equals(buildHeaders([['x-a', '1']])), + ).toBe(true); + }); + + test('value casing DOES participate — only names are folded', () => { + expect( + buildHeaders([['X-A', 'v']]).equals(buildHeaders([['X-A', 'V']])), + ).toBe(false); + }); + + test('the order of distinct NAMES does not matter', () => { + const ab = buildHeaders([ + ['X-A', '1'], + ['X-B', '2'], + ]); + const ba = buildHeaders([ + ['X-B', '2'], + ['X-A', '1'], + ]); + expect(ab.equals(ba)).toBe(true); + expect(ba.equals(ab)).toBe(true); + }); +}); + +describe('Headers.equals directly (HTTP-13): values, order and subsets', () => { + test('the order of VALUES under one name does matter (HTTP-14)', () => { + const ab = buildHeaders([ + ['X-T', 'a'], + ['X-T', 'b'], + ]); + const ba = buildHeaders([ + ['X-T', 'b'], + ['X-T', 'a'], + ]); + expect(ab.equals(ba)).toBe(false); + }); + + test('a strict subset is not equal, in either direction', () => { + const one = buildHeaders([['X-A', '1']]); + const two = buildHeaders([ + ['X-A', '1'], + ['X-B', '2'], + ]); + expect(one.equals(two)).toBe(false); + expect(two.equals(one)).toBe(false); + }); + + test('same name count, disjoint names, is not equal', () => { + // The length pre-check passes here, so this is the case that proves the per-name lookup runs. + expect( + buildHeaders([['X-A', '1']]).equals(buildHeaders([['X-B', '1']])), + ).toBe(false); + }); + + test('same names with different value COUNTS is not equal', () => { + const one = buildHeaders([['X-T', 'a']]); + const two = buildHeaders([ + ['X-T', 'a'], + ['X-T', 'a'], + ]); + expect(one.equals(two)).toBe(false); + expect(two.equals(one)).toBe(false); + }); + + test('two empty instances are equal', () => { + expect( + Headers.newBuilder().build().equals(Headers.newBuilder().build()), + ).toBe(true); + }); +}); diff --git a/packages/core/src/http/headers.ts b/packages/core/src/http/headers.ts index 7cf0350..f452d7e 100644 --- a/packages/core/src/http/headers.ts +++ b/packages/core/src/http/headers.ts @@ -1,6 +1,6 @@ // SPDX-License-Identifier: MIT // packages/core/src/http/headers.ts -import type {Builder} from './builder.js'; +import {EMPTY_VALUE_LIST, type Builder} from './builder.js'; import {HeaderValidationError} from './errors.js'; import { hasForbiddenNameByte, @@ -130,10 +130,14 @@ export class Headers { * Returns every value stored under `name`, in insertion order. * * @param name - the header name, as a string or a {@link HeaderName}. - * @returns a read-only, frozen list of values — empty when the name is absent. + * @returns a read-only, frozen list of values — the shared frozen empty list when the name is + * absent. Frozen on both paths, so mutating it cannot reach the model (HTTP-5). */ getAll(name: string | HeaderName): readonly string[] { - return this.#valuesByLowerName.get(toRawName(name).toLowerCase()) ?? []; + return ( + this.#valuesByLowerName.get(toRawName(name).toLowerCase()) ?? + EMPTY_VALUE_LIST + ); } /** diff --git a/packages/core/src/http/query-params.test.ts b/packages/core/src/http/query-params.test.ts index 2865794..fb51ebf 100644 --- a/packages/core/src/http/query-params.test.ts +++ b/packages/core/src/http/query-params.test.ts @@ -1,7 +1,8 @@ // SPDX-License-Identifier: MIT // packages/core/src/http/query-params.test.ts // Exercises: HTTP-28 (case-sensitive, multi-value, value-less param), HTTP-29/32 (RFC 3986 encoding), -// HTTP-30 (order-sensitive equality, empty-list dropped), HTTP-31 (lenient parse) +// HTTP-30 (order-sensitive equality, empty-list dropped), HTTP-31 (lenient parse), +// HTTP-5 (getAll returns a frozen list on every path, present name or absent) import {describe, expect, test} from 'bun:test'; import fc from 'fast-check'; import { @@ -9,6 +10,22 @@ import { decodeQueryComponent, encodeQueryComponent, } from './query-params.js'; +import {UrlConstructionError} from './errors.js'; + +/** + * Strings that mix ordinary text with UNPAIRED surrogate code units. `fc.string()` alone never + * produces one — its default unit is printable ASCII — so the URIError path it is here to cover + * would go ungenerated. + */ +const surrogateBearingString = fc.string({ + unit: fc.oneof( + fc.constantFrom('a', 'b', ' ', '=', '&', '%', '+', '\u{1F600}'), + fc + .integer({min: 0xd800, max: 0xdfff}) + .map(code => String.fromCharCode(code)), + ), + maxLength: 8, +}); describe('case-sensitive names and multi-value (HTTP-28)', () => { test('page and Page are distinct names', () => { @@ -149,3 +166,96 @@ test('the component decoder treats a literal + as data, not a space (HTTP-29, PA test('the decoder falls back to raw text on malformed percent-encoding (HTTP-31)', () => { expect(decodeQueryComponent('a%zzb')).toBe('a%zzb'); }); + +describe('lone surrogates are rejected where they are supplied (HTTP-29, HTTP-31)', () => { + // `encodeRfc3986Component` is `encodeURIComponent`, which throws a bare `URIError: URI malformed` + // on a string carrying an unpaired surrogate. Before audit #67 / #76 that escaped the + // `DexpaceError` tree entirely, and it escaped from `encode()` and `equals()` — accessors whose + // TSDoc documents no throw at all — rather than from the `add()` that accepted the value. + const LONE_HIGH = '\uD800'; + const LONE_LOW = '\uDFFF'; + + test.each([ + ['a lone high surrogate value', 'c', LONE_HIGH], + ['a lone low surrogate value', 'c', LONE_LOW], + ['a lone surrogate name', LONE_HIGH, 'v'], + ['a lone surrogate inside a longer value', 'c', `ok${LONE_HIGH}ok`], + ])('add() rejects %s with UrlConstructionError', (_label, name, value) => { + expect(() => QueryParams.newBuilder().add(name, value)).toThrow( + UrlConstructionError, + ); + }); + + test('a well-formed surrogate PAIR is accepted and encoded as UTF-8', () => { + // U+1F600, one code point spelled with two code units. Rejecting this too would make the check + // "no astral characters", which HTTP-29 does not say. + expect( + QueryParams.newBuilder().add('emoji', '\u{1F600}').build().encode(), + ).toBe('emoji=%F0%9F%98%80'); + }); + + test('parse() stays lenient and substitutes U+FFFD, because HTTP-31 forbids throwing', () => { + // HTTP-31 is MUST-level about `parse` never throwing, so the strict `add()` path cannot be the + // one `parse` uses for a value it did not choose. Replacement matches what the platform's own + // query serializer does with the same input: `new URL('https://x/?a=\uD800').search` is + // `?a=%EF%BF%BD` (measured 2026-09-05). This is `Headers`' outbound/inbound split, applied to + // the query model. + const parsed = QueryParams.parse(`a=${LONE_HIGH}`); + expect(parsed.get('a')).toBe('�'); + expect(parsed.encode()).toBe('a=%EF%BF%BD'); + }); + + test('parse() sanitizes the NAME as well as the value', () => { + const parsed = QueryParams.parse(`${LONE_HIGH}=v`); + expect(parsed.has('�')).toBe(true); + expect(parsed.encode()).toBe('%EF%BF%BD=v'); + }); + + test('no URIError escapes encode() or equals(), whatever the builder admitted (property)', () => { + fc.assert( + fc.property(surrogateBearingString, surrogateBearingString, (n, v) => { + let params: QueryParams; + try { + params = QueryParams.newBuilder().add(n, v).build(); + } catch (e: unknown) { + expect(e).toBeInstanceOf(UrlConstructionError); + return; + } + expect(() => params.encode()).not.toThrow(); + expect(() => + params.equals(QueryParams.newBuilder().build()), + ).not.toThrow(); + }), + {numRuns: 500}, + ); + }); + + test('no anything escapes parse(), whatever it is handed (property, HTTP-31)', () => { + fc.assert( + fc.property(surrogateBearingString, raw => { + const parsed = QueryParams.parse(raw); + expect(() => parsed.encode()).not.toThrow(); + }), + {numRuns: 500}, + ); + }); +}); + +describe('getAll returns a frozen list on every path (HTTP-5)', () => { + const params = QueryParams.newBuilder().add('x', '1').add('x', '2').build(); + + test('the present-name list is frozen', () => { + const values = params.getAll('x'); + expect(Object.isFrozen(values)).toBe(true); + expect(() => (values as string[]).push('3')).toThrow(TypeError); + expect(params.getAll('x')).toEqual(['1', '2']); + }); + + test('the absent-name list is frozen too, and is the same shared instance', () => { + const first = params.getAll('nope'); + const second = params.getAll('also-nope'); + expect(Object.isFrozen(first)).toBe(true); + expect(first).toBe(second); + expect(() => (first as string[]).push('x')).toThrow(TypeError); + }); +}); diff --git a/packages/core/src/http/query-params.ts b/packages/core/src/http/query-params.ts index a990680..0166b9b 100644 --- a/packages/core/src/http/query-params.ts +++ b/packages/core/src/http/query-params.ts @@ -1,7 +1,12 @@ // SPDX-License-Identifier: MIT // packages/core/src/http/query-params.ts -import type {Builder} from './builder.js'; -import {encodeRfc3986Component} from './rfc3986.js'; +import {EMPTY_VALUE_LIST, type Builder} from './builder.js'; +import {UrlConstructionError} from './errors.js'; +import { + encodeRfc3986Component, + hasLoneSurrogate, + toWellFormed, +} from './rfc3986.js'; /** * @internal @@ -28,6 +33,22 @@ export function decodeQueryComponent(value: string): string { } } +/** + * The strict half of the surrogate rule, applied by `add()`. + * + * `encodeRfc3986Component` is `encodeURIComponent`, which throws a bare `URIError: URI malformed` + * on an unpaired surrogate — outside the `DexpaceError` tree, and out of `encode()` or `equals()` + * rather than out of the call that supplied the value. Rejecting here puts the failure at the call + * site, which is the same place HTTP-35 and HTTP-17/18 put theirs (audit #67 / #76). + */ +function requireWellFormed(kind: 'name' | 'value', text: string): void { + if (hasLoneSurrogate(text)) { + throw new UrlConstructionError( + `query parameter ${kind} contains an unpaired surrogate and cannot be percent-encoded`, + ); + } +} + let createQueryParams: ( valuesByName: ReadonlyMap, insertionOrder: readonly string[], @@ -43,7 +64,9 @@ let createQueryParams: ( * Encoding and parsing are deliberately asymmetric and kept as separate operations. * {@link QueryParams.encode} is strict RFC 3986 percent-encoding — not * `application/x-www-form-urlencoded`, so a space is `%20` and never `+` (HTTP-29/32). - * {@link QueryParams.parse} is lenient and never throws (HTTP-31). + * {@link QueryParams.parse} is lenient and never throws (HTTP-31). The asymmetry extends to + * unpaired surrogates: {@link QueryParamsBuilder.add} rejects one, while + * {@link QueryParams.parse} substitutes U+FFFD for it. * * @example * ```ts @@ -99,8 +122,10 @@ export class QueryParams { * (HTTP-31). * * A `null`, `undefined`, or blank input yields empty parameters; a leading `?` is tolerated; a - * segment with no `=` or a trailing `=` yields an empty-string value; a stray `&` is skipped; and - * malformed percent-encoding falls back to the raw text rather than failing. + * segment with no `=` or a trailing `=` yields an empty-string value; a stray `&` is skipped; + * malformed percent-encoding falls back to the raw text rather than failing; and an unpaired + * surrogate is replaced with U+FFFD rather than rejected the way + * {@link QueryParamsBuilder.add} rejects one, so the result is always encodable. * * @param raw - the query string, with or without its leading `?`. * @returns the parsed, frozen parameters. @@ -116,9 +141,12 @@ export class QueryParams { const eqIndex = segment.indexOf('='); const rawName = eqIndex === -1 ? segment : segment.slice(0, eqIndex); const rawValue = eqIndex === -1 ? '' : segment.slice(eqIndex + 1); + // HTTP-31 is MUST-level that parsing never throws, so the strict `add()` path above cannot be + // the one `parse` uses on text it did not choose — exactly the split `Headers` draws between + // its outbound (`add`) and inbound (`addInbound`) methods for HTTP-18 against HTTP-19. builder.add( - decodeQueryComponent(rawName), - decodeQueryComponent(rawValue), + toWellFormed(decodeQueryComponent(rawName)), + toWellFormed(decodeQueryComponent(rawValue)), ); } return builder.build(); @@ -138,10 +166,11 @@ export class QueryParams { * Returns every value stored under `name`, in insertion order. * * @param name - the parameter name. - * @returns a read-only, frozen list of values — empty when the name is absent. + * @returns a read-only, frozen list of values — the shared frozen empty list when the name is + * absent. Frozen on both paths, so mutating it cannot reach the model (HTTP-5). */ getAll(name: string): readonly string[] { - return this.#valuesByName.get(name) ?? []; + return this.#valuesByName.get(name) ?? EMPTY_VALUE_LIST; } /** @@ -201,9 +230,15 @@ export class QueryParamsBuilder implements Builder { * @param value - the value; `null` records a value-less parameter as a single empty string * (HTTP-28). * @returns this builder, for chaining. + * @throws {@link UrlConstructionError} when the name or the value carries an unpaired surrogate. + * Such a string has no UTF-8 form, so RFC 3986 percent-encoding is undefined for it and + * {@link QueryParams.encode} could only fail — it is rejected here, at the call that supplied it. + * A well-formed surrogate pair is ordinary text and is accepted (HTTP-29). */ add(name: string, value: string | null): this { const actualValue = value ?? ''; + requireWellFormed('name', name); + requireWellFormed('value', actualValue); if (!this.#valuesByName.has(name)) { this.#insertionOrder.push(name); this.#valuesByName.set(name, []); diff --git a/packages/core/src/http/request-conditions.test.ts b/packages/core/src/http/request-conditions.test.ts index 4382f64..b89c3bb 100644 --- a/packages/core/src/http/request-conditions.test.ts +++ b/packages/core/src/http/request-conditions.test.ts @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT // packages/core/src/http/request-conditions.test.ts -// Exercises: HTTP-50 (comma-joined If-Match/If-None-Match, RFC 1123 dates, idempotent apply, any-tag exclusivity) +// Exercises: HTTP-50 (comma-joined If-Match/If-None-Match, RFC 1123 dates, idempotent apply, any-tag +// exclusivity, and an invalid Date rejected at the setter rather than emitted as `Invalid Date`) import {describe, expect, test} from 'bun:test'; import { RequestConditions, @@ -134,3 +135,53 @@ describe('ifUnmodifiedSince (HTTP-50)', () => { ]); }); }); + +describe('an invalid Date is rejected at the setter (HTTP-50)', () => { + // `toRfc1123` is `date.toUTCString()`, which renders the literal string `Invalid Date` for a NaN + // time value. `Invalid Date` is HTAB-free printable ASCII, so HTTP-18's outbound header grammar + // waves it through and it reaches the wire as `If-Modified-Since: Invalid Date` — a header no + // server can evaluate, produced from a caller mistake made several frames earlier. HTTP-50's + // "emit RFC 1123 dates" is not satisfiable from a NaN instant, so the setter is where it fails. + // Measured on the pre-fix tree, audit #67 / #76. + test.each([ + ['new Date("nope")', new Date('nope')], + ['new Date(NaN)', new Date(Number.NaN)], + ])( + 'ifModifiedSince rejects %s with RequestConditionsValidationError', + (_label, date) => { + expect(() => + RequestConditions.newBuilder().ifModifiedSince(date), + ).toThrow(RequestConditionsValidationError); + }, + ); + + test.each([ + ['new Date("nope")', new Date('nope')], + ['new Date(NaN)', new Date(Number.NaN)], + ])( + 'ifUnmodifiedSince rejects %s with RequestConditionsValidationError', + (_label, date) => { + expect(() => + RequestConditions.newBuilder().ifUnmodifiedSince(date), + ).toThrow(RequestConditionsValidationError); + }, + ); + + test('the message names the setter, so the caller knows which field to fix', () => { + expect(() => + RequestConditions.newBuilder().ifModifiedSince(new Date('nope')), + ).toThrow(/If-Modified-Since/); + expect(() => + RequestConditions.newBuilder().ifUnmodifiedSince(new Date('nope')), + ).toThrow(/If-Unmodified-Since/); + }); + + test('no invalid instant can reach applyTo, so no header renders "Invalid Date"', () => { + const builder = RequestConditions.newBuilder(); + expect(() => builder.ifModifiedSince(new Date('nope'))).toThrow( + RequestConditionsValidationError, + ); + const headers = builder.build().applyTo(Headers.newBuilder().build()); + expect(headers.has('If-Modified-Since')).toBe(false); + }); +}); diff --git a/packages/core/src/http/request-conditions.ts b/packages/core/src/http/request-conditions.ts index a973e88..db1d564 100644 --- a/packages/core/src/http/request-conditions.ts +++ b/packages/core/src/http/request-conditions.ts @@ -30,6 +30,25 @@ function toRfc1123(date: Date): string { return date.toUTCString(); } +/** + * Copies `date` after rejecting a NaN time value. + * + * `toUTCString()` is total — it renders the literal string `Invalid Date` rather than throwing — + * and `Invalid Date` is HTAB-free printable ASCII, so HTTP-18's outbound header grammar accepts it + * and it reaches the wire as `If-Modified-Since: Invalid Date`. HTTP-50 requires an RFC 1123 date, + * which a NaN instant cannot produce, so the failure belongs at the setter that was handed the bad + * `Date` rather than several frames downstream (audit #67 / #76). + */ +function copyValidInstant(date: Date, headerName: string): Date { + const time = date.getTime(); + if (Number.isNaN(time)) { + throw new RequestConditionsValidationError( + `${headerName}: date must be a valid instant, got an invalid Date`, + ); + } + return new Date(time); +} + // eslint-disable-next-line max-params -- private, builder-internal plumbing; field count fixed by HTTP-50 let createRequestConditions: ( ifMatch: readonly ETag[], @@ -45,7 +64,9 @@ let createRequestConditions: ( * Multiple entity-tags emit as one comma-separated header; dates emit in RFC 1123 form. * {@link RequestConditions.applyTo} uses `set`, never `add`, so applying the same conditions twice * cannot duplicate a header. The any-tag (`*`) is mutually exclusive with concrete entity-tags, and - * repeated `*` collapses to one — enforced when the tag is added, not at emission. + * repeated `*` collapses to one — enforced when the tag is added, not at emission. An invalid + * `Date` is likewise rejected by the setter that was handed it, so no instance can emit the literal + * header value `Invalid Date`. * * @example * ```ts @@ -204,9 +225,12 @@ export class RequestConditionsBuilder implements Builder { * @param date - the instant; copied, not aliased, so a caller mutating its own `Date` after * `build()` cannot change what {@link RequestConditions.applyTo} emits. * @returns this builder, for chaining. + * @throws {@link RequestConditionsValidationError} when `date` carries a NaN time value. An + * invalid `Date` renders as the literal `Invalid Date`, which the outbound header grammar accepts + * and no server can evaluate, so it is rejected here rather than emitted (HTTP-50). */ ifModifiedSince(date: Date): this { - this.#ifModifiedSince = new Date(date.getTime()); + this.#ifModifiedSince = copyValidInstant(date, 'If-Modified-Since'); return this; } @@ -216,9 +240,11 @@ export class RequestConditionsBuilder implements Builder { * @param date - the instant; copied, not aliased, exactly as in * {@link RequestConditionsBuilder.ifModifiedSince}. * @returns this builder, for chaining. + * @throws {@link RequestConditionsValidationError} when `date` carries a NaN time value, for the + * reason given on {@link RequestConditionsBuilder.ifModifiedSince} (HTTP-50). */ ifUnmodifiedSince(date: Date): this { - this.#ifUnmodifiedSince = new Date(date.getTime()); + this.#ifUnmodifiedSince = copyValidInstant(date, 'If-Unmodified-Since'); return this; } diff --git a/packages/core/src/http/request-options.test.ts b/packages/core/src/http/request-options.test.ts index 7bec354..bcfc28f 100644 --- a/packages/core/src/http/request-options.test.ts +++ b/packages/core/src/http/request-options.test.ts @@ -3,6 +3,7 @@ // Exercises: HTTP-34 (EMPTY sentinel, defensive tag copy), HTTP-35 (timeout/maxRetries validation), // AUTH-4 (the per-call auth descriptor tier, added in Phase 5c) import {describe, expect, test} from 'bun:test'; +import fc from 'fast-check'; import {createAuthDescriptor} from '../auth/descriptor.js'; import {createAuthRequirement} from '../auth/requirement.js'; import {RequestOptions} from './request-options.js'; @@ -74,6 +75,31 @@ describe('operation auth descriptor (AUTH-4, docs/work/mvp/2026-09-04-open-items }); }); +const timeoutCandidate = fc.oneof( + fc.double({noNaN: false}), + fc.integer({min: -10, max: 10}), + fc.constantFrom(2 ** 32 - 1, 2 ** 32, Number.MAX_SAFE_INTEGER), +); + +/** + * Either the builder refuses `candidate` with the typed error, or it admits a value inside + * `AbortSignal.timeout()`'s range. There is no third outcome — that is the whole HTTP-35 claim. + */ +function expectAdmittedTimeoutInRange(candidate: number): void { + let accepted: number | undefined; + try { + accepted = RequestOptions.newBuilder() + .timeoutMs(candidate) + .build().timeoutMs; + } catch (e: unknown) { + expect(e).toBeInstanceOf(RequestOptionsValidationError); + return; + } + expect(Number.isInteger(accepted)).toBe(true); + expect(accepted).toBeGreaterThanOrEqual(1); + expect(accepted).toBeLessThanOrEqual(2 ** 32 - 1); +} + describe('timeout validation (HTTP-35)', () => { test('rejects zero or negative timeout', () => { expect(() => RequestOptions.newBuilder().timeoutMs(0)).toThrow( @@ -93,9 +119,47 @@ describe('timeout validation (HTTP-35)', () => { ); }); - test('accepts a fractional millisecond timeout, which a deadline can honor', () => { - expect(RequestOptions.newBuilder().timeoutMs(1.5).build().timeoutMs).toBe( - 1.5, + // Flipped by audit #67 / #76. The old case pinned `timeoutMs(1.5)` as accepted "because a deadline + // can honor a fractional millisecond". Nothing downstream can: the only consumer is + // `composeSignal`, which hands the value to `AbortSignal.timeout()`, and that throws + // `RangeError: The value of "delay" is out of range. It must be an integer.` — inside the + // transport, one seam away from the setter that accepted it. HTTP-35 puts the range check at the + // setter, so the range checked is `AbortSignal.timeout()`'s, the only one a transport can honor. + test('rejects a fractional timeout, which no transport deadline can honor', () => { + expect(() => RequestOptions.newBuilder().timeoutMs(1.5)).toThrow( + RequestOptionsValidationError, + ); + }); + + test("rejects a timeout above AbortSignal.timeout()'s ceiling of 2**32 - 1", () => { + expect(() => RequestOptions.newBuilder().timeoutMs(2 ** 32)).toThrow( + RequestOptionsValidationError, + ); + expect(() => + RequestOptions.newBuilder().timeoutMs(Number.MAX_SAFE_INTEGER), + ).toThrow(RequestOptionsValidationError); + }); + + test('accepts the ceiling itself, so the boundary is inclusive', () => { + expect( + RequestOptions.newBuilder() + .timeoutMs(2 ** 32 - 1) + .build().timeoutMs, + ).toBe(2 ** 32 - 1); + }); + + // The invariant, stated runtime-independently: a value this setter accepts is inside + // `AbortSignal.timeout()`'s documented range, and anything else fails here. It is asserted as a + // property rather than against `AbortSignal.timeout()` itself because the two runtimes disagree — + // Bun accepts `1.5` and `2**32` where Node raises `RangeError` — which is precisely why the range + // is checked in the model instead of left to whichever runtime the caller happens to be on. + // `tests/node-conformance/seams.test.mjs` closes the Node half. + test('every accepted timeout is an integer in 1..2**32 - 1 (property)', () => { + fc.assert( + fc.property(timeoutCandidate, candidate => { + expectAdmittedTimeoutInRange(candidate); + }), + {numRuns: 500}, ); }); diff --git a/packages/core/src/http/request-options.ts b/packages/core/src/http/request-options.ts index 77e7e7c..b0258dd 100644 --- a/packages/core/src/http/request-options.ts +++ b/packages/core/src/http/request-options.ts @@ -4,6 +4,13 @@ import type {AuthDescriptor} from '../auth/descriptor.js'; import type {Builder} from './builder.js'; import {RequestOptionsValidationError} from './errors.js'; +/** + * The largest timeout `AbortSignal.timeout()` accepts, and therefore the largest one this model + * will hold: the platform rejects anything above it with `RangeError: The value of "delay" is out + * of range. It must be >= 0 && <= 4294967295.` (HTTP-35, audit #67 / #76). + */ +const MAX_TIMEOUT_MS = 2 ** 32 - 1; + // eslint-disable-next-line max-params -- private, builder-internal plumbing; one parameter per HTTP-34 field let createRequestOptions: ( timeoutMs: number | undefined, @@ -92,7 +99,11 @@ export class RequestOptions { .operationAuth(this.#operationAuth); } - /** The per-call timeout in milliseconds, or `undefined` to use the configured default. */ + /** + * The per-call timeout in milliseconds, or `undefined` to use the configured default. Always an + * integer in `1 .. 2**32 - 1` when defined — {@link RequestOptionsBuilder.timeoutMs} admits + * nothing else, so a transport can pass it to `AbortSignal.timeout()` unchecked. + */ get timeoutMs(): number | undefined { return this.#timeoutMs; } @@ -172,20 +183,31 @@ export class RequestOptionsBuilder implements Builder { * The range check is the FULL range, not merely its lower bound. `Infinity` and `NaN` are as out * of range as `-1`: a non-finite deadline is one no clock can compare against, so it degrades to * "no deadline" silently rather than failing at the call site that supplied it, which is exactly - * what HTTP-35 exists to prevent. Not required to be integral, unlike `maxRetries` -- a timeout is - * a duration and a fractional millisecond is meaningful. + * what HTTP-35 exists to prevent. + * + * The range is `AbortSignal.timeout()`'s — an integer in `1 .. 2**32 - 1` — because that is the + * only range a transport can honor. `composeSignal` is the one consumer of this value and it + * hands it straight to `AbortSignal.timeout()`, which throws a `RangeError` on a fractional + * millisecond and on anything above `4294967295`. A timeout this setter accepted and a transport + * then refused is the failure HTTP-35 exists to move to the call site, so integrality is checked + * here and not treated as an implementation detail of one transport (audit #67 / #76 flipped the + * earlier reading, which accepted `1.5` on the argument that a duration may be fractional; no + * consumer of this field can express one). * * @param value - the timeout in milliseconds, or `undefined` for no override. Zero is rejected * rather than reinterpreted: it means "no timeout" in one transport and is an error in another * (HTTP-35). * @returns this builder, for chaining. - * @throws {@link RequestOptionsValidationError} when a defined value is zero, negative, or not - * finite. + * @throws {@link RequestOptionsValidationError} when a defined value is zero, negative, not + * finite, not an integer, or greater than `2**32 - 1`. */ timeoutMs(value: number | undefined): this { - if (value !== undefined && !(Number.isFinite(value) && value > 0)) { + if ( + value !== undefined && + !(Number.isInteger(value) && value > 0 && value <= MAX_TIMEOUT_MS) + ) { throw new RequestOptionsValidationError( - `timeout must be a finite positive duration, got ${String(value)}`, + `timeout must be an integer number of milliseconds in 1..${String(MAX_TIMEOUT_MS)}, got ${String(value)}`, ); } this.#timeoutMs = value; diff --git a/packages/core/src/http/rfc3986.ts b/packages/core/src/http/rfc3986.ts index a2d6db5..c96955a 100644 --- a/packages/core/src/http/rfc3986.ts +++ b/packages/core/src/http/rfc3986.ts @@ -1,13 +1,66 @@ // SPDX-License-Identifier: MIT // packages/core/src/http/rfc3986.ts +/** + * Matches an UNPAIRED surrogate code unit, and only an unpaired one. + * + * In a `u`-mode pattern the engine works in code points, so a well-formed surrogate pair is one + * non-surrogate code point and does not match, while a lone high or low unit stays a surrogate code + * point and does. Equivalent to `!String.prototype.isWellFormed()`, which is ES2024 and so outside + * this repo's declared `lib` (`tsconfig.base.json` pins `ES2023`) even though the + * `engines.node >= 20.3` runtime has it — raising `lib` for one predicate is a wider change than + * the predicate is worth. + * + * Two patterns, one rule: `.test()` must not carry `lastIndex` between calls, and `.replace()` must + * be global. Neither is exported; the two functions below are, so no caller can pick the wrong one. + */ +const LONE_SURROGATE = /\p{Surrogate}/u; +const LONE_SURROGATE_GLOBAL = /\p{Surrogate}/gu; + +/** Unicode's replacement character, what a lenient repair substitutes for an unpaired surrogate. */ +const REPLACEMENT_CHARACTER = '\uFFFD'; + +/** + * Whether `value` carries an unpaired surrogate, and so has no UTF-8 form and cannot be + * percent-encoded. The strict half of the rule: a call site that was HANDED such a string rejects + * it (audit #67 / #76). + * + * @param value - the string to inspect. + * @returns `true` when at least one surrogate code unit is unpaired. + */ +export function hasLoneSurrogate(value: string): boolean { + return LONE_SURROGATE.test(value); +} + +/** + * `value` with every unpaired surrogate replaced by U+FFFD. The lenient half, for a call site that + * MUST NOT throw — `QueryParams.parse` under HTTP-31. Matches what the platform's own query + * serializer does with the same input: `new URL('https://x/?a=\uD800').search` is `?a=%EF%BF%BD` + * (measured 2026-09-05). + * + * @param value - the string to repair. + * @returns `value` with unpaired surrogates replaced; the same string when there are none. + */ +export function toWellFormed(value: string): string { + return value.replace(LONE_SURROGATE_GLOBAL, REPLACEMENT_CHARACTER); +} + /** * Percent-encodes a single URL component per RFC 3986, patching `encodeURIComponent`'s divergence: * `encodeURIComponent` leaves `! * ' ( )` unescaped, but none of them are in RFC 3986's unreserved * set (HTTP-29). * - * @param value - the raw component value. + * Deliberately NOT total, and deliberately not guarded here. `encodeURIComponent` throws + * `URIError: URI malformed` on a string carrying an unpaired surrogate, because such a string has + * no UTF-8 form. Every caller in this package rejects or repairs that input BEFORE reaching here — + * `QueryParamsBuilder.add`, `QueryParams.parse` and `substitutePathParams` each do, and each throws + * the error class its own call site already throws — so a second, silent guard inside the encoder + * would only move the failure back off the call site (audit #67 / #76). + * + * @param value - the raw component value; must not carry an unpaired surrogate. * @returns the percent-encoded component. + * @throws A platform `URIError` when `value` carries an unpaired surrogate. Callers validate first; + * see the note above. */ export function encodeRfc3986Component(value: string): string { return encodeURIComponent(value).replace( diff --git a/packages/core/src/io/tee-sink.test.ts b/packages/core/src/io/tee-sink.test.ts index 32560d8..0ac8c76 100644 --- a/packages/core/src/io/tee-sink.test.ts +++ b/packages/core/src/io/tee-sink.test.ts @@ -7,7 +7,8 @@ // IO-42 (write after close rejects with the source intact), // IO-13 (the tap mirrors the primary's exact encoded bytes, and refuses a label identically), // IO-16 (the tee's own writable bridge still feeds the tap), -// IO-3 (a negative count is an argument error, rejected before any transfer) +// IO-3 (a negative count is an argument error, rejected before any transfer; and a non-integral +// tapLimit is one too, rejected at the constructor rather than at the first write) import {describe, expect, test} from 'bun:test'; import fc from 'fast-check'; import {BufferedSink} from './buffered-sink.js'; @@ -15,6 +16,7 @@ import {BufferedSource} from './buffered-source.js'; import {ByteQueue} from './byte-queue.js'; import {ClosedResourceError} from './errors.js'; import {TeeSink} from './tee-sink.js'; +import {InvariantViolation} from '../invariant.js'; import {writeAll} from './pump.js'; import { collectingWritableStream, @@ -300,4 +302,45 @@ describe('argument validation (IO-3)', () => { 'InvariantViolation', ); }); + + // The constructor checked `tapLimit >= 0` only, so a fractional cap was accepted and the fault + // surfaced at the FIRST write instead: `#mirror` computes `room = tapLimit - tap.size`, hands it + // to `ByteQueue.copyTo`, and `assertCount` rejects it as `count must be a non-negative integer, + // got 2.5` — a message about the wrong parameter, at the wrong call, on a path where the primary + // write has not run yet. A byte count is integral for the same reason `count` is (IO-3, IO-26). + // Measured on the pre-fix tree, audit #67 / #76. + test.each([2.5, 0.5, -0.5, Number.NaN, Number.NEGATIVE_INFINITY])( + 'a tapLimit of %p is rejected at the constructor', + tapLimit => { + const {stream} = collectingWritableStream(); + expect( + () => new TeeSink(BufferedSink.overStream(stream), tapLimit), + ).toThrow(InvariantViolation); + }, + ); + + test('the message names tapLimit, not count', () => { + const {stream} = collectingWritableStream(); + expect(() => new TeeSink(BufferedSink.overStream(stream), 2.5)).toThrow( + /tapLimit/, + ); + }); + + test.each([0, 1, 4096, Number.POSITIVE_INFINITY])( + 'a tapLimit of %p is still accepted', + tapLimit => { + const {stream} = collectingWritableStream(); + expect( + () => new TeeSink(BufferedSink.overStream(stream), tapLimit), + ).not.toThrow(); + }, + ); + + test('the unbounded default stays Infinity, which is not an integer', () => { + // `Number.isInteger(Infinity)` is false, so the check has to admit it explicitly — it is the + // documented default and the cap `#mirror` reads as "no cap" (IO-26). + const {stream} = collectingWritableStream(); + const tee = new TeeSink(BufferedSink.overStream(stream)); + expect(tee.snapshot().length).toBe(0); + }); }); diff --git a/packages/core/src/io/tee-sink.ts b/packages/core/src/io/tee-sink.ts index dc6e560..83b1d15 100644 --- a/packages/core/src/io/tee-sink.ts +++ b/packages/core/src/io/tee-sink.ts @@ -31,9 +31,18 @@ export class TeeSink implements Sink { readonly #tapLimit: number; constructor(primary: Sink, tapLimit: number = Number.POSITIVE_INFINITY) { + // Integrality, not merely `>= 0`. A tap cap is a byte count, so IO-3's rule for `count` applies + // to it — and a fractional cap was not merely odd, it was deferred: `#mirror` computes + // `room = tapLimit - tap.size` and hands it to `ByteQueue.copyTo`, where `assertCount` rejects + // it as "count must be a non-negative integer, got 2.5". That names the wrong parameter, fires + // at the first write rather than at the construction that supplied it, and does so before the + // primary write on a path that has already taken bytes from the caller. `Infinity` is admitted + // explicitly: it is not an integer, and it is the documented unbounded default (IO-26). + // Audit #67 / #76. invariant( - tapLimit >= 0, - `tapLimit must be non-negative, got ${String(tapLimit)}`, + (Number.isInteger(tapLimit) && tapLimit >= 0) || + tapLimit === Number.POSITIVE_INFINITY, + `tapLimit must be a non-negative integer or Infinity, got ${String(tapLimit)}`, ); this.#primary = primary; this.#tapLimit = tapLimit; diff --git a/packages/core/src/seams/operation.test.ts b/packages/core/src/seams/operation.test.ts index 3927e4e..5e588a9 100644 --- a/packages/core/src/seams/operation.test.ts +++ b/packages/core/src/seams/operation.test.ts @@ -1,8 +1,10 @@ // SPDX-License-Identifier: MIT // packages/core/src/seams/operation.test.ts // Exercises: SEAM-26 (the four projections default to empty), SEAM-27 (buildRequest's encoding and base-URL -// composition rules), HTTP-7 (a body projected onto a body-forbidding method fails assembly), reusing -// HTTP-29's encodeRfc3986Component for path-segment encoding. +// composition rules, and that a placeholder is satisfied only by an OWN property of pathParams), +// HTTP-7 (a body projected onto a body-forbidding method fails assembly), reusing +// HTTP-29's encodeRfc3986Component for path-segment encoding — including the unpaired-surrogate input it +// cannot encode, which is rejected here rather than allowed to escape as a bare URIError. import {describe, expect, test} from 'bun:test'; import fc from 'fast-check'; import { @@ -158,6 +160,136 @@ describe('SEAM-27: dot-segment path-param values are rejected, not silently norm }); }); +describe('SEAM-27: a placeholder is satisfied only by an OWN property of pathParams', () => { + // `pathParams?.[name]` reached the whole prototype chain, so `{constructor}` against `{}` resolved to + // `Object`'s own constructor, stringified, and shipped + // `/users/function%20Object%28%29%20%7B%20%5Bnative%20code%5D%20%7D` instead of failing assembly. Every + // placeholder MUST have a *supplied* value (SEAM-27); a name the caller never supplied is a missing value + // whatever `Object.prototype` happens to carry. Measured on the pre-fix tree, audit #67 / #76. + test.each([ + 'constructor', + 'toString', + 'hasOwnProperty', + 'valueOf', + '__proto__', + ])( + 'a {%s} placeholder against empty pathParams throws OperationAssemblyError', + name => { + expect(() => + buildRequest('https://api.example.com', { + method: 'GET', + pathTemplate: `/users/{${name}}`, + pathParams: {}, + }), + ).toThrow(OperationAssemblyError); + }, + ); + + test('the error names the placeholder, not the inherited member it resolved to', () => { + expect(() => + buildRequest('https://api.example.com', { + method: 'GET', + pathTemplate: '/users/{constructor}', + pathParams: {}, + }), + ).toThrow(/missing value for path parameter "constructor"/); + }); + + test('an own property named like a prototype member is still honored', () => { + const request = buildRequest('https://api.example.com', { + method: 'GET', + pathTemplate: '/users/{constructor}', + pathParams: {constructor: 'me'}, + }); + expect(request.url.pathname).toBe('/users/me'); + }); + + test('a null-prototype pathParams object still resolves its own keys', () => { + const pathParams = Object.assign(Object.create(null) as object, { + id: 'x', + }) as Record; + const request = buildRequest('https://api.example.com', { + method: 'GET', + pathTemplate: '/users/{id}', + pathParams, + }); + expect(request.url.pathname).toBe('/users/x'); + }); +}); + +describe('SEAM-27: an unpaired surrogate in a path-param value is rejected', () => { + // `encodeRfc3986Component` is `encodeURIComponent`, which throws a bare `URIError: URI malformed` + // on a string with no UTF-8 form. Before audit #67 / #76 that escaped `buildRequest` outside the + // `DexpaceError` tree and outside its `@throws` list. It is now `OperationAssemblyError`, the + // class this call site already throws for a path-param value it cannot use. + test.each([ + ['a lone high surrogate', '\uD800'], + ['a lone low surrogate', '\uDFFF'], + ['a lone surrogate inside a longer value', 'ok\uD800ok'], + ])('%s throws OperationAssemblyError', (_label, value) => { + expect(() => + buildRequest('https://host', { + method: 'GET', + pathTemplate: '/things/{id}', + pathParams: {id: value}, + }), + ).toThrow(OperationAssemblyError); + }); + + test('a well-formed surrogate pair is ordinary text and is encoded', () => { + const request = buildRequest('https://host', { + method: 'GET', + pathTemplate: '/things/{id}', + pathParams: {id: '\u{1F600}'}, + }); + expect(request.url.pathname).toBe('/things/%F0%9F%98%80'); + }); + + test('the query projection cannot carry one either, from a builder or from parse()', () => { + // `composeQuery` calls `operationQuery.encode()`, the second `encodeRfc3986Component` path into + // `buildRequest`. Both ways of obtaining a `QueryParams` are now closed: the builder rejects an + // unpaired surrogate, `parse` replaces it. + expect(() => QueryParams.newBuilder().add('c', '\uD800')).toThrow( + /unpaired surrogate/, + ); + const request = buildRequest('https://host', { + method: 'GET', + pathTemplate: '/things', + query: QueryParams.parse('c=\uD800'), + }); + expect(request.url.search).toBe('?c=%EF%BF%BD'); + }); + + test('no URIError escapes buildRequest, whatever the descriptor carries (property)', () => { + fc.assert( + fc.property( + fc.string({ + unit: fc.oneof( + fc.constantFrom('a', '/', '.', '%', ' ', '\u{1F600}'), + fc + .integer({min: 0xd800, max: 0xdfff}) + .map(code => String.fromCharCode(code)), + ), + minLength: 1, + maxLength: 6, + }), + value => { + try { + buildRequest('https://host', { + method: 'GET', + pathTemplate: '/things/{id}', + pathParams: {id: value}, + }); + } catch (e: unknown) { + expect(e).toBeInstanceOf(OperationAssemblyError); + } + }, + ), + {numRuns: 500}, + ); + }); +}); + describe('a path-param value containing / is encoded, not split (property)', () => { test('holds for arbitrary generated path-param values', () => { fc.assert( diff --git a/packages/core/src/seams/operation.ts b/packages/core/src/seams/operation.ts index b6213f2..0683691 100644 --- a/packages/core/src/seams/operation.ts +++ b/packages/core/src/seams/operation.ts @@ -6,13 +6,14 @@ import type {Headers} from '../http/headers.js'; import type {QueryParams} from '../http/query-params.js'; import type {Method} from '../http/method.js'; import {UrlConstructionError, DexpaceError} from '../http/errors.js'; -import {encodeRfc3986Component} from '../http/rfc3986.js'; +import {encodeRfc3986Component, hasLoneSurrogate} from '../http/rfc3986.js'; /** * Thrown when `buildRequest()` cannot assemble a request from its descriptor: a `{name}` - * placeholder in `pathTemplate` has no value in `pathParams`, or a supplied value is a dot segment - * (`.`/`..`) that the WHATWG URL parser would normalize into a path rewrite instead of keeping as - * one literal segment. + * placeholder in `pathTemplate` has no OWN value in `pathParams` — an inherited member such as + * `constructor` does not satisfy one — or a supplied value is a dot segment (`.`/`..`) that the + * WHATWG URL parser would normalize into a path rewrite instead of keeping as one literal segment, + * or a supplied value carries an unpaired surrogate and so has no percent-encoded form. * * @public */ @@ -50,9 +51,10 @@ export interface OperationDescriptor { readonly pathTemplate: string; /** - * Values for `pathTemplate`'s `{name}` placeholders. Every placeholder must have a value here; - * each value is percent-encoded as a single path segment, so a value containing `/` cannot inject - * an extra segment (SEAM-27). Defaults to empty. + * Values for `pathTemplate`'s `{name}` placeholders. Every placeholder must have an OWN property + * here — a name reachable only through the prototype chain, such as `constructor`, is treated as + * absent — and each value is percent-encoded as a single path segment, so a value containing `/` + * cannot inject an extra segment (SEAM-27). Defaults to empty. */ readonly pathParams?: Readonly> | undefined; @@ -103,7 +105,16 @@ function substitutePathParams( pathParams: Readonly> | undefined, ): string { return template.replace(PATH_PARAM_RE, (_match, name: string) => { - const value = pathParams?.[name]; + // `Object.hasOwn`, not `pathParams?.[name]`: the indexed read walks the prototype chain, so + // `{constructor}` against `{}` resolved to `Object.prototype.constructor`, stringified through + // `encodeRfc3986Component`, and shipped a native-code source text as a path segment instead of + // failing assembly. SEAM-27 requires every placeholder to have a *supplied* value, and a name + // the caller never supplied is missing whatever `Object.prototype` happens to carry + // (audit #67 / #76). + const value = + pathParams !== undefined && Object.hasOwn(pathParams, name) + ? pathParams[name] + : undefined; if (value === undefined) { throw new OperationAssemblyError( `missing value for path parameter "${name}"`, @@ -120,6 +131,17 @@ function substitutePathParams( name, ); } + // A string carrying an unpaired surrogate has no UTF-8 form, so `encodeRfc3986Component` — i.e. + // `encodeURIComponent` — throws a bare `URIError: URI malformed`. That escaped `buildRequest` + // outside the `DexpaceError` tree and outside its documented `@throws`. Rejected here with the + // class this call site already throws, rather than guarded inside the encoder, so the failure + // names the parameter (audit #67 / #76). + if (hasLoneSurrogate(value)) { + throw new OperationAssemblyError( + `path parameter "${name}" contains an unpaired surrogate and cannot be percent-encoded`, + name, + ); + } return encodeRfc3986Component(value); }); } @@ -158,8 +180,10 @@ function composeQuery( * @param baseUrl - the absolute base URL to project the operation onto. * @param operation - the operation to assemble into a request. * @returns the assembled request. - * @throws {@link OperationAssemblyError} when a `{name}` placeholder has no value in `pathParams`, - * or a supplied value is a dot segment (`.`/`..`) — fix the descriptor; no request was assembled. + * @throws {@link OperationAssemblyError} when a `{name}` placeholder has no own value in + * `pathParams` (a name inherited from `Object.prototype` does not count as supplied), or a supplied + * value is a dot segment (`.`/`..`), or a supplied value carries an unpaired surrogate — fix the + * descriptor; no request was assembled. * @throws {@link UrlConstructionError} when `baseUrl` is malformed, non-absolute, or carries a * fragment — supply a clean absolute base URL. * @throws {@link RequestBodyNotAllowedError} when the descriptor pairs a body with GET, HEAD, TRACE diff --git a/packages/core/src/seams/transport.test.ts b/packages/core/src/seams/transport.test.ts index 81a78ea..d43d497 100644 --- a/packages/core/src/seams/transport.test.ts +++ b/packages/core/src/seams/transport.test.ts @@ -3,12 +3,16 @@ // Exercises: SEAM-18's residual (composeSignal is the per-call-options-threading helper's cancellation half), // XCUT-2 (timeout vs. caller-cancellation told apart by signal.reason.name, not a message string). // No stub Transport is constructed — neither composeSignal nor isTimeoutSignal takes or returns one. +// Also HTTP-35 (composeSignal's documented RangeError is AbortSignal.timeout()'s own, and no value +// RequestOptionsBuilder accepts can produce it). import {describe, expect, test} from 'bun:test'; import { composeSignal, isTimeoutSignal, CancellationError, } from './transport.js'; +import {RequestOptions} from '../http/request-options.js'; +import {RequestOptionsValidationError} from '../http/errors.js'; describe('composeSignal', () => { test('returns undefined when neither input is supplied', () => { @@ -55,3 +59,29 @@ describe('isTimeoutSignal', () => { expect(isTimeoutSignal(new AbortController().signal)).toBe(false); }); }); + +describe('composeSignal timeout range (HTTP-35)', () => { + // `composeSignal` hands `timeoutMs` straight to `AbortSignal.timeout()`, and what that does with + // an out-of-range delay is a RUNTIME decision, measured 2026-09-05: Node raises + // `RangeError: The value of "delay" is out of range` for `1.5`, for `2**32` and for `-1`; Bun + // accepts `1.5` and `2**32` and raises a `TypeError` for `-1`. So the only claim assertable on + // both is the one below — that no value `RequestOptionsBuilder` accepts can reach that fork at + // all. `tests/node-conformance/seams.test.mjs` asserts the Node half, where the throw is real. + // This is why audit #67 / #76 put the range check in the model rather than clamping here. + test('every timeout RequestOptionsBuilder accepts composes without throwing', () => { + for (const value of [1, 1000, 2 ** 32 - 1]) { + const accepted = RequestOptions.newBuilder() + .timeoutMs(value) + .build().timeoutMs; + expect(() => composeSignal(undefined, accepted)).not.toThrow(); + } + }); + + test('a timeout the builder rejects never reaches composeSignal', () => { + for (const value of [1.5, 2 ** 32, -1, 0]) { + expect(() => RequestOptions.newBuilder().timeoutMs(value)).toThrow( + RequestOptionsValidationError, + ); + } + }); +}); diff --git a/packages/core/src/seams/transport.ts b/packages/core/src/seams/transport.ts index 8cd62fc..8388603 100644 --- a/packages/core/src/seams/transport.ts +++ b/packages/core/src/seams/transport.ts @@ -76,8 +76,20 @@ export interface Transport { * logic. * * @param userSignal - an optional caller-supplied abort signal. - * @param timeoutMs - an optional timeout, in milliseconds. + * @param timeoutMs - an optional timeout, in milliseconds. Must be an integer in + * `1 .. 2**32 - 1` — the range Node's `AbortSignal.timeout()` accepts. A value taken from + * {@link RequestOptions.timeoutMs} always is, because {@link RequestOptionsBuilder.timeoutMs} + * rejects everything else at the call site (HTTP-35). A transport's own `defaultTimeoutMs` + * construction option is NOT validated by this package and is the one remaining way an + * out-of-range value reaches here. * @returns the composed signal, the sole supplied signal, or `undefined` when neither is supplied. + * @throws Whatever the host runtime's `AbortSignal.timeout()` raises for an out-of-range delay, + * unwrapped. The runtimes disagree, measured 2026-09-05: Node raises `RangeError` for a fractional + * value, for anything above `4294967295`, and for a negative one; Bun accepts the first two and + * raises `TypeError` for the third. Not wrapped in a `DexpaceError` and not clamped here — it is a + * programming error in whatever supplied the value, and the divergence is exactly why the range + * lives on {@link RequestOptionsBuilder.timeoutMs}, which rejects every such value identically on + * both runtimes (HTTP-35, audit #67 / #76). * * @public */ diff --git a/tests/node-conformance/seams.test.mjs b/tests/node-conformance/seams.test.mjs index ba8f420..1ddc013 100644 --- a/tests/node-conformance/seams.test.mjs +++ b/tests/node-conformance/seams.test.mjs @@ -12,7 +12,7 @@ // 20.3.0. 20.3.0 is the first release carrying both. import assert from 'node:assert/strict'; import {describe, it} from 'node:test'; -import {composeSignal, isTimeoutSignal} from '@dexpace/core'; +import {composeSignal, isTimeoutSignal, RequestOptions} from '@dexpace/core'; describe('composeSignal on the declared Node floor', () => { it('returns a distinct AbortSignal.any() result when both a signal and a timeout are supplied', () => { @@ -101,3 +101,37 @@ describe('Web Crypto on the declared Node floor', () => { ); }); }); + +describe('composeSignal timeout range on Node (HTTP-35)', () => { + // Runtime-divergent by measurement, 2026-09-05: `AbortSignal.timeout(1.5)` and + // `AbortSignal.timeout(2 ** 32)` raise `RangeError` on Node and are accepted on Bun, and a + // negative delay raises `RangeError` on Node against `TypeError` on Bun. `bun test` therefore + // cannot assert either half of this, which is what puts the case here rather than only in + // `packages/core/src/seams/transport.test.ts`. Added by audit #67 / #76, which moved the range + // check onto `RequestOptionsBuilder.timeoutMs` for this reason. + it('accepts every timeout RequestOptionsBuilder accepts, at both ends of the range', () => { + for (const value of [1, 1000, 2 ** 32 - 1]) { + const accepted = RequestOptions.newBuilder() + .timeoutMs(value) + .build().timeoutMs; + assert.equal(accepted, value); + assert.ok( + composeSignal(undefined, accepted) instanceof AbortSignal, + `composeSignal must accept the timeout ${value}, which the model admits`, + ); + } + }); + + it('would raise RangeError on the values the model now rejects', () => { + for (const value of [1.5, 2 ** 32]) { + assert.throws( + () => composeSignal(undefined, value), + RangeError, + `Node's AbortSignal.timeout() must still reject ${value}; the model is what keeps it unreachable`, + ); + assert.throws(() => RequestOptions.newBuilder().timeoutMs(value), { + name: 'RequestOptionsValidationError', + }); + } + }); +});