montagejs / joey

An HTTP content negotiation client and server JavaScript library, inspired by Sinatra and JSGI, and using Q promises.
Other
40 stars 7 forks source link

Allowing CORS requests #9

Open wmertens opened 10 years ago

wmertens commented 10 years ago

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

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 commented 10 years ago

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?

wmertens commented 10 years ago

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/montagejs/joey/issues/9#issuecomment-29637245 .

Stuk commented 10 years ago

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

wmertens commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

kriskowal commented 10 years ago

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