Seems incompatible with ring-cors

[livtanong]

Minimal repro case. README contains repro instructions.
https://github.com/levitanong/repro-reitit-cors

Ring-cors automatically accepts OPTIONS and specially treats preflight requests. Currently, the only way i've found to make it work is by wrapping the ring-handler itself, circumventing the idiomatic :middleware keys. The trade off is, the metadata is lost, and to preserve some functionality, we must extract that metadata and with-meta the wrapped handler with the extracted metadata.

[ikitommi]

Hi, and thanks for the repro. Reitit takes a match-first approach which causes the issue here. The ring-handler first checks if there is a route matching the request :uri and request-method and only after then applies the middleware chain and the handler.

The repro routing tree doesn't have an :options method for the /v1/foo endpoint, so the route doesn't match and the middleware is not run. For things like cors, this is not that good. I guess the options are:

0 - document the route-first approach properly

• "use ring-cors outside of ring-router"
• "to use ring-cors within reitit, add manually (or programmatically) the :options endpoints into your route tree"
• works, but kinda lame

1 - automatically create the :options endpoints for (all) routes

• for both reitit-ring and reitit-http
• would be easy to implement, just a default in the reitit-ring compile-hook
• what would the :options handler do if called directly? return some standard http-response? or (constantly nil)? Is there a defined standard response for this in the http standard?
• are there cases when the auto-generated :options handler is not wanted? -> should it be behind an router option like ::ring/create-option-method-for-endpoints?. if so, what would be the default?
• I guess the generated endpoint should be omitted from apidocs (easily with :no-doc true)

2 - special handling of :options (or even cors) into reitit-ring

• special handling of :options in reitit-ring at request runtime
• or even built-in support for cors via router options? (like in Pedestal)
• don't think this is a good, idea.

3 - cors-middleware & cors-interceptors

• a additional issue on top of the actual problem, should there be a pre-packaged mw & interceptor for cors in the reitit-middleware &/ reitit-interceptors package? we would get benefits like:
  • documentation (e.g. a named mw/interceptor exposing it's options into documentation tools)
  • fail-fast spec-validation of options
  • could wrap the ring-cors or be a rewrite of if (with optimized perf)
    • if a rewrite, are we on a road to hoarde everything in? :(
    • could the optimizations / changes be pushed into ring-cors repo instead?

[livtanong]

I think it makes sense to be able to support option 0, whether or not you choose to solve this problem via option 0. Which is to say, there exists an indeterminate number of libs that work in a way that is incompatible with reitit's route-first philosophy, and I think it would be useful to support that in general

In order to make 0 work, however, something must be done about the use of metadata to attach the routes to the ring-handler. A helper function to preserve the metadata like

(defn outer-middleware [ring-handler middleware] ...)

might be all that is needed. This would effectively support exemptions from the route-first rule.

edit: a word, formatting

[ikitommi]

For 0, we could add a optional 3rd argument to the ring-handler, which is options map, which can have a :middleware key. The reitit.http/ring-handler already has this as one has to pass in the :executor and extra :interceptors too, which also effect also the default routes. With ring it would be:

(def app
  (ring/ring-handler
    (ring/router
      ["/ping" (constantly {:status 200, :body "pong"})])
    (ring/create-default-handler)
    {:middleware [wrap-log wrap-cors]}))

The implementation would actually push the middleware on top of all endpoints middleware stack, so they are part of the endpoint documentation too.

The outer-middleware kinda helper is a good idea, could do effectively the same.

[livtanong]

I do like your idea more, with the optional map containing :middleware. It allows for further extension and configuration.

[stijnopheide]

I think that the solution should have:

1. OPTIONS resources documented for the ones that support it (so not sure that solution 1 - "all routes have OPTIONS and hide them from docs" is viable in this case). We are using the exported swagger docs to create AWS API Gateway resources.
2. auto-generate the OPTIONS for endpoints that support CORS. yada has this functionality, but I'm not sure if it's possible. (I'm new to reitit).

[ikitommi]

Thanks for the comments.

@stijnopheide Related to 2, both middleware & interceptors can inspect the route data both at runtime (like yada) and also at creation time. Still, neither way can be used to modify the resource the endpoint data, e.g. add :options handler. This is by design - it would be really powerful (and would solve this), but would also be a source of confusion & could cause hard to track bugs as any mounted middleware/interceptor could override something in the route data. Comments most welcome on that issue too.

For now, to solve this inside a router - the :options method generation needs to be done in a router-level. Would it be ok if we created the :options default method for all endpoints, which any client definitions would override? This can be easily done with a router compile-hook and meta-merge merge hints. We could also just create the endpoint for the endpoints which have some route data defined.

(note to self: need to think how to make the router compiler more composable to allow multiple standalone compilers to run)

In all cases, all the request-time processing in reitit is stuffed into middleware & interceptors, which need to be parametrized to do their work. Options are:

I) via arguments to middleware/interceptors
II) via route data

With I, we could use the ring-cors as-is, but tested and it's kinda slow (15µs to merge all the headers). II would require anyway a custom mw & interceptor to be written. With custom one, we could use spec to get fail-fast at creation time & in future suggestion to spelling etc.

With some googling, at least these options seem to exist:

{:access-control
 {:allow-origin "http://foo.example"
  :allow-methods #{:get :post :put}
  :allow-credentials true
  :allow-headers #{"X-PINGOTHER" "Content-Type"}
  :expose-headers #{"X-My-Custom-Header" "X-Another-Custom-Header"}
  :max-age 86400}}

[livtanong]

Default :options sounds good. Just to solidify my understanding: While allowing ring-cors to work as a wrapper around the ring handler would work, it would end up doing its ring-cors work on every single request that comes in, slowing things down. We want to use default :options so that at least the ring-cors work can trigger specifically when the request method matches :options. Did I get that right?

[ikitommi]

Related to adding top-level middleware to router (retaining meta-data), see #146.

[ikitommi]

Did a PR (#148) to generate the undocumented :options handler (200 OK) for all defined routes. The route definitions will override the default (e.g. a top-level handler or a direct :options handler). There is a new ring-router option ::ring/default-options-handler to change the handler or disable the handler generation.

Deployed in Clojars as 0.2.3-SNAPSHOT. Does this solve the problem?

[livtanong]

Yes it does! looks good :D

[ikitommi]

what do you think @stijnopheide? The cors (middleware/interceptor) is an addon on top of this.

I think the default could return the methods too:

Minimally, the response should be a 200 OK and have an Allow header with a list of HTTP methods that may be used on this resource. As an authorized user on an API, if you were to request OPTIONS /users/me, you should receive something like…

200 OK
Allow: HEAD,GET,PUT,DELETE,OPTIONS

Goal in not to try to be 100% http compliant, but I guess there is no reason not to push that info to the clients...

[stijnopheide]

@ikitommi, yes I think that'll work for us!

[ikitommi]

Thanks for the comments.

Added the Allow header + mirrored the changed to reitit-http too.

=> [metosin/reitit "0.2.3"], the CHANGELOG.

[wpcarro]

I'm not proud to admit this, but I've been wrestling with CORS issues for the past two days. I switched from compojure to reitit recently. I was excited when I saw that reitit was considering bundling CORS support into the library.

For someone like me who is less familiar with the data-driven middleware approach, can anyone provide an example of how to use wrap-cors? wrap-cors accepts a few arguments...

• access-control-allow-origin
• access-control-allow-methods

...it's unclear to me how I can apply these kwargs to wrap-cors using a vector.

I'm currently failing with the following setup:

(-> (ring/ring-handler
     (ring/router
      [my-routes]
      {:data {:coercion reitit.coercion.spec/coercion
              :muuntaja m/instance
              :middleware [wrap-params
                           muuntaja/format-middleware
                           coercion/coerce-exceptions-middleware
                           coercion/coerce-request-middleware
                           coercion/coerce-response-middleware]}})
     (ring/create-default-handler)
     {:middleware [[wrap-cors :access-control-allow-origin #"http://localhost:3000"]]})
    (jetty/run-jetty {:port 9001 :join? false}))

In general I feel like CORS is a pretty simple concept. In practice, I feel like I'm tripping over it.

[jtlocsei]

@wpcarro the steps that got it working for me were

1. Add [ring-cors "0.1.13"] dependency to project.clj
2. Include [ring.middleware.cors :refer [wrap-cors]] in the :require of your routes namespace
3. Use wrap-cors middleware on the route itself, either as a function or a vector.

As a function:

["/my-api-endpoint"
 {:middleware [#(wrap-cors % 
                     :access-control-allow-origin [#".*"] 
                     :access-control-allow-methods [:get :post])]}

As a vector:

["/my-api-endpoint"
 {:middleware [[wrap-cors
                   :access-control-allow-origin [#".*"]
                   :access-control-allow-methods [:get :post]]]}

[kentbull]

Here's what ended up working for me:

(def router-config
  {:data {:coercion coercion-spec/coercion
          :muuntaja m/instance
          :middleware [swagger/swagger-feature
                       muuntaja/format-middleware
                       exception/exception-middleware
                       coercion/coerce-request-middleware
                       coercion/coerce-response-middleware
                       ]}})

(defn routes
  [env]
  (ring/ring-handler
    (ring/router
      ["/v1" {:middleware [#(wrap-cors %
                              :access-control-allow-origin [#".*"]
                              :access-control-allow-credentials "true"
                              :access-control-allow-methods [:get :post :put :delete])]}
       (app/routes env)]
      router-config)))
      
 (defn app
  [env]
  (router/routes env))