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

Allowing CORS requests #9

Open
wmertens opened this issue Dec 2, 2013 · 7 comments
Open

Allowing CORS requests #9

wmertens opened this issue Dec 2, 2013 · 7 comments
Assignees

Comments

@wmertens
Copy link
Contributor

wmertens commented Dec 2, 2013

I wrote this middleware to allow all CORS requests, just add .use(AllowAllCors) where you want it in the chain. I think this should be one of the default Apps, perhaps with configurability about what requests are allowed. Thoughts?

It's not super-tested but it seems to work 😊

var AllowAllCors;

AllowAllCors = function(next) {
  return function(request, response) {
    var h, hasCors, myHeaders;
    myHeaders = {};
    hasCors = false;
    if (h = request.headers.origin) {
      myHeaders['Access-Control-Allow-Origin'] = h;
      hasCors = true;
    }
    if (h = request.headers['access-control-request-method']) {
      myHeaders['Access-Control-Allow-Methods'] = h;
      hasCors = true;
    }
    if (h = request.headers['access-control-request-headers']) {
      myHeaders['Access-Control-Allow-Headers'] = h;
      hasCors = true;
    }
    if (hasCors) {
      myHeaders['Access-Control-Max-Age'] = 60 * 60 * 24 * 365;
    }
    if (hasCors && request.method === 'OPTIONS') {
      response.status = 200;
      response.headers = myHeaders;
      return response;
    } else {
      return Q.when(next.apply(this, arguments), function(response) {
        var key;
        if (response) {
          if (response.headers) {
            for (key in myHeaders) {
              if (!response.headers[key]) {
                response.headers[key] = myHeaders[key];
              }
            }
          } else {
            response.headers = myHeaders;
          }
          return response;
        }
      });
    }
  };
};
@Stuk
Copy link
Contributor

Stuk commented Dec 2, 2013

There is actually a cors middleware already:

joey/joey.js

Line 126 in 75dfe2a

chain.cors = function (origin, methods, headers) {
(although it isn't documented). Does this do what you need?

@wmertens
Copy link
Contributor Author

wmertens commented Dec 3, 2013

No, that doesn't work because it doesn't respond with the origin; when you
simply return '*' you can't do authenticated CORS requests.
On Dec 2, 2013 6:15 PM, "Stuart Knightley" notifications@github.com wrote:

There is actually a cors middleware already:
https://github.com/montagejs/joey/blob/75dfe2a396898b720a9a295645c8f9cb6ecad868/joey.js#L126(although it isn't documented). Does this do what you need?


Reply to this email directly or view it on GitHubhttps://github.com//issues/9#issuecomment-29637245
.

@Stuk
Copy link
Contributor

Stuk commented Dec 3, 2013

That feature is by design, as allowing authenticated cross origin requests from any origin is a security issue.

@wmertens
Copy link
Contributor Author

wmertens commented Dec 3, 2013

Isn't it up to the application owner to decide if it is a security issue?

Even then, if you want to allow more than one origin, you will need to return something parametrized as above since you can only specify one origin. Perhaps adding a regex to the middleware would help? E.g. something like .use(AllowAllCorsFor(/^https?://[^\/]*company.com//)) or .use(AllowAllCorsFor({host:/.*company.com$/, path:/^interface\//}))?

@Stuk
Copy link
Contributor

Stuk commented Dec 3, 2013

Yep, you are correct that the app owner can decide what they do and do not want to allow. However, the people that designed CORS, who know a lot more about security than I do, have specifically disallowed that case and so I am very reluctant to add a feature to Joey that makes it easy. An app owner that understands the consequences can always explicitly write code to make it possible, as you have done.

While reading the spec I noticed that for the case where one wants authenticated access to resources from any origin the authors suggest using an access token explicitly passed with each request, as in OAuth. See the numbered lists in the Security section.

If you want to allow more than one origin you can give a space-separated list of origins to the cors middleware/access-control-allow-origin header.

@wmertens
Copy link
Contributor Author

wmertens commented Dec 6, 2013

On the other hand, a package like https://github.com/antono/connect-cors already allows you to make "*" return the correct string.

@kriskowal
Copy link
Member

@wmertens that’s a great observation. We’ll try to find some time to study both solutions and make some changes to our existing .cors() app.

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

No branches or pull requests

3 participants