HTTP Header with case-insensitive header names

[alexanderbartels]

Hi, i've discovered another minor issue.

Describe the bug define content-type: text/html as response header results in duplicate content type header:

To Reproduce create a route that returns a html response like

  return ({
      headers: {
          ...filterProxyRequestHeaders(response.headers),
          "content-type": "text/html"
      },
      body: bodyStream
  });

Expected behavior response contains only "content-type": "text/html" as content type header.

Desktop (please complete the following information):

• OS => MacOS BigSur
• Package + Version

  "@marblejs/core": "^3.4.8",
  "@marblejs/middleware-body": "^3.4.8",
  "@marblejs/middleware-logger": "^3.4.8",

• Node version => v14.12.0

Additional context i found this one as a short explainer that the headers in http protocol are case-insensitive: https://stackoverflow.com/a/5259004

My use case for a bit more context: I use undici a nodejs http client: https://github.com/nodejs/undici All response headers are in lower case letters and my plan is to forward them, as my marble server acts like a proxy in this case.

Workaround For now i can map the content-type header to the upper case one. But this can be tedious process if more headers are affected.

Let me know if i can help with that if you want to improve this.

[JozefFlakus]

Thanks for reporting this issue.

I'm thinking about a potential solution to this problem but I'm not sure about consequences with the introduction of this fix.

I see two options:

1. Transform all lower case headers to upper case format -> all outgoing headers will be in upper case format
2. Don't transform lower case headers but detect duplicates (eg. content-type and Content-Type and always take the last applied header) -> outgoing headers won't be normalized (mix of upper and lower cased headers)

Both options can introduce a potential hidden breaking change to already implemented solutions, but still... is it really a breaking change or rather a bug fix? 🤔 💭

[alexanderbartels]

maybe get some inspiration from other software products:

• NGINX: looks like they use a lower-cased hash of the header name to prevent duplicates for incoming headers (like your solution no. 2) But their http proxy module only accept lower case htt header names and otherwise it crashes (like your solution no. 1 but with lower cased letters). Source: https://www.nginx.com/resources/wiki/start/topics/examples/headers_management/
• undici: the http client i'm using also uses the always lower case http header approach. Described in the readme: https://github.com/nodejs/undici
• fastify: nodejs webserver also uses always lower case http headers: https://github.com/fastify/help/issues/71
• envoy: Envoy one of the popular reverse proxies also uses always lower case headers by default (but have a custom formatting option). Source: https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/header_casing

What do you think about going all lower case as it looks like the default approch for a lot of software products handling http headers. I think this approach has the advantage that it is

• "easy" to implement,
• easy to document ("all http headers are lower cased") and
• already known by developers that have worked with some of the software products mentioned above.

Both options can introduce a potential hidden breaking change to already implemented solutions, but still... is it really a breaking change or rather a bug fix?

Based on the HTTP Spec it is a non breaking change. As the headers are case insensitive. But in reality most Applications working with headers might do simple string matching like headers["X-Some-header"] === "Some Value" Application consumed by Browsers should not break. But if a microservice is implemented and consumed by another service this might be a breaking change for them. And some test cases might break as well. As i can imagine that developers don't care about the spec if they test if a header is sent. If i set a header like X-Some-Header i would write my test to expect exactly this header.

[JozefFlakus]

But in reality most Applications working with headers might do simple string matching like headers["X-Some-header"] === "Some Value"

[...]

As i can imagine that developers don't care about the spec if they test if a header is sent.

That's the thing that I'm worrying the most.

I would propose to fix it with a minor version release, just to add some extra safety space for developers.

What do you think about going all lower case as it looks like the default approch for a lot of software products handling http headers.

True that. Definitely more performant than the opposite solution.

You can find all relevant server response processing (together with headers management). https://github.com/marblejs/marble/tree/master/packages/core/src/http/response I think this part has to be refactored/rewritten - I'm not satisfied with how it looks and works like now. Basically the form and logic that you see in these files (skipping some minor improvements and fixes) was not touch since the first initial release.

[JozefFlakus]

@alexanderbartels Safety check - do you wan't to propose a PR? If not I would like to start the work on this issue.

[DawidYerginyan]

Even tho It is technically a bugfix and a non breaking change according to the HTTP Spec I'd opt to introduce it gradually for the ☝🏻 mentioned reasons with service2service communication and test cases. I propose to patch the behavior change under a feature-toggle disabled by default with a sufficient warning linking to the issue, stating that It is going to be the default behavior in the X upcoming version and introduce it as a breaking change in the next minor/major.

One option would be to control it with an env variable, something like

MARBLE_HTTP_CASE_INSENSITIVE_HEADER_NAMES=true

Considering that the change affects only the HTTP server, It could also be a temporary optional server factory parameter

import { createServer } from '@marblejs/core';

const httpServer = createServer({
  port,
  listener,
  dependencies,
  options: {
    httpsOptions,
    caseInsensitiveHeaderNames: true,
  },
});

[alexanderbartels]

sorry for the late response - its a bit crazy at work currently. @JozefFlakus if you haven't started yet, i can have a look at this in the upcoming weekend.

I like the idea from @DawidYerginyan to use a feature flag. I think an env variable would be a proper choice. With this solution the code must not be touched twice.

[JozefFlakus]

Closing because the improvement is available in v4.0 release candidate.