-
Notifications
You must be signed in to change notification settings - Fork 230
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
fix: regression in header case-insensitivity #188
Conversation
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!
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 |
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-data
requests where one header would just bemultipart/form-data
while the other would be that plus theboundary
.undefined
header 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:
headers
helper 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-type
can safely search for that in the available headers.For testing all of this, I've updated the
multipart/form-data
test fixture to have itscontent-type
header asContent-Type
to force this case-insensitive lookup for targets that need it.😰