Conversation
SukkaW
force-pushed
the
http2-support
branch
from
ddb62fb to
9a92849
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #102 +/- ##
==========================================
+ Coverage 96.47% 96.57% +0.09%
==========================================
Files 8 8
Lines 596 613 +17
Branches 222 230 +8
==========================================
+ Hits 575 592 +17
Misses 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@pi0 I believe this PR is ready for review now. All tests passed on my machine locally 
|
SukkaW
changed the title
There was a problem hiding this comment.
Pull request overview
Adds initial HTTP/2 (h2) listener support to the proxy server as a step toward issue #38 (HTTP2/3 support), including middleware updates to handle HTTP/2-specific header semantics and new integration tests.
Changes:
- Introduce
http2?: booleanoption and create an HTTP/2 secure listener (withallowHTTP1) when enabled. - Update incoming/outgoing middleware + core utils to handle HTTP/2 pseudo-headers (
:authority) and remove HTTP/2-incompatible hop-by-hop headers. - Add HTTP/2 integration tests (via
undiciwith h2-enabled agent) and extend stubs/tests forhttpVersionMajor.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/middleware/web-outgoing.test.ts | Adds coverage for :authority redirect rewriting and HTTP/2 connection-header behavior. |
| test/https-proxy.test.ts | Exports helper functions for listening (currently used by new HTTP/2 tests). |
| test/http2-proxy.test.ts | New HTTP/2 listener integration tests (HTTP/2+HTTP/1.1 clients over TLS). |
| test/_stubs.ts | Enhances IncomingMessage stubs with httpVersion/httpVersionMajor; tweaks ProxyServer stub typing. |
| src/types.ts | Adds http2 option and documents SSL usage for HTTP/2 listener. |
| src/server.ts | Implements HTTP/2 secure server creation path; broadens request/response typings to include HTTP/2 compat types. |
| src/middleware/web-outgoing.ts | Removes HTTP/2-incompatible headers (e.g., connection), supports :authority for redirects/status handling. |
| src/middleware/web-incoming.ts | Uses :authority as a fallback for x-forwarded-host. |
| src/middleware/_utils.ts | Extends middleware typing to accept HTTP/2 request/response types (with one remaining generic constraint issue). |
| src/_utils.ts | Filters HTTP/2 pseudo-headers from outgoing request options and uses :authority for host/port derivation. |
| package.json | Adds undici dev dependency for HTTP/2-capable fetch in tests. |
| pnpm-lock.yaml | Locks undici addition. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (4)
src/server.ts:85
- When
http2is enabled, the code always creates a TLS server viahttp2.createSecureServer(...), butsslis still optional inProxyServerOptions. If a user setshttp2: truewithout providing TLS credentials, the server will fail at runtime; consider validatingoptions.ssl(or requiring it in types) and throwing a clear error early.
if (this.options.http2) {
this._server = http2.createSecureServer({ ...this.options.ssl, allowHTTP1: true }, closure);
} else if (this.options.ssl) {
this._server = https.createServer(this.options.ssl, closure);
src/server.ts:43
_webPassesis still typed asProxyMiddleware<http.ServerResponse>[], but whenhttp2is enabled therespassed through the passes can behttp2.Http2ServerResponse. This mismatch is currently papered over with casts in_getPasses/listen, reducing type safety and making it easy for a pass to accidentally use HTTP/1-only APIs. Consider widening the_webPassestype (and the middleware list types) to includeHttp2ServerResponseso incompatibilities are caught by typecheck.
_webPasses: ProxyMiddleware<http.ServerResponse>[] = [...webIncomingMiddleware];
_wsPasses: ProxyMiddleware<net.Socket>[] = [...websocketIncomingMiddleware];
test/http2-proxy.test.ts:25
undiciAgentinstances keep connection pools open; without closing them, the test runner can hang or report open handles. Add a top-levelafterAlltoclose()/destroy()bothhttp1Agentandhttp2Agentonce the suite completes.
const http1Agent = new Agent({
allowH2: false,
connect: {
// Allow to use SSL self signed
rejectUnauthorized: false,
},
});
const http2Agent = new Agent({
allowH2: true,
connect: {
// Allow to use SSL self signed
rejectUnauthorized: false,
},
});
src/middleware/_utils.ts:29
defineProxyMiddleware's generic constraint is stillT extends ServerResponse | Socket, butProxyMiddlewarenow supportsHttp2ServerResponseas well. This makes it impossible to declare a middleware explicitly typed asProxyMiddleware<Http2ServerResponse>(it won't satisfy the constraint) and is inconsistent with the updatedProxyMiddlewaretype. WidendefineProxyMiddleware's constraint to includeHttp2ServerResponse.
export function defineProxyMiddleware<T extends ServerResponse | Socket = ServerResponse>(
m: ProxyMiddleware<T>,
) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughThis pull request introduces HTTP/2 support to the proxy library by adding type-aware request/response handling, HTTP/2 pseudo-header management, conditional server creation with HTTP/2 flags, and comprehensive test coverage including new HTTP/2 integration tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Proxy as HTTP/2 Proxy
participant Target as Target Server
Client->>Proxy: HTTP/2 Request (with :authority pseudo-header)
Note over Proxy: Extract authority header<br/>Identify HTTP/2 (httpVersionMajor > 1)<br/>Strip pseudo-headers from outgoing
Proxy->>Proxy: setupOutgoing:<br/>Copy :authority → host<br/>Strip HTTP/2 pseudo-headers
Proxy->>Target: HTTP/1.1 Request (host header)
Target-->>Proxy: Response (statusCode, headers)
Note over Proxy: setConnection: Delete for HTTP/2+<br/>writeStatusCode: Skip statusMessage for HTTP/2+<br/>setRedirectHostRewrite: Prefer :authority
Proxy-->>Client: HTTP/2 Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
test/_stubs.ts (1)
60-61: Minor type inconsistency in return cast.The return type is
ProxyServer<any, any>but the cast on line 61 uses the unparameterizedProxyServer. This works due to theas unknownintermediate cast but could be slightly cleaner.Optional fix for type consistency
export function stubProxyServer(overrides: Record<string, unknown> = {}): ProxyServer<any, any> { - return overrides as unknown as ProxyServer; + return overrides as unknown as ProxyServer<any, any>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/_stubs.ts` around lines 60 - 61, The stubProxyServer function returns ProxyServer<any, any> but currently casts to the raw ProxyServer type; update the return cast to match the declared generic parameters—change the final cast to "as unknown as ProxyServer<any, any>" in the stubProxyServer function so the returned type is consistently ProxyServer<any, any>.test/http2-proxy.test.ts (1)
87-91: Consider awaitingsource.close()for proper cleanup.
source.close()(forhttp.Server/https.Server) is asynchronous and accepts an optional callback. While it often works without awaiting, for deterministic test cleanup, consider wrapping it in a Promise or using a callback to ensure the server is fully closed before the test suite ends.♻️ Optional: Await source close for consistent cleanup
afterAll(async () => { // cleans up await new Promise<void>((resolve) => proxy.close(resolve)); - source.close(); + await new Promise<void>((resolve, reject) => + source.close((err) => err ? reject(err) : resolve()) + ); });Also applies to: 161-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/http2-proxy.test.ts` around lines 87 - 91, The afterAll cleanup currently calls source.close() without awaiting its completion; change it to await the server close by wrapping source.close in a Promise (or using its callback) so the test waits for full shutdown, similar to how proxy.close(resolve) is awaited; update all occurrences (e.g., the afterAll blocks around source.close at the shown locations) to await the Promise-returning close to ensure deterministic teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 56: The package entry for undici is fine but HTTP/2 is opt-in; when you
create or configure undici's Agent or Client (Agent, Client) for HTTP/2 tests
set the allowH2 option to true (e.g., pass allowH2: true to Agent/Client
constructor or options) so HTTP/2 is actually enabled, and optionally bump the
package version to 7.22.0 if you want the latest patch release.
In `@src/server.ts`:
- Around line 82-88: The server currently calls http2.createSecureServer when
this.options.http2 is true but does not validate that this.options.ssl is
provided; add a runtime check in the server initialization (before calling
http2.createSecureServer) that if this.options.http2 is true and
this.options.ssl is falsy, throw or reject with a clear error explaining that
HTTP/2 requires SSL (referencing ProxyServerOptions in the message), otherwise
proceed to call http2.createSecureServer; ensure the check is placed alongside
the existing branch that inspects this.options.http2 / this.options.ssl so the
error occurs early with a helpful message rather than a cryptic
createSecureServer failure.
In `@test/_utils.ts`:
- Around line 16-27: The proxyListen helper can miss synchronous errors because
proxy.listen(0, "127.0.0.1") is called before attaching the server error
listener; to fix, in proxyListen wrap the call to proxy.listen(...) in a
try/catch and immediately reject on synchronous throw, then grab (proxy as
any)._server and attach server.once("error", reject) and
server.once("listening", ...) as before; keep the existing resolve logic in the
"listening" handler for port resolution.
---
Nitpick comments:
In `@test/_stubs.ts`:
- Around line 60-61: The stubProxyServer function returns ProxyServer<any, any>
but currently casts to the raw ProxyServer type; update the return cast to match
the declared generic parameters—change the final cast to "as unknown as
ProxyServer<any, any>" in the stubProxyServer function so the returned type is
consistently ProxyServer<any, any>.
In `@test/http2-proxy.test.ts`:
- Around line 87-91: The afterAll cleanup currently calls source.close() without
awaiting its completion; change it to await the server close by wrapping
source.close in a Promise (or using its callback) so the test waits for full
shutdown, similar to how proxy.close(resolve) is awaited; update all occurrences
(e.g., the afterAll blocks around source.close at the shown locations) to await
the Promise-returning close to ensure deterministic teardown.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
package.jsonsrc/_utils.tssrc/middleware/_utils.tssrc/middleware/web-incoming.tssrc/middleware/web-outgoing.tssrc/server.tssrc/types.tstest/_stubs.tstest/_utils.tstest/http-proxy.test.tstest/http2-proxy.test.tstest/https-proxy.test.tstest/middleware/web-incoming.test.tstest/middleware/web-outgoing.test.ts
| "sse": "^0.0.8", | ||
| "typescript": "^5.9.3", | ||
| "unbuild": "^3.6.1", | ||
| "undici": "^7.21.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if undici 7.21.0 exists and get latest version
echo "=== Checking undici versions ==="
curl -s https://registry.npmjs.org/undici | jq '.dist-tags | {latest, latest_v7: (.["7.21.0"] // "not found")}'
# Get detailed info about 7.21.0
curl -s https://registry.npmjs.org/undici/7.21.0 | jq '{version: .version, name: .name, deprecated: .deprecated}'
# Get latest version info
curl -s https://registry.npmjs.org/undici/latest | jq '{version: .version, name: .name, deprecated: .deprecated}'Repository: unjs/httpxy
Length of output: 386
🏁 Script executed:
#!/bin/bash
# Check for known vulnerabilities in undici
echo "=== Checking for security advisories ==="
curl -s https://registry.npmjs.org/-/npm/v1/security/advisories?package=undici | jq '.advisories // empty'
# Alternative: check npm audit for this package
npm view undici@7.21.0 --json | jq '{version: .version, keywords: .keywords[0:5], description: .description}'Repository: unjs/httpxy
Length of output: 282
🌐 Web query:
undici HTTP/2 support Node.js
💡 Result:
Undici (and therefore Node.js’s built-in fetch) is primarily an HTTP/1.1 client, but it does have HTTP/2 client support as an experimental, opt-in feature. [1] [2]
How to use HTTP/2 with undici
Enable it by using an Agent/Client with allowH2: true (HTTP/2 is negotiated via ALPN). The docs also note constraints like “TLS-only” and “h2c is not supported” (so in practice you typically need https:// + a server that negotiates h2). [2]
import { Agent, setGlobalDispatcher, fetch } from 'undici'
setGlobalDispatcher(new Agent({ allowH2: true }))
const res = await fetch('https://example.com/')
console.log(res.status)Node.js built-in fetch and HTTP/2
Node’s built-in fetch() uses the undici version bundled with your Node release, and HTTP/2 is not enabled by default—there’s an open request to support HTTP/2 by default in fetch. [3] [4]
Sources: [1] undici repo README, [2] undici Client API docs (UNPKG) incl. allowH2 + HTTP/2 notes, [3] undici discussion mentioning enabling H2 via allowH2: true for fetch, [4] undici issue requesting HTTP/2-by-default for fetch.
LGTM - undici is suitable for HTTP/2 testing with proper configuration.
The undici 7.21.0 package is stable and has no known security vulnerabilities. However, HTTP/2 support is an experimental, opt-in feature that requires explicit configuration (allowH2: true on the Agent/Client). As a devDependency, it won't affect production bundles. Note that version 7.22.0 is available if you want to stay current with the latest patch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 56, The package entry for undici is fine but HTTP/2 is
opt-in; when you create or configure undici's Agent or Client (Agent, Client)
for HTTP/2 tests set the allowH2 option to true (e.g., pass allowH2: true to
Agent/Client constructor or options) so HTTP/2 is actually enabled, and
optionally bump the package version to 7.22.0 if you want the latest patch
release.
| if (this.options.http2) { | ||
| this._server = http2.createSecureServer({ ...this.options.ssl, allowHTTP1: true }, closure); | ||
| } else if (this.options.ssl) { | ||
| this._server = https.createServer(this.options.ssl, closure); | ||
| } else { | ||
| this._server = http.createServer(closure); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the server.ts file around the mentioned lines
cat -n src/server.ts | sed -n '1,100p'Repository: unjs/httpxy
Length of output: 4435
🏁 Script executed:
# Check for any validation around http2 and ssl options
rg -n "http2|ssl" src/server.ts -B2 -A2 | head -100Repository: unjs/httpxy
Length of output: 2305
🏁 Script executed:
# Check the type definitions and options interface
rg -n "options\." src/server.ts | head -30Repository: unjs/httpxy
Length of output: 450
🏁 Script executed:
# Look for any tests that show how http2 is used
fd -e test.ts -e test.js -e spec.ts | xargs rg -l "http2" 2>/dev/null | head -5Repository: unjs/httpxy
Length of output: 79
🏁 Script executed:
# Check the test file to see how http2 is used
cat -n test/http2-proxy.test.tsRepository: unjs/httpxy
Length of output: 6341
🏁 Script executed:
# Check the ProxyServerOptions type definition
rg -n "ProxyServerOptions" src/types.ts -A20 | head -60Repository: unjs/httpxy
Length of output: 793
🏁 Script executed:
# Look for any comments or documentation about http2 requirement
rg -n "http2|HTTP/2" --type=ts -C2 | head -100Repository: unjs/httpxy
Length of output: 5729
HTTP/2 requires SSL configuration, which is already documented but lacks runtime validation.
The implementation uses http2.createSecureServer which requires SSL options. While the ProxyServerOptions JSDoc already documents this requirement ("Object to be passed to https.createServer() or http2.createSecureServer() if the http2 option is enabled"), there is no runtime validation. If a user sets http2: true without providing ssl, the code will fail at runtime with a cryptic error from createSecureServer.
Adding validation would prevent configuration mistakes:
Proposed runtime validation
if (this.options.http2) {
+ if (!this.options.ssl) {
+ throw new Error("HTTP/2 requires SSL configuration (options.ssl)");
+ }
this._server = http2.createSecureServer({ ...this.options.ssl, allowHTTP1: true }, closure);
} else if (this.options.ssl) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (this.options.http2) { | |
| this._server = http2.createSecureServer({ ...this.options.ssl, allowHTTP1: true }, closure); | |
| } else if (this.options.ssl) { | |
| this._server = https.createServer(this.options.ssl, closure); | |
| } else { | |
| this._server = http.createServer(closure); | |
| } | |
| if (this.options.http2) { | |
| if (!this.options.ssl) { | |
| throw new Error("HTTP/2 requires SSL configuration (options.ssl)"); | |
| } | |
| this._server = http2.createSecureServer({ ...this.options.ssl, allowHTTP1: true }, closure); | |
| } else if (this.options.ssl) { | |
| this._server = https.createServer(this.options.ssl, closure); | |
| } else { | |
| this._server = http.createServer(closure); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server.ts` around lines 82 - 88, The server currently calls
http2.createSecureServer when this.options.http2 is true but does not validate
that this.options.ssl is provided; add a runtime check in the server
initialization (before calling http2.createSecureServer) that if
this.options.http2 is true and this.options.ssl is falsy, throw or reject with a
clear error explaining that HTTP/2 requires SSL (referencing ProxyServerOptions
in the message), otherwise proceed to call http2.createSecureServer; ensure the
check is placed alongside the existing branch that inspects this.options.http2 /
this.options.ssl so the error occurs early with a helpful message rather than a
cryptic createSecureServer failure.
There was a problem hiding this comment.
Currently, the option.ssl is typed as any, and I don't want to touch it right now. Implementing a complex types that read allowH2 and then determine ssl type could be a breaking change.
There was a problem hiding this comment.
@SukkaW, I understand your concern about type complexity and breaking changes. Just to clarify, the suggestion was for runtime validation (a simple if check that throws an error) rather than type-level enforcement, so it wouldn't require changing the ssl: any type or introducing conditional types.
However, I completely respect your decision to keep the implementation as-is. The JSDoc already documents the requirement clearly, and users who set http2: true without SSL will get an error from Node.js anyway (though it will be less explicit). This is a reasonable trade-off to avoid additional complexity.
Feel free to proceed with the current implementation!
| export function proxyListen( | ||
| proxy: ReturnType<typeof httpProxy.createProxyServer>, | ||
| ): Promise<number> { | ||
| return new Promise((resolve, reject) => { | ||
| proxy.listen(0, "127.0.0.1"); | ||
| const server = (proxy as any)._server as net.Server; | ||
| server.once("error", reject); | ||
| server.once("listening", () => { | ||
| resolve((server.address() as AddressInfo).port); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Potential race condition: error listener attached after listen() call.
The error listener on line 22 is attached after proxy.listen() is called on line 20. If the server emits an error synchronously or very quickly, the reject handler may not be registered in time.
Proposed fix to attach error listener before listen
export function proxyListen(
proxy: ReturnType<typeof httpProxy.createProxyServer>,
): Promise<number> {
return new Promise((resolve, reject) => {
- proxy.listen(0, "127.0.0.1");
const server = (proxy as any)._server as net.Server;
+ proxy.listen(0, "127.0.0.1");
+ // Note: _server is created inside listen(), so we must access it after
server.once("error", reject);
server.once("listening", () => {
resolve((server.address() as AddressInfo).port);
});
});
}Actually, since _server is created inside listen(), the current order is necessary. However, this means errors during initial binding could be missed. Consider wrapping in a try-catch or accepting this as a test utility limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/_utils.ts` around lines 16 - 27, The proxyListen helper can miss
synchronous errors because proxy.listen(0, "127.0.0.1") is called before
attaching the server error listener; to fix, in proxyListen wrap the call to
proxy.listen(...) in a try/catch and immediately reject on synchronous throw,
then grab (proxy as any)._server and attach server.once("error", reject) and
server.once("listening", ...) as before; keep the existing resolve logic in the
"listening" handler for port resolution.
There was a problem hiding this comment.
very quickly
It is impossible. That's not how JavaScript or the event loop works.
synchronously
What's the point anyway? Throwing in new Promise() will result in an automatic rejection.
There was a problem hiding this comment.
@SukkaW You're absolutely right on both points. I apologize for the confusion.
The event loop guarantees that all synchronous code within the Promise executor completes before any asynchronous events can be processed, so the error and listening listeners will always be attached before those events can fire. Additionally, any synchronous throw within the Promise executor is automatically converted to a rejection by the Promise constructor.
The code is correct as written. Thank you for the clarification!
First step toward #38 with HTTP/2 server/listener support.
Summary by CodeRabbit
New Features
http2configuration option to enable HTTP/2 listeners alongside HTTP/1.x compatibility.Tests
Chores