Skip to content
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

Skip request if origin is not allowed #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kurayama
Copy link

Requests are currently going through even if the origin is not allowed.

@codecov
Copy link

codecov bot commented Mar 30, 2017

Codecov Report

Merging #34 into v2.x will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff         @@
##           v2.x    #34   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         1      1           
  Lines        54     54           
  Branches     19     19           
===================================
  Hits         54     54
Impacted Files Coverage Δ
index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a396989...11abb36. Read the comment docs.

@kurayama kurayama closed this Mar 30, 2017
@kurayama kurayama reopened this Mar 30, 2017
@ruimarinho
Copy link

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

@ruimarinho
Copy link

ping @dead-horse

@@ -59,7 +59,7 @@ module.exports = function(options) {
// FIXME: origin can be promise
origin = options.origin(ctx);
if (!origin) {
return next();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a breaking change here, this will make requests which not from browser all failed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dead-horse isn't that covered by L53 already?

@fengmk2 fengmk2 changed the base branch from v2.x to master June 16, 2017 08:25
@mrtbld
Copy link

mrtbld commented Sep 14, 2017

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

In case a server does not wish to participate in the CORS protocol, its HTTP response to the CORS or CORS-preflight request must not include any of the above headers. The server is encouraged to use the 403 status in such HTTP responses.

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

@mrtbld
Copy link

mrtbld commented Sep 15, 2017

@ruimarinho

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.

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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ruimarinho
Copy link

So in both case it is safe even without skipping anything. As long as the server respects HTTP methods semantics.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants