inertiajs / inertia-rails

The Rails adapter for Inertia.js.
https://inertiajs.com
MIT License
529 stars 43 forks source link

make CSRF protection work out of the box (#71) #72

Closed buhrmi closed 2 years ago

buhrmi commented 3 years ago

This PR removes the need for the manual steps to make CSRF protection work. It does this by storing the current form_authenticity_token in the XSRF-TOKEN cookie (which is picked up by Axios automatically) and adding the the X-XSRF-TOKEN header value (which is sent by Axios by default) in the list of possible authenticity tokens. Fixes #71

louis-smit commented 2 years ago

Hey everyone. Is there any update on this? Seems to work great and would be cool to have it merged inπŸ™‚

buhrmi commented 2 years ago

I do have some second thoughts about this PR: It might be sufficient to simply add a skip_before_action :verify_authenticity_token, if: -> { request.inertia? } to the ApplicationController.

This effectively disables CSRF protection whenever a request is made with Inertia. My thinking is that a potential CSRF attack cannot include custom headers, meaning that if a request is an inertia request, it cannot possibly be a CSRF attack.

Please correct me if I'm wrong

louis-smit commented 2 years ago

My thinking is that a potential CSRF attack cannot include custom headers, meaning that if a request is an inertia request, it cannot possibly be a CSRF attack.

I did some research and you are right. I had no idea CSRF attacks cannot have custom headers. Inertia requests include custom headers, so therefore cannot be a CSRF attack.

If anyone with more knowledge of CSRF can weight in on this it would be great :)

bknoles commented 2 years ago

Thanks for taking this on @buhrmi ! We sort of solved this with the installer that wires up the token by default, but that leaves out anyone who installs Inertia Rails manually, so this is much nicer. A few questions / asks.

  1. Could you remove the lines in the generators that set the header in Axios? Those would be unnecessary after this change, correct? Lines are here, here, and here. I could certainly remove these in a follow on PR, but I think it'd be nice to have it all in a single PR.
  2. Could you and/or @louis-smit post the resources you found discussing CSRF and custom headers? It would be nice to have those available for future readers who might be investigating this same thing
  3. Could you add a test or two for these changes? Especially for the change that is overwriting the request_authenticity_tokens method in the Rails core, it'd be really nice if the test suite yelled at us if/when a future version of Rails changes the way that method works

Thanks again! I really love to see other people jumping in on the project (although that might not be apparent from our sometimes slow responses 😜)

louis-smit commented 2 years ago

although that might not be apparent from our sometimes slow responses 😜)

Not at all! Awesome project, really loving Inertia + Rails.

Could you and/or @louis-smit post the resources you found discussing CSRF and custom headers? It would be nice to have those available for future readers who might be investigating this same thing.

Certainly. While I'm not a Java developer, I think the Play Framework documentation summarized it pretty well:

Simply put, an attacker can coerce a victims browser to make the following types of requests:

  • All GET requests
  • POST requests with bodies of type application/x-www-form-urlencoded, multipart/form-data and text/plain

An attacker can not:

  • Coerce the browser to use other request methods such as PUT and DELETE
  • Coerce the browser to post other content types, such as application/json
  • Coerce the browser to send new cookies, other than those that the server has already set
  • Coerce the browser to set arbitrary headers, other than the normal headers the browser adds to requests

If an inertia request is sent, it features the X-Requested-With, X-Inertia, X-Inertia-Version headers - and the response contains the X-Inertia header.

if: -> { request.inertia? } in turn picks up on the custom headers, which therefore implies arbitrary headers, which an attacker cannot set.

However, I have not actually built any real world CSRF protection features - I just use whatever Rails gives me. So I'd very much like someone with more knowledge (such as @buhrmi who's idea it was) to confirm this is not a dumb thing to do.

Thanks again for your fantastic work. I've been looking for something like Inertia for a very long time.

BrandonShar commented 2 years ago

I'm going to close this because it's been inactive for a bit and it looks like there's some debate on whether's its necessary.

Please feel free to reopen if my read on this is incorrect!