From cecccdfa063898a8e35493de7464bba4348006d8 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Wed, 17 Jun 2026 04:52:47 +0300 Subject: [PATCH 1/2] 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/2] 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