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.

Sign up for GitHub

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

Jump to bottom

allow_cors may break CORS #93

Open
nezed opened this issue Jun 16, 2020 · 3 comments · May be fixed by #489
Open

allow_cors may break CORS #93

nezed opened this issue Jun 16, 2020 · 3 comments · May be fixed by #489
Labels
bug help wanted

Comments

@nezed
Copy link

nezed commented Jun 16, 2020

users[].allow_cors = true may lead to multiple Access-Control-Allow-Origin headers

Actual behaviour:

Responses with multiple headers is rejected by Chrome browser with following error:

The 'Access-Control-Allow-Origin' header contains multiple values 'http://ui.tabix.io, *', but only one is allowed.

Steps to reproduce:

  1. add_http_cors_header = 1 is default behaviour for tabix.io and grafana-clickhouse plugin, also it may be set as default for some users.
  2. In this case clickhouse-server responses includes Access-Control-Allow-Origin: * header.
  3. Setting chproxy option users[].allow_cors = true leads to injecting second Access-Control-Allow-Origin: origin header.
  4. Response that contains two CROS headers and is rejected by Chrome 
    Access-Control-Allow-Origin: *                     # by clickhouse-server
Access-Control-Allow-Origin: http://ui.tabix.io    # by chproxy

Expected behaviour

One of this

  1. don't add Allow-Origin header is response already has one
    • if users[].allow_cors = true then drop all Allow-Origin headers in source response if any and write single Allow-Origin header in same manner as chproxy does now
    • if users[].allow_cors = false then do nothing (user may expect that add_http_cors_header will take effect and experience troubles if chproxy changes this behaviour)

For me option 2 looks way better, at least because it respects real Origin header value from client
@hagen1778
Copy link
Contributor

Thanks for so detailed report @nezed!

@bobelev
Copy link

bobelev commented Nov 16, 2020

any updates on this?

@seifchen
Copy link

@hagen1778 any updates on this?

matthieugouel added a commit to matthieugouel/chproxy that referenced this issue Dec 1, 2024
@matthieugouel
Fix allow_cors: true returning two Access-Control-Allow-Origin he…
e2fc96d 
…aders

Fixes ContentSquare#93.
The `Access-Control-Allow-Origin` was set before on the response before the proxy call,
and ClickHouse was returning the response with its own `Access-Control-Allow-Origin` (in my case, "*").
So, having `allow_cors: true` was eventually returning two `Access-Control-Allow-Origin`, one from chproxy and one from ClickHouse.

This commit just move the `"Access-Control-Allow-Origin` after the proxy call, overriding the value returned by ClickHouse.
If `allow_cors: false`, chproxy does not change the value (so it can be, I believe, any value set by ClickHouse), else, with `allow_cors: true`,
it will override to either the value of `Origin` request if any or else `*`.
@matthieugouel matthieugouel linked a pull request Dec 1, 2024 that will close this issue
Open
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug help wanted
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix allow_cors: true returning two Access-Control-Allow-Origin headers matthieugouel/chproxy
4 participants
@nezed @hagen1778 @bobelev @seifchen