Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: proxy_set_header Host $host may lose port info #2079

Merged
merged 2 commits into from
Jan 27, 2021

Conversation

halfcrazy
Copy link
Contributor

@halfcrazy halfcrazy commented Aug 19, 2020

What this PR does / why we need it:

proxy_set_header Host $host may lose port info, use http_host instead.

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible?

@halfcrazy halfcrazy changed the title fix proxy_set_header Host $host may lost port info fix proxy_set_header Host $host may lose port info Aug 19, 2020
@membphis
Copy link
Member

pls add test cases

@moonming
Copy link
Member

@halfcrazy $http_host ONLY get value from request header field, is that ok?
$host: in this order of precedence: host name from the request line, or host name from the “Host” request header field, or the server name matching a request
refer to: http://nginx.org/en/docs/http/ngx_http_core_module.html

@moonming
Copy link
Member

proxy_set_header Host $host may lose port info, use http_host instead.

And is this PR anything with proxy_set_header?

@halfcrazy
Copy link
Contributor Author

proxy_set_header Host $host may lose port info, use http_host instead.

And is this PR anything with proxy_set_header?

Since we use proxy_set_header to set the Host field in header for upstream server. Nginx $host variable doesn't contain a port, as rfc2616 a request to ports besides 80, 443 should always include a port in the host field.

[1]https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.23

@halfcrazy
Copy link
Contributor Author

pls add test cases

Sorry, I have no idea how to add a test for this case.

@moonming
Copy link
Member

pls add test cases

Sorry, I have no idea how to add a test for this case.

you can add test cases to test if port exists.

@membphis
Copy link
Member

@halfcrazy https://github.com/apache/apisix/blob/master/.travis/apisix_cli_test.sh

you can take a look at this shell script for testing.

@halfcrazy
Copy link
Contributor Author

Test case added.

@membphis
Copy link
Member

Test case added.

"proxy_set_header Host $host may lose port info, use http_host instead."

we need to add test for this case

@moonming
Copy link
Member

moonming commented Jan 5, 2021

ping @halfcrazy @membphis

@halfcrazy halfcrazy closed this Jan 12, 2021
@dbit-xia
Copy link

dbit-xia commented Jan 19, 2021

Hi, May I ask the conclusion of this question? tks @halfcrazy

@halfcrazy
Copy link
Contributor Author

halfcrazy commented Jan 26, 2021

Hi, May I ask the conclusion of this question? tks @halfcrazy

There's a lot of change to apisix template since this pr, the $host indeed should change to $http_host but I don't familiar with Perl style test cant write a test for this.

@membphis
Copy link
Member

let us reopen this PR, I think @Firstsawyou can help you, all right? @Firstsawyou

@membphis membphis reopened this Jan 26, 2021
@membphis
Copy link
Member

I think we need to process the conflict files first.

image

@Firstsawyou
Copy link
Contributor

let us reopen this PR, I think @Firstsawyou can help you, all right? @Firstsawyou

Let me see.

@Firstsawyou
Copy link
Contributor

hi @halfcrazy
The title of the PR should look like this:
fix: proxy_set_header Host $host may lose port info

@halfcrazy halfcrazy changed the title fix proxy_set_header Host $host may lose port info fix: proxy_set_header Host $host may lose port info Jan 26, 2021
@halfcrazy halfcrazy force-pushed the fix/missing-port branch 2 times, most recently from a821462 to bea7853 Compare January 26, 2021 12:54
t/lib/server.lua Outdated Show resolved Hide resolved
t/node/http_host.t Outdated Show resolved Hide resolved
@halfcrazy halfcrazy force-pushed the fix/missing-port branch 3 times, most recently from e94041f to 972200a Compare January 26, 2021 15:47
Copy link
Member

@lilien1010 lilien1010 left a comment

Choose a reason for hiding this comment

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

LGTM

@spacewander spacewander merged commit 8d61d31 into apache:master Jan 27, 2021
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.

7 participants