Fix for invalid v4 signature w/ chunked file that contains non-ASCII characters in key #1632

e-tip · 2016-07-29T11:42:10Z

Brief description of the changes

Took the code from aws js sdk and ported to fineuploader replacing calls to encodeURIComponent for the key value. and removed an erroneous escape

What browsers and operating systems have you tested these changes on?

All browsers

Are all automated tests passing? [REQUIRED]

yes

Is this pull request against develop or some other non-master branch?

yes

Fixes #1630

CLAassistant · 2016-07-29T11:42:11Z

All committers have signed the CLA.

client/js/s3/s3.xhr.upload.handler.js

-                    return encodedKey.replace(/%2F/g, "/");
+                    var encodedKey = handler.getThirdPartyFileId(id);
+                    return qq.s3.util.uriEscapePath(encodedKey);
+                    // var encodedKey = encodeURIComponent(handler.getThirdPartyFileId(id));


rnicholus · 2016-07-29T14:36:34Z

Interesting. Thanks for the pull request. Some follow up questions:

What resource did you use to come up with the code you added?
Have you performed any manual tests? If so, can you explain what you tested and under what conditions?
Can you please write some unit tests to cover each of the two new methods you added?

client/js/s3/request-signer.js

@@ -148,7 +148,8 @@ qq.s3.RequestSigner = function(o) {
                if (queryParamIdx > 0) {
                    path = endOfUri.substr(0, queryParamIdx);
                }
-                return escape("/" + decodeURIComponent(path));
+                return "/" + path;
+                //return escape("/" + decodeURIComponent(path));


e-tip · 2016-07-29T15:02:29Z

Hi,
i took the added code from here
https://github.com/aws/aws-sdk-js/blob/master/lib/util.js#L46 and line #56
I came up with this solution because in aws documentation ( http://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html ) there's a section that suggests to use a custom function to URIEncode because
The standard UriEncode functions provided by your development platform may not work because of differences in implementation and related ambiguity in the underlying RFCs. We recommend that you write your own custom UriEncode function to ensure that your encoding will work.

rnicholus · 2016-07-29T15:12:25Z

Thanks for the update. We'll need to abide by the Apache 2.0 license, since the code you added is from the AWS JS SDK which uses this license. I'm not sure what specific requirements that license has for attribution - I haven't looked yet.

So, remaining here:

Manual testing in various v4 scenarios
Unit tests for the two new methods added
Proper attribution for the code copied from the AWS JS SDK.

e-tip · 2016-07-29T15:21:36Z

Regarding the Manual testing:
I've tried to upload in 2 different regions ( ireland and frankfurt ) this files and all succeded

button_border copia.pdf
e & o.pdf
orderform & provaè+àòù++àà.zip
clean_s3.zip
logo.png
no prof.jpg
in order to cover as much cases that i can. I ve tried with both

key : 'filename'

key : function(id) {
    return uploader.getName(id);
}

Regarding the license... do you think that's better to rewrite the code not using the aws code ?

rnicholus · 2016-07-29T15:48:05Z

do you think that's better to rewrite the code not using the aws code ?

Not necessarily. We just need to abide by the terms of the license. Most likely attribution is required. If so, I can be sure that this is respected in the appropriate location.

Other manual testing will need to completed, but I can take care of that as a final step once the other items above are covered.

e-tip · 2016-07-29T15:52:11Z

Now i'm on vacation for two weeks. is it a problem if i write the tests as soon as i return to work?

rnicholus · 2016-07-29T16:12:24Z

No problem. I'll try to take care of the license and manual testing myself in that timeframe.

e-tip · 2016-08-18T12:59:38Z

Here i'm, back from vacation.
As this is the first time i write tests, i have some difficulties to understand what needs to be tested. Can you please do an theorical example ( for example i would test if function returns right results if passing an empty string or something like that. )
Thanks and sorry for bothering you

rnicholus · 2016-08-18T19:25:17Z

I think some tests that target the new functions and the old one that was changed, specifically, are needed. Those should go in test/unit/s3/util.js. Then I would write or update at least one test in test/unit/s3/chunked-uploads.js to ensure non-ascii characters are properly encoded for all requests sent by the library.

I will follow-up with manual tests before merge.

e-tip · 2016-09-20T07:49:05Z

Sorry for the late feedback. I've wrote tests for the modified function and pushed to my fork

rnicholus · 2016-09-20T14:26:44Z

Great work, thanks. I'll run a few tests locally and then release this as 5.11.9 if all looks well. If this hasn't progressed by next week, feel free to ping me as it may have dropped off my radar.

rnicholus · 2016-09-20T15:47:15Z

Since some of this code was borrowed from the AWS SDK-JS project, we are (as far as I can tell) bound by the Apache 2.0 license, thus we must do the following:

include a copy of the license in any redistribution you may make that includes Apache software
provide clear attribution to The Apache Software Foundation for any distributions that include Apache software

But quite frankly, I'm not sure. Does this fall under "fair use" since you're only using a couple of regexps? Does "fair use" even apply to code? Maybe @lsegal can weigh in on this, since he contributed the referenced code to the AWS repo.

e-tip · 2016-09-20T17:22:35Z

Well... as the function is just a string replace with some extras i can rewrite it in many ways to get the same result

rnicholus · 2016-09-20T17:24:44Z

No need to re-write - just a question of attribution, which will be easily solved.

lsegal · 2016-09-20T17:29:00Z

Ownership for the code falls directly to AWS, so my response may not be authoritative in any way (since I'm not an Amazon lawyer, or any kind of lawyer frankly).

That said, this code looks fine to me personally. I would recommend that if you borrowed a function simply call that out in the code comments for the functions. A simple AWS escape function pulled from aws-sdk-js licensed under Apache 2.0 - http://github.com/aws/aws-sdk-js notice would probably make their end happy enough.

rnicholus · 2016-09-20T17:30:09Z

Thanks so much for your prompt response @lsegal. It looks like only a line here and there was borrowed, so a comment linking to the reference/inspiration seems reasonable to me. If it ever turns out we are in violation of the license in any sense (or of any license), I will make appropriate adjustments ASAP when prompted.

lsegal · 2016-09-20T17:51:04Z

@rnicholus I think that's great. I would only add that those functions were basically taken wholesale in their entirety (having touched those util functions quite a few times I immediately recognized the code when I saw the PR diff), so it's not quite "a line here and there". They were modified slightly (removed comments and added some callouts to qq), but simply attributing the full functions would be the right thing to do IMO.

rnicholus · 2016-09-20T17:55:15Z

You're absolutely right @lsegal. We'll go with the comment. If Amazon legal ever finds this to be unsuitable, don't hesitate to let me know and I will adjust post-haste.

@e-tip Do you think you could add a comment above the function(s) similar to the one mentioned a few comments above?

e-tip · 2016-09-20T19:11:09Z

@rnicholus , sure i can.

… under Apache 2.0 license

e-tip · 2016-09-22T08:06:13Z

Comment added

rnicholus · 2016-09-22T17:27:04Z

This will be released with 5.11.9. I may include one or two more cases in 5.11.9. That has yet to be determined.

#1632

tiziano added 2 commits July 29, 2016 12:21

Fixed issues with filename with strange chars in

7732d2b

Fixed issues in s3 uploader where signature isn't correctly computed

f26f0c9

rnicholus reviewed Jul 29, 2016
View reviewed changes

client/js/s3/s3.xhr.upload.handler.js

return encodedKey.replace(/%2F/g, "/");

var encodedKey = handler.getThirdPartyFileId(id);

return qq.s3.util.uriEscapePath(encodedKey);

// var encodedKey = encodeURIComponent(handler.getThirdPartyFileId(id));

This comment was marked as spam.

Sign in to view

rnicholus reviewed Jul 29, 2016
View reviewed changes

rnicholus added bug to fix waiting for feedback 3 - Doing S3 labels Jul 29, 2016

rnicholus added this to the 5.10.2 milestone Jul 29, 2016

rnicholus changed the title ~~Fix for issue #1630~~ Fix for invalid v4 signature w/ chunked file that contains non-ASCII characters in key Jul 29, 2016

Removed commented code

93bf45a

Merge branch 'develop' into master

2bd9c29

e-tip and others added 3 commits August 29, 2016 19:41

Merge branch 'develop' into master

494691c

Merge branch 'develop' into master

064e50a

Wrote tests for function uriEscapePath

2048f7d

rnicholus modified the milestones: 5.11.9, 5.10.2 Sep 20, 2016

Added comment that attributes uriEscape function code to AWS released…

0e5b911

… under Apache 2.0 license

rnicholus approved these changes Sep 22, 2016

View reviewed changes

rnicholus removed the waiting for feedback label Sep 22, 2016

rnicholus merged commit 2c73fdb into FineUploader:develop Sep 22, 2016

rnicholus added 5 - Done and removed 3 - Doing labels Sep 22, 2016

rnicholus added a commit that referenced this pull request Sep 22, 2016

chore(build): start of 5.11.9 release work

e4cbc93

#1632

rnicholus mentioned this pull request Oct 6, 2016

[S3] The request signature we calculated does not match the signature you provided #1630

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for invalid v4 signature w/ chunked file that contains non-ASCII characters in key #1632

Fix for invalid v4 signature w/ chunked file that contains non-ASCII characters in key #1632

e-tip commented Jul 29, 2016 •

edited by rnicholus

Loading

CLAassistant commented Jul 29, 2016 •

edited

Loading

This comment was marked as spam.

rnicholus commented Jul 29, 2016

This comment was marked as spam.

e-tip commented Jul 29, 2016

rnicholus commented Jul 29, 2016 •

edited

Loading

e-tip commented Jul 29, 2016 •

edited

Loading

rnicholus commented Jul 29, 2016

e-tip commented Jul 29, 2016

rnicholus commented Jul 29, 2016

e-tip commented Aug 18, 2016

rnicholus commented Aug 18, 2016

e-tip commented Sep 20, 2016

rnicholus commented Sep 20, 2016

rnicholus commented Sep 20, 2016

e-tip commented Sep 20, 2016

rnicholus commented Sep 20, 2016

lsegal commented Sep 20, 2016 •

edited

Loading

rnicholus commented Sep 20, 2016 •

edited

Loading

lsegal commented Sep 20, 2016 •

edited

Loading

rnicholus commented Sep 20, 2016 •

edited

Loading

e-tip commented Sep 20, 2016

e-tip commented Sep 22, 2016

rnicholus commented Sep 22, 2016

Fix for invalid v4 signature w/ chunked file that contains non-ASCII characters in key #1632

Fix for invalid v4 signature w/ chunked file that contains non-ASCII characters in key #1632

Conversation

e-tip commented Jul 29, 2016 • edited by rnicholus Loading

Brief description of the changes

What browsers and operating systems have you tested these changes on?

Are all automated tests passing? [REQUIRED]

Is this pull request against develop or some other non-master branch?

CLAassistant commented Jul 29, 2016 • edited Loading

This comment was marked as spam.

rnicholus commented Jul 29, 2016

This comment was marked as spam.

e-tip commented Jul 29, 2016

rnicholus commented Jul 29, 2016 • edited Loading

e-tip commented Jul 29, 2016 • edited Loading

rnicholus commented Jul 29, 2016

e-tip commented Jul 29, 2016

rnicholus commented Jul 29, 2016

e-tip commented Aug 18, 2016

rnicholus commented Aug 18, 2016

e-tip commented Sep 20, 2016

rnicholus commented Sep 20, 2016

rnicholus commented Sep 20, 2016

e-tip commented Sep 20, 2016

rnicholus commented Sep 20, 2016

lsegal commented Sep 20, 2016 • edited Loading

rnicholus commented Sep 20, 2016 • edited Loading

lsegal commented Sep 20, 2016 • edited Loading

rnicholus commented Sep 20, 2016 • edited Loading

e-tip commented Sep 20, 2016

e-tip commented Sep 22, 2016

rnicholus commented Sep 22, 2016

e-tip commented Jul 29, 2016 •

edited by rnicholus

Loading

CLAassistant commented Jul 29, 2016 •

edited

Loading

rnicholus commented Jul 29, 2016 •

edited

Loading

e-tip commented Jul 29, 2016 •

edited

Loading

lsegal commented Sep 20, 2016 •

edited

Loading

rnicholus commented Sep 20, 2016 •

edited

Loading

lsegal commented Sep 20, 2016 •

edited

Loading

rnicholus commented Sep 20, 2016 •

edited

Loading