From 61c73969116be7a4dcee9898e704354441dc2430 Mon Sep 17 00:00:00 2001 From: matthieugouel Date: Sun, 1 Dec 2024 11:14:52 +0100 Subject: [PATCH 1/6] fix: `allow_cors: true` returning two `Access-Control-Allow-Origin` headers 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 `*`. --- proxy.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/proxy.go b/proxy.go index 15ca73ea..c02dbd8a 100644 --- a/proxy.go +++ b/proxy.go @@ -107,14 +107,6 @@ func (rp *reverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) { log.Debugf("%s: request start", s) requestSum.With(s.labels).Inc() - if s.user.allowCORS { - origin := req.Header.Get("Origin") - if len(origin) == 0 { - origin = "*" - } - rw.Header().Set("Access-Control-Allow-Origin", origin) - } - req.Body = &statReadCloser{ ReadCloser: req.Body, bytesRead: requestBodyBytes.With(s.labels), @@ -149,6 +141,14 @@ func (rp *reverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) { rp.proxyRequest(s, srw, srw, req) } + if s.user.allowCORS { + origin := req.Header.Get("Origin") + if len(origin) == 0 { + origin = "*" + } + rw.Header().Set("Access-Control-Allow-Origin", origin) + } + // It is safe calling getQuerySnippet here, since the request // has been already read in proxyRequest or serveFromCache. query := getQuerySnippet(req) From 818168b4bab09f867d075e3ca7a8317e3962cd0f Mon Sep 17 00:00:00 2001 From: matthieugouel Date: Fri, 13 Dec 2024 11:53:16 +0100 Subject: [PATCH 2/6] fix: add tests for allow CORS behavior --- main_test.go | 53 ++++++++++++++++++++++++++++++++++++ testdata/http.allow.cors.yml | 15 ++++++++++ 2 files changed, 68 insertions(+) create mode 100644 testdata/http.allow.cors.yml diff --git a/main_test.go b/main_test.go index 7e721a29..a8c4ed05 100644 --- a/main_test.go +++ b/main_test.go @@ -896,6 +896,59 @@ func TestServe(t *testing.T) { }, startHTTP, }, + { + "http allow CORS false", + "testdata/http.yml", + func(t *testing.T) { + req, err := http.NewRequest(http.MethodOptions, "http://127.0.0.1:9090?query=asd", nil) + checkErr(t, err) + resp, err := http.DefaultClient.Do(req) + checkErr(t, err) + if resp.StatusCode != http.StatusOK { + t.Fatalf("unexpected status code: %d; expected: %d", resp.StatusCode, http.StatusOK) + } + + resp.Body.Close() + checkHeader(t, resp, "Access-Control-Allow-Origin", "") + }, + startHTTP, + }, + { + "http allow CORS true without request Origin header", + "testdata/http.allow.cors.yml", + func(t *testing.T) { + q := "SELECT 123" + req, err := http.NewRequest("GET", "http://127.0.0.1:9090?query="+url.QueryEscape(q), nil) + checkErr(t, err) + resp, err := http.DefaultClient.Do(req) + checkErr(t, err) + if resp.StatusCode != http.StatusOK { + t.Fatalf("unexpected status code: %d; expected: %d", resp.StatusCode, http.StatusOK) + } + resp.Body.Close() + checkHeader(t, resp, "Access-Control-Allow-Origin", "*") + }, + startHTTP, + }, + { + "http allow CORS true with request Origin header", + "testdata/http.allow.cors.yml", + func(t *testing.T) { + q := "SELECT 123" + req, err := http.NewRequest("GET", "http://127.0.0.1:9090?query="+url.QueryEscape(q), nil) + checkErr(t, err) + req.Header.Set("Origin", "http://example.com") + resp, err := http.DefaultClient.Do(req) + checkErr(t, err) + if resp.StatusCode != http.StatusOK { + t.Fatalf("unexpected status code: %d; expected: %d", resp.StatusCode, http.StatusOK) + } + + resp.Body.Close() + checkHeader(t, resp, "Access-Control-Allow-Origin", "http://example.com") + }, + startHTTP, + }, } // Wait until CHServer starts. diff --git a/testdata/http.allow.cors.yml b/testdata/http.allow.cors.yml new file mode 100644 index 00000000..db445b56 --- /dev/null +++ b/testdata/http.allow.cors.yml @@ -0,0 +1,15 @@ +log_debug: true +server: + http: + listen_addr: ":9090" + allowed_networks: ["127.0.0.1/24"] + +users: + - name: "default" + to_cluster: "default" + to_user: "default" + allow_cors: true + +clusters: + - name: "default" + nodes: ["127.0.0.1:18124"] \ No newline at end of file From b0051c78aba8b1fcb9d9c5a24a646bd5c7c0974f Mon Sep 17 00:00:00 2001 From: Christophe Kalenzaga Date: Tue, 24 Dec 2024 12:04:26 +0100 Subject: [PATCH 3/6] fix failing tests --- main_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/main_test.go b/main_test.go index a8c4ed05..65ab647e 100644 --- a/main_test.go +++ b/main_test.go @@ -900,15 +900,14 @@ func TestServe(t *testing.T) { "http allow CORS false", "testdata/http.yml", func(t *testing.T) { - req, err := http.NewRequest(http.MethodOptions, "http://127.0.0.1:9090?query=asd", nil) + req, err := http.NewRequest(http.MethodOptions, "http://127.0.0.1:9090?query=cors", nil) checkErr(t, err) resp, err := http.DefaultClient.Do(req) checkErr(t, err) if resp.StatusCode != http.StatusOK { t.Fatalf("unexpected status code: %d; expected: %d", resp.StatusCode, http.StatusOK) } - - resp.Body.Close() + defer resp.Body.Close() checkHeader(t, resp, "Access-Control-Allow-Origin", "") }, startHTTP, @@ -917,7 +916,7 @@ func TestServe(t *testing.T) { "http allow CORS true without request Origin header", "testdata/http.allow.cors.yml", func(t *testing.T) { - q := "SELECT 123" + q := "cors" req, err := http.NewRequest("GET", "http://127.0.0.1:9090?query="+url.QueryEscape(q), nil) checkErr(t, err) resp, err := http.DefaultClient.Do(req) @@ -925,7 +924,7 @@ func TestServe(t *testing.T) { if resp.StatusCode != http.StatusOK { t.Fatalf("unexpected status code: %d; expected: %d", resp.StatusCode, http.StatusOK) } - resp.Body.Close() + defer resp.Body.Close() checkHeader(t, resp, "Access-Control-Allow-Origin", "*") }, startHTTP, @@ -934,7 +933,7 @@ func TestServe(t *testing.T) { "http allow CORS true with request Origin header", "testdata/http.allow.cors.yml", func(t *testing.T) { - q := "SELECT 123" + q := "cors" req, err := http.NewRequest("GET", "http://127.0.0.1:9090?query="+url.QueryEscape(q), nil) checkErr(t, err) req.Header.Set("Origin", "http://example.com") @@ -943,8 +942,7 @@ func TestServe(t *testing.T) { if resp.StatusCode != http.StatusOK { t.Fatalf("unexpected status code: %d; expected: %d", resp.StatusCode, http.StatusOK) } - - resp.Body.Close() + defer resp.Body.Close() checkHeader(t, resp, "Access-Control-Allow-Origin", "http://example.com") }, startHTTP, @@ -1161,6 +1159,8 @@ func fakeCHHandler(w http.ResponseWriter, r *http.Request) { // execute sleep 1.5 sec time.Sleep(1500 * time.Millisecond) fmt.Fprint(w, b) + case q == "cors": + w.Header().Set("Access-Control-Allow-Origin", "*") default: if strings.Contains(string(query), killQueryPattern) { fakeCHState.kill() From 7be2d1e71b37c3798995e51e82891170a1f1c280 Mon Sep 17 00:00:00 2001 From: Christophe Kalenzaga Date: Tue, 24 Dec 2024 12:18:10 +0100 Subject: [PATCH 4/6] add failing test --- main_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/main_test.go b/main_test.go index 65ab647e..2abd57ed 100644 --- a/main_test.go +++ b/main_test.go @@ -900,7 +900,8 @@ func TestServe(t *testing.T) { "http allow CORS false", "testdata/http.yml", func(t *testing.T) { - req, err := http.NewRequest(http.MethodOptions, "http://127.0.0.1:9090?query=cors", nil) + q := "cors" + req, err := http.NewRequest("GET", "http://127.0.0.1:9090?query="+url.QueryEscape(q), nil) checkErr(t, err) resp, err := http.DefaultClient.Do(req) checkErr(t, err) From f18a73482c6673160ab86542af42c1680aad9de8 Mon Sep 17 00:00:00 2001 From: matthieugouel Date: Wed, 25 Dec 2024 17:32:15 +0100 Subject: [PATCH 5/6] fix failing tests --- main_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main_test.go b/main_test.go index 2abd57ed..d8d3b76f 100644 --- a/main_test.go +++ b/main_test.go @@ -909,7 +909,7 @@ func TestServe(t *testing.T) { t.Fatalf("unexpected status code: %d; expected: %d", resp.StatusCode, http.StatusOK) } defer resp.Body.Close() - checkHeader(t, resp, "Access-Control-Allow-Origin", "") + checkHeader(t, resp, "Access-Control-Allow-Origin", "*") }, startHTTP, }, From f6f90cbb24f3f59145771b7a1de8f8271ae3284f Mon Sep 17 00:00:00 2001 From: Christophe Kalenzaga Date: Tue, 4 Feb 2025 09:35:27 +0100 Subject: [PATCH 6/6] add TODO --- main_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/main_test.go b/main_test.go index d8d3b76f..c907ec14 100644 --- a/main_test.go +++ b/main_test.go @@ -900,6 +900,8 @@ func TestServe(t *testing.T) { "http allow CORS false", "testdata/http.yml", func(t *testing.T) { + // TODO: rework this test because it doesn't fails when it should + // cf the discussion in https://github.com/ContentSquare/chproxy/pull/489 q := "cors" req, err := http.NewRequest("GET", "http://127.0.0.1:9090?query="+url.QueryEscape(q), nil) checkErr(t, err) @@ -917,6 +919,8 @@ func TestServe(t *testing.T) { "http allow CORS true without request Origin header", "testdata/http.allow.cors.yml", func(t *testing.T) { + // TODO: rework this test because it doesn't fails when it should + // cf the discussion in https://github.com/ContentSquare/chproxy/pull/489 q := "cors" req, err := http.NewRequest("GET", "http://127.0.0.1:9090?query="+url.QueryEscape(q), nil) checkErr(t, err) @@ -934,6 +938,8 @@ func TestServe(t *testing.T) { "http allow CORS true with request Origin header", "testdata/http.allow.cors.yml", func(t *testing.T) { + // TODO: rework this test because it doesn't fails when it should + // cf the discussion in https://github.com/ContentSquare/chproxy/pull/489 q := "cors" req, err := http.NewRequest("GET", "http://127.0.0.1:9090?query="+url.QueryEscape(q), nil) checkErr(t, err)