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

Multi-valued header is not consistently handled #2692

Closed
1 task done
todddialpad opened this issue Feb 22, 2023 · 3 comments
Closed
1 task done

Multi-valued header is not consistently handled #2692

todddialpad opened this issue Feb 22, 2023 · 3 comments

Comments

@todddialpad
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

See #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants