fix!: reject non-ASCII headers at the model layer and resolve pagination follow-ups (#207)

PR: #207

Author: OmarAlJarrah

docs/implementation-plan.md

@@ -375,8 +375,8 @@ defaults (per Square: `FAIL_ON_UNKNOWN_PROPERTIES=false`, `WRITE_DATES_AS_TIMEST
 > Superseded by #30 (pagination unification): `Page` now exposes the raw per-page `Response` and is `Closeable` (materialized `items` and derived `statusCode` / `headers` / `request` survive `close()`), strategies return `PageInfo` (`nextRequest == null` = end of stream), and `SimplePage` was removed.
 
 **Status: shipped.** `Page`, `Paginator`, `PaginationStrategy`, and the three strategies
-(`Cursor` / `PageNumber` / `LinkHeader`) are in `sdk-core/.../pagination`, alongside
-helper types `SimplePage` and `RequestRebuilder`. `Paginator` gained a `maxPages` safety cap
+(`Cursor` / `PageNumber` / `LinkHeader`) are in `sdk-core/.../pagination`, alongside the
+helper type `RequestRebuilder`. `Paginator` gained a `maxPages` safety cap
 (default `Long.MAX_VALUE`) beyond the original sketch, to bound runaway iteration against servers
 that never advance their cursor.
 

docs/refs-comparison.md

@@ -230,7 +230,7 @@ below records where each scheme's design was sourced from:
 ### Pagination
 
 `Paginator<T>` + `Page<T>` ship in `sdk-core`, driven by a `PaginationStrategy` (cursor,
-page-number, token, link-header), and `pagination.PagedIterable` wraps the result. Reference
+page-number, link-header), and `pagination.PagedIterable` wraps the result. Reference
 designs we drew on:
 
 - **Square**: `SyncPagingIterable<T>` (`Iterable<T>` lazy iterator), `SyncPage<T>` (per-page holder), `BiDirectionalPage<T>` (forward + backward cursors), `CustomPager<T>` (user-implementation stub for HATEOAS).
@@ -240,7 +240,7 @@ designs we drew on:
 **What shipped, and what's left:**
 
 1. `Paginator<T>` and `Page<T>` are in `sdk-core`. `iterateAll()` returns a lazy `Iterable<T>`; `streamAll()` returns a Java 8 `Stream<T>`. Each call hands back an independent iterator with its own state.
-2. The strategy is injected via `PaginationStrategy`, with concrete impls covering cursor (`next_cursor` / `prev_cursor`), page-number, token, and link-header (RFC 8288). A `maxPages` cap guards against servers that never advance their cursor.
+2. The strategy is injected via `PaginationStrategy`, with three concrete impls: cursor (`CursorPaginationStrategy`, forward-only), page-number, and link-header (RFC 8288). Token-style APIs (`next_page_token`, `pageToken`, …) reuse `CursorPaginationStrategy` with a configurable query-param name, so no separate token strategy ships (see `architecture.md`). A `maxPages` cap guards against servers that never advance their cursor.
 3. Async variants for `sdk-async-coroutines` (`Flow<T>`) and `sdk-async-reactor` (`Flux<T>`) are not yet built.
 4. `BiDirectionalPage` is deferred until a real API needs it; Square's pattern is good when needed.
 
@@ -333,7 +333,7 @@ adapters where noted):
 - **Idempotency-key step.** Auto-injects `Idempotency-Key: UUID.randomUUID()` for `POST`/`PUT`/`PATCH`; caller-set header wins; pluggable key strategy. [`pipeline/step/IdempotencyKeyStep.kt`]
 - **Auth.** `Credential` family + RFC 7235 challenge parsing + Basic/Digest/Composite `ChallengeHandler`s + `AuthStep` pillar. [`auth/`, `http/pipeline/steps/`]
 - **`sdk-serde-jackson` adapter.** Kotlin + JSR-310 + Jdk8 modules; `FAIL_ON_UNKNOWN_PROPERTIES` and `WRITE_DATES_AS_TIMESTAMPS` disabled; `Tristate<T>` via `TristateModule`.
-- **Pagination primitives.** `Paginator<T>` + `Page<T>` + `PaginationStrategy` (cursor / page-number / token / link-header) with a `maxPages` cap; `PagedIterable` wrapper.
+- **Pagination primitives.** `Paginator<T>` + `Page<T>` + `PaginationStrategy` (cursor / page-number / link-header) with a `maxPages` cap; `PagedIterable` wrapper.
 - **Client identity header.** `ClientIdentityStep` building the `dexpace-sdk/<ver> jvm/<javaver>` token line.
 - **Tracer event vocabulary + metrics seam.** `HttpTracer` with named retry/request/response events; `Meter`/`LongCounter`/`DoubleHistogram` separate from tracing.
 - **SSE streaming.** WHATWG reader in `sdk-core`; backpressured `Flux<ServerSentEvent>` in `sdk-async-reactor`.

sdk-core/api/sdk-core.api

@@ -406,6 +406,7 @@ public final class org/dexpace/sdk/core/http/common/Headers$Builder {
 	public final fun add (Lorg/dexpace/sdk/core/http/common/HttpHeaderName;Ljava/lang/String;)Lorg/dexpace/sdk/core/http/common/Headers$Builder;
 	public final fun add (Lorg/dexpace/sdk/core/http/common/HttpHeaderName;Ljava/util/List;)Lorg/dexpace/sdk/core/http/common/Headers$Builder;
 	public final fun addAll (Lorg/dexpace/sdk/core/http/common/Headers;)Lorg/dexpace/sdk/core/http/common/Headers$Builder;
+	public final fun addUnsafeNonAscii (Ljava/lang/String;Ljava/lang/String;)Lorg/dexpace/sdk/core/http/common/Headers$Builder;
 	public final fun build ()Lorg/dexpace/sdk/core/http/common/Headers;
 	public final fun remove (Ljava/lang/String;)Lorg/dexpace/sdk/core/http/common/Headers$Builder;
 	public final fun remove (Lorg/dexpace/sdk/core/http/common/HttpHeaderName;)Lorg/dexpace/sdk/core/http/common/Headers$Builder;
@@ -1831,6 +1832,15 @@ public final class org/dexpace/sdk/core/instrumentation/ClientLogger {
 public final class org/dexpace/sdk/core/instrumentation/ClientLogger$Companion {
 }
 
+public final class org/dexpace/sdk/core/instrumentation/DroppedHeaderLogging : java/lang/Enum {
+	public static final field ONCE_PER_HEADER Lorg/dexpace/sdk/core/instrumentation/DroppedHeaderLogging;
+	public static final field PER_OCCURRENCE Lorg/dexpace/sdk/core/instrumentation/DroppedHeaderLogging;
+	public static final field VERBOSE_ONLY Lorg/dexpace/sdk/core/instrumentation/DroppedHeaderLogging;
+	public static fun getEntries ()Lkotlin/enums/EnumEntries;
+	public static fun valueOf (Ljava/lang/String;)Lorg/dexpace/sdk/core/instrumentation/DroppedHeaderLogging;
+	public static fun values ()[Lorg/dexpace/sdk/core/instrumentation/DroppedHeaderLogging;
+}
+
 public abstract interface class org/dexpace/sdk/core/instrumentation/HttpTracer {
 	public fun attemptFailed (Ljava/lang/Throwable;Ljava/lang/Long;)V
 	public fun attemptRetriesExhausted (Ljava/lang/Throwable;)V

sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HeaderValidation.kt

@@ -28,29 +28,33 @@ package org.dexpace.sdk.core.http.common
  *    and `0x7F`), which covers CR, LF, and NUL. An embedded `\r`/`\n` is the same
  *    request/header-splitting vector guarded against for header values: once the name is
  *    serialised an attacker could inject a new header or a second request. A NUL or other control
- *    character is illegal in a field-name, and the two reference transports handle it differently at
- *    their raw API (OkHttp's `addHeader` throws unchecked, the JDK builder drops it); their adapters
- *    now catch and drop uniformly, but a splitting vector should never get that far. Validating here
- *    rejects it loudly at construction — fast, uniform, and transport-independent.
+ *    character is illegal in a field-name; validating here rejects it loudly at construction —
+ *    fast, uniform, and transport-independent.
+ *  - **Any non-ASCII byte** (code point `>= 0x80`). RFC 7230 field-names are ASCII `token`s, and
+ *    every shipped reference transport (OkHttp, the JDK `HttpClient`) rejects a non-ASCII name at
+ *    its own model layer, so a name no transport can put on the wire is refused here at
+ *    construction rather than accepted by the model and then silently dropped mid-dispatch.
  *
- * Policy: the control-character set is intentionally narrower than RFC 7230's full `tchar`
- * allow-list — restricting names to `tchar` would reject some non-ASCII names that certain
- * transports accept, whereas the control-character set is illegal everywhere and covers the
- * splitting/injection surface. This mirrors the conservative stance taken for values in
+ * Policy: the accepted set is printable ASCII (`0x20`–`0x7E`) — still wider than RFC 7230's
+ * `tchar`, so a name a transport's stricter grammar rejects (e.g. an interior space) is left for
+ * the transport to drop rather than pre-judged here. Control characters and non-ASCII bytes, which
+ * no transport can encode, are rejected outright. This mirrors the stance taken for values in
  * [requireValidHeaderValues].
  *
  * @return the trimmed, validated name
- * @throws IllegalArgumentException if the trimmed name is blank or contains a control character
+ * @throws IllegalArgumentException if the trimmed name is blank, or contains a control character or
+ *   a non-ASCII byte
  */
 @JvmSynthetic
 internal fun requireValidHeaderName(rawName: String): String {
     val trimmed = rawName.trim()
     require(trimmed.isNotEmpty()) { "Header name must not be blank." }
     trimmed.forEach { ch ->
         require(!isProhibitedInName(ch.code)) {
-            "Header name '${escapeControlCharacters(rawName)}' must not contain control characters " +
-                "(carriage return, line feed, NUL, or other C0/DEL bytes); " +
-                "such characters enable request/header splitting."
+            "Header name '${escapeControlCharacters(rawName)}' must be printable ASCII: it must not " +
+                "contain control characters (carriage return, line feed, NUL, or other C0/DEL bytes) " +
+                "or non-ASCII bytes; control characters enable request/header splitting, and no " +
+                "reference transport can encode a non-ASCII name."
         }
     }
     return trimmed
@@ -70,12 +74,19 @@ internal fun requireValidHeaderName(rawName: String): String {
  * broader control-character set closes the same splitting/injection surface the name check does
  * while staying narrower than the strict field-value grammar.
  *
- * Non-ASCII (for example UTF-8) bytes are NOT rejected — that is the conservative stance shared
- * with the name check: a value some transports accept is not refused at the model layer. [name]
- * only labels the error message; the value itself is never echoed, so a secret or oversized value
- * is not leaked into a log line.
+ * Non-ASCII bytes (code point `>= 0x80`) are rejected as well, matching OkHttp's field-value
+ * grammar (HTAB plus printable ASCII `0x20`–`0x7E`). The shipped transports are not unanimous here
+ * — the JDK `HttpClient` will serialise obs-text (`0x80`+) on the wire — so the model deliberately
+ * takes the *most restrictive* shipped transport's stance: a value it accepts is sendable by every
+ * shipped transport, and a value only some can encode is refused at construction rather than
+ * silently dropped by the stricter one mid-dispatch. This governs **outbound, caller-set** values;
+ * a header parsed from an already-received response takes the lenient
+ * [requireValidInboundHeaderValue] path (via `Headers.Builder.addUnsafeNonAscii`), which preserves
+ * the obs-text a server may legitimately send. [name] only labels the error message; the value
+ * itself is never echoed, so a secret or oversized value is not leaked into a log line.
  *
- * @throws IllegalArgumentException if any value contains a prohibited control character
+ * @throws IllegalArgumentException if any value contains a prohibited control character or a
+ *   non-ASCII byte
  */
 @JvmSynthetic
 internal fun requireValidHeaderValues(
@@ -85,22 +96,61 @@ internal fun requireValidHeaderValues(
     values.forEach { value ->
         value.forEach { ch ->
             require(!isProhibitedInValue(ch.code)) {
-                "Header value for '$name' must not contain control characters (carriage return, " +
-                    "line feed, NUL, or other C0/DEL bytes, except horizontal tab); " +
-                    "such characters enable request/header splitting."
+                "Header value for '$name' must be ASCII: it must not contain control characters " +
+                    "(carriage return, line feed, NUL, or other C0/DEL bytes, except horizontal tab) " +
+                    "or non-ASCII bytes; control characters enable request/header splitting, and " +
+                    "OkHttp's field-value grammar rejects a non-ASCII value."
             }
         }
     }
 }
 
-/** Whether [code] is a control character prohibited in a header name — the full C0 range and DEL. */
-private fun isProhibitedInName(code: Int): Boolean = code <= LAST_C0_CONTROL || code == DEL_CONTROL
+/**
+ * Validates the [value] of an *inbound* (response) header [name] leniently. Like OkHttp's
+ * `addUnsafeNonAscii`/`addLenient` read path, the ASCII restriction is dropped so **non-ASCII bytes
+ * are permitted**; unlike that path — which does no value validation at all — control characters are
+ * still rejected here as defence-in-depth against a misbehaving transport (a stray CR/LF/NUL has no
+ * place in a parsed field-value). RFC 7230 explicitly allows obs-text (`0x80`+) in field-content,
+ * and a server legitimately puts Latin-1 bytes in values such as a `Content-Disposition` filename;
+ * applying the stricter outbound grammar of [requireValidHeaderValues] to a response would silently
+ * strip those headers. [name] only labels the error message; the value itself is never echoed.
+ *
+ * @throws IllegalArgumentException if [value] contains a prohibited control character
+ */
+@JvmSynthetic
+internal fun requireValidInboundHeaderValue(
+    name: String,
+    value: String,
+) {
+    value.forEach { ch ->
+        require(!isProhibitedInInboundValue(ch.code)) {
+            "Inbound header value for '$name' must not contain control characters (carriage " +
+                "return, line feed, NUL, or other C0/DEL bytes, except horizontal tab)."
+        }
+    }
+}
 
 /**
- * Whether [code] is a control character prohibited in a header value — the same set as for a name,
- * minus horizontal tab (`0x09`), which RFC 7230 permits as field-value whitespace.
+ * Whether [code] is prohibited in a header name — the full C0 range, DEL, and every non-ASCII byte
+ * (`>= 0x80`). `code >= DEL_CONTROL` covers DEL (`0x7F`) and all of `0x80`+ in one comparison.
  */
-private fun isProhibitedInValue(code: Int): Boolean =
+private fun isProhibitedInName(code: Int): Boolean = code <= LAST_C0_CONTROL || code >= DEL_CONTROL
+
+/**
+ * Whether [code] is prohibited in an outbound header value — the same set as for a name, minus
+ * horizontal tab (`0x09`), which RFC 7230 permits as field-value whitespace. Shared with [MediaType]
+ * so a media type's rendered form and the header value it becomes agree on the accepted byte set.
+ */
+@JvmSynthetic
+internal fun isProhibitedInValue(code: Int): Boolean =
+    (code <= LAST_C0_CONTROL && code != HORIZONTAL_TAB) || code >= DEL_CONTROL
+
+/**
+ * Whether [code] is prohibited in an *inbound* header value: control characters (`0x00`–`0x1F`
+ * except HTAB, plus DEL `0x7F`) are still rejected, but non-ASCII bytes (`0x80`+) are permitted —
+ * the obs-text a server may legitimately send. See [requireValidInboundHeaderValue].
+ */
+private fun isProhibitedInInboundValue(code: Int): Boolean =
     (code <= LAST_C0_CONTROL && code != HORIZONTAL_TAB) || code == DEL_CONTROL
 
 /**
@@ -126,7 +176,12 @@ private const val HORIZONTAL_TAB: Int = 0x09
 /** Highest code point in the C0 control range (US, `0x1F`); everything at or below is illegal in a name. */
 private const val LAST_C0_CONTROL: Int = 0x1F
 
-/** The DEL control character (`0x7F`), the lone control code above the C0 range. */
+/**
+ * The DEL control character (`0x7F`). Used as the low bound of the rejected high range: for a name
+ * and an outbound value, DEL and every byte above it (the non-ASCII bytes, `0x80`+) are prohibited
+ * (`code >= DEL_CONTROL`); an inbound value rejects only DEL itself (`code == DEL_CONTROL`),
+ * permitting obs-text.
+ */
 private const val DEL_CONTROL: Int = 0x7F
 
 /** Radix for rendering a control character's code point as the hex digits of a `\uXXXX` escape. */

sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt

@@ -159,6 +159,36 @@ public data class Headers private constructor(
                 headersMap.computeIfAbsent(canonicalKey(trimmedName)) { mutableListOf() }.addAll(values)
             }
 
+        /**
+         * Adds an inbound (already-received) header, permitting non-ASCII (obs-text) bytes in
+         * [value] that the strict outbound [add] grammar rejects.
+         *
+         * A transport uses this when copying a *response*'s headers into the model. RFC 7230 allows
+         * obs-text (`0x80`+) in a field-value — a server legitimately puts Latin-1 bytes in values
+         * such as a `Content-Disposition` filename — and both reference transports surface such
+         * values on read, so validating them with the outbound grammar would silently drop
+         * legitimate response headers. The name is still validated strictly (control and non-ASCII
+         * bytes rejected); in the value, only the ASCII restriction is relaxed — control characters
+         * are still rejected. Like OkHttp's `addUnsafeNonAscii` this relaxes the value's ASCII
+         * restriction; unlike it (whose `addLenient` validates nothing on the value), the
+         * control-character guard is kept here as defence-in-depth.
+         *
+         * @param name the header name
+         * @param value the header value, which may contain non-ASCII bytes
+         * @return this builder
+         * @throws IllegalArgumentException if [name] is blank or contains a control/non-ASCII byte,
+         *   or [value] contains a prohibited control character
+         */
+        public fun addUnsafeNonAscii(
+            name: String,
+            value: String,
+        ): Builder =
+            apply {
+                val trimmedName = requireValidHeaderName(name)
+                requireValidInboundHeaderValue(trimmedName, value)
+                headersMap.computeIfAbsent(canonicalKey(trimmedName)) { mutableListOf() }.add(value)
+            }
+
         /**
          * Adds a header with the specified typed name and value.
          */

sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HttpHeaderName.kt

@@ -24,8 +24,8 @@ import java.util.concurrent.ConcurrentHashMap
  * first caller to intern a given name "wins"; subsequent lookups with different casing
  * yield the same shared instance.
  *
- * Whitespace is trimmed from the input before interning, and the name is validated: a blank name
- * or one carrying an interior control character is rejected (see [fromString]).
+ * Whitespace is trimmed from the input before interning, and the name is validated: a blank name,
+ * or one carrying an interior control character or a non-ASCII byte, is rejected (see [fromString]).
  *
  * Designed for Java 8 bytecode compatibility — no APIs newer than Java 8 are used.
  */
@@ -220,12 +220,14 @@ public class HttpHeaderName private constructor(
          *
          * The name is validated up front by [requireValidHeaderName]: a blank name, or one whose
          * trimmed form contains an interior control character (CR, LF, NUL, or any other C0/DEL
-         * byte), is rejected with an [IllegalArgumentException]. This is the same guard the
-         * String-keyed [Headers.Builder] API applies, so an interned name carried through the typed
-         * header API is guaranteed control-character-free and cannot reach a transport as a
-         * header-splitting vector.
+         * byte) or a non-ASCII byte (code point >= 0x80), is rejected with an
+         * [IllegalArgumentException]. This is the same guard the String-keyed [Headers.Builder] API
+         * applies, so an interned name carried through the typed header API is guaranteed to be
+         * printable ASCII — control-character-free (it cannot reach a transport as a header-splitting
+         * vector) and free of the non-ASCII bytes no reference transport can encode.
          *
-         * @throws IllegalArgumentException if [name] is blank or contains a control character
+         * @throws IllegalArgumentException if [name] is blank, or contains a control character or a
+         *   non-ASCII byte
          */
         @JvmStatic
         public fun fromString(name: String): HttpHeaderName {