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 85ef3af8..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,6 +160,7 @@ public data class Headers private constructor( values: List, ): Builder = apply { + requireValidHeaderName(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 { + requireValidHeaderName(name) validateValues(name, values) headersMap[sanitizeName(name)] = values.toMutableList() } 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 72e5b2be..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 @@ -258,6 +258,152 @@ 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")) + } + + @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 8e3b1382..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,10 +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. 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. + * 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, @@ -129,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 032e2cf0..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,10 +54,24 @@ 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. - 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