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

Support decode 'utf8' (no dash) #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alexstrat
Copy link

I stumbled upon a field using utf8 (no utf-8) as encoding.

@dougwilson dougwilson self-assigned this Mar 23, 2017
@dougwilson dougwilson added the pr label Mar 23, 2017
test/test.js Outdated Show resolved Hide resolved
@dougwilson
Copy link
Contributor

Thanks, this is great! The RFC notes that any encoding can be specified in the header and only makes the two we support a MUST. But, ideally the RFC specifies that it SHOULD only be registered charsets, which utf8 is not a registered charset (registrations: https://www.iana.org/assignments/character-sets/character-sets.xhtml). That doesn't mean we can't add this, but before doing so would like to scrutinize it to make sure we are making the right decision here.

Can you provide any details for where you ran into this use?

@alexstrat
Copy link
Author

I ran into this when downloading files from files.slack.com (a file uploaded to Slack):

curl -I --cookie "xxxxxxxxxx" https://files.slack.com/files-pri/T029MPGH6-F4NPHFJDS/download/pasted_image_at_2017_03_23_16_59.png
HTTP/1.1 200 OK
Content-Type: image/png
Content-Length: 28292
Connection: keep-alive
Accept-Ranges: bytes
Cache-Control: max-age=315360000, public
Content-Disposition: attachment; filename="Pasted image at 2017_03_23 16_59.png"; filename*=utf8''Pasted%20image%20at%202017_03_23%2016_59.png
Date: Thu, 23 Mar 2017 19:11:03 GMT
Etag: "485e0240d05d82f4b1055dbba3b88f01"
X-Backend: imgproxy-0e7899cd2fc239c39
X-Robots-Tag: noindex
X-Cache: Miss from cloudfront
Via: 1.1 3943e81340bd903a74d536bc9599c3f3.cloudfront.net (CloudFront)
X-Amz-Cf-Id: AKTTZG6-RJ1RUhnNfIMJCn7YE2Swo-NzpnGvpOiSg03lI1bL7k83eQ==

I was surprised as well.

@dougwilson
Copy link
Contributor

Awesome information, @alexstrat ! I'll get a new version published tonight (US) 👍

@dougwilson
Copy link
Contributor

dougwilson commented Mar 23, 2017

I've also been looking at other implementations and here is a running list of the results so far:

@alexstrat
Copy link
Author

alexstrat commented Mar 23, 2017

Another POV: should parse fail when content-disposition header contains a parameter which field is not decodable? Shouldn't it only ignore the parameter and move on?
(in my case i was only interested in the type)

@dougwilson
Copy link
Contributor

dougwilson commented Mar 23, 2017

Another POV: should parse fail when content-disposition header contains a parameter which field is not decodable? Shouldn't it only ignore the parameter and move on?

I think this is a great discussion and it would be a shame if it were to get either (a) lost as soon as this PR is merged & closed or (b) caused a derailment delaying the merge of the PR.

I would suggest opening a new issue raising this point. Some ideas on starting talking questions: (a) what specifically would be considered a recoverable parse failure vs an unrecoverable failure (b) when should be returned when the various different recoverable failures occur (c) how can we keep in tact a strict mode for any users who would want a complete failure (d) what, if any, guidance does the RFC have on this.

@alexstrat
Copy link
Author

Definitely! 👍 Opened #14

@dougwilson
Copy link
Contributor

Sorry, forgot this PR; your post on the other issue reminded me I never merged it 😢 I'll get an update to match Chrome's behavior, which would work here as well.

@s25g5d4
Copy link

s25g5d4 commented Jul 21, 2018

Hello, any update on this? I just came back to check the status of my issue (#20), and found here already a fix.

I actually wrote a patch that adds an option to ignore parsing errors on extended parameters, but after reading your comments in #14 I'm pretty sure you don't like the idea.

@s25g5d4
Copy link

s25g5d4 commented Jul 25, 2018

FYI: I reported the incorrect filename*=utf8 parameter problem to slack, and they fixed in 2 days. I'm surprised that it looks like no one reports the problem to slack before, and they fixed their server in such a short time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants