LPology / Simple-Ajax-Uploader

Javascript file upload plugin with progress bar support. Works in all major browsers, including IE7+, Chrome, Firefox, Safari, and Opera. No dependencies - use it with or without jQuery.
995 stars 267 forks source link

Add option not to URI-encode custom request headers #96

Closed madand closed 9 years ago

madand commented 9 years ago

I use Yii framework and encountered an issue with hard-coded URI-encoding of customHeaders option values. It brakes the CSRF token verification.

This simple patch makes URI-encoding optional. Also, resolves #93

madand commented 9 years ago

@budyaga thanks for sharing your code.

But I prefer to send CSRF token in HTTP headers, not as a request param. Here is how my code (for the patched uploader) looks like:

var myCustomHeaders = {};
myCustomHeaders['X-CSRF-Token'] = yii.getCsrfToken();
var uploader = new ss.SimpleUpload({
    ....
    customHeaders: myCustomHeaders,
    encodeCustomHeaders: false
});
krivochenko commented 9 years ago

@MadAnd at first I used your code, but then I had same problem with another plugin, I started looking for more universal variant. Make a patch for each plug-in is not very convenient. Not all plugins allow to send additional headers.

I think, good way for solved this problem, add this header to every AJAX requests:

$(function() {
    $.ajaxSetup({
        headers: { 'X-CSRF-Token': yii.getCsrfToken()}
    });
});

But, I think, it will not work for SimpleAjaxUploader, so it don't use jQuery.

madand commented 9 years ago

@budyaga I agree that making (header enabling) patches for every upload plugin is not very convenient, but it gives great opportunity to make theses plugins (and thus the world) better :smiley:

Yes $.ajaxSetup is convenient, but jQuery-only way. And it turns out, that doing AJAX without jQuery is not that scary as it might seem.

LPology commented 9 years ago

Just merged your pull request. Thanks for doing this.

LPology commented 9 years ago

After doing further research, I've decided to make the encodeCustomHeaders option default to false instead of true for greater consistency with HTTP 1.1 spec which states that header values should only be encoded if they contain characters from character sets other than ISO-8859-1.

This SO thread has a lot of good info: http://stackoverflow.com/questions/324470/http-headers-encoding-decoding-in-java

Because encoding header text is a remedy for non-standard use cases, encoding by default is an anti pattern. The fault is mine -- it was a mistake to accept a previous pull request which added URL encoding to headers by default. Thank you for providing the start to a solution for my error.

I'll be pushing an update shortly.

madand commented 9 years ago

@LPology the reason behind setting encodeCustomHeaders to true by default, was retaining backwards compatibility for existing users. So if you decided to change that mechanic, I would recommend to explicitly marking this as such (e.g. by bumping the major version and/or reflecting in docs etc.).

LPology commented 9 years ago

@MadAnd I figured that was probably your reasoning, and in most cases I fully agree with retaining backwards compatibility. This is only the second ever update to the plugin that could affect backwards compatibility (and the other was only a very minor change), so I'm right there with you on that.

In this case, we're correcting an anti pattern, while at the same time positively impacting what I suspect is a large majority of users. I suspect that only a small number of people prefer headers to be encoded out of compliance with HTTP spec.

I did bump the major version to 2.1, and while there is a note in the changelog, I'll update that as well to be more prominent, similar to the change notes for version 1.9.1 further down.

So, thank you again for your input on this. This mess was entirely of my own making, but I believe this has corrected the course going forward.

madand commented 9 years ago

@LPology I fully agree that sticking with the HTTP spec is the only right way to do the (useful) things in the Web.

Thank you for a great piece of simple yet powerful library!