Skip to content

fix: support Host header rewrite - #523

Merged
minhtule merged 2 commits into
masterfrom
feat/mt/webapp-host-rewrite
Sep 10, 2026
Merged

fix: support Host header rewrite#523
minhtule merged 2 commits into
masterfrom
feat/mt/webapp-host-rewrite

Conversation

@minhtule

@minhtule minhtule commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Changes

  • Host in webApp.requestHeaders or a GAT request header rewrite now replaces the Host header sent to the upstream
    • net/http ignores a Host entry in the header map and sends Request.Host instead, so the rewrite was silently dropped

Copilot AI lite review requested due to automatic review settings September 8, 2026 23:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 WebApp upstream header rewriting so that rewriting Host (via webApp.requestHeaders or GAT RequestHeaderRewrites) actually changes the Host/:authority sent by Go’s net/http transport, by writing to http.Request.Host instead of the header map.

Changes:

  • Route all outbound header rewrites through a new helper that special-cases Host by setting r.Out.Host.
  • Apply the same Host handling to both config-driven and GAT-provided request header rewrites.
  • Add unit tests verifying config Host rewrites, GAT overrides, and case-insensitive (host) handling.

Reviewed changes

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

File Description
internal/webapphandler/handler.go Introduces setHeader() and uses it to ensure Host rewrites affect r.Out.Host (what net/http actually sends upstream).
internal/webapphandler/handler_test.go Extends TestRewrite to validate Host rewrite behavior (config, GAT override, lowercase key).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

net/http ignores a Host entry in the header map and sends Request.Host
instead, so a Host rewrite from config or the GAT was silently dropped.
Copilot AI review requested due to automatic review settings September 9, 2026 00:03
@minhtule
minhtule force-pushed the feat/mt/webapp-host-rewrite branch from e10e719 to 41d0f77 Compare September 9, 2026 00:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

@minhtule
minhtule requested a review from clement0010 September 9, 2026 00:06
@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.71%. Comparing base (47bfdd0) to head (7295c22).
⚠️ Report is 13 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
- Coverage   93.74%   93.71%   -0.04%     
==========================================
  Files          42       42              
  Lines        2684     2687       +3     
==========================================
+ Hits         2516     2518       +2     
- Misses        167      168       +1     
  Partials        1        1              
Flag Coverage Δ
integration 64.63% <80.00%> (-0.04%) ⬇️
unit 88.64% <100.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
internal/webapphandler/handler.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@clement0010 clement0010 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍🏻

@minhtule
minhtule requested a review from sghiocel September 9, 2026 04:15
func setHeader(r *httputil.ProxyRequest, name, value string) {
switch http.CanonicalHeaderKey(name) {
case "Host":
r.Out.Host = value

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We have the following comment on line 124-126:

	// Per-resource request header rewrites from the GAT are applied last, so they override
	// any config headers with the same name. Malformed or unsupported headers are skipped
	// rather than failing the request.

So for GAT rewrites, we expect to proceed with the request if the header is malformed. From what I understand though the Host header is under additional validation:

HTTP/1.1: https://github.com/golang/go/blob/go1.27.0/src/net/http/request.go#L631
(changes it into an empty string "" if it does not validate)

HTTP/2:
https://github.com/golang/go/blob/go1.27.0/src/net/http/internal/httpcommon/httpcommon.go#L235-L237 (fails the request with 502)

So the question is, do we want to preserve this behavior? Do we want to validate the Host header ourselves up front and for Config rewrites, have our own error, and for GAT try to skip as we did before? Or are we OK with empty Host or 502 in this invalid cases (if it includes invalid symbols like @ / # etc)

@minhtule minhtule Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the comment is a bit misleading. We only skip the header if the template fails to parse or evaluate (updated in 7295c22). We never validate the header name and value according to the RFCs. Go would do those validations and fail the request instead.

Copilot AI review requested due to automatic review settings September 10, 2026 00:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/webapphandler/handler.go:155

  • When rewriting the Host header, setHeader updates r.Out.Host but leaves any existing Host entry in r.Out.Header. Even though net/http ignores Header["Host"] when writing the request, keeping it can create an inconsistent state for any later code/logging that inspects r.Out.Header. Clear the header-map entry when setting r.Out.Host.

@minhtule
minhtule merged commit 33d9400 into master Sep 10, 2026
14 checks passed
@minhtule
minhtule deleted the feat/mt/webapp-host-rewrite branch September 10, 2026 00:59
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.

4 participants