Skip to content

fix: build HTTP transport explicitly to avoid HTTP/2 leak#443

Merged
jeffersonrodrigues92 merged 1 commit into
developfrom
fix/http2-tlsnextproto-clear
May 25, 2026
Merged

fix: build HTTP transport explicitly to avoid HTTP/2 leak#443
jeffersonrodrigues92 merged 1 commit into
developfrom
fix/http2-tlsnextproto-clear

Conversation

@brunobls
Copy link
Copy Markdown
Member

http.DefaultTransport can be mutated at boot by other libraries (notably OpenTelemetry registering an h2 handler in TLSNextProto). Cloning it inherited that handler, so HTTPS connections negotiated HTTP/2 via ALPN and hit the stdlib hpack encoder issue under the concurrent-goroutine usage pattern of this client — reproduced in production on 2026-04-10 (61 errors on reporter-manager, 14 on reporter-worker, circuit-breaker cascade, 503s).

Replace the Clone()-based construction with an explicit *http.Transport. TLSNextProto is initialized to a non-nil empty map (the stdlib opt-out signal). Proxy, DialContext, TLS, pool and timeout settings are now set explicitly, no longer inherited from a mutable global. HTTP/1.1 is the deliberate choice for this client because middleware and async revalidation share the same host concurrently.

Adds client_transport_test.go with regression tests that contaminate http.DefaultTransport with an h2 handler and assert it does not leak into the client, plus pinning tests for pool and timeout defaults.

Pull Request Checklist

Pull Request Type

  • Feature
  • Fix
  • Refactor
  • Pipeline
  • Tests
  • Documentation

Checklist

Please check each item after it's completed.

  • I have tested these changes locally.
  • I have updated the documentation accordingly.
  • I have added necessary comments to the code, especially in complex areas.
  • I have ensured that my changes adhere to the project's coding standards.
  • I have checked for any potential security issues.
  • I have ensured that all tests pass.
  • I have updated the version appropriately (if applicable).
  • I have confirmed this code is ready for review.

Additional Notes

Obs: Please, always remember to target your PR to develop branch instead of main.

…HTTP/2 leak

http.DefaultTransport can be mutated at boot by other libraries (notably
OpenTelemetry registering an h2 handler in TLSNextProto). Cloning it
inherited that handler, so HTTPS connections negotiated HTTP/2 via ALPN
and hit the stdlib hpack encoder issue under the concurrent-goroutine
usage pattern of this client — reproduced in production on 2026-04-10
(61 errors on reporter-manager, 14 on reporter-worker, circuit-breaker
cascade, 503s).

Replace the Clone()-based construction with an explicit *http.Transport.
TLSNextProto is initialized to a non-nil empty map (the stdlib opt-out
signal). Proxy, DialContext, TLS, pool and timeout settings are now set
explicitly, no longer inherited from a mutable global. HTTP/1.1 is the
deliberate choice for this client because middleware and async
revalidation share the same host concurrently.

Adds client_transport_test.go with regression tests that contaminate
http.DefaultTransport with an h2 handler and assert it does not leak
into the client, plus pinning tests for pool and timeout defaults.

Refs: docs/lib-commons/incidents/2026-04-10-http2-protocol-mismatch.md
Generated-by: Claude
AI-Model: claude-opus-4-7
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3a638b59-1015-480f-a1a3-67adcaa6b416

📥 Commits

Reviewing files that changed from the base of the PR and between 9fd63f7 and 7941e34.

📒 Files selected for processing (2)
  • commons/tenant-manager/client/client.go
  • commons/tenant-manager/client/client_transport_test.go

Walkthrough

The HTTP client factory function was refactored to construct a new explicit *http.Transport instead of cloning http.DefaultTransport, ensuring HTTP/1.1 enforcement via ForceAttemptHTTP2=false, transport instance isolation, and explicit connection pooling and timeout configuration. Three unit tests were added to verify transport isolation, HTTP/2 prevention, and expected default values.

Changes

Cohort / File(s) Summary
HTTP Client Transport Configuration
commons/tenant-manager/client/client.go
Refactored newDefaultHTTPClient() to build a new *http.Transport with explicit configuration including environment proxy support, custom dialer, HTTP/2 disabled via ForceAttemptHTTP2=false, empty TLSNextProto map, and defined connection pooling/timeout parameters. Added imports for crypto/tls and net.
Transport Test Suite
commons/tenant-manager/client/client_transport_test.go
Added three regression tests: TestNewDefaultHTTPClient_UsesHTTP1Only verifies HTTP/2 negotiation is blocked, TestNewDefaultHTTPClient_TransportIsIsolated confirms transport instance isolation from http.DefaultTransport, and TestNewDefaultHTTPClient_ExpectedDefaults pins expected client timeout and transport configuration values.

Comment @coderabbitai help to get the list of available commands and usage tips.

@lerian-studio
Copy link
Copy Markdown
Contributor

This PR has had no activity for 20 days and is now marked stale. It will be closed in 7 days unless updated. Add the no-stale label to keep it open indefinitely.

Learn more about the routine and exempt labels: https://github.com/LerianStudio/github-actions-shared-workflows/blob/main/instructions/repository-routines.md

@lerian-studio lerian-studio added the stale No recent activity — will be closed if not updated label May 25, 2026
@jeffersonrodrigues92 jeffersonrodrigues92 merged commit a39e5d9 into develop May 25, 2026
3 checks passed
@github-actions github-actions Bot deleted the fix/http2-tlsnextproto-clear branch May 25, 2026 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale No recent activity — will be closed if not updated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants