Skip request if origin is not allowed #34
Conversation
Codecov Report
@@ Coverage Diff @@
## v2.x #34 +/- ##
===================================
Coverage 100% 100%
===================================
Files 1 1
Lines 54 54
Branches 19 19
===================================
Hits 54 54
Continue to review full report at Codecov.
|
|
@fengmk2, unless I'm missing something, this seems like a security concern as the request will be processed even in the case of CORS not being allowed. Either everyone is aware that there has to be another middleware which tests for the existence of the header and skips route processing or independently of the origin validation the route logic will still execute. |
|
ping @dead-horse |
| @@ -59,7 +59,7 @@ module.exports = function(options) { | |||
| // FIXME: origin can be promise | |||
| origin = options.origin(ctx); | |||
| if (!origin) { | |||
| return next(); | |||
There was a problem hiding this comment.
I think it is a breaking change here, this will make requests which not from browser all failed.
There was a problem hiding this comment.
@dead-horse isn't that covered by L53 already?
|
For what it's worth, I think it would more correct for this feature to be optional and to use status 403 instead of 404. The CORS protocol standard says (emphasis mine):
The first part describes the current behavior. It seems that this PR address the second par (in bold). The wording "is encouraged" suggest that it's optional, i.e. not required for the CORS protocol to function correctly. Then again I'm not really sure "Skip request if origin is not allowed" relates to "In case a server does not wish to participate in the CORS protocol". |
Actually a browser that uses the CORS protocol will always use a CORS-preflight request (OPTIONS) before attempting a non GET/HEAD request (i.e. requests with side effects), and will abort if the response headers don't allow it to proceed (in which case the POST/PUT/DELETE request is not sent). For GET and HEAD, the actual request is sent without any prior CORS-preflight request because they should only retrieve data and have no side effects; the browser will still block the response content from being accessed by the requester if the server doesn't allow it. So in both case it is safe even without skipping anything. As long as the server respects HTTP methods semantics. Ultimately, CORS relies on browser compliance and isn't meant to replace request authorisation; it's just a safe and standard way to break free of the strict same-origin policy implemented by browsers. |
| @@ -99,7 +99,7 @@ module.exports = function(options) { | |||
| // The request is outside the scope of this specification. | |||
| if (!ctx.get('Access-Control-Request-Method')) { | |||
| // this not preflight request, ignore it | |||
| return next(); | |||
| return; | |||
There was a problem hiding this comment.
@kurayama I think this is debatable and if the spec says it's of scope, then it is my understanding we should let the request pass on to the next middleware.
It's still a waste of resources processing GET/HEAD requests if the origin is invalid and the browser won't be displaying the response anyway. |
Requests are currently going through even if the origin is not allowed.