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

duplicate CORS headers leads to browser CORS errors #777

Closed
malud opened this issue Oct 12, 2023 · 6 comments · Fixed by #804
Closed

duplicate CORS headers leads to browser CORS errors #777

malud opened this issue Oct 12, 2023 · 6 comments · Fixed by #804
Labels
bug Something isn't working
Milestone

Comments

@malud
Copy link
Collaborator

malud commented Oct 12, 2023

Describe the bug
If you connect a backend api via proxy block and you have CORS options enabled and the api sends also such headers this leads to duplicate cors headers which the browser does not allow.

Current workaround is to add remove_response_headers = ["access-control-allow-origin", "access-control-allow-credentials"] to your backend definition.

@malud malud added the bug Something isn't working label Oct 12, 2023
@johakoch
Copy link
Collaborator

johakoch commented Feb 7, 2024

The problem may also occur with request:

    endpoint "/foo" {
      request "r" {
        url = "/bar"
        backend = "be"
      }
      response {
        headers = backend_responses.r.headers
      }
    }

@johakoch
Copy link
Collaborator

johakoch commented Feb 10, 2024

The CORS response headers must not be set/added before

	nextHandler.ServeHTTP(rw, req)

in CORS.ServeNextHTTP(). Instead we could register headers to remove/set/add with the writer.Response and execute the modifications after (? or before?) r.applyModifier().

@filex
Copy link
Contributor

filex commented Feb 10, 2024

The desired effect is that the a-c-a-* headers generated by Couper's cors block always win.(we do not want to be "smart" and try to merge the headers).

Ideally, we use the set modifier to make sure that our generated header override any corresponding header coming from the upstream response.

However, we might have the situation where the upstream sends more CORS headers than Couper would generate, e.g. max-age or allow-credentials. In this case we have to actively remove those headers to avoid mixed or unexpected CORS statements.

@filex
Copy link
Contributor

filex commented Feb 10, 2024

Does the problem only occur with payload requests? The OPTIONS PFR is handled by Couper without actually executing the endpoint, isn't it?

@johakoch
Copy link
Collaborator

Does the problem only occur with payload requests? The OPTIONS PFR is handled by Couper without actually executing the endpoint, isn't it?

Yes

@johakoch
Copy link
Collaborator

Ideally, we use the set modifier to make sure that our generated header override any corresponding header coming from the upstream response.

We already set them. The problem is not to set it (in contrast to add), it's the right point in the process: The response headers must not be set prior to the nextHandler's ServeHTTP() (as they are, currently), because some next handler will add/set the backend's response headers.

However, we might have the situation where the upstream sends more CORS headers than Couper would generate, e.g. max-age or allow-credentials. In this case we have to actively remove those headers to avoid mixed or unexpected CORS statements.

ACK

@johakoch johakoch linked a pull request Feb 16, 2024 that will close this issue
@johakoch johakoch added this to the 1.13 milestone Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants