From c557a8b230784c2cf71a2ee0b2b9ff1c3dbbe0dc Mon Sep 17 00:00:00 2001 From: Jian Qiu Date: Thu, 3 Sep 2026 23:53:36 +0800 Subject: [PATCH] fix(appsrv): do not honor CORS credentials for wildcard origins (#25501) With cors_hosts unset (the default), the CORS middleware allowed all origins and echoed the request origin together with Access-Control-Allow-Credentials, so any website could make authenticated cross-origin requests against the API with the user's cookies. Credentials are now only honored for explicitly listed origins: with an empty or wildcard origin allowlist the middleware responds with Access-Control-Allow-Origin: * and no credentials header. Co-authored-by: Qiu Jian Co-authored-by: Claude --- pkg/appsrv/cors.go | 10 ++++ pkg/appsrv/cors_test.go | 91 ++++++++++++++++++++++++++++++ pkg/cloudcommon/options/options.go | 2 +- 3 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 pkg/appsrv/cors_test.go diff --git a/pkg/appsrv/cors.go b/pkg/appsrv/cors.go index 3d13b94f61..4720e9cbcd 100644 --- a/pkg/appsrv/cors.go +++ b/pkg/appsrv/cors.go @@ -138,6 +138,9 @@ type CorsOptions struct { ExposedHeaders []string // AllowCredentials indicates whether the request can include user credentials like // cookies, HTTP authentication or client side SSL certificates. + // Credentials are only honored for explicitly listed origins: when AllowedOrigins + // is empty or contains "*", echoing any origin with credentials would allow any + // website to make credentialed requests. AllowCredentials bool // MaxAge indicates how long (in seconds) the results of a preflight request // can be cached @@ -215,6 +218,13 @@ func NewCors(options CorsOptions) *Cors { } } + // Credentials with a wildcard/unset origin allowlist would let any + // website send authenticated cross-origin requests, so credentials are + // only honored for explicitly listed origins + if c.allowedOriginsAll { + c.allowCredentials = false + } + // Allowed Headers if len(options.AllowedHeaders) == 0 { // Use sensible defaults diff --git a/pkg/appsrv/cors_test.go b/pkg/appsrv/cors_test.go new file mode 100644 index 0000000000..1570f3cd1d --- /dev/null +++ b/pkg/appsrv/cors_test.go @@ -0,0 +1,91 @@ +// Copyright 2019 Yunion +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package appsrv + +import ( + "net/http/httptest" + "testing" +) + +// with an unset or wildcard origin allowlist, credentials must never be +// honored: echoing any origin with credentials would let any website make +// authenticated cross-origin requests +func TestCorsWildcardDisablesCredentials(t *testing.T) { + c := NewCors(CorsOptions{ + AllowedMethods: []string{"GET"}, + AllowedHeaders: []string{"*"}, + AllowCredentials: true, + }) + if c.allowCredentials { + t.Fatalf("wildcard origins must not honor credentials") + } + req := httptest.NewRequest("OPTIONS", "/", nil) + req.Header.Set("Origin", "http://evil.com") + req.Header.Set("Access-Control-Request-Method", "GET") + w := httptest.NewRecorder() + c.handlePreflight(w, req) + if got := w.Header().Get("Access-Control-Allow-Origin"); got != "*" { + t.Fatalf("Allow-Origin = %q, want *", got) + } + if got := w.Header().Get("Access-Control-Allow-Credentials"); got != "" { + t.Fatalf("Allow-Credentials must not be set, got %q", got) + } + + // actual request + req = httptest.NewRequest("GET", "/", nil) + req.Header.Set("Origin", "http://evil.com") + w = httptest.NewRecorder() + c.handleActualRequest(w, req) + if got := w.Header().Get("Access-Control-Allow-Credentials"); got != "" { + t.Fatalf("Allow-Credentials must not be set, got %q", got) + } +} + +// explicitly listed origins keep credentials, other origins get nothing +func TestCorsExplicitOriginsKeepCredentials(t *testing.T) { + c := NewCors(CorsOptions{ + AllowedOrigins: []string{"foo.com"}, + AllowedMethods: []string{"GET"}, + AllowedHeaders: []string{"*"}, + AllowCredentials: true, + }) + if !c.allowCredentials { + t.Fatalf("explicit origins must honor credentials") + } + req := httptest.NewRequest("OPTIONS", "/", nil) + req.Header.Set("Origin", "http://foo.com") + req.Header.Set("Access-Control-Request-Method", "GET") + w := httptest.NewRecorder() + c.handlePreflight(w, req) + if got := w.Header().Get("Access-Control-Allow-Origin"); got != "http://foo.com" { + t.Fatalf("Allow-Origin = %q, want http://foo.com", got) + } + if got := w.Header().Get("Access-Control-Allow-Credentials"); got != "true" { + t.Fatalf("Allow-Credentials = %q, want true", got) + } + + // a disallowed origin gets no CORS headers + req = httptest.NewRequest("OPTIONS", "/", nil) + req.Header.Set("Origin", "http://evil.com") + req.Header.Set("Access-Control-Request-Method", "GET") + w = httptest.NewRecorder() + c.handlePreflight(w, req) + if got := w.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Fatalf("disallowed origin must not get Allow-Origin, got %q", got) + } + if got := w.Header().Get("Access-Control-Allow-Credentials"); got != "" { + t.Fatalf("disallowed origin must not get Allow-Credentials, got %q", got) + } +} diff --git a/pkg/cloudcommon/options/options.go b/pkg/cloudcommon/options/options.go index 1f2a536fc7..e2082fbbe8 100644 --- a/pkg/cloudcommon/options/options.go +++ b/pkg/cloudcommon/options/options.go @@ -66,7 +66,7 @@ type BaseOptions struct { LogVerboseLevel int `help:"log verbosity level" default:"0"` LogFilePrefix string `help:"prefix of log files"` - CorsHosts []string `help:"List of hostname that allow CORS"` + CorsHosts []string `help:"List of hostname that allow CORS, credentials are only honored for explicitly listed origins"` TempPath string `help:"Path for store temp file, at least 40G space" default:"/opt/yunion/tmp"` ApplicationID string `help:"Application ID"`