Skip to content

Conversation

jonsmorrow
Copy link

This change updates the logic for xForwardedHeaders=true so that hostname instead of host is set in X-Forwarded-Host so that the port is never included. Additionally, port is set in X-Forwarded-Port.

This change updates the logic for xForwardedHeaders=true so that hostname instead of host
is set in X-Forwarded-Host so that the port is never included. Additionally, port is set in
X-Forwarded-Port.

Signed-off-by: Jon Morrow <[email protected]>
@jonsmorrow
Copy link
Author

I'm not 100% why the tests are failing for multipart-parser. They pass for me locally. They appear to pass in the workflow before the workflow fails.

@MichaelDeBoey MichaelDeBoey changed the title Use X-Forwarded-Port header when passing xForwardedHeaders option feat(fetch-proxy): use X-Forwarded-Port header when passing xForwardedHeaders option Oct 4, 2025
@MichaelDeBoey MichaelDeBoey requested a review from Copilot October 4, 2025 14:51
Copy link

@Copilot 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 change updates the X-Forwarded headers logic to properly separate hostname and port information when xForwardedHeaders is enabled. Previously, the port was included in the X-Forwarded-Host header, which is not the standard practice.

  • Updated X-Forwarded-Host to use url.hostname instead of url.host to exclude port information
  • Added X-Forwarded-Port header to properly communicate port information
  • Updated tests and documentation to reflect the new header behavior

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
packages/fetch-proxy/src/lib/fetch-proxy.ts Modified X-Forwarded headers logic to separate hostname and port
packages/fetch-proxy/src/lib/fetch-proxy.test.ts Updated test descriptions and assertions to verify new header behavior
packages/fetch-proxy/README.md Updated documentation to mention X-Forwarded-Port support

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

proxyHeaders.append('X-Forwarded-Proto', url.protocol.replace(/:$/, ''))
proxyHeaders.append('X-Forwarded-Host', url.host)
proxyHeaders.append('X-Forwarded-Host', url.hostname)
proxyHeaders.append('X-Forwarded-Port', url.port)
Copy link

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

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

The url.port property returns an empty string for default ports (80 for HTTP, 443 for HTTPS). This could result in an empty X-Forwarded-Port header value. Consider using url.port || (url.protocol === 'https:' ? '443' : '80') to provide the default port when none is explicitly specified.

Suggested change
proxyHeaders.append('X-Forwarded-Port', url.port)
proxyHeaders.append('X-Forwarded-Port', url.port || (url.protocol === 'https:' ? '443' : '80'))

Copilot uses AI. Check for mistakes.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants