fix(hmac-auth) use ngx var request_uri instead uri #3339
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shashiranjan84
force-pushed
the
fix/hmac-auth
branch
2 times, most recently
from
c9cbf3c to
738af76
Compare
shashiranjan84
force-pushed
the
fix/hmac-auth
branch
from
738af76 to
524eef2
Compare
|
lgtm |
thibaultcha
requested changes
Mar 27, 2018
| assert.res_status(200, res) | ||
| end) | ||
|
|
||
| it("should pass with GET with request-line having encoded query param", function() |
There was a problem hiding this comment.
what do we mean here by "encoded"? The below tests don't URL-encode the querystring which has an (invalid) space character.
If you wish to test a URL-encoded value, you have to encode it yourself (:send() doesn't do it for you)
There was a problem hiding this comment.
(It is a good idea to test this btw, because $request_uri will return the raw, encoded, URL, while other variables or APIs may not, so we definitely should test this 👍)
There was a problem hiding this comment.
Thanks @thibaultcha, I made bad assumption. I have encoded the parameters now.
| assert.res_status(200, res) | ||
| end) | ||
|
|
||
| it("should pass with GET with request-line having encode path param", function() |
shashiranjan84
force-pushed
the
fix/hmac-auth
branch
from
524eef2 to
eb7d77a
Compare
thibaultcha
reviewed
Mar 27, 2018
| it("should pass with GET with request-line having encoded query param", function() | ||
| local date = os.date("!%a, %d %b %Y %H:%M:%S GMT") | ||
| local escaped_uri = fmt("/request?name=foo bar", | ||
| ngx.escape_uri("foo bar")) |
There was a problem hiding this comment.
That won't work, the template string should contain %s in lieu of the hard-coded argument
There was a problem hiding this comment.
Indeed, I am blind, fixed now.
`ngx.var.uri` is normalized and so `ngx.var.request_uri` should be used to get request origional uri with arguments
shashiranjan84
force-pushed
the
fix/hmac-auth
branch
from
eb7d77a to
fd0fb89
Compare
thibaultcha
reviewed
Mar 28, 2018
| it("should pass with GET with request-line having encoded query param", function() | ||
| local date = os.date("!%a, %d %b %Y %H:%M:%S GMT") | ||
| local escaped_uri = fmt("/request?name=%s", | ||
| ngx.escape_uri("foo bar")) |
There was a problem hiding this comment.
btw, we could use ngx.encode_args() here, but that's fine
thibaultcha
approved these changes
Mar 28, 2018
kikito
pushed a commit
to kikito/kong
that referenced
this pull request
Apr 10, 2018
`ngx.var.uri` is normalized and so `ngx.var.request_uri` should be used to get the request original URI with arguments and preserved percent-encoding. From Kong#3339
mlehner616
added a commit
to mlehner616/kong
that referenced
this pull request
Aug 13, 2018
Fix PR Kong#3339 by adding support for older signatures generated from `request-line` without the query_string. To reduce performance impact, calculate the new signature version first. Only if that first validation fails, try again with the deprecated signing function.
thibaultcha
pushed a commit
that referenced
this pull request
Aug 17, 2018
Since #3339, signatures must be generated with querystring arguments. This is breaking for many clients still relying on the signature mechanism. This patch adds support for a fallback to the old signature generation (without querystring arguments) when the newer signature verification fails. Fix #3672 From #3699
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
ngx.var.uriis normalized and songx.var.request_urishould beused to get request origional uri with arguments
Full changelog
get original request uri