Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Need encodeURIComponent instead of encodeURI #216

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants

satb commented Jan 10, 2014

We need to use encodeURIComponent instead of encodeURI to account for special characters like '+' in the file name. This is especially so for those that directly upload to s3 from the browser where they have no control over filenames

Need encodeURIComponent instead of encodeURI
We need to use encodeURIComponent instead of encodeURI to account for special characters like '+' in the file name. This is especially so for those that directly upload to s3 from the browser where they have no control over filenames
Contributor

domenic commented Jan 10, 2014

I am almost certain this was changed to encodeURI in the past in order to fix another bug. Have you ensured that all the tests pass with your change?

Contributor

domenic commented Jan 10, 2014

Also, I will not merge this without a new test, which must fail previously, and succeed afterward.

satb commented Jan 10, 2014

No I haven't run the tests - it just made more sense that encodeURIComponent is the right thing to use. encodeURIComponent is used to encode the filename and special characters (other than single quote) is taken care of. Try it out with some special characters (say, a + in the file name) in the file name it will most certainly fail with encodeURI. If there was another bug that caused the change to encodeURI, I'll try to run the tests to see if everything passes.

Contributor

domenic commented Jan 10, 2014

Amazon's encoding rules are not normal URL encoding rules.

satb commented Jan 10, 2014

You may be right that Amazon has some exceptions, but encodeURI will just not work in some conditions. As I said, simply upload a file with a special character in it directly to S3 and try with encodeURI - it will fail. So encodeURI is not the right thing in all cases, yet I cannot be sure if encodeURIComponent is the right thing either that covers every case. Unless I know all of Amazon's rules, I can't be sure about what the exception cases are. Do you know of any documentation that tells what Amazon encoding won't work with standard encoding rules?

Contributor

domenic commented Jan 10, 2014

Please produce a failing test. I am not convinced by your theoretical concerns.

Contributor

domenic commented Jan 10, 2014

Do you know of any documentation that tells what Amazon encoding won't work with standard encoding rules?

Yes, the Amazon AWS documentation discusses the rules at length, and we implement them---using encodeURI.

satb commented Jan 10, 2014

The pull request is a direct result of a test that failed in production for us :). So you can rest assured its not "theoretical"

satb added some commits Jan 10, 2014

added test get with special character in file name
Added test case to include special characters in file name
adding ability to no encode - in case the files are submitted directl…
…y from client, then the URL might have to be encoded on the client itself so the form can be submitted properly.
Contributor

domenic commented Feb 24, 2014

I am very sorry for forgetting about this and not merging it.

Can you rebase your commits so that (a) they make no formatting changes at all, and in general don't change areas of Knox that are unrelated to the relevant bug; and (b) there is only one of them?

@domenic domenic closed this Jun 11, 2014

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