node: cover h2c upgrade route and TLS protocol negotiation - #22862
Merged
Conversation
StartHTTPEndpoint negotiates four protocol routes and only one of them was tested. Add the missing coverage: - TestHTTP2H2CUpgrade pins the HTTP/1.1 Upgrade route into h2c (RFC 7540 Section 3.2), which is what "curl --http2" uses over cleartext. The x/net/http2/h2c wrapper serves both this and prior knowledge, while the stdlib http.Protocols route (SetUnencryptedHTTP2) only recognizes the prior-knowledge preface, so swapping the wrapper for it would silently downgrade these clients to HTTP/1.1. - TestHTTPSEndpoint covers the ServeTLS path behind --https.enabled, which had no test at all: h2 negotiated via ALPN, plus the HTTP/1.1 fallback. The certificate is generated in-process, so no fixture lands on disk. Also drop the testing.Short guard on TestHTTP2H2C: it is labelled "slow test" but runs in well under a second, so it only hid the test from the short suite.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds missing test coverage around JSON-RPC listener protocol negotiation in StartHTTPEndpoint (the shared entry point for HTTP, HTTPS, WS, engine), without changing production behavior. This helps prevent regressions in HTTP/2 cleartext (h2c) upgrade handling and TLS ALPN negotiation.
Changes:
- Remove the
testing.Short()skip from the existing h2c (prior-knowledge) test so it runs in-short. - Add
TestHTTP2H2CUpgradeto pin the HTTP/1.1Upgrade: h2croute (curl--http2cleartext behavior). - Add
TestHTTPSEndpointplus an in-test self-signed cert helper to coverServeTLS, including HTTP/2 via ALPN and HTTP/1.1 fallback.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yperbasis
approved these changes
Jul 29, 2026
yperbasis
left a comment
Member
There was a problem hiding this comment.
Verified locally: -count=5 and -race green; replicating the geth-style protocol config (ethereum/go-ethereum#33812) makes both new tests fail as intended.
Two non-blocking nits:
- In
newSelfSignedCert,IsCA: trueis silently dropped from the encoded cert becauseBasicConstraintsValidis not set, andKeyUsageCertSignis unused for a leaf served underInsecureSkipVerify. Either drop both, or go further and pin the cert viaRootCAs— the 127.0.0.1 IP SAN is already there, and it would remove the//nolint:gosec. Pinning would also requireBasicConstraintsValid: true. TestHTTPSEndpointsets no client timeout, so a server hang would only surface at the go-test deadline. Same shape as the existingTestHTTP2H2C, so fine to leave as is.
Address review nits on the HTTPS endpoint test: - Verify the server certificate against a pool holding just that cert, which drops InsecureSkipVerify and its gosec suppression. The 127.0.0.1 IP SAN was already there, so no cert change is needed for the name check. IsCA and KeyUsageCertSign go away: they were dead on a leaf, and a self-signed cert in the root pool is accepted as a chain of length one without a basic-constraints check. - Give the client a timeout so a server hang surfaces there rather than at the go-test deadline.
Contributor
Author
|
@yperbasis Updated sw to solve non blocking issue |
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.
StartHTTPEndpointis the single entry point for every JSON-RPC listener(http, https, separate ws, engine) and negotiates four protocol routes.
Only one of them had a test. This PR adds the missing coverage; no
production code changes.
h2c upgrade route
TestHTTP2H2CUpgradepins the HTTP/1.1 Upgrade route into h2c (RFC 7540Section 3.2) — the one
curl --http2uses over cleartext, as opposed to--http2-prior-knowledge.Worth pinning because the two h2c routes are not interchangeable. The
x/net/http2/h2cwrapper we use serves both, while the stdlibhttp.Protocolsroute (SetUnencryptedHTTP2) only recognizes theprior-knowledge preface and has no handling for
Upgrade: h2c. Replacingthe wrapper with the stdlib API — as geth did in ethereum/go-ethereum#33812,
which is the upstream equivalent of our #19100 — would silently downgrade
these clients to HTTP/1.1. The test now catches that.
TLS endpoint
TestHTTPSEndpointcovers theServeTLSpath behind--https.enabled,which had no test at all: HTTP/2 negotiated via ALPN, plus the HTTP/1.1
fallback on the same listener. The certificate is generated in-process, so
no fixture lands on disk and no
git restore-mtimeentry is needed.Short-mode guard
Dropped the
testing.Short()guard onTestHTTP2H2C: it is labelled"slow test" but runs in well under a second, so it only hid the test from
the
-shortsuite.Testing
-count=3with no flakes.vacuously: applying geth's exact protocol config (
SetHTTP1+SetUnencryptedHTTP2) toStartHTTPEndpointmakesTestHTTPSEndpointfail, and dropping the
h2cwrapper makesTestHTTP2H2CUpgradefail.go test ./node/... -shortgreen,make lintclean.Test-only change, so the Red → Green cycle does not apply: these pin
existing behavior that was untested.