Normalize allowed request headers and store them in a sorted set (fixes #170)

bench_test.go

@@ -2,6 +2,7 @@ package cors
 
 import (
 	"net/http"
+	"strings"
 	"testing"
 )
 
@@ -87,7 +88,22 @@ func BenchmarkPreflightHeader(b *testing.B) {
 	req, _ := http.NewRequest(http.MethodOptions, dummyEndpoint, nil)
 	req.Header.Add(headerOrigin, dummyOrigin)
 	req.Header.Add(headerACRM, http.MethodGet)
-	req.Header.Add(headerACRH, "Accept")
+	req.Header.Add(headerACRH, "accept")
+	handler := Default().Handler(testHandler)
+
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		handler.ServeHTTP(resps[i], req)
+	}
+}
+
+func BenchmarkPreflightAdversarialACRH(b *testing.B) {
+	resps := makeFakeResponses(b.N)
+	req, _ := http.NewRequest(http.MethodOptions, dummyEndpoint, nil)
+	req.Header.Add(headerOrigin, dummyOrigin)
+	req.Header.Add(headerACRM, http.MethodGet)
+	req.Header.Add(headerACRH, strings.Repeat(",", 1024))
 	handler := Default().Handler(testHandler)
 
 	b.ReportAllocs()

cors.go

@@ -26,6 +26,8 @@ import (
 	"os"
 	"strconv"
 	"strings"
+
+	"github.com/rs/cors/internal"
 )
 
 var headerVaryOrigin = []string{"Origin"}
@@ -111,7 +113,11 @@ type Cors struct {
 	// Optional origin validator function
 	allowOriginFunc func(r *http.Request, origin string) (bool, []string)
 	// Normalized list of allowed headers
-	allowedHeaders []string
+	// Note: the Fetch standard guarantees that CORS-unsafe request-header names
+	// (i.e. the values listed in the Access-Control-Request-Headers header)
+	// are unique and sorted;
+	// see https://fetch.spec.whatwg.org/#cors-unsafe-request-header-names.
+	allowedHeaders internal.SortedSet
 	// Normalized list of allowed methods
 	allowedMethods []string
 	// Pre-computed normalized list of exposed headers
@@ -183,15 +189,19 @@ func New(options Options) *Cors {
 	}
 
 	// Allowed Headers
+	// Note: the Fetch standard guarantees that CORS-unsafe request-header names
+	// (i.e. the values listed in the Access-Control-Request-Headers header)
+	// are lowercase; see https://fetch.spec.whatwg.org/#cors-unsafe-request-header-names.
 	if len(options.AllowedHeaders) == 0 {
 		// Use sensible defaults
-		c.allowedHeaders = []string{"Accept", "Content-Type", "X-Requested-With"}
+		c.allowedHeaders = internal.NewSortedSet("accept", "content-type", "x-requested-with")
 	} else {
-		c.allowedHeaders = convert(options.AllowedHeaders, http.CanonicalHeaderKey)
+		normalized := convert(options.AllowedHeaders, strings.ToLower)
+		c.allowedHeaders = internal.NewSortedSet(normalized...)
 		for _, h := range options.AllowedHeaders {
 			if h == "*" {
 				c.allowedHeadersAll = true
-				c.allowedHeaders = nil
+				c.allowedHeaders = internal.SortedSet{}
 				break
 			}
 		}
@@ -351,10 +361,12 @@ func (c *Cors) handlePreflight(w http.ResponseWriter, r *http.Request) {
 		c.logf("  Preflight aborted: method '%s' not allowed", reqMethod)
 		return
 	}
-	reqHeadersRaw := r.Header["Access-Control-Request-Headers"]
-	reqHeaders, reqHeadersEdited := convertDidCopy(splitHeaderValues(reqHeadersRaw), http.CanonicalHeaderKey)
-	if !c.areHeadersAllowed(reqHeaders) {
-		c.logf("  Preflight aborted: headers '%v' not allowed", reqHeaders)
+	// Note: the Fetch standard guarantees that at most one
+	// Access-Control-Request-Headers header is present in the preflight request;
+	// see step 5.2 in https://fetch.spec.whatwg.org/#cors-preflight-fetch-0.
+	reqHeaders, found := first(r.Header, "Access-Control-Request-Headers")
+	if found && !c.allowedHeadersAll && !c.allowedHeaders.Subsumes(reqHeaders[0]) {
+		c.logf("  Preflight aborted: headers '%v' not allowed", reqHeaders[0])
 		return
 	}
 	if c.allowedOriginsAll {
@@ -365,14 +377,10 @@ func (c *Cors) handlePreflight(w http.ResponseWriter, r *http.Request) {
 	// Spec says: Since the list of methods can be unbounded, simply returning the method indicated
 	// by Access-Control-Request-Method (if supported) can be enough
 	headers["Access-Control-Allow-Methods"] = r.Header["Access-Control-Request-Method"]
-	if len(reqHeaders) > 0 {
+	if found && len(reqHeaders[0]) > 0 {
 		// Spec says: Since the list of headers can be unbounded, simply returning supported headers
 		// from Access-Control-Request-Headers can be enough
-		if reqHeadersEdited || len(reqHeaders) != len(reqHeadersRaw) {
-			headers.Set("Access-Control-Allow-Headers", strings.Join(reqHeaders, ", "))
-		} else {
-			headers["Access-Control-Allow-Headers"] = reqHeadersRaw
-		}
+		headers["Access-Control-Allow-Headers"] = reqHeaders
 	}
 	if c.allowCredentials {
 		headers["Access-Control-Allow-Credentials"] = headerTrue
@@ -492,24 +500,3 @@ func (c *Cors) isMethodAllowed(method string) bool {
 	}
 	return false
 }
-
-// areHeadersAllowed checks if a given list of headers are allowed to used within
-// a cross-domain request.
-func (c *Cors) areHeadersAllowed(requestedHeaders []string) bool {
-	if c.allowedHeadersAll || len(requestedHeaders) == 0 {
-		return true
-	}
-	for _, header := range requestedHeaders {
-		found := false
-		for _, h := range c.allowedHeaders {
-			if h == header {
-				found = true
-				break
-			}
-		}
-		if !found {
-			return false
-		}
-	}
-	return true
-}

cors_test.go

@@ -303,19 +303,19 @@ func TestSpec(t *testing.T) {
 			"AllowedHeaders",
 			Options{
 				AllowedOrigins: []string{"http://foobar.com"},
-				AllowedHeaders: []string{"X-Header-1", "x-header-2"},
+				AllowedHeaders: []string{"X-Header-1", "x-header-2", "X-HEADER-3"},
 			},
 			"OPTIONS",
 			map[string]string{
 				"Origin":                         "http://foobar.com",
 				"Access-Control-Request-Method":  "GET",
-				"Access-Control-Request-Headers": "X-Header-2, X-HEADER-1",
+				"Access-Control-Request-Headers": "x-header-1,x-header-2",
 			},
 			map[string]string{
 				"Vary":                         "Origin, Access-Control-Request-Method, Access-Control-Request-Headers",
 				"Access-Control-Allow-Origin":  "http://foobar.com",
 				"Access-Control-Allow-Methods": "GET",
-				"Access-Control-Allow-Headers": "X-Header-2, X-Header-1",
+				"Access-Control-Allow-Headers": "x-header-1,x-header-2",
 			},
 			true,
 		},
@@ -329,13 +329,13 @@ func TestSpec(t *testing.T) {
 			map[string]string{
 				"Origin":                         "http://foobar.com",
 				"Access-Control-Request-Method":  "GET",
-				"Access-Control-Request-Headers": "X-Requested-With",
+				"Access-Control-Request-Headers": "x-requested-with",
 			},
 			map[string]string{
 				"Vary":                         "Origin, Access-Control-Request-Method, Access-Control-Request-Headers",
 				"Access-Control-Allow-Origin":  "http://foobar.com",
 				"Access-Control-Allow-Methods": "GET",
-				"Access-Control-Allow-Headers": "X-Requested-With",
+				"Access-Control-Allow-Headers": "x-requested-with",
 			},
 			true,
 		},
@@ -349,13 +349,13 @@ func TestSpec(t *testing.T) {
 			map[string]string{
 				"Origin":                         "http://foobar.com",
 				"Access-Control-Request-Method":  "GET",
-				"Access-Control-Request-Headers": "X-Header-2, X-HEADER-1",
+				"Access-Control-Request-Headers": "x-header-1,x-header-2",
 			},
 			map[string]string{
 				"Vary":                         "Origin, Access-Control-Request-Method, Access-Control-Request-Headers",
 				"Access-Control-Allow-Origin":  "http://foobar.com",
 				"Access-Control-Allow-Methods": "GET",
-				"Access-Control-Allow-Headers": "X-Header-2, X-Header-1",
+				"Access-Control-Allow-Headers": "x-header-1,x-header-2",
 			},
 			true,
 		},
@@ -369,7 +369,7 @@ func TestSpec(t *testing.T) {
 			map[string]string{
 				"Origin":                         "http://foobar.com",
 				"Access-Control-Request-Method":  "GET",
-				"Access-Control-Request-Headers": "X-Header-3, X-Header-1",
+				"Access-Control-Request-Headers": "x-header-1,x-header-3",
 			},
 			map[string]string{
 				"Vary": "Origin, Access-Control-Request-Method, Access-Control-Request-Headers",
@@ -577,8 +577,8 @@ func TestDefault(t *testing.T) {
 	if !s.allowedOriginsAll {
 		t.Error("c.allowedOriginsAll should be true when Default")
 	}
-	if s.allowedHeaders == nil {
-		t.Error("c.allowedHeaders should be nil when Default")
+	if s.allowedHeaders.Size() == 0 {
+		t.Error("c.allowedHeaders should be empty when Default")
 	}
 	if s.allowedMethods == nil {
 		t.Error("c.allowedMethods should be nil when Default")
@@ -712,64 +712,6 @@ func TestOptionsSuccessStatusCodeOverride(t *testing.T) {
 	})
 }
 
-func TestCorsAreHeadersAllowed(t *testing.T) {
-	cases := []struct {
-		name             string
-		allowedHeaders   []string
-		requestedHeaders []string
-		want             bool
-	}{
-		{
-			name:             "nil allowedHeaders",
-			allowedHeaders:   nil,
-			requestedHeaders: []string{"X-PINGOTHER, Content-Type"},
-			want:             false,
-		},
-		{
-			name:             "star allowedHeaders",
-			allowedHeaders:   []string{"*"},
-			requestedHeaders: []string{"X-PINGOTHER, Content-Type"},
-			want:             true,
-		},
-		{
-			name:             "empty reqHeader",
-			allowedHeaders:   nil,
-			requestedHeaders: []string{},
-			want:             true,
-		},
-		{
-			name:             "match allowedHeaders",
-			allowedHeaders:   []string{"Content-Type", "X-PINGOTHER", "X-APP-KEY"},
-			requestedHeaders: []string{"X-PINGOTHER, Content-Type"},
-			want:             true,
-		},
-		{
-			name:             "not matched allowedHeaders",
-			allowedHeaders:   []string{"X-PINGOTHER"},
-			requestedHeaders: []string{"X-API-KEY, Content-Type"},
-			want:             false,
-		},
-		{
-			name:             "allowedHeaders should be a superset of requestedHeaders",
-			allowedHeaders:   []string{"X-PINGOTHER"},
-			requestedHeaders: []string{"X-PINGOTHER, Content-Type"},
-			want:             false,
-		},
-	}
-
-	for _, tt := range cases {
-		tt := tt
-
-		t.Run(tt.name, func(t *testing.T) {
-			c := New(Options{AllowedHeaders: tt.allowedHeaders})
-			have := c.areHeadersAllowed(convert(splitHeaderValues(tt.requestedHeaders), http.CanonicalHeaderKey))
-			if have != tt.want {
-				t.Errorf("Cors.areHeadersAllowed() have: %t want: %t", have, tt.want)
-			}
-		})
-	}
-}
-
 func TestAccessControlExposeHeadersPresence(t *testing.T) {
 	cases := []struct {
 		name    string