Unable to upload file to S3 if computer's time is not synchronized #1387

Closed
alexsaveliev opened this Issue Mar 19, 2015 · 23 comments

Projects

None yet

5 participants

@alexsaveliev

Environment:

  • fineuploader 5.1.3
  • Chrome/Windows (I believe it doesn't matter)

Steps to reproduce:

Problem description:

An error appears

<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>AccessDenied</Code><Message>Invalid according to Policy: Policy expired.</Message><RequestId>...</RequestId><HostId>.....</HostId></Error>

Console output:

[Fine Uploader 5.1.3] Received 1 files.
all.fine-uploader-5.1.3.min.js:16 [Fine Uploader 5.1.3] Attempting to validate image.
all.fine-uploader-5.1.3.min.js:16 [Fine Uploader 5.1.3] Generating new thumbnail for 0
all.fine-uploader-5.1.3.min.js:16 [Fine Uploader 5.1.3] Attempting to draw client-side image preview.
all.fine-uploader-5.1.3.min.js:16 [Fine Uploader 5.1.3] Attempting to determine if sample.mp3 can be rendered in this browser
all.fine-uploader-5.1.3.min.js:16 [Fine Uploader 5.1.3] First pass: check type attribute of blob object.
all.fine-uploader-5.1.3.min.js:16 [Fine Uploader 5.1.3] sample.mp3 is not previewable in this browser per the blob's type attr
all.fine-uploader-5.1.3.min.js:16 [Fine Uploader 5.1.3] Not previewable
all.fine-uploader-5.1.3.min.js:16 [Fine Uploader 5.1.3] Sending simple upload request for 0
all.fine-uploader-5.1.3.min.js:16 [Fine Uploader 5.1.3] Submitting S3 signature request for 0
all.fine-uploader-5.1.3.min.js:16 [Fine Uploader 5.1.3] Sending POST request for 0
all.fine-uploader-5.1.3.min.js:16 [Fine Uploader 5.1.3] Sending upload request for 0
upload.fineuploader.com/:1 POST http://upload.fineuploader.com/ 403 (Forbidden)
all.fine-uploader-5.1.3.min.js:16 [Fine Uploader 5.1.3] Received response status 403 with body: <?xml version="1.0" encoding="UTF-8"?>
<Error><Code>AccessDenied</Code><Message>Invalid according to Policy: Policy expired.</Message><RequestId>E0B80E0216D27E32</RequestId><HostId>XEKD0yb95aoI87baTkof+u/rXRvQG6OV5xvG3McLdZ/ZxFGg/Y+OawFCP0bLEp9I3Eq63MEcC00=</HostId></Error>
all.fine-uploader-5.1.3.min.js:16 [Fine Uploader 5.1.3] Simple upload request failed for 0

I believe it's because JS code adds 5 minutes to the current date when making a signature

expirationDate = new Date(),
....
policy.expiration = qq.s3.util.getPolicyExpirationDate(expirationDate);
....
date.setMinutes(date.getMinutes() + 5);

I suggest to add an optional 'policy expiration' parameter that will control expiration threshold. By default it may be set to 5 minutes

Sincerely, Alex

@rnicholus
Member

Yes, this is correct and by design, and you'll really want to ensure that client machines have reasonably accurate time, not just for Fine Uploader or S3.

We can consider adding such an option in a future release. In the meantime, you can probaby update the expiration server-side as part of the policy signing process.

@mcorrigan

I second correcting this. We have thousands of users trying to upload everyday, some of which have had incorrect system times. While that I agree that their system times should be accurate, it seems unreasonable for us to expect it to always be so. I don't know of any professional web services that fail due to an incorrect system time on the client. As the policy is being processed by our own servers first (validation and signing), it would make sense to use the time on the server for these requests. As a paid customer, we hope you will strongly consider making this change.

Thanks for all your hard work on this.

@rnicholus
Member

I don't see how we will be able to consider server-time, without some breaking changes or complex code adjustments. This is especially true as we look into support for v4 signatures. However, we will consider allowing the 5 minute expiration time to be adjusted via a client-side option.

@schmuhl
schmuhl commented May 13, 2015

We are seeing a lot of failed uploads because of this. While I am stupefied by the apparent number of people with bad system times, this should not be a prerequisite for them using our site. Otherwise we love the functionality of FineUploader as it solves a number of problems for us.

Is there any way - or incentive - that can expedite the inclusion of the configurable expiration time?

@rnicholus
Member

You have at least a couple options, all are fairly easy to achieve since Fine Uploader is a JavaScript library.

  1. Override the qq.s3.util.getPolicyExpirationDate function with your own logic. This can perhaps be simply done by keeping a copy of the original implementation in scope, updating the passed Date to reflect your desired expiration date, and then calling the original function with the modified Date.
  2. Modify the code locally to change the + 5 minutes part of the expression in getPolicyExpiration to your desired value.

In both cases, you'll need to be conscious of changes to these methods when Fine Uploader is updated, but I don't intend to make any breaking changes to this API. I intend to make this possible either as part of #1336 or shortly thereafter in Fine Uploader 5.3.

@mikesherov

@rnicholus the same cause of this bug infects the setting of the x-amz-date header, which is not configurable on the amazon side so can't be worked around in the way you suggested here: #1387 (comment)

While I understand that a user should keep their clock up to date, we're using the S3 uploader in production for millions of users, and telling each of them to update their clock is not really a workable solution.

To mitigate this issue, I've modified my local copy of fineuploader to account for clock drift in the following way:

  1. On page load, pass down the unix timestamp generated by the server as a global variable.
  2. Calculate clock drift and store it as a param for the signatur request:
signature: {
          endpoint: CONFIG.cloud_uploader.signature.endpoint,
          params: {
            drift: (UNIX_TIMESTAMP * 1000) - Date.now()
          },
         ...
  1. Hack fineuploader getToSign as follows:
headers["x-amz-date"] = new Date().toUTCString();

to:

var driftAdjustedDate = Date.now() + options.signatureSpec.params.drift;
headers["x-amz-date"] = new Date(driftAdjustedDate).toUTCString();

I know this is a hack, but a better written patch can be done without breaking backwards compatibility nor a complicated patch. Please consider fixing this issue as it will improve the reliability of the S3 solution for large sites.

@rnicholus rnicholus added 3 - Doing and removed 0 - Discuss labels Nov 5, 2015
@rnicholus rnicholus added this to the 5.4.0 milestone Nov 5, 2015
@rnicholus
Member

I moved this into the schedule for 5.4.0. In order to account for user time issues, I intend to provide a new option that will allow you to specify the relative policy expiration date. It will default to 5 minutes from present, which is the current hardcoded value that has caused some problems for all who have weighed in on this case.

@mikesherov

@rnicholus, thanks for responding. I'm afraid the proposed solution will not work for the issue I outlined in #1387 (comment)

Correct me if I'm wrong, but a more robust solution involves passing server time as a drift parameter to account for users clocks being wrong.

I really love Fineuploader, and dislike having to maintain a hacked fork. Would you be willing to accept a patch that introduces a drift parameter?

@rnicholus
Member

@mikesherov You're correct, I forgot about x-amz-date. Too much time has passed since this issue was created, and I must have forgotten about that detail. Thanks for reminding me. I'm certainly open to a patch. It would be great if you have a unit test as well, or at least passing unit tests after the change. But if not, I'll take care of that as part of my review of the code. If you submit a pull request, please do so against the develop branch.

I still hope to get this in as part of 5.4.0, which is nearing completion, and already S3-themed.

@mikesherov

Yeah, I'll write a test as well. Are we in agreement on the interface? I'll add a drift parameter? I no longer think it belongs as a key on the signature object but rather a key on the main options object since it effects more than just signature signing.

Thoughts?

@rnicholus
Member

Just deleted my last comment after thinking it over some more.

I can see it being a standalone option property (root-level) or on the request object option. Either way. Are you expecting this value to be a date? If so, it should be in ISO8601 format to make parsing easier. Fine Uploader will need to reformat it and generate future dates in order to properly support POST and multipart upload API requests across v2 and v4 signatures.

Don't worry too much about the tests if they take up too much of your time. I'll make sure they are in there either way.

@mikesherov

OK, I'll put it on the request object, but how about the date being handed in being the unix timestamp in milliseconds, so the code would still be able to determine drift easily by doing serverDate - Date.now()?

@rnicholus
Member

Yes, I think that should work.

@rnicholus
Member

@mikesherov Due to scheduling obstacles that would prevent a release until the end of December, I'm aiming to release no later than 18 Nov. Will you possibly have a PR ready within the next few days? If not, I'll do my best to implement this feature as we have discussed so I can get this in for the 5.4.0 release.

@mikesherov

I'll see if I can get this done today. If not, I'll defer to you. Thanks for your patience and support!

@rnicholus rnicholus modified the milestone: 5.5.0, 5.4.0 Nov 12, 2015
@rnicholus
Member

I'm pushing this, along with additional required work described at the bottom of #1258, to a 5.5.0 release. I don't think I can get out 5.4.0 in the next couple weeks if I include these cases. After 5.4.0 releases, I expect there to be other requests that pop up after some start to make use of v4 signature support. Any of those non-critical issues will also be part of 5.5.0.

@mikesherov

OK, thats fine. Just let me know if you want me to work on this after 5.4 is out. Thanks again for all your hard work!

@rnicholus rnicholus added 2 - Do and removed 3 - Doing labels Nov 13, 2015
@rnicholus rnicholus added a commit that referenced this issue Dec 18, 2015
@rnicholus rnicholus start of work on #1387 for 5.5.0 562ce1a
@rnicholus rnicholus added a commit that referenced this issue Dec 18, 2015
@rnicholus rnicholus feat(s3): introduction of drift param + docs
TODO:
   - update x-amz-date using drift value
   - update policy expiration date using drift value
#1387
357541e
@rnicholus
Member

work on this case has started for 5.5.0

@rnicholus rnicholus added 3 - Doing and removed 2 - Do labels Dec 18, 2015
@rnicholus rnicholus added a commit that referenced this issue Dec 21, 2015
@rnicholus rnicholus fix(build): lint warnings
closes #1387
dbb7e1d
@rnicholus
Member

This feature is now complete and on the develop branch. Can someone pull down 5.5.0-1 and verify that this fixes your clock drift issues? Once this has ben verified by at least one user, I'll consider it complete and ready to be released with the rest of 5.5.0.

@mikesherov

Looks good to me.

@rnicholus
Member

@schmuhl @mcorrigan @alexsaveliev Are you still interested in this feature? If so, can you verify that this change addresses the issue for you?

@rnicholus
Member

Since I haven't received any further feedback, I'm going to call this done and ready for 5.5.0. After I complete #1422, I plan on cutting a new release.

@rnicholus rnicholus closed this in 6dd5031 Jan 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment