Skip to content

Conversation

@sapphi-red
Copy link
Contributor

Requests from HTTP2 had a different header values for Host and X-Forward-* from requests from HTTP 1.x. This PR fixes that inconsistency.

related vitejs/vite#21117

@Corvince
Copy link

At first I was happy to have contributed (indirectly) to vite, but the responsibility certainly feels different 😆

Feel free to tag me in the future for issues related to http2, I wasn't aware of the scale of the problem, only saw the forward header briefly mentioned in #2 and fixed in #42 (which does not include the other fixes present in this PR).

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes inconsistencies in the Host and X-Forwarded-* headers when proxying requests from HTTP/2 to HTTP/1. The fix ensures that the HTTP/2 :authority pseudo-header is properly converted to the Host header for HTTP/1 backends.

  • Adds support for HTTP/2 :authority pseudo-header across the proxy pipeline
  • Updates header handling in setupOutgoing, XHeaders, getPort, and setRedirectHostRewrite functions
  • Includes comprehensive test coverage for HTTP/2 to HTTP/1 proxy scenarios

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
lib/http-proxy/common.ts Maps HTTP/2 :authority header to host header for outgoing HTTP/1 requests and updates port extraction logic
lib/http-proxy/passes/web-incoming.ts Uses :authority as fallback for x-forwarded-host header in HTTP/2 requests
lib/http-proxy/passes/web-outgoing.ts Supports :authority header in redirect host rewrite logic for HTTP/2 requests; removes trailing whitespace
lib/test/lib/http2-proxy.test.ts Adds new integration tests for HTTP/2 to HTTP/1 proxy scenarios using both proxy server and custom HTTP/2 server
lib/test/lib/http-proxy-passes-web-incoming.test.ts Adds unit test verifying correct x-forwarded-* header handling for HTTP/2 requests
lib/test/lib/http-proxy-passes-web-outgoing.test.ts Adds unit test for redirect host rewrite with HTTP/2 :authority header

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

});
req.on('end', () => {
source.close();
proxy.close();
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

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

The HTTP2 client connection should be closed to prevent resource leaks. Add client.close() before calling done(). For example:

req.on('end', () => {
  source.close();
  proxy.close();
  client.close();
  done();
});
Suggested change
proxy.close();
proxy.close();
client.close();

Copilot uses AI. Check for mistakes.
});
req.on('end', () => {
source.close();
ownServer.close();
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

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

The HTTP2 client connection should be closed to prevent resource leaks. Add client.close() before calling done(). For example:

req.on('end', () => {
  source.close();
  ownServer.close();
  client.close();
  done();
});
Suggested change
ownServer.close();
ownServer.close();
client.close();

Copilot uses AI. Check for mistakes.
@williamstein williamstein mentioned this pull request Nov 29, 2025
@williamstein williamstein merged commit aac3d2b into sagemathinc:main Nov 29, 2025
9 checks passed
@williamstein
Copy link
Contributor

@sapphi-red sorry about that. I've made a release including this -- 1.23.1 .

@williamstein
Copy link
Contributor

Regarding the copilot AI review, I think they are useful as a sort of linter. In this case, I don't care about potential leaks in unit tests, which is all it flagged.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants