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

fix: regression in header case-insensitivity #188

Merged
merged 2 commits into from
Nov 27, 2020

Conversation

erunion
Copy link
Contributor

@erunion erunion commented Oct 16, 2020

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:

  • Duplicated and both instances having differing data. This was mostly apparent on multipart/form-data requests where one header would just be multipart/form-data while the other would be that plus the boundary.
  • Some targets would completely fail to interpret the header, resulting in either an 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:

  • Added a new 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 like content-type can safely search for that in the available headers.
  • Updated every target that was looking for case-sensitive headers to now use this new helper to do the same work it needs to do.

For testing all of this, I've updated the multipart/form-data test fixture to have its content-type header as Content-Type to force this case-insensitive lookup for targets that need it.

😰

Copy link

@DMarby DMarby left a 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!

Copy link
Contributor

@jgiovaresco jgiovaresco left a 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

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
Copy link
Contributor

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

Copy link
Contributor Author

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!

@erunion
Copy link
Contributor Author

erunion commented Oct 31, 2020

@jgiovaresco Updated with your feedback.

I'm wondering if we should also use the helper for

I'd love to swap the reqOpts.headers object with an object that has some functionality like caseless so we could eliminate the need for allHeaders and this new helper utility added, but that's a larger refactor that I think is out of scope for resolving this regression.

Copy link
Contributor

@develohpanda develohpanda left a 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 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests!

@develohpanda develohpanda merged commit 86e7b0e into Kong:master Nov 27, 2020
@erunion erunion deleted the fix/header-insensitivity-issues branch November 27, 2020 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants