Automatic OPTIONS responses were removed

[williamkapke]

A recent checkin (d1366f2) stopped Restify's auto handling of OPTIONS requests. We now have the ability to create them manually.

Although more control is good, manually adding OPTIONS for every endpoint is cumbersome and (if your application relies on CORS requests) can make it hard to move to Restify v2.

I propose that the auto generated OPTIONS responses are "on by default" and developers can override the default by specifying server.opts(...). It would probably also be good to have a way to globally disable them.

[ricardograca]

You don't have to add a handler for OPTIONS on every endpoint. You have two options. The first one is to use the built-in plugin that restores the behavior you're looking for:

server.use(restify.fullResponse())

The other (which I prefer since it affords more control) is to listen for the "MethodNotAllowed" event from the server like so:

// This is a simplified example just to give you an idea
// You will probably need more allowed headers

function unknownMethodHandler(req, res) {
  if (req.method.toLowerCase() === 'options') {
    var allowHeaders = ['Accept', 'Accept-Version', 'Content-Type', 'Api-Version'];

    if (res.methods.indexOf('OPTIONS') === -1) res.methods.push('OPTIONS');

    res.header('Access-Control-Allow-Credentials', true);
    res.header('Access-Control-Allow-Headers', allowHeaders.join(', '));
    res.header('Access-Control-Allow-Methods', res.methods.join(', '));
    res.header('Access-Control-Allow-Origin', req.headers.origin);

    return res.send(204);
  }
  else
    return res.send(new restify.MethodNotAllowedError());
}

server.on('MethodNotAllowed', unknownMethodHandler);

[williamkapke]

I should have mentioned there are line comments in d1366f2 that give more background on this.

[ricardograca]

Ok, but the behavior of version 1.4 in regards to OPTIONS requests was very similar to the second option I posted. In fact I think I copied it from that version. Does it make so much difference to you that you have to define the unknown method handler function yourself to handle OPTIONS requests? If an endpoint does have a handler for OPTIONS this function will be overridden, so I think that covers all cases right?

[williamkapke]

Nope, it doesn't make a difference to me.

Yup, seems to handle the cases. Which is why I suggested it in the line comment.

Basically this issue was added to bring awareness and because @mcavage is considering baking it back in & asked if I'd make an issue so this topic was tracked. If he decides to go the unknownMethodHandler route- he'll let the world know on this thread and close the issue. :)

[mcavage]

Yeah - this has already come up enough times in issues and offline emails to me that I'm going to make it "first class". I'm not sure yet what I want it to look like, but I'm probably going to have something like: server.use(restify.cors()), or some such thing.

[mcavage]

Ok - I dropped support back into #master - I would actually like some feedback from you all on whether this is what you want, before I ship it out via NPM. Here's a (stupidly simple) sample, and semantics are explained below:

var restify = require('restify');

function foo(req, res, next) {
        res.send(204);
        next();
}

var srv = restify.createServer();
srv.use(restify.CORS());

srv.put('/foo', foo);
srv.get('/foo', foo);
srv.del('/foo', foo);

srv.listen(8080);

So, to get "simple/actual" request headers for CORS flying, you'll need to use server.use(restify.CORS()) - and this will only take effect for requests that have an origin header. The defaults are pretty sane (I think), which is to set Access-Control-Allow-Origin to * not set Access-Control-Allow-Credentials and to default Access-Control-Expose-Headers to the stuff restify relies on (mainly, versioning). You can override these with at plugin creation time:

server.use(restify.CORS({
    origins: ['foo.com', 'a.foo.com', 'bar.com', ...],
    headers: ['x-ping', 'x-other', ...]
}));

For preflight requests, the internals of restify automatically handle this, similar to the way it did in 1.4, with the exception you can override on a per-URL basis with server.opts, in which case the restify logic won't kick in, and it's on you to do the right thing for CORS.

For the example above, here's some sample curl request showing what happens:

GET request with no Origin header (so no CORS):

$ curl -isS http://127.0.0.1:8080/foo
HTTP/1.1 204 No Content
Date: Sat, 26 Jan 2013 18:45:24 GMT
Connection: keep-alive

Tack on Origin:

$ curl -isS http://127.0.0.1:8080/foo -H 'origin: foo.com'
HTTP/1.1 204 No Content
Access-Control-Allow-Origin: *
Access-Control-Expose-Headers: api-version, content-length, content-md5, content-type, date, request-id, response-time
Date: Sat, 26 Jan 2013 18:47:27 GMT
Connection: keep-alive

Here's a bad preflight request (note no access-control-request-method), so it 404s:

$ curl -isS http://127.0.0.1:8080/foo -H 'origin: foo.com' -X OPTIONS
HTTP/1.1 405 Method Not Allowed
Allow: PUT, GET, DELETE, POST
Content-Type: application/json
Content-Length: 67
Date: Sat, 26 Jan 2013 18:48:06 GMT
Connection: keep-alive

{"code":"MethodNotAllowedError","message":"OPTIONS is not allowed"}

And a proper preflight request:

$ curl -isS http://127.0.0.1:8080/foo -H 'origin: foo.com' -X OPTIONS -H 'access-control-request-method: GET'
HTTP/1.1 200 OK
Allow: PUT, GET, DELETE, POST
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: PUT, GET, DELETE, POST
Access-Control-Allow-Headers: accept-version, content-type, request-id, x-api-version, x-request-id
Access-Control-Max-Age: 3600
Date: Sat, 26 Jan 2013 18:49:00 GMT
Connection: keep-alive
Transfer-Encoding: chunked

Play around some, and let me know how that gets on.

[benjie]

Adding headers to the restify.CORS({headers:[...]}) option does not add them to ALLOW_HEADERS, thus they are rejected by this loop.

[benjie]

I think var headers = req.headers['access-control-request-headers']; is a string, so (headers || []).forEach( fails when you request headers - I've changed it to (headers || "").toLowerCase().split(", ").forEach( in my own code, which has stopped it producing errors just there. I would have submitted a pull request, but I'm so new to this code I'm not really sure what form or intent is preferred by the authors.

[ricardograca]

Small problem. Are you sure the access-control-request-headers header will always be separated with ", " and not ","?

[benjie]

@ricardograca I believe this is what the RFCs/specs state, but I haven't checked. You can .split(/, */) if you prefer. I've never seen them without the spaces.

[benjie]

It might be worth adding origin and accept to the default ALLOW_HEADERS too?

[mcavage]

Sorry for the delay, I was traveling.

@benjie - this loop is out of scope of what is passed into headers for the plugin - that's what I was saying above - the default for preflight requests is like what restify 1.4 was - you will need to override that with a custom server.opts(...) definition(s). And yes, good catch on the access-control-request-headers being a string, I'll fix that up - this also needs a bunch of regression tests, but I was in a hurry, and wanted to get people the ability to use it. Anyway - origin you definitely don't want, as the browser is going to send that anyway, and I believe accept is in the same boat, but I'm not 100% sure w/o validating in a bunch of browsers - I can just put it in :)

@ricadograca/@benjie - they can be separated either way, I use this foo.split(/\s*,\s*/) in most places to parse header values.

[benjie]

you will need to override that with a custom server.opts(...) definition(s)

Okay, I'll take a look at doing that :)

Anyway - origin you definitely don't want, as the browser is going to send that anyway, and I believe accept is in the same boat, but I'm not 100% sure w/o validating in a bunch of browsers - I can just put it in :)

My apologies, you're right of course - I got confused and added the unnecessary headers to my access-control-request-headers in requests 😫