Skip to content

fix: reject control characters in header names#140

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/header-name-crlf-validation
Open

fix: reject control characters in header names#140
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/header-name-crlf-validation

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Problem

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 passed through the model layer intact.

Downstream, the two reference transports then diverged on such a name:

  • OkHttp called addHeader(name, value) with no guard. OkHttp rejects an illegal name with an unchecked IllegalArgumentException, which escaped execute() despite its @Throws(IOException) contract and bypassed the transport's IOException handling / retry pipeline.
  • JDK wrapped header(name, value) in try/catch(IllegalArgumentException) and silently dropped the offending header.

So the same SDK-level request produced two different observable outcomes depending on transport. It was also a header-injection surface for attacker-controlled names.

Change

  • Add validateName beside validateValues in Headers.Builder, invoked from the String-based add/set before the name is stored. It rejects blank names and any character in the C0 control range (0x000x1F, which covers CR/LF/NUL) or DEL (0x7F).
  • The typed HttpHeaderName overloads carry pre-validated token names, so they are unaffected.
  • Correct the OkHttp and JDK transport-adapter comments that previously claimed names were validated upstream; they now describe the actual name and value guarantees.

The policy deliberately stops at control characters rather than RFC 7230's full tchar allow-list, mirroring the conservative CR/LF-only stance already used for values: it closes the splitting/injection surface (control characters are illegal in every transport) without rejecting non-ASCII names that some transports accept.

Tests

Added coverage in HeadersTest for CR, LF, CRLF, NUL, and DEL injection in names via both add and set (single- and list-value overloads), a blank-name rejection, a message-naming check, and an accepted-name case (including whitespace trimming).

Gated build (scoped, --no-daemon)

  • ./gradlew :sdk-core:test :sdk-core:ktlintCheck :sdk-core:detekt :sdk-core:apiCheckpassed
  • ./gradlew :sdk-transport-okhttp:compileKotlin :sdk-transport-okhttp:ktlintCheck :sdk-transport-okhttp:detekt :sdk-transport-jdkhttp:compileKotlin :sdk-transport-jdkhttp:ktlintCheckpassed (detekt is not run on the JDK 11 module)

No public API change (validateName is private), so apiCheck passes with no apiDump.

Closes #114

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.
@OmarAlJarrah OmarAlJarrah changed the title Reject control characters in header names fix: reject control characters in header names Jun 17, 2026
@OmarAlJarrah

Copy link
Copy Markdown
Member Author

This adds a validateName guard to Headers.Builder that rejects C0 control characters and DEL in header names, closing a header-splitting vector for names to match the existing CR/LF check on values. The guard itself is correct and well-tested, but it's only wired into half the API, so the stated guarantee doesn't actually hold. This needs another pass before merging.

Issues

Name validation is missing on the HttpHeaderName-typed overloadssdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt:178-185, 244-251

The new guard is only called from the two String overloads. The typed add(HttpHeaderName, List<String>) and set(HttpHeaderName, List<String>) overloads call validateValues but never validate the name, and store name.caseInsensitiveName directly. Since HttpHeaderName.fromString only trims and lowercases with no control-char check (HttpHeaderName.kt:221-226), a caller can do Headers.builder().add(HttpHeaderName.fromString("X-Evil\nInjected"), "v") and produce a Headers whose name carries an embedded LF — exactly the splitting vector the KDoc claims to close at the model layer. The typed API is a public bypass; the validation needs to live on a shared path that both the String and HttpHeaderName overloads route through.

okhttp adapter comment overstates the guaranteesdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt:54-58

The updated comment now asserts names reject control characters upstream so addHeader only ever sees well-formed input. With the bypass above, a name interned through fromString with an embedded CR/LF reaches entries() unvalidated, and OkHttp's addHeader throws an unchecked IllegalArgumentException. The okhttp loop has no guard, so the exception escapes adapt() — unlike the jdkhttp adapter, which wraps builder.header(...) in try/catch (RequestAdapter.kt:123-131). Either close the model-layer gap so the comment becomes true, or don't claim a guarantee the upstream code doesn't enforce.

No test exercises the typed overload for name validationsdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt:261-360

All 13 new tests go through the String add/set overloads only. A test that built a malicious name via HttpHeaderName.fromString and passed it through the typed add/set would have caught the gap above. Please add coverage for the typed path alongside the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Header names bypass CR/LF validation while header values are validated

1 participant