issues/436: Fix a number of CORS issues (#442)

If allowedOrigins is nil, `domain` and its www variant are used
Validate allowed origins, methods, headers, etc.

- Fixes: #436
Hat tip:
- https://jub0bs.com/posts/2023-02-08-fearless-cors/ : This is a really great blogpost on all things CORS.

CHANGELOG.md

@@ -3,6 +3,9 @@
 Most recent version is listed first.  
 
 
+# v0.1.2
+- ong/middleware: Fix a number of CORS issues: https://github.com/komuw/ong/pull/442
+
 # v0.1.1
 - ong/middleware: do not show hint: https://github.com/komuw/ong/pull/457 
 

config/config.go

@@ -204,11 +204,11 @@ func (o Opts) GoString() string {
 // loadShedBreachLatency is the p99 latency at which point we start dropping(loadshedding) requests. If it is less than 1nanosecond, [DefaultLoadShedBreachLatency] is used instead.
 //
 // allowedOrigins, allowedMethods, allowedHeaders, allowCredentials & corsCacheDuration are used by the CORS middleware.
-// If allowedOrigins is nil, all origins are allowed. You can also use []string{"*"} to allow all.
+// If allowedOrigins is nil, [domain] and its www variant are used. Use []string{"*"} to allow all.
 // If allowedMethods is nil, "GET", "POST", "HEAD" are allowed. Use []string{"*"} to allow all.
 // If allowedHeaders is nil, "Origin", "Accept", "Content-Type", "X-Requested-With" are allowed. Use []string{"*"} to allow all.
 // allowCredentials indicates whether the request can include user credentials like cookies, HTTP authentication or client side SSL certificates.
-// corsCacheDuration is the duration that preflight responses will be cached. If it is less than 1second, [DefaultCorsCacheDuration] is used instead.
+// corsCacheDuration is the duration that preflight responses will be cached. If it is less than 1second, [DefaultCorsCacheDuration] is used instead, 0 second is an exception and is used as is.
 //
 // csrfTokenDuration is the duration that csrf cookie will be valid for. If it is less than 1second, [DefaultCsrfCookieDuration] is used instead.
 //

config/example_test.go

@@ -43,7 +43,7 @@ func ExampleNew() {
 		// If the p99 response latencies, over the last 5minutes is more than 200ms, then start loadshedding.
 		200*time.Millisecond,
 		// Allow access from these origins for CORs.
-		[]string{"example.net", "example.org"},
+		[]string{"http://example.net", "https://example.org"},
 		// Allow only GET and POST for CORs.
 		[]string{"GET", "POST"},
 		// Allow all http headers for CORs.

middleware/cors.go

@@ -1,13 +1,16 @@
 package middleware
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
+	"net/url"
 	"slices"
 	"strings"
 	"time"
 
 	"github.com/komuw/ong/config"
+	"github.com/komuw/ong/internal/acme"
 )
 
 // Some of the code here is inspired(or taken from) by:
@@ -24,6 +27,8 @@ import (
 // `http://example.com` and `https://example.com` are different origins(http vs https)
 //
 // - https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS
+// - https://jub0bs.com/posts/2023-02-08-fearless-cors
+// - https://fetch.spec.whatwg.org/#cors-preflight-request <- The fetch standard is also the canonical reference for CORS. YES.
 
 const (
 	// header is used by browsers when issuing a preflight request.
@@ -40,7 +45,8 @@ const (
 	acacHeader = "Access-Control-Allow-Credentials"
 	// header to allow CORS to resources in a private network(eg behind a VPN)
 	// you can set this header to `true` when you receive a preflight request if you want to allow access.
-	// Otherwise omit it entirely(as we will in this library)
+	// Otherwise omit it entirely(as we will in this library).
+	// If we ever implement this, read; https://jub0bs.com/posts/2023-02-08-fearless-cors/#3-provide-support-for-private-network-access
 	acrpnHeader = "Access-Control-Request-Private-Network"
 	_           = acrpnHeader
 	// acmaHeader stores how long(in seconds) the results of a preflight request can be cached.
@@ -54,7 +60,7 @@ const (
 
 // cors is a middleware to implement Cross-Origin Resource Sharing support.
 //
-// If allowedOrigins is nil, all origins are allowed. You can also use * to allow all.
+// If allowedOrigins is nil, domain and its www variant are used. You can also use * to allow all.
 // If allowedMethods is nil, "GET", "POST", "HEAD" are allowed. Use * to allow all.
 // If allowedHeaders is nil, "Origin", "Accept", "Content-Type", "X-Requested-With" are allowed. Use * to allow all.
 func cors(
@@ -64,19 +70,47 @@ func cors(
 	allowedHeaders []string,
 	allowCredentials bool,
 	corsCacheDuration time.Duration,
+	domain string,
 ) http.HandlerFunc {
-	allowedOrigins, allowedWildcardOrigins := getOrigins(allowedOrigins)
+	if err := acme.Validate(domain); err != nil {
+		panic(err)
+	}
+
+	allowedOrigins, allowedWildcardOrigins := getOrigins(allowedOrigins, domain)
+	if err := validateAllowedOrigins(allowedOrigins); err != nil {
+		panic(err)
+	}
 	allowedMethods = getMethods(allowedMethods)
+	if err := validateAllowedMethods(allowedMethods); err != nil {
+		panic(err)
+	}
 	allowedHeaders = getHeaders(allowedHeaders)
+	if err := validateAllowedRequestHeaders(allowedHeaders); err != nil {
+		panic(err)
+	}
+
+	if err := validateAllowCredentials(allowCredentials, allowedOrigins, allowedMethods, allowedHeaders); err != nil {
+		panic(err)
+	}
 
-	if corsCacheDuration < 1*time.Second { // It is measured in seconds.
+	if corsCacheDuration < 1*time.Second && (corsCacheDuration != 0*time.Second) {
+		// It is measured in seconds.
+		// Specifying a value of 0 tells browsers not to cache the preflight response, hence we should make it possible for one to specify zero seconds.
 		corsCacheDuration = config.DefaultCorsCacheDuration
 	}
 
 	return func(w http.ResponseWriter, r *http.Request) {
-		if r.Method == http.MethodOptions && r.Header.Get(acrmHeader) != "" {
+		if (r.Method == http.MethodOptions) && (r.Header.Get(acrmHeader) != "") && (r.Header.Get(originHeader) != "") {
+			/*
+				The Fetch standard says that a preflight request is one that:
+				(a) uses the OPTIONS method
+				(b) includes an Origin header,
+				(c) includes an Access-Control-Request-Method header.
+				- https://fetch.spec.whatwg.org/#cors-preflight-request
+				- https://jub0bs.com/posts/2023-02-08-fearless-cors/#4-categorise-requests-correctly
+			*/
 			// handle preflight request
-			handlePreflight(w, r, allowedOrigins, allowedWildcardOrigins, allowedMethods, allowedHeaders, allowCredentials, corsCacheDuration)
+			handlePreflight(w, r, allowedOrigins, allowedWildcardOrigins, allowedMethods, allowedHeaders, allowCredentials, int(corsCacheDuration.Seconds()))
 			// Preflight requests are standalone and should stop the chain as some other
 			// middleware may not handle OPTIONS requests correctly. One typical example
 			// is authentication middleware ; OPTIONS requests won't carry authentication headers.
@@ -97,10 +131,11 @@ func handlePreflight(
 	allowedMethods []string,
 	allowedHeaders []string,
 	allowCredentials bool,
-	corsCacheDuration time.Duration,
+	corsCacheDuration int,
 ) {
 	headers := w.Header()
 	origin := r.Header.Get(originHeader)
+	// According to the CORS/Fetch standard, method names are case-sensitive. So do not uppercase/lowercase reqMethod.
 	reqMethod := r.Header.Get(acrmHeader) // note this is different from the one in `handleActualRequest`
 	reqHeader := r.Header.Get(acrhHeader)
 
@@ -150,7 +185,15 @@ func handlePreflight(
 	// (b)
 	// spec says we can return just one method instead of all the supported ones.
 	// that one method has to be the one that came in via the `acrmHeader`
-	headers.Set(acamHeader, strings.ToUpper(reqMethod))
+	headers.Set(
+		acamHeader,
+		// According to the CORS/Fetch standard, method names are case-sensitive.
+		// Thus if preflight has `put`, the header should have `put`(not `PUT`) else browser will show error.
+		//
+		// - https://fetch.spec.whatwg.org/#methods
+		// - https://jub0bs.com/posts/2023-02-08-fearless-cors/#violation-of-the-case-sensitivity-of-method-names
+		reqMethod,
+	)
 
 	// (c)
 	if len(reqMethod) > 0 {
@@ -160,7 +203,7 @@ func handlePreflight(
 	}
 
 	// (d)
-	headers.Set(acmaHeader, fmt.Sprintf("%d", int(corsCacheDuration.Seconds())))
+	headers.Set(acmaHeader, fmt.Sprintf("%d", corsCacheDuration))
 
 	// (e)
 	if allowCredentials {
@@ -255,6 +298,9 @@ func isMethodAllowed(method string, allowedMethods []string) bool {
 		return true
 	}
 
+	// The spec talks of normalizing some methods(`DELETE`, `GET`, `HEAD`, `OPTIONS`, `POST`, `PUT`) to uppercase.
+	// This library normalizes all methods not just the ones mentioned in the spec.
+	// https://fetch.spec.whatwg.org/#concept-method-normalize
 	method = strings.ToUpper(method)
 	if method == http.MethodOptions {
 		// Always allow preflight requests
@@ -297,13 +343,16 @@ func areHeadersAllowed(reqHeader string, allowedHeaders []string) bool {
 	return true
 }
 
-func getOrigins(ao []string) (allowedOrigins []string, allowedWildcardOrigins []wildcard) {
+func getOrigins(ao []string, domain string) (allowedOrigins []string, allowedWildcardOrigins []wildcard) {
 	if len(ao) == 0 {
-		return []string{"*"}, []wildcard{}
-	}
-
-	if slices.Contains(allowedOrigins, "*") {
-		return []string{"*"}, []wildcard{}
+		if strings.Contains(domain, "*") { // `acme.Validate(domain)` should have been called prior.
+			domain = domain[2:] // remove the `*` and `.`
+		}
+		domains := []string{"https://" + domain}
+		if !strings.HasPrefix(domain, "www") && strings.Count(domain, ".") == 1 {
+			domains = append(domains, "https://www."+domain) // as a special case, add `www`
+		}
+		return domains, []wildcard{}
 	}
 
 	canon := []string{}
@@ -332,6 +381,7 @@ func getMethods(am []string) []string {
 	if len(am) == 0 {
 		return []string{
 			// the spec by default allows this simple methods for cross-origin-requests: GET, POST, HEAD.
+			// https://fetch.spec.whatwg.org/#methods
 			strings.ToUpper(http.MethodGet),
 			strings.ToUpper(http.MethodPost),
 			strings.ToUpper(http.MethodHead),
@@ -352,12 +402,16 @@ func getMethods(am []string) []string {
 
 func getHeaders(ah []string) []string {
 	if len(ah) == 0 {
-		// use sensible defaults.
+		// Use the list of CORS safelisted request headers.
+		// - https://developer.mozilla.org/en-US/docs/Glossary/CORS-safelisted_request_header
+		// - https://fetch.spec.whatwg.org/#no-cors-safelisted-request-header-name
+		// - https://fetch.spec.whatwg.org/#privileged-no-cors-request-header-name
 		return []string{
-			http.CanonicalHeaderKey("Origin"),
 			http.CanonicalHeaderKey("Accept"),
+			http.CanonicalHeaderKey("Accept-Language"),
+			http.CanonicalHeaderKey("Content-Language"),
 			http.CanonicalHeaderKey("Content-Type"),
-			http.CanonicalHeaderKey("X-Requested-With"),
+			http.CanonicalHeaderKey("Range"),
 		}
 	}
 
@@ -372,3 +426,139 @@ func getHeaders(ah []string) []string {
 
 	return allowedHeaders
 }
+
+func validateAllowedOrigins(allowedOrigins []string) error {
+	/*
+		origin is defined by the scheme (protocol), hostname (domain), and port
+		https://developer.mozilla.org/en-US/docs/Glossary/Origin
+	*/
+	if len(allowedOrigins) > 1 && slices.Contains(allowedOrigins, "*") {
+		return errors.New("ong/middleware/cors: single wildcard should not be used together with other allowedOrigins")
+	}
+
+	if len(allowedOrigins) == 1 && allowedOrigins[0] == "*" {
+		return nil
+	}
+
+	for _, origin := range allowedOrigins {
+		u, err := url.Parse(origin)
+		if err != nil {
+			return err
+		}
+
+		if u.Scheme == "" {
+			return fmt.Errorf("ong/middleware/cors: scheme should not be empty `%v`", origin)
+		}
+		if u.Host == "" {
+			return fmt.Errorf("ong/middleware/cors: host should not be empty `%v`", origin)
+		}
+		if u.Path != "" {
+			return fmt.Errorf("ong/middleware/cors: should not contain url path `%v`", origin)
+		}
+
+		if strings.Count(origin, "*") > 1 {
+			return fmt.Errorf("ong/middleware/cors: should not contain more than one wildcard `%v`", origin)
+		}
+		if strings.Count(origin, "*") == 1 {
+			if !strings.HasPrefix(u.Host, "*") {
+				return fmt.Errorf("ong/middleware/cors: wildcard should be prefixed to host `%v`", origin)
+			}
+		}
+	}
+
+	return nil
+}
+
+func validateAllowCredentials(
+	allowCredentials bool,
+	allowedOrigins []string,
+	allowedMethods []string,
+	allowedHeaders []string,
+) error {
+	// Credentialed requests cannot be used with wildcard.
+	// https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#credentialed_requests_and_wildcards
+
+	// `validateAllowedOrigins` has already checked that wildcard can only exist in slice of len 1.
+	if allowCredentials && len(allowedOrigins) == 1 && allowedOrigins[0] == "*" {
+		return errors.New("ong/middleware/cors: allowCredentials should not be used together with wildcard allowedOrigins")
+	}
+	if allowCredentials && len(allowedMethods) == 1 && allowedMethods[0] == "*" {
+		return errors.New("ong/middleware/cors: allowCredentials should not be used together with wildcard allowedMethods")
+	}
+	if allowCredentials && len(allowedHeaders) == 1 && allowedHeaders[0] == "*" {
+		return errors.New("ong/middleware/cors: allowCredentials should not be used together with wildcard allowedHeaders")
+	}
+
+	if allowCredentials {
+		// Credentialed requests should not be used with 'http' scheme. Should require 'https'.
+		// https://jub0bs.com/posts/2023-02-08-fearless-cors/#disallow-insecure-origins-by-default
+		// https://portswigger.net/research/exploiting-cors-misconfigurations-for-bitcoins-and-bounties
+		for _, origin := range allowedOrigins {
+			u, err := url.Parse(origin)
+			if err != nil {
+				return err
+			}
+			if u.Scheme == "http" {
+				return fmt.Errorf("ong/middleware/cors: allowCredentials should not be used together with origin that uses unsecure scheme `%v`", origin)
+			}
+		}
+	}
+
+	return nil
+}
+
+func validateAllowedMethods(allowedMethods []string) error {
+	// There are some methods that are disallowed.
+	// https://fetch.spec.whatwg.org/#methods
+	for _, m := range allowedMethods {
+		if slices.Contains([]string{"CONNECT", "TRACE", "TRACK"}, strings.ToUpper(m)) {
+			return fmt.Errorf("ong/middleware/cors: method is forbidden in CORS allowedMethods `%v`", m)
+		}
+	}
+
+	return nil
+}
+
+func validateAllowedRequestHeaders(allowedHeaders []string) error {
+	// There are some headers that are disallowed.
+	// https://fetch.spec.whatwg.org/#terminology-headers
+	for _, h := range allowedHeaders {
+		if slices.Contains([]string{
+			"ACCEPT-CHARSET",
+			"ACCEPT-ENCODING",
+			"ACCESS-CONTROL-REQUEST-HEADERS",
+			"ACCESS-CONTROL-REQUEST-METHOD",
+			"CONNECTION",
+			"CONTENT-LENGTH",
+			"COOKIE",
+			"COOKIE2",
+			"DATE",
+			"DNT",
+			"EXPECT",
+			"HOST",
+			"KEEP-ALIVE",
+			"ORIGIN",
+			"REFERER",
+			"SET-COOKIE",
+			"TE",
+			"TRAILER",
+			"TRANSFER-ENCODING",
+			"Upgrade",
+			"VIA",
+		},
+			// Spec says; "If name is a byte-case-insensitive match"
+			// So the use of `strings.ToUpper` here is correct.
+			strings.ToUpper(h),
+		) {
+			return fmt.Errorf("ong/middleware/cors: header is forbidden in CORS allowedHeaders `%v`", h)
+		}
+
+		if strings.HasPrefix(strings.ToLower(h), "proxy-") || strings.HasPrefix(strings.ToLower(h), "sec-") {
+			// Spec says; "If name when byte-lowercased starts with `proxy-` or `sec-`"
+			// So the use of `strings.ToLower` here is correct.
+			return fmt.Errorf("ong/middleware/cors: header is forbidden in CORS allowedHeaders `%v`", h)
+		}
+	}
+
+	return nil
+}