Automattic / knox

S3 Lib
MIT License
1.74k stars 285 forks source link

Fixed special characters in filename for putFile() #225

Closed Albert-IV closed 10 years ago

Albert-IV commented 10 years ago

If headers are included, name header is not passed through any escape operations, so files with unicode characters will throw mystery errors in your application. I've included relavent tests that fail if the filename header is included.

I've added a helper function to the client to retrieve the key used for the x-amz-meta-filename header (as it can be cased different ways). I then use that to set an escaped filename in the merged headers, if passed.

Sorry about the slight formatting change, my text editor auto-trims on save.

domenic commented 10 years ago

Excellent work, thank you. Will test, merge, and release this weekend.

Albert-IV commented 10 years ago

Awesome, glad to hear.

There may be other methods that are vulnerable to having unicode characters in their filenames; I only found the putFile() bug thanks to a fun issue I tracked down on production.

I looked at put() and it seems to have its headers sanitized correctly.

domenic commented 10 years ago

Actually upon reviewing this I think it's not a good idea. If you set custom headers, it is your responsibility to encode them correctly in the form Amazon expects. Indeed, I believe a lot of Knox users are already doing that, and including this change would be a breaking change for them (it would cause double-encoding).

So I'm sorry but I think you need to do your header-encoding manually.

tantalum commented 10 years ago

So basically your keeping a bug to be backwards-compatible with people who had to work around your bug... Windows Is Still Alive!!

domenic commented 10 years ago

It's not a bug. Not every library does everything. Knox doesn't encode your headers for you if you forget.

Albert-IV commented 10 years ago

I understand not wanting to break existing user's functionality with double encoding.

I just think it's a little weird that a helper alias that is supposed to be easier than .put() is less safe than just using .put() directly (as that method correctly sanitizes filename header for the user).

Would it make sense to export the encodeSpecialCharacters() method somewhere to be used outside the internals of Knox? Its been documented in other issues here the Amazon S3 naming convention isn't as simple as encodeURIComponent()ing the filename.

Peronsally I've copied the encodeSpecialCharacters() code into my own project to encode the header correctly, but it might make sense to have it usable through knox instead.

domenic commented 10 years ago

put does nothing special with the x-amz-meta-filename header...

Albert-IV commented 10 years ago

Ahh, really? I thought I had read the filename went through some sanitization when I was going through the library. Whoops!

EDIT: I had misread, the filename that gets put into the client.request() method goes through sanitization, not the actual headers.