CORS: the middleware misclassifies all OPTIONS requests as preflight requests

[jub0bs]

Issue Description

Echo's CORS middleware misclassifies all OPTIONS requests as preflight requests, thereby unduly preventing requests from hitting user-registered OPTIONS endpoints.

I've previously discussed the general problem on my personal blog and how Echo suffers from it in issue #2510.

Checklist

• Dependencies installed
• No typos
• Searched existing issues and docs

Expected behaviour

(For this section and the next, please refer to the server code below.)

Consider the OPTIONS requests resulting from the following two curl commands:

curl -v -XOPTIONS \
  localhost:8080/hello

curl -v -XOPTIONS \
  -H "Origin: https://example.com" \
  localhost:8080/hello

According to the Fetch standard, neither request is a preflight request, because

• the first one lacks both an Origin header and an Access-Control-Request-Method header, and
• the second one lacks an Access-Control-Request-Method header.

Therefore, those requests should get through the CORS middleware, exercise the handler registered on /hello, and get a response of this kind:

HTTP/1.1 204 No Content
Allow: GET, OPTIONS
Date: Mon, 23 Oct 2023 12:30:33 GMT

In contrast, an OPTIONS requests resulting from the following curl command is a bona fide preflight request; as such, it should be (and is) intercepted and handled by the CORS middleware:

curl -v -XOPTIONS \
  -H "Origin: https://example.com" \
  -H "Access-Control-Request-Method: PUT" \
  localhost:8080/hello

HTTP/1.1 204 No Content
Access-Control-Allow-Methods: PUT
Access-Control-Allow-Origin: *
Vary: Origin
Vary: Access-Control-Request-Method
Vary: Access-Control-Request-Headers
-snip-

Actual behaviour

The first two aforementioned OPTIONS requests get intercepted and handled by the CORS middleware rather than by the handler registered on OPTIONS /hello.

Steps to reproduce

1. Run the program to start the server.
2. Exercise it via curl(see commands above).

Working code to debug

package main

import (
	"errors"
	"net/http"

	"github.com/labstack/echo/v4"
	"github.com/labstack/echo/v4/middleware"
)

func main() {
	e := echo.New()
	e.Use(middleware.CORSWithConfig(middleware.CORSConfig{
		AllowOrigins: []string{"*"},
		AllowMethods: []string{http.MethodPut},
	}))
	e.GET("/hello", func(c echo.Context) error {
		return c.String(http.StatusOK, "Hello, World!")
	})
	e.OPTIONS("/hello", func(c echo.Context) error {
		c.Response().Header().Set("Allow", "GET, OPTIONS")
		c.Response().WriteHeader(http.StatusNoContent)
		return nil
	})
	if err := e.Start(":8080"); err != nil && !errors.Is(err, http.ErrServerClosed) {
		e.Logger.Fatal(err)
	}
}

Version/commit

Echo v4.11.2. See my go.mod below:

module whatever

go 1.21.3

require github.com/labstack/echo/v4 v4.11.2

require (
	github.com/golang-jwt/jwt v3.2.2+incompatible // indirect
	github.com/labstack/gommon v0.4.0 // indirect
	github.com/mattn/go-colorable v0.1.13 // indirect
	github.com/mattn/go-isatty v0.0.19 // indirect
	github.com/valyala/bytebufferpool v1.0.0 // indirect
	github.com/valyala/fasttemplate v1.2.2 // indirect
	golang.org/x/crypto v0.14.0 // indirect
	golang.org/x/net v0.17.0 // indirect
	golang.org/x/sys v0.13.0 // indirect
	golang.org/x/text v0.13.0 // indirect
	golang.org/x/time v0.3.0 // indirect
)

[aldas]

This is the use-case for middleware.CORSConfig.Skipper to skip these routes. Registering OPTIONS routes and expecting them to work with CORS middleware is a edge case for skipper.

p.s. middleware could be run even before routing is done (e.Pre()) so by current implementation middleware do not try to know where (route) they lead to. If there is a path(s) that must not be executed by certain middleware(s) then .Skipper should be used.

[jub0bs]

Ideally, there shouldn't be a need to use that skipper functionality in order to get the CORS middleware to work properly. I interpret such a need as a defect in Echo's current design. The standard library's ServeMux doesn't suffer from this problem. 🤷

[aldas]

well, there could be other middlewares that render the response after CORS middleware so there is not even guarantee that original handler, that was registered, would be executed. So this is definitely a rare edge case which is up to the developer to handle/decide what should happen.

.Skipper is handy way to avoid unnecessary complexity due to the rare edge cases. If we could start to assume that at the end of the handler chain is route registered for OPTIONS then we would create another edge case when there is a middleware that decides do end chain prematurely and render its own response.

[jub0bs]

@aldas Could you clarify which edge cases you're referring to? I'm not sure how this issue relates to CSRF, to be honest. In my opinion, CSRF protection (if any) should be handled after the CORS middleware has executed. Schematically: cors(csrf(handler)) rather than csrf(cors(handler)).

Edit: Ideally, I'd like to see a test that passes with the current behaviour but would fail if the CORS middleware was modified to correctly categorise OPTIONS requests.

[aldas]

Sorry, I meant CORS everywhere here where I mentioned CSRF. I'll edit these comment to use correct term, which I actually meant.

[aldas]

Reasons why I do not think that deciding to run CORS middleware should have feature describe in this issue:

In no particular order:

• Adding check into CORS middleware to check if at the end of handler chain is handler that was added by e.OPTIONS would add complexity which is unnecessary 99.999% cases. It makes things harder to maintain and adds minuscule performance hit for majority of use-cases. For these edge cases middleware.CORSConfig.Skipper exists. This come from Echo maintainer viewpoint - almost every middleware and method there is - has someone in the wild that has their special use-case which would need yet another if or else and it makes sense to direct people to general solutions/workaround like .Skipper.
• Every middleware could be run before routing has happened. For example: e.Pre(middleware.CORS()). At that time we do not know what route we are matching to. It could be that we do not match any route and some other middleware will stop middleware/handler chain execution (For example: authentication/authorization middlewares - if you decide to add them after CORS).
• It is not 100% certain that even with e.Use(middleware.CORS()) route added with e.OPTIONS() will be reached. It could be that some other middleware will stop middleware/handler chain execution (For example: authentication/authorization middlewares). Or something as silly as this:

	e.Use(middleware.CORS())
	e.Use(func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			// this is silly but any middleware after CORS could alter how request ends
			if strings.Contains(c.Request().Header.Get("Referer"), "google") {
				return echo.ErrUnauthorized
			}
			return next(c)
		}
	})

• Consider e.Any(). Should routes added with that be excluded from CORS? If e.OPTIONS is very explicit use from developer then e.Any is not. Should Echo assume developer intent in that case - and which one it should be?

Basically - Echo does not try to fix every problem in the world, it does not try to hold your hand for every case as it would add too much complexity to the codebase as there are far more "stranger" use-cases where people want middlewares to no to run. Echo does very little to guess developer intentions - is this deliberate design choice or something that evolved over the time - I can not say - this is just how it works.

[jub0bs]

@aldas

Adding check into CORS middleware to check if at the end of handler chain is handler that was added by e.OPTIONS would add complexity which is unnecessary 99.999% cases.

That's not what I'm suggesting, though. The CORS middleware would remain oblivious to the workings of middleware down the chain and the handler at the end of it.

However, the CORS middleware would inspect the request to determine whether it is indeed a preflight or not. The current implementation already checks whether the request's method is OPTIONS; all it should do is also check whether the request contains an Origin header and an Access-Control-Request-Header:

func isPreflight(r *http.Request) bool {
  return r.Method == http.MethodOptions &&
    r.Header.Get("Origin") != "" &&
    r.Header.Get("Access-Control-Request-Header") != ""
}

Every middleware could [...]

The onus is on users to stack their middleware properly. In the great majority of cases, if a CORS middleware is used, it should be the outermost middleware.

It is not 100% certain that even with e.Use(middleware.CORS()) route added with e.OPTIONS() will be reached.

That's true; there could be a middleware further down the chain that does something interesting with OPTIONS requests, but that's not the concern of the CORS middleware; see my first point above.

Consider e.Any(). [...]

I'm not sure I'm following you on this, but I suspect my answer would be related to my second point above (correct stacking of middleware).

Echo does not try to fix every problem in the world [...]

And nobody is asking it to. But Echo could make an effort to comply with established standards. You mention complexity, but Echo diverges from standards, forcing its users to jump through hoops (in the form of a skipper, perhaps) in order to get the behaviour they rightfully expect; this is a source of accidental complexity.

[aldas]

I after good night sleep I took another look into this thing. Basically we are talking at the moment of this block

echo/middleware/cors.go

Lines 202 to 207 in 98a5237

	if origin == "" {
	if !preflight {
	return next(c)
	}
	return c.NoContent(http.StatusNoContent)
	}

Lets not delve into preflight variable which has incorrect name, it should be isOptions or something like that.
We could change it to something like that:

			if origin != "" {
				return next(c)
			}

In that cases all non Cross-Origin requests (does not mater if it is OPTIONS or not) will continue into next middleware/handler. Therefore, if there is developer added e.OPTIONS() route it will be executed or default options handler which was added in Echo v4.7.0 Allow header support in Router + default options handler will kick in and send 204 no content response

but this has a potential problem to cause problems with existing applications. See this comment and reasoning why we do not always call next(c)

echo/middleware/cors.go

Lines 187 to 189 in 98a5237

	// 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).

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).

Problem is that instead of 204s these applications would see 401 now. This is probably OK.

---

I am omitting here question if CORS middleware should check for per standard preflight or not.

Preflight request is an OPTIONS request, using two or three HTTP request headers: Origin ,Access-Control-Request-Method, and optionally Access-Control-Request-Headers

[aldas]

I am being cautious here - dealing with widely used and old code base is mostly choosing if change can be at all done. We are trying to have stable environment for users and often can not afford radical changes - even seemingly simple as this. This is vastly different from new/smaller libraries.

[jub0bs]

@aldas Ok, but then I would say that, in order to allow unauthenticated OPTIONS requests (if that's indeed the desired behaviour), the auth middleware should be applied more selectively (to some, not all, routes). In any case, catering for this use case in the CORS middleware seems to indicate poor separation of concerns. I can understand how fixing this in Echo would be too much effort and potentially break existing apps, though.

[aldas]

one thing here is that OPTIONS request are most of the time unauthenticated when application is using cookies as cookies are not send by browser for preflight requests.

The impact for this change I think would mostly be in visible in your monitoring stack and some of the 204 responses would be now 401s. Functionally there probably is no difference. As we are talking at the moment non-Cross-origin OPTIONS request - these can not be from browser as web-browsers have their CORS security set, so it leave only API-2-API requests.

[jub0bs]

As we are talking at the moment non-Cross-origin OPTIONS request [...]

Note that I'm referring not just to non-CORS OPTIONS requests, but also to non-preflight CORS OPTIONS requests, e.g. requests issued with

fetch('https://example.com', {method: 'OPTIONS'})

Note that such requests may well be authenticated:

fetch('https://example.com', {method: 'OPTIONS', credentials: 'include'})

In my original post, those non-preflight CORS OPTIONS requests are captured by the following "test case":

curl -v -XOPTIONS -H "Origin: https://example.com" localhost:8080/hello

Those requests get intercepted by Echo's CORS middleware and treated as preflight even though they're not.

Anyway, even though you may think I'm splitting hairs, I think Echo v5 presents you with an opportunity to shake things up and have a more disciplined CORS middleware.