-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CORS: the middleware misclassifies all OPTIONS requests as preflight requests #2534
Comments
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 ( |
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 |
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.
|
@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: 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 |
Sorry, I meant CORS everywhere here where I mentioned CSRF. I'll edit these comment to use correct term, which I actually meant. |
Reasons why I do not think that deciding to run CORS middleware should have feature describe in this issue: In no particular order:
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)
}
})
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. |
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 func isPreflight(r *http.Request) bool {
return r.Method == http.MethodOptions &&
r.Header.Get("Origin") != "" &&
r.Header.Get("Access-Control-Request-Header") != ""
}
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.
That's true; there could be a middleware further down the chain that does something interesting with
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).
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. |
I after good night sleep I took another look into this thing. Basically we are talking at the moment of this block Lines 202 to 207 in 98a5237
Lets not delve into 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 but this has a potential problem to cause problems with existing applications. See this comment and reasoning why we do not always call Lines 187 to 189 in 98a5237
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
|
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. |
@aldas Ok, but then I would say that, in order to allow unauthenticated |
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. |
Note that I'm referring not just to non-CORS 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 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. |
Issue Description
Echo's CORS middleware misclassifies all
OPTIONS
requests as preflight requests, thereby unduly preventing requests from hitting user-registeredOPTIONS
endpoints.I've previously discussed the general problem on my personal blog and how Echo suffers from it in issue #2510.
Checklist
Expected behaviour
(For this section and the next, please refer to the server code below.)
Consider the
OPTIONS
requests resulting from the following twocurl
commands:curl -v -XOPTIONS \ -H "Origin: https://example.com" \ localhost:8080/hello
According to the Fetch standard, neither request is a preflight request, because
Origin
header and anAccess-Control-Request-Method
header, andAccess-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:In contrast, an
OPTIONS
requests resulting from the followingcurl
command is a bona fide preflight request; as such, it should be (and is) intercepted and handled by the CORS middleware:Actual behaviour
The first two aforementioned
OPTIONS
requests get intercepted and handled by the CORS middleware rather than by the handler registered onOPTIONS /hello
.Steps to reproduce
curl
(see commands above).Working code to debug
Version/commit
Echo v4.11.2. See my
go.mod
below:The text was updated successfully, but these errors were encountered: