fix(middleware): classify CORS preflight per Fetch standard (#3060)

Only OPTIONS requests that include both Origin and
Access-Control-Request-Method are treated as CORS preflights.

Previously every OPTIONS request was short-circuited, so non-CORS
OPTIONS handlers (and OPTIONS without ACR-Method) never ran (#2534).

Author: sonnemusk

middleware/cors.go

@@ -201,18 +201,16 @@ func (config CORSConfig) ToMiddleware() (echo.MiddlewareFunc, error) {
 
 			res.Header().Add(echo.HeaderVary, echo.HeaderOrigin)
 
-			// Preflight request is an OPTIONS request, using three HTTP request headers: Access-Control-Request-Method,
-			// Access-Control-Request-Headers, and the Origin header. See: https://developer.mozilla.org/en-US/docs/Glossary/Preflight_request
-			// For simplicity we just consider method type and later `Origin` header.
-			preflight := req.Method == http.MethodOptions
-
-			// Although router adds special handler in case of OPTIONS method we avoid calling next for OPTIONS in this middleware
-			// as CORS requests do not have cookies / authentication headers by default, so we could get stuck in auth
-			// middlewares by calling next(c).
-			// But we still want to send `Allow` header as response in case of Non-CORS OPTIONS request as router default
-			// handler does.
+			// A CORS preflight request is an OPTIONS request that includes both
+			// Origin and Access-Control-Request-Method (Fetch standard / RFC 9110).
+			// Treating every OPTIONS request as preflight incorrectly short-circuits
+			// non-CORS OPTIONS handlers (see #2534).
+			preflight := isCORSPreflight(req)
+
+			// Surface router Allow for OPTIONS (preflight and non-preflight). For
+			// non-preflight OPTIONS the request continues to next(c).
 			routerAllowMethods := ""
-			if preflight {
+			if req.Method == http.MethodOptions {
 				tmpAllowMethods, ok := c.Get(echo.ContextKeyHeaderAllow).(string)
 				if ok && tmpAllowMethods != "" {
 					routerAllowMethods = tmpAllowMethods
@@ -222,10 +220,7 @@ func (config CORSConfig) ToMiddleware() (echo.MiddlewareFunc, error) {
 
 			// No Origin provided. This is (probably) not request from actual browser - proceed executing middleware chain
 			if origin == "" {
-				if preflight { // req.Method=OPTIONS
-					return c.NoContent(http.StatusNoContent)
-				}
-				return next(c) // let non-browser calls through
+				return next(c) // let non-browser / non-CORS OPTIONS through
 			}
 
 			allowedOrigin, allowed, err := allowOriginFunc(c, origin)
@@ -292,6 +287,17 @@ func (config CORSConfig) ToMiddleware() (echo.MiddlewareFunc, error) {
 	}, nil
 }
 
+// isCORSPreflight reports whether r is a CORS preflight request.
+//
+// Per the Fetch standard, a preflight is an OPTIONS request with both an Origin
+// header and an Access-Control-Request-Method header. OPTIONS alone (with or
+// without Origin) is not sufficient.
+func isCORSPreflight(r *http.Request) bool {
+	return r.Method == http.MethodOptions &&
+		r.Header.Get(echo.HeaderOrigin) != "" &&
+		r.Header.Get(echo.HeaderAccessControlRequestMethod) != ""
+}
+
 func (config CORSConfig) starAllowOriginFunc(c *echo.Context, origin string) (string, bool, error) {
 	return "*", true, nil
 }

middleware/cors_test.go

@@ -19,6 +19,7 @@ func TestCORS(t *testing.T) {
 	e := echo.New()
 	req := httptest.NewRequest(http.MethodOptions, "/", nil) // Preflight request
 	req.Header.Set(echo.HeaderOrigin, "http://example.com")
+	req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodGet)
 	rec := httptest.NewRecorder()
 	c := e.NewContext(req, rec)
 
@@ -33,6 +34,79 @@ func TestCORS(t *testing.T) {
 	assert.Equal(t, "*", rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
 }
 
+// TestCORS_NonPreflightOPTIONSPassThrough locks in #2534: only true CORS
+// preflights short-circuit the middleware. OPTIONS without the preflight
+// headers must reach the next handler.
+func TestCORS_NonPreflightOPTIONSPassThrough(t *testing.T) {
+	e := echo.New()
+	mw := CORS("*")
+
+	t.Run("OPTIONS without Origin reaches next", func(t *testing.T) {
+		req := httptest.NewRequest(http.MethodOptions, "/hello", nil)
+		rec := httptest.NewRecorder()
+		c := e.NewContext(req, rec)
+		called := false
+		handler := mw(func(c *echo.Context) error {
+			called = true
+			return c.NoContent(http.StatusNoContent)
+		})
+		assert.NoError(t, handler(c))
+		assert.True(t, called, "expected next to run for non-preflight OPTIONS")
+		assert.Empty(t, rec.Header().Get(echo.HeaderAccessControlAllowMethods))
+	})
+
+	t.Run("OPTIONS with Origin but without Access-Control-Request-Method reaches next", func(t *testing.T) {
+		req := httptest.NewRequest(http.MethodOptions, "/hello", nil)
+		req.Header.Set(echo.HeaderOrigin, "https://example.com")
+		rec := httptest.NewRecorder()
+		c := e.NewContext(req, rec)
+		called := false
+		handler := mw(func(c *echo.Context) error {
+			called = true
+			return c.NoContent(http.StatusNoContent)
+		})
+		assert.NoError(t, handler(c))
+		assert.True(t, called, "expected next to run when ACR-Method is missing")
+		// Origin was allowed; simple CORS headers may be set, but not preflight Allow-Methods.
+		assert.Equal(t, "*", rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
+		assert.Empty(t, rec.Header().Get(echo.HeaderAccessControlAllowMethods))
+	})
+
+	t.Run("true preflight still short-circuits next", func(t *testing.T) {
+		req := httptest.NewRequest(http.MethodOptions, "/hello", nil)
+		req.Header.Set(echo.HeaderOrigin, "https://example.com")
+		req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodPut)
+		rec := httptest.NewRecorder()
+		c := e.NewContext(req, rec)
+		called := false
+		handler := mw(func(c *echo.Context) error {
+			called = true
+			return c.String(http.StatusOK, "should not run")
+		})
+		assert.NoError(t, handler(c))
+		assert.False(t, called, "preflight must not call next")
+		assert.Equal(t, http.StatusNoContent, rec.Code)
+		assert.Equal(t, "*", rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
+		assert.NotEmpty(t, rec.Header().Get(echo.HeaderAccessControlAllowMethods))
+	})
+}
+
+func TestIsCORSPreflight(t *testing.T) {
+	req := httptest.NewRequest(http.MethodOptions, "/", nil)
+	assert.False(t, isCORSPreflight(req))
+
+	req.Header.Set(echo.HeaderOrigin, "https://example.com")
+	assert.False(t, isCORSPreflight(req))
+
+	req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodPut)
+	assert.True(t, isCORSPreflight(req))
+
+	req = httptest.NewRequest(http.MethodGet, "/", nil)
+	req.Header.Set(echo.HeaderOrigin, "https://example.com")
+	req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodPut)
+	assert.False(t, isCORSPreflight(req))
+}
+
 func TestCORSConfig(t *testing.T) {
 	var testCases = []struct {
 		name             string
@@ -275,6 +349,11 @@ func TestCORSConfig(t *testing.T) {
 			for k, v := range tc.whenHeaders {
 				req.Header.Set(k, v)
 			}
+			// Intentional preflight cases: OPTIONS + Origin need ACR-Method per Fetch.
+			if method == http.MethodOptions && req.Header.Get(echo.HeaderOrigin) != "" &&
+				req.Header.Get(echo.HeaderAccessControlRequestMethod) == "" {
+				req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodGet)
+			}
 
 			err = h(c)
 
@@ -413,6 +492,10 @@ func TestCORSWithConfig_AllowMethods(t *testing.T) {
 			c := e.NewContext(req, rec)
 
 			req.Header.Set(echo.HeaderOrigin, tc.whenOrigin)
+			if tc.whenOrigin != "" {
+				// Real preflight requires Access-Control-Request-Method (#2534).
+				req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodGet)
+			}
 			if tc.whenAllowContextKey != "" {
 				c.Set(echo.ContextKeyHeaderAllow, tc.whenAllowContextKey)
 			}
@@ -479,12 +562,13 @@ func TestCorsHeaders(t *testing.T) {
 			expectStatus:  http.StatusOK,
 		},
 		{
-			name:              "preflight, allow any origin, missing origin header = no CORS logic done",
+			// OPTIONS without Origin is not a CORS preflight; request continues to the router.
+			name:              "OPTIONS no origin, allow any origin = no CORS preflight short-circuit",
 			originDomain:      "", // Request does not have Origin header
 			allowedOrigin:     "*",
 			method:            http.MethodOptions,
 			expected:          false,
-			expectStatus:      http.StatusNoContent,
+			expectStatus:      http.StatusNoContent, // router default OPTIONS/Allow path
 			expectAllowHeader: "OPTIONS, GET, POST",
 		},
 		{
@@ -497,7 +581,7 @@ func TestCorsHeaders(t *testing.T) {
 			expectAllowHeader: "OPTIONS, GET, POST",
 		},
 		{
-			name:              "preflight, allow any origin, missing origin header = no CORS logic done",
+			name:              "OPTIONS no origin, allow specific origin = no CORS preflight short-circuit",
 			originDomain:      "", // Request does not have Origin header
 			allowedOrigin:     "http://example.com",
 			method:            http.MethodOptions,
@@ -548,6 +632,15 @@ func TestCorsHeaders(t *testing.T) {
 			if tc.originDomain != "" {
 				req.Header.Set(echo.HeaderOrigin, tc.originDomain)
 			}
+			// True CORS preflight requires Access-Control-Request-Method (#2534).
+			if tc.method == http.MethodOptions && tc.originDomain != "" && tc.expected {
+				req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodGet)
+			}
+			// Disallowed origin + OPTIONS still needs ACR-Method to be classified as preflight
+			// (middleware then omits ACAO and returns 204).
+			if tc.method == http.MethodOptions && tc.originDomain != "" && !tc.expected {
+				req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodGet)
+			}
 
 			// we run through whole Echo handler chain to see how CORS works with Router OPTIONS handler
 			e.ServeHTTP(rec, req)
@@ -606,6 +699,7 @@ func Test_allowOriginFunc(t *testing.T) {
 		rec := httptest.NewRecorder()
 		c := e.NewContext(req, rec)
 		req.Header.Set(echo.HeaderOrigin, origin)
+		req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodGet)
 		cors, err := CORSConfig{UnsafeAllowOriginFunc: allowOriginFunc}.ToMiddleware()
 		assert.NoError(t, err)