Merge pull request #1885 from versity/sis/bucket-cors-rules-validation

fix: fixes PutBucketCors CORSRules validation
This commit is contained in:
Ben McClelland
2026-02-24 08:48:01 -08:00
committed by GitHub
8 changed files with 186 additions and 43 deletions
+45 -10
View File
@@ -28,8 +28,11 @@ import (
// headerRegex is the regexp to validate http header names
var headerRegex = regexp.MustCompile(`^[!#$%&'*+\-.^_` + "`" + `|~0-9A-Za-z]+$`)
type CORSHeader string
type CORSHTTPMethod string
type (
CORSHeader string
CORSHTTPMethod string
CORSOrigin string
)
// IsValid validates the CORS http header
// the rules are based on http RFC
@@ -53,7 +56,7 @@ func (ch CORSHeader) ToLower() string {
// IsValid validates the cors http request method:
// the methods are case sensitive
func (cm CORSHTTPMethod) IsValid() bool {
return cm.IsEmpty() || cm == http.MethodGet || cm == http.MethodHead || cm == http.MethodPut ||
return cm == http.MethodGet || cm == http.MethodHead || cm == http.MethodPut ||
cm == http.MethodPost || cm == http.MethodDelete
}
@@ -67,6 +70,21 @@ func (cm CORSHTTPMethod) String() string {
return string(cm)
}
// String converts the origin value to 'string'
func (co CORSOrigin) String() string {
return string(co)
}
// Validate validates the cors allowed origin
func (co CORSOrigin) Validate() error {
// make sure no double wildcard present
if strings.Count(co.String(), "*") > 1 {
return s3err.GetMultipleWildcardCORSOriginErr(co.String())
}
return nil
}
type CORSConfiguration struct {
Rules []CORSRule `xml:"CORSRule"`
}
@@ -140,20 +158,30 @@ type CORSRule struct {
AllowedMethods []CORSHTTPMethod `xml:"AllowedMethod"`
AllowedHeaders []CORSHeader `xml:"AllowedHeader"`
ExposeHeaders []CORSHeader `xml:"ExposeHeader"`
AllowedOrigins []string `xml:"AllowedOrigin"`
AllowedOrigins []CORSOrigin `xml:"AllowedOrigin"`
ID *string
MaxAgeSeconds *int32
}
// Validate validates and returns error if CORS configuration has invalid rule
func (cr *CORSRule) Validate() error {
// validate CORS allowed headers
for _, header := range cr.AllowedHeaders {
if !header.IsValid() {
debuglogger.Logf("invalid CORS allowed header: %s", header)
return s3err.GetInvalidCORSHeaderErr(header.String())
// AllowedOrigins can't be empty
if len(cr.AllowedOrigins) == 0 {
debuglogger.Logf("empty CORS allowed origins")
return s3err.GetAPIError(s3err.ErrMalformedXML)
}
// validate CORS allowed origins
for _, origin := range cr.AllowedOrigins {
if err := origin.Validate(); err != nil {
debuglogger.Logf("invalid CORS allowed origin: %s", origin)
return err
}
}
// AllowedMethods can't be empty
if len(cr.AllowedMethods) == 0 {
debuglogger.Logf("empty CORS allowed methods")
return s3err.GetAPIError(s3err.ErrMalformedXML)
}
// validate CORS allowed methods
for _, method := range cr.AllowedMethods {
if !method.IsValid() {
@@ -161,6 +189,13 @@ func (cr *CORSRule) Validate() error {
return s3err.GetUnsopportedCORSMethodErr(method.String())
}
}
// validate CORS allowed headers
for _, header := range cr.AllowedHeaders {
if !header.IsValid() {
debuglogger.Logf("invalid CORS allowed header: %s", header)
return s3err.GetInvalidCORSHeaderErr(header.String())
}
}
// validate CORS expose headers
for _, header := range cr.ExposeHeaders {
if !header.IsValid() {
@@ -181,7 +216,7 @@ func (cr *CORSRule) Match(origin string, method CORSHTTPMethod, headers []CORSHe
// check if the provided origin exists in CORS AllowedOrigins
for _, or := range cr.AllowedOrigins {
if wildcardMatch(or, origin) {
if wildcardMatch(or.String(), origin) {
originFound = true
if or == "*" {
// mark wildcardOrigin as true, if "*" is found in AllowedOrigins
+75 -19
View File
@@ -22,6 +22,31 @@ import (
"github.com/versity/versitygw/s3err"
)
func TestCORSOrigin_Validate(t *testing.T) {
tests := []struct {
name string
input string
err error
}{
{"invalid double wildcard", "*hello*", s3err.GetMultipleWildcardCORSOriginErr("*hello*")},
{"invalid double wildcard 2", "something**", s3err.GetMultipleWildcardCORSOriginErr("something**")},
{"invalid double wildcard 3", "som*eth*ing", s3err.GetMultipleWildcardCORSOriginErr("som*eth*ing")},
{"invalid multiple wildcard", "http://*.example.com/*/path/*", s3err.GetMultipleWildcardCORSOriginErr("http://*.example.com/*/path/*")},
{"invalid multiple wildcard 2", "http://example.com/*/so/*/much/*/wildcards/*/in/*/this/*/path", s3err.GetMultipleWildcardCORSOriginErr("http://example.com/*/so/*/much/*/wildcards/*/in/*/this/*/path")},
{"no wildcard", "http://127.0.0.1:8080", nil},
{"one wildcard", "http://127.0.0.1:8080/*", nil},
// empty string - valid
{"empty origin", "", nil},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
co := CORSOrigin(tt.input)
gotErr := co.Validate()
assert.Equal(t, tt.err, gotErr)
})
}
}
func TestCORSHeader_IsValid(t *testing.T) {
tests := []struct {
name string
@@ -50,7 +75,7 @@ func TestCORSHTTPMethod_IsValid(t *testing.T) {
method CORSHTTPMethod
want bool
}{
{"empty valid", "", true},
{"empty invalid", "", false},
{"GET valid", http.MethodGet, true},
{"HEAD valid", http.MethodHead, true},
{"PUT valid", http.MethodPut, true},
@@ -158,9 +183,13 @@ func TestCORSConfiguration_Validate(t *testing.T) {
{"nil config", nil, s3err.GetAPIError(s3err.ErrMalformedXML)},
{"nil rules", &CORSConfiguration{}, s3err.GetAPIError(s3err.ErrMalformedXML)},
{"empty rules", &CORSConfiguration{Rules: []CORSRule{}}, s3err.GetAPIError(s3err.ErrMalformedXML)},
{"invalid rule", &CORSConfiguration{Rules: []CORSRule{{AllowedHeaders: []CORSHeader{"Invalid Header"}}}}, s3err.GetInvalidCORSHeaderErr("Invalid Header")},
{"invalid rule", &CORSConfiguration{Rules: []CORSRule{{
AllowedHeaders: []CORSHeader{"Invalid Header"},
AllowedOrigins: []CORSOrigin{"origin"},
AllowedMethods: []CORSHTTPMethod{http.MethodPut},
}}}, s3err.GetInvalidCORSHeaderErr("Invalid Header")},
{"valid rule", &CORSConfiguration{Rules: []CORSRule{{
AllowedOrigins: []string{"origin"},
AllowedOrigins: []CORSOrigin{"origin"},
AllowedHeaders: []CORSHeader{"X-Test"},
AllowedMethods: []CORSHTTPMethod{http.MethodGet},
ExposeHeaders: []CORSHeader{"X-Expose"},
@@ -195,7 +224,7 @@ func TestCORSConfiguration_IsAllowed(t *testing.T) {
name: "allowed exact origin",
input: input{
cfg: &CORSConfiguration{Rules: []CORSRule{{
AllowedOrigins: []string{"http://allowed.com"},
AllowedOrigins: []CORSOrigin{"http://allowed.com"},
AllowedMethods: []CORSHTTPMethod{http.MethodGet},
AllowedHeaders: []CORSHeader{"X-Test"},
}}},
@@ -219,7 +248,7 @@ func TestCORSConfiguration_IsAllowed(t *testing.T) {
name: "allowed wildcard origin",
input: input{
cfg: &CORSConfiguration{Rules: []CORSRule{{
AllowedOrigins: []string{"*"},
AllowedOrigins: []CORSOrigin{"*"},
AllowedMethods: []CORSHTTPMethod{http.MethodGet},
AllowedHeaders: []CORSHeader{"X-Test"},
}}},
@@ -243,7 +272,7 @@ func TestCORSConfiguration_IsAllowed(t *testing.T) {
name: "forbidden no matching origin",
input: input{
cfg: &CORSConfiguration{Rules: []CORSRule{{
AllowedOrigins: []string{"http://nope.com"},
AllowedOrigins: []CORSOrigin{"http://nope.com"},
}}},
origin: "http://not-allowed.com",
method: http.MethodGet,
@@ -257,7 +286,7 @@ func TestCORSConfiguration_IsAllowed(t *testing.T) {
name: "forbidden method not allowed",
input: input{
cfg: &CORSConfiguration{Rules: []CORSRule{{
AllowedOrigins: []string{"http://allowed.com"},
AllowedOrigins: []CORSOrigin{"http://allowed.com"},
AllowedMethods: []CORSHTTPMethod{http.MethodPost},
AllowedHeaders: []CORSHeader{"X-Test"},
}}},
@@ -274,7 +303,7 @@ func TestCORSConfiguration_IsAllowed(t *testing.T) {
name: "forbidden header not allowed",
input: input{
cfg: &CORSConfiguration{Rules: []CORSRule{{
AllowedOrigins: []string{"http://allowed.com"},
AllowedOrigins: []CORSOrigin{"http://allowed.com"},
AllowedMethods: []CORSHTTPMethod{http.MethodGet},
AllowedHeaders: []CORSHeader{"X-Test"},
}}},
@@ -307,25 +336,52 @@ func TestCORSRule_Validate(t *testing.T) {
{
name: "valid rule",
rule: CORSRule{
AllowedOrigins: []string{"http://allowed.com"},
AllowedOrigins: []CORSOrigin{"http://allowed.com"},
AllowedMethods: []CORSHTTPMethod{http.MethodGet},
AllowedHeaders: []CORSHeader{"X-Test"},
},
want: nil,
},
{
name: "empty allowed origins",
rule: CORSRule{
AllowedOrigins: []CORSOrigin{},
AllowedMethods: []CORSHTTPMethod{http.MethodPost},
AllowedHeaders: []CORSHeader{"X-Test"},
},
want: s3err.GetAPIError(s3err.ErrMalformedXML),
},
{
name: "invalid allowed origins",
rule: CORSRule{
AllowedOrigins: []CORSOrigin{"http://allowed*/*"},
AllowedMethods: []CORSHTTPMethod{http.MethodGet},
AllowedHeaders: []CORSHeader{"X-Test"},
},
want: s3err.GetMultipleWildcardCORSOriginErr("http://allowed*/*"),
},
{
name: "invalid allowed methods",
rule: CORSRule{
AllowedOrigins: []string{"http://allowed.com"},
AllowedOrigins: []CORSOrigin{"http://allowed.com"},
AllowedMethods: []CORSHTTPMethod{"invalid_method"},
AllowedHeaders: []CORSHeader{"X-Test"},
},
want: s3err.GetUnsopportedCORSMethodErr("invalid_method"),
},
{
name: "empty allowed methods",
rule: CORSRule{
AllowedOrigins: []CORSOrigin{"http://allowed.com"},
AllowedMethods: []CORSHTTPMethod{},
AllowedHeaders: []CORSHeader{"X-Test"},
},
want: s3err.GetAPIError(s3err.ErrMalformedXML),
},
{
name: "invalid allowed header",
rule: CORSRule{
AllowedOrigins: []string{"http://allowed.com"},
AllowedOrigins: []CORSOrigin{"http://allowed.com"},
AllowedMethods: []CORSHTTPMethod{http.MethodGet},
AllowedHeaders: []CORSHeader{"Invalid Header"},
},
@@ -334,7 +390,7 @@ func TestCORSRule_Validate(t *testing.T) {
{
name: "invalid allowed header",
rule: CORSRule{
AllowedOrigins: []string{"http://allowed.com"},
AllowedOrigins: []CORSOrigin{"http://allowed.com"},
AllowedMethods: []CORSHTTPMethod{http.MethodGet},
AllowedHeaders: []CORSHeader{"Content-Length"},
ExposeHeaders: []CORSHeader{"Content-Encoding", "invalid header"},
@@ -371,7 +427,7 @@ func TestCORSRule_Match(t *testing.T) {
name: "exact origin and method match",
input: input{
rule: CORSRule{
AllowedOrigins: []string{"http://allowed.com"},
AllowedOrigins: []CORSOrigin{"http://allowed.com"},
AllowedMethods: []CORSHTTPMethod{http.MethodGet},
AllowedHeaders: []CORSHeader{"X-Test"},
},
@@ -385,7 +441,7 @@ func TestCORSRule_Match(t *testing.T) {
name: "wildcard origin match",
input: input{
rule: CORSRule{
AllowedOrigins: []string{"*"},
AllowedOrigins: []CORSOrigin{"*"},
AllowedMethods: []CORSHTTPMethod{http.MethodPost},
AllowedHeaders: []CORSHeader{"X-Test"},
},
@@ -399,7 +455,7 @@ func TestCORSRule_Match(t *testing.T) {
name: "wildcard containing origin match",
input: input{
rule: CORSRule{
AllowedOrigins: []string{"http://random*"},
AllowedOrigins: []CORSOrigin{"http://random*"},
AllowedMethods: []CORSHTTPMethod{http.MethodPost},
AllowedHeaders: []CORSHeader{"X-Test"},
},
@@ -413,7 +469,7 @@ func TestCORSRule_Match(t *testing.T) {
name: "wildcard allowed headers match",
input: input{
rule: CORSRule{
AllowedOrigins: []string{"http://something.com"},
AllowedOrigins: []CORSOrigin{"http://something.com"},
AllowedMethods: []CORSHTTPMethod{http.MethodPost},
AllowedHeaders: []CORSHeader{"X-*"},
},
@@ -427,7 +483,7 @@ func TestCORSRule_Match(t *testing.T) {
name: "origin mismatch",
input: input{
rule: CORSRule{
AllowedOrigins: []string{"http://allowed.com"},
AllowedOrigins: []CORSOrigin{"http://allowed.com"},
AllowedMethods: []CORSHTTPMethod{http.MethodGet},
AllowedHeaders: []CORSHeader{"X-Test"},
},
@@ -441,7 +497,7 @@ func TestCORSRule_Match(t *testing.T) {
name: "method mismatch",
input: input{
rule: CORSRule{
AllowedOrigins: []string{"http://allowed.com"},
AllowedOrigins: []CORSOrigin{"http://allowed.com"},
AllowedMethods: []CORSHTTPMethod{http.MethodPost},
AllowedHeaders: []CORSHeader{"X-Test"},
},
@@ -455,7 +511,7 @@ func TestCORSRule_Match(t *testing.T) {
name: "header mismatch",
input: input{
rule: CORSRule{
AllowedOrigins: []string{"http://allowed.com"},
AllowedOrigins: []CORSOrigin{"http://allowed.com"},
AllowedMethods: []CORSHTTPMethod{http.MethodGet},
AllowedHeaders: []CORSHeader{"X-Test"},
},
+1 -1
View File
@@ -319,7 +319,7 @@ func TestS3ApiController_GetBucketCors(t *testing.T) {
cors := &auth.CORSConfiguration{
Rules: []auth.CORSRule{
{
AllowedOrigins: []string{"origin"},
AllowedOrigins: []auth.CORSOrigin{"origin"},
AllowedMethods: []auth.CORSHTTPMethod{http.MethodPut},
AllowedHeaders: []auth.CORSHeader{"X-Amz-Date"},
},
+2 -2
View File
@@ -466,7 +466,7 @@ func TestS3ApiController_PutBucketCors(t *testing.T) {
validBody, err := xml.Marshal(auth.CORSConfiguration{
Rules: []auth.CORSRule{
{
AllowedOrigins: []string{"*"},
AllowedOrigins: []auth.CORSOrigin{"*"},
AllowedMethods: []auth.CORSHTTPMethod{http.MethodPost},
},
},
@@ -476,7 +476,7 @@ func TestS3ApiController_PutBucketCors(t *testing.T) {
invalidCors, err := xml.Marshal(auth.CORSConfiguration{
Rules: []auth.CORSRule{
{
AllowedOrigins: []string{"origin"},
AllowedOrigins: []auth.CORSOrigin{"origin"},
AllowedMethods: []auth.CORSHTTPMethod{"invalid_method"},
},
},
+1 -1
View File
@@ -33,7 +33,7 @@ func TestS3ApiController_CORSOptions(t *testing.T) {
cors, err := xml.Marshal(auth.CORSConfiguration{
Rules: []auth.CORSRule{
{
AllowedOrigins: []string{"example.com"},
AllowedOrigins: []auth.CORSOrigin{"example.com"},
AllowedMethods: []auth.CORSHTTPMethod{http.MethodGet, http.MethodPost},
AllowedHeaders: []auth.CORSHeader{"Content-Type", "Content-Disposition"},
ExposeHeaders: []auth.CORSHeader{"Content-Encoding", "date"},
+8
View File
@@ -1036,6 +1036,14 @@ func GetInvalidCORSRequestHeaderErr(header string) APIError {
}
}
func GetMultipleWildcardCORSOriginErr(origin string) APIError {
return APIError{
Code: "InvalidRequest",
Description: fmt.Sprintf(`AllowedOrigin "%s" can not have more than one wildcard.`, origin),
HTTPStatusCode: http.StatusBadRequest,
}
}
func GetUnsopportedCORSMethodErr(method string) APIError {
return APIError{
Code: "InvalidRequest",
+52 -10
View File
@@ -54,21 +54,63 @@ func PutBucketCors_empty_cors_rules(s *S3Conf) error {
})
}
func PutBucketCors_invalid_allowed_origins(s *S3Conf) error {
testName := "PutBucketCors_invalid_allowed_origins"
return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
for i, test := range []struct {
allowedOrigins []string
expectedErr s3err.APIError
}{
// empty allowed origins
{[]string{}, s3err.GetAPIError(s3err.ErrMalformedXML)},
// multiple wildcards
{[]string{"*example.com*"}, s3err.GetMultipleWildcardCORSOriginErr("*example.com*")},
{[]string{"http://127.0.0.1:7070", "https://*:7070/*"}, s3err.GetMultipleWildcardCORSOriginErr("https://*:7070/*")},
{[]string{"a/really/*/long/*/path/*"}, s3err.GetMultipleWildcardCORSOriginErr("a/really/*/long/*/path/*")},
{[]string{"http://***", "www.example.net"}, s3err.GetMultipleWildcardCORSOriginErr("http://***")},
} {
err := putBucketCors(s3client, &s3.PutBucketCorsInput{
Bucket: &bucket,
CORSConfiguration: &types.CORSConfiguration{
CORSRules: []types.CORSRule{
{
AllowedOrigins: test.allowedOrigins,
AllowedMethods: []string{http.MethodPost},
AllowedHeaders: []string{"x-amz-expected-bucket-owner"},
ExposeHeaders: []string{"x-vgw-something"},
},
},
},
})
if err := checkApiErr(err, test.expectedErr); err != nil {
return fmt.Errorf("test %v failed: %w", i+1, err)
}
}
return nil
})
}
func PutBucketCors_invalid_method(s *S3Conf) error {
testName := "PutBucketCors_invalid_method"
return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
for _, test := range []struct {
invalidMethod string
allowedMethods []string
expectedErr s3err.APIError
}{
{"get", []string{"get"}},
{"put", []string{"put"}},
{"post", []string{"post"}},
{"head", []string{"head"}},
{"delete", []string{"delete"}},
{http.MethodPatch, []string{http.MethodGet, http.MethodPatch}},
{http.MethodOptions, []string{http.MethodPost, http.MethodOptions}},
{"invalid_method", []string{http.MethodGet, http.MethodHead, http.MethodPost, http.MethodPut, http.MethodDelete, "invalid_method"}},
// empty array
{[]string{}, s3err.GetAPIError(s3err.ErrMalformedXML)},
// empty method
{[]string{""}, s3err.GetUnsopportedCORSMethodErr("")},
// invalid methods
{[]string{"get"}, s3err.GetUnsopportedCORSMethodErr("get")},
{[]string{"put"}, s3err.GetUnsopportedCORSMethodErr("put")},
{[]string{"post"}, s3err.GetUnsopportedCORSMethodErr("post")},
{[]string{"head"}, s3err.GetUnsopportedCORSMethodErr("head")},
{[]string{"delete"}, s3err.GetUnsopportedCORSMethodErr("delete")},
{[]string{http.MethodGet, http.MethodPatch}, s3err.GetUnsopportedCORSMethodErr(http.MethodPatch)},
{[]string{http.MethodPost, http.MethodOptions}, s3err.GetUnsopportedCORSMethodErr(http.MethodOptions)},
{[]string{http.MethodGet, http.MethodHead, http.MethodPost, http.MethodPut, http.MethodDelete, "invalid_method"}, s3err.GetUnsopportedCORSMethodErr("invalid_method")},
} {
err := putBucketCors(s3client, &s3.PutBucketCorsInput{
Bucket: &bucket,
@@ -84,7 +126,7 @@ func PutBucketCors_invalid_method(s *S3Conf) error {
},
})
if err := checkApiErr(err, s3err.GetUnsopportedCORSMethodErr(test.invalidMethod)); err != nil {
if err := checkApiErr(err, test.expectedErr); err != nil {
return err
}
}
+2
View File
@@ -603,6 +603,7 @@ func TestDeleteBucketPolicy(ts *TestState) {
func TestPutBucketCors(ts *TestState) {
ts.Run(PutBucketCors_non_existing_bucket)
ts.Run(PutBucketCors_empty_cors_rules)
ts.Run(PutBucketCors_invalid_allowed_origins)
ts.Run(PutBucketCors_invalid_method)
ts.Run(PutBucketCors_invalid_header)
ts.Run(PutBucketCors_md5)
@@ -1591,6 +1592,7 @@ func GetIntTests() IntTests {
"DeleteBucketPolicy_success": DeleteBucketPolicy_success,
"PutBucketCors_non_existing_bucket": PutBucketCors_non_existing_bucket,
"PutBucketCors_empty_cors_rules": PutBucketCors_empty_cors_rules,
"PutBucketCors_invalid_allowed_origins": PutBucketCors_invalid_allowed_origins,
"PutBucketCors_invalid_method": PutBucketCors_invalid_method,
"PutBucketCors_invalid_header": PutBucketCors_invalid_header,
"PutBucketCors_md5": PutBucketCors_md5,