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 2022/01/25 14:23:18 UTC
[GitHub] [apisix] Baoyuantop opened a new pull request #6201: feat: add expires timestamp for CSRF plugin
Baoyuantop opened a new pull request #6201:
URL: https://github.com/apache/apisix/pull/6201
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
resolve https://github.com/apache/apisix/issues/6141
### Pre-submission checklist:
<!--
Please follow the PR manners:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. If you need to resolve merge conflicts after the PR is reviewed, please merge master but do not rebase
6. Use "request review" to notify the reviewer once you have resolved the review
7. Only reviewer can click "Resolve conversation" to mark the reviewer's review resolved
-->
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [ ] 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Baoyuantop commented on pull request #6201: feat: add expires timestamp for CSRF plugin
Posted by GitBox <gi...@apache.org>.
Baoyuantop commented on pull request #6201:
URL: https://github.com/apache/apisix/pull/6201#issuecomment-1049542775
I have a problem when I change the test: I need to carry the cookie returned from the previous response. Is there any way to do it? The cookie is now encoded in the test code.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #6201: feat: add expires timestamp for CSRF plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #6201:
URL: https://github.com/apache/apisix/pull/6201#issuecomment-1050467855
You can refer to:
> One request after another:
https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Baoyuantop commented on a change in pull request #6201: feat: add expires timestamp for CSRF plugin
Posted by GitBox <gi...@apache.org>.
Baoyuantop commented on a change in pull request #6201:
URL: https://github.com/apache/apisix/pull/6201#discussion_r823534645
##########
File path: t/plugin/csrf.t
##########
@@ -158,5 +159,204 @@ Cookie: apisix-csrf-token=eyJleHBpcmVzIjo3MjAwLCJyYW5kb20iOjAuMjE2ODAxOTYyNTEwND
--- request
POST /hello
--- more_headers
-apisix-csrf-token: eyJzaWduIjoiZTlhNWVkOTBmZDc2YjRhMTYyMzg1ZDU2Y2ZhZDI1N2MxNmI0MWY1MjFjZWUwODczNzExM2NlYzZkZDQwMWJmNyIsInJhbmRvbSI6MC4zNjcxNDg2NDI2MjE0MywiZXhwaXJlcyI6NzIwMH0=
-Cookie: apisix-csrf-token=eyJzaWduIjoiZTlhNWVkOTBmZDc2YjRhMTYyMzg1ZDU2Y2ZhZDI1N2MxNmI0MWY1MjFjZWUwODczNzExM2NlYzZkZDQwMWJmNyIsInJhbmRvbSI6MC4zNjcxNDg2NDI2MjE0MywiZXhwaXJlcyI6NzIwMH0=
+apisix-csrf-token: eyJyYW5kb20iOjAuNDI5ODYzMTk3MTYxMzksInNpZ24iOiI0ODRlMDY4NTkxMWQ5NmJhMDc5YzQ1ZGI0OTE2NmZkYjQ0ODhjODVkNWQ0NmE1Y2FhM2UwMmFhZDliNjE5OTQ2IiwiZXhwaXJlcyI6MjY0MzExOTYyNH0=
+Cookie: apisix-csrf-token=eyJyYW5kb20iOjAuNDI5ODYzMTk3MTYxMzksInNpZ24iOiI0ODRlMDY4NTkxMWQ5NmJhMDc5YzQ1ZGI0OTE2NmZkYjQ0ODhjODVkNWQ0NmE1Y2FhM2UwMmFhZDliNjE5OTQ2IiwiZXhwaXJlcyI6MjY0MzExOTYyNH0=
+
+
+
+=== TEST 10: change expired
+--- 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,
+ [[{
+ "uri": "/hello",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ },
+ "plugins": {
+ "csrf": {
+ "key": "userkey",
+ "expires": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 11: expired csrf token
+--- request
+POST /hello
+--- more_headers
+apisix-csrf-token: eyJyYW5kb20iOjAuMDY3NjAxMDQwMDM5MzI4LCJzaWduIjoiOTE1Yjg2MjBhNTg1N2FjZmIzNjIxOTNhYWVlN2RkYjY5NmM0NWYwZjE5YjY5Zjg3NjM4ZTllNGNjNjYxYjQwNiIsImV4cGlyZXMiOjE2NDMxMjAxOTN9
+Cookie: apisix-csrf-token=eyJyYW5kb20iOjAuMDY3NjAxMDQwMDM5MzI4LCJzaWduIjoiOTE1Yjg2MjBhNTg1N2FjZmIzNjIxOTNhYWVlN2RkYjY5NmM0NWYwZjE5YjY5Zjg3NjM4ZTllNGNjNjYxYjQwNiIsImV4cGlyZXMiOjE2NDMxMjAxOTN9
+--- error_code: 401
+--- error_log: token has expired
+
+
+
+=== TEST 12: set expires 1
+--- 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,
+ [[{
+ "uri": "/hello",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ },
+ "plugins": {
+ "csrf": {
+ "key": "userkey",
+ "expires": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
Review comment:
Yes, modified
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Baoyuantop commented on pull request #6201: feat: add expires timestamp for CSRF plugin
Posted by GitBox <gi...@apache.org>.
Baoyuantop commented on pull request #6201:
URL: https://github.com/apache/apisix/pull/6201#issuecomment-1023046785
> LGTM. Thanks for addressing the changes.
>
> > Also, how about ignoring the check when conf.expires = 0
>
> What do you think?
Agree
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #6201: feat: add expires timestamp for CSRF plugin
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #6201:
URL: https://github.com/apache/apisix/pull/6201
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bisakhmondal commented on a change in pull request #6201: feat: add expires timestamp for CSRF plugin
Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on a change in pull request #6201:
URL: https://github.com/apache/apisix/pull/6201#discussion_r823344595
##########
File path: t/plugin/csrf.t
##########
@@ -158,5 +159,204 @@ Cookie: apisix-csrf-token=eyJleHBpcmVzIjo3MjAwLCJyYW5kb20iOjAuMjE2ODAxOTYyNTEwND
--- request
POST /hello
--- more_headers
-apisix-csrf-token: eyJzaWduIjoiZTlhNWVkOTBmZDc2YjRhMTYyMzg1ZDU2Y2ZhZDI1N2MxNmI0MWY1MjFjZWUwODczNzExM2NlYzZkZDQwMWJmNyIsInJhbmRvbSI6MC4zNjcxNDg2NDI2MjE0MywiZXhwaXJlcyI6NzIwMH0=
-Cookie: apisix-csrf-token=eyJzaWduIjoiZTlhNWVkOTBmZDc2YjRhMTYyMzg1ZDU2Y2ZhZDI1N2MxNmI0MWY1MjFjZWUwODczNzExM2NlYzZkZDQwMWJmNyIsInJhbmRvbSI6MC4zNjcxNDg2NDI2MjE0MywiZXhwaXJlcyI6NzIwMH0=
+apisix-csrf-token: eyJyYW5kb20iOjAuNDI5ODYzMTk3MTYxMzksInNpZ24iOiI0ODRlMDY4NTkxMWQ5NmJhMDc5YzQ1ZGI0OTE2NmZkYjQ0ODhjODVkNWQ0NmE1Y2FhM2UwMmFhZDliNjE5OTQ2IiwiZXhwaXJlcyI6MjY0MzExOTYyNH0=
+Cookie: apisix-csrf-token=eyJyYW5kb20iOjAuNDI5ODYzMTk3MTYxMzksInNpZ24iOiI0ODRlMDY4NTkxMWQ5NmJhMDc5YzQ1ZGI0OTE2NmZkYjQ0ODhjODVkNWQ0NmE1Y2FhM2UwMmFhZDliNjE5OTQ2IiwiZXhwaXJlcyI6MjY0MzExOTYyNH0=
+
+
+
+=== TEST 10: change expired
+--- 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,
+ [[{
+ "uri": "/hello",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ },
+ "plugins": {
+ "csrf": {
+ "key": "userkey",
+ "expires": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 11: expired csrf token
+--- request
+POST /hello
+--- more_headers
+apisix-csrf-token: eyJyYW5kb20iOjAuMDY3NjAxMDQwMDM5MzI4LCJzaWduIjoiOTE1Yjg2MjBhNTg1N2FjZmIzNjIxOTNhYWVlN2RkYjY5NmM0NWYwZjE5YjY5Zjg3NjM4ZTllNGNjNjYxYjQwNiIsImV4cGlyZXMiOjE2NDMxMjAxOTN9
+Cookie: apisix-csrf-token=eyJyYW5kb20iOjAuMDY3NjAxMDQwMDM5MzI4LCJzaWduIjoiOTE1Yjg2MjBhNTg1N2FjZmIzNjIxOTNhYWVlN2RkYjY5NmM0NWYwZjE5YjY5Zjg3NjM4ZTllNGNjNjYxYjQwNiIsImV4cGlyZXMiOjE2NDMxMjAxOTN9
+--- error_code: 401
+--- error_log: token has expired
+
+
+
+=== TEST 12: set expires 1
+--- 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,
+ [[{
+ "uri": "/hello",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ },
+ "plugins": {
+ "csrf": {
+ "key": "userkey",
+ "expires": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
Review comment:
redundant? `/route/1` already has the same config.
##########
File path: t/plugin/csrf.t
##########
@@ -158,5 +159,204 @@ Cookie: apisix-csrf-token=eyJleHBpcmVzIjo3MjAwLCJyYW5kb20iOjAuMjE2ODAxOTYyNTEwND
--- request
POST /hello
--- more_headers
-apisix-csrf-token: eyJzaWduIjoiZTlhNWVkOTBmZDc2YjRhMTYyMzg1ZDU2Y2ZhZDI1N2MxNmI0MWY1MjFjZWUwODczNzExM2NlYzZkZDQwMWJmNyIsInJhbmRvbSI6MC4zNjcxNDg2NDI2MjE0MywiZXhwaXJlcyI6NzIwMH0=
-Cookie: apisix-csrf-token=eyJzaWduIjoiZTlhNWVkOTBmZDc2YjRhMTYyMzg1ZDU2Y2ZhZDI1N2MxNmI0MWY1MjFjZWUwODczNzExM2NlYzZkZDQwMWJmNyIsInJhbmRvbSI6MC4zNjcxNDg2NDI2MjE0MywiZXhwaXJlcyI6NzIwMH0=
+apisix-csrf-token: eyJyYW5kb20iOjAuNDI5ODYzMTk3MTYxMzksInNpZ24iOiI0ODRlMDY4NTkxMWQ5NmJhMDc5YzQ1ZGI0OTE2NmZkYjQ0ODhjODVkNWQ0NmE1Y2FhM2UwMmFhZDliNjE5OTQ2IiwiZXhwaXJlcyI6MjY0MzExOTYyNH0=
+Cookie: apisix-csrf-token=eyJyYW5kb20iOjAuNDI5ODYzMTk3MTYxMzksInNpZ24iOiI0ODRlMDY4NTkxMWQ5NmJhMDc5YzQ1ZGI0OTE2NmZkYjQ0ODhjODVkNWQ0NmE1Y2FhM2UwMmFhZDliNjE5OTQ2IiwiZXhwaXJlcyI6MjY0MzExOTYyNH0=
+
+
+
+=== TEST 10: change expired
+--- 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,
+ [[{
+ "uri": "/hello",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ },
+ "plugins": {
+ "csrf": {
+ "key": "userkey",
+ "expires": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 11: expired csrf token
+--- request
+POST /hello
+--- more_headers
+apisix-csrf-token: eyJyYW5kb20iOjAuMDY3NjAxMDQwMDM5MzI4LCJzaWduIjoiOTE1Yjg2MjBhNTg1N2FjZmIzNjIxOTNhYWVlN2RkYjY5NmM0NWYwZjE5YjY5Zjg3NjM4ZTllNGNjNjYxYjQwNiIsImV4cGlyZXMiOjE2NDMxMjAxOTN9
+Cookie: apisix-csrf-token=eyJyYW5kb20iOjAuMDY3NjAxMDQwMDM5MzI4LCJzaWduIjoiOTE1Yjg2MjBhNTg1N2FjZmIzNjIxOTNhYWVlN2RkYjY5NmM0NWYwZjE5YjY5Zjg3NjM4ZTllNGNjNjYxYjQwNiIsImV4cGlyZXMiOjE2NDMxMjAxOTN9
+--- error_code: 401
+--- error_log: token has expired
+
+
+
+=== TEST 12: set expires 1
+--- 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,
+ [[{
+ "uri": "/hello",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ },
+ "plugins": {
+ "csrf": {
+ "key": "userkey",
+ "expires": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 13: token has expired after sleep 2s
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+
+ local uri = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/hello"
+
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {method = "GET"})
+ if not res then
+ ngx.say(err)
+ return
+ end
+ local cookie = res.headers["Set-Cookie"]
+ local token = cookie:match("=([^;]+)")
+
+ ngx.sleep(2)
+
+ local res, err = httpc:request_uri(uri, {
+ method = "POST",
+ headers = {
+ ["apisix-csrf-token"] = token,
+ ["Cookie"] = cookie,
+ }
+ })
+ if not res then
+ ngx.say(err)
+ return
+ end
+
+ if res.status >= 300 then
+ ngx.status = res.status
+ end
+ }
+ }
+--- error_code: 401
+--- error_log: token has expired
+
+
+
+=== TEST 14: set expires 0
+--- 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,
+ [[{
+ "uri": "/hello",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ },
+ "plugins": {
+ "csrf": {
+ "key": "userkey",
+ "expires": 0
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 15: token no expired after sleep 1s
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+
+ local uri = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/hello"
+
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {method = "GET"})
+ if not res then
+ ngx.say(err)
+ return
+ end
+
+ ngx.sleep(1)
+
+ local cookie = res.headers["Set-Cookie"]
+ local token = cookie:match("=([^;]+)")
+
+ local res, err = httpc:request_uri(uri, {
+ method = "POST",
+ headers = {
+ ["apisix-csrf-token"] = token,
+ ["Cookie"] = cookie,
+ }
+ })
+ if not res then
+ ngx.say(err)
+ return
+ end
+
+ if res.status >= 300 then
+ ngx.status = res.status
+ end
+ }
Review comment:
set the `ngx.status = res.status`
`ngx.say(res.body)`
https://github.com/ledgetech/lua-resty-http
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bisakhmondal commented on a change in pull request #6201: feat: add expires timestamp for CSRF plugin
Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on a change in pull request #6201:
URL: https://github.com/apache/apisix/pull/6201#discussion_r794309154
##########
File path: apisix/plugins/csrf.lua
##########
@@ -129,6 +135,10 @@ function _M.access(conf, ctx)
return
end
+ if conf.expires == 0 then
+ return
+ end
Review comment:
Ahh, sorry I confused you. I didn't mean to ignore csrf token validation. I meant we can ignore `token has expired check`.
##########
File path: docs/en/latest/plugins/csrf.md
##########
@@ -43,6 +43,8 @@ In the following we define `GET`, `HEAD` and `OPTIONS` as the `safe-methods` and
| expires | number | optional | `7200` | | Expiration time(s) of csrf cookie. |
| key | string | required | | | The secret key used to encrypt the cookie. |
+**Note: When expires is set to 0 the plugin will ignore all checks**
Review comment:
```suggestion
**Note: When expires is set to 0 the plugin will ignore checking if the token is expired or not.**
```
##########
File path: apisix/plugins/csrf.lua
##########
@@ -112,6 +113,11 @@ local function check_csrf_token(conf, ctx, token)
core.log.error("no expires in token")
return false
end
+ local time_now = ngx_time()
+ if time_now - expires > conf.expires then
+ core.log.error("token has expired")
+ return false
+ end
Review comment:
```suggestion
if conf.expires > 0 and time_now - expires > conf.expires then
core.log.error("token has expired")
return false
end
```
Just that's it.
##########
File path: t/plugin/csrf.t
##########
@@ -158,5 +159,92 @@ Cookie: apisix-csrf-token=eyJleHBpcmVzIjo3MjAwLCJyYW5kb20iOjAuMjE2ODAxOTYyNTEwND
--- request
POST /hello
--- more_headers
-apisix-csrf-token: eyJzaWduIjoiZTlhNWVkOTBmZDc2YjRhMTYyMzg1ZDU2Y2ZhZDI1N2MxNmI0MWY1MjFjZWUwODczNzExM2NlYzZkZDQwMWJmNyIsInJhbmRvbSI6MC4zNjcxNDg2NDI2MjE0MywiZXhwaXJlcyI6NzIwMH0=
-Cookie: apisix-csrf-token=eyJzaWduIjoiZTlhNWVkOTBmZDc2YjRhMTYyMzg1ZDU2Y2ZhZDI1N2MxNmI0MWY1MjFjZWUwODczNzExM2NlYzZkZDQwMWJmNyIsInJhbmRvbSI6MC4zNjcxNDg2NDI2MjE0MywiZXhwaXJlcyI6NzIwMH0=
+apisix-csrf-token: eyJyYW5kb20iOjAuNDI5ODYzMTk3MTYxMzksInNpZ24iOiI0ODRlMDY4NTkxMWQ5NmJhMDc5YzQ1ZGI0OTE2NmZkYjQ0ODhjODVkNWQ0NmE1Y2FhM2UwMmFhZDliNjE5OTQ2IiwiZXhwaXJlcyI6MjY0MzExOTYyNH0=
+Cookie: apisix-csrf-token=eyJyYW5kb20iOjAuNDI5ODYzMTk3MTYxMzksInNpZ24iOiI0ODRlMDY4NTkxMWQ5NmJhMDc5YzQ1ZGI0OTE2NmZkYjQ0ODhjODVkNWQ0NmE1Y2FhM2UwMmFhZDliNjE5OTQ2IiwiZXhwaXJlcyI6MjY0MzExOTYyNH0=
+
+
+
+=== TEST 10: change expired
+--- 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,
+ [[{
+ "uri": "/hello",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ },
+ "plugins": {
+ "csrf": {
+ "key": "userkey",
+ "expires": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 11: expired csrf token
+--- request
+POST /hello
+--- more_headers
+apisix-csrf-token: eyJyYW5kb20iOjAuMDY3NjAxMDQwMDM5MzI4LCJzaWduIjoiOTE1Yjg2MjBhNTg1N2FjZmIzNjIxOTNhYWVlN2RkYjY5NmM0NWYwZjE5YjY5Zjg3NjM4ZTllNGNjNjYxYjQwNiIsImV4cGlyZXMiOjE2NDMxMjAxOTN9
+Cookie: apisix-csrf-token=eyJyYW5kb20iOjAuMDY3NjAxMDQwMDM5MzI4LCJzaWduIjoiOTE1Yjg2MjBhNTg1N2FjZmIzNjIxOTNhYWVlN2RkYjY5NmM0NWYwZjE5YjY5Zjg3NjM4ZTllNGNjNjYxYjQwNiIsImV4cGlyZXMiOjE2NDMxMjAxOTN9
+--- error_code: 401
+--- error_log: token has expired
+
+
+
+=== TEST 12: set expires 0
Review comment:
please update it accordingly. Split it into 2 cases
- expires = 1 sec, sleep = 2 sec after drawing a token then check `token has expired`
- expires = 0, sleep = 1, no expiration
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #6201: feat: add expires timestamp for CSRF plugin
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #6201:
URL: https://github.com/apache/apisix/pull/6201#discussion_r792244700
##########
File path: t/plugin/csrf.t
##########
@@ -158,5 +159,45 @@ Cookie: apisix-csrf-token=eyJleHBpcmVzIjo3MjAwLCJyYW5kb20iOjAuMjE2ODAxOTYyNTEwND
--- request
POST /hello
--- more_headers
-apisix-csrf-token: eyJzaWduIjoiZTlhNWVkOTBmZDc2YjRhMTYyMzg1ZDU2Y2ZhZDI1N2MxNmI0MWY1MjFjZWUwODczNzExM2NlYzZkZDQwMWJmNyIsInJhbmRvbSI6MC4zNjcxNDg2NDI2MjE0MywiZXhwaXJlcyI6NzIwMH0=
-Cookie: apisix-csrf-token=eyJzaWduIjoiZTlhNWVkOTBmZDc2YjRhMTYyMzg1ZDU2Y2ZhZDI1N2MxNmI0MWY1MjFjZWUwODczNzExM2NlYzZkZDQwMWJmNyIsInJhbmRvbSI6MC4zNjcxNDg2NDI2MjE0MywiZXhwaXJlcyI6NzIwMH0=
+apisix-csrf-token: eyJyYW5kb20iOjAuNDI5ODYzMTk3MTYxMzksInNpZ24iOiI0ODRlMDY4NTkxMWQ5NmJhMDc5YzQ1ZGI0OTE2NmZkYjQ0ODhjODVkNWQ0NmE1Y2FhM2UwMmFhZDliNjE5OTQ2IiwiZXhwaXJlcyI6MjY0MzExOTYyNH0=
+Cookie: apisix-csrf-token=eyJyYW5kb20iOjAuNDI5ODYzMTk3MTYxMzksInNpZ24iOiI0ODRlMDY4NTkxMWQ5NmJhMDc5YzQ1ZGI0OTE2NmZkYjQ0ODhjODVkNWQ0NmE1Y2FhM2UwMmFhZDliNjE5OTQ2IiwiZXhwaXJlcyI6MjY0MzExOTYyNH0=
+
+
+
+=== TEST 10: expired csrf token
+--- config
+ location /t {
Review comment:
It seems that the test case doesn't access `/t`?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bisakhmondal commented on a change in pull request #6201: feat: add expires timestamp for CSRF plugin
Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on a change in pull request #6201:
URL: https://github.com/apache/apisix/pull/6201#discussion_r823996695
##########
File path: t/plugin/csrf.t
##########
@@ -158,5 +159,170 @@ Cookie: apisix-csrf-token=eyJleHBpcmVzIjo3MjAwLCJyYW5kb20iOjAuMjE2ODAxOTYyNTEwND
--- request
POST /hello
--- more_headers
-apisix-csrf-token: eyJzaWduIjoiZTlhNWVkOTBmZDc2YjRhMTYyMzg1ZDU2Y2ZhZDI1N2MxNmI0MWY1MjFjZWUwODczNzExM2NlYzZkZDQwMWJmNyIsInJhbmRvbSI6MC4zNjcxNDg2NDI2MjE0MywiZXhwaXJlcyI6NzIwMH0=
-Cookie: apisix-csrf-token=eyJzaWduIjoiZTlhNWVkOTBmZDc2YjRhMTYyMzg1ZDU2Y2ZhZDI1N2MxNmI0MWY1MjFjZWUwODczNzExM2NlYzZkZDQwMWJmNyIsInJhbmRvbSI6MC4zNjcxNDg2NDI2MjE0MywiZXhwaXJlcyI6NzIwMH0=
+apisix-csrf-token: eyJyYW5kb20iOjAuNDI5ODYzMTk3MTYxMzksInNpZ24iOiI0ODRlMDY4NTkxMWQ5NmJhMDc5YzQ1ZGI0OTE2NmZkYjQ0ODhjODVkNWQ0NmE1Y2FhM2UwMmFhZDliNjE5OTQ2IiwiZXhwaXJlcyI6MjY0MzExOTYyNH0=
+Cookie: apisix-csrf-token=eyJyYW5kb20iOjAuNDI5ODYzMTk3MTYxMzksInNpZ24iOiI0ODRlMDY4NTkxMWQ5NmJhMDc5YzQ1ZGI0OTE2NmZkYjQ0ODhjODVkNWQ0NmE1Y2FhM2UwMmFhZDliNjE5OTQ2IiwiZXhwaXJlcyI6MjY0MzExOTYyNH0=
+
+
+
+=== TEST 10: change expired
+--- 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,
+ [[{
+ "uri": "/hello",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ },
+ "plugins": {
+ "csrf": {
+ "key": "userkey",
+ "expires": 1
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 11: expired csrf token
+--- request
+POST /hello
+--- more_headers
+apisix-csrf-token: eyJyYW5kb20iOjAuMDY3NjAxMDQwMDM5MzI4LCJzaWduIjoiOTE1Yjg2MjBhNTg1N2FjZmIzNjIxOTNhYWVlN2RkYjY5NmM0NWYwZjE5YjY5Zjg3NjM4ZTllNGNjNjYxYjQwNiIsImV4cGlyZXMiOjE2NDMxMjAxOTN9
+Cookie: apisix-csrf-token=eyJyYW5kb20iOjAuMDY3NjAxMDQwMDM5MzI4LCJzaWduIjoiOTE1Yjg2MjBhNTg1N2FjZmIzNjIxOTNhYWVlN2RkYjY5NmM0NWYwZjE5YjY5Zjg3NjM4ZTllNGNjNjYxYjQwNiIsImV4cGlyZXMiOjE2NDMxMjAxOTN9
+--- error_code: 401
+--- error_log: token has expired
+
+
+
+=== TEST 12: token has expired after sleep 2s
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+
+ local uri = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/hello"
+
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {method = "GET"})
+ if not res then
+ ngx.say(err)
+ return
+ end
+ local cookie = res.headers["Set-Cookie"]
+ local token = cookie:match("=([^;]+)")
+
+ ngx.sleep(2)
+
+ local res, err = httpc:request_uri(uri, {
+ method = "POST",
+ headers = {
+ ["apisix-csrf-token"] = token,
+ ["Cookie"] = cookie,
+ }
+ })
+ if not res then
+ ngx.say(err)
+ return
+ end
+
+ if res.status >= 300 then
+ ngx.status = res.status
+ end
+ }
+ }
+--- error_code: 401
+--- error_log: token has expired
+
+
+
+=== TEST 13: set expires 0
+--- 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,
+ [[{
+ "uri": "/hello",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1
+ }
+ },
+ "plugins": {
+ "csrf": {
+ "key": "userkey",
+ "expires": 0
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 14: token no expired after sleep 1s
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+
+ local uri = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/hello"
+
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {method = "GET"})
+ if not res then
+ ngx.say(err)
+ return
+ end
+
+ ngx.sleep(1)
+
+ local cookie = res.headers["Set-Cookie"]
+ local token = cookie:match("=([^;]+)")
+
+ local res, err = httpc:request_uri(uri, {
+ method = "POST",
+ headers = {
+ ["apisix-csrf-token"] = token,
+ ["Cookie"] = cookie,
+ }
+ })
+ if not res then
+ ngx.say(err)
+ return
+ end
+
+ if res.status >= 300 then
+ ngx.status = res.status
+ end
+ ngx.say(res.body)
+ }
+ }
Review comment:
```suggestion
end
ngx.status = res.status
ngx.print(res.body)
}
}
--- response_body
hello world
```
I think you missed checking the `response-body`. Add this snippet. Thanks
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bisakhmondal commented on a change in pull request #6201: feat: add expires timestamp for CSRF plugin
Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on a change in pull request #6201:
URL: https://github.com/apache/apisix/pull/6201#discussion_r792358041
##########
File path: apisix/plugins/csrf.lua
##########
@@ -75,11 +75,12 @@ end
local function gen_csrf_token(conf)
local random = math.random()
- local sign = gen_sign(random, conf.expires, conf.key)
+ local expires_timestamp = ngx_time() + conf.expires
+ local sign = gen_sign(random, expires_timestamp, conf.key)
Review comment:
I'd say sign with the current timestamp. and at line 116 perform a diff `(ngx_time()-expires)` and test if it is greater than `conf.expires`. In this way, expiry is localized to apisix as it supports the current need of the user. Any mistake in setting up conf expiry (maybe a big value) value and the tokens generated with that value can't be used to exploit the infra because generated tokens can be revoked by reconfiguring the `expires` field.
Also, how about ignoring the check when conf.expires = 0
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Baoyuantop commented on a change in pull request #6201: feat: add expires timestamp for CSRF plugin
Posted by GitBox <gi...@apache.org>.
Baoyuantop commented on a change in pull request #6201:
URL: https://github.com/apache/apisix/pull/6201#discussion_r792674932
##########
File path: apisix/plugins/csrf.lua
##########
@@ -75,11 +75,12 @@ end
local function gen_csrf_token(conf)
local random = math.random()
- local sign = gen_sign(random, conf.expires, conf.key)
+ local expires_timestamp = ngx_time() + conf.expires
+ local sign = gen_sign(random, expires_timestamp, conf.key)
Review comment:
Thank you for your guidance.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org