sanic-org / sanic

Accelerate your web app development | Build fast. Run fast.
https://sanic.dev
MIT License
18k stars 1.54k forks source link

Multi-valued header is not consistently handled #2692

Closed todddialpad closed 1 year ago

todddialpad commented 1 year ago

Is there an existing issue for this?

Describe the bug

Using the following test app:

from sanic import Sanic
from sanic.response import text

app = Sanic("MyHelloWorldApp")

@app.get("/")
async def hello_world(request):
  return text('\n'.join(f'{k}: {request.headers.getall(k)}' for k in set(request.headers)))

Looking at the HTTP standard here: https://www.rfc-editor.org/rfc/rfc9110.html#fields

I think the following should return the same result when sent to the test app: curl -H "MYHEADER: a" -H "MYHEADER: b" http://127.0.0.1:8000

user-agent: ['curl/7.79.1']
host: ['127.0.0.1:8000']
accept: ['*/*']
myheader: ['a', 'b']

curl -H "MYHEADER: a,b" http://127.0.0.1:8000

user-agent: ['curl/7.79.1']
host: ['127.0.0.1:8000']
accept: ['*/*']
myheader: ['a,b']

If I am reading the standard correctly, the value of a header should be parsed as a comma-separated, value list.

It doesn't seem to be. I think the second example should return the same result as the first one.

Code snippet

see above

Expected Behavior

The following two headers should both result in a multi-valued key in request.headers

MYHEADER: a
MYHEADER: b

and MYHEADER: a,b

How do you run Sanic?

Sanic CLI

Operating System

Linux

Sanic Version

22.9.0

Additional context

No response

ahopkins commented 1 year ago

I am not sure I agree this is a bug, and is a part of a very early design decision (cannot find the source right now). Basically the point being that additional parsing of comma-delimited fields is generally a case-by-case item. We do it for some headers on accessor to not incur the performance penalty, which is the main concern.

BUT.... we may be able to do it on access here too.

ahopkins commented 1 year ago

After some more consideration on this, parsing the following as identical does not make sense for Sanic.

Foo: one
Foo: two
Foo: one,two

The getone and getall methods are meant to be used to retrieve raw header values. Any parsing of headers that we do ends up directly on the Request object. To achieve the result you are looking for (parsed values of headers returned by getall) is a departure from the intended implementation.

That is meant to retrieve all of the headers that were passed, without doing any parsing. Any further parsing is probably an app-side implementation. Or it is something done on the Request object (like request.accept, request.forwarded, or request.token).

ahopkins commented 1 year ago

See https://github.com/sanic-org/sanic/pull/2696

While not directly addressing your point, it would work to get all of the values in a single field. You would need to parse yourself.