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/01/05 03:48:10 UTC
[GitHub] [apisix-dashboard] imjoey opened a new pull request #1206: fix: well handle with malformed auth token in request header
imjoey opened a new pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206
Signed-off-by: imjoey <ma...@gmail.com>
Please answer these questions before submitting a pull request
- Why submit this pull request?
- [x] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- Related issues
Fixes #1204 .
___
### Bugfix
- Description
If the request to manager-api does not provide a valid authentication token, manager-api will panic. Please see #1204 for details.
- How to fix?
This PR is going to add an additional check that if `token` is `nil`. In the meantime, relative e2e test cases are also committed.
Any feedback is welcome. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] imjoey commented on a change in pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
imjoey commented on a change in pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#discussion_r551733304
##########
File path: api/internal/filter/authentication.go
##########
@@ -41,8 +42,8 @@ func Authentication() gin.HandlerFunc {
"message": "Request Unauthorized",
}
- if err != nil || !token.Valid {
- log.Warnf("token validate failed: %s, %v", err, token.Valid)
+ if err != nil || token == nil || !token.Valid {
+ log.Warnf("token validate failed: %s, %v", err, false)
Review comment:
@starsz Thanks. It's to keep log format that same as before. How about change this to
```suggestion
log.Warnf("token validate failed: %s", err)
```
----------------------------------------------------------------
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-dashboard] imjoey commented on pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
imjoey commented on pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#issuecomment-754468519
> @imjoey to 2.3 or master?
@juzhiyuan IMHO, this fix should be applied to both `v2.3` and `master`. Since the `v2.3` branch would be merged into master branch after the release of tag 2.3, this PR can be only applied to `v2.3` branch for now, which will be available in master branch some time later. 😄 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] imjoey commented on pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
imjoey commented on pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#issuecomment-754470012
> emmm let me do a cherry-pick from master to v2.3
That would be great since the `master` users could get this hot fix soon. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] juzhiyuan commented on pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#issuecomment-754465784
@imjoey to 2.3 or master?
----------------------------------------------------------------
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-dashboard] codecov-io edited a comment on pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#issuecomment-754373831
# [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=h1) Report
> Merging [#1206](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=desc) (d43dabd) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/45e90ec8e13a1c9659645034f1fd50f0552b93e3?el=desc) (45e90ec) will **not change** coverage.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1206 +/- ##
=======================================
Coverage 42.38% 42.38%
=======================================
Files 31 31
Lines 1930 1930
=======================================
Hits 818 818
Misses 1001 1001
Partials 111 111
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [api/internal/filter/authentication.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1206/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2ZpbHRlci9hdXRoZW50aWNhdGlvbi5nbw==) | `0.00% <0.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=footer). Last update [45e90ec...d43dabd](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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-dashboard] codecov-io commented on pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#issuecomment-754373831
# [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=h1) Report
> Merging [#1206](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=desc) (a148dc0) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/45e90ec8e13a1c9659645034f1fd50f0552b93e3?el=desc) (45e90ec) will **not change** coverage.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1206 +/- ##
=======================================
Coverage 42.38% 42.38%
=======================================
Files 31 31
Lines 1930 1930
=======================================
Hits 818 818
Misses 1001 1001
Partials 111 111
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [api/internal/filter/authentication.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1206/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2ZpbHRlci9hdXRoZW50aWNhdGlvbi5nbw==) | `0.00% <0.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=footer). Last update [45e90ec...a148dc0](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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-dashboard] juzhiyuan merged pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
juzhiyuan merged pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206
----------------------------------------------------------------
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-dashboard] starsz commented on a change in pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#discussion_r551758004
##########
File path: api/internal/filter/authentication.go
##########
@@ -41,8 +42,8 @@ func Authentication() gin.HandlerFunc {
"message": "Request Unauthorized",
}
- if err != nil || !token.Valid {
- log.Warnf("token validate failed: %s, %v", err, token.Valid)
+ if err != nil || token == nil || !token.Valid {
+ log.Warnf("token validate failed: %s, %v", err, false)
Review comment:
@imjoey Good. Thanks a lot.
----------------------------------------------------------------
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-dashboard] juzhiyuan commented on pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#issuecomment-754469546
emmm let me do a cherry-pick from master to v2.3
----------------------------------------------------------------
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-dashboard] imjoey commented on a change in pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
imjoey commented on a change in pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#discussion_r551755233
##########
File path: api/internal/filter/authentication.go
##########
@@ -41,8 +42,8 @@ func Authentication() gin.HandlerFunc {
"message": "Request Unauthorized",
}
- if err != nil || !token.Valid {
- log.Warnf("token validate failed: %s, %v", err, token.Valid)
+ if err != nil || token == nil || !token.Valid {
+ log.Warnf("token validate failed: %s, %v", err, false)
Review comment:
@starsz Done and PR renewed, thanks for your suggestion.
----------------------------------------------------------------
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-dashboard] codecov-io edited a comment on pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#issuecomment-754373831
# [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=h1) Report
> Merging [#1206](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=desc) (ed21881) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/45e90ec8e13a1c9659645034f1fd50f0552b93e3?el=desc) (45e90ec) will **decrease** coverage by `0.05%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1206 +/- ##
==========================================
- Coverage 42.38% 42.33% -0.06%
==========================================
Files 31 31
Lines 1930 1930
==========================================
- Hits 818 817 -1
- Misses 1001 1002 +1
Partials 111 111
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [api/internal/filter/authentication.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1206/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2ZpbHRlci9hdXRoZW50aWNhdGlvbi5nbw==) | `0.00% <0.00%> (ø)` | |
| [api/internal/core/store/store.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1206/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmUuZ28=) | `79.37% <0.00%> (-0.63%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=footer). Last update [45e90ec...ed21881](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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-dashboard] codecov-io edited a comment on pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#issuecomment-754373831
# [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=h1) Report
> Merging [#1206](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=desc) (b98d152) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/45e90ec8e13a1c9659645034f1fd50f0552b93e3?el=desc) (45e90ec) will **not change** coverage.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1206 +/- ##
=======================================
Coverage 42.38% 42.38%
=======================================
Files 31 31
Lines 1930 1930
=======================================
Hits 818 818
Misses 1001 1001
Partials 111 111
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [api/internal/filter/authentication.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1206/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2ZpbHRlci9hdXRoZW50aWNhdGlvbi5nbw==) | `0.00% <0.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=footer). Last update [45e90ec...b98d152](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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-dashboard] imjoey commented on a change in pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
imjoey commented on a change in pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#discussion_r551755233
##########
File path: api/internal/filter/authentication.go
##########
@@ -41,8 +42,8 @@ func Authentication() gin.HandlerFunc {
"message": "Request Unauthorized",
}
- if err != nil || !token.Valid {
- log.Warnf("token validate failed: %s, %v", err, token.Valid)
+ if err != nil || token == nil || !token.Valid {
+ log.Warnf("token validate failed: %s, %v", err, false)
Review comment:
@starsz Done and PR renewed, thanks for your suggestion. Review is welcome. 😄
----------------------------------------------------------------
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-dashboard] starsz commented on a change in pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#discussion_r551731839
##########
File path: api/internal/filter/authentication.go
##########
@@ -41,8 +42,8 @@ func Authentication() gin.HandlerFunc {
"message": "Request Unauthorized",
}
- if err != nil || !token.Valid {
- log.Warnf("token validate failed: %s, %v", err, token.Valid)
+ if err != nil || token == nil || !token.Valid {
+ log.Warnf("token validate failed: %s, %v", err, false)
Review comment:
Hi, I think `false` is not necessary to log.
----------------------------------------------------------------
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-dashboard] codecov-io edited a comment on pull request #1206: fix: well handle with malformed auth token in request header
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1206:
URL: https://github.com/apache/apisix-dashboard/pull/1206#issuecomment-754373831
# [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=h1) Report
> Merging [#1206](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=desc) (44bfaa2) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/45e90ec8e13a1c9659645034f1fd50f0552b93e3?el=desc) (45e90ec) will **not change** coverage.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1206 +/- ##
=======================================
Coverage 42.38% 42.38%
=======================================
Files 31 31
Lines 1930 1930
=======================================
Hits 818 818
Misses 1001 1001
Partials 111 111
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [api/internal/filter/authentication.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1206/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2ZpbHRlci9hdXRoZW50aWNhdGlvbi5nbw==) | `0.00% <0.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=footer). Last update [45e90ec...44bfaa2](https://codecov.io/gh/apache/apisix-dashboard/pull/1206?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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