Dynamic CORS handling based on path params

[bakatz]

Discussed in #2509

^{Originally posted by bakatz August 21, 2023}
Problem: CORSConfig (in the cors middleware) allows a function (AllowOriginFunc) to be used to determine if a given origin is allowed. However, that function is limited as it only includes a single parameter: the path. In my particular situation, I'm building an authentication service that needs to use a certain parameter in the request's path parameters (c.Param("something")) to determine which URLs are allowed for CORS purposes. This is impossible with the AllowOriginFunc not having the context in the function signature.

Solution:

1. Add echo.Context to the AllowOriginFunc signature.
2. To all echo.POST/GET/etc functions, add a new flag i.e. useSamePathParamsForOptions which auto generates an options route with the same path. Currently there's a bug or potential "by design" issue where path params are not evaluated unless there's a matching route, so you have to manually add an echo.OPTIONS(...) call for each route to generate the route for CORS purposes if you want to access path variables.

If the maintainers like this solution, I'll add a PR - I've already implemented it in my own repo and have tests for it, so it's very easy for me to add it to the echo project as well.

Maybe something that can/should go in echo v5, let me know.

[jonasps]

Hi, you have an interesting problem, tough I believe that the CORS settings should only depend on the value from origin and nothing else. Mabey there is a better way of restricting the access to your application instead of CORS setting?

[aldas]

I'm building an authentication service that needs to use a certain parameter in the request's path parameters (c.Param("something")) to determine which URLs are allowed for CORS purposes.

To be honest - I probably do not understand exactly the use-case but if we are talking about authentication I think you can achieve this by adding your authentication middleware before CORS middleware and detect things that you need.

or you could conditionally execute cors like that

func main() {
	e := echo.New()

	e.Use(func(next echo.HandlerFunc) echo.HandlerFunc {
		cors := middleware.CORS()
		return func(c echo.Context) error {
			param := c.Param("something")
			if param == "nope" { // stop middleware/handler chain
				return echo.ErrUnauthorized
			}
			if param == "skip" {
				return next(c) // execute next middleware without CORS
			}
			return cors(next)(c) // execute CORS and then next middleware
		}
	})

	e.GET("/:something", func(c echo.Context) error {
		return c.String(http.StatusOK, "Hi")
	})

	if err := e.Start(":8080"); err != nil && !errors.Is(err, http.ErrServerClosed) {
		e.Logger.Fatal(err)
	}
}

[jub0bs]

@bakatz I strongly believe that custom callbacks are a bad idea for CORS middleware. Why not simply be more selective as to which paths you apply the CORS middleware to at the router level?

@aldas You write

you can achieve this by adding your authentication middleware before CORS middleware

However, since preflight requests are not themselves authenticated, CORS middleware must in general come before any authentication middleware.

[aldas]

I would say - everything depends on the situation. Most of the time frameworks do not try to guess user intentions/requirements. If it were so then the CORS middleware would be mandatory/added implicitly.

CORS middleware must in general come before any authentication middleware.

there is always "skip" functionality you can add for cases when methodtype=OPTIONS as these request do not contain auth.

I really do not know what @bakatz is building, but it could be some kind of highly dynamic virtual host supporting server and dynamically changing routes application with "weird" requirements - in that case when you are sure what you are doing you can bend the middleware/handler chain by adding conditions.

Problem with (web)frameworks is that there are so many different (valid or not) user requirements so eventually every single line is configurable and writing configuration could be longer than writing your middleware from the scratch.

[jub0bs]

If it were so then the CORS middleware would be mandatory/added implicitly

Why? Why not adopt secure defaults (i.e. no CORS configuration whatsoever) instead?

there is always "skip" functionality you can add for cases when methodtype=OPTIONS as these request do not contain auth.

Careful, though: not all OPTIONS requests are preflight requests, and some OPTIONS requests may very well carry credentials.

I had to check but the (erroneous) assumption that all OPTIONS requests are preflight requests is present in the CORS middleware. Perhaps I should open a separate issue about that...

[aldas]

@jub0bs if there is a case where request is considered incorrectly preflight please create issue (preferably with example).

currently it consider method + origin although Mozilla says

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

but this probably is not something catastrophically wrong with that as long as browser requests are covered and non-browser calls (api2api etc) do not send "origin" nothing serious happens as CORS is important for browsers and OPTIONS for API2API calls is quite uncommon or we would have tickets about that. We usually have tickets that CORS does not work when CURLing application etc.

[bakatz]

I'm just going to close this - the solution I implemented already works and after some further consideration I don't think it's appropriate to add this to the echo codebase anyway, as it's a problem unique to what I'm doing.

Thanks for the feedback.

[jub0bs]

@aldas The absence of ticket about this doesn't guarantee that the implementation is sound. I'll create a separate issue (with examples) when time allows.