ring-clojure / ring-defaults

A library to provide sensible Ring middleware defaults
MIT License
345 stars 32 forks source link

It would be convenient to be able to access anti-forgery token from the request instead of a dynamic var #23

Closed timothypratley closed 6 years ago

timothypratley commented 6 years ago

Because of the order that site-defaults get applied by wrap-defaults, if you serve a static file it bypasses anti-forgery. This seems like a bad idea in general, but a concrete case where this hurts is when trying to use Sente: https://github.com/ptaoussanis/sente/issues/318 Sente requires a token, which is not present for pages using site-defaults

weavejester commented 6 years ago

Can you go into a little more detail about the particular circumstances involved? How is the anti-forgery token supposed to be incorporated into the page if it's a static resource?

timothypratley commented 6 years ago

Sure thing;

The anti-forgery token is incorporated into the session as per any request, so long as it is not short-circuited by the wrap-resource or wrap-file middleware... so removing them from the chain allows the anti-forgery token to work correctly.

As a concrete example, the way I work around this issue is:

(wrap-defaults app-routes (dissoc site-defaults :static))

Then provide a custom route:

(route/resources "/")

What I proposing is that wrap-resource and wrap-file should come before wrap-anti-forgery in the wrap-defaults implementation, allowing pages served as files to have a token.

A common use case where having the token there is useful is when hosting a page that has JavaScript or ClojureScript loaded in the page that wants to communicate with the server, so if that file is served from a file it should enjoy the same protections as a hiccup response. This is not limited to Sente. In the case of Sente the CSAF token is useful in establishing that the websocket is originating from a page served by the server.

If the proposed solution seems acceptable to you, please let me know and I'll make a pull request.

weavejester commented 6 years ago

Sorry, I'm still not understanding. Perhaps I'm missing something obvious here.

Client-side code needs to be given the anti-forgery token in some manner, so that it can be incorporated into a parameter or a header when a POST request is sent. How are you intending to inject a dynamically generated token into a static resource?

timothypratley commented 6 years ago

Hi James,

My apologies for being loose with my terms. I'll attempt a more precise description to clarify:

On the server there are two "static" resources: index.html and app.js index.html has a script src="app.js" in it, so when visiting mysite.com/index.html I get a HTML page that in turn loads the JavaScript file and executes it. The code in the JavaScript can access the browser "session", which is where the token may be found. The token is part of the session, not the static resources that were served.

Current site defaults prevent the session from being established or the token being included in the session when the request gets served from a resource or file.

Conversely; nothing needs to be injected, the token just needs to be in the session (a result of the request flowing through the anti-forgery and session middleware).

I'll make a minimal example project to demonstrate and post a link when it is ready.

weavejester commented 6 years ago

The code in the JavaScript can access the browser "session", which is where the token may be found.

Can you explain what you mean by "session"? By default the anti-forgery middleware uses the server session, which cannot be accessed by client-side code.

timothypratley commented 6 years ago

Hi James,

Yes. Naturally, you are correct :) The session is on the server.

In the Sente case, the WebSocket handshake involves transmitting the token to the client. So as best I can describe it when the WebSocket is established and a session is present, then the handshake can transmit the token. When the session is not present then no token can be transmitted.

I've made a start on a minimal project as a more concrete discussion point of the behavior I'm describing... but please be patient with me as it is taking me longer than expected to pull together :)

Regards, Timothy

weavejester commented 6 years ago

So as best I can describe it when the WebSocket is established and a session is present, then the handshake can transmit the token. When the session is not present then no token can be transmitted.

Sure, but the presence of the session on the static page has no bearing on whether that session is present when opening the websocket. They're two distinct requests.

I've made a start on a minimal project as a more concrete discussion point of the behavior I'm describing... but please be patient with me as it is taking me longer than expected to pull together :)

No problem.

timothypratley commented 6 years ago

Hi James,

I've created the minimal example (with no Sente involved). The code is here: https://github.com/timothypratley/anti-forgery-example

The important part is in handler.clj:

(GET "/token" req
  (do
    (println "AF" af/*anti-forgery-token*)
    (or (get-in req [:session :csrf-token])
        (get-in req [:session :ring.middleware.anti-forgery/anti-forgery-token])
        (get-in req [:session "__anti-forgery-token"])
        "No anti-forgery token found")))

When this code executes for the first time, the anti-forgery-token is not in the session yet, but is in the anti-forgery-token var. See https://github.com/ring-clojure/ring-anti-forgery/blob/092cbaa4a95d2da3d5ff925198edac92fb8b7055/src/ring/middleware/anti_forgery.clj#L91 implementation... the token is added to the session after the handler is called, not before.

When the end-point is called a second time, the token is now in the session.

To use the example:

  1. git clone git@github.com:timothypratley/anti-forgery-example.git && cd anti-forgery-example
  2. lein ring server-headless (it is important not to start a browser yet!)
  3. Navigate to localhost:3000/index.html (this must be the first URL you visit, the file gets statically loaded, no session or anti-forgery tokens. If you have visited a page already, restart the server to clear any sessions)
  4. Click 'request token' and the response is "No anti-forgery token found"
  5. Click 'use token' and the response is 403: Invalid anti-forgery
  6. Click 'request token' again and the response is a token
  7. Click 'use token' and the response is 200: You voted

Several things are clear to me after creating this example:

A) My suggestion to change the wrap-defaults was a bad idea. B) Servers can/must use the af/anti-forgery-token instead of looking for the token in the session... C) Sente does not (and should not) depend on the anti-forgery middleware, so it cannot use af/anti-forgery-token directly. D) A user of Sente can pass in a custom function that provides the token from af/anti-forgery-token... but this seems tedious compared to if the token was in the session. E) Sente only tries to send the token once (on handshake), and never tries to get another one.

Would it be possible to put the token in the session prior to calling the handler? That would make this annoying situation go away... Sente (and any server) could rely on the token from the session instead of the var.

Regards, Timothy

weavejester commented 6 years ago

The fact that the anti-forgery token is in the session is an implementation detail, which is why the session keyword for the token is namespaced. It's not intended to be used directly.

Nor can you always assume that the token is in the session to begin with - the anti-forgery middleware allows for different strategies to be used that may communicate the token through a different mechanism. The token may be communicated through a cookie, for example, or be a signed token that doesn't require a second communication channel.

weavejester commented 6 years ago

That said, the token could be attached to a key on the request map. I see no reason not to do that.

timothypratley commented 6 years ago

Hi James, Yup, makes sense to me... I've opened a tentative pull request along those lines. I'm not entirely sure what the 3 arity path is for or how it should behave yet.

timothypratley commented 6 years ago

Hi James,

Thank you for releasing ring-anti-forgery 1.3.0 :) Will you also be releasing ring-defaults to depend on this release version?

Regards, Timothy

weavejester commented 6 years ago

Done.

timothypratley commented 6 years ago

awesome, thank you very much! :)