swagger-express-middleware icon indicating copy to clipboard operation
swagger-express-middleware copied to clipboard

Add new middleware to handle non-CORS OPTIONS requests

Open JamesMGreene opened this issue 8 years ago • 12 comments

I'd like to see a new middleware added to handle non-CORS OPTIONS requests by responding 200 OK with an Allow response header detailing which HTTP methods are supported for that path.

This should be easy to add as that same logic is already used to construct the Access-Control-Allow-Methods response header in the CORS middleware today: lib/cors.js#L50-L53

This new middleware should only end the response if there isn't an actual OPTIONS route configured in the associated Swagger spec. It should add the Allow response header no matter what, though.

JamesMGreene avatar Mar 16 '16 16:03 JamesMGreene

Advanced Feature Idea (Extending the Original Idea): There could also be a config option for this new middleware to enable returning the JSON spec for each HTTP method based on the associated Swagger spec, a la the section "The response body and API documentation" from this intriguing blog post.

JamesMGreene avatar Mar 16 '16 16:03 JamesMGreene

While this is a cool idea, and I like the blog post you linked to, I don't think this falls under the scope of what I want to include in Swagger Express Middleware. At least not unless Swagger starts incorporating Hypermedia and HATEOAS into the spec itself. Maybe it could go in a separate npm package called something like swagger-hypermedia-middleware.

However, it won't be hard for you to write this middleware yourself. The metadata middleware decorates the Request object with all the Swagger metadata for each request, so as long as your middleware comes after the metadata middleware, you'll be able to use that metadata to set whatever headers and/or response body you want - even for OPTIONS requests. (as long as your middleware comes before the cors middleware)

JamesMessinger avatar Mar 16 '16 16:03 JamesMessinger

Good point about me just making that middleware myself, though I do still see value in the first comment's core functionality (the non-hypermedia part) being a part of this module. Sure you're not open to that part?

If not, that's fine, and I'll create my own module to extend this middleware. shrugs

JamesMGreene avatar Mar 16 '16 16:03 JamesMGreene

For the first part... I don't like the idea of creating a CORS-ish middleware for non-CORS requests. Seems like a pretty specific use case and not general-purpose enough for a Swagger library.

However, I could totally be open to adding an option to the existing CORS middleware to have it not automatically send the response. If that option is set, then the CORS middleware would just set the CORS headers (if the request is a CORS request) and would then pass-through to the next middleware, which would give you an opportunity to perform additional logic and send your own custom response.

JamesMessinger avatar Mar 16 '16 16:03 JamesMessinger

To clarify, I'm just suggesting that there be a new middleware to handle responding to [non-CORS preflight] OPTIONS requests in the standard/expected RESTful way:

  • Responding with 200 OK status
  • Including a response header named Allow that details all of the possible HTTP methods that can be accepted at that path, e.g. Allow: GET, POST, PUT, DELETE, OPTIONS, based on the associated Swagger spec.

This seems very general purpose as it is just a normal [but oft-overlooked] standard response to an OPTIONS request for HTTP/RESTful services, which pre-dates the creation of CORS.

JamesMGreene avatar Mar 16 '16 17:03 JamesMGreene

P.S. Once again, I'm fine creating it myself but it seems like a pretty easy and standard functionality that would fit well in this collection of middleware since it is very easy to accomplish using the associated Swagger spec, just like you do in the CORS middleware.

Just want to make sure you know what I am referring to since I get the impression we're not on the same page.

JamesMGreene avatar Mar 16 '16 17:03 JamesMGreene

To clarify, I'm just suggesting that there be a new middleware to handle responding to [non-CORS preflight] OPTIONS requests in the standard/expected RESTful way:

  • Responding with 200 OK status
  • Including a response header named Allow that details all of the possible HTTP methods that can be accepted at that path, e.g. Allow: GET, POST, PUT, DELETE, OPTIONS, based on the associated Swagger spec.

Isn't that what the CORS middleware does today? It (mistakenly) responds to all OPTIONS requests - even non-CORS ones - with the Allow header set, based on the Swagger spec

JamesMessinger avatar Mar 16 '16 17:03 JamesMessinger

The CORS middleware currently sets the Access-Control-Request-Method response header (e.g. Access-Control-Request-Method: GET, POST) in this fashion for any HTTP request [with a CORS Origin request header (#41, #42)] -- not just for HTTP OPTIONS requests.

The proposed middleware would set the Allow response header (e.g. Allow: GET, POST, OPTIONS) in the same fashion for all HTTP OPTIONS requests -- at least non-CORS requests, though you could also include CORS preflight requests without any known detriment.

JamesMGreene avatar Mar 16 '16 18:03 JamesMGreene

Oh! NOW I see what you mean! Sorry... I guess I'm not firing on all cylinders today. Thanks for persevering.

I honestly thought the CORS middleware was already doing that. But I was mixing-up the Access-Control-Allow-Method header and the Allow header.

Yes, I 100% agree that automatically setting the Allow header is a common use case and certainly generic enough for this library to handle. The only question now is whether it should be its own middleware, or just be part of the CORS middleware. That is, if the request is a CORS request (i.e. it has an Origin header), then the CORS middleware would set the Access-Control-Allow-Method header and immediately send the response, as it does today. But if the request is not a CORS request, then the CORS middleware would just set the Allow header and then call next()

JamesMessinger avatar Mar 16 '16 18:03 JamesMessinger

I think it feels a little strange to have to enable the CORS middleware if you really just wanted the normal OPTIONS responses but did not want to enable CORS due to security reasons.

JamesMGreene avatar Mar 16 '16 18:03 JamesMGreene

Yeah, agreed. I was just trying to be lazy. :)

So what should this new middleware be called? middleware.allow() would certainly work, but maybe that's too narrow-focused? Maybe it should be more generic, like middleware.setHeaders(), so it could be used to set other commonly-needed headers in the future? Can you think of any other headers besides Allow that would be commonly needed and could be inferred from a Swagger API?

JamesMessinger avatar Mar 16 '16 18:03 JamesMessinger

I'm pretty new to Swagger (but not HTTP/REST). That said, no other common HTTP response headers spring to mind that aren't either already set by Express internally (Content-Type, Content-Length, ETag, Vary, etc.) or else requiring customized values based on coded non-Swagger logic.

Probably the next most common HTTP response headers that are not dealt with would be those related to caching (Cache-Control, Expires, Pragma (+ ETag, Vary)) but I don't think there are any obvious defaults for those. :question:

After those, maybe HSTS (Strict-Transport-Security)...? But again, doesn't seem like there is a sensible default value for that, and setting that unbeknownst to those serving the site can actually end up causing some headaches. :confused:

JamesMGreene avatar Mar 16 '16 19:03 JamesMGreene