From a67ea8f9c65f528ef97e2635423879ef8bd4d82f Mon Sep 17 00:00:00 2001 From: sonnemusk Date: Thu, 23 Jul 2026 18:49:17 +0800 Subject: [PATCH] fix(middleware): classify CORS preflight per Fetch standard 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). --- middleware/cors.go | 36 +++++++++------ middleware/cors_test.go | 100 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 118 insertions(+), 18 deletions(-) diff --git a/middleware/cors.go b/middleware/cors.go index ed9cbca50..b4ddbd7f9 100644 --- a/middleware/cors.go +++ b/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 } diff --git a/middleware/cors_test.go b/middleware/cors_test.go index 5de4ca063..cd208fb8d 100644 --- a/middleware/cors_test.go +++ b/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)