fix!: reject non-ASCII headers at the model layer and resolve pagination follow-ups#207
Merged
Merged
Conversation
…ion follow-ups The header model previously accepted non-ASCII header names and values, but both reference transports (OkHttp and the JDK HttpClient) reject them, so such a header was silently dropped from the wire on every transport. Tighten HeaderValidation to reject non-ASCII names and values at construction — one fail-fast contract that matches what the transports can actually emit — and lower the transports' residual header-drop log (now only a model-valid but non-token header, e.g. one carrying an interior space) from WARN to verbose so a hot path re-setting such a header no longer floods the log. Pre-1.0 validation break; no public signature changes. Also resolves the follow-ups to the pagination unification: - byPage().stream() no longer strands the held page's connection when a short-circuiting terminal (findFirst/limit/anyMatch) abandons the stream: the stream registers the view's close() as its close handler, so try-with-resources on the Stream releases the held page. - Dedupe the pagination internals: a shared parseOrClose (the close-on-parse-failure invariant, previously copied into both paginators), a single orderedStream iterator-to-Stream adapter, and a shared requirePositiveMaxPages guard called from all four constructors. - Fix stale KDoc and reference docs left over from the unification (strategy parse returns PageInfo, not Page; a delivered page's body is typically already drained; three shipped strategies, not four). - Add coverage for the response-close branches that had none: the sync parse-failure release, the drainPage throwing-sink-over-throwing-close intersection, and both closeStagedPageQuietly swallow paths. Closes #167, #168, #203, #204, #205, #206
Building on the header-drop change, expose how loudly a transport surfaces a model-valid header it cannot encode (e.g. a name with an interior space, which OkHttp and the JDK client reject under their stricter RFC 7230 token grammar). Pure-verbose logging left the loss invisible at default log levels, and warning on every request floods the log when a hot path re-sets the same header. Neither extreme suits everyone, so make it the caller's choice via a new DroppedHeaderLogging enum: - PER_OCCURRENCE — WARN on every drop (a per-request audit trail). - ONCE_PER_HEADER — WARN the first time each header name is dropped by a transport, verbose thereafter. The default: visible without flooding. - VERBOSE_ONLY — never WARN; drops log only at verbose. Both transports expose it on their Builder and on the BYO create(client, ...) factory; RequestAdapter carries the policy and a per-(transport, header-name) dedup guard. The log level itself remains the operator's to filter via SLF4J — this only chooses the level and dedup the transport emits, which a level alone cannot express. The per-header decision is unit-tested directly (the transport test modules run on slf4j-nop and cannot capture emitted logs), with a Builder wiring smoke test per transport.
This was
linked to
issues
Jul 3, 2026
pagination: dedupe close-on-parse-failure, the iterator→Stream adapter, and maxPages validation
#204
Closed
…paths Header value validation was tightened to reject non-ASCII bytes, but the same validator gates the inbound path, so a response header carrying legal obs-text (RFC 7230 permits 0x80+ in a field-value, e.g. a Latin-1 Content-Disposition filename) was silently dropped from the parsed Response. The tightening was also justified as "no transport can send it", which is false for the JDK HttpClient (it serialises obs-text on the wire). - Add Headers.Builder.addUnsafeNonAscii: strict name, lenient value (control characters still rejected, non-ASCII permitted). Both response adapters use it when copying response headers, mirroring OkHttp's lenient read path, so legal obs-text values survive. - Keep outbound, caller-set value validation strict, matching OkHttp's field-value grammar; reword the docs to state that as a deliberate most-restrictive-transport policy rather than a claim about all transports. - Align MediaType's value check with the shared header-value predicate so a media type's rendered form always satisfies the outbound grammar. Also fixes three pagination/transport robustness issues: - parseOrClose no longer lets a throwing response close() mask the parse failure; the close error is attached as suppressed. - CloseablePages.stream() wraps the checked IOException from close() in UncheckedIOException instead of sneaky-throwing it undeclared through Stream.close(). - The transport dropped-header WARN dedup is gated on the WARN level being enabled (so a disabled level does not consume the one-shot) and bounded so a caller emitting unbounded distinct malformed names cannot grow it without limit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves the six open issues in the #167–#206 range (the rest were already closed as part of the earlier simplification sweep). Two themes: tightening header validation, and finishing the pagination-unification follow-ups.
Header validation (breaking)
Headers.BuilderandHttpHeaderName.fromStringpreviously accepted non-ASCII header names and values, rejecting only control characters. But both reference transports — OkHttp and the JDKHttpClient— reject non-ASCII, so a caller who set a non-ASCII header had it silently dropped from the wire on every transport, with only a per-request WARN log to show for it.Reject non-ASCII at the model layer (
HeaderValidation): names and values are now validated to printable ASCII at construction, so a header no transport can send fails fast withIllegalArgumentExceptioninstead of vanishing mid-dispatch. Pre-1.0 validation break; no public signatures change insdk-core, so itsapiCheckis unchanged.Make the transport dropped-header log policy configurable. After the tightening, the only header that still reaches the transport's drop path is a model-valid but non-token one (e.g. a name with an interior space, which OkHttp/JDK reject under their stricter RFC 7230 token grammar). How loudly that surfaces is now the caller's choice via a new
DroppedHeaderLoggingenum, exposed on both transports'Builderand their BYOcreate(client, …)factory:PER_OCCURRENCE— WARN on every drop (a per-request audit trail).ONCE_PER_HEADER(default) — WARN the first time each header name is dropped by a transport, verbose thereafter: visible at default log levels without flooding a hot path.VERBOSE_ONLY— never WARN; drops log only at verbose.The log level remains the operator's to filter via SLF4J; the policy only chooses the level and dedup the transport emits, which a level alone can't express.
Resolves #167, #168.
Pagination follow-ups
byPage().stream()short-circuited byfindFirst/limit/anyMatchleft the held page's liveResponseopen even under try-with-resources, because the stream had no close handler. The stream now registers the view'sclose()as itsonClose, so closing the stream releases the held page. (pagination: byPage().stream() strands the held page's connection on short-circuit #203)PaginationInternal.kt: the close-on-parse-failure invariant (parseOrClose, previously copy-pasted into the sync and async paginators), the iterator→Streamadapter (orderedStream), and themaxPagesguard (requirePositiveMaxPages, now called from all four constructors). No behavior change. (pagination: dedupe close-on-parse-failure, the iterator→Stream adapter, and maxPages validation #204)PaginationStrategy.parsereturnsPageInfo(notPage); a page delivered toforEachPageAsynchas its body typically already drained by the strategy;implementation-plan.md/refs-comparison.mdnow describe the three shipped strategies (no separate "token" strategy, noprev_cursor). (pagination: KDoc and docs still reference removed/renamed types after the unification #205)drainPagethrowing-sink-over-throwing-close intersection, and bothcloseStagedPageQuietlyswallow paths. (pagination: add coverage for the untested response-close branches #206)Testing
./gradlew :sdk-core:test :sdk-core:ktlintCheck :sdk-core:detekt :sdk-core:apiCheck, plus the equivalents forsdk-transport-okhttpandsdk-transport-jdkhttp, all pass locally. The transports' drop-not-throw tests were repurposed from a non-ASCII header (no longer constructible) to a model-valid non-token name (interior space) that the transports still reject. TheDroppedHeaderLoggingdecision is unit-tested directly onRequestAdapter(the transport test modules run onslf4j-nop, so emitted logs can't be captured), with aBuilderwiring smoke test per transport. New public API (DroppedHeaderLogging, the Builder setter, and thecreateoverloads) is captured viaapiDump.Closes #167, #168, #203, #204, #205, #206