You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/05/13 14:15:02 UTC
[GitHub] [apisix] Gary-Airwallex opened a new pull request #4244: feat: support uri encoding in redirect
Gary-Airwallex opened a new pull request #4244:
URL: https://github.com/apache/apisix/pull/4244
### What this PR does / why we need it:
Redirect plugin support uri encoding, similar to the implementation in proxy_rewrite.
Fix (part of) https://github.com/apache/apisix/issues/4229.
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [x] Have you modified the corresponding document?
* [x] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #4244: feat: support uri encoding in redirect
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #4244:
URL: https://github.com/apache/apisix/pull/4244#discussion_r631858010
##########
File path: t/plugin/redirect.t
##########
@@ -813,3 +813,100 @@ GET /hello
hello world
--- no_error_log
[error]
+
+
+
+=== TEST 33: add plugin with new regex_uri: encode_uri = true
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "redirect": {
+ "regex_uri": ["^/test/(.*)", "http://test.com/${1}"],
+ "ret_code": 301,
+ "encode_uri": true
+ }
+ },
+ "uri": "/test/*",
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 34: regex_uri redirect with special characters
+--- request
+GET /test/with%20space
+--- error_code: 200
+--- response_headers
+Location: http://test.com/with%20space
+--- error_code: 301
+--- no_error_log
+[error]
+
+
+
+=== TEST 35: add plugin with new uri: encode_uri = true
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "redirect": {
+ "uri": "$uri",
+ "ret_code": 301,
+ "encode_uri": true
+ }
+ },
+ "uri": "/hello*"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 36: redirect with special characters
+--- request
+GET /hello/with%20space
+--- response_headers
+Location: /hello/with%20space
+--- error_code: 301
+--- no_error_log
+[error]
Review comment:
A blank line needs to be added at the end of the file.
##########
File path: apisix/plugins/redirect.lua
##########
@@ -160,17 +163,17 @@ function _M.rewrite(conf, ctx)
end
if ret_code then
+ local new_uri
if uri then
- local new_uri, err = concat_new_uri(uri, ctx)
+ local err
+ new_uri, err = concat_new_uri(uri, ctx)
if not new_uri then
core.log.error("failed to generate new uri by: " .. uri .. err)
return 500
end
-
- core.response.set_header("Location", new_uri)
- return ret_code
elseif regex_uri then
- local new_uri, n, err = re_sub(ctx.var.uri, regex_uri[1],
+ local n, err
+ new_uri, n, err = re_sub(ctx.var.uri, regex_uri[1],
regex_uri[2], "jo")
Review comment:
```suggestion
new_uri, n, err = re_sub(ctx.var.uri, regex_uri[1],
regex_uri[2], "jo")
```
This style is better.
##########
File path: apisix/plugins/redirect.lua
##########
@@ -22,6 +22,8 @@ local re_gmatch = ngx.re.gmatch
local re_sub = ngx.re.sub
local ipairs = ipairs
local ngx = ngx
+local str_find = core.string.find
+local sub_str = string.sub
Review comment:
```suggestion
local str_find = core.string.find
local str_sub = string.sub
```
I think this would be better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #4244: feat: support uri encoding in redirect
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #4244:
URL: https://github.com/apache/apisix/pull/4244#discussion_r632220841
##########
File path: docs/en/latest/plugins/redirect.md
##########
@@ -41,8 +41,9 @@ URI redirect.
| uri | string | optional | | | New URL which can contain Nginx variable, eg: `/test/index.html`, `$uri/index.html`. You can refer to variables in a way similar to `${xxx}` to avoid ambiguity, eg: `${uri}foo/index.html`. If you just need the original `$` character, add `\` in front of it, like this one: `/\$foo/index.html`. If you refer to a variable name that does not exist, this will not produce an error, and it will be used as an empty string. |
| regex_uri | array[string] | optional | | | Use regular expression to match URL from client, when the match is successful, the URL template will be redirected to. If the match is not successful, the URL from the client will be forwarded to the upstream. Only one of `uri` and `regex_uri` can be exist. For example: [" ^/iresty/(.*)/(.*)/(.*)", "/$1-$2-$3"], the first element represents the matching regular expression and the second element represents the URL template that is redirected to. |
| ret_code | integer | optional | 302 | [200, ...] | Response code |
+| encode_uri | boolean | optional | false | | When set to `true` the uri in `Location` header will be encoded. |
Review comment:
We may tell people the encoding rule, for instance:
```suggestion
| encode_uri | boolean | optional | false | | When set to `true` the uri in `Location` header will be encoded as per [RFC3986](https://datatracker.ietf.org/doc/html/rfc3986)|
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #4244: feat: support uri encoding in redirect
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #4244:
URL: https://github.com/apache/apisix/pull/4244
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #4244: feat: support uri encoding in redirect
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #4244:
URL: https://github.com/apache/apisix/pull/4244
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #4244: feat: support uri encoding in redirect
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #4244:
URL: https://github.com/apache/apisix/pull/4244#discussion_r631894623
##########
File path: apisix/plugins/redirect.lua
##########
@@ -179,11 +182,23 @@ function _M.rewrite(conf, ctx)
return 500
end
- if n > 0 then
- core.response.set_header("Location", new_uri)
- return ret_code
+ if n < 1 then
+ return
end
end
+
+ if conf.encode_uri then
+ local index = str_find(new_uri, "?")
+ if index then
+ new_uri = core.utils.uri_safe_encode(sub_str(new_uri, 1, index-1)) ..
+ sub_str(new_uri, index)
Review comment:
```suggestion
new_uri = core.utils.uri_safe_encode(sub_str(new_uri, 1, index-1)) ..
sub_str(new_uri, index)
```
Bad style.
##########
File path: docs/zh/latest/plugins/redirect.md
##########
@@ -31,8 +31,9 @@ URI 重定向插件。
| uri | string | 可选 | | | 可以包含 Nginx 变量的 URI，例如：`/test/index.html`, `$uri/index.html`。你可以通过类似于 `$ {xxx}` 的方式引用变量，以避免产生歧义，例如：`${uri}foo/index.html`。若你需要保留 `$` 字符，那么使用如下格式：`/\$foo/index.html` |
| regex_uri | array[string] | 可选 | | | 转发到上游的新 `uri` 地址, 使用正则表达式匹配来自客户端的 `uri`，当匹配成功后使用模板替换发送重定向到客户端, 未匹配成功时将客户端请求的 `uri` 转发至上游。`uri` 和 `regex_uri` 不可以同时存在。例如：["^/iresty/(.*)/(.*)/(.*)","/$1-$2-$3"] 第一个元素代表匹配来自客户端请求的 `uri` 正则表达式，第二个元素代表匹配成功后发送重定向到客户端的 `uri` 模板。 |
| ret_code | integer | 可选 | 302 | [200, ...] | 请求响应码 |
+| encode_uri | boolean | 可选 | false | | 当设置为 `true` 时，对返回的 `Location` header进行编码. |
-`http_to_https` 和 `uri` 两个中只能配置一个。
+`http_to_https`，`uri`，`regex_uri` 三个中只能配置一个。
Review comment:
```suggestion
`http_to_https`，`uri` 或 `regex_uri` 三个中只能配置一个。
```
##########
File path: docs/en/latest/plugins/redirect.md
##########
@@ -41,8 +41,9 @@ URI redirect.
| uri | string | optional | | | New URL which can contain Nginx variable, eg: `/test/index.html`, `$uri/index.html`. You can refer to variables in a way similar to `${xxx}` to avoid ambiguity, eg: `${uri}foo/index.html`. If you just need the original `$` character, add `\` in front of it, like this one: `/\$foo/index.html`. If you refer to a variable name that does not exist, this will not produce an error, and it will be used as an empty string. |
| regex_uri | array[string] | optional | | | Use regular expression to match URL from client, when the match is successful, the URL template will be redirected to. If the match is not successful, the URL from the client will be forwarded to the upstream. Only one of `uri` and `regex_uri` can be exist. For example: [" ^/iresty/(.*)/(.*)/(.*)", "/$1-$2-$3"], the first element represents the matching regular expression and the second element represents the URL template that is redirected to. |
| ret_code | integer | optional | 302 | [200, ...] | Response code |
+| encode_uri | boolean | optional | false | | When set to `true` the uri in `Location` header will be encoded. |
-Only one of `http_to_https` or `uri` can be specified.
+Only one of `http_to_https`, `uri`, `regex_uri` can be specified.
Review comment:
```suggestion
Only one of `http_to_https`, `uri` or `regex_uri` can be specified.
```
Is this better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org