-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
http: handle multi-value content-disposition header correctly #50977
http: handle multi-value content-disposition header correctly #50977
Conversation
Review requested:
|
Fixes #50978 |
@ShogunPanda @mcollina tagging you to get your eyes on this since you reviewed the related PR before. This change is important because in some libraries, headers are always passed as arrays (usually single value) to |
@ArsalanDotMe can you amend commit message, the ci is failing |
Headers in nodejs can be arrays and current workaround for content-disposition header do not take this into account. This change fixes that and makes sure array values are handled properly.
yes, sure. just a sec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Landed in 6e90fed |
Headers in nodejs can be arrays and current workaround for content-disposition header do not take this into account. This change fixes that and makes sure array values are handled properly. PR-URL: #50977 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Headers in nodejs can be arrays and current workaround for content-disposition header do not take this into account. This change fixes that and makes sure array values are handled properly. PR-URL: #50977 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
I'm not sure where/how is the correct way for me to raise this, but we need this fixed in the 18 LTS (It was introduced in 18.16.0 - #46528). |
@richardlau |
I've added the lts-watch-v18.x label to this which will put it on the list of things to consider for the next non-security Node.js 18.x release. |
Ok, thanks. We will have to ask people to downgrade to 18.5 in the meantime. |
Headers in nodejs can be arrays and current workaround for content-disposition header do not take this into account. This change fixes that and makes sure array values are handled properly. PR-URL: #50977 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This change fixes a bug introduced by an earlier PR #46528. Basically, in the earlier PR the code was written with the assumption that content-disposition header will always have a scalar string value but in fact Node.js allows users to provide an array as a value for a header.
If you provide an array value for content-disposition header, then the Buffer string would contain characters that are invalid for headers and that results in a
ERR_INVALID_HTTP_RESPONSE
error.