Skip to content
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

[FIX] AmazonS3: Quote file.name for ContentDisposition for files with commas #8593

Merged
merged 1 commit into from
Oct 28, 2017

Conversation

@xenithorb
Copy link
Contributor Author

xenithorb commented Oct 22, 2017

The quicker this gets out the better because I have no idea how to retroactively fix these attributes because they're stored IN S3.

This was caused by #8296, which introduced the Content-Distribution attribute in order to de-obfuscate names during download, the problem is/was that the ${ file.name } item wasn't quoted and produced double headers because of the comma in the file names due to a amazon S3 bug.

Firefox apparently happily ignores the double headers, but chromium and all chromium/blink based browsers do not and throw up a ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_DISPOSITION error, causing the image to not be displayed for those type of clients. This CAN be fixed by retroactively adding quotes around the filename ( so filename=a , b would need to be changed to filename="a , b" ). The change here is doing this after the fact, but does not fix the images that will be brokenly displayed since #8296 was deployed.

@xenithorb
Copy link
Contributor Author

Also see this badly placed comment, that I totally did not see until after the fact, lol. : 3e72e0c#commitcomment-25082377

Copy link
Member

@geekgonecrazy geekgonecrazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this only makes sense

A comma in there with out quotes would mess up parsing and cause all kinds of issues especially with s3.

@xenithorb
Copy link
Contributor Author

Bear in mind one thing worth nothing that I just thought of are files with double quotes now.. Not exactly sure how that will be handled (i.e. escaped properly), but it should be a lesser occurrence than other odd chars in file names.

@rodrigok rodrigok added this to the 0.59.3 milestone Oct 28, 2017
@rodrigok rodrigok merged commit 4168dcb into RocketChat:develop Oct 28, 2017
@xenithorb xenithorb deleted the s3_contentdist_quote_fix branch October 28, 2017 21:38
rodrigok added a commit that referenced this pull request Oct 29, 2017
[FIX] AmazonS3: Quote file.name for ContentDisposition for files with commas
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants