From 0eebc928f755eea12592f7d5e4c6749e7e83d5be Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Sat, 29 Aug 2026 09:10:58 +0800 Subject: [PATCH] fix: complete bucket CORS protocol validation Integrate the strict B3 XML, validation, checksum, wildcard, MaxAge, and Origin-null response contract with the C-prime site-replication register from #75. Preserve fail-closed metadata behavior and rejected-preflight cache variation while keeping legacy-invalid development metadata readable and repairable through a valid CORS PUT or DELETE. Add combined parser, handler, browser-response, namespace, replication, restart, and legacy-repair regressions, and update the internal design contract. Refs #75 Signed-off-by: Feng Ruohang --- cmd/api-router.go | 20 +- cmd/bucket-cors-adversarial_test.go | 235 ++++++++++++++++++ cmd/bucket-cors-handlers.go | 11 +- cmd/bucket-cors-middleware_test.go | 144 ++++++++++- cmd/bucket-cors-site-replication_test.go | 60 +++++ cmd/bucket-metadata-sys.go | 6 + cmd/bucket-metadata.go | 23 +- cmd/generic-handlers.go | 5 +- docs/site-replication/CORS-LWW-DESIGN.md | 61 ++++- internal/bucket/cors/cors.go | 195 +++++++++++++-- internal/bucket/cors/cors_adversarial_test.go | 222 +++++++++++++++++ internal/bucket/cors/cors_test.go | 15 +- 12 files changed, 946 insertions(+), 51 deletions(-) create mode 100644 cmd/bucket-cors-adversarial_test.go create mode 100644 internal/bucket/cors/cors_adversarial_test.go diff --git a/cmd/api-router.go b/cmd/api-router.go index 6eb6a9715..8de86e9bb 100644 --- a/cmd/api-router.go +++ b/cmd/api-router.go @@ -18,6 +18,7 @@ package cmd import ( + "context" "errors" "net" "net/http" @@ -32,6 +33,8 @@ import ( "github.com/rs/cors" ) +type bucketCorsAppliedKey struct{} + func newHTTPServerFn() *xhttp.Server { globalObjLayerMutex.RLock() defer globalObjLayerMutex.RUnlock() @@ -652,7 +655,8 @@ func registerAPIRouter(router *mux.Router) { // (request is complete). For an actual request it adds the applicable // Access-Control-* response headers and returns false so the request // continues down the handler chain. If no rule matches a preflight it writes -// 403 and returns true. +// 403 and returns true. A matched actual request is marked in its context so +// inner legacy middleware does not rewrite an explicitly allowed null origin. func applyBucketCors(w http.ResponseWriter, r *http.Request, cfg *bktcors.Config) (handled bool) { origin := r.Header.Get("Origin") if origin == "" { @@ -671,21 +675,21 @@ func applyBucketCors(w http.ResponseWriter, r *http.Request, cfg *bktcors.Config // determine the outcome, including when the request is rejected. h.Add("Vary", "Access-Control-Request-Method") h.Add("Vary", "Access-Control-Request-Headers") - rule, allowedOrigin, allowedHeaders, ok := cfg.MatchPreflight(origin, method, reqHeaders) + rule, allowedOrigin, allowedHeaders, maxAgeSeconds, ok := cfg.MatchPreflight(origin, method, reqHeaders) if !ok { writeResponse(w, http.StatusForbidden, nil, mimeNone) return true } setBucketCorsOriginHeaders(h, allowedOrigin, origin) - h.Set("Access-Control-Allow-Methods", method) + h.Set("Access-Control-Allow-Methods", strings.Join(rule.AllowedMethods, ", ")) if len(allowedHeaders) > 0 { h.Set("Access-Control-Allow-Headers", strings.Join(allowedHeaders, ", ")) } if len(rule.ExposeHeaders) > 0 { h.Set("Access-Control-Expose-Headers", strings.Join(rule.ExposeHeaders, ", ")) } - if rule.MaxAgeSeconds > 0 { - h.Set("Access-Control-Max-Age", strconv.Itoa(rule.MaxAgeSeconds)) + if maxAgeSeconds != nil { + h.Set("Access-Control-Max-Age", strconv.Itoa(*maxAgeSeconds)) } writeResponse(w, http.StatusOK, nil, mimeNone) return true @@ -696,6 +700,7 @@ func applyBucketCors(w http.ResponseWriter, r *http.Request, cfg *bktcors.Config if !ok { return false // no matching rule → no CORS headers, continue normally } + *r = *r.WithContext(context.WithValue(r.Context(), bucketCorsAppliedKey{}, struct{}{})) setBucketCorsOriginHeaders(h, allowedOrigin, origin) if len(rule.ExposeHeaders) > 0 { h.Set("Access-Control-Expose-Headers", strings.Join(rule.ExposeHeaders, ", ")) @@ -703,6 +708,11 @@ func applyBucketCors(w http.ResponseWriter, r *http.Request, cfg *bktcors.Config return false } +func bucketCorsWasApplied(r *http.Request) bool { + _, ok := r.Context().Value(bucketCorsAppliedKey{}).(struct{}) + return ok +} + func setBucketCorsOriginHeaders(h http.Header, allowedOrigin, requestOrigin string) { if allowedOrigin == "*" { h.Set("Access-Control-Allow-Origin", "*") diff --git a/cmd/bucket-cors-adversarial_test.go b/cmd/bucket-cors-adversarial_test.go new file mode 100644 index 000000000..49c0fe9b1 --- /dev/null +++ b/cmd/bucket-cors-adversarial_test.go @@ -0,0 +1,235 @@ +// Copyright (c) 2015-2021 MinIO, Inc. +// +// This file is part of MinIO Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +package cmd + +import ( + "bytes" + "encoding/base64" + "encoding/binary" + "hash/crc32" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/minio/minio/internal/auth" +) + +func TestPutBucketCorsWireValidation(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testPutBucketCorsWireValidation, + endpoints: []string{"PutBucketCors"}, + }) +} + +func testPutBucketCorsWireValidation(_ ObjectLayer, _ string, bucketName string, apiRouter http.Handler, + creds auth.Credentials, t *testing.T, +) { + valid := `*GET` + rule := `*GET` + tests := []struct { + name string + body string + want int + wantCode string + }{ + { + name: "second XML root", + body: valid + ``, + want: http.StatusBadRequest, + wantCode: "MalformedXML", + }, + { + name: "255 Unicode character ID", + body: `` + strings.Repeat("界", 255) + `*GET`, + want: http.StatusOK, + }, + { + name: "256 Unicode character ID", + body: `` + strings.Repeat("界", 256) + `*GET`, + want: http.StatusBadRequest, + wantCode: "MalformedXML", + }, + { + name: "lowercase method", + body: `*get`, + want: http.StatusBadRequest, + wantCode: "MalformedXML", + }, + { + name: "empty origin", + body: `GET`, + want: http.StatusBadRequest, + wantCode: "MalformedXML", + }, + { + name: "question mark origin wildcard", + body: `https://?.example.comGET`, + want: http.StatusBadRequest, + wantCode: "MalformedXML", + }, + { + name: "question mark header wildcard", + body: `*GETx-amz-?`, + want: http.StatusBadRequest, + wantCode: "MalformedXML", + }, + { + name: "unknown element", + body: `*GET`, + want: http.StatusBadRequest, + wantCode: "MalformedXML", + }, + { + name: "empty max age", + body: `*GET`, + want: http.StatusBadRequest, + wantCode: "MalformedXML", + }, + { + name: "zero max age", + body: `*GET0`, + want: http.StatusOK, + }, + { + name: "max age int32 overflow", + body: `*GET2147483648`, + want: http.StatusBadRequest, + wantCode: "MalformedXML", + }, + { + name: "100 rules", + body: `` + strings.Repeat(rule, 100) + ``, + want: http.StatusOK, + }, + { + name: "101 rules", + body: `` + strings.Repeat(rule, 101) + ``, + want: http.StatusBadRequest, + wantCode: "MalformedXML", + }, + { + name: "exactly 64 KiB", + body: sizedCORSConfig(maxBucketCorsSize), + want: http.StatusOK, + }, + { + name: "over 64 KiB", + body: sizedCORSConfig(maxBucketCorsSize + 1), + want: http.StatusBadRequest, + wantCode: "EntityTooLarge", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req, err := newTestSignedRequestV4(http.MethodPut, getBucketCorsURL("", bucketName), + int64(len(tt.body)), bytes.NewReader([]byte(tt.body)), creds.AccessKey, creds.SecretKey, nil) + if err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + apiRouter.ServeHTTP(rec, req) + if rec.Code != tt.want { + t.Fatalf("expected status %d, got %d: %s", tt.want, rec.Code, rec.Body.String()) + } + if tt.wantCode != "" && !bytes.Contains(rec.Body.Bytes(), []byte(""+tt.wantCode+"")) { + t.Fatalf("expected error code %s, got: %s", tt.wantCode, rec.Body.String()) + } + }) + } +} + +func sizedCORSConfig(size int) string { + prefix := `*GET` + suffix := `` + return prefix + strings.Repeat(" ", size-len(prefix)-len(suffix)) + suffix +} + +func TestPutBucketCorsChecksumValidation(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testPutBucketCorsChecksumValidation, + endpoints: []string{"PutBucketCors"}, + }) +} + +func testPutBucketCorsChecksumValidation(_ ObjectLayer, _ string, bucketName string, apiRouter http.Handler, + creds auth.Credentials, t *testing.T, +) { + body := []byte(`*GET`) + tests := []struct { + name string + configure func(*http.Request) + want int + wantCode string + }{ + { + name: "missing checksum", + configure: func(req *http.Request) { + req.Header.Del("Content-Md5") + }, + want: http.StatusBadRequest, + wantCode: "MissingContentMD5", + }, + { + name: "bad content md5", + configure: func(req *http.Request) { + req.Header.Set("Content-Md5", getMD5HashBase64([]byte("different body"))) + }, + want: http.StatusBadRequest, + wantCode: "BadDigest", + }, + { + name: "valid sdk crc32", + configure: func(req *http.Request) { + req.Header.Del("Content-Md5") + req.Header.Set("X-Amz-Sdk-Checksum-Algorithm", "CRC32") + req.Header.Set("X-Amz-Checksum-Crc32", corsCRC32Base64(body)) + }, + want: http.StatusOK, + }, + { + name: "bad sdk crc32", + configure: func(req *http.Request) { + req.Header.Del("Content-Md5") + req.Header.Set("X-Amz-Sdk-Checksum-Algorithm", "CRC32") + req.Header.Set("X-Amz-Checksum-Crc32", corsCRC32Base64([]byte("different body"))) + }, + want: http.StatusBadRequest, + wantCode: "BadDigest", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req, err := newTestRequest(http.MethodPut, getBucketCorsURL("", bucketName), int64(len(body)), bytes.NewReader(body)) + if err != nil { + t.Fatal(err) + } + tt.configure(req) + if err = signRequestV4(req, creds.AccessKey, creds.SecretKey); err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + apiRouter.ServeHTTP(rec, req) + if rec.Code != tt.want || (tt.wantCode != "" && !bytes.Contains(rec.Body.Bytes(), []byte(""+tt.wantCode+""))) { + t.Fatalf("expected status %d and code %s, got %d: %s", tt.want, tt.wantCode, rec.Code, rec.Body.String()) + } + }) + } +} + +func corsCRC32Base64(data []byte) string { + var checksum [4]byte + binary.BigEndian.PutUint32(checksum[:], crc32.ChecksumIEEE(data)) + return base64.StdEncoding.EncodeToString(checksum[:]) +} diff --git a/cmd/bucket-cors-handlers.go b/cmd/bucket-cors-handlers.go index 26a3e22d1..09acc2f77 100644 --- a/cmd/bucket-cors-handlers.go +++ b/cmd/bucket-cors-handlers.go @@ -27,6 +27,7 @@ import ( humanize "github.com/dustin/go-humanize" "github.com/minio/madmin-go/v3" "github.com/minio/minio/internal/bucket/cors" + hashpkg "github.com/minio/minio/internal/hash" "github.com/minio/minio/internal/logger" "github.com/minio/mux" "github.com/minio/pkg/v3/policy" @@ -69,9 +70,9 @@ func (api objectAPIHandlers) PutBucketCorsHandler(w http.ResponseWriter, r *http return } - // PutBucketCors requires a Content-Md5 (or a supported trailing/full - // checksum). validateLengthAndChecksum wraps r.Body so the supplied - // digest is verified as the body is read below. + // PutBucketCors requires a Content-Md5 or a supported full-header + // checksum. validateLengthAndChecksum wraps r.Body so the supplied digest + // is verified as the body is read below. if !validateLengthAndChecksum(r) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrMissingContentMD5), r.URL) return @@ -79,6 +80,10 @@ func (api objectAPIHandlers) PutBucketCorsHandler(w http.ResponseWriter, r *http corsBytes, err := io.ReadAll(r.Body) if err != nil { + if errors.Is(err, hashpkg.ErrInvalidChecksum) { + writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrBadDigest), r.URL) + return + } writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) return } diff --git a/cmd/bucket-cors-middleware_test.go b/cmd/bucket-cors-middleware_test.go index 4cff41aca..be6d1ddda 100644 --- a/cmd/bucket-cors-middleware_test.go +++ b/cmd/bucket-cors-middleware_test.go @@ -40,6 +40,7 @@ func TestPerBucketCorsPreflight(t *testing.T) { req := httptest.NewRequest(http.MethodOptions, "/mybucket/obj", nil) req.Header.Set("Origin", "http://example.com") req.Header.Set("Access-Control-Request-Method", "GET") + req.Header.Set("Access-Control-Request-Headers", "X-Amz-Date") handled := applyBucketCors(rec, req, cfg) if !handled { @@ -48,12 +49,25 @@ func TestPerBucketCorsPreflight(t *testing.T) { if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "http://example.com" { t.Fatalf("allow-origin = %q", got) } - if rec.Code != http.StatusOK { - t.Fatalf("preflight status = %d", rec.Code) + if got := rec.Header().Get("Access-Control-Allow-Credentials"); got != "true" { + t.Fatalf("allow-credentials = %q", got) + } + if got := rec.Header().Get("Access-Control-Allow-Methods"); got != "GET, PUT" { + t.Fatalf("allow-methods = %q", got) + } + if got := rec.Header().Get("Access-Control-Allow-Headers"); got != "X-Amz-Date" { + t.Fatalf("allow-headers = %q", got) } if got := rec.Header().Get("Access-Control-Expose-Headers"); got != "ETag" { t.Fatalf("expose-headers = %q", got) } + if got := rec.Header().Get("Access-Control-Max-Age"); got != "3000" { + t.Fatalf("max-age = %q", got) + } + requireCorsVary(t, rec.Header()) + if rec.Code != http.StatusOK { + t.Fatalf("preflight status = %d", rec.Code) + } requireCorsOriginVary(t, rec.Header()) } @@ -92,20 +106,24 @@ func TestPerBucketCorsPreflightNoMatch(t *testing.T) { if rec.Code != http.StatusForbidden { t.Fatalf("expected 403 for disallowed origin, got %d", rec.Code) } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Fatalf("rejected preflight returned allow-origin %q", got) + } requireCorsVary(t, rec.Header()) } -func TestPerBucketCorsPreflightWildcardOrigin(t *testing.T) { - cfg := &cors.Config{CORSRules: []cors.Rule{{ - AllowedOrigins: []string{"*"}, - AllowedMethods: []string{"GET"}, - AllowedHeaders: []string{"*"}, - ExposeHeaders: []string{"ETag"}, - }}} +func TestPerBucketCorsPreflightWildcardOriginAndZeroMaxAge(t *testing.T) { + doc := `*GETHEAD*ETag0` + cfg, err := cors.ParseBucketCorsConfig(strings.NewReader(doc)) + if err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodOptions, "/mybucket/obj", nil) req.Header.Set("Origin", "https://app.example.com") - req.Header.Set("Access-Control-Request-Method", http.MethodGet) + req.Header.Set("Access-Control-Request-Method", "GET") + req.Header.Set("Access-Control-Request-Headers", "RANGE") if handled := applyBucketCors(rec, req, cfg); !handled { t.Fatal("expected preflight to be handled") @@ -116,12 +134,58 @@ func TestPerBucketCorsPreflightWildcardOrigin(t *testing.T) { if got := rec.Header().Get("Access-Control-Allow-Credentials"); got != "" { t.Fatalf("allow-credentials = %q", got) } + if got := rec.Header().Get("Access-Control-Allow-Methods"); got != "GET, HEAD" { + t.Fatalf("allow-methods = %q", got) + } + if got := rec.Header().Get("Access-Control-Allow-Headers"); got != "RANGE" { + t.Fatalf("allow-headers = %q", got) + } if got := rec.Header().Get("Access-Control-Expose-Headers"); got != "ETag" { t.Fatalf("expose-headers = %q", got) } + if got := rec.Header().Get("Access-Control-Max-Age"); got != "0" { + t.Fatalf("max-age = %q", got) + } requireCorsVary(t, rec.Header()) } +func TestPerBucketCorsPreflightUsesFirstFullyMatchingRule(t *testing.T) { + cfg := &cors.Config{CORSRules: []cors.Rule{ + { + AllowedOrigins: []string{"https://app.example.com"}, + AllowedMethods: []string{"GET"}, + AllowedHeaders: []string{"x-a"}, + ExposeHeaders: []string{"x-rule-a"}, + MaxAgeSeconds: 1, + }, + { + AllowedOrigins: []string{"https://app.example.com"}, + AllowedMethods: []string{"GET", "HEAD"}, + AllowedHeaders: []string{"*"}, + ExposeHeaders: []string{"x-rule-b"}, + MaxAgeSeconds: 2, + }, + }} + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodOptions, "/mybucket/obj", nil) + req.Header.Set("Origin", "https://app.example.com") + req.Header.Set("Access-Control-Request-Method", "GET") + req.Header.Set("Access-Control-Request-Headers", "X-B") + + if handled := applyBucketCors(rec, req, cfg); !handled { + t.Fatal("expected preflight to be handled") + } + if got := rec.Header().Get("Access-Control-Expose-Headers"); got != "x-rule-b" { + t.Fatalf("selected rule expose-headers = %q", got) + } + if got := rec.Header().Get("Access-Control-Allow-Methods"); got != "GET, HEAD" { + t.Fatalf("selected rule allow-methods = %q", got) + } + if got := rec.Header().Get("Access-Control-Max-Age"); got != "2" { + t.Fatalf("selected rule max-age = %q", got) + } +} + func TestPerBucketCorsActualRequest(t *testing.T) { cfg := &cors.Config{CORSRules: []cors.Rule{{ AllowedOrigins: []string{"*"}, @@ -237,6 +301,66 @@ func testBucketCorsNoConfigUsesGlobalFallback(_ ObjectLayer, _ string, bucket st } } +func TestPerBucketCorsActualPatternOriginSupportsCredentials(t *testing.T) { + cfg := &cors.Config{CORSRules: []cors.Rule{{ + AllowedOrigins: []string{"https://*.example.com"}, + AllowedMethods: []string{"GET"}, + }}} + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/mybucket/obj", nil) + req.Header.Set("Origin", "https://app.example.com") + + if handled := applyBucketCors(rec, req, cfg); handled { + t.Fatal("actual request must continue") + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://app.example.com" { + t.Fatalf("allow-origin = %q", got) + } + if got := rec.Header().Get("Access-Control-Allow-Credentials"); got != "true" { + t.Fatalf("allow-credentials = %q", got) + } +} + +func TestPerBucketCorsActualNullOriginSurvivesForwardingMiddleware(t *testing.T) { + next := setBucketForwardingMiddleware(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + })) + + t.Run("per-bucket null origin", func(t *testing.T) { + cfg := &cors.Config{CORSRules: []cors.Rule{{ + AllowedOrigins: []string{"null"}, + AllowedMethods: []string{"GET"}, + }}} + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/mybucket/obj", nil) + req.Header.Set("Origin", "null") + + if handled := applyBucketCors(rec, req, cfg); handled { + t.Fatal("actual request must continue") + } + next.ServeHTTP(rec, req) + + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "null" { + t.Fatalf("allow-origin = %q", got) + } + if got := rec.Header().Get("Access-Control-Allow-Credentials"); got != "true" { + t.Fatalf("allow-credentials = %q", got) + } + }) + + t.Run("legacy unmarked null origin", func(t *testing.T) { + rec := httptest.NewRecorder() + rec.Header().Set("Access-Control-Allow-Origin", "null") + req := httptest.NewRequest(http.MethodGet, "/mybucket/obj", nil) + + next.ServeHTTP(rec, req) + + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { + t.Fatalf("allow-origin = %q", got) + } + }) +} + func requireCorsVary(t *testing.T, header http.Header) { t.Helper() values := strings.Join(header.Values("Vary"), ",") diff --git a/cmd/bucket-cors-site-replication_test.go b/cmd/bucket-cors-site-replication_test.go index 3e3b5feb8..6654660b1 100644 --- a/cmd/bucket-cors-site-replication_test.go +++ b/cmd/bucket-cors-site-replication_test.go @@ -20,6 +20,7 @@ package cmd import ( "bytes" "encoding/base64" + "encoding/binary" "encoding/json" "errors" "net/http" @@ -750,6 +751,65 @@ func TestPeerBucketCorsCreatedAtFloor(t *testing.T) { }) } +func TestLegacyInvalidCorsMetadataCanBeDeleted(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testLegacyInvalidCorsMetadataCanBeDeleted, + endpoints: []string{"GetBucketCors"}, + }) +} + +func testLegacyInvalidCorsMetadataCanBeDeleted(obj ObjectLayer, _ string, bucket string, _ http.Handler, _ auth.Credentials, t *testing.T) { + ctx := t.Context() + meta, err := readBucketMetadata(ctx, obj, bucket) + if err != nil { + t.Fatal(err) + } + legacyAt := meta.Created.Add(time.Second) + meta.CorsConfigXML = []byte(`https://app.example.comget`) + meta.CorsConfigUpdatedAt = legacyAt + + data := make([]byte, 4, meta.Msgsize()+4) + binary.LittleEndian.PutUint16(data[0:2], bucketMetadataFormat) + binary.LittleEndian.PutUint16(data[2:4], bucketMetadataVersion) + data, err = meta.MarshalMsg(data) + if err != nil { + t.Fatal(err) + } + if err = saveConfig(ctx, obj, pathJoin(bucketMetaPrefix, bucket, bucketMetadataFile), data); err != nil { + t.Fatal(err) + } + + globalBucketMetadataSys.Remove(bucket) + loaded, err := globalBucketMetadataSys.GetConfigFromDisk(ctx, bucket) + if err != nil { + t.Fatalf("strict load made all bucket metadata unavailable: %v", err) + } + if loaded.corsConfigErr == nil || loaded.corsConfig != nil { + t.Fatalf("legacy CORS state = (%#v, %v), want fail-closed parse error", loaded.corsConfig, loaded.corsConfigErr) + } + globalBucketMetadataSys.Set(bucket, loaded) + if _, gotAt, err := globalBucketMetadataSys.GetCorsConfig(bucket); err == nil || !gotAt.Equal(legacyAt) { + t.Fatalf("GetCorsConfig = timestamp %v, error %v; want legacy timestamp and error", gotAt, err) + } + if _, gotAt, err := globalBucketMetadataSys.GetCorsConfigXML(bucket); err == nil || !gotAt.Equal(legacyAt) { + t.Fatalf("GetCorsConfigXML = timestamp %v, error %v; want legacy timestamp and error", gotAt, err) + } + + deleteAt, err := updateLocalBucketCORSMetadata(ctx, obj, bucket, nil) + if err != nil { + t.Fatalf("DELETE could not repair legacy invalid CORS: %v", err) + } + globalBucketMetadataSys.Remove(bucket) + repaired, err := globalBucketMetadataSys.GetConfigFromDisk(ctx, bucket) + if err != nil { + t.Fatal(err) + } + if repaired.corsConfigErr != nil || repaired.corsConfig != nil || len(repaired.CorsConfigXML) != 0 || !repaired.CorsConfigUpdatedAt.Equal(deleteAt) { + t.Fatalf("repaired CORS state = (%q, %#v, %v, %v), want tombstone at %v", repaired.CorsConfigXML, repaired.corsConfig, repaired.corsConfigErr, repaired.CorsConfigUpdatedAt, deleteAt) + } +} + func testPeerBucketCorsCreatedAtFloor(_ ObjectLayer, _ string, bucket string, _ http.Handler, _ auth.Credentials, t *testing.T) { ctx := t.Context() meta, err := globalBucketMetadataSys.Get(bucket) diff --git a/cmd/bucket-metadata-sys.go b/cmd/bucket-metadata-sys.go index cb442ca5e..ae2462115 100644 --- a/cmd/bucket-metadata-sys.go +++ b/cmd/bucket-metadata-sys.go @@ -370,6 +370,9 @@ func (sys *BucketMetadataSys) GetCorsConfig(bucket string) (*cors.Config, time.T if err != nil { return nil, time.Time{}, err } + if meta.corsConfigErr != nil { + return nil, meta.CorsConfigUpdatedAt, meta.corsConfigErr + } if meta.corsConfig == nil { return nil, time.Time{}, errConfigNotFound } @@ -384,6 +387,9 @@ func (sys *BucketMetadataSys) GetCorsConfigXML(bucket string) ([]byte, time.Time if err != nil { return nil, time.Time{}, err } + if meta.corsConfigErr != nil { + return nil, meta.CorsConfigUpdatedAt, meta.corsConfigErr + } if len(meta.CorsConfigXML) == 0 { return nil, time.Time{}, errConfigNotFound } diff --git a/cmd/bucket-metadata.go b/cmd/bucket-metadata.go index 556510b61..ba4c0eb5b 100644 --- a/cmd/bucket-metadata.go +++ b/cmd/bucket-metadata.go @@ -113,6 +113,7 @@ type BucketMetadata struct { bucketTargetConfig *madmin.BucketTargets bucketTargetConfigMeta map[string]string corsConfig *cors.Config + corsConfigErr error } // newBucketMetadata creates BucketMetadata with the supplied name and Created to Now. @@ -261,6 +262,12 @@ func loadBucketMetadataParse(ctx context.Context, objectAPI ObjectLayer, bucket return b, err } } + if b.corsConfigErr != nil { + // Keep the rest of the bucket metadata available so an operator can + // replace or delete a CORS document accepted by an older, more lenient + // build. Defer unrelated metadata migration until CORS is repaired. + return b, nil + } // migrate unencrypted remote targets if err = b.migrateTargetConfig(ctx, objectAPI); err != nil { @@ -320,10 +327,17 @@ func (b *BucketMetadata) parseAllConfigs(ctx context.Context, objectAPI ObjectLa b.taggingConfig = nil } + b.corsConfigErr = nil if len(b.CorsConfigXML) != 0 { - b.corsConfig, err = cors.ParseBucketCorsConfig(bytes.NewReader(b.CorsConfigXML)) - if err != nil { - return err + cfg, corsErr := cors.ParseBucketCorsConfig(bytes.NewReader(b.CorsConfigXML)) + if corsErr == nil { + corsErr = cfg.Validate() + } + if corsErr != nil { + b.corsConfig = nil + b.corsConfigErr = fmt.Errorf("invalid bucket CORS configuration: %w", corsErr) + } else { + b.corsConfig = cfg } } else { b.corsConfig = nil @@ -522,6 +536,9 @@ func (b *BucketMetadata) Save(ctx context.Context, api ObjectLayer) error { if err := b.parseAllConfigs(ctx, api); err != nil { return err } + if b.corsConfigErr != nil { + return b.corsConfigErr + } data := make([]byte, 4, b.Msgsize()+4) diff --git a/cmd/generic-handlers.go b/cmd/generic-handlers.go index 88a111668..40f2b3f14 100644 --- a/cmd/generic-handlers.go +++ b/cmd/generic-handlers.go @@ -476,9 +476,10 @@ func setRequestValidityMiddleware(h http.Handler) http.Handler { // is obtained from centralized etcd configuration service. func setBucketForwardingMiddleware(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if origin := w.Header().Get("Access-Control-Allow-Origin"); origin == "null" { + if origin := w.Header().Get("Access-Control-Allow-Origin"); origin == "null" && !bucketCorsWasApplied(r) { // This is a workaround change to ensure that "Origin: null" - // incoming request to a response back as "*" instead of "null" + // incoming request to a response back as "*" instead of "null". + // Per-bucket CORS preserves an explicitly allowed "null" origin. w.Header().Set("Access-Control-Allow-Origin", "*") } if globalDNSConfig == nil || !globalBucketFederation || diff --git a/docs/site-replication/CORS-LWW-DESIGN.md b/docs/site-replication/CORS-LWW-DESIGN.md index 0db351c6f..f80c3337c 100644 --- a/docs/site-replication/CORS-LWW-DESIGN.md +++ b/docs/site-replication/CORS-LWW-DESIGN.md @@ -6,9 +6,9 @@ - Baseline: `e4e3007da6d7d1198a6a050e34f84566d40a9654` - Working branch: `codex/issue-75-cors-hardening` - Decision: CORS-specific deterministic last-writer-wins register, described below -- Implementation state: implemented and locally verified; uncommitted and unpushed -- Release state: not released; remote CI, merge, tag, package, and image gates remain separate -- Final design/implementation review: Claude Code Opus 5 Max found no P0/P1 and judged the implementation GO; its mandatory documentation corrections are incorporated here +- Implementation state: B2 is commit `724f8703d` in PR #80; the B3 strict-wire integration is resolved locally and awaits final combined verification before submission +- Release state: PR #80 remains open; nothing is merged, tagged, packaged, published as an image, or deployed +- Final design/implementation review: the B2 implementation was GO; the combined B2+B3 Opus 5 Max review found one test-build conflict and one legacy-metadata load risk, both corrected before combined testing This document defines the replication state, ordering, persistence, status, healing, concurrency, compatibility, and test contract for per-bucket CORS. @@ -51,16 +51,21 @@ The following are deliberately out of scope: ### Adjacent issue-75 changes in the same candidate -The current dirty issue-75 candidate also contains CORS work outside the LWW -register itself: +The final issue-75 candidate also contains CORS work outside the LWW register +itself: -- stricter `cors.Config.Validate()` rules for empty origins and unsupported - wildcard forms; -- matcher signature and response-selection changes needed to distinguish a - literal `*` origin from a patterned match; +- a strict, namespace-tolerant XML wire parser that rejects trailing roots, + unknown/nested elements, duplicate singleton fields, invalid integer shape, + and non-whitespace character data; +- Unicode code-point ID counting, exact uppercase S3 methods, non-empty header + elements, and int32-compatible MaxAge validation; +- a single-`*` matcher and response-selection changes needed to distinguish a + literal `*` origin from a patterned or explicit `null` match; - fail-closed metadata-error handling in the HTTP middleware; -- preflight expose headers and complete `Vary` behavior; and -- HTTP protocol negative tests. +- complete allowed-method, explicit MaxAge=0, expose-header, credentials, and + `Vary` preflight behavior; +- checksum mismatch classification as `BadDigest`; and +- parser, signed-handler, browser-response, and protocol adversarial tests. Those changes share the same CORS release gate and are present in the reviewed diff, but they are not part of the replication conflict key or join algorithm. @@ -153,6 +158,13 @@ base64 string equality remaining safe for the shared metadata helper. An invalid wire value is not a candidate winner and is never propagated. Peer apply rejects it before any metadata write. +A bucket may nevertheless contain a CORS document written by a pre-release, +more lenient build. Loading such metadata keeps policy, lifecycle, versioning, +and the other bucket fields available, but stashes the CORS parse/validation +error and exposes no active CORS config. CORS GET and middleware lookup return +that error, so browser handling fails closed. A valid PUT or DELETE can repair +the record; any attempt to save a newly invalid CORS document remains rejected. + ## Deterministic Ordering States use the following total order: @@ -381,6 +393,9 @@ After cache removal or process restart: - a live state restores the same parsed rules and source timestamp; - a tombstone restores nil payload plus its non-zero timestamp; - a baseline remains nil plus zero timestamp. +- a legacy-invalid raw document leaves the non-CORS bucket metadata readable, + disables per-bucket CORS fail-closed, and remains repairable through a valid + CORS PUT or DELETE. ## Error Handling @@ -391,6 +406,7 @@ After cache removal or process restart: | empty non-nil payload | reject | | malformed XML | reject before saving | | semantically invalid CORS rules | reject before saving | +| legacy-invalid CORS already on disk | load other metadata, return a CORS-specific error, and permit CORS replacement or deletion | | event before bucket `CreatedAt` | ignore and log once per bucket | | missing bucket metadata | return an error; do not create metadata implicitly | | exact duplicate or lower state | successful no-op | @@ -453,6 +469,8 @@ The implementation is acceptable only while all of these invariants hold: 10. Same-payload/newer-timestamp heal advances the older barrier. 11. Initial sync and retry preserve tombstones and source timestamps. 12. Disk reload and cache reload preserve state kind, payload, and timestamp. +13. A legacy-invalid CORS document cannot activate global fallback, hide other + bucket metadata, or prevent a valid CORS PUT/DELETE repair. ## Test Contract @@ -462,6 +480,7 @@ The required test matrix is: | --- | --- | | Wire | canonical base64 accepted; case-different decoded bytes differ; malformed and non-canonical base64 rejected | | Validation | invalid XML and semantically invalid origin/method/rule rejected without mutation | +| Strict wire | standard S3 namespace accepted; trailing root, unknown/nested elements, duplicate singleton fields, lowercase methods, byte-counted Unicode IDs, and invalid MaxAge rejected | | Ordering | older event ignored; newer event applied; duplicate no-op; equal live/live order-independent; equal PUT/DELETE chooses tombstone | | Barrier | same payload with newer timestamp is persisted and healed | | Tombstone | delayed PUT cannot resurrect; missed DELETE wins heal; repeated DELETE is idempotent | @@ -472,11 +491,12 @@ The required test matrix is: | Initial sync | baseline omitted; live and tombstone emitted with exact source timestamp | | Lineage | pre-creation event ignored; post-creation event applied | | Restart | cache removal/disk reload preserves tombstone or live state and status timestamp | +| Legacy repair | a lenient historical document loads fail-closed without hiding other metadata and can be deleted or replaced | | Full seam | signed admin dispatch -> peer apply -> real status collection -> local heal -> cache reload -> remote heal dispatch | ## Local Verification Record -The current uncommitted implementation has passed: +The committed B2 implementation passed: - the supplied adversarial base64 and same-payload/newer-timestamp tests; - focused CORS normal tests; @@ -489,6 +509,12 @@ The current uncommitted implementation has passed: - gofmt and `git diff --check`; - a signed admin dispatch -> apply -> status -> heal -> cache reload test. +After integrating B3 and resolving overlap, focused strict-parser, +validation, middleware, replication, namespace, and legacy-repair tests also +pass. Full combined normal/race, client, compatibility, and release-gate runs +are intentionally scheduled only after the code and documentation solution is +fully frozen. + The repository `make lint` bootstrap could not download its private copy of golangci-lint because the network returned HTTP status 000. The same exact v2.13.1 binary already installed locally was used with the Makefile's build @@ -496,7 +522,7 @@ tags, timeout, and configuration and reported zero issues. ## Independent Review Record -Three read-only local Claude Code reviews used canonical model +Four read-only local Claude Code reviews used canonical model `claude-opus-5` at `max` effort. The first review rejected the pre-fix candidate and identified the unsafe @@ -520,6 +546,15 @@ not selected and retransmitted. The hardening and all mandatory documentation corrections are incorporated in the current tree. The final selected solution is therefore the C-prime register and invariants recorded in this document. +The fourth review examined the resolved B2+B3 combination. It confirmed that +the C-prime register, strict wire parser, MaxAge presence, wildcard credentials, +Origin-null marker, rejected-preflight `Vary`, checksum classification, and +peer validation can coexist. It found a conflict-resolution test helper typo +and the risk that strict parsing could make all bucket metadata unavailable for +a document accepted by a lenient development build. The helper was corrected; +metadata loading now stashes a CORS-specific error, fails browser behavior +closed, rejects new invalid saves, and allows a valid CORS PUT/DELETE repair. + ## Release Gates An implementation-level GO means only that the local CORS state machine and diff --git a/internal/bucket/cors/cors.go b/internal/bucket/cors/cors.go index 4776d6eff..f36432144 100644 --- a/internal/bucket/cors/cors.go +++ b/internal/bucket/cors/cors.go @@ -22,10 +22,11 @@ package cors import ( "encoding/xml" "errors" + "fmt" "io" + "strconv" "strings" - - "github.com/minio/pkg/v3/wildcard" + "unicode/utf8" ) // maxCORSRules is the maximum number of rules allowed per bucket (AWS S3 limit). @@ -34,6 +35,10 @@ const maxCORSRules = 100 // maxCORSRuleIDLen is the maximum length of a CORSRule (AWS S3 limit). const maxCORSRuleIDLen = 255 +// maxCORSMaxAgeSeconds is the largest value representable by the int32 +// MaxAgeSeconds shape used by the S3 API model. +const maxCORSMaxAgeSeconds = 1<<31 - 1 + // supportedMethods are the HTTP methods permitted in an AllowedMethod element. var supportedMethods = map[string]bool{ "GET": true, @@ -57,17 +62,153 @@ type Rule struct { AllowedOrigins []string `xml:"AllowedOrigin"` ExposeHeaders []string `xml:"ExposeHeader"` MaxAgeSeconds int `xml:"MaxAgeSeconds"` + + maxAgeSecondsSet bool +} + +type corsXMLUnknown struct { + XMLName xml.Name +} + +type corsXMLValue struct { + Text string `xml:",chardata"` + Unknown []corsXMLUnknown `xml:",any"` +} + +type configXML struct { + XMLName xml.Name `xml:"CORSConfiguration"` + CORSRules []ruleXML `xml:"CORSRule"` + Text string `xml:",chardata"` + Unknown []corsXMLUnknown `xml:",any"` +} + +type ruleXML struct { + ID []corsXMLValue `xml:"ID"` + AllowedHeaders []corsXMLValue `xml:"AllowedHeader"` + AllowedMethods []corsXMLValue `xml:"AllowedMethod"` + AllowedOrigins []corsXMLValue `xml:"AllowedOrigin"` + ExposeHeaders []corsXMLValue `xml:"ExposeHeader"` + MaxAgeSeconds []corsXMLValue `xml:"MaxAgeSeconds"` + Text string `xml:",chardata"` + Unknown []corsXMLUnknown `xml:",any"` } // ParseBucketCorsConfig parses a CORS configuration from the given reader. func ParseBucketCorsConfig(r io.Reader) (*Config, error) { - var c Config - if err := xml.NewDecoder(r).Decode(&c); err != nil { + var parsed configXML + decoder := xml.NewDecoder(r) + if err := decoder.Decode(&parsed); err != nil { return nil, err } + if strings.TrimSpace(parsed.Text) != "" { + return nil, xml.UnmarshalError("unexpected character data in CORSConfiguration") + } + if len(parsed.Unknown) > 0 { + return nil, xml.UnmarshalError(fmt.Sprintf("unexpected element <%s> in CORSConfiguration", parsed.Unknown[0].XMLName.Local)) + } + + c := Config{ + XMLName: parsed.XMLName, + CORSRules: make([]Rule, len(parsed.CORSRules)), + } + for i := range parsed.CORSRules { + rule, err := parseCORSRuleXML(parsed.CORSRules[i]) + if err != nil { + return nil, err + } + c.CORSRules[i] = rule + } + + // Decode consumes one document element. Only XML whitespace, comments, and + // processing instructions are permitted after it. + for { + token, err := decoder.Token() + if err == io.EOF { + break + } + if err != nil { + return nil, err + } + switch token := token.(type) { + case xml.CharData: + if strings.TrimSpace(string(token)) == "" { + continue + } + case xml.Comment, xml.ProcInst: + continue + } + return nil, errors.New("unexpected XML content after CORSConfiguration") + } return &c, nil } +func parseCORSRuleXML(parsed ruleXML) (Rule, error) { + if strings.TrimSpace(parsed.Text) != "" { + return Rule{}, xml.UnmarshalError("unexpected character data in CORSRule") + } + if len(parsed.Unknown) > 0 { + return Rule{}, xml.UnmarshalError(fmt.Sprintf("unexpected element <%s> in CORSRule", parsed.Unknown[0].XMLName.Local)) + } + if len(parsed.ID) > 1 { + return Rule{}, xml.UnmarshalError("duplicate ID element in CORSRule") + } + if len(parsed.MaxAgeSeconds) > 1 { + return Rule{}, xml.UnmarshalError("duplicate MaxAgeSeconds element in CORSRule") + } + + rule := Rule{} + var err error + if len(parsed.ID) == 1 { + if rule.ID, err = corsXMLText("ID", parsed.ID[0]); err != nil { + return Rule{}, err + } + } + if rule.AllowedHeaders, err = corsXMLTexts("AllowedHeader", parsed.AllowedHeaders); err != nil { + return Rule{}, err + } + if rule.AllowedMethods, err = corsXMLTexts("AllowedMethod", parsed.AllowedMethods); err != nil { + return Rule{}, err + } + if rule.AllowedOrigins, err = corsXMLTexts("AllowedOrigin", parsed.AllowedOrigins); err != nil { + return Rule{}, err + } + if rule.ExposeHeaders, err = corsXMLTexts("ExposeHeader", parsed.ExposeHeaders); err != nil { + return Rule{}, err + } + if len(parsed.MaxAgeSeconds) == 1 { + value, valueErr := corsXMLText("MaxAgeSeconds", parsed.MaxAgeSeconds[0]) + if valueErr != nil { + return Rule{}, valueErr + } + age, parseErr := strconv.ParseInt(strings.TrimSpace(value), 10, 32) + if parseErr != nil { + return Rule{}, xml.UnmarshalError("invalid MaxAgeSeconds value") + } + rule.MaxAgeSeconds = int(age) + rule.maxAgeSecondsSet = true + } + return rule, nil +} + +func corsXMLTexts(name string, values []corsXMLValue) ([]string, error) { + result := make([]string, len(values)) + for i := range values { + value, err := corsXMLText(name, values[i]) + if err != nil { + return nil, err + } + result[i] = value + } + return result, nil +} + +func corsXMLText(name string, value corsXMLValue) (string, error) { + if len(value.Unknown) > 0 { + return "", xml.UnmarshalError(fmt.Sprintf("element <%s> must not contain child element <%s>", name, value.Unknown[0].XMLName.Local)) + } + return value.Text, nil +} + // Validate checks the config against the S3 constraints. func (c *Config) Validate() error { if len(c.CORSRules) == 0 { @@ -77,7 +218,10 @@ func (c *Config) Validate() error { return errors.New("CORSConfiguration exceeds the maximum number of rules") } for _, r := range c.CORSRules { - if len(r.ID) > maxCORSRuleIDLen { + if !utf8.ValidString(r.ID) { + return errors.New("CORSRule ID must contain valid UTF-8") + } + if utf8.RuneCountInString(r.ID) > maxCORSRuleIDLen { return errors.New("CORSRule ID exceeds the maximum length of 255 characters") } if len(r.AllowedOrigins) == 0 { @@ -98,11 +242,14 @@ func (c *Config) Validate() error { } } for _, m := range r.AllowedMethods { - if !supportedMethods[strings.ToUpper(m)] { + if !supportedMethods[m] { return errors.New("unsupported method in CORSRule: " + m) } } for _, h := range r.AllowedHeaders { + if h == "" { + return errors.New("AllowedHeader must not be empty") + } if strings.Contains(h, "?") { return errors.New("AllowedHeader may not contain wildcard '?': " + h) } @@ -110,17 +257,34 @@ func (c *Config) Validate() error { return errors.New("AllowedHeader may contain at most one wildcard '*': " + h) } } + for _, h := range r.ExposeHeaders { + if h == "" { + return errors.New("ExposeHeader must not be empty") + } + } if r.MaxAgeSeconds < 0 { return errors.New("MaxAgeSeconds must not be negative") } + if int64(r.MaxAgeSeconds) > maxCORSMaxAgeSeconds { + return errors.New("MaxAgeSeconds exceeds the maximum S3 integer value") + } } return nil } +func matchSingleWildcard(pattern, value string) bool { + prefix, suffix, found := strings.Cut(pattern, "*") + if !found { + return pattern == value + } + return len(value) >= len(prefix)+len(suffix) && + strings.HasPrefix(value, prefix) && strings.HasSuffix(value, suffix) +} + func (r Rule) matchAllowedOrigin(origin string) (string, bool) { - for _, o := range r.AllowedOrigins { - if o == "*" || wildcard.MatchSimple(o, origin) { - return o, true + for _, allowedOrigin := range r.AllowedOrigins { + if matchSingleWildcard(allowedOrigin, origin) { + return allowedOrigin, true } } return "", false @@ -135,7 +299,7 @@ func (r Rule) HasAllowedOrigin(origin string) bool { // HasAllowedMethod reports whether the rule allows the given HTTP method. func (r Rule) HasAllowedMethod(method string) bool { for _, m := range r.AllowedMethods { - if strings.EqualFold(m, method) { + if m == method { return true } } @@ -161,7 +325,7 @@ func (r Rule) FilterAllowedHeaders(reqHeaders []string) ([]string, bool) { func (r Rule) headerAllowed(header string) bool { for _, h := range r.AllowedHeaders { - if h == "*" || wildcard.MatchSimple(strings.ToLower(h), strings.ToLower(header)) { + if matchSingleWildcard(strings.ToLower(h), strings.ToLower(header)) { return true } } @@ -186,7 +350,7 @@ func (c *Config) MatchRule(origin, method string) (rule *Rule, allowedOrigin str // this keeps evaluating subsequent rules until one fully satisfies the // preflight request, since an earlier origin/method match with a more // restrictive header list must not shadow a later, more permissive rule. -func (c *Config) MatchPreflight(origin, method string, reqHeaders []string) (rule *Rule, allowedOrigin string, allowedHeaders []string, ok bool) { +func (c *Config) MatchPreflight(origin, method string, reqHeaders []string) (rule *Rule, allowedOrigin string, allowedHeaders []string, maxAgeSeconds *int, ok bool) { for i := range c.CORSRules { r := &c.CORSRules[i] matchedOrigin, originOK := r.matchAllowedOrigin(origin) @@ -197,7 +361,10 @@ func (c *Config) MatchPreflight(origin, method string, reqHeaders []string) (rul if !headersOK { continue } - return r, matchedOrigin, allowed, true + if r.maxAgeSecondsSet || r.MaxAgeSeconds != 0 { + maxAgeSeconds = &r.MaxAgeSeconds + } + return r, matchedOrigin, allowed, maxAgeSeconds, true } - return nil, "", nil, false + return nil, "", nil, nil, false } diff --git a/internal/bucket/cors/cors_adversarial_test.go b/internal/bucket/cors/cors_adversarial_test.go new file mode 100644 index 000000000..794c298fe --- /dev/null +++ b/internal/bucket/cors/cors_adversarial_test.go @@ -0,0 +1,222 @@ +// Copyright (c) 2015-2021 MinIO, Inc. +// +// This file is part of MinIO Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +package cors + +import ( + "strconv" + "strings" + "testing" +) + +func TestParseStandardS3Namespace(t *testing.T) { + doc := `https://app.example.comGET` + cfg, err := ParseBucketCorsConfig(strings.NewReader(doc)) + if err != nil { + t.Fatal(err) + } + if err = cfg.Validate(); err != nil { + t.Fatal(err) + } + if _, _, ok := cfg.MatchRule("https://app.example.com", "GET"); !ok { + t.Fatal("standard S3 namespace document did not produce a matching rule") + } +} + +const minimalCORSConfig = `*GET` + +func TestParseRejectsTrailingXMLRoot(t *testing.T) { + for name, suffix := range map[string]string{ + "second root": ``, + "text": `junk`, + "dangling close": ``, + } { + t.Run(name, func(t *testing.T) { + if _, err := ParseBucketCorsConfig(strings.NewReader(minimalCORSConfig + suffix)); err == nil { + t.Fatalf("expected trailing %s to be rejected", name) + } + }) + } +} + +func TestParseAllowsXMLMiscAfterRoot(t *testing.T) { + for name, suffix := range map[string]string{ + "whitespace": " \n\t", + "comment": ``, + "processing instruction": ``, + } { + t.Run(name, func(t *testing.T) { + if _, err := ParseBucketCorsConfig(strings.NewReader(minimalCORSConfig + suffix)); err != nil { + t.Fatalf("valid trailing XML misc was rejected: %v", err) + } + }) + } +} + +func TestValidateCORSRuleIDCountsCharacters(t *testing.T) { + doc := `` + strings.Repeat("界", 255) + `*GET` + cfg, err := ParseBucketCorsConfig(strings.NewReader(doc)) + if err != nil { + t.Fatalf("parse failed: %v", err) + } + if err = cfg.Validate(); err != nil { + t.Fatalf("255-character rule ID must be accepted: %v", err) + } + + cfg.CORSRules[0].ID += "界" + if err = cfg.Validate(); err == nil { + t.Fatal("256-character rule ID must be rejected") + } +} + +func TestValidateRejectsNonCanonicalAllowedMethod(t *testing.T) { + doc := `*get` + cfg, err := ParseBucketCorsConfig(strings.NewReader(doc)) + if err != nil { + t.Fatalf("parse failed: %v", err) + } + if err = cfg.Validate(); err == nil { + t.Fatal("expected lowercase AllowedMethod to be rejected") + } +} + +func TestAllowedMethodMatchingIsCaseSensitive(t *testing.T) { + rule := Rule{AllowedMethods: []string{"GET"}} + if !rule.HasAllowedMethod("GET") { + t.Fatal("expected canonical GET to match") + } + if rule.HasAllowedMethod("get") { + t.Fatal("lowercase request method must not match canonical GET") + } +} + +func TestParseRejectsElementsOutsideCORSShape(t *testing.T) { + tests := map[string]string{ + "unknown root child": `*GET`, + "unknown rule child": `*GET`, + "nested origin child": `GET`, + "duplicate id": `ab*GET`, + "duplicate max age": `*GET12`, + "empty max age": `*GET`, + "overflow max age": `*GET2147483648`, + } + + for name, doc := range tests { + t.Run(name, func(t *testing.T) { + if _, err := ParseBucketCorsConfig(strings.NewReader(doc)); err == nil { + t.Fatal("expected parse error") + } + }) + } +} + +func TestMaxAgeSecondsPresence(t *testing.T) { + tests := []struct { + name string + element string + value int + present bool + }{ + {name: "absent"}, + {name: "zero", element: `0`, present: true}, + {name: "positive", element: `3000`, value: 3000, present: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + doc := `*GET` + tt.element + `` + cfg, err := ParseBucketCorsConfig(strings.NewReader(doc)) + if err != nil { + t.Fatalf("parse failed: %v", err) + } + rule := cfg.CORSRules[0] + _, _, _, maxAgeSeconds, ok := cfg.MatchPreflight("https://example.com", "GET", nil) + if !ok { + t.Fatal("expected rule to match") + } + present := maxAgeSeconds != nil + if rule.MaxAgeSeconds != tt.value || present != tt.present { + t.Fatalf("MaxAgeSeconds = %d, present = %v", rule.MaxAgeSeconds, present) + } + }) + } +} + +func TestValidateRuleCountBoundary(t *testing.T) { + rule := Rule{AllowedOrigins: []string{"*"}, AllowedMethods: []string{"GET"}} + cfg := Config{CORSRules: make([]Rule, 100)} + for i := range cfg.CORSRules { + cfg.CORSRules[i] = rule + } + if err := cfg.Validate(); err != nil { + t.Fatalf("100 rules must be accepted: %v", err) + } + cfg.CORSRules = append(cfg.CORSRules, rule) + if err := cfg.Validate(); err == nil { + t.Fatal("101 rules must be rejected") + } +} + +func TestValidateMaxAgeSecondsBoundary(t *testing.T) { + cfg := Config{CORSRules: []Rule{{ + AllowedOrigins: []string{"*"}, + AllowedMethods: []string{"GET"}, + MaxAgeSeconds: maxCORSMaxAgeSeconds, + }}} + if err := cfg.Validate(); err != nil { + t.Fatalf("MaxAgeSeconds int32 maximum must be accepted: %v", err) + } + if strconv.IntSize > 32 { + overflow := int64(maxCORSMaxAgeSeconds) + 1 + cfg.CORSRules[0].MaxAgeSeconds = int(overflow) + if err := cfg.Validate(); err == nil { + t.Fatal("MaxAgeSeconds above int32 maximum must be rejected") + } + } +} + +func TestSingleWildcardMatching(t *testing.T) { + tests := []struct { + pattern string + value string + want bool + }{ + {"*", "https://example.com", true}, + {"https://*.example.com", "https://api.example.com", true}, + {"https://*.example.com", "https://.example.com", true}, + {"https://*.example.com", "http://api.example.com", false}, + {"https://?.example.com", "https://a.example.com", false}, + } + for _, tt := range tests { + if got := matchSingleWildcard(tt.pattern, tt.value); got != tt.want { + t.Errorf("matchSingleWildcard(%q, %q) = %v, want %v", tt.pattern, tt.value, got, tt.want) + } + } +} + +func TestMatchRuleReturnsMatchedOriginPattern(t *testing.T) { + cfg := Config{CORSRules: []Rule{{ + AllowedOrigins: []string{"https://app.example.com", "https://*", "*"}, + AllowedMethods: []string{"GET"}, + }}} + tests := []struct { + origin string + want string + }{ + {"https://app.example.com", "https://app.example.com"}, + {"https://other.example.com", "https://*"}, + {"http://other.example.com", "*"}, + } + for _, tt := range tests { + _, got, ok := cfg.MatchRule(tt.origin, "GET") + if !ok || got != tt.want { + t.Errorf("origin %q matched %q, ok=%v; want %q", tt.origin, got, ok, tt.want) + } + } +} diff --git a/internal/bucket/cors/cors_test.go b/internal/bucket/cors/cors_test.go index f9924c822..dbf56f5b1 100644 --- a/internal/bucket/cors/cors_test.go +++ b/internal/bucket/cors/cors_test.go @@ -62,6 +62,8 @@ func TestValidateRejections(t *testing.T) { "multi wildcard header": `*GETx-*-*`, "question mark origin": `https://?.example.comGET`, "question mark header": `*GETx-amz-?`, + "empty allowed header": `*GET`, + "empty expose header": `*GET`, "overlong id": `` + strings.Repeat("a", 256) + `*GET`, } for name, doc := range cases { @@ -121,7 +123,7 @@ func TestMatchPreflightFallsThroughToLaterRule(t *testing.T) { t.Fatalf("parse failed: %v", err) } - rule, _, allowed, ok := c.MatchPreflight("https://app.example.com", "GET", []string{"x-custom-header"}) + rule, _, allowed, _, ok := c.MatchPreflight("https://app.example.com", "GET", []string{"x-custom-header"}) if !ok { t.Fatal("expected MatchPreflight to succeed via the later, permissive rule") } @@ -155,3 +157,14 @@ func TestMatchAllowedOriginReturnsFirstMatchingPattern(t *testing.T) { } } } + +func TestFilterAllowedHeadersPreservesRequestedNames(t *testing.T) { + rule := Rule{AllowedHeaders: []string{"x-amz-*"}} + allowed, ok := rule.FilterAllowedHeaders([]string{"X-Amz-Date", " X-AMZ-Meta-Test "}) + if !ok { + t.Fatal("expected both request headers to match") + } + if got := strings.Join(allowed, ","); got != "X-Amz-Date,X-AMZ-Meta-Test" { + t.Fatalf("allowed headers = %q", got) + } +}