From cecccdfa063898a8e35493de7464bba4348006d8 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Wed, 17 Jun 2026 04:52:47 +0300 Subject: [PATCH 1/4] fix: reject control characters in header names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Headers.Builder validated header values for CR/LF but never validated header names: name handling only lower-cased and trimmed surrounding whitespace, so an interior \r, \n, NUL, or other control character survived into the model layer. Downstream the two reference transports diverged on such a name — the OkHttp transport threw an unchecked IllegalArgumentException out of execute() (escaping its IOException contract and bypassing the retry pipeline), while the JDK transport silently dropped the header. It was also a header-injection surface for attacker-controlled names. Add validateName beside validateValues, invoked from the String-based add/set before the name is stored. It rejects blank names and any character in the C0 control range (0x00-0x1F, covering CR/LF/NUL) or DEL (0x7F). The typed (HttpHeaderName) overloads already carry pre-validated token names, so they are unaffected. The policy deliberately stops at control characters rather than RFC 7230's full tchar allow-list, so it does not reject non-ASCII names that some transports accept while still closing the splitting/injection surface. Correct the OkHttp and JDK transport-adapter comments that previously claimed names were validated upstream; they now describe the actual name and value guarantees. --- .../dexpace/sdk/core/http/common/Headers.kt | 40 ++++++++ .../sdk/core/http/common/HeadersTest.kt | 97 +++++++++++++++++++ .../jdkhttp/internal/RequestAdapter.kt | 9 +- .../okhttp/internal/RequestAdapter.kt | 7 +- 4 files changed, 146 insertions(+), 7 deletions(-) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt index 85ef3af8..ceeb7aee 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt @@ -160,6 +160,7 @@ public data class Headers private constructor( values: List, ): Builder = apply { + validateName(name) validateValues(name, values) headersMap.computeIfAbsent(sanitizeName(name)) { mutableListOf() }.addAll(values) } @@ -218,6 +219,7 @@ public data class Headers private constructor( values: List, ): Builder = apply { + validateName(name) validateValues(name, values) headersMap[sanitizeName(name)] = values.toMutableList() } @@ -333,5 +335,43 @@ public data class Headers private constructor( } } } + + /** + * Rejects header names that cannot legally appear on the wire before they reach a transport. + * + * An embedded carriage return (`\r`) or line feed (`\n`) in a name is the same + * request/header-splitting vector guarded against for values: once the name is serialised an + * attacker could inject a new header or a second request. A NUL or any other ASCII control + * character is likewise illegal in an RFC 7230 field-name (`token`) and is rejected by — or + * silently dropped at — the transport layer, so the two reference transports diverge (OkHttp + * throws unchecked, the JDK transport drops the header) when such a name slips through. + * Validating here at the transport-agnostic model layer fails fast and uniformly. + * + * Note [sanitizeName] trims surrounding whitespace and lower-cases, but it never removes an + * *interior* control character, so this check is the only thing standing between a malformed + * name and the transport. A blank name has no canonical form and is rejected as well. + * + * Policy: reject the C0 control range and DEL (code points `0x00`-`0x1F` and `0x7F`), which + * covers CR, LF, and NUL. This is intentionally narrower than RFC 7230's full `tchar` + * allow-list — restricting names to `tchar` only would reject some non-ASCII names that + * certain transports accept, whereas the control-character set is illegal everywhere and + * covers the splitting/injection surface. + */ + private fun validateName(name: String) { + require(name.isNotBlank()) { "Header name must not be blank." } + name.forEach { ch -> + require(ch.code > LAST_C0_CONTROL && ch.code != DEL_CONTROL) { + "Header name '$name' must not contain control characters " + + "(carriage return, line feed, NUL, or other C0/DEL bytes); " + + "such characters enable request/header splitting." + } + } + } + + /** Highest code point in the C0 control range (US, `0x1F`); everything at or below is illegal. */ + private const val LAST_C0_CONTROL: Int = 0x1F + + /** The DEL control character (`0x7F`), the lone control code above the C0 range. */ + private const val DEL_CONTROL: Int = 0x7F } } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt index 72e5b2be..522c1069 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt @@ -258,6 +258,103 @@ class HeadersTest { assertNull(headers.get("X-Trace-Id")) } + // ---- name validation (request/header-splitting guard) ----------------------- + + @Test + fun `add rejects a name containing a line feed`() { + assertFailsWith { + Headers.builder().add("X-Evil\nInjected", "v") + } + } + + @Test + fun `add rejects a name containing a carriage return`() { + assertFailsWith { + Headers.builder().add("X-Evil\rInjected", "v") + } + } + + @Test + fun `add rejects a name containing CRLF`() { + assertFailsWith { + Headers.builder().add("X-Evil\r\nInjected: 1", "v") + } + } + + @Test + fun `add rejects a name containing a NUL`() { + assertFailsWith { + Headers.builder().add("X-Evil\u0000Injected", "v") + } + } + + @Test + fun `add rejects a name containing a DEL control character`() { + assertFailsWith { + Headers.builder().add("X-Evil\u007FInjected", "v") + } + } + + @Test + fun `add list overload rejects a name containing CR or LF`() { + assertFailsWith { + Headers.builder().add("X-Evil\nInjected", listOf("v")) + } + } + + @Test + fun `set rejects a name containing a line feed`() { + assertFailsWith { + Headers.builder().set("X-Evil\nInjected", "v") + } + } + + @Test + fun `set rejects a name containing a carriage return`() { + assertFailsWith { + Headers.builder().set("X-Evil\rInjected", "v") + } + } + + @Test + fun `set list overload rejects a name containing a NUL`() { + assertFailsWith { + Headers.builder().set("X-Evil\u0000Injected", listOf("v")) + } + } + + @Test + fun `add rejects a blank name`() { + assertFailsWith { + Headers.builder().add(" ", "v") + } + } + + @Test + fun `the name rejection message names the offending header`() { + val thrown = + assertFailsWith { + Headers.builder().add("X-Trace-Id\nInjected", "v") + } + assertTrue( + thrown.message?.lowercase()?.contains("x-trace-id") == true, + "message should name the header, got: ${thrown.message}", + ) + } + + @Test + fun `normal names without control characters are accepted`() { + val headers = + Headers.builder() + .add("X-Plain", "a") + // Surrounding whitespace is trimmed by name normalisation, not rejected. + .set(" Authorization ", "Bearer t") + .build() + + assertEquals("a", headers.get("X-Plain")) + assertEquals("Bearer t", headers.get("Authorization")) + } + // ---- accessors & equality coverage ------------------------------------------ @Test diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt index 032886d2..1c17bcfb 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt @@ -102,10 +102,11 @@ internal class RequestAdapter( * `IllegalArgumentException` escape [adapt] (and therefore `execute`, declared * `@Throws(IOException)`) where a caller's `catch(IOException)` would not observe it. * - * Note this catch guards against the JDK's restricted *name* set only. Illegal header - * *values* (CR/LF and similar) are now rejected upstream by `Headers.Builder`, so a value - * with control characters never reaches this point — the `IllegalArgumentException` handled - * here is the JDK refusing a restricted name, not a malformed value. + * Note this catch guards against the JDK's restricted *name* set only. Malformed header names + * (CR/LF, NUL, other control characters) and illegal header values (CR/LF) are rejected + * upstream by `Headers.Builder`, so neither a control-character name nor such a value reaches + * this point — the `IllegalArgumentException` handled here is the JDK refusing a *restricted* + * (but otherwise well-formed) name, not a malformed name or value. */ private fun attachHeaders( builder: HttpRequest.Builder, diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt index 88af552b..b7be9172 100644 --- a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt @@ -51,9 +51,10 @@ internal class RequestAdapter( continue } for (value in values) { - // Header names/values are validated upstream by Headers.Builder (CR/LF and other - // illegal characters are rejected at construction), so addHeader receives only - // well-formed values here. + // Header names and values are validated upstream by Headers.Builder: names reject + // control characters (CR/LF, NUL, the rest of the C0/DEL range) and values reject + // CR/LF, so addHeader receives only well-formed input here and never throws on a + // malformed name or value. builder.addHeader(rawName, value) } } From 9c084b2b7aeba557e807e3cdfb0a63c2bf0bbeea Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Wed, 24 Jun 2026 08:09:18 +0300 Subject: [PATCH 2/4] fix: validate header names on the typed API and harden transport header adaptation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Header-name validation previously lived only on the String-keyed Headers.Builder path and checked the raw, untrimmed name. HttpHeaderName.fromString — the typed header API — performed no name validation at all, so a control-character name could be interned and handed to a transport as a request/header-splitting vector. - Extract the name check into a shared requireValidHeaderName, now invoked by both Headers.Builder and HttpHeaderName.fromString. It validates the trimmed name (surrounding HTTP whitespace is stripped and harmless; an interior C0/DEL control byte is rejected) and rejects blank names. fromString now rejects a blank or control-character name instead of interning it. - OkHttp's addHeader rejects any non-printable-ASCII byte, so a model-valid non-ASCII (for example UTF-8) name or value would throw an unchecked IllegalArgumentException out of the synchronous execute(), past its @Throws(IOException) contract. Catch it per header and drop the offending entry, mirroring the JDK transport adapter, and correct the adapter comments that claimed addHeader never throws. - Render control characters as a unicode escape in the rejection message rather than echoing the raw bytes. No public API change. Adds coverage for the typed-API rejection, the C0/DEL predicate boundary, the rejection-message escaping, and the OkHttp and JDK drop-not-throw paths (sync and async). --- .../core/http/common/HeaderNameValidation.kt | 79 +++++++++++++++++++ .../dexpace/sdk/core/http/common/Headers.kt | 42 +--------- .../sdk/core/http/common/HttpHeaderName.kt | 13 ++- .../sdk/core/http/common/HeadersTest.kt | 49 ++++++++++++ .../core/http/common/HttpHeaderNameTest.kt | 27 +++++-- .../jdkhttp/internal/RequestAdapter.kt | 13 +-- .../transport/jdkhttp/JdkHttpTransportTest.kt | 26 ++++++ .../okhttp/internal/RequestAdapter.kt | 23 ++++-- .../transport/okhttp/OkHttpTransportTest.kt | 50 ++++++++++++ 9 files changed, 263 insertions(+), 59 deletions(-) create mode 100644 sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HeaderNameValidation.kt diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HeaderNameValidation.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HeaderNameValidation.kt new file mode 100644 index 00000000..89e31a2b --- /dev/null +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HeaderNameValidation.kt @@ -0,0 +1,79 @@ +/* + * Copyright (c) 2026 dexpace and Omar Aljarrah + * + * Licensed under the MIT License. See LICENSE in the project root. + * SPDX-License-Identifier: MIT + */ + +package org.dexpace.sdk.core.http.common + +/** + * Validates an HTTP header name at the transport-agnostic model layer. Shared by the + * String-keyed [Headers.Builder] API and the typed [HttpHeaderName.fromString] entry point so a + * malformed name cannot slip through either one — the two were previously inconsistent (only the + * String API validated, and only against the raw input). + * + * The check runs on the **trimmed** name, matching the trimming both call sites apply before + * storing or interning. `String.trim()` strips only the surrounding *whitespace* code points — + * space, tab, CR, LF, and the C0 information separators (`0x1C`–`0x1F`) — so a leading or trailing + * one of those is removed before it could reach the wire and is harmless. Every other control byte + * survives the trim: a surrounding NUL or DEL, like any *interior* control character, is rejected. + * What is rejected: + * + * - **A blank name.** A field-name must be a non-empty RFC 7230 `token`; an empty or + * all-whitespace name has no canonical form. + * - **Any interior control character** — the C0 control range and DEL (code points `0x00`–`0x1F` + * 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. + * + * 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 CR/LF-only stance taken for values. + */ +@JvmSynthetic +internal fun requireValidHeaderName(rawName: String) { + val trimmed = rawName.trim() + require(trimmed.isNotEmpty()) { "Header name must not be blank." } + trimmed.forEach { ch -> + require(ch.code > LAST_C0_CONTROL && ch.code != DEL_CONTROL) { + "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." + } + } +} + +/** + * Renders [name] for an error message with every control character replaced by its `\uXXXX` + * escape, so a raw CR/LF/NUL from the rejected name never lands verbatim in a log line while the + * printable portion still identifies the offending header. + */ +private fun escapeControlCharacters(name: String): String = + buildString { + name.forEach { ch -> + if (ch.code <= LAST_C0_CONTROL || ch.code == DEL_CONTROL) { + append("\\u") + append(ch.code.toString(HEX_RADIX).padStart(ESCAPE_HEX_WIDTH, '0')) + } else { + append(ch) + } + } + } + +/** 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. */ +private const val DEL_CONTROL: Int = 0x7F + +/** Radix for rendering a control character's code point as the hex digits of a `\uXXXX` escape. */ +private const val HEX_RADIX: Int = 16 + +/** Zero-padded width of a `\uXXXX` escape's hex digits. */ +private const val ESCAPE_HEX_WIDTH: Int = 4 diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt index ceeb7aee..836df400 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt @@ -160,7 +160,7 @@ public data class Headers private constructor( values: List, ): Builder = apply { - validateName(name) + requireValidHeaderName(name) validateValues(name, values) headersMap.computeIfAbsent(sanitizeName(name)) { mutableListOf() }.addAll(values) } @@ -219,7 +219,7 @@ public data class Headers private constructor( values: List, ): Builder = apply { - validateName(name) + requireValidHeaderName(name) validateValues(name, values) headersMap[sanitizeName(name)] = values.toMutableList() } @@ -335,43 +335,5 @@ public data class Headers private constructor( } } } - - /** - * Rejects header names that cannot legally appear on the wire before they reach a transport. - * - * An embedded carriage return (`\r`) or line feed (`\n`) in a name is the same - * request/header-splitting vector guarded against for values: once the name is serialised an - * attacker could inject a new header or a second request. A NUL or any other ASCII control - * character is likewise illegal in an RFC 7230 field-name (`token`) and is rejected by — or - * silently dropped at — the transport layer, so the two reference transports diverge (OkHttp - * throws unchecked, the JDK transport drops the header) when such a name slips through. - * Validating here at the transport-agnostic model layer fails fast and uniformly. - * - * Note [sanitizeName] trims surrounding whitespace and lower-cases, but it never removes an - * *interior* control character, so this check is the only thing standing between a malformed - * name and the transport. A blank name has no canonical form and is rejected as well. - * - * Policy: reject the C0 control range and DEL (code points `0x00`-`0x1F` and `0x7F`), which - * covers CR, LF, and NUL. This is intentionally narrower than RFC 7230's full `tchar` - * allow-list — restricting names to `tchar` only would reject some non-ASCII names that - * certain transports accept, whereas the control-character set is illegal everywhere and - * covers the splitting/injection surface. - */ - private fun validateName(name: String) { - require(name.isNotBlank()) { "Header name must not be blank." } - name.forEach { ch -> - require(ch.code > LAST_C0_CONTROL && ch.code != DEL_CONTROL) { - "Header name '$name' must not contain control characters " + - "(carriage return, line feed, NUL, or other C0/DEL bytes); " + - "such characters enable request/header splitting." - } - } - } - - /** Highest code point in the C0 control range (US, `0x1F`); everything at or below is illegal. */ - private const val LAST_C0_CONTROL: Int = 0x1F - - /** The DEL control character (`0x7F`), the lone control code above the C0 range. */ - private const val DEL_CONTROL: Int = 0x7F } } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HttpHeaderName.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HttpHeaderName.kt index 9db9e410..56eb0403 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HttpHeaderName.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HttpHeaderName.kt @@ -24,7 +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. + * 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]). * * Designed for Java 8 bytecode compatibility — no APIs newer than Java 8 are used. */ @@ -216,9 +217,19 @@ public class HttpHeaderName private constructor( * lower-case (US locale) for the interning key. The case-preserved form of the * first caller to intern a given key wins; subsequent calls with different casing * yield the same shared instance. + * + * 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. + * + * @throws IllegalArgumentException if [name] is blank or contains a control character */ @JvmStatic public fun fromString(name: String): HttpHeaderName { + requireValidHeaderName(name) val trimmed = name.trim() val key = trimmed.lowercase(Locale.US) // computeIfAbsent is available on Java 8. diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt index 522c1069..7182c0a8 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt @@ -355,6 +355,55 @@ class HeadersTest { assertEquals("Bearer t", headers.get("Authorization")) } + @Test + fun `a surrounding-whitespace control character is trimmed, not rejected`() { + // Validation runs on the trimmed name: a leading tab or trailing line feed is stripped + // before it could reach the wire, so it is harmless. Only an interior control character + // is a splitting vector. Built without escape literals to keep the bytes unambiguous. + val tab = 9.toChar() + val lf = 10.toChar() + val headers = + Headers.builder() + .add(tab + "X-Foo" + lf, "v") + .build() + + assertEquals("v", headers.get("X-Foo")) + } + + @Test + fun `the name rejection message escapes control characters instead of echoing them`() { + val lf = 10.toChar() + val thrown = + assertFailsWith { + Headers.builder().add("X-Trace-Id" + lf + "Injected", "v") + } + val message = thrown.message ?: "" + assertFalse(message.contains(lf), "raw control character must not appear in the message") + assertTrue(message.contains("X-Trace-Id"), "message should still name the offending header") + val backslash = 92.toChar() + assertTrue( + message.contains(backslash + "u000a"), + "control character should be rendered as a \\uXXXX escape, got: $message", + ) + } + + @Test + fun `name validation rejects interior control bytes through 0x1F and DEL but accepts space`() { + // Pin the predicate boundary the shared validator introduces: an interior byte in the C0 + // range (up to and including 0x1F) and DEL (0x7F) is rejected, while 0x20 (space) — the + // first non-control code point — is accepted, since the policy is deliberately narrower + // than RFC 7230's tchar set. Constructed with toChar() so the bytes are unambiguous. + val tab = 9.toChar() // 0x09, inside the C0 range + val unitSeparator = 31.toChar() // 0x1F, top of the C0 range + val del = 127.toChar() // 0x7F + val space = 32.toChar() // 0x20, first accepted code point + assertFailsWith { Headers.builder().add("X-Foo" + tab + "Bar", "v") } + assertFailsWith { Headers.builder().add("X-Foo" + unitSeparator + "Bar", "v") } + assertFailsWith { Headers.builder().add("X-Foo" + del + "Bar", "v") } + val accepted = Headers.builder().add("X-Foo" + space + "Bar", "v").build() + assertEquals("v", accepted.get("X-Foo Bar")) + } + // ---- accessors & equality coverage ------------------------------------------ @Test diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HttpHeaderNameTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HttpHeaderNameTest.kt index 60deb3a1..df0979f7 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HttpHeaderNameTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HttpHeaderNameTest.kt @@ -13,6 +13,7 @@ import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicReferenceArray import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertNotNull import kotlin.test.assertSame import kotlin.test.assertTrue @@ -82,13 +83,25 @@ class HttpHeaderNameTest { } @Test - fun `fromString with empty string interns the empty key`() { - val empty = HttpHeaderName.fromString("") - val whitespace = HttpHeaderName.fromString(" ") - // Both trim to the empty string and intern under the same key. - assertSame(empty, whitespace) - assertEquals("", empty.caseInsensitiveName) - assertEquals("", empty.caseSensitiveName) + fun `fromString rejects a blank name`() { + // An empty or all-whitespace name has no canonical form and is not a valid field-name. + assertFailsWith { HttpHeaderName.fromString("") } + assertFailsWith { HttpHeaderName.fromString(" ") } + } + + @Test + fun `fromString rejects a name with an interior control character`() { + // The typed API shares Headers.Builder's name validation, so a control-character name + // cannot be interned and reach a transport as a header-splitting vector. Surrounding + // whitespace is trimmed (see the trimming test); only interior control bytes are rejected. + val cr = 13.toChar() + val lf = 10.toChar() + val nul = 0.toChar() + val del = 127.toChar() + assertFailsWith { HttpHeaderName.fromString("X-Evil" + cr + lf + "Injected") } + assertFailsWith { HttpHeaderName.fromString("X-Evil" + lf + "Injected") } + assertFailsWith { HttpHeaderName.fromString("X-Evil" + nul + "Injected") } + assertFailsWith { HttpHeaderName.fromString("X-Evil" + del + "Injected") } } @Test diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt index 99451704..3035c943 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt @@ -104,11 +104,12 @@ internal class RequestAdapter( * `IllegalArgumentException` escape [adapt] (and therefore `execute`, declared * `@Throws(IOException)`) where a caller's `catch(IOException)` would not observe it. * - * Note this catch guards against the JDK's restricted *name* set only. Malformed header names - * (CR/LF, NUL, other control characters) and illegal header values (CR/LF) are rejected - * upstream by `Headers.Builder`, so neither a control-character name nor such a value reaches - * this point — the `IllegalArgumentException` handled here is the JDK refusing a *restricted* - * (but otherwise well-formed) name, not a malformed name or value. + * Upstream `Headers.Builder` validation closes the request/header-splitting surface + * (control-character names and CR/LF values are rejected before they reach here), but it does + * not mirror the JDK's full field-name/value grammar. The `IllegalArgumentException` caught + * here is therefore the JDK refusing either a name in its restricted set or a model-valid + * name/value it nonetheless rejects (e.g. a non-token / non-ASCII byte the SDK deliberately + * permits) — not a control-character splitting vector, which never gets this far. */ private fun attachHeaders( builder: HttpRequest.Builder, @@ -130,7 +131,7 @@ internal class RequestAdapter( .event("transport.jdkhttp.header.rejected") .field("name", rawName) .cause(e) - .log("JDK rejected header value; dropping before dispatch") + .log("JDK rejected header name/value; dropping before dispatch") } } } diff --git a/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt b/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt index 9e9278d8..41fc9f90 100644 --- a/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt +++ b/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt @@ -56,6 +56,7 @@ import kotlin.test.assertEquals import kotlin.test.assertFails import kotlin.test.assertFailsWith import kotlin.test.assertNotNull +import kotlin.test.assertNull import kotlin.test.assertTrue /** @@ -327,6 +328,31 @@ class JdkHttpTransportTest { assertEquals("kept", recorded.headers["X-Pass-Through"]) } + @Test + fun `headerRejectedByJdkIsDroppedNotThrown`() { + // The SDK model layer permits a non-ASCII header name (it rejects only control characters), + // but the JDK's field-name grammar rejects a non-token byte. The adapter must drop it rather + // than let the unchecked IllegalArgumentException escape execute()'s @Throws(IOException) + // contract — the same drop-and-log path the OkHttp adapter now mirrors. Built with toChar() + // so the offending byte is unambiguous in source. + val oUmlaut = 246.toChar() // 'o' with diaeresis (U+00F6): not an RFC 7230 token char + server.enqueue(MockResponse.Builder().code(200).body("ok").build()) + val request = + Request.builder() + .method(Method.GET) + .url(server.url("/non-token-name").toUrl()) + .addHeader("X-Uni" + oUmlaut + "code", "plain") + .addHeader("X-Pass-Through", "kept") + .build() + // execute must NOT throw; the rejected header is simply absent on the wire. + transport.execute(request).use { response -> + assertEquals(200, response.status.code) + } + val recorded = server.takeRequest() + assertNull(recorded.headers["X-Uni" + oUmlaut + "code"], "non-token name must be dropped") + assertEquals("kept", recorded.headers["X-Pass-Through"]) + } + @Test fun `expectHeaderDoesNotCrashTheRequest`() { // `Expect` is in the JDK's disallowed-header set — setting it directly on diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt index 4db7d0ba..0036a852 100644 --- a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt @@ -54,11 +54,24 @@ internal class RequestAdapter( continue } for (value in values) { - // Header names and values are validated upstream by Headers.Builder: names reject - // control characters (CR/LF, NUL, the rest of the C0/DEL range) and values reject - // CR/LF, so addHeader receives only well-formed input here and never throws on a - // malformed name or value. - builder.addHeader(rawName, value) + // Header names and values are validated upstream by Headers.Builder (names reject + // control characters; values reject CR/LF), which closes the request/header-splitting + // surface. OkHttp is stricter still: it rejects any byte outside printable ASCII in a + // name (0x21-0x7e) or value (tab and 0x20-0x7e), so a model-valid non-ASCII (e.g. + // UTF-8) name or value — which the SDK deliberately permits — would make addHeader + // throw an unchecked IllegalArgumentException. Catch it and drop just that header, + // mirroring the JDK transport's attachHeaders, so the exception never escapes adapt + // (and therefore sync execute, declared @Throws(IOException)) as an unchecked failure + // a caller's catch(IOException) would miss. + try { + builder.addHeader(rawName, value) + } catch (e: IllegalArgumentException) { + logger.atVerbose() + .event("transport.okhttp.header.rejected") + .field("name", rawName) + .cause(e) + .log("OkHttp rejected header name/value; dropping before dispatch") + } } } val methodToken = request.method.method diff --git a/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt b/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt index 2d8d0ad4..1c02c5be 100644 --- a/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt +++ b/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt @@ -50,6 +50,7 @@ import kotlin.test.assertFails import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertNotNull +import kotlin.test.assertNull import kotlin.test.assertTrue /** @@ -328,6 +329,55 @@ class OkHttpTransportTest { assertEquals("kept", recorded.headers["X-Pass-Through"]) } + @Test + fun headerRejectedByOkHttpStricterRuleIsDroppedNotThrown() { + // The SDK model layer permits non-ASCII header names/values (it rejects only control + // characters), but OkHttp restricts both to printable ASCII and throws an unchecked + // IllegalArgumentException otherwise. The adapter must drop such a header — mirroring the + // JDK transport — so the exception never escapes execute()'s @Throws(IOException) contract. + // The offending byte is built with toChar() to keep it unambiguous in source. + val oUmlaut = 246.toChar() // 'o' with diaeresis (U+00F6): valid UTF-8, not printable ASCII + server.enqueue(MockResponse.Builder().code(200).body("ok").build()) + val request = + Request.builder() + .method(Method.GET) + .url(server.url("/non-ascii").toUrl()) + .addHeader("X-Uni" + oUmlaut + "code", "plain") // non-ASCII name + .addHeader("X-Plain", "v" + oUmlaut + "lue") // non-ASCII value + .addHeader("X-Pass-Through", "kept") + .build() + // execute must NOT throw; the rejected headers are simply absent on the wire. + transport.execute(request).use { response -> + assertEquals(200, response.status.code) + } + val recorded = server.takeRequest() + assertNull(recorded.headers["X-Uni" + oUmlaut + "code"], "non-ASCII name must be dropped") + assertNull(recorded.headers["X-Plain"], "header carrying a non-ASCII value must be dropped") + assertEquals("kept", recorded.headers["X-Pass-Through"]) + } + + @Test + fun headerRejectedByOkHttpStricterRuleIsDroppedNotThrownAsync() { + // The adapter runs on the async path too. A header OkHttp rejects must be dropped so the + // future completes normally (not exceptionally) and the request still dispatches. + val oUmlaut = 246.toChar() // 'o' with diaeresis (U+00F6) + server.enqueue(MockResponse.Builder().code(200).body("ok").build()) + val request = + Request.builder() + .method(Method.GET) + .url(server.url("/non-ascii-async").toUrl()) + .addHeader("X-Uni" + oUmlaut + "code", "plain") + .addHeader("X-Pass-Through", "kept") + .build() + val response = transport.executeAsync(request).get(5, TimeUnit.SECONDS) + response.use { + assertEquals(200, it.status.code) + } + val recorded = server.takeRequest() + assertNull(recorded.headers["X-Uni" + oUmlaut + "code"], "non-ASCII name must be dropped") + assertEquals("kept", recorded.headers["X-Pass-Through"]) + } + // -------- request body streaming -------- @Test From c02d0fc558f50635773b9a866ba1fcc445744206 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Wed, 24 Jun 2026 12:04:31 +0300 Subject: [PATCH 3/4] fix: reject control characters in header values and harden inbound header adaptation Broaden header-value validation to reject the full C0 control range and DEL, matching the policy already applied to header names, while still permitting horizontal tab (legal in field-values per RFC 7230) and non-ASCII bytes. Name and value validation now share a single HeaderValidation module, and the name check returns its trimmed form so callers no longer trim twice. Make response adaptation resilient: both transports copy inbound response headers through a per-header guard that drops and logs a header the model layer rejects instead of failing the entire response. The underlying clients parse response headers leniently and can surface a control byte (notably over HTTP/2); the model-layer guard is an outbound injection concern and must not turn a readable response into an error. Raise the outbound dropped-header log to WARNING: a header the caller set that the transport cannot encode is silent data loss and should be visible by default, whereas restricted-header drops and the inbound response-header drop remain at verbose. Add coverage for value rejection (NUL, DEL, and the predicate boundary), the inbound drop path on both transports, and the JDK async drop-not-throw path. --- .../core/http/common/HeaderNameValidation.kt | 79 ---------- .../sdk/core/http/common/HeaderValidation.kt | 136 ++++++++++++++++++ .../dexpace/sdk/core/http/common/Headers.kt | 40 ++---- .../sdk/core/http/common/HttpHeaderName.kt | 5 +- .../sdk/core/http/common/HeadersTest.kt | 51 ++++++- .../sdk/transport/jdkhttp/JdkHttpTransport.kt | 2 +- .../jdkhttp/internal/RequestAdapter.kt | 11 +- .../jdkhttp/internal/ResponseAdapter.kt | 35 ++++- .../transport/jdkhttp/JdkHttpTransportTest.kt | 23 +++ .../internal/ResponseAdapterLeakTest.kt | 52 ++++++- .../sdk/transport/okhttp/OkHttpTransport.kt | 2 +- .../okhttp/internal/RequestAdapter.kt | 23 +-- .../okhttp/internal/ResponseAdapter.kt | 35 ++++- .../internal/ResponseAdapterInboundTest.kt | 72 ++++++++++ 14 files changed, 431 insertions(+), 135 deletions(-) delete mode 100644 sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HeaderNameValidation.kt create mode 100644 sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HeaderValidation.kt create mode 100644 sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/internal/ResponseAdapterInboundTest.kt diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HeaderNameValidation.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HeaderNameValidation.kt deleted file mode 100644 index 89e31a2b..00000000 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HeaderNameValidation.kt +++ /dev/null @@ -1,79 +0,0 @@ -/* - * Copyright (c) 2026 dexpace and Omar Aljarrah - * - * Licensed under the MIT License. See LICENSE in the project root. - * SPDX-License-Identifier: MIT - */ - -package org.dexpace.sdk.core.http.common - -/** - * Validates an HTTP header name at the transport-agnostic model layer. Shared by the - * String-keyed [Headers.Builder] API and the typed [HttpHeaderName.fromString] entry point so a - * malformed name cannot slip through either one — the two were previously inconsistent (only the - * String API validated, and only against the raw input). - * - * The check runs on the **trimmed** name, matching the trimming both call sites apply before - * storing or interning. `String.trim()` strips only the surrounding *whitespace* code points — - * space, tab, CR, LF, and the C0 information separators (`0x1C`–`0x1F`) — so a leading or trailing - * one of those is removed before it could reach the wire and is harmless. Every other control byte - * survives the trim: a surrounding NUL or DEL, like any *interior* control character, is rejected. - * What is rejected: - * - * - **A blank name.** A field-name must be a non-empty RFC 7230 `token`; an empty or - * all-whitespace name has no canonical form. - * - **Any interior control character** — the C0 control range and DEL (code points `0x00`–`0x1F` - * 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. - * - * 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 CR/LF-only stance taken for values. - */ -@JvmSynthetic -internal fun requireValidHeaderName(rawName: String) { - val trimmed = rawName.trim() - require(trimmed.isNotEmpty()) { "Header name must not be blank." } - trimmed.forEach { ch -> - require(ch.code > LAST_C0_CONTROL && ch.code != DEL_CONTROL) { - "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." - } - } -} - -/** - * Renders [name] for an error message with every control character replaced by its `\uXXXX` - * escape, so a raw CR/LF/NUL from the rejected name never lands verbatim in a log line while the - * printable portion still identifies the offending header. - */ -private fun escapeControlCharacters(name: String): String = - buildString { - name.forEach { ch -> - if (ch.code <= LAST_C0_CONTROL || ch.code == DEL_CONTROL) { - append("\\u") - append(ch.code.toString(HEX_RADIX).padStart(ESCAPE_HEX_WIDTH, '0')) - } else { - append(ch) - } - } - } - -/** 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. */ -private const val DEL_CONTROL: Int = 0x7F - -/** Radix for rendering a control character's code point as the hex digits of a `\uXXXX` escape. */ -private const val HEX_RADIX: Int = 16 - -/** Zero-padded width of a `\uXXXX` escape's hex digits. */ -private const val ESCAPE_HEX_WIDTH: Int = 4 diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HeaderValidation.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HeaderValidation.kt new file mode 100644 index 00000000..dad0c4ae --- /dev/null +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HeaderValidation.kt @@ -0,0 +1,136 @@ +/* + * Copyright (c) 2026 dexpace and Omar Aljarrah + * + * Licensed under the MIT License. See LICENSE in the project root. + * SPDX-License-Identifier: MIT + */ + +package org.dexpace.sdk.core.http.common + +/** + * Validates an HTTP header name at the transport-agnostic model layer and returns its trimmed + * form. Shared by the String-keyed [Headers.Builder] API and the typed [HttpHeaderName.fromString] + * entry point so a malformed name cannot slip through either one — the two were previously + * inconsistent (only the String API validated, and only against the raw input). The trimmed name is + * returned so callers reuse it instead of trimming a second time. + * + * The check runs on the **trimmed** name. `String.trim()` removes only surrounding *whitespace* — + * the full Unicode class `Char.isWhitespace` recognises (`Character.isWhitespace || + * Character.isSpaceChar`: ASCII space and tab, the C0 line/separator controls, and the Unicode + * space separators such as NBSP) — so leading or trailing whitespace is stripped before it could + * reach the wire and is harmless. A surrounding control byte that is *not* whitespace — NUL, DEL, + * and the other non-whitespace C0 codes — survives the trim and is rejected, exactly like an + * *interior* control character. What is rejected: + * + * - **A blank name.** A field-name must be a non-empty RFC 7230 `token`; an empty or + * all-whitespace name has no canonical form. + * - **Any interior control character** — the C0 control range and DEL (code points `0x00`–`0x1F` + * 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. + * + * 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 + * [requireValidHeaderValues]. + * + * @return the trimmed, validated name + * @throws IllegalArgumentException if the trimmed name is blank or contains a control character + */ +@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." + } + } + return trimmed +} + +/** + * Validates the [values] of a header [name] at the transport-agnostic model layer, applying the + * same control-character policy as [requireValidHeaderName] with **one deliberate exception**: + * horizontal tab (`0x09`) is permitted. Unlike a field-name `token`, an RFC 7230 field-value may + * carry HTAB as whitespace between field-content, and the two reference transports accept it (it is + * the one control byte OkHttp's value rule allows), so rejecting it would refuse a legitimate value. + * + * Every other C0 control (`0x00`–`0x1F`, which covers CR, LF, and NUL) and DEL (`0x7F`) is + * rejected. A bare CR/LF is the request/header-splitting vector — once a value is serialised an + * attacker could inject a new header or a second request — and the remaining control bytes are + * illegal in a field-value on every transport. The earlier policy here rejected only CR/LF; the + * 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. + * + * @throws IllegalArgumentException if any value contains a prohibited control character + */ +@JvmSynthetic +internal fun requireValidHeaderValues( + name: String, + values: List, +) { + 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." + } + } + } +} + +/** 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 + +/** + * 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. + */ +private fun isProhibitedInValue(code: Int): Boolean = + (code <= LAST_C0_CONTROL && code != HORIZONTAL_TAB) || code == DEL_CONTROL + +/** + * Renders [name] for an error message with every control character replaced by its `\uXXXX` + * escape, so a raw CR/LF/NUL from the rejected name never lands verbatim in a log line while the + * printable portion still identifies the offending header. + */ +private fun escapeControlCharacters(name: String): String = + buildString { + name.forEach { ch -> + if (ch.code <= LAST_C0_CONTROL || ch.code == DEL_CONTROL) { + append("\\u") + append(ch.code.toString(HEX_RADIX).padStart(ESCAPE_HEX_WIDTH, '0')) + } else { + append(ch) + } + } + } + +/** Horizontal tab (`0x09`) — the one C0 control RFC 7230 permits in a field-value (but not a name). */ +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. */ +private const val DEL_CONTROL: Int = 0x7F + +/** Radix for rendering a control character's code point as the hex digits of a `\uXXXX` escape. */ +private const val HEX_RADIX: Int = 16 + +/** Zero-padded width of a `\uXXXX` escape's hex digits. */ +private const val ESCAPE_HEX_WIDTH: Int = 4 diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt index 836df400..2d555f4b 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt @@ -160,9 +160,9 @@ public data class Headers private constructor( values: List, ): Builder = apply { - requireValidHeaderName(name) - validateValues(name, values) - headersMap.computeIfAbsent(sanitizeName(name)) { mutableListOf() }.addAll(values) + val trimmedName = requireValidHeaderName(name) + requireValidHeaderValues(trimmedName, values) + headersMap.computeIfAbsent(sanitizeName(trimmedName)) { mutableListOf() }.addAll(values) } /** @@ -181,7 +181,7 @@ public data class Headers private constructor( values: List, ): Builder = apply { - validateValues(name.caseInsensitiveName, values) + requireValidHeaderValues(name.caseInsensitiveName, values) headersMap.computeIfAbsent(name.caseInsensitiveName) { mutableListOf() }.addAll(values) } @@ -219,9 +219,9 @@ public data class Headers private constructor( values: List, ): Builder = apply { - requireValidHeaderName(name) - validateValues(name, values) - headersMap[sanitizeName(name)] = values.toMutableList() + val trimmedName = requireValidHeaderName(name) + requireValidHeaderValues(trimmedName, values) + headersMap[sanitizeName(trimmedName)] = values.toMutableList() } /** @@ -248,7 +248,7 @@ public data class Headers private constructor( values: List, ): Builder = apply { - validateValues(name.caseInsensitiveName, values) + requireValidHeaderValues(name.caseInsensitiveName, values) headersMap[name.caseInsensitiveName] = values.toMutableList() } @@ -311,29 +311,5 @@ public data class Headers private constructor( * so locale-sensitive folding (Turkish `i`, etc.) would be incorrect here. */ private fun sanitizeName(value: String): String = value.lowercase(Locale.US).trim() - - /** - * Rejects header values that would enable request/header splitting before they reach a - * transport. A bare carriage return (`\r`) or line feed (`\n`) in a value lets an - * attacker inject a new header or even a second request once the value is serialised; - * OkHttp throws unchecked on such values and the JDK transport silently drops them, so we - * validate here at the transport-agnostic model layer to fail fast and uniformly. - * - * Policy: reject **only** CR/LF, not OkHttp's stricter printable-ASCII-only rule. CR/LF - * are the splitting vector and are illegal in every transport; tightening further would - * reject legitimate UTF-8 values that some transports (and the JDK) accept, so the - * conservative CR/LF check is the right model-layer contract. - */ - private fun validateValues( - name: String, - values: List, - ) { - values.forEach { value -> - require(value.indexOf('\r') < 0 && value.indexOf('\n') < 0) { - "Header value for '$name' must not contain a carriage return or line feed " + - "(\\r / \\n); such characters enable request/header splitting." - } - } - } } } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HttpHeaderName.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HttpHeaderName.kt index 56eb0403..97dd1c90 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HttpHeaderName.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/HttpHeaderName.kt @@ -229,8 +229,9 @@ public class HttpHeaderName private constructor( */ @JvmStatic public fun fromString(name: String): HttpHeaderName { - requireValidHeaderName(name) - val trimmed = name.trim() + // requireValidHeaderName trims and validates, returning the trimmed form so we do not + // trim a second time before interning. + val trimmed = requireValidHeaderName(name) val key = trimmed.lowercase(Locale.US) // computeIfAbsent is available on Java 8. return INTERN.computeIfAbsent(key) { HttpHeaderName(trimmed, key) } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt index 7182c0a8..59efe91f 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt @@ -216,6 +216,52 @@ class HeadersTest { } } + @Test + fun `add rejects a value containing a NUL`() { + assertFailsWith { + Headers.builder().add("X-Evil", "ok" + 0.toChar() + "bad") + } + } + + @Test + fun `add rejects a value containing a DEL control character`() { + assertFailsWith { + Headers.builder().add("X-Evil", "ok" + 127.toChar() + "bad") + } + } + + @Test + fun `value validation rejects interior control bytes through 0x1F and DEL but accepts tab and space`() { + // Pins the value-side predicate boundary. The policy mirrors the name check — the C0 + // range (up to and including 0x1F) and DEL (0x7F) are rejected — with one deliberate + // exception: horizontal tab (0x09), which RFC 7230 permits as field-value whitespace. + // Space (0x20), the first non-control code point, is accepted. Constructed with toChar() + // so the bytes are unambiguous. + val nul = 0.toChar() // 0x00, bottom of the C0 range + val unitSeparator = 31.toChar() // 0x1F, top of the C0 range + val del = 127.toChar() // 0x7F + val tab = 9.toChar() // 0x09, the one permitted control character + val space = 32.toChar() // 0x20, first accepted code point + assertFailsWith { Headers.builder().add("X-Foo", "a" + nul + "b") } + assertFailsWith { Headers.builder().add("X-Foo", "a" + unitSeparator + "b") } + assertFailsWith { Headers.builder().add("X-Foo", "a" + del + "b") } + val accepted = Headers.builder().add("X-Foo", "a" + tab + space + "b").build() + assertEquals("a" + tab + space + "b", accepted.get("X-Foo")) + } + + @Test + fun `the value rejection message does not echo the value`() { + // The value may be a secret (Authorization) or oversized; unlike the name, it is never + // rendered into the error message — only the header name is, to locate the offender. + val thrown = + assertFailsWith { + Headers.builder().add("X-Trace-Id", "secret-token" + 0.toChar() + "x") + } + val message = thrown.message ?: "" + assertTrue(message.contains("X-Trace-Id"), "message should name the header, got: $message") + assertFalse(message.contains("secret-token"), "message must not echo the value") + } + @Test fun `the rejection message names the offending header`() { val thrown = @@ -229,13 +275,14 @@ class HeadersTest { } @Test - fun `normal values without CR or LF are accepted`() { + fun `normal values without prohibited control characters are accepted`() { val headers = Headers.builder() .add("X-Plain", "hello world") .set("Authorization", "Bearer abc.def-ghi") .add(HttpHeaderName.SET_COOKIE, "id=42; Path=/") - // Tabs and UTF-8 are not CR/LF, so they must pass the conservative check. + // Horizontal tab is the one permitted control character, and UTF-8 is not a control + // character at all, so both pass the conservative check. .set("X-Unicode", "café\tvalue") .build() diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt index a80a4b0e..3f24774f 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt @@ -87,7 +87,7 @@ public class JdkHttpTransport private constructor( ) : HttpClient, AsyncHttpClient { private val log: ClientLogger = ClientLogger("org.dexpace.sdk.transport.jdkhttp.JdkHttpTransport") private val requestAdapter: RequestAdapter = RequestAdapter(log) - private val responseAdapter: ResponseAdapter = ResponseAdapter() + private val responseAdapter: ResponseAdapter = ResponseAdapter(log) /** * Latches `true` on the first [close] so subsequent calls are no-ops. `AtomicBoolean` diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt index 3035c943..98c4bd38 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt @@ -105,8 +105,9 @@ internal class RequestAdapter( * `@Throws(IOException)`) where a caller's `catch(IOException)` would not observe it. * * Upstream `Headers.Builder` validation closes the request/header-splitting surface - * (control-character names and CR/LF values are rejected before they reach here), but it does - * not mirror the JDK's full field-name/value grammar. The `IllegalArgumentException` caught + * (control-character names, and control-character values bar horizontal tab, are rejected + * before they reach here), but it does not mirror the JDK's full field-name/value grammar. + * The `IllegalArgumentException` caught * here is therefore the JDK refusing either a name in its restricted set or a model-valid * name/value it nonetheless rejects (e.g. a non-token / non-ASCII byte the SDK deliberately * permits) — not a control-character splitting vector, which never gets this far. @@ -127,7 +128,11 @@ internal class RequestAdapter( try { builder.header(rawName, value) } catch (e: IllegalArgumentException) { - logger.atVerbose() + // Warn (not verbose): this is a header the caller explicitly set being silently + // dropped because this transport cannot encode it — surfaced by default so the + // loss is visible. Restricted-header drops above stay at verbose (expected, the + // JDK recomputes or forbids them), as does the inbound response-header drop. + logger.atWarning() .event("transport.jdkhttp.header.rejected") .field("name", rawName) .cause(e) diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/ResponseAdapter.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/ResponseAdapter.kt index 4f3d493a..4c07b632 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/ResponseAdapter.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/ResponseAdapter.kt @@ -11,6 +11,7 @@ import org.dexpace.sdk.core.http.common.Headers import org.dexpace.sdk.core.http.common.MediaType import org.dexpace.sdk.core.http.common.Protocol import org.dexpace.sdk.core.http.response.Status +import org.dexpace.sdk.core.instrumentation.ClientLogger import org.dexpace.sdk.core.io.Io import java.io.IOException import java.io.InputStream @@ -46,7 +47,9 @@ import org.dexpace.sdk.core.http.response.ResponseBody as SdkResponseBody * never leaked — both the synchronous `execute` and asynchronous `sendAsync().thenApply` * paths route through this method, so both inherit the guard. */ -internal class ResponseAdapter { +internal class ResponseAdapter( + private val logger: ClientLogger, +) { fun adapt( sdkRequest: SdkRequest, jdkResponse: HttpResponse, @@ -67,7 +70,7 @@ internal class ResponseAdapter { // on `add(...)`, so we can pass the keys through verbatim. jdkResponse.headers().map().forEach { (name, values) -> for (value in values) { - headersBuilder.add(name, value) + addInboundHeader(headersBuilder, name, value) } } val headers = headersBuilder.build() @@ -92,6 +95,34 @@ internal class ResponseAdapter { } } + /** + * Copies one inbound (response) header into [headersBuilder]. + * + * The model layer's `add` validation is an **outbound** injection guard — it stops an SDK + * caller from putting a CR/LF or other control character into a *request* that is then + * serialised to a server. A response has already been received; the JDK client parses response + * headers leniently and can surface a control byte (notably over HTTP/2), so a strict + * re-validation here would let one odd server header throw out of `add` and, via the outer + * catch, fail the entire response. Catch that rejection and drop just the offending header — + * the body and the rest of the headers still reach the caller. The header name is logged (not + * the value, which may be sensitive) at verbose, mirroring the request adapter's drop-and-log. + */ + private fun addInboundHeader( + headersBuilder: Headers.Builder, + name: String, + value: String, + ) { + try { + headersBuilder.add(name, value) + } catch (e: IllegalArgumentException) { + logger.atVerbose() + .event("transport.jdkhttp.response.header.dropped") + .field("name", name) + .cause(e) + .log("dropping malformed inbound header from response") + } + } + /** * Parses the `Content-Type` header value into an SDK [MediaType]. Returns `null` when * the header is absent or the value cannot be parsed — surfacing the body without a diff --git a/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt b/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt index 41fc9f90..03e30131 100644 --- a/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt +++ b/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt @@ -353,6 +353,29 @@ class JdkHttpTransportTest { assertEquals("kept", recorded.headers["X-Pass-Through"]) } + @Test + fun `headerRejectedByJdkIsDroppedNotThrownAsync`() { + // The adapter runs on the async path too. A header the JDK rejects must be dropped so the + // future completes normally (not exceptionally) and the request still dispatches — + // mirroring the sync drop-not-throw path and the OkHttp transport's async coverage. + val oUmlaut = 246.toChar() // 'o' with diaeresis (U+00F6): not an RFC 7230 token char + server.enqueue(MockResponse.Builder().code(200).body("ok").build()) + val request = + Request.builder() + .method(Method.GET) + .url(server.url("/non-token-name-async").toUrl()) + .addHeader("X-Uni" + oUmlaut + "code", "plain") + .addHeader("X-Pass-Through", "kept") + .build() + val response = transport.executeAsync(request).get(5, TimeUnit.SECONDS) + response.use { + assertEquals(200, it.status.code) + } + val recorded = server.takeRequest() + assertNull(recorded.headers["X-Uni" + oUmlaut + "code"], "non-token name must be dropped") + assertEquals("kept", recorded.headers["X-Pass-Through"]) + } + @Test fun `expectHeaderDoesNotCrashTheRequest`() { // `Expect` is in the JDK's disallowed-header set — setting it directly on diff --git a/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/ResponseAdapterLeakTest.kt b/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/ResponseAdapterLeakTest.kt index cf21f679..f57b06bb 100644 --- a/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/ResponseAdapterLeakTest.kt +++ b/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/ResponseAdapterLeakTest.kt @@ -8,6 +8,7 @@ package org.dexpace.sdk.transport.jdkhttp.internal import org.dexpace.sdk.core.http.request.Method +import org.dexpace.sdk.core.instrumentation.ClientLogger import org.dexpace.sdk.core.io.Io import org.dexpace.sdk.io.OkioIoProvider import java.io.ByteArrayInputStream @@ -23,7 +24,9 @@ import java.util.concurrent.atomic.AtomicBoolean import javax.net.ssl.SSLSession import kotlin.test.BeforeTest import kotlin.test.Test +import kotlin.test.assertEquals import kotlin.test.assertFails +import kotlin.test.assertNull import kotlin.test.assertTrue import org.dexpace.sdk.core.http.request.Request as SdkRequest @@ -34,6 +37,8 @@ import org.dexpace.sdk.core.http.request.Request as SdkRequest * `sendAsync().thenApply` paths route through `adapt`, so closing in `adapt` covers both. */ class ResponseAdapterLeakTest { + private val logger = ClientLogger("test.ResponseAdapterLeakTest") + @BeforeTest fun setUp() { Io.installProvider(OkioIoProvider) @@ -47,7 +52,7 @@ class ResponseAdapterLeakTest { // the catch path. This stands in for any real post-body failure (provider missing, OOM // while wrapping, builder rejection) without relying on global IoProvider state. val response = ThrowingVersionResponse(code = 200, bodyStream = body) - val adapter = ResponseAdapter() + val adapter = ResponseAdapter(logger) val ex = assertFails { @@ -63,7 +68,7 @@ class ResponseAdapterLeakTest { val closed = AtomicBoolean(false) val body = TrackingInputStream("payload".toByteArray(), closed) val response = OkResponse(code = 200, bodyStream = body) - val adapter = ResponseAdapter() + val adapter = ResponseAdapter(logger) val sdkResponse = adapter.adapt(simpleRequest(), response) @@ -74,6 +79,34 @@ class ResponseAdapterLeakTest { assertTrue(closed.get(), "closing the SDK response must cascade-close the body") } + @Test + fun `adapt drops a malformed inbound response header instead of failing the response`() { + // A server (notably over HTTP/2) can send a header value the JDK client parses leniently + // but the SDK model layer rejects. Adaptation must drop that one header and still surface + // the response — the outbound injection guard does not apply to a received response. DEL + // (0x7F) is rejected by the model layer yet survives lenient inbound parsing. + val del = 127.toChar() + val closed = AtomicBoolean(false) + val body = TrackingInputStream("ok".toByteArray(), closed) + val response = + HeaderResponse( + code = 200, + bodyStream = body, + headerMap = + mapOf( + "X-Good" to listOf("fine"), + "X-Bad" to listOf("a" + del + "b"), + ), + ) + val adapter = ResponseAdapter(logger) + + val sdkResponse = adapter.adapt(simpleRequest(), response) + + assertEquals("fine", sdkResponse.headers.get("X-Good"), "well-formed inbound header must survive") + assertNull(sdkResponse.headers.get("X-Bad"), "malformed inbound header must be dropped, not throw") + sdkResponse.close() + } + private fun simpleRequest(): SdkRequest = SdkRequest.builder() .method(Method.GET) @@ -125,6 +158,21 @@ class ResponseAdapterLeakTest { override fun version(): HttpClient.Version = HttpClient.Version.HTTP_1_1 } + /** Successful [HttpResponse] carrying caller-supplied headers, to drive the inbound-drop path. */ + private class HeaderResponse( + private val code: Int, + private val bodyStream: InputStream, + private val headerMap: Map>, + ) : BaseResponse() { + override fun statusCode(): Int = code + + override fun body(): InputStream = bodyStream + + override fun version(): HttpClient.Version = HttpClient.Version.HTTP_1_1 + + override fun headers(): HttpHeaders = HttpHeaders.of(headerMap) { _, _ -> true } + } + /** * Shared abstract base supplying inert implementations of the [HttpResponse] members the * adapter never consults, so the concrete fakes only override what each test needs. diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransport.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransport.kt index 61223827..1e298c29 100644 --- a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransport.kt +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransport.kt @@ -70,7 +70,7 @@ public class OkHttpTransport private constructor( ) : HttpClient, AsyncHttpClient { private val log: ClientLogger = ClientLogger("org.dexpace.sdk.transport.okhttp.OkHttpTransport") private val requestAdapter: RequestAdapter = RequestAdapter(log) - private val responseAdapter: ResponseAdapter = ResponseAdapter() + private val responseAdapter: ResponseAdapter = ResponseAdapter(log) /** * Latches `true` on the first [close] so subsequent calls are no-ops. `AtomicBoolean` diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt index 0036a852..9769c264 100644 --- a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt @@ -55,18 +55,23 @@ internal class RequestAdapter( } for (value in values) { // Header names and values are validated upstream by Headers.Builder (names reject - // control characters; values reject CR/LF), which closes the request/header-splitting - // surface. OkHttp is stricter still: it rejects any byte outside printable ASCII in a - // name (0x21-0x7e) or value (tab and 0x20-0x7e), so a model-valid non-ASCII (e.g. - // UTF-8) name or value — which the SDK deliberately permits — would make addHeader - // throw an unchecked IllegalArgumentException. Catch it and drop just that header, - // mirroring the JDK transport's attachHeaders, so the exception never escapes adapt - // (and therefore sync execute, declared @Throws(IOException)) as an unchecked failure - // a caller's catch(IOException) would miss. + // control characters; values reject control characters except horizontal tab), + // which closes the request/header-splitting surface. OkHttp is stricter still: it + // rejects any byte outside printable ASCII in a name (0x21-0x7e) or value (tab and + // 0x20-0x7e), so a model-valid non-ASCII (e.g. UTF-8) name or value — which the SDK + // deliberately permits — would make addHeader throw an unchecked + // IllegalArgumentException. Catch it and drop just that header, mirroring the JDK + // transport's attachHeaders, so the exception never escapes adapt (and therefore + // sync execute, declared @Throws(IOException)) as an unchecked failure a caller's + // catch(IOException) would miss. try { builder.addHeader(rawName, value) } catch (e: IllegalArgumentException) { - logger.atVerbose() + // Warn (not verbose): this is a header the caller explicitly set being silently + // dropped because this transport cannot encode it — surfaced by default so the + // loss is visible. Restricted-header drops below stay at verbose (expected, the + // transport recomputes them), as does the inbound response-header drop. + logger.atWarning() .event("transport.okhttp.header.rejected") .field("name", rawName) .cause(e) diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/ResponseAdapter.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/ResponseAdapter.kt index e4aea480..a13d153e 100644 --- a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/ResponseAdapter.kt +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/ResponseAdapter.kt @@ -11,6 +11,7 @@ import org.dexpace.sdk.core.http.common.Headers import org.dexpace.sdk.core.http.common.MediaType import org.dexpace.sdk.core.http.common.Protocol import org.dexpace.sdk.core.http.response.Status +import org.dexpace.sdk.core.instrumentation.ClientLogger import org.dexpace.sdk.core.io.Io import okhttp3.Protocol as OkProtocol import okhttp3.Response as OkResponse @@ -40,7 +41,9 @@ import org.dexpace.sdk.core.http.response.ResponseBody as SdkResponseBody * No retention of the OkHttp response object is necessary beyond the body — the body's * own close releases everything. */ -internal class ResponseAdapter { +internal class ResponseAdapter( + private val logger: ClientLogger, +) { fun adapt( sdkRequest: SdkRequest, okhttpResponse: OkResponse, @@ -48,7 +51,7 @@ internal class ResponseAdapter { try { val headersBuilder = Headers.Builder() for ((name, value) in okhttpResponse.headers) { - headersBuilder.add(name, value) + addInboundHeader(headersBuilder, name, value) } // OkHttp 5.x exposes `Response.body` as a non-nullable `ResponseBody`; bodies for // status codes that have no payload (204, 304, etc.) are zero-length, not null. We @@ -86,6 +89,34 @@ internal class ResponseAdapter { } } + /** + * Copies one inbound (response) header into [headersBuilder]. + * + * The model layer's `add` validation is an **outbound** injection guard — it stops an SDK + * caller from putting a CR/LF or other control character into a *request* that is then + * serialised to a server. A response has already been received; OkHttp parses response headers + * leniently and can surface a control byte (notably over HTTP/2), so a strict re-validation + * here would let one odd server header throw out of `add` and, via the outer catch, fail the + * entire response. Catch that rejection and drop just the offending header — the body and the + * rest of the headers still reach the caller. The header name is logged (not the value, which + * may be sensitive) at verbose, mirroring the request adapter's drop-and-log. + */ + private fun addInboundHeader( + headersBuilder: Headers.Builder, + name: String, + value: String, + ) { + try { + headersBuilder.add(name, value) + } catch (e: IllegalArgumentException) { + logger.atVerbose() + .event("transport.okhttp.response.header.dropped") + .field("name", name) + .cause(e) + .log("dropping malformed inbound header from response") + } + } + /** * Maps an OkHttp [OkProtocol] onto the SDK's [Protocol]. OkHttp's `HTTP_2` covers both * normal ALPN-negotiated HTTP/2 and prior-knowledge mode in our pipeline's eyes — the diff --git a/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/internal/ResponseAdapterInboundTest.kt b/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/internal/ResponseAdapterInboundTest.kt new file mode 100644 index 00000000..c72fb6ce --- /dev/null +++ b/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/internal/ResponseAdapterInboundTest.kt @@ -0,0 +1,72 @@ +/* + * Copyright (c) 2026 dexpace and Omar Aljarrah + * + * Licensed under the MIT License. See LICENSE in the project root. + * SPDX-License-Identifier: MIT + */ + +package org.dexpace.sdk.transport.okhttp.internal + +import okhttp3.Headers +import okhttp3.Protocol +import okhttp3.Response +import okhttp3.ResponseBody.Companion.toResponseBody +import org.dexpace.sdk.core.http.request.Method +import org.dexpace.sdk.core.instrumentation.ClientLogger +import org.dexpace.sdk.core.io.Io +import org.dexpace.sdk.io.OkioIoProvider +import java.net.URL +import kotlin.test.BeforeTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull +import okhttp3.Request as OkRequest +import org.dexpace.sdk.core.http.request.Request as SdkRequest + +/** + * Verifies that [ResponseAdapter] drops a malformed inbound (response) header rather than letting + * the model layer's outbound validation fail the whole response. OkHttp parses response headers + * leniently and can surface a control byte (notably over HTTP/2); the adapter must catch the + * model-layer rejection per header and keep the rest of the response intact. + */ +class ResponseAdapterInboundTest { + @BeforeTest + fun setUp() { + Io.installProvider(OkioIoProvider) + } + + @Test + fun `adapt drops a malformed inbound response header instead of failing the response`() { + // DEL (0x7F) is rejected by the model layer yet allowed past OkHttp's lenient + // addUnsafeNonAscii, standing in for a control byte a server delivers over the wire. + val del = 127.toChar() + val headers = + Headers.Builder() + .add("X-Good", "fine") + .addUnsafeNonAscii("X-Bad", "a" + del + "b") + .build() + val okResponse = + Response.Builder() + .request(OkRequest.Builder().url("http://example.test/inbound").build()) + .protocol(Protocol.HTTP_1_1) + .code(200) + .message("OK") + .headers(headers) + .body("ok".toResponseBody(null)) + .build() + val adapter = ResponseAdapter(ClientLogger("test.ResponseAdapterInboundTest")) + + val sdkResponse = adapter.adapt(simpleRequest(), okResponse) + + sdkResponse.use { + assertEquals("fine", it.headers.get("X-Good"), "well-formed inbound header must survive") + assertNull(it.headers.get("X-Bad"), "malformed inbound header must be dropped, not throw") + } + } + + private fun simpleRequest(): SdkRequest = + SdkRequest.builder() + .method(Method.GET) + .url(URL("http://example.test/inbound")) + .build() +} From 1e5a4aaa01eafc456749d1de077026ece93ae1f4 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Wed, 24 Jun 2026 12:22:06 +0300 Subject: [PATCH 4/4] refactor: drop redundant header-name trim in Headers.Builder The String-keyed add/set already trim and validate the name via requireValidHeaderName before storing it, so routing the trimmed name back through sanitizeName re-trimmed it. Add a canonicalKey helper that only case-folds an already-validated name and use it for the storage key; keep sanitizeName for the accessors and remove, which receive raw caller input. --- .../org/dexpace/sdk/core/http/common/Headers.kt | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt index 2d555f4b..c0f05fe1 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt @@ -162,7 +162,7 @@ public data class Headers private constructor( apply { val trimmedName = requireValidHeaderName(name) requireValidHeaderValues(trimmedName, values) - headersMap.computeIfAbsent(sanitizeName(trimmedName)) { mutableListOf() }.addAll(values) + headersMap.computeIfAbsent(canonicalKey(trimmedName)) { mutableListOf() }.addAll(values) } /** @@ -221,7 +221,7 @@ public data class Headers private constructor( apply { val trimmedName = requireValidHeaderName(name) requireValidHeaderValues(trimmedName, values) - headersMap[sanitizeName(trimmedName)] = values.toMutableList() + headersMap[canonicalKey(trimmedName)] = values.toMutableList() } /** @@ -306,10 +306,18 @@ public data class Headers private constructor( public fun builder(): Builder = Builder() /** - * Normalises a header name to its canonical (lower-case, trimmed) storage key. + * Normalises a raw, caller-supplied header name to its canonical (lower-case, trimmed) + * storage key. Used by the accessors and `remove`, which receive untrimmed input. * `Locale.US` is used deliberately — HTTP header names are ASCII-only per RFC 7230, * so locale-sensitive folding (Turkish `i`, etc.) would be incorrect here. */ private fun sanitizeName(value: String): String = value.lowercase(Locale.US).trim() + + /** + * Canonical storage key for a name that was already trimmed and validated by + * [requireValidHeaderName]. Only case-folding is needed — re-trimming (as [sanitizeName] + * does for raw input) would be redundant. `Locale.US` per [sanitizeName]'s rationale. + */ + private fun canonicalKey(validatedName: String): String = validatedName.lowercase(Locale.US) } }