added optional query parameter to s3.get

[pconstr]

In order to page through the list of objects in a bucket, it is necessary to pass parameters to GET

As far as I could see, this is not possible with s3.get as it stands.
Appending a query string to the path parameter breaks authorization, and would be kludgy anyway.

So I propose adding a optional query parameter to s3.get

[SaltwaterC]

Kudos to Amazon for this screw up. Or should I say backward incompatible change into the signing algorithm. The mindless drones in charge with the APIs done it again.

Thing is that passing a query param to path used to work. GET ?acl or GET object?acl still works, but GET ?marker=object or GET ?max-keys=100 does not work as S3 API behaves like it never heard of those parameters. Behaves like GET ?foo.

I have a bucket with over 150k objects in it and I had to iterate that bucket about three weeks ago in order to run some cleanup tasks. GET ?marker=object used to work back then. I still have the script that I used. Now it throws 403 errors.

Some info from a debug version:

GET /?acl => to sign string:

GET


Tue, 06 Mar 2012 09:24:50 GMT
/bucket-name/?acl

=> works without error.

GET /?max-keys=10 => to sign string:

GET


Tue, 06 Mar 2012 09:27:07 GMT
/bucket-name/?max-keys=10

=> error with StringToSign: 'GET\n\n\nTue, 06 Mar 2012 09:27:07 GMT\n/bucket-name/' (o_O)

I'll merge your solution since it works by avoiding to pass the query params to the signing path, but I have to make sure that the rest of the things that should be passed to path (such as ?acl, ?location, ?torrent, etc) won't break.

[pconstr]

Interesting. Indeed this looks a bit more complicated than I thought: http://docs.amazonwebservices.com/AmazonS3/latest/dev/RESTAuthentication.html

Thanks for reacting quickly to this.

[SaltwaterC]

Figured out the whole thing. Since the latest modifications of the signing method, S3 makes a clear distinction between subresources and request parameters. Technically both are query arguments, but from the signing point of view they are different:

• the subresources are part of the "string to sign" and must be sorted in their lexicographic order
• the request parameters must not be part of the "string to sign"

I changed a little bit your idea:

• query is merged in path in internals.checkPath() and things like s3.get('?uploads', {'max-uploads': 10}, [...]) are now properly evaluated
• the internals.standardHeaders() parses the path and signes only the subresources after it properly sorts the arguments
• backward compatibility aka s3.get('?uploads&max-uploads=10', [...]) evaluates the same as your method s3.get('?uploads', {'max-uploads': 10}, [...]) in order to keep the existing code happy

I'll publish the new version ASAP. I still have to write some unit tests.

[pconstr]

Nice. I'm looking forward to the new version.

[SaltwaterC]

v0.6.6 is out. Still have to write the docs for query / s3.get(), but the functionality is there.

[pconstr]

Just gave it a try and it looks like there's still something wrong.
If I set a marker or prefix parameter with / characters, in the response xml I see those escaped to %2F
Perhaps they are getting escaped twice?

I'll try to find some time later today to have a look at the code.

[SaltwaterC]

I'll have a look. Due to the way querystring.stringify() works, it may break some things. I may use the manual merging method as I did for subresources where the above method doesn't do it as S3 expects it. This is the code that may break it:

    if ( ! tools.isEmpty(query)) {
        query = qs.stringify(query);
        if (path.indexOf('?') === -1) {
            path += '?';
        } else {
            path += '&';
        }
        path = path + query;
    }
    path = encodeURI(path);
    return path;

I'll have to properly test it after a meeting.

[SaltwaterC]

Checked the issue. Indeed, querystring.stringify() encodes the values. This replaces it:

        var queryPieces = [];
        for (var i in query) {
            queryPieces.push(i + '=' + query[i]);
        }
        query = queryPieces.join('&');

I'll publish a new version ASAP.

[SaltwaterC]

Published a new version that contains the above patch. Please let me know if everything is OK for you.

[pconstr]

just checked. everything is working correctly for me. thanks