Skip to content

reverseproxy: Fix check for header_up Host {upstream_hostport} redundancy#7564

Open
yubiuser wants to merge 3 commits intocaddyserver:masterfrom
yubiuser:fix/hostport
Open

reverseproxy: Fix check for header_up Host {upstream_hostport} redundancy#7564
yubiuser wants to merge 3 commits intocaddyserver:masterfrom
yubiuser:fix/hostport

Conversation

@yubiuser
Copy link
Copy Markdown

PR #7454 changed the header from header_up Host {hostport} to header_up Host {upstream_hostport} when reverse_proxy for HTTPS.

However, the PR forgot to change a parser check to the new default. This results in logspam when users revert to the old behavior setting header_up Host {hostport}. Caddy starts to warn about unnecessary header_up being doubled - while this is not true anymore.

2026-03-12_10-07

No AI was used.

Signed-off-by: yubiuser <github@yubiuser.dev>
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 12, 2026

CLA assistant check
All committers have signed the CLA.

@francislavoie
Copy link
Copy Markdown
Member

The change is not quite right, it needs to take into account that hostport is only passed for HTTPS and not HTTP.

Signed-off-by: yubiuser <github@yubiuser.dev>
@yubiuser
Copy link
Copy Markdown
Author

Thanks, you're correct. I've added a commit that checks if the commonScheme is https and only in this case performs the check.

Copy link
Copy Markdown
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

This LGTM. Any other changes needed @francislavoie ?

@francislavoie
Copy link
Copy Markdown
Member

It's technically not whether the scheme is https but rather if TLS is enabled on the transport which can happen due to a few different transport options.

@mholt
Copy link
Copy Markdown
Member

mholt commented Mar 23, 2026

Ok yeah that's a good point. @yubiuser how would you feel about changing that?

@yubiuser
Copy link
Copy Markdown
Author

I'll look into it, but I'm not familiar with the code base so I might miss some of the possible transport options to enable TLS.

@francislavoie
Copy link
Copy Markdown
Member

Basically the logic would need to be moved around line 884 where (after the big switch) when the TLS transport stuff is being handled, where it would then need to consult the h.Headers.Request to see if the config mistake was made. A bit awkward, but it's the most correct way to handle it I think.

Signed-off-by: yubiuser <github@yubiuser.dev>
@yubiuser
Copy link
Copy Markdown
Author

Thanks for the hint. I moved the check into the TLS section and used h.Headers.Request.Set to check if the users wanted to add Host upstream_hostport.

Do you want me to squash the commits into a single one?

@yubiuser
Copy link
Copy Markdown
Author

CI test fail due to govulncheck now detects https://pkg.go.dev/vuln/GO-2026-4775 (published yesterday) and golangci-lint was updated to v2.11.4 on 22th or March.

Should those issues be addressed in a separate PR?

@francislavoie
Copy link
Copy Markdown
Member

Thanks!

No need to squash, we'll squash on merge. We'll fix the lint separately then rebase and merge this when we're ready.

@francislavoie francislavoie changed the title Fix check for header_up reverseproxy: Fix check for header_up Host {upstream_hostport} redundancy Mar 24, 2026
@francislavoie francislavoie added the bug 🐞 Something isn't working label Mar 24, 2026
@francislavoie francislavoie added this to the v2.11.3 milestone Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 🐞 Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants