-
Notifications
You must be signed in to change notification settings - Fork 206
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
RequestTimeTooSkewed Error #74
Comments
Hi @mattsmith haha - this old chestnut ! Last time I thought about this, I came to the conclusion that the time stamp absolutely should come from the server, BUT there is no way to do that without breaking the API between the server and the client. That's because IIRC, the client needs to know the time for the request to AWS, so it would have to get it back in the response, and that changes the response format. We'd have to increment the version number! There's a bunch of other changes that would benefit from a more flexible response format, so this will happen one day. The question is: is this how we get started? If you'd like to take a crack at a PR I'd be very interested. I'd propose a JSON response. And maybe I'm wrong - maybe you can do it w/o changing the response... want to check? |
So had a quick check on how you handle the response, you check the length of the response which could be used to handle two "versions" of the endpoint without breaking old implementations. Kinda messy, implementing 0.1.x with a new endpoint spec would seem cleaner to me. I was looking at this ember addon and liked how they pushed all the configuration to the server (https://github.com/benefitcloud/ember-uploader/wiki/S3-Server-Setup). Thoughts? I'll take a crack at it and see if I can make some progress. |
that's cleaner for sure, but it forces someone implementing evaporateJS to do extra work. right now the server work required is very light. perhaps we could have a default option in which the very minimum is done by the server, and the option for the server to do more of the configuration. e..g. in the JSON response, the server has to include properties x, y, z, and optionally can include a,b,c, which if included override the client side settings. thoughts? |
@tomsaffell I was poking through the code and I find this: https://github.com/TTLabs/EvaporateJS/blob/master/evaporate.js#L580 Turns out, if you set timeUrl when creating the Evaporate object it will request the time from the server: new Evaporate({
signerUrl: '/sign_auth', # Do not change this in the example app
aws_key: 'your aws_key here',
bucket: 'your s3 bucket name here',
timeUrl: '/sign_auth/time',
}); All you have to do is respond with an RF 2616 date and you're fine. Maybe all that needs to be done is update the docs/wiki. |
woah! the power of open source! i didnt even know about that.. would you mind updating the readme and sending a PR? thanks! |
@tomsaffell Took a while but ran into this again so figured it was worth documenting correctly. |
@mattsmith , I looked at the code snippet and think it's not reliable because XMLHttpRequest requires an asynchronous callback. We're just assuming that by the time the request finishes, we have a date...but I don't think this can be guaranteed. If you tried this option, did it work reliably? var xmlHttpRequest = new XMLHttpRequest();
xmlHttpRequest.open("GET", con.timeUrl + '?requestTime=' + new Date().getTime(), false);
xmlHttpRequest.send();
requester.dateString = xmlHttpRequest.responseText; |
Even worse, we call it synchronously... |
Around Daylight Savings Time shift we started receiving issues with uploads failing silently. The underlying issue was AWS was responding to uploads with: RequestTimeTooSkewed - The difference between the request time and the current time is too large.
The date is using new Date().toUTCString(); which can be incorrect if the client's time is incorrect. I believe this should come from the server and not the client.
If I submit a pull request to fall back on the server time (received when signing the request) would this be accepted?
Also, I could be missing another way to handle this. Is there one?
The text was updated successfully, but these errors were encountered: