Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

beforeRequest hook is not called before the actual request #994

Closed
jameslshannon opened this issue Dec 14, 2019 · 9 comments · Fixed by #1051
Closed

beforeRequest hook is not called before the actual request #994

jameslshannon opened this issue Dec 14, 2019 · 9 comments · Fixed by #1051
Labels
bug Something does not work as it should ✭ help wanted ✭

Comments

@jameslshannon
Copy link

I had some AWS API integrations using Got v9.6.0 that were working fine until the Got dependency was updated to 10.0.4 by the server when all of a sudden the integrations broke. Despite the updated example code for aws4 integration using prefixUrl which I updated to, it still didn't work. Further debugging revealed that not only was aws4 rejecting the v10 options object (complaining it was type object and not string), even if I passed it as a string instead, the resultant options object from aws4 was completely mangled (properties concatenated, etc.) - hence why the integrations broke. As soon as I reverted my code and the Got dependency to 9.6.0 again, everything was back to working fine.

aws4 compatibility may therefore be worth looking at (I'm guessing this is commonly used) as well as the example shown at https://github.com/sindresorhus/got#aws - for now I'll therefore hold at 9.6.0

Thanks,

James

@szmarczak
Copy link
Collaborator

Could you post here the error that Got throws?

@jameslshannon
Copy link
Author

It's not a specific Got error (I think it was something like "body, form and json are mutually exclusive" or something - because the v10 options passed to aws4 are mangled before Got processes them it's not surprising since most of the required options are missing due to incorrect concatenation of various options properties. It's easy to reproduce using the sample code for AWS.

@szmarczak
Copy link
Collaborator

Thanks, will try it out.

@szmarczak
Copy link
Collaborator

Can you try again with v10.2.0? Unfortunately I don't use AWS, so I can't help you further.

@reconbot Would you be able to look into this?

@jameslshannon
Copy link
Author

Did some more testing with this today using 10.2.0. I can no longer reproduce the type/header mangling issue described above. However, it still doesn't play nicely with aws4.

To start with aws4 is dependent on both the path and hostname options which were previously set automatically when using baseUrl - the change to prefixUrl means that these both now need to be specified in addition to prefixUrl. Even with this change though, despite aws4 setting the AWS authentication headers in the options, I think the change to the json/body handling in v10 is causing issues with the aws4 signature calculation, resulting in either 403 or 400 errors with AWS requests.

I'm therefore having to stay on 9.6.0 and the sample code for aws4 integration should be commented as such.

@szmarczak szmarczak added bug Something does not work as it should ✭ help wanted ✭ labels Dec 31, 2019
@szmarczak szmarczak changed the title aws4 compatibility broken in v10 (tested 10.0.4) beforeRequest is not called before the actual request Dec 31, 2019
@szmarczak szmarczak changed the title beforeRequest is not called before the actual request beforeRequest hook is not called before the actual request Dec 31, 2019
@szmarczak
Copy link
Collaborator

You're right. After calling the beforeRequest hooks there's some body-normalization thing.

@szmarczak szmarczak mentioned this issue Jan 25, 2020
1 task
@reconbot
Copy link
Contributor

reconbot commented Jan 25, 2020

cc @mhart I'm not able to take a look right now

@mhart
Copy link

mhart commented Jan 25, 2020

Ay? Why cc me? aws4 hasn't changed anything 🤷‍♂

@szmarczak
Copy link
Collaborator

I believe that OP was using json option and the beforeRequest hook was called before body normalization. If anybody could point out if this is the faulty thing here, that would be great (I don't have an AWS account to test this). Anyway I'll try to fix this issue next week.

@szmarczak szmarczak mentioned this issue Feb 6, 2020
18 tasks
szmarczak added a commit to szmarczak/got that referenced this issue Feb 29, 2020
szmarczak added a commit to szmarczak/got that referenced this issue Mar 3, 2020
szmarczak added a commit to szmarczak/got that referenced this issue Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as it should ✭ help wanted ✭
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants