Add unprefixed S3 request headers support #1258

Merged
merged 2 commits into from Nov 10, 2015

Projects

None yet

4 participants

@sixlettervariables
Contributor

This addresses FineUploader/fine-uploader#1214 and adds support for a predefined list of headers which are appropriate for S3 without the x-amz-meta- prefix. Documentation may need a stronger notice about checking these as well. (Cr35/IE9)

@sixlettervariables sixlettervariables Updated s3's util.js to allow unprefixed headers
Add support for S3 uploading with valid unprefixed headers like Cache-Control or Content-Disposition. Removes requirement for monkey-patching qq.s3.util internals.
fe4586d
@sixlettervariables sixlettervariables Fixed jslint note about trailing whitespace
75b201f
@rnicholus
Member

Thanks for this. We'll sort out the build failure in the develop branch and take a closer look at this when we begin work on the next release.

@sixlettervariables
Contributor

You're welcome. This was by far and away the easiest to integrate uploader
which worked with S3. I've backported this successfully to 5.0.3 and am
using it in a QA environment now.

On Fri, Jul 25, 2014 at 11:07 AM, Ray Nicholus notifications@github.com
wrote:

Thanks for this. We'll sort out the build failure in the develop branch
and take a closer look at this when we begin work on the next release.


Reply to this email directly or view it on GitHub
#1258 (comment).

Christopher A. Watford
christopher.watford@gmail.com

@sixlettervariables
Contributor

@rnicholus, Let me know if you need me to rework this against 5.0.4.

@rnicholus
Member

No apparent merge conflicts. So, this is still clean. When we get around to this, we'll likely just merge it into develop anyway and make any required adjustments before making it live.

@stevenringo

Have noticed some issues with this:

  • parameters are URL encoded, resulting in headers with values such as attachment%3B%20filename%3Dfile.txt. You definitely don't want that.
  • I don't get why the default is to have x-amz- prefixes?
  • These seem never to get caught, as the name is munged into content-disposition for example:
case "Cache-Control":
case "Content-Disposition":
case "Content-Encoding":

Thanks!

@rnicholus
Member

@stevenringo Your comment doesn't appear to be related to this case.

@rnicholus
Member

...except for the last point

@rnicholus
Member

Regarding URL encoded params: this is necessary. In some cases, these params are sent as headers (when the S3 REST API is used). You can't have non-ascii chars in headers. Also, even when these params are sent in the message-body, any special characters are ripped out by S3 anyway, since these params are included as headers when S3 responds to a request for the object.

All custom params are prefixed with x-amz, per the S3 API specification.

@rnicholus
Member

Also, not sure what this comment means:

These seem never to get caught, as the name is munged into content-disposition for example:
case "Cache-Control":
case "Content-Disposition":
case "Content-Encoding":

The code looks correct to me.

@stevenringo

These are not custom headers. These are standard http headers as per rfc2616. Ideally then there should be two parameters; one for bona fide custom headers, that are not part of the spec (i.e. needs x-amz-), and one for headers that are part of the HTTP spec, respectively.

@stevenringo

Also, not sure what this comment means...

_getPrefixedParamName: function (name) seems to pass name as content-disposition (lowercases the actual header)

@rnicholus
Member

I think you are misunderstanding the context of this code. Custom headers in the context of Fine Uploader are any headers that the user/integrator specifies via the customHeaders option or API methods in Fine Uploader.

@rnicholus
Member

Not seeing where the header name's case is changed. Can you reference that line number?

@sixlettervariables
Contributor

@stevenringo, Amazon includes those as allowable un-prefixed headers, so they're included without the Amazon prefix.

@stevenringo

@sixlettervariables They should also not be URL encoded.

@rnicholus
Member

There isn't anything on that line or in that function that changes the case of the header name. And I explained earlier why parameter values are URL encoded.

@rnicholus rnicholus locked and limited conversation to collaborators Aug 28, 2014
@rnicholus
Member

I do however see your point about encoding the values of the non x-amz params.  We will ensure that is examined closer before this is merged in.  Thanks.

On Thu, Aug 28, 2014 at 5:00 PM, Steven Ringo notifications@github.com
wrote:

@sixlettervariables They should also not be URL encoded.

Reply to this email directly or view it on GitHub:
#1258 (comment)

@rnicholus rnicholus added the 2 - Do label Sep 21, 2014
@rnicholus rnicholus added this to the 5.3.0 milestone Apr 17, 2015
@Korijn Korijn added a commit to Korijn/fine-uploader that referenced this pull request Apr 30, 2015
@Korijn Korijn Add unprefixed Azure request headers support
Adds support for a predefined list of headers which are appropriate for
Azure without the x-ms-meta- prefix.

Resolves issue #1212 and is based on similar pull request #1258.
3bceeac
@Korijn Korijn added a commit to Korijn/fine-uploader that referenced this pull request Jul 6, 2015
@Korijn Korijn Add unprefixed Azure request headers support
Adds support for a predefined list of headers which are appropriate for
Azure without the x-ms-meta- prefix.

Resolves issue #1212 and is based on similar pull request #1258.
ed6255f
@rnicholus rnicholus modified the milestone: 5.3.0, 5.4.0 Jul 6, 2015
@rnicholus
Member

I'm going to work on integrating this change as I wrap up the 5.4.0 release.

@rnicholus rnicholus unlocked this conversation Nov 10, 2015
@rnicholus rnicholus merged commit 75b201f into FineUploader:develop Nov 10, 2015

0 of 2 checks passed

clahub Not all contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci The Travis CI build failed
Details
@rnicholus rnicholus added a commit that referenced this pull request Nov 10, 2015
@rnicholus rnicholus fix(s3): lint errors
closes #1258
714187b
@rnicholus rnicholus added a commit that referenced this pull request Nov 10, 2015
@rnicholus rnicholus chore(build): inc build num
closes #1258
8dfd0b0
@rnicholus rnicholus added 5 - Done and removed 2 - Do labels Nov 10, 2015
@rnicholus
Member

Changes are being staged in the develop branch as part of 5.4.0-9. I had to make a few small adjustments to address linting errors. Please let me know before the release (which is coming up soon) if anything is missing.

@sixlettervariables
Contributor

@rnicholus: +1 from me.

@arnoldad

@rnicholus, excellent work. This is going to save us some considerable SS overhead.

I'll experiment with the dev build on my own, but for the benifit of others:

  1. Do I assume correctly we will be able to set the content-disposition header with a call to the setCustomHeaders() method from within the onSubmitted callback?
  2. Had you given any thought to a boolean configuration option to set the content-disposition header on s3 automatically using the supplied filename? (this would look really good on your feature list, and probably covers most users' needs for setting s3 object headers)
@rnicholus
Member

@arnoldad You should be able to set any of the headers listed in the commit diff. Let me know if anything is not working as expected.

@arnoldad

@rnicholus, the devel branch has a slight issue (that I was easily able to work around).
in _getPrefixedParamName at fine-uploader/client/js/s3/util.js:73

Since the header names are coming in after being transformed with toLower() the switch cases with capitalization don't match.

I understand that this is to prevent signature mismatches, but when i correct the issue by creating an alternate case without capitalization, the uploads work flawlessly with the header set using the following code.

                onSubmitted: function () {
                  // get the filename of the most recently submitted file
                  var currentFileId = this._totalFilesInBatch - 1;
                  var filename = this.getName(currentFileId);

                  // add filename to amazon content-disposition header
                  var customHeaders = {
                    "Content-Disposition": "attachment; filename=\""+filename+"\""
                  };
                  this.setCustomHeaders(customHeaders, currentFileId);

                },
@rnicholus
Member

@arnoldad Thanks for the update. Can you submit a pull request against develop with your change? Do the unit tests pass with your change as well?

@arnoldad

Sure, I'll create a pull request after running unit tests. The Current workaround is a bit hacky. Do with it what you will. A more permanent solution might be to apply the toLower() after the param names are filtered with _getPrefixedParamName. (EDIT - The latter won't work as aws headers are case sensitive, and a lower-cased Content-Disposition header doesn't work in most browsers)

@rnicholus
Member

Thanks! Note that the unit tests cannot be run in Phantom anymore due to the age of Phantom 1.x and the serious issues with binaries in Phantom 2.x. So, Firefox is the suggested way to run them.

grunt clean dev test:firefox

I hope to remove grunt and simplify the build a lot as part of 6.0.

@rnicholus
Member

@arnoldad Looking at the pull request I merged in, it's clear that there is a lot missing, unfortunately. I also noticed that the "header" values are being unnecessarily URL encoded.

At this point, I don't intend to tackle this issue or #1387 as part of 5.4.0 anymore. Instead, I'm going to push them to a 5.5.0 release. 5.4.0 has been in progress long enough. I'd like to move to the testing phase followed by a release. 5.5.0 will, hopefully, be released in less than a couple months.

@rnicholus rnicholus removed this from the 5.4.0 milestone Nov 12, 2015
@rnicholus
Member

Remaining work moved to #1494.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment