Multiple values for access-control-allow-origin

[oeway]

Checklist

• [ x] The bug is reproducible against the latest release and/or master.
• [ x] There are no similar issues or pull requests to fix it yet.

Describe the bug

When using starlette (through FastAPI) CORSMiddleware with python-socketio (another ASGI app mounted via app.mount()), I got multiple values for the access-control-allow-origin headers. This is because both python socketio and the CORSMiddleware will inject CORS headers in different cases. In the response I see two duplicated headers:

Access-Control-Allow-Origin: http://localhost:3000
access-control-allow-origin: http://localhost:3000

And the browser complain it as a CORS error.

To reproduce

from fastapi import FastAPI
import socketio

from fastapi.middleware.cors import CORSMiddleware
app = FastAPI()

sio = socketio.AsyncServer(async_mode="asgi", cors_allowed_origins="http://localhost:3000")

sio_app = socketio.ASGIApp(socketio_server=sio)

app.mount("/", sio_app)

app.add_middleware(
        CORSMiddleware,
        allow_origins= ["http://localhost:3000"],
        allow_credentials=False,
        allow_methods=["*"],
        allow_headers=["Content-Type", "Authorization"],
 )

Expected behavior

The CORSMiddleware should check existing headers in different cases and update them only if the headers in different cases not exists.

Actual behavior

The CORSMiddleware add the cors haders and result in multiple duplicated values.

Environment

• OS: MacOS
• Python version: 3.8.11
• Starlette version: 0.14.2

Additional context

[iamthen0ise]

Doesn't this contradict the very idea of the CORS? it seems to me that it would be better to validate the Origin header on the backend against the list of allowed sources

[oeway]

Doesn't this contradict the very idea of the CORS? it seems to me that it would be better to validate the Origin header on the backend against the list of allowed sources

Sorry that I don't get what you mean. Do you mean I should not use the cors middleware? In this case we only want the cors header to be sent correctly for all the resources. When I say differences cases, I mean the same origin header in upper and lower cases.

[oeway]

I did some more experiments and found out the issue is actually coming from calling "MutableHeaders.update()" used in the CORSMiddleware.

See the example below:

    @app.middleware("http")
    async def add_cors_header(request: Request, call_next):
        response = await call_next(request)
        print('==========before====>headers', response.headers)
        headers = {"Access-Control-Allow-Credentials": "true"}
        response.headers.update(headers)
        print('==========after====>headers', response.headers)
        return response

And this is what I see from the console:

==========before====>headers MutableHeaders({'Content-Type': 'text/plain; charset=UTF-8', 'Access-Control-Allow-Origin': 'http://localhost:3000', 'Access-Control-Allow-Credentials': 'true'})
==========after====>headers MutableHeaders({'Content-Type': 'text/plain; charset=UTF-8', 'Access-Control-Allow-Origin': 'http://localhost:3000', 'Access-Control-Allow-Credentials': 'true', 'access-control-allow-credentials': 'true'})

As you can see, for some reason, when we update the headers, it results in both versions: 'Access-Control-Allow-Credentials' and 'access-control-allow-credentials'.

[aminalaee]

Merged in #235. Can be closed.

[oeway]

Hi @aminalaee thanks for looking into it. However it seems #235 is about test client, I don't get why that can fix the issue mentioned here, could you please clarify?

[aminalaee]

@oeway Sorry, closed the wrong issue. I'll re-open this.

[declaresub]

I do not think that this is a bug in starlette, but a problem in engineio. It appears that engineio is adding headers without checking for duplicates.

In engineio/server.py, one finds the following at line 460:

    cors_headers = self._cors_headers(environ)
    start_response(r['status'], r['headers'] + cors_headers)

I will likely take this up with engineio, but the practical fix, at least for me, is to disable CORS handling in socketio by setting cors_allowed_origins=[] when creating the engineio server object. For fastapi-socketio:

socket_manager = SocketManager(app=app, cors_allowed_origins=[])

I should probably also complain to unicorn for sending multiple response headers with the same name; servers aren't supposed to do this except for Set-Cookie and certain other headers like Vary.

[Kludex]

I should probably also complain to unicorn for sending multiple response headers with the same name; servers aren't supposed to do this except for Set-Cookie and certain other headers like Vary.

https://asgi.readthedocs.io/en/latest/specs/www.html#http

The ASGI design decision is to transport both request and response headers as lists of 2-element [name, value] lists and preserve headers exactly as they were provided.

[declaresub]

This is an unfortunate design decision that appears to be based on a misreading of RFC7230. ASGI spec:

"Multiple header fields with the same name are complex in HTTP. RFC 7230 states that for any header field that can appear multiple times, it is exactly equivalent to sending that header field only once with all the values joined by commas.

However, RFC 7230 and RFC 6265 make it clear that this rule does not apply to the various headers used by HTTP cookies (Cookie and Set-Cookie). The Cookie header must only be sent once by a user-agent, but the Set-Cookie header may appear repeatedly and cannot be joined by commas. The ASGI design decision is to transport both request and response headers as lists of 2-element [name, value] lists and preserve headers exactly as they were provided."

RFC 7230:

" 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)."

Not all header fields are defined as comma-separated lists.

[Kludex]

I meant that uvicorn follows ASGI specs, so you should probably "complain" on asgiref.

[declaresub]

Ah. Thanks for the link.

[oeway]

@declaresub I understand that it's more relevant to fix from the spec and starlette implemented it correctly. However, for practical reasons, do you think we can have a fix in the CORSMiddleware? Instead of blindly add the CORS headers, we should check whether the same header (in different capitalization) exists already. What do you think?

[declaresub]

For this particular issue, the fix needed is to use exactly one of starlette or engine.io to handle CORS, as noted earlier. I haven't gone through cors.py as carefully as I could, but I didn't see any place where headers were being appended. Perhaps it would be useful to make sure that it overwrites any existing headers that it sets.

The real solution to sloppy middleware is, of course, more middleware. I have it in mind to write some header middleware that checks for multiple headers with the same name, and does something when such are found. I don't believe this can be done reliably in starlette, because middleware can always be added outside the control of starlette.

[Kludex]

For this particular issue, the fix needed is to use exactly one of starlette or engine.io to handle CORS, as noted earlier.

If that's the case, I don't think we have an issue on Starlette.

[Kludex]

I don't think it makes sense for the CORSMiddleware to verify if the headers Access-Control-Allow-Origin are present, as the middleware itself should be in charge of adding them.

Given the statement above:

For this particular issue, the fix needed is to use exactly one of starlette or engine.io to handle CORS, as noted earlier.

I'm closing the issue. If you think this is not solved, or there's a different point of view that was not presented, feel free to express yourself here, in a new issue, or reach out to us on our Gitter channel.

[zdanl]

I mailed you, so that i don't have to reopen this, but I am having a weird behavior here,

origins = [
 "*"
]

# Temporary fix for CORS issue
app.add_middleware(
    CORSMiddleware,
    allow_origins=origins,
    allow_credentials=True,
    allow_methods=["*"],
    allow_headers=["*"],
)

everytime i add a single CORS middleware origin, I grepped, its the only call, it adds two and yields the known bug.

Any suggestion?

[Kludex]

This should do: https://github.com/encode/starlette/issues/1309#issuecomment-1019192997

[zdanl]

Please explain as if I was retarded. (Sorry, spent a night.) =)

[Kludex]

I mean, you are using python-socketio, right?

[zdanl]

I mean, you are using python-socketio, right?

FastAPI with uvicorn

[Kludex]

Can you share a minimal reproducible example?

[zdanl]

https://github.com/lyrasadie/lyra-api/blob/main/api/__init__.py

When not commented out, 2 CORS headers. When, none.

[zdanl]

Can you share a minimal reproducible example?

https://github.com/lyrasadie/lyra-api/blob/main/api/__init__.py

When not commented out, 2 CORS headers. When, none. Aware of the missing [ ] in comment

[ADyerVEP]

@zdanl did you ever figure this out?

[choigawoon]

i stuck into the same problem. my app uses both FastAPI and python-socketio.

origins = [

"http://mydomain:9999",

# "http://mydomain:5173",

]

is there any workaround for this?