[v0.11.0] `Access-Control-Request-Headers` is matched as-is, without normalization.
stonedu1011 opened this issue · 15 comments
Test to reproduce the issue:
func TestCORS(t *testing.T) {
handler := cors.New(cors.Options{
AllowedHeaders: []string{"X-Test-Header"},
ExposedHeaders: []string{"X-Test-Header"},
OptionsPassthrough: false,
})
rw := httptest.NewRecorder()
req := httptest.NewRequest(http.MethodOptions, "http://localhost/cors/test", nil)
req.Header.Set("Origin", "localhost")
req.Header.Set("Access-Control-Request-Method", http.MethodGet)
req.Header.Set("Access-Control-Request-Headers", "X-Test-Header")
handler.HandlerFunc(rw, req)
resp := rw.Result()
g := gomega.NewWithT(t)
t.Logf("Headers: %v\n", resp.Header)
g.Expect(resp.Header).To(gomega.HaveKey("Access-Control-Allow-Origin"))
g.Expect(resp.Header).To(gomega.HaveKey("Access-Control-Allow-Methods"))
g.Expect(resp.Header).To(gomega.HaveKey("Access-Control-Allow-Headers"))
}
Result: The test succeeded in v0.10.1 but failed in v0.11.0:
=== RUN TestCORS
cors_test.go:27: Headers: map[Vary:[Origin, Access-Control-Request-Method, Access-Control-Request-Headers]]
cors_test.go:28:
Expected
<http.Header | len:1>: {
"Vary": [
"Origin, Access-Control-Request-Method, Access-Control-Request-Headers",
],
}
to have key
<string>: Access-Control-Allow-Origin
--- FAIL: TestCORS (0.00s)
Cause:
In v0.10.1 and previous versions, both AllowedHeaders and actual header values were normalized with http.CanonicalHeaderKey.
In v0.11.1, AllowedHeaders is normalized using strings.ToLower (cors.go Ln 199). However, the actual header values are not normalized ( internal/sortedset.go Ln 69-79)
@stonedu1011 This is working as expected. In addition to #176, see #180 and #181.
TL;DR: According to the Fetch standard, the Access-Control-Request-Headers header only ever contains lowercase values; this library takes advantage of that guarantee.
@stonedu1011 Can we close this issue?
I was also hit by this and had to spend an hour now debugging why upgrading this library is breaking tests. I am not sure if it is really so costly to convert all incoming headers to lowercase before proceeding? I mean, in general in Internet protocols you should be standard-strict in your outputs and lenient in your inputs to maximize interoperability.
This has now been reported few times, here, #174, #176, #181.
Also, I do not find it nice that headers in Access-Control-Allow-Headers are now all lowercase as well, it looks ugly. :-) They are not in examples here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers
I was also hit by this and had to spend an hour now debugging why upgrading this library is breaking tests. [...] This has now been reported few times
Sorry about the breakage, which indeed seems to have affected several people. Perhaps one mistake was not to highlight, in the library's changelog, the change in behaviour. Note that all those issues have now been closed, though, and people seem far from outraged. As explained in those other issues, if you must test your CORS-aware server, make sure to respect the format for the ACRH header: comma-separated, lowercase, unique, lexicographically ordered values.
[...] in general in Internet protocols you should be standard-strict in your outputs and lenient in your inputs to maximize interoperability.
The Fetch standard does specify that the ACRH header contains lowercase elements and, although field names are case-insensitive, intermediates (proxies, gateways, middleware, etc.) must not change the case of field values; at least, that's my understanding of HTTP standards. Therefore, CORS middleware should reasonably expect elements of ACRH to be lowercase.
As for Postel's law, I believe that @rs agrees with me about not being too liberal, in order to guarantee better performance. But if you can implement the behaviour you desire without affecting performance too detrimentally, be my guest and submit a PR; @rs might be willing to accept it.
Also, I do not find it nice that headers in Access-Control-Allow-Headers are now all lowercase as well, it looks ugly.
I'm puzzled: why would you care? The CORS protocol is not for human consumption... You configure your CORS middleware, apply it where it counts, and you should be able to forget about it.
They are not in examples here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers
Elements in the value of the Access-Control-Allow-Headers header can indeed be in any case; the Fetch standard makes provisions for this. Incidentally, note that I rectified incorrect cases in the examples for Access-Control-Request-Headers a few months ago, following my contribution to rs/cors: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Request-Headers
Sorry about the breakage, which indeed seems to have affected several people
It is OK. Thanks for all the work and attention to details here. It is just frustrating that this might be much easier with some changelog entry or README entry or something, explaining that headers have to be now lower case and sorted Access-Control-Allow-Headers, especially in one's tests. Going through PRs and issues to figure out what has happening after a simple version bump takes time.
But if you can implement the behaviour you desire without affecting performance too detrimentally
I am not familiar enough with implementation here and no time to really go into it, but doing lowercase on a string should be O(n) on the string and that should not be too expensive? But I am not sure what margin of performance people are going for here. Sorting can be probably slow, true.
In any case, I think it is fine how it is, we should optimize for the main use case, not for testing. But README entry might be enough here.
Incidentally, note that I rectified incorrect cases in the examples for Access-Control-Request-Headers a few months ago
You might have to go around the web fixing more pages:
this might be much easier with some changelog entry or README entry or something
I strongly agree: rs/cors does need a changelog; feel free (if time allows) to open an issue about that. FWIW, jub0bs/cors has one, in which I endeavour to highlight all important changes.
especially in one's tests
I'm curious about this. What are those tests meant to check? That the http.Handler under test is configured for CORS? If so, would this concern not better be addressed by some integration or e2e test (in which the client would play a role)?
doing lowercase on a string should be O(n) on the string and that should not be too expensive
Lowercasing a string indeed takes O(n) time (where n is the string's length), but may require an allocation, and one of my goals is to minimise needless allocations. I don't know of any compiler optimisation that eliminates this allocation; see https://go.dev/play/p/4EiSWuHvtT1
I think it is fine how it is, we should optimize for the main use case, not for testing.
That's the thing: it's not just about making the library easy for normal people to use, but also to make its middleware hard for adversaries to abuse. In this connection, see #170 (if you haven't already).
You might have to go around the web fixing more pages.
Good catch. I wasn't familiar with http.dev. I'll see if I can fix that page, somehow.
What are those tests meant to check?
In my case I use many library to build my HTTP stack for apps and I have tests which check that given a particular HTTP request I get expected HTTP response. It is useful to check that behavior is consistent as I upgrade libraries and dependencies. Or at least that I am aware of a change. But yes, the client is simulated which was a problem in this case. :-)
Lowercasing a string indeed takes O(n) time (where n is the string's length), but may require an allocation
I think using the following to convert string into []byte in-place:
func String2ByteSlice(str string) []byte {
return unsafe.Slice(unsafe.StringData(str), len(str))
}
func ByteSlice2String(bs []byte) string {
if len(bs) == 0 {
return ""
}
return unsafe.String(unsafe.SliceData(bs), len(bs))
}
func lower(b byte) byte {
if 'A' <= b && b <= 'Z' {
return b + ('a' - 'A')
}
return b
}
And then going into a loop over []byte slice converting to ASCII lower case each character should be zero allocations? And then back to string conversion. I think strings.ToLower does unicode-aware conversion which can change the length of the string.
Thanks for the additional context.
I think using the following to convert string into []byte in-place
That requires the unsafe package, though. I don't know about @rs, but I am reluctant to import unsafe.
It would be dangerous to use unsafe on strings we don't own.
An alternative way may be to have both lowercase and mime-case versions of allowed headers in the allow set with equal position value?
An alternative way may be to have both lowercase and mime-case versions of allowed headers in the allow set with equal position value?
If case insensitivity is the goal, the library should cater for all case variations, e.g. not just x-api-key and X-Api-Key but also X-API-KEY, X-aPi-kEY, etc. Not sure how viable that would be without performing case normalisation.
I think the library's goal is easy of used combined with performance and safety.
To me only supporting the two most common cases and leaving the unusual ones for performance reasons feels reasonable. What do you think?
I think it is fine as it is, just document it somewhere. :-) Something like:
The library strictly conforms to the fetch spec, so if you are writing tests against the library, make sure your
Access-Control-Allow-Headersheader has sorted and lowercase list of headers (all standard compliant browsers do that as well).
What do you think?
If you're asking me, I think case sensitivity in ACRH should be maintained, for reasons explained in an earlier comment. As far as jub0bs/cors is concerned, unless someone can offer convincing counterarguments, I'm unwilling to compromise on case sensitivity. As for rs/cors, it's out of my hands, but implementing what you suggested seems straightforward.