-
Notifications
You must be signed in to change notification settings - Fork 240
fix: regression in header case-insensitivity #188
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: regression in header case-insensitivity #188
Conversation
DMarby
left a comment
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.
Looks great, thank you for finding this regression and fixing it!
jgiovaresco
left a comment
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.
Nice catch 👏
I'm wondering if we should also use the helper for
httpsnippet/src/targets/c/libcurl.js
Line 31 in 675df1a
.push('curl_easy_setopt(hnd, CURLOPT_COOKIE, "%s");', source.allHeaders.cookie) httpsnippet/src/targets/node/fetch.js
Lines 79 to 82 in 675df1a
reqOpts.headers.cookie = cookies } else { reqOpts.headers = {} reqOpts.headers.cookie = cookies
src/index.js
Outdated
| Object.keys(request.headersObj).forEach(header => { | ||
| if (header.toLowerCase() === 'content-type') { | ||
| foundContentType = true | ||
| request.headersObj[header] = 'multipart/form-data; boundary=' + boundary |
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.
I think I miss something I don't understand why we would not use the new helper functions you have added?
we would have something like
const contentTypeHeader = helpers.hasHeader('content-type') ? helpers.getHeaderName('content-type'): 'content-type';
request.headersObj[contentTypeHeader] = 'multipart/form-data; boundary=' + boundary
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.
yeah this was an oversight on my part. will update!
|
@jgiovaresco Updated with your feedback.
I'd love to swap the |
develohpanda
left a comment
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.
caseless would be cool, but I agree out of scope for this regression.
Thank you for taking the time to investigate and fix this! 🎉 Sorry about the delay, I've been quite bogged down for a while!
| @@ -0,0 +1,43 @@ | |||
| /* global describe, it */ | |||
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.
Nice tests!
This fixes a regression that was introduced in the recent header case-insensitivity work (#178) that is causing edge cases where, on certain targets, it's possible for a header to be:
multipart/form-datarequests where one header would just bemultipart/form-datawhile the other would be that plus theboundary.undefinedheader being set, being set in ways that it shouldn't be, or the target completely failing to function because a header wasn't being removed.To resolve this I've:
headershelper with a couple methods to traverse the now case-insensitive object of headers so that these targets that need special case handling for something likecontent-typecan safely search for that in the available headers.For testing all of this, I've updated the
multipart/form-datatest fixture to have itscontent-typeheader asContent-Typeto force this case-insensitive lookup for targets that need it.😰