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

Case-sensitive header APIs are not usable #1246

Open
meiamsome opened this issue Nov 14, 2019 · 12 comments
Open

Case-sensitive header APIs are not usable #1246

meiamsome opened this issue Nov 14, 2019 · 12 comments
Labels
bug evaluation needed proposal needs to be validated or tested before fully implementing it in k6 new-http issues that would require (or benefit from) a new HTTP API

Comments

@meiamsome
Copy link

It is not possible to test a non-conformant HTTP server that is handling headers in a case-sensitive manner. The following example script:

import http from 'k6/http'

export default () => {
  let res = http.get(
    'https://httpbin.org/headers',
    {
      headers: {
        'lower-case-header': 'value'
      }
    }
  )
  console.log(res.body)
}

Results in the header being sent as Lower-Case-Header, meaning that if the server checks the headers in a case-sensitive manner then the header becomes impossible to set in k6.

@na--
Copy link
Member

na-- commented Nov 14, 2019

Ah... I think this is something that the Go net/http stdlib helpfully "fixes" for us 🤦‍♂️ The good news is that it seems circumvent-able: https://stackoverflow.com/questions/24945806/add-unaltered-lowercase-headers-to-golang-http-request

Thanks for reporting this issue!

@na-- na-- added bug evaluation needed proposal needs to be validated or tested before fully implementing it in k6 labels Nov 14, 2019
@na-- na-- added this to the v0.27.0 milestone Nov 14, 2019
@cuonglm
Copy link
Contributor

cuonglm commented Nov 15, 2019

Ah... I think this is something that the Go net/http stdlib helpfully "fixes" for us 🤦‍♂ The good news is that it seems circumvent-able: https://stackoverflow.com/questions/24945806/add-unaltered-lowercase-headers-to-golang-http-request

Thanks for reporting this issue!

IIRC, HTTP headers are case-insensitive, so if the server strictly checks for lower case only, it's non-compliant.

@na--
Copy link
Member

na-- commented Nov 15, 2019

Yeah, that's why I put the evaluation needed label. After all, if we have to make a lot of changes to k6 in order to support non-compliant HTTP servers, it's probably not a good idea to do so... But if we can make a few tweaks so that we don't automatically transform user-supplied custom request header, it's probably worth doing.

@meiamsome, can you give us some details about your use case?

@imiric
Copy link
Contributor

imiric commented May 20, 2020

I took a brief look at this, and while the fix itself is straightforward, adding tests for it seems impossible. See 3e3c347.

The headers in the request passed to the handler are already transformed to the canonical case used in Go, so we can't write them out in their original case in the response to assert on it. Same goes for the internal httplib endpoints, of course.

The fix works though (confirmed with --http-debug), but not sure if we should keep it if we can't ensure it will continue to work.

@na-- na-- modified the milestones: v0.27.0, v0.28.0 May 21, 2020
@na-- na-- removed this from the v0.28.0 milestone Sep 9, 2020
@na--
Copy link
Member

na-- commented Jan 18, 2022

Another instance of this problem: https://community.k6.io/t/case-sensitive-headers/2745

The "fix" seems simple, so maybe we should implement it, even if we can't test it? 🤷‍♂️ And, if we really want to write a test, we can spin up a raw TCP server and parse the raw HTTP request without net/http? That should be simple enough to implement?

@na-- na-- added this to the v0.37.0 milestone Jan 18, 2022
@codebien codebien added the new-http issues that would require (or benefit from) a new HTTP API label Jan 18, 2022
@na-- na-- removed this from the v0.37.0 milestone Jan 18, 2022
@na--
Copy link
Member

na-- commented Jan 18, 2022

This is not as straightforward as it seems on the surface, so I removed the milestone again... For context, golang/go#5022 (comment) is the very good rationale for why Go doesn't have case-sensitive headers. And that makes complete sense, especially for HTTP servers. However, k6 is a testing tool and there's an argument to be made that artificial restrictions have no place in it. Sending arbitrary case in the HTTP header names is not against the RFC, the RFC just means that servers shouldn't care what the case of incoming header names is... And I'd argue that, even if it was against the RFC, it should be reasonably possible for k6 to even send somewhat malformed HTTP requests. Things like that are a normal part of QA and testing and a testing tool should reasonably accommodate them... 🤷‍♂️

However, while it's seems like the "fix" on our side is simple, just replace result.Req.Header.Set(key, str) with result.Req.Header[key] = []string{str}, that would have implications we might not realize. For example, this will now produce a different result:

http.get(whatever, {headers: {
  'foo': 'bar',
  'Foo': 'baz',
}});

It's not clear that it will actually be a bad result, per se, since it will actually allow k6 to send multiple headers with the same header name. We currently can't do that (because we use a JS {key: value} object and pass keys through CanonicalMIMEHeaderKey), but the HTTP RFC actually allows it in some cases, see this from https://www.rfc-editor.org/rfc/rfc7230#section-3.2.2:

A sender MUST NOT generate multiple header fields with the same field name in a message unless either the entire field value for that header field is defined as a comma-separated list [i.e., #(values)] or the header field is a well-known exception (as noted below).

So, yeah, I removed the milestone, but I'm not closing the issue either. We're just not rushing to "fix" it before we know the implications. For now, given that there aren't any workarounds to the issues described here, and the fact that so few people have hit them, I'd advise any affected users to comment with their use cases here and 👍 the issue, so we can have more details about the scope and impact of this. In the meantime, you can use xk6 extensions to solve these types of issues for yourself:

@mstoykov
Copy link
Collaborator

As an additional note the HTTP2 RFC specifies that all header field names MUST be lower case. Which IMO makes fixing this even less relevant as the world moves more and more towards http2.

p.s. same is true for HTTP3 at least in the latest draft I could find.

@xendren
Copy link

xendren commented Aug 1, 2022

As an additional note the HTTP2 RFC specifies that all header field names MUST be lower case. Which IMO makes fixing this even less relevant as the world moves more and more towards http2.

p.s. same is true for HTTP3 at least in the latest draft I could find.

Since the original issue is that k6 is internally changing the header to some camel-case format, won't you have to fix this to support HTTP2 since it "MUST be lower case"? It seems the better solution would be to leave the headers as is in the script. If an error occurs, the test developers can change the headers to match the requirements for the protocol. As it is now, there is no workaround because k6 is internally changing the headers. Without a workaround, this may eliminate k6 as a viable load testing solution.

@mstoykov
Copy link
Collaborator

mstoykov commented Aug 1, 2022

Since the original issue is that k6 is internally changing the header to some camel-case format, won't you have to fix this to support HTTP2 since it "MUST be lower case"?

golang's stdlib does this automatically. Otherwise it wouldn't be passing any tests and most servers will have problems with it.

Without a workaround, this may eliminate k6 as a viable load testing solution.

We are aware of this, but the same is true for a lot of the other issues as well. At the moment this seems like a problem for a small percentage of the users, and also a problem that literally will solve itself ... as http1.x becomes less and less common.

I am kind of surpised people still have problems with this ... then again we support ntlm, so I guess I shouldn't be.

And again - if there was some easy fix with no drawbacks - we would likely have implemented it. Unfortunately it seems like it will be quite involved and likely add more problems than not.

@oleiade
Copy link
Member

oleiade commented Jan 23, 2023

As requested by @na-- I come in to report an example of a situation in which I ran into the issue:

  • During the implementation of the experimental tracing module, we found out that the B3 tracing context specification specifies the b3 header must be lowercase.
  • A user reported their tracing backend did not process Capitalized headers (B3 as we send them)
  • After some inspection, I found that although the tracing client was setting headers formatted as lowercased because we rely on k6/http.request, the headers would eventually be set using the Go stdlib's http.Header.Set method, which capitalizes them.

Further, I also discovered that as soon as one uses http.Header.Set or http.Header.Add methods, Go uses a pretty intrusive normalization strategy. For instance, http.Header.Set("X-B3-ParentSpanId", "foo") turns the header field name into X-B3-Parentspanid.

Outcome: I don't necessarily expect the k6/http to change its behavior for the above reasons. As a user with knowledge of k6's internal, I expect this to become fixed organically if Go ends up aligning the behavior of its HTTP implementation on the HTTP2 specification.

This is the first time in ten years of using the language that I have this "nonsense" feeling about a Go standard API. As a user and a maintainer, I prefer for k6's http.request function to use direct assignment headers[key] = value over the Set and Add methods.

@oleiade
Copy link
Member

oleiade commented Jan 23, 2023

edited☝🏻to reflect my change of opinion regarding the matter.

@oleiade
Copy link
Member

oleiade commented Feb 10, 2023

We have recently met with the k6 maintainers team to discuss some potential solutions for this.

no decisions have been made, and the following is there as information to feed further research and investigation.

parseRequest

One way to address the issue that's been discussed was to make the http module's currently private parseRequest method public, to offer the ability to modules such as k6/experimental/tracing to produce their request method with a specific behavior: such as one treating headers in a case-insensitive manner.

rawHeaders

Another way we discussed was to add a rawHeaders attribute to the params of the http.request function. This attribute would consist of an Object (or Map) that users would be expected to fill with the headers they wish to attach to requests. Those rawHeaders would be sent as-is, respecting the casing provided by the user. Those rawHeaders would supersede the headers field. Passing both attributes would lead to an error.

different types for different behaviors

Another idea we discussed was to establish a distinction between headers passed as an Object, which would keep behaving the way they do now, and passing it as a Map, in which case it would be treated as raw headers, similar to what was discussed .

conclusion

One conclusion of the discussion was that ideally, we would favor a solution that involves no, or very limited, changes to the current UX.

An important bit of information inherited from that discussion was that Goja Object protect the ordering of the keys, as they were inserted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug evaluation needed proposal needs to be validated or tested before fully implementing it in k6 new-http issues that would require (or benefit from) a new HTTP API
Projects
None yet
Development

No branches or pull requests

8 participants