From af88b88ea5089fd5b24bfbaa8e43a0121ee57661 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Tue, 13 Jun 2023 15:58:42 +0200 Subject: [PATCH] fix: check Origin to match configured CORS Origin --- pkg/handler/cors_test.go | 211 ++++++++++++++++++++++++-------- pkg/handler/unrouted_handler.go | 44 +++---- pkg/handler/utils_test.go | 15 ++- 3 files changed, 197 insertions(+), 73 deletions(-) diff --git a/pkg/handler/cors_test.go b/pkg/handler/cors_test.go index 1979f54..03b94b0 100644 --- a/pkg/handler/cors_test.go +++ b/pkg/handler/cors_test.go @@ -9,29 +9,10 @@ import ( ) func TestCORS(t *testing.T) { - SubTest(t, "Preflight", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) { - handler, _ := NewHandler(Config{ - StoreComposer: composer, - }) - - (&httpTest{ - Method: "OPTIONS", - ReqHeader: map[string]string{ - "Origin": "tus.io", - }, - Code: http.StatusOK, - ResHeader: map[string]string{ - "Access-Control-Allow-Headers": "Authorization, Origin, X-Requested-With, X-Request-ID, X-HTTP-Method-Override, Content-Type, Upload-Length, Upload-Offset, Tus-Resumable, Upload-Metadata, Upload-Defer-Length, Upload-Concat", - "Access-Control-Allow-Methods": "POST, HEAD, PATCH, OPTIONS, GET, DELETE", - "Access-Control-Max-Age": "86400", - "Access-Control-Allow-Origin": "tus.io", - }, - }).Run(handler, t) - }) - - SubTest(t, "Conditional allow methods", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) { + SubTest(t, "PreFlight - Conditional allow methods", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) { handler, _ := NewHandler(Config{ StoreComposer: composer, + CorsOrigin: "https://tus.io", DisableTermination: true, DisableDownload: true, }) @@ -39,37 +20,185 @@ func TestCORS(t *testing.T) { (&httpTest{ Method: "OPTIONS", ReqHeader: map[string]string{ - "Origin": "tus.io", + "Origin": "https://tus.io", }, Code: http.StatusOK, ResHeader: map[string]string{ "Access-Control-Allow-Headers": "Authorization, Origin, X-Requested-With, X-Request-ID, X-HTTP-Method-Override, Content-Type, Upload-Length, Upload-Offset, Tus-Resumable, Upload-Metadata, Upload-Defer-Length, Upload-Concat", "Access-Control-Allow-Methods": "POST, HEAD, PATCH, OPTIONS", "Access-Control-Max-Age": "86400", - "Access-Control-Allow-Origin": "tus.io", + "Access-Control-Allow-Origin": "https://tus.io", }, }).Run(handler, t) }) + SubTest(t, "PreFlight - No Origin configured", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) { + composer = NewStoreComposer() + composer.UseCore(store) - SubTest(t, "Request", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) { handler, _ := NewHandler(Config{ StoreComposer: composer, + CorsOrigin: "", }) (&httpTest{ - Name: "Actual request", - Method: "GET", - ReqHeader: map[string]string{ - "Origin": "tus.io", + Method: "OPTIONS", + DisallowedResHeader: []string{ + "Access-Control-Allow-Origin", + "Access-Control-Allow-Methods", + "Access-Control-Allow-Headers", + "Access-Control-Max-Age", }, - Code: http.StatusMethodNotAllowed, - ResHeader: map[string]string{ - "Access-Control-Expose-Headers": "Upload-Offset, Location, Upload-Length, Tus-Version, Tus-Resumable, Tus-Max-Size, Tus-Extension, Upload-Metadata, Upload-Defer-Length, Upload-Concat", - "Access-Control-Allow-Origin": "tus.io", + Code: http.StatusOK, + ReqHeader: map[string]string{ + "Origin": "https://tus.io", }, }).Run(handler, t) }) + SubTest(t, "PreFlight - Disabled CORS", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) { + composer = NewStoreComposer() + composer.UseCore(store) + handler, _ := NewHandler(Config{ + StoreComposer: composer, + CorsOrigin: "", + DisableCors: true, + }) + + (&httpTest{ + Method: "OPTIONS", + DisallowedResHeader: []string{ + "Access-Control-Allow-Origin", + "Access-Control-Allow-Methods", + "Access-Control-Allow-Headers", + "Access-Control-Max-Age", + }, + Code: http.StatusOK, + ReqHeader: map[string]string{ + "Origin": "https://tus.io", + }, + }).Run(handler, t) + }) + SubTest(t, "PreFlight - Wildcard Origin", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) { + composer = NewStoreComposer() + composer.UseCore(store) + + handler, _ := NewHandler(Config{ + StoreComposer: composer, + CorsOrigin: "*", + }) + + (&httpTest{ + Method: "OPTIONS", + ResHeader: map[string]string{ + "Access-Control-Allow-Origin": "*", + "Access-Control-Allow-Methods": "POST, HEAD, PATCH, OPTIONS, GET, DELETE", + "Access-Control-Allow-Headers": "Authorization, Origin, X-Requested-With, X-Request-ID, X-HTTP-Method-Override, Content-Type, Upload-Length, Upload-Offset, Tus-Resumable, Upload-Metadata, Upload-Defer-Length, Upload-Concat", + "Access-Control-Max-Age": "86400", + }, + Code: http.StatusOK, + ReqHeader: map[string]string{ + "Origin": "https://tus.io", + }, + }).Run(handler, t) + }) + SubTest(t, "PreFlight - Matching Origin", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) { + composer = NewStoreComposer() + composer.UseCore(store) + + handler, _ := NewHandler(Config{ + StoreComposer: composer, + CorsOrigin: "https://tus.io", + }) + + (&httpTest{ + Method: "OPTIONS", + ResHeader: map[string]string{ + "Access-Control-Allow-Origin": "https://tus.io", + "Access-Control-Allow-Methods": "POST, HEAD, PATCH, OPTIONS, GET, DELETE", + "Access-Control-Allow-Headers": "Authorization, Origin, X-Requested-With, X-Request-ID, X-HTTP-Method-Override, Content-Type, Upload-Length, Upload-Offset, Tus-Resumable, Upload-Metadata, Upload-Defer-Length, Upload-Concat", + "Access-Control-Max-Age": "86400", + }, + Code: http.StatusOK, + ReqHeader: map[string]string{ + "Origin": "https://tus.io", + }, + }).Run(handler, t) + }) + SubTest(t, "PreFlight - Not Matching Origin", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) { + composer = NewStoreComposer() + composer.UseCore(store) + + handler, _ := NewHandler(Config{ + StoreComposer: composer, + CorsOrigin: "https://tus.net", + }) + + (&httpTest{ + Method: "OPTIONS", + DisallowedResHeader: []string{ + "Access-Control-Allow-Origin", + "Access-Control-Allow-Methods", + "Access-Control-Allow-Headers", + "Access-Control-Max-Age", + }, + Code: http.StatusOK, + ReqHeader: map[string]string{ + "Origin": "https://tus.io", + }, + }).Run(handler, t) + }) + SubTest(t, "Actual Request - Wildcard Origin", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) { + composer = NewStoreComposer() + composer.UseCore(store) + + handler, _ := NewHandler(Config{ + StoreComposer: composer, + CorsOrigin: "*", + }) + + (&httpTest{ + Method: "POST", + ResHeader: map[string]string{ + "Access-Control-Allow-Origin": "*", + "Access-Control-Expose-Headers": "Upload-Offset, Location, Upload-Length, Tus-Version, Tus-Resumable, Tus-Max-Size, Tus-Extension, Upload-Metadata, Upload-Defer-Length, Upload-Concat", + }, + DisallowedResHeader: []string{ + "Access-Control-Allow-Methods", + "Access-Control-Allow-Headers", + "Access-Control-Max-Age", + }, + Code: http.StatusPreconditionFailed, + ReqHeader: map[string]string{ + "Origin": "https://tus.io", + }, + }).Run(handler, t) + }) + SubTest(t, "Actual Request - Matching Origin", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) { + composer = NewStoreComposer() + composer.UseCore(store) + + handler, _ := NewHandler(Config{ + StoreComposer: composer, + CorsOrigin: "https://tus.io", + }) + + (&httpTest{ + Method: "POST", + ResHeader: map[string]string{ + "Access-Control-Allow-Origin": "https://tus.io", + "Access-Control-Expose-Headers": "Upload-Offset, Location, Upload-Length, Tus-Version, Tus-Resumable, Tus-Max-Size, Tus-Extension, Upload-Metadata, Upload-Defer-Length, Upload-Concat", + }, + DisallowedResHeader: []string{ + "Access-Control-Allow-Methods", + "Access-Control-Allow-Headers", + "Access-Control-Max-Age", + }, + Code: http.StatusPreconditionFailed, + ReqHeader: map[string]string{ + "Origin": "https://tus.io", + }, + }).Run(handler, t) + }) SubTest(t, "AppendHeaders", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) { handler, _ := NewHandler(Config{ StoreComposer: composer, @@ -77,7 +206,7 @@ func TestCORS(t *testing.T) { req, _ := http.NewRequest("OPTIONS", "", nil) req.Header.Set("Tus-Resumable", "1.0.0") - req.Header.Set("Origin", "tus.io") + req.Header.Set("Origin", "https://tus.io") req.Host = "tus.io" res := httptest.NewRecorder() @@ -96,20 +225,4 @@ func TestCORS(t *testing.T) { t.Errorf("expected header to contain METHOD but got: %#v", methods) } }) - - SubTest(t, "Disable CORS", func(t *testing.T, store *MockFullDataStore, composer *StoreComposer) { - handler, _ := NewHandler(Config{ - StoreComposer: composer, - DisableCors: true, - }) - - (&httpTest{ - Method: "OPTIONS", - ReqHeader: map[string]string{ - "Origin": "tus.io", - }, - Code: http.StatusOK, - ResHeader: map[string]string{}, - }).Run(handler, t) - }) } diff --git a/pkg/handler/unrouted_handler.go b/pkg/handler/unrouted_handler.go index 76ce951..f1fbd7e 100644 --- a/pkg/handler/unrouted_handler.go +++ b/pkg/handler/unrouted_handler.go @@ -225,30 +225,32 @@ func (handler *UnroutedHandler) Middleware(h http.Handler) http.Handler { if origin := r.Header.Get("Origin"); !handler.config.DisableCors && origin != "" { var configuredOrigin = handler.config.CorsOrigin - if configuredOrigin != "" { - origin = configuredOrigin + if configuredOrigin == "*" { + origin = "*" } - header.Set("Access-Control-Allow-Origin", origin) - header.Set("Vary", "Origin") + if configuredOrigin == origin { + header.Set("Access-Control-Allow-Origin", origin) + header.Set("Vary", "Origin") - if r.Method == "OPTIONS" { - allowedMethods := "POST, HEAD, PATCH, OPTIONS" - if !handler.config.DisableDownload { - allowedMethods += ", GET" + if r.Method == "OPTIONS" { + allowedMethods := "POST, HEAD, PATCH, OPTIONS" + if !handler.config.DisableDownload { + allowedMethods += ", GET" + } + + if !handler.config.DisableTermination { + allowedMethods += ", DELETE" + } + + // Preflight request + header.Add("Access-Control-Allow-Methods", allowedMethods) + header.Add("Access-Control-Allow-Headers", "Authorization, Origin, X-Requested-With, X-Request-ID, X-HTTP-Method-Override, Content-Type, Upload-Length, Upload-Offset, Tus-Resumable, Upload-Metadata, Upload-Defer-Length, Upload-Concat") + header.Set("Access-Control-Max-Age", "86400") + + } else { + // Actual request + header.Add("Access-Control-Expose-Headers", "Upload-Offset, Location, Upload-Length, Tus-Version, Tus-Resumable, Tus-Max-Size, Tus-Extension, Upload-Metadata, Upload-Defer-Length, Upload-Concat") } - - if !handler.config.DisableTermination { - allowedMethods += ", DELETE" - } - - // Preflight request - header.Add("Access-Control-Allow-Methods", allowedMethods) - header.Add("Access-Control-Allow-Headers", "Authorization, Origin, X-Requested-With, X-Request-ID, X-HTTP-Method-Override, Content-Type, Upload-Length, Upload-Offset, Tus-Resumable, Upload-Metadata, Upload-Defer-Length, Upload-Concat") - header.Set("Access-Control-Max-Age", "86400") - - } else { - // Actual request - header.Add("Access-Control-Expose-Headers", "Upload-Offset, Location, Upload-Length, Tus-Version, Tus-Resumable, Tus-Max-Size, Tus-Extension, Upload-Metadata, Upload-Defer-Length, Upload-Concat") } } diff --git a/pkg/handler/utils_test.go b/pkg/handler/utils_test.go index c2e6356..6df2a94 100644 --- a/pkg/handler/utils_test.go +++ b/pkg/handler/utils_test.go @@ -52,9 +52,10 @@ type httpTest struct { ReqBody io.Reader ReqHeader map[string]string - Code int - ResBody string - ResHeader map[string]string + Code int + ResBody string + ResHeader map[string]string + DisallowedResHeader []string } func (test *httpTest) Run(handler http.Handler, t *testing.T) *httptest.ResponseRecorder { @@ -82,6 +83,14 @@ func (test *httpTest) Run(handler http.Handler, t *testing.T) *httptest.Response } } + for _, key := range test.DisallowedResHeader { + header := w.Header().Get(key) + + if header != "" { + t.Errorf("Not Expected '%s' (got '%s')", key, header) + } + } + if test.ResBody != "" && w.Body.String() != test.ResBody { t.Errorf("Expected '%s' as body (got '%s'", test.ResBody, w.Body.String()) }