Fix allow_cors: true returning two Access-Control-Allow-Origin headers

[matthieugouel]

Description

Fixes #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 *.

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

Checklist

• Linter passes correctly
• Add tests which fail without the change (if possible)
• All tests passing
• Extended the README / documentation, if necessary

Does this introduce a breaking change?

• Yes
• No

Further comments

The only thing I'm not sure is if we want to pass the initial Access-Control-Allow-Origin that was set originally.
But in my understanding of the feature, the goal is not to add this header to the proxied request to ClickHouse, but rather on the chproxy response.

[vfoucault]

Hi there and thanks for your contribution.

I honestly don't believe that cors, moved from line 110 to 144 will not have any effect.
Response is handled in that block

if shouldReturnFromCache {
		rp.serveFromCache(s, srw, req, origParams, q)
	} else {
		rp.proxyRequest(s, srw, srw, req)
	}

Adding a header after doesn't seems to have any effect since the WriteHeader would already have occurred in io.go line 71.

I don't have any idea right now on how to handle this. best would be to open an issue, and feel free to propose a solution for this issue as well.

thanks

[matthieugouel]

Hey,

Without any change, a call with to chproxy with Origin header (set to localhost:1234) and allow_origin: true option will result with those response headers:

< HTTP/2 200
< access-control-allow-headers: origin, x-requested-with, x-clickhouse-format, x-clickhouse-user, x-clickhouse-key, Authorization
< access-control-allow-methods: POST, GET, OPTIONS
< access-control-allow-origin: localhost:1234
< access-control-allow-origin: *
< access-control-max-age: 86400
< alt-svc: h3=":443"; ma=2592000
< content-type: text/tab-separated-values; charset=UTF-8
< date: Mon, 02 Dec 2024 16:39:23 GMT
< server: Caddy
< x-clickhouse-format: TabSeparated
< x-clickhouse-query-id: 180D6993B76A406F
< x-clickhouse-server-display-name: cc2ee7e84a61
< x-clickhouse-summary: {"read_rows":"0","read_bytes":"0","written_rows":"0","written_bytes":"0","total_rows_to_read":"0","result_rows":"0","result_bytes":"0","elapsed_ns":"4444212"}
< x-clickhouse-timezone: UTC
< content-length: 0
<

You can notice the duplicate access-control-allow-origin that the issue #93 is referring to.

When doing the same test with the change of this PR (no config change), we have the following response headers:

< HTTP/2 200
< access-control-allow-headers: origin, x-requested-with, x-clickhouse-format, x-clickhouse-user, x-clickhouse-key, Authorization
< access-control-allow-methods: POST, GET, OPTIONS
< access-control-allow-origin: localhost:1234
< access-control-max-age: 86400
< alt-svc: h3=":443"; ma=2592000
< content-type: text/tab-separated-values; charset=UTF-8
< date: Mon, 02 Dec 2024 16:46:11 GMT
< server: Caddy
< x-clickhouse-format: TabSeparated
< x-clickhouse-query-id: 180D69FCF89CFE24
< x-clickhouse-server-display-name: cc2ee7e84a61
< x-clickhouse-summary: {"read_rows":"0","read_bytes":"0","written_rows":"0","written_bytes":"0","total_rows_to_read":"0","result_rows":"0","result_bytes":"0","elapsed_ns":"4273178"}
< x-clickhouse-timezone: UTC
< content-length: 0
<

As you can see we have only one access-control-allow-origin corresponding to the Origin.

To finish we can do the same test without the origin on the PRs's version:

< HTTP/2 200
< access-control-allow-origin: *
< alt-svc: h3=":443"; ma=2592000
< content-type: text/tab-separated-values; charset=UTF-8
< date: Mon, 02 Dec 2024 16:47:43 GMT
< server: Caddy
< x-clickhouse-format: TabSeparated
< x-clickhouse-query-id: 180D69FCF89CFE25
< x-clickhouse-server-display-name: cc2ee7e84a61
< x-clickhouse-summary: {"read_rows":"0","read_bytes":"0","written_rows":"0","written_bytes":"0","total_rows_to_read":"0","result_rows":"0","result_bytes":"0","elapsed_ns":"4530191"}
< x-clickhouse-timezone: UTC
< content-length: 0
<

Which is the same result as without the change, ie. only one access-control-allow-origin set as *.

The change works because it will update the access-control-allow-origin chproxy response after the header change made by the proxified response from ClickHouse. This proxified response adds its own access-control-allow-origin, so if chproxy add access-control-allow-origin before, you will end up with two times the header.
Conversely, updating this header after the proxified response will override ClickHouse access-control-allow-origin header and so you will always end up with only 1 header.

[mga-chka]

lgtm, we should release a new version including your fix in 1-2 weeks (we first need to validate and merge the other active PRs)

[mga-chka]

I was a bit in a rush in my "looks good to me". Since the bug is tricky and could reappear after a refactoring, can you add a test in main_test.go?

[matthieugouel]

Hey @mga-chka! Thanks for looking at this.
I added some tests but unfortunately they do not work for now. For some reasons I don't get the test response does not contains the headers set by the proxy. So the tests I've added never contain Access-Control-Allow-Origin.

Would you have an idea of why?

EDIT: it seems that every header set after those lines are not returned in the tests: https://github.com/ContentSquare/chproxy/blob/master/proxy.go#L146-L150

[mga-chka]

sorry, I had quite busy days, I'll try to look at the pb before the end of the week. If it's too complex to get the tests works then we will merge the PR as is.

[mga-chka]

FYI, your tests were missing a new behavior in the fake clickhouse to add the "Access-Control-Allow-Origin".
I've added the fix, I'll merge the PR next year then do a release.

[mga-chka]

I was not meticulous enough before writting my previous msg. I didn't fix one of your 3 tests, when I fixed it, it was not working anymore. Can you have a look at it? Moreover, the 2 passing tests works even without your change on proxy.go, which means they're not good enough.

[matthieugouel]

Hey!

If the fake CH instance is returning Access-Control-Allow-Origin: *, I guess it makes sense to fix the test by expecting* so I fixed it in a new commit.
Now all the tests works, but indeed the tests also work without the change we are trying to test. I think it's because the way the tests are generally implemented which are not exactly representing what's happen with the headers in reality.

I don't have much time to look at the tests implementation right now, feel free to change the PR as you see best!