Fix encoding for non-ASCII filenames in Content-Disposition header #58259
Conversation
Resolved incorrect encoding handling in Content-Disposition header
|
Review requested:
|
nodejs-github-bot
added
http
| @@ -635,6 +635,19 @@ function parseUniqueHeadersOption(headers) { | |||
|
|
|||
| return unique; | |||
| } | |||
| function maybeEncodeFilenameHeader(value) { | |||
| const filenameMatch = value.match(/filename="(.+?)"/); | |||
There was a problem hiding this comment.
If I'm reading this correctly it assumes that the value for the filename is always quoted. It's been a while since I've looked but I'm not sure if that's actually required by the relevant specs. RFC2183 where this is originally defined generally is quite careful about indicating when the value must be quoted and I don't recall anything there saying that filename must be a quoted string value.
There was a problem hiding this comment.
Thanks for pointing that out, @jasnell — you're absolutely right. The current implementation incorrectly assumes that the filename value is always quoted, which isn't guaranteed per RFC 2183. According to the spec, the filename parameter can be either a quoted-string or a token, so it's valid for it to appear without quotes.
I've updated the logic to handle both quoted and unquoted filename values using a more flexible regex:
-> const filenameMatch = value.match(/filename=(?:"([^"]+)"|([^;\s]+))/);
-> const filename = filenameMatch ? (filenameMatch[1] || filenameMatch[2]) : null;
Let me know if you think we should handle any additional edge cases. Thanks again!
Resolved incorrect encoding handling in Content-Disposition header