From 871ae5d76352d68eb9fe62a805d27ce1bc377d61 Mon Sep 17 00:00:00 2001 From: Marina Biryukova Date: Wed, 30 Apr 2025 16:06:04 +0300 Subject: [PATCH] [#226] Improve CORS validation Signed-off-by: Marina Biryukova --- internal/handler/cors.go | 24 ++++++++++ internal/handler/cors_test.go | 90 +++++++++++++++++++++++++++++++++++ internal/logs/logs.go | 1 + 3 files changed, 115 insertions(+) diff --git a/internal/handler/cors.go b/internal/handler/cors.go index 7e8db93..a4b8b6b 100644 --- a/internal/handler/cors.go +++ b/internal/handler/cors.go @@ -79,6 +79,10 @@ func (h *Handler) Preflight(req *fasthttp.RequestCtx) { } for _, rule := range corsConfig.CORSRules { + if err = checkCORSRuleWildcards(rule); err != nil { + log.Error(logs.InvalidCorsRule, zap.Error(err), logs.TagField(logs.TagDatapath)) + continue + } for _, o := range rule.AllowedOrigins { if o == string(origin) || o == wildcard || (strings.Contains(o, "*") && match(o, string(origin))) { for _, m := range rule.AllowedMethods { @@ -147,6 +151,10 @@ func (h *Handler) SetCORSHeaders(req *fasthttp.RequestCtx) { } for _, rule := range corsConfig.CORSRules { + if err = checkCORSRuleWildcards(rule); err != nil { + log.Error(logs.InvalidCorsRule, zap.Error(err), logs.TagField(logs.TagDatapath)) + continue + } for _, o := range rule.AllowedOrigins { if o == string(origin) || (strings.Contains(o, "*") && len(o) > 1 && match(o, string(origin))) { for _, m := range rule.AllowedMethods { @@ -178,6 +186,22 @@ func (h *Handler) SetCORSHeaders(req *fasthttp.RequestCtx) { } } +func checkCORSRuleWildcards(rule data.CORSRule) error { + for _, origin := range rule.AllowedOrigins { + if strings.Count(origin, wildcard) > 1 { + return fmt.Errorf("invalid allowed origin: %s", origin) + } + } + + for _, header := range rule.AllowedHeaders { + if strings.Count(header, wildcard) > 1 { + return fmt.Errorf("invalid allowed header: %s", header) + } + } + + return nil +} + func (h *Handler) getCORSConfig(ctx context.Context, log *zap.Logger, cidStr string) (*data.CORSConfiguration, error) { cnrID, err := h.resolveContainer(ctx, cidStr) if err != nil { diff --git a/internal/handler/cors_test.go b/internal/handler/cors_test.go index 1ac07d7..629449f 100644 --- a/internal/handler/cors_test.go +++ b/internal/handler/cors_test.go @@ -176,6 +176,55 @@ func TestPreflight(t *testing.T) { }, status: fasthttp.StatusBadRequest, }, + { + name: "invalid allowed origin", + corsConfig: &data.CORSConfiguration{ + CORSRules: []data.CORSRule{ + { + AllowedOrigins: []string{"*.example.*"}, + AllowedMethods: []string{"HEAD"}, + }, + }, + }, + requestHeaders: map[string]string{ + fasthttp.HeaderOrigin: "http://www.example.com", + fasthttp.HeaderAccessControlRequestMethod: "HEAD", + }, + expectedHeaders: map[string]string{ + fasthttp.HeaderAccessControlAllowOrigin: "", + fasthttp.HeaderAccessControlAllowMethods: "", + fasthttp.HeaderAccessControlAllowHeaders: "", + fasthttp.HeaderAccessControlExposeHeaders: "", + fasthttp.HeaderAccessControlMaxAge: "", + fasthttp.HeaderAccessControlAllowCredentials: "", + }, + status: fasthttp.StatusForbidden, + }, + { + name: "invalid allowed header", + corsConfig: &data.CORSConfiguration{ + CORSRules: []data.CORSRule{ + { + AllowedOrigins: []string{"*example.com"}, + AllowedMethods: []string{"HEAD"}, + AllowedHeaders: []string{"x-amz-*-*"}, + }, + }, + }, + requestHeaders: map[string]string{ + fasthttp.HeaderOrigin: "http://www.example.com", + fasthttp.HeaderAccessControlRequestMethod: "HEAD", + }, + expectedHeaders: map[string]string{ + fasthttp.HeaderAccessControlAllowOrigin: "", + fasthttp.HeaderAccessControlAllowMethods: "", + fasthttp.HeaderAccessControlAllowHeaders: "", + fasthttp.HeaderAccessControlExposeHeaders: "", + fasthttp.HeaderAccessControlMaxAge: "", + fasthttp.HeaderAccessControlAllowCredentials: "", + }, + status: fasthttp.StatusForbidden, + }, } { t.Run(tc.name, func(t *testing.T) { if tc.corsConfig != nil { @@ -328,6 +377,47 @@ func TestSetCORSHeaders(t *testing.T) { fasthttp.HeaderAccessControlAllowCredentials: "", }, }, + { + name: "invalid allowed origin", + corsConfig: &data.CORSConfiguration{ + CORSRules: []data.CORSRule{ + { + AllowedOrigins: []string{"*.example.*"}, + AllowedMethods: []string{"GET"}, + }, + }, + }, + requestHeaders: map[string]string{ + fasthttp.HeaderOrigin: "http://www.example.com", + }, + expectedHeaders: map[string]string{ + fasthttp.HeaderAccessControlAllowOrigin: "", + fasthttp.HeaderAccessControlAllowMethods: "", + fasthttp.HeaderVary: "", + fasthttp.HeaderAccessControlAllowCredentials: "", + }, + }, + { + name: "invalid allowed header", + corsConfig: &data.CORSConfiguration{ + CORSRules: []data.CORSRule{ + { + AllowedOrigins: []string{"*example.com"}, + AllowedMethods: []string{"GET"}, + AllowedHeaders: []string{"x-amz-*-*"}, + }, + }, + }, + requestHeaders: map[string]string{ + fasthttp.HeaderOrigin: "http://www.example.com", + }, + expectedHeaders: map[string]string{ + fasthttp.HeaderAccessControlAllowOrigin: "", + fasthttp.HeaderAccessControlAllowMethods: "", + fasthttp.HeaderVary: "", + fasthttp.HeaderAccessControlAllowCredentials: "", + }, + }, } { t.Run(tc.name, func(t *testing.T) { epoch++ diff --git a/internal/logs/logs.go b/internal/logs/logs.go index 86921dd..68b8835 100644 --- a/internal/logs/logs.go +++ b/internal/logs/logs.go @@ -127,6 +127,7 @@ const ( EmptyAccessControlRequestMethodHeader = "empty Access-Control-Request-Method request header" CORSRuleWasNotMatched = "cors rule was not matched" CouldntCacheCors = "couldn't cache cors" + InvalidCorsRule = "invalid cors rule, skip" ) // Log messages with the "external_storage" tag. -- 2.47.2