feat: serialize query params passed as argument #34
Conversation
Axios allows for query params to be passed as part of the config argument (instead of the url). This commit serializes the query params before signing. Before the query params were discarded and the signature did not match.
| @@ -44,6 +45,11 @@ export const aws4Interceptor = (options?: InterceptorOptions, credentials?: Cred | |||
| throw new Error("No URL present in request config, unable to sign request"); | |||
| } | |||
|
|
|||
| if(config.params) { | |||
| config.url = buildUrl(config.url, config.params, config.paramsSerializer); | |||
There was a problem hiding this comment.
I think mutating an incoming config is an action at a distance anti-pattern. Better to make a copy (deep clone).
There was a problem hiding this comment.
Not sure if I can follow regarding the anti-pattern. The params object would need to be deleted. I can create a clone at the beginning and refactor all following operations using the cloned config. But it adds overhead, if people use more complex custom serializer functions and stuff.
There was a problem hiding this comment.
As a package consumer, I do not expect the package to make any modifications to my input object.
Imagine I'd want to make two queries, with small modifications:
const req = {
method: "GET",
params: {
foo: 1
}
}
request(req)
// this request would fail, because `req.params` was clobbered
req.params.foo = 2
request(req)There was a problem hiding this comment.
I believe axios already uses a copy of the passed req. I added a test for illustration.
There was a problem hiding this comment.
You are right! Indeed, axios internals make a deep clone already.
| const request: AxiosRequestConfig = { | ||
| method: "GET", | ||
| url: "https://example.com/foobar", | ||
| params: {foo: "bar"}, |
There was a problem hiding this comment.
Need to add test data for for alphabetical sort of the params.
There was a problem hiding this comment.
Ignore this part. It was related to the comments above.
| @@ -44,6 +45,11 @@ export const aws4Interceptor = (options?: InterceptorOptions, credentials?: Cred | |||
| throw new Error("No URL present in request config, unable to sign request"); | |||
| } | |||
|
|
|||
| if(config.params) { | |||
| config.url = buildUrl(config.url, config.params, config.paramsSerializer); | |||
There was a problem hiding this comment.
Not sure if buildUrl does the alphabetical sort, but it is part of the signing procedure.
https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html
Sort the parameter names by character code point in ascending order. Parameters with duplicate names should be sorted by value. For example, a parameter name that begins with the uppercase letter F precedes a parameter name that begins with a lowercase letter b.
There was a problem hiding this comment.
buildUrl does not sort. I see it's stated in the docs. Testing against the Amazon seller api, which requires AWS4 signature, query param order made no difference. Signature matched, regardless param order.
There was a problem hiding this comment.
It definitely makes a difference. I have ran into it, but I don't remember with which call.
I have sorting added to my hack to make this work.
There was a problem hiding this comment.
It does not make a difference, since aws4 already sorts query params according to the spec. https://github.com/mhart/aws4/blob/a413aadd9e4b4e58842937a9ad53354be41ef4a1/aws4.js#L260
There was a problem hiding this comment.
The parsing of the object happens here:
https://github.com/mhart/aws4/blob/a413aadd9e4b4e58842937a9ad53354be41ef4a1/aws4.js#L123-L125
And it depends on the following param being set: signQuery.
But this package does not set this param, and thus query does not get parsed and does not get sorted.
There was a problem hiding this comment.
I was under the impression that the sorting only needs to happen for creating the signature.
Oh I see. So you think that the aws4 code will sort and sign it. And then when Amazon receives the request, they won't try to sign the URI as-is, but they'll parse it, and sort and sign also?
In other words what goes into the actual HTTP request does not matter?
There was a problem hiding this comment.
That was my assumption. But it is solely based on good faith in the aws4 author. If the actual http request would matter, implementing query param sorting in a signing lib like aws4 would make no sense to me. It would lead to missmatch errors all the time – like in our case where the url for signing would be correctly sorted but the actual url is not.
But truth be told: After googling about the signing processes it seems that different aws teams indeed implement the signing in various ways and I found a few issues that pointed out that the aws sdk tests are not 100% compliant with the spec as well. So I think you are right, it should be implemented.
There was a problem hiding this comment.
Also see #35. Not 100% related to this issue, but somewhat is.
There was a problem hiding this comment.
Okay, probably better to close this PR in favor of #35
There was a problem hiding this comment.
I think #35 is complimentary to this one. As @aws-sdk/signature-v4 replaces aws4 package. But we still need to handle the params.
|
@all-contributors please add @moltar for review |
|
@all-contributors please add @florianbepunkt for code |
|
I've put up a pull request to add @florianbepunkt! 🎉 |
Axios allows for query params to be passed as part of the config argument (instead of the url). This commit serializes the query params before signing. Before the query params were discarded and the signature did not match.
I added a separate test for this. Typing is rather loose but aligns with the @types/axios package.
Parameter serialization should be independent of whether a relative or absolute url was passed in the req config, therefore I added it as a first step.