httpcore: headers with comma-separated list values handled as single strings

[DestyNova]

Nim Version

2.2.2

Description

Some HTTP headers can have multiple values, e.g.:

Connection: keep-alive
Connection: Upgrade

httpcore seems to correctly combine subsequent header values into one seq[string], so when contains(values, "Upgrade") is called, it will return true in this case.

However, it's also legal for those values to be combined into a comma-separated string:

Connection: keep-alive, Upgrade

Firefox does this when opening a websocket, and in this case we get a HttpRequestHeader which is a single element sequence @["keep-alive, Upgrade"], and contains returns false, so the websocket handshake stops there.

Current Output

Expected Output

Known Workarounds

Mirco Ackermann discovered the problem in this happyx issue, where websockets weren't working at all, because Firefox sends the header Connection: keep-alive, Upgrade instead of two separate headers (actually Chrome just sends one: Connection: Upgrade.

So one workaround for happyx was to replace the call to httpcore's contains function with a new function that splits the header value by comma. But this should only be done for fields where it's legal for the header value to be a list or set. I'm not sure what the best solution is.

Additional Information

No response

[metagn]

import httpcore

let header = parseHeader("Connection: keep-alive, Upgrade")
echo header

echo contains(header.value, "Upgrade")
echo contains(header.value, "upgrade")

echo contains(HttpHeaderValues(header.value), "Upgrade")
echo contains(HttpHeaderValues(header.value), "upgrade")

(key: "Connection", value: @["keep-alive", "Upgrade"])
true
false
true
true

Does this work as expected?

[DestyNova]

Does this work as expected?

I'm not really sure when it should be case-sensitive or not, but that's pretty neat and looks like a much nicer workaround!

I also tried using newHttpHeaders but it did something different:

import httpcore

let headers = newHttpHeaders({ "Content-Type": "application/json", "Contains": "Upgrade", "Contains": "keep-alive" })
assert contains(headers["Contains"], "keep-alive")
assert contains(headers["Contains"], "Upgrade")
assert contains(headers["Contains"], "upgrade")

let headers2 = newHttpheaders({ "Content-Type": "application/json", "Contains": "keep-alive, Upgrade" })
assert contains(headers2["Contains"], "keep-alive")     # <--- fails
assert contains(headers2["Contains"], "Upgrade")
assert contains(headers2["Contains"], "upgrade")

[mirco-ackermann]

Does this work as expected?

This works as expected. The bug we experienced was caused by happyx using its own header parsing that does not use parseList. Happyx populates datastructures imported from httpcore which misled us to look for the error in httpcore.

But httpcore seems to be a bit to eager to parseList the payload. For example,

import httpcore

let header = parseHeader("Last-Modified: Wed, 21 Oct 2015 07:28:00 GMT")
echo header

gives (key: "Last-Modified", value: @["Wed", "21 Oct 2015 07:28:00 GMT"]) where I'd expect the whole date to be represented as a single string. There are other headers like Retry-After that also encode date and time including a comma.