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

Path with %2B does not get correct signature #115

Closed
adityapatadia opened this issue Aug 11, 2020 · 17 comments
Closed

Path with %2B does not get correct signature #115

adityapatadia opened this issue Aug 11, 2020 · 17 comments

Comments

@adityapatadia
Copy link

Path like this does not get signed correctly: https://college-static.s3.ap-south-1.amazonaws.com/public/asset/img/course_assets/10%2B2.jpg

@mhart
Copy link
Owner

mhart commented Aug 11, 2020

Can you post the code you're using?

@adityapatadia
Copy link
Author

adityapatadia commented Aug 12, 2020

I am just signing with simple code:

const got = require("got");
let parsedUrl = url.parse(fullUrl);

let signHeads = aws4.sign({
      service: 's3',
      host: parsedUrl.host,
      path: parsedUrl.path,
      method: 'GET'
    }, {
      accessKeyId:<awskey>,
      secretAccessKey:<awssecret>
    });
let requestOptions = {};
requestOptions.headers = signHeads.headers;

let response = await got.get(fullUrl, requestOptions);

This line here is creating the problem: https://github.com/mhart/aws4/blob/master/aws4.js#L262

When url mentioned above is passed, the %2B sign gets converted to + by decodeUrl function and then it's replaced by space chracter ' '. This basically creates signature with wrong path.

@mhart
Copy link
Owner

mhart commented Aug 12, 2020 via email

@adityapatadia
Copy link
Author

adityapatadia commented Aug 12, 2020

I use GOT (https://www.npmjs.com/package/got) to make http requests. I have updated my code above.

@adityapatadia
Copy link
Author

I removed the part where + sign gets replaced by blank space and everything is working as expected.

@mhart
Copy link
Owner

mhart commented Aug 12, 2020

Right, the problem is you're giving a URL that has an escaped character in it already, then parsing the URL, which unescapes the character, but then you're not using the path given to you from aws4.sign, you're using the original path.

The docs for got say that it can take the same options as Node.js https – so you should just be able to do:

const got = require('got');
let parsedUrl = url.parse(fullUrl);

let requestOptions = aws4.sign({
      service: 's3',
      host: parsedUrl.host,
      path: parsedUrl.path,
      method: 'GET'
    }, {
      accessKeyId:<awskey>,
      secretAccessKey:<awssecret>
    });

let response = await got(requestOptions);

@mhart
Copy link
Owner

mhart commented Aug 12, 2020

You can double-check that the signing is working correctly by using the host and path directly:

const got = require('got');

let requestOptions = aws4.sign({
      service: 's3',
      host: 'college-static.s3.ap-south-1.amazonaws.com',
      path: '/public/asset/img/course_assets/10%2B2.jpg',
      method: 'GET'
    }, {
      accessKeyId:<awskey>,
      secretAccessKey:<awssecret>
    });

let response = await got(requestOptions);

@adityapatadia
Copy link
Author

adityapatadia commented Aug 12, 2020

The URL parser does not unescape character. In fact, your above code also produces signatureDoesNotMatch error from S3.

Following is output from aws4.sign

{
  service: 's3',
  host: 'college-static.s3.ap-south-1.amazonaws.com',
  path: '/public/asset/img/course_assets/10%2B2.jpg',
  method: 'GET',
  headers: {
    Host: 'college-static.s3.ap-south-1.amazonaws.com',
    'X-Amz-Content-Sha256': 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855',
    'X-Amz-Date': '20200812T122909Z',
    Authorization: 'AWS4-HMAC-SHA256 <redacted>'
  }
}

@mhart
Copy link
Owner

mhart commented Aug 12, 2020

You're right – my bad. Actually aws4 is expecting unescaped characters – s3 is particularly bad with character escapes, which makes this case tricky.

Does it work when the path is /public/asset/img/course_assets/10+2.jpg?

@adityapatadia
Copy link
Author

Nope, then S3 throws 404.

<?xml version="1.0" encoding="UTF-8"?>
<Error>
    <Code>NoSuchKey</Code>
    <Message>The specified key does not exist.</Message>
    <Key>public/asset/img/course_assets/10 2.jpg</Key>
    <RequestId>96FB45368D589404</RequestId>
    <HostId>Xb5HgUhZ+pui39dLhGoeEYdItw0N6eqhYJDquVCRvXVtL9Epz/IulE7CymdDRjGXUjRcFDfPtQs=</HostId>
</Error>

As you can see the plus sign gets replaced by space.

@adityapatadia
Copy link
Author

@mhart
Copy link
Owner

mhart commented Aug 12, 2020

So the signature is correct, but S3 itself is replacing the + with a space. Sigh.

Ok – looks like I need to create another test case. There are already dozens in

describe('#canonicalString()', function() {
– but it looks like this one slipped through.

Thanks for bringing it to my attention – I'll double check how it works with the other services

@mhart
Copy link
Owner

mhart commented Aug 12, 2020

Does it work if that line is replaced with:

if (decodePath) piece = decodeURIComponent(piece.replace(/\+/g, ' '))

???

I think that way I can deal with ppl passing in URLs that have a + in them (that S3 will convert to a space) and also passing in an explicit %2B

@adityapatadia
Copy link
Author

Yes this works fine.

@mhart
Copy link
Owner

mhart commented Aug 12, 2020

Ok thanks – will run a bunch of tests against various services and double-check it's compatible 👍

@mhart mhart closed this as completed in 6b5da6f Aug 12, 2020
@mhart
Copy link
Owner

mhart commented Aug 12, 2020

Thanks so much! And apologies again for assuming the lib was doing this correctly – it was a regression introduced 9 months ago which I should've had tests for 😞

Should be fixed in v1.10.1

@adityapatadia
Copy link
Author

Nevermind. Thanks a lot for making a quick fix and release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants