-
-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not verify CSRF for every rodauth request #2
Comments
We should probably add a Though I'm wondering what Roda's CSRF protection does differently 🤔 You either require a valid CSRF token for a route or not, it should also require that you explicitly disable CSRF protection for a certain route IMO. Maybe there is more to CSRF protection than I think. |
roda's csrf protection is just |
That's good to know, I thought you were using Roda's |
I think I found the issue (sort of). rodauth-rails calls verify_authenticity_token inside the rodauth feature, as an extension to the csrf plugin; however, there is a rodauth controller, where this callback should/could be defined: class RodauthController < ApplicationController
before_action :verify_authenticity_token
end In order to make this more compliant with the rails way, rodauth should skip/disable the roda csrf plugin, and let the rodauth controller validate the authenticity token where rails expects it. Rails developers already know that before_action, and can choose to skip it if they desire so (or conditionally, as is my case). Also, I found out that csrf protection is disabled in test mode by default, although for some reason that check doesn't pick up that variable. |
How rodauth-rails works is that, before every action, the following happens:
So, rodauth-rails doesn't execute "actions" on the controller, which means Note that rodauth-rails disables Roda's CSRF plugin: rodauth-rails/lib/rodauth/rails/app.rb Line 16 in 45ba96c
And in test mode the CRSF protection should be properly skipped, because |
I see now, thx for the details. The way I see it, it should then work like this:
or
Of course, this is a totally separate "feature request", I guess :) However, from the perspective of putting rails things in rails, I think that csrf protection should be altogether "removed" from rodauth and put into rails controls, as it's less of a burden for a rails developer, as the internet is full of "how do I disable csrf protection in rails" and this project would benefit of complying with the norm. Let me know what you think. |
It's true, rails disables csrf protection in test mode by default. I managed to circumvent this by explicitly defining in config.action_controller.allow_forgery_protection = false and it works now. However, my problem still stands (having rodauth routes that I want to skip csrf protection for). |
Thank you for your input. I'll try to provide an answer for the two suggestions: 1️⃣ I do agree the current behaviour is non-standard compared to other Rails gems, but I think that's a result of Rodauth's unique design, and I think it would be slippery slope to try to mask rodauth-rails into something it isn't. rodauth-rails doesn't run controller actions, so I don't think it's correct to try to run Action Controller callbacks. Note that AFAIK there is no I also don't know how to call Action Controller callbacks separately from executing a controller action, and we'd need to ensure that options like 2️⃣ I've explored making rodauth-rails a Rails engine, I was going back on forth when deciding on the design, but in the end I didn't see how it would benefit from being an engine, and I chose a simple railtie instead. I wanted everything to integrate automatically, but without hiding things that aren't necessary to hide. For example, I wanted to show copy Rodauth controller in the codebase for transparency. If we provide instructions on disabling CSRF protection, I don't see a benefit in having it on the controller level. It would add additional maintenance burden, and IMO it would lead away from understanding how rodauth-rails works. Note that you can already disable CSRF protection: rails_check_csrf! { super() unless request.path == oauth_token_path } |
Thx, your response is pretty valid. I get most of the concerns, I'll leave a few more comments taking these into consideration:
Agreed, better not go that route then.
I'd argue that, if it were an engine, you'd manage to keep
I just read this now. What do you mean by "for transparency"? I think that you'd gain from not exposing this detail to the user, for the possibility of him doing smth "rails-y", like before actions.
The thing is, I already do this here. |
I understand what you mean. However, the transparency is useful when the user wants to change the directory of their templates, which requires renaming the Rodauth controller and changing It also allows the user to define additional helper methods via the current I have a comment in the generated Rodauth controller file that this controller is used only for rendering views, so I'm hoping that is enough. I feel like making rodauth-rails an engine would be an overkill just to hide RodauthController, though I'll give it some more thought.
Thank you for shedding light on this, rodauth-rails should definitely hook into |
Great, thx Janko, this will do perfect, thx for the patch!
Just to be clear, my suggestions are only based on what devise does and what rails users might expect or be confused with. Your concerns are perfectly valid to me. However, I can feel that you'd like rodauth-rails to become a worthy competitor to devise, which is still the rails auth standard, and in order to do so, it should achieve most of the goals that devise does, while doing everything else better. Those are, IMO:
Most of your concerns are amendable:
True, but if
I'd argue that they could define a
Not only for that, it'd also "hide" your default views, thereby achieving that "quick-and-dirty bootstrap" and "customize as you go". However, I like it already as is, so take my comments with a "speculative grain of salt", as I'm assuming that users will feel one way, but they might feel differently in the end, so this might already be the setup users want. Maybe it's worth to wait for a few more opinions and questions. P.S.: A very useful thing for this project would be a "How to migrate from Devise to Rodauth-Rails" page :) |
This makes CSRF protection with Rails follow the same rules as when using Roda directly, meaning users can do things like override `#check_csrf?` to skip CSRF protection for certain routes, and that will now work correctly with rodauth-rails. The rodauth-oauth gem is one example where CSRF protection needs to be skipped for some routes. Closes #2
Yes, I would like this gem to be equally easy to work with as Devise, so it's great to hear any feedback. I agree that Yes, you're 100% percent correct regarding adding helper methods via
Note that with rodauth-rails views already hidden, by default Rodauth's internal templates are rendered, until the user decides to run
Oh yeah, I haven't considered this, it sounds very useful indeed 👍 Personally I'm not experienced with Devise (I've created rodauth-rails because I didn't feel like learning Devise and then figuring out it's too complex for me 😛), but I will try to write a guide at some point. |
This makes CSRF protection with Rails follow the same rules as when using Roda directly, meaning users can do things like override `#check_csrf?` to skip CSRF protection for certain routes, and that will now work correctly with rodauth-rails. The rodauth-oauth gem is one example where CSRF protection needs to be skipped for some routes. Closes #2
To actually support the full range of callbacks, there would need to be an around_rodauth do
rails_controller_instance.run_callbacks(:process_action) do
yield # or super(), depending on how `around_rodauth` would be defined
end
end That would be sufficient plus perhaps disabling the Roda-owned CSRF check or adding I expended some energy trying to do this myself with a little bit too much cleverness, but ultimately was foiled by the Rodauth lifecycle and when the route handlers are actually defined: methods.grep(/^_handle_/).each do |route_handler|
define_method(route_handler) do
rails_controller_instance.run_callbacks(:process_action) do
super()
end
end
end It may still be possible but I opted for a simpler solution that actually addressed my immediate needs: before_rodauth do
super() if defined?(super)
rails_controller_instance.instance_eval do
track_session
set_seo
set_embedded_origin
check_http_basic if Config.staging?
end
end EDIT: The above wasn't so graceful in the end because a |
Interesting, thank you for sharing. The problem with implementing an around or after callback like this is that it won't work with redirects or early responses, because Rodauth uses I think the correct way to do this would be to utilize Roda's hooks plugin. I'll see if it's possible to add |
@bjeanes Actually, do you think you could make a proposal for the |
I had a reminder to come back to this but then work got a bit busy. I will do that today, but first I'm going to have a short play in |
Here is a proof-of-concept for the https://github.com/jeremyevans/rodauth/compare/master...bjeanes:around_rodauth?expand=1 I'll email Jeremy on the mailing list now to discuss whether he'd be interested in moving in this direction. |
@HoneyryderChuck I'm preparing to release 2ca331e, which would run Action Controller action callbacks and rescue handlers around Rodauth actions. Because now CSRF token would be verified automatically as part of running controller action callbacks, I've removed explicit CSRF protection and overrides of However, I think this can be problematic when integrating rodauth-oauth, because it overrides I'm not sure exactly how to solve this problem, I thought you might have ideas. My only idea was adding something like: skip_before_action :verify_authenticity_token, unless: -> { rodauth.check_csrf? } to the Rodauth controller, but the problem is this won't be possible for existing users of rodauth-rails, as their Rodauth controller has already been generated. That problem probably wouldn't exist if rodauth-rails was an engine like you initially suggested, though I still think that would bring its own challenges. Maybe there is another solution I'm not seeing. |
@janko What about calling |
I'll also note that performing CSRF protection twice may not be horrendous in most cases (personally, I emit a bunch of metrics that woudl be doubled up so it would be PITA, but I think for most it'll just be the wasted clock time) |
@janko thanks for the heads-up! This would have gone under my radar otherwise.
I guess that one came back to haunt you :) eheh . For the record, although I still think an engine would be more appropriate, this was just one of the reasons. I don't think one can do much about already existing controllers, besides documentation and deprecation warnings. Would a rodauth controlller callback be able to check for present of both However, when turning it on, maybe it's best to had a filter for skipping it for JSON requests? Something in the lines of: skip_before_action :verify_authenticity_token, unless: -> { request.accepts_json? } I guess you'd want this in the generator altogether, to make using |
@bjeanes I like this idea. I wasn't sure whether CSRF token verification can be skipped at the controller instance level, but the def rails_controller_callbacks
rails_controller_instance.allow_forgery_protection = false
rails_controller_instance.run_callbacks(:process_action) do
yield
end
end I think this should work nicely, because Rodauth calls |
When we started running controller action callbacks around Rodauth actions, we've disabled Rodauth's #check_csrf behaviour, because Rails already verifies the CSRF token as part of callbacks. However, some Rodauth extensions such as rodauth-oauth disable checking CSRF tokens for certain routes, and our current implementation wouldn't take that into account, because CSRF protection would always be run, regardless of what `#check_csrf?` returns. We fix that by reintroducing the previous CSRF logic, where CSRF token is verified on `#check_csrf` which Rodauth automatically calls before each action depending on `#check_csrf?`. We also disable CSRF protection before `before_*` callbacks, so that it's not run twice or in cases when `#check_csrf?` returns false. Once `before_*` callbacks have run, we re-enable CSRF protection, so that Rails form helpers still add CSRF token tags. Concerns #2
Makes sense to me! |
I'm having a problem integrating roda-oauth in a particular case where I need to allow a POST request to an
/oauth-token
URI; such requests are usually performed in the backend, and do not go through an html form, therefore the CSRF protection doesn't make sense.This route is defined from inside the rodauth feature, however,
rodauth-rails
runs theverify_authenticity_token
by default for every rodauth request, so it breaks my flow.My suggestion would be to remove this line, or skip it given some circumstance around the request. The roda integration with csrf protection works, so there's something being done there that allows csrf protection to be skipped which hasn't been replicated in rodauth-rails.
The text was updated successfully, but these errors were encountered: