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.

Sign up for GitHub

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

Jump to bottom

fix(redirect): set redirect server port when enable http_to_https #6686

Merged
merged 12 commits into from
May 5, 2022
Merged

fix(redirect): set redirect server port when enable http_to_https #6686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
20d3037
feat(plugin-redirect): set redirect server port when enable http_to_h…
kwanhur Mar 22, 2022
b18a5a6
feat(plugin-redirect): set redirect server port when enable http_to_h…
kwanhur Mar 22, 2022
a886510
style: reindex t/plugin/redirect.t
kwanhur Mar 23, 2022
cab61f9
style: merge pull
kwanhur Mar 23, 2022
fc97e85
style: reindex t/plugin/redirect.t
kwanhur Mar 23, 2022
6e1e331
Merge branch 'feat-redirect-port' of github.com:kwanhur/apisix into f…
kwanhur Mar 23, 2022
5f344e0
chore: merge master and fix conflicts
kwanhur Apr 28, 2022
4726d79
feat(redirect): redirect server port with x-forwarded-port when enabl…
kwanhur Apr 30, 2022
721aa8f
feat(redirect): redirect to well-known port when ret_port nil
kwanhur Apr 30, 2022
76007ac
feat(redirect): redirect to well-known port when ret_port less than z…
kwanhur Apr 30, 2022
8767233
docs: update plugin redirect port when http_to_https at en usage
kwanhur May 1, 2022
75fa6f1
docs: update plugin redirect port when http_to_https at zh usage
kwanhur May 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apisix/core/ctx.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ do
upstream_cache_bypass = true,

var_x_forwarded_proto = true,
var_x_forwarded_port = true,
}

-- sort in alphabetical
Expand Down
9 changes: 8 additions & 1 deletion apisix/plugins/redirect.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ local ipairs = ipairs
local ngx = ngx
local str_find = core.string.find
local str_sub = string.sub
local tonumber = tonumber

local lrucache = core.lrucache.new({
ttl = 300, count = 100
Expand Down Expand Up @@ -147,6 +148,7 @@ function _M.rewrite(conf, ctx)
core.log.info("plugin rewrite phase, conf: ", core.json.delay_encode(conf))

local ret_code = conf.ret_code
local ret_port = tonumber(ctx.var["var_x_forwarded_port"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Recently, I upgraded apisix with the latest code, and found that there was an error in converting http to https. When redirecting, I always bring a port 80. It should be caused by this change, because I am listening on port 80 and port 443, but ngx_tpl There is a piece of code in it that is "set $var_x_forwarded_port $server_port;", which means that the port from http to https is always the same. I don't think this change is very reasonable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, seems I got what you mean. The server port 80 and then enable http_to_https, it'll always redirect to https://host:80 then get 400 bad request. cc @tokers @spacewander @tzssangglass

We need to take more consideration about #6686 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Can we set a default value for this case?
If there is no X-Forwarded-Port in clinet reuqest headers, then the redirect plugin use 443 as the default https port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has default as the server_port

apisix/apisix/cli/ngx_tpl.lua

Line 668 in 1b5c190

set $var_x_forwarded_port $server_port;

As mentioned before #6686 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

It has default as the server_port

I mean in the redirect plugin, if someone enable http_to_https and the client request doesn't pass X-Forwarded-Port, use 443 as the default port in Location and don't care about the server_port variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, it is definitely impossible to know the listening port of https for this domain name if it is not configured from http to https, so it can be considered that the port can be configured, but the default is 443?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the conversation before #6686 (comment)

The original implement with an extra attribute ret_port, but no acceptable.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can read the correct port from config.yaml?

Copy link

Choose a reason for hiding this comment

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

把这个set $var_x_forwarded_port $server_port; 改成set $var_x_forwarded_port 443;

local uri = conf.uri
local regex_uri = conf.regex_uri

Expand All @@ -155,7 +157,12 @@ function _M.rewrite(conf, ctx)
if conf.http_to_https and _scheme == "http" then
-- TODO： add test case
-- PR: https://github.com/apache/apisix/pull/1958
uri = "https://$host$request_uri"
if ret_port == nil or ret_port == 443 or ret_port <= 0 or ret_port > 65535 then
uri = "https://$host$request_uri"
else
uri = "https://$host:" .. ret_port .. "$request_uri"
end

local method_name = ngx.req.get_method()
if method_name == "GET" or method_name == "HEAD" then
ret_code = 301
Expand Down
4 changes: 2 additions & 2 deletions docs/en/latest/plugins/redirect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ The `redirect` Plugin can be used to configure redirects.

Only one of `http_to_https`, `uri` and `regex_uri` can be configured.

* When enabling `http_to_https`, the port in the redirect URL will be the value of header `X-Forwarded-Port` or the port of the server.

:::

## Enabling the Plugin
Expand Down Expand Up @@ -106,7 +108,6 @@ Content-Type: text/html
Content-Length: 166
Connection: keep-alive
Location: /test/default.html

...
```

Expand Down Expand Up @@ -136,7 +137,6 @@ curl http://127.0.0.1:9080/hello -i
HTTP/1.1 301 Moved Permanently
...
Location: https://127.0.0.1:9443/hello

...
```

Expand Down
18 changes: 10 additions & 8 deletions docs/zh/latest/plugins/redirect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,21 @@ description: 本文介绍了关于 Apache APISIX `redirect` 插件的基本信

## 属性

| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
| ------------------- | ------------- | --------- | ------- | ---------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| http_to_https | boolean | 是 | false | [true,false] | 当设置为 `true` 并且请求是 HTTP 时，它将被重定向具有相同 URI 和 301 状态码的 HTTPS。 |
| uri | string | 是 | | | 要重定向到的 URI，可以包含 NGINX 变量。例如：`/test/index.htm`, `$uri/index.html`，`${uri}/index.html`。如果你引入了一个不存在的变量，它不会报错，而是将其视为一个空变量。 |
| regex_uri | array[string] | 是 | | | 将来自客户端的 URL 与正则表达式匹配并重定向。当匹配成功后使用模板替换发送重定向到客户端，如果未匹配成功会将客户端请求的 URI 转发至上游。 和 `regex_uri` 不可以同时存在。例如：["^/iresty/(.)/(.)/(.*)","/$1-$2-$3"] 第一个元素代表匹配来自客户端请求的 URI 正则表达式，第二个元素代表匹配成功后发送重定向到客户端的 URI 模板。 |
| ret_code | integer | 是 | 302 | [200, ...] | HTTP 响应码 |
| encode_uri | boolean | 是 | false | [true,false] | 当设置为 `true` 时，对返回的 `Location` Header 按照 [RFC3986](https://datatracker.ietf.org/doc/html/rfc3986)的编码格式进行编码。 |
| append_query_string | boolean | 是 | false | [true,false] | 当设置为 `true` 时，将原始请求中的查询字符串添加到 `Location` Header。如果已配置 `uri` 或 `regex_uri` 已经包含查询字符串，则请求中的查询字符串将附加一个`&`。如果你已经处理过查询字符串（例如，使用 NGINX 变量 `$request_uri`），请不要再使用该参数以避免重复。 |
| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
|---------------------|---------------|-----|-------|--------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| http_to_https | boolean | 是 | false | [true,false] | 当设置为 `true` 并且请求是 HTTP 时，它将被重定向具有相同 URI 和 301 状态码的 HTTPS。 |
| uri | string | 是 | | | 要重定向到的 URI，可以包含 NGINX 变量。例如：`/test/index.htm`, `$uri/index.html`，`${uri}/index.html`。如果你引入了一个不存在的变量，它不会报错，而是将其视为一个空变量。 |
| regex_uri | array[string] | 是 | | | 将来自客户端的 URL 与正则表达式匹配并重定向。当匹配成功后使用模板替换发送重定向到客户端，如果未匹配成功会将客户端请求的 URI 转发至上游。 和 `regex_uri` 不可以同时存在。例如：["^/iresty/(.)/(.)/(.*)","/$1-$2-$3"] 第一个元素代表匹配来自客户端请求的 URI 正则表达式，第二个元素代表匹配成功后发送重定向到客户端的 URI 模板。 |
| ret_code | integer | 是 | 302 | [200, ...] | HTTP 响应码 |
| encode_uri | boolean | 是 | false | [true,false] | 当设置为 `true` 时，对返回的 `Location` Header 按照 [RFC3986](https://datatracker.ietf.org/doc/html/rfc3986)的编码格式进行编码。 |
| append_query_string | boolean | 是 | false | [true,false] | 当设置为 `true` 时，将原始请求中的查询字符串添加到 `Location` Header。如果已配置 `uri` 或 `regex_uri` 已经包含查询字符串，则请求中的查询字符串将附加一个`&`。如果你已经处理过查询字符串（例如，使用 NGINX 变量 `$request_uri`），请不要再使用该参数以避免重复。 |

:::note

`http_to_https`、`uri` 和 `regex_uri` 只能配置其中一个属性。

* 当开启 `http_to_https` 时，重定向 URL 中的端口将是 `X-Forwarded-Port` 请求头的值或服务器的端口。

:::

## 启用插件
Expand Down
108 changes: 78 additions & 30 deletions t/plugin/redirect.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,11 +435,59 @@ GET /hello
Host: foo.com
--- error_code: 301
--- response_headers
Location: https://foo.com:1984/hello



=== TEST 19: redirect(pass well-known port 443 to x-forwarded-port)
--- request
GET /hello
--- more_headers
Host: foo.com
x-forwarded-port: 443
--- error_code: 301
--- response_headers
Location: https://foo.com/hello



=== TEST 19: enable http_to_https with ret_code(not take effect)
=== TEST 20: redirect(pass negative number to x-forwarded-port)
--- request
GET /hello
--- more_headers
Host: foo.com
x-forwarded-port: -443
--- error_code: 301
--- response_headers
Location: https://foo.com/hello



=== TEST 21: redirect(pass number more than 65535 to x-forwarded-port)
--- request
GET /hello
--- more_headers
Host: foo.com
x-forwarded-port: 65536
--- error_code: 301
--- response_headers
Location: https://foo.com/hello



=== TEST 22: redirect(pass invalid non-number to x-forwarded-port)
--- request
GET /hello
--- more_headers
Host: foo.com
x-forwarded-port: ok
--- error_code: 301
--- response_headers
Location: https://foo.com/hello



=== TEST 23: enable http_to_https with ret_code(not take effect)
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -473,18 +521,18 @@ passed



=== TEST 20: redirect
=== TEST 24: redirect
--- request
GET /hello
--- more_headers
Host: foo.com
--- error_code: 301
--- response_headers
Location: https://foo.com/hello
Location: https://foo.com:1984/hello



=== TEST 21: wrong configure, enable http_to_https with uri
=== TEST 25: wrong configure, enable http_to_https with uri
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -519,7 +567,7 @@ qr/error_msg":"failed to check the configuration of plugin redirect err: value s



=== TEST 22: enable http_to_https with upstream
=== TEST 26: enable http_to_https with upstream
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -558,18 +606,18 @@ passed



=== TEST 23: redirect
=== TEST 27: redirect
--- request
GET /hello
--- more_headers
Host: test.com
--- error_code: 301
--- response_headers
Location: https://test.com/hello
Location: https://test.com:1984/hello



=== TEST 24: set ssl(sni: test.com)
=== TEST 28: set ssl(sni: test.com)
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -600,7 +648,7 @@ passed



=== TEST 25: client https request
=== TEST 29: client https request
--- config
listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl;

Expand Down Expand Up @@ -674,7 +722,7 @@ close: 1 nil}



=== TEST 26: add plugin with new uri: /test/add
=== TEST 30: add plugin with new uri: /test/add
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -709,46 +757,46 @@ passed



=== TEST 27: http to https post redirect
=== TEST 31: http to https post redirect
--- request
POST /hello-https
--- more_headers
Host: test.com
--- response_headers
Location: https://test.com/hello-https
Location: https://test.com:1984/hello-https
--- error_code: 308
--- no_error_log
[error]



=== TEST 28: http to https get redirect
=== TEST 32: http to https get redirect
--- request
GET /hello-https
--- more_headers
Host: test.com
--- response_headers
Location: https://test.com/hello-https
Location: https://test.com:1984/hello-https
--- error_code: 301
--- no_error_log
[error]



=== TEST 29: http to https head redirect
=== TEST 33: http to https head redirect
--- request
HEAD /hello-https
--- more_headers
Host: test.com
--- response_headers
Location: https://test.com/hello-https
Location: https://test.com:1984/hello-https
--- error_code: 301
--- no_error_log
[error]



=== TEST 30: add plugin with new regex_uri: /test/1 redirect to http://test.com/1
=== TEST 34: add plugin with new regex_uri: /test/1 redirect to http://test.com/1
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -787,7 +835,7 @@ passed



=== TEST 31: regex_uri redirect
=== TEST 35: regex_uri redirect
--- request
GET /test/1
--- response_headers
Expand All @@ -798,7 +846,7 @@ Location: http://test.com/1



=== TEST 32: regex_uri not match, get response from upstream
=== TEST 36: regex_uri not match, get response from upstream
--- request
GET /hello
--- error_code: 200
Expand All @@ -809,7 +857,7 @@ hello world



=== TEST 33: add plugin with new regex_uri: encode_uri = true
=== TEST 37: add plugin with new regex_uri: encode_uri = true
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -849,7 +897,7 @@ passed



=== TEST 34: regex_uri redirect with special characters
=== TEST 38: regex_uri redirect with special characters
--- request
GET /test/with%20space
--- error_code: 200
Expand All @@ -861,7 +909,7 @@ Location: http://test.com/with%20space



=== TEST 35: add plugin with new uri: encode_uri = true
=== TEST 39: add plugin with new uri: encode_uri = true
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -895,7 +943,7 @@ passed



=== TEST 36: redirect with special characters
=== TEST 40: redirect with special characters
--- request
GET /hello/with%20space
--- response_headers
Expand All @@ -906,7 +954,7 @@ Location: /hello/with%20space



=== TEST 37: add plugin with new uri: $uri (append_query_string = true)
=== TEST 41: add plugin with new uri: $uri (append_query_string = true)
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -940,7 +988,7 @@ passed



=== TEST 38: redirect
=== TEST 42: redirect
--- request
GET /hello?name=json
--- response_headers
Expand All @@ -951,7 +999,7 @@ Location: /hello?name=json



=== TEST 39: add plugin with new uri: $uri?type=string (append_query_string = true)
=== TEST 43: add plugin with new uri: $uri?type=string (append_query_string = true)
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -985,7 +1033,7 @@ passed



=== TEST 40: redirect
=== TEST 44: redirect
--- request
GET /hello?name=json
--- response_headers
Expand All @@ -996,7 +1044,7 @@ Location: /hello?type=string&name=json



=== TEST 41: enable http_to_https (pass X-Forwarded-Proto)
=== TEST 45: enable http_to_https (pass X-Forwarded-Proto)
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -1036,12 +1084,12 @@ passed



=== TEST 42: enable http_to_https (pass X-Forwarded-Proto)
=== TEST 46: enable http_to_https (pass X-Forwarded-Proto)
--- request
GET /hello
--- more_headers
Host: foo.com
X-Forwarded-Proto: http
--- error_code: 301
--- response_headers
Location: https://foo.com/hello
Location: https://foo.com:1984/hello