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 <rh@vonng.com>
This commit is contained in:
Feng Ruohang
2026-08-29 09:10:58 +08:00
parent 724f8703d8
commit 0eebc928f7
12 changed files with 946 additions and 51 deletions
+181 -14
View File
@@ -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 <ID> (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
}
@@ -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 := `<CORSConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><CORSRule><AllowedOrigin>https://app.example.com</AllowedOrigin><AllowedMethod>GET</AllowedMethod></CORSRule></CORSConfiguration>`
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 = `<CORSConfiguration><CORSRule><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod></CORSRule></CORSConfiguration>`
func TestParseRejectsTrailingXMLRoot(t *testing.T) {
for name, suffix := range map[string]string{
"second root": `<Extra/>`,
"text": `junk`,
"dangling close": `</Extra>`,
} {
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": `<!-- trailing comment -->`,
"processing instruction": `<?cors-test done?>`,
} {
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 := `<CORSConfiguration><CORSRule><ID>` + strings.Repeat("界", 255) + `</ID><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod></CORSRule></CORSConfiguration>`
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 := `<CORSConfiguration><CORSRule><AllowedOrigin>*</AllowedOrigin><AllowedMethod>get</AllowedMethod></CORSRule></CORSConfiguration>`
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": `<CORSConfiguration><Unknown/><CORSRule><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod></CORSRule></CORSConfiguration>`,
"unknown rule child": `<CORSConfiguration><CORSRule><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod><Unknown/></CORSRule></CORSConfiguration>`,
"nested origin child": `<CORSConfiguration><CORSRule><AllowedOrigin><Unknown/></AllowedOrigin><AllowedMethod>GET</AllowedMethod></CORSRule></CORSConfiguration>`,
"duplicate id": `<CORSConfiguration><CORSRule><ID>a</ID><ID>b</ID><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod></CORSRule></CORSConfiguration>`,
"duplicate max age": `<CORSConfiguration><CORSRule><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod><MaxAgeSeconds>1</MaxAgeSeconds><MaxAgeSeconds>2</MaxAgeSeconds></CORSRule></CORSConfiguration>`,
"empty max age": `<CORSConfiguration><CORSRule><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod><MaxAgeSeconds/></CORSRule></CORSConfiguration>`,
"overflow max age": `<CORSConfiguration><CORSRule><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod><MaxAgeSeconds>2147483648</MaxAgeSeconds></CORSRule></CORSConfiguration>`,
}
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: `<MaxAgeSeconds>0</MaxAgeSeconds>`, present: true},
{name: "positive", element: `<MaxAgeSeconds>3000</MaxAgeSeconds>`, value: 3000, present: true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
doc := `<CORSConfiguration><CORSRule><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod>` + tt.element + `</CORSRule></CORSConfiguration>`
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)
}
}
}
+14 -1
View File
@@ -62,6 +62,8 @@ func TestValidateRejections(t *testing.T) {
"multi wildcard header": `<CORSConfiguration><CORSRule><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod><AllowedHeader>x-*-*</AllowedHeader></CORSRule></CORSConfiguration>`,
"question mark origin": `<CORSConfiguration><CORSRule><AllowedOrigin>https://?.example.com</AllowedOrigin><AllowedMethod>GET</AllowedMethod></CORSRule></CORSConfiguration>`,
"question mark header": `<CORSConfiguration><CORSRule><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod><AllowedHeader>x-amz-?</AllowedHeader></CORSRule></CORSConfiguration>`,
"empty allowed header": `<CORSConfiguration><CORSRule><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod><AllowedHeader/></CORSRule></CORSConfiguration>`,
"empty expose header": `<CORSConfiguration><CORSRule><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod><ExposeHeader/></CORSRule></CORSConfiguration>`,
"overlong id": `<CORSConfiguration><CORSRule><ID>` + strings.Repeat("a", 256) + `</ID><AllowedOrigin>*</AllowedOrigin><AllowedMethod>GET</AllowedMethod></CORSRule></CORSConfiguration>`,
}
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)
}
}