Skip to content

Commit ecd917d

Browse files
deepakganesh78Copilot
deepakganesh78
and
Copilot
committed
fix(middleware): only short-circuit CORS preflights
Treat OPTIONS requests as CORS preflights only when Origin and Access-Control-Request-Method are present, so non-preflight OPTIONS handlers can run. Fixes #2534 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 4f2f975 commit ecd917d

2 files changed

Lines changed: 135 additions & 37 deletions

File tree

‎middleware/cors.go‎

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -190,18 +190,12 @@ func CORSWithConfig(config CORSConfig) echo.MiddlewareFunc {
190190

191191
res.Header().Add(echo.HeaderVary, echo.HeaderOrigin)
192192

193-
// Preflight request is an OPTIONS request, using three HTTP request headers: Access-Control-Request-Method,
194-
// Access-Control-Request-Headers, and the Origin header. See: https://developer.mozilla.org/en-US/docs/Glossary/Preflight_request
195-
// For simplicity we just consider method type and later `Origin` header.
196-
preflight := req.Method == http.MethodOptions
197-
198-
// Although router adds special handler in case of OPTIONS method we avoid calling next for OPTIONS in this middleware
199-
// as CORS requests do not have cookies / authentication headers by default, so we could get stuck in auth
200-
// middlewares by calling next(c).
201-
// But we still want to send `Allow` header as response in case of Non-CORS OPTIONS request as router default
202-
// handler does.
193+
preflight := isCORSPreflight(req)
194+
195+
// Echo's router adds an Allow header for OPTIONS requests. Copy it before true
196+
// CORS preflight requests short-circuit the handler chain.
203197
routerAllowMethods := ""
204-
if preflight {
198+
if req.Method == http.MethodOptions {
205199
tmpAllowMethods, ok := c.Get(echo.ContextKeyHeaderAllow).(string)
206200
if ok && tmpAllowMethods != "" {
207201
routerAllowMethods = tmpAllowMethods
@@ -211,10 +205,7 @@ func CORSWithConfig(config CORSConfig) echo.MiddlewareFunc {
211205

212206
// No Origin provided. This is (probably) not request from actual browser - proceed executing middleware chain
213207
if origin == "" {
214-
if !preflight {
215-
return next(c)
216-
}
217-
return c.NoContent(http.StatusNoContent)
208+
return next(c)
218209
}
219210

220211
if config.AllowOriginFunc != nil {
@@ -305,3 +296,9 @@ func CORSWithConfig(config CORSConfig) echo.MiddlewareFunc {
305296
}
306297
}
307298
}
299+
300+
func isCORSPreflight(r *http.Request) bool {
301+
return r.Method == http.MethodOptions &&
302+
r.Header.Get(echo.HeaderOrigin) != "" &&
303+
r.Header.Get(echo.HeaderAccessControlRequestMethod) != ""
304+
}

‎middleware/cors_test.go‎

Lines changed: 123 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,9 @@ func TestCORS(t *testing.T) {
6565
}),
6666
whenMethod: http.MethodOptions,
6767
whenHeaders: map[string]string{
68-
echo.HeaderOrigin: "localhost",
69-
echo.HeaderContentType: echo.MIMEApplicationJSON,
68+
echo.HeaderOrigin: "localhost",
69+
echo.HeaderContentType: echo.MIMEApplicationJSON,
70+
echo.HeaderAccessControlRequestMethod: http.MethodGet,
7071
},
7172
expectHeaders: map[string]string{
7273
echo.HeaderAccessControlAllowOrigin: "localhost",
@@ -84,8 +85,9 @@ func TestCORS(t *testing.T) {
8485
}),
8586
whenMethod: http.MethodOptions,
8687
whenHeaders: map[string]string{
87-
echo.HeaderOrigin: "localhost",
88-
echo.HeaderContentType: echo.MIMEApplicationJSON,
88+
echo.HeaderOrigin: "localhost",
89+
echo.HeaderContentType: echo.MIMEApplicationJSON,
90+
echo.HeaderAccessControlRequestMethod: http.MethodGet,
8991
},
9092
expectHeaders: map[string]string{
9193
echo.HeaderAccessControlMaxAge: "1",
@@ -100,8 +102,9 @@ func TestCORS(t *testing.T) {
100102
}),
101103
whenMethod: http.MethodOptions,
102104
whenHeaders: map[string]string{
103-
echo.HeaderOrigin: "localhost",
104-
echo.HeaderContentType: echo.MIMEApplicationJSON,
105+
echo.HeaderOrigin: "localhost",
106+
echo.HeaderContentType: echo.MIMEApplicationJSON,
107+
echo.HeaderAccessControlRequestMethod: http.MethodGet,
105108
},
106109
expectHeaders: map[string]string{
107110
echo.HeaderAccessControlMaxAge: "0",
@@ -118,8 +121,9 @@ func TestCORS(t *testing.T) {
118121
}),
119122
whenMethod: http.MethodOptions,
120123
whenHeaders: map[string]string{
121-
echo.HeaderOrigin: "localhost",
122-
echo.HeaderContentType: echo.MIMEApplicationJSON,
124+
echo.HeaderOrigin: "localhost",
125+
echo.HeaderContentType: echo.MIMEApplicationJSON,
126+
echo.HeaderAccessControlRequestMethod: http.MethodGet,
123127
},
124128
notExpectHeaders: map[string]string{
125129
echo.HeaderAccessControlAllowOrigin: "localhost",
@@ -137,8 +141,9 @@ func TestCORS(t *testing.T) {
137141
}),
138142
whenMethod: http.MethodOptions,
139143
whenHeaders: map[string]string{
140-
echo.HeaderOrigin: "localhost",
141-
echo.HeaderContentType: echo.MIMEApplicationJSON,
144+
echo.HeaderOrigin: "localhost",
145+
echo.HeaderContentType: echo.MIMEApplicationJSON,
146+
echo.HeaderAccessControlRequestMethod: http.MethodGet,
142147
},
143148
expectHeaders: map[string]string{
144149
echo.HeaderAccessControlAllowOrigin: "*", // Note: browsers will ignore and complain about responses having `*`
@@ -156,8 +161,9 @@ func TestCORS(t *testing.T) {
156161
}),
157162
whenMethod: http.MethodOptions,
158163
whenHeaders: map[string]string{
159-
echo.HeaderOrigin: "localhost",
160-
echo.HeaderContentType: echo.MIMEApplicationJSON,
164+
echo.HeaderOrigin: "localhost",
165+
echo.HeaderContentType: echo.MIMEApplicationJSON,
166+
echo.HeaderAccessControlRequestMethod: http.MethodGet,
161167
},
162168
expectHeaders: map[string]string{
163169
echo.HeaderAccessControlAllowOrigin: "*",
@@ -178,8 +184,9 @@ func TestCORS(t *testing.T) {
178184
}),
179185
whenMethod: http.MethodOptions,
180186
whenHeaders: map[string]string{
181-
echo.HeaderOrigin: "localhost",
182-
echo.HeaderContentType: echo.MIMEApplicationJSON,
187+
echo.HeaderOrigin: "localhost",
188+
echo.HeaderContentType: echo.MIMEApplicationJSON,
189+
echo.HeaderAccessControlRequestMethod: http.MethodGet,
183190
},
184191
expectHeaders: map[string]string{
185192
echo.HeaderAccessControlAllowOrigin: "localhost", // This could end up as cross-origin attack
@@ -197,6 +204,7 @@ func TestCORS(t *testing.T) {
197204
whenHeaders: map[string]string{
198205
echo.HeaderOrigin: "localhost",
199206
echo.HeaderContentType: echo.MIMEApplicationJSON,
207+
echo.HeaderAccessControlRequestMethod: http.MethodGet,
200208
echo.HeaderAccessControlRequestHeaders: "Special-Request-Header",
201209
},
202210
expectHeaders: map[string]string{
@@ -210,17 +218,23 @@ func TestCORS(t *testing.T) {
210218
givenMW: CORSWithConfig(CORSConfig{
211219
AllowOrigins: []string{"http://*.example.com"},
212220
}),
213-
whenMethod: http.MethodOptions,
214-
whenHeaders: map[string]string{echo.HeaderOrigin: "http://aaa.example.com"},
221+
whenMethod: http.MethodOptions,
222+
whenHeaders: map[string]string{
223+
echo.HeaderOrigin: "http://aaa.example.com",
224+
echo.HeaderAccessControlRequestMethod: http.MethodGet,
225+
},
215226
expectHeaders: map[string]string{echo.HeaderAccessControlAllowOrigin: "http://aaa.example.com"},
216227
},
217228
{
218229
name: "ok, preflight request with `AllowOrigins` which allow all subdomains bbb with *",
219230
givenMW: CORSWithConfig(CORSConfig{
220231
AllowOrigins: []string{"http://*.example.com"},
221232
}),
222-
whenMethod: http.MethodOptions,
223-
whenHeaders: map[string]string{echo.HeaderOrigin: "http://bbb.example.com"},
233+
whenMethod: http.MethodOptions,
234+
whenHeaders: map[string]string{
235+
echo.HeaderOrigin: "http://bbb.example.com",
236+
echo.HeaderAccessControlRequestMethod: http.MethodGet,
237+
},
224238
expectHeaders: map[string]string{echo.HeaderAccessControlAllowOrigin: "http://bbb.example.com"},
225239
},
226240
}
@@ -265,6 +279,70 @@ func TestCORS(t *testing.T) {
265279
}
266280
}
267281

282+
func TestCORS_NonPreflightOPTIONSPassThrough(t *testing.T) {
283+
e := echo.New()
284+
cors := CORSWithConfig(CORSConfig{
285+
AllowOrigins: []string{"*"},
286+
AllowMethods: []string{http.MethodPut},
287+
})
288+
289+
for _, tc := range []struct {
290+
name string
291+
headers map[string]string
292+
}{
293+
{
294+
name: "OPTIONS without Origin",
295+
},
296+
{
297+
name: "OPTIONS with Origin but without Access-Control-Request-Method",
298+
headers: map[string]string{
299+
echo.HeaderOrigin: "https://example.com",
300+
},
301+
},
302+
} {
303+
t.Run(tc.name, func(t *testing.T) {
304+
req := httptest.NewRequest(http.MethodOptions, "/hello", nil)
305+
for k, v := range tc.headers {
306+
req.Header.Set(k, v)
307+
}
308+
rec := httptest.NewRecorder()
309+
c := e.NewContext(req, rec)
310+
called := false
311+
312+
h := cors(func(c echo.Context) error {
313+
called = true
314+
c.Response().Header().Set(echo.HeaderAllow, "GET, OPTIONS")
315+
return c.NoContent(http.StatusNoContent)
316+
})
317+
318+
assert.NoError(t, h(c))
319+
assert.True(t, called)
320+
assert.Equal(t, "GET, OPTIONS", rec.Header().Get(echo.HeaderAllow))
321+
assert.Empty(t, rec.Header().Get(echo.HeaderAccessControlAllowMethods))
322+
})
323+
}
324+
325+
t.Run("true preflight short-circuits next", func(t *testing.T) {
326+
req := httptest.NewRequest(http.MethodOptions, "/hello", nil)
327+
req.Header.Set(echo.HeaderOrigin, "https://example.com")
328+
req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodPut)
329+
rec := httptest.NewRecorder()
330+
c := e.NewContext(req, rec)
331+
called := false
332+
333+
h := cors(func(c echo.Context) error {
334+
called = true
335+
return c.NoContent(http.StatusOK)
336+
})
337+
338+
assert.NoError(t, h(c))
339+
assert.False(t, called)
340+
assert.Equal(t, http.StatusNoContent, rec.Code)
341+
assert.Equal(t, "*", rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
342+
assert.Equal(t, "PUT", rec.Header().Get(echo.HeaderAccessControlAllowMethods))
343+
})
344+
}
345+
268346
func Test_allowOriginScheme(t *testing.T) {
269347
tests := []struct {
270348
domain, pattern string
@@ -293,11 +371,14 @@ func Test_allowOriginScheme(t *testing.T) {
293371
}
294372

295373
e := echo.New()
374+
// These OPTIONS cases exercise preflight origin matching, so each request
375+
// includes Access-Control-Request-Method.
296376
for _, tt := range tests {
297377
req := httptest.NewRequest(http.MethodOptions, "/", nil)
298378
rec := httptest.NewRecorder()
299379
c := e.NewContext(req, rec)
300380
req.Header.Set(echo.HeaderOrigin, tt.domain)
381+
req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodGet)
301382
cors := CORSWithConfig(CORSConfig{
302383
AllowOrigins: []string{tt.pattern},
303384
})
@@ -384,11 +465,14 @@ func Test_allowOriginSubdomain(t *testing.T) {
384465
}
385466

386467
e := echo.New()
468+
// These OPTIONS cases exercise preflight origin matching, so each request
469+
// includes Access-Control-Request-Method.
387470
for _, tt := range tests {
388471
req := httptest.NewRequest(http.MethodOptions, "/", nil)
389472
rec := httptest.NewRecorder()
390473
c := e.NewContext(req, rec)
391474
req.Header.Set(echo.HeaderOrigin, tt.domain)
475+
req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodGet)
392476
cors := CORSWithConfig(CORSConfig{
393477
AllowOrigins: []string{tt.pattern},
394478
})
@@ -411,19 +495,20 @@ func TestCORSWithConfig_AllowMethods(t *testing.T) {
411495

412496
whenOrigin string
413497
whenAllowMethods []string
498+
whenPreflight bool
414499

415500
expectAllow string
416501
expectAccessControlAllowMethods string
417502
}{
418503
{
419-
name: "custom AllowMethods, preflight, no origin, sets only allow header from context key",
504+
name: "custom AllowMethods, OPTIONS no origin, sets only allow header from context key",
420505
allowContextKey: "OPTIONS, GET",
421506
whenAllowMethods: []string{http.MethodGet, http.MethodHead},
422507
whenOrigin: "",
423508
expectAllow: "OPTIONS, GET",
424509
},
425510
{
426-
name: "default AllowMethods, preflight, no origin, no allow header in context key and in response",
511+
name: "default AllowMethods, OPTIONS no origin, no allow header in context key and in response",
427512
allowContextKey: "",
428513
whenAllowMethods: nil,
429514
whenOrigin: "",
@@ -434,6 +519,7 @@ func TestCORSWithConfig_AllowMethods(t *testing.T) {
434519
allowContextKey: "OPTIONS, GET",
435520
whenAllowMethods: []string{http.MethodGet, http.MethodHead},
436521
whenOrigin: "http://google.com",
522+
whenPreflight: true,
437523
expectAllow: "OPTIONS, GET",
438524
expectAccessControlAllowMethods: "GET,HEAD",
439525
},
@@ -442,6 +528,7 @@ func TestCORSWithConfig_AllowMethods(t *testing.T) {
442528
allowContextKey: "OPTIONS, GET",
443529
whenAllowMethods: nil,
444530
whenOrigin: "http://google.com",
531+
whenPreflight: true,
445532
expectAllow: "OPTIONS, GET",
446533
expectAccessControlAllowMethods: "OPTIONS, GET",
447534
},
@@ -450,6 +537,7 @@ func TestCORSWithConfig_AllowMethods(t *testing.T) {
450537
allowContextKey: "",
451538
whenAllowMethods: nil,
452539
whenOrigin: "http://google.com",
540+
whenPreflight: true,
453541
expectAllow: "",
454542
expectAccessControlAllowMethods: "GET,HEAD,PUT,PATCH,POST,DELETE",
455543
},
@@ -472,6 +560,9 @@ func TestCORSWithConfig_AllowMethods(t *testing.T) {
472560
c := e.NewContext(req, rec)
473561

474562
req.Header.Set(echo.HeaderOrigin, tc.whenOrigin)
563+
if tc.whenPreflight {
564+
req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodGet)
565+
}
475566
if tc.allowContextKey != "" {
476567
c.Set(echo.ContextKeyHeaderAllow, tc.allowContextKey)
477568
}
@@ -494,6 +585,7 @@ func TestCorsHeaders(t *testing.T) {
494585
expected bool
495586
expectStatus int
496587
expectAllowHeader string
588+
whenPreflight bool
497589
}{
498590
{
499591
name: "non-preflight request, allow any origin, missing origin header = no CORS logic done",
@@ -536,7 +628,7 @@ func TestCorsHeaders(t *testing.T) {
536628
expectStatus: http.StatusOK,
537629
},
538630
{
539-
name: "preflight, allow any origin, missing origin header = no CORS logic done",
631+
name: "OPTIONS no origin, allow any origin = no CORS preflight short-circuit",
540632
originDomain: "", // Request does not have Origin header
541633
allowedOrigin: "*",
542634
method: http.MethodOptions,
@@ -552,9 +644,10 @@ func TestCorsHeaders(t *testing.T) {
552644
expected: true,
553645
expectStatus: http.StatusNoContent,
554646
expectAllowHeader: "OPTIONS, GET, POST",
647+
whenPreflight: true,
555648
},
556649
{
557-
name: "preflight, allow any origin, missing origin header = no CORS logic done",
650+
name: "OPTIONS no origin, allow specific origin = no CORS preflight short-circuit",
558651
originDomain: "", // Request does not have Origin header
559652
allowedOrigin: "http://example.com",
560653
method: http.MethodOptions,
@@ -570,6 +663,7 @@ func TestCorsHeaders(t *testing.T) {
570663
expected: false,
571664
expectStatus: http.StatusNoContent,
572665
expectAllowHeader: "OPTIONS, GET, POST",
666+
whenPreflight: true,
573667
},
574668
{
575669
name: "preflight, allow specific origin, matching origin header = CORS logic done",
@@ -579,6 +673,7 @@ func TestCorsHeaders(t *testing.T) {
579673
expected: true,
580674
expectStatus: http.StatusNoContent,
581675
expectAllowHeader: "OPTIONS, GET, POST",
676+
whenPreflight: true,
582677
},
583678
}
584679

@@ -605,6 +700,9 @@ func TestCorsHeaders(t *testing.T) {
605700
if tc.originDomain != "" {
606701
req.Header.Set(echo.HeaderOrigin, tc.originDomain)
607702
}
703+
if tc.whenPreflight {
704+
req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodGet)
705+
}
608706

609707
// we run through whole Echo handler chain to see how CORS works with Router OPTIONS handler
610708
e.ServeHTTP(rec, req)
@@ -658,11 +756,14 @@ func Test_allowOriginFunc(t *testing.T) {
658756
const origin = "http://example.com"
659757

660758
e := echo.New()
759+
// These OPTIONS cases exercise preflight AllowOriginFunc behavior, so each
760+
// request includes Access-Control-Request-Method.
661761
for _, allowOriginFunc := range allowOriginFuncs {
662762
req := httptest.NewRequest(http.MethodOptions, "/", nil)
663763
rec := httptest.NewRecorder()
664764
c := e.NewContext(req, rec)
665765
req.Header.Set(echo.HeaderOrigin, origin)
766+
req.Header.Set(echo.HeaderAccessControlRequestMethod, http.MethodGet)
666767
cors := CORSWithConfig(CORSConfig{
667768
AllowOriginFunc: allowOriginFunc,
668769
})

0 commit comments

Comments
 (0)