Skip to content

Fix HTTP header value handling in client-v2 - #3228

Open
Gurumote wants to merge 4 commits into
ClickHouse:mainfrom
Gurumote:main
Open

Gurumote wants to merge 4 commits into
ClickHouse:mainfrom
Gurumote:main

Conversation

@Gurumote

@Gurumote Gurumote commented Oct 19, 2026 •

Copy link
Copy Markdown

Closes #3227
Summary

This PR addresses an inconsistency in HTTP header value validation within client-v2 that results in configured header values being transmitted under an alternative header name and subsequently ignored by the ClickHouse server.

The existing PATTERN_HEADER_VALUE_ASCII expression restricts whitespace between visible ASCII characters to a single space. Consequently, otherwise representable values containing consecutive spaces or internal tabs are incorrectly classified and subjected to the Header-Name* encoding mechanism, resulting in silent loss of the original header.
Changes Introduced

Refined ASCII Header Value Validation: Modified PATTERN_HEADER_VALUE_ASCII to accommodate consecutive spaces and internal tabs within printable ASCII header values, ensuring that supported values are transmitted using their original header names.

Explicit Invalid Value Handling: Introduced explicit exception handling for header values that cannot be represented safely using the supported transmission mechanism, preventing silent fallback to an unsupported encoding format.

Verification

Validate header values containing single and consecutive spaces.

Verify the handling of internal tab characters.

Confirm that unsupported values result in a descriptive ClientException.

Ensure that existing valid header values retain their established behavior.

Compatibility Considerations

This change corrects the transmission of previously mishandled header values. Values that cannot be represented through the supported mechanism will fail explicitly rather than being transmitted under an alternative header name that the server does not recognize.


Note

Medium Risk
Header behavior changes fix silent drops but may fail requests that previously sent mis-encoded headers; User-Agent composition and response-close semantics also changed in the HTTP transport path.

Overview
Fixes client-v2 HTTP custom header handling in HttpAPIClientHelper.setHeader: PATTERN_HEADER_VALUE_ASCII now treats printable ASCII with multiple spaces or tabs between tokens as valid, so values go out on the real header name instead of being rewritten to Header-Name* (which ClickHouse ignored).

Invalid header values no longer get RFC 5987-style UTF-8''… encoding; the client throws a ClientException with a clear message when a value is outside the supported ASCII rules (e.g. non-ASCII), avoiding silent misconfiguration.

The same class also drops WorkloadIdentitySupport from the helper constructor and builds a cached defaultUserAgent locally (buildDefaultUserAgent / correctUserAgentHeader using CLIENT_NAME and Client.CLIENT_USER_AGENT). TransportResponseImpl no longer wraps the response body in ResponseBodyInputStream or swallows ConnectionClosedException on close—it uses the entity stream directly.

Reviewed by Cursor Bugbot for commit 59f440a. Bugbot is set up for automated code reviews on this repo. Configure here.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

} catch (Exception e) {
throw new ClientException("Failed to construct input stream", e);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Response close handling accidentally removed

High Severity

TransportResponseImpl no longer swallows ConnectionClosedException when a response or its body stream is closed after a partial read. Closing now surfaces that drain failure, so QueryResponse.close() can throw ClientException after a mid-stream server abort even when the caller already stopped reading. This is unrelated to the header-value fix and breaks the existing close-after-partial-read path.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit e6e7dff. Configure here.

@chernser

Copy link
Copy Markdown
Contributor

@Gurumote

Thanks!
please revert line ending so difference is visible.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 59f440a. Configure here.

LOG.warn("failed to construct http client version string", e);
}
return userAgent.toString();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User-Agent identity support removed

High Severity

correctUserAgentHeader rebuilds a local defaultUserAgent instead of using WorkloadIdentitySupport.getUserAgent. CLIENT_ID_METADATA is omitted, updateClientName no longer changes the sent header, and the format no longer matches lv/os/hc:ahc_ that tests and ClickHouse client identity expect.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 59f440a. Configure here.

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

Labels

None yet

Projects

None yet

3 participants