Skip to content
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

Use verify_authenticity_token directly #20

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nevans
Copy link
Contributor

@nevans nevans commented Oct 1, 2024

This is a significant refactoring, in order to use verify_authenticity_token directly.

This was motivated by issues with GitHub CodeSpaces: their proxy mangles the Origin header (infuriating). That can be hard to debug already, but unfortunately this gem lost all of the context for why verified_request? returned false. Fortunately, verify_authenticity_token does add the needed context to the exception message. Once I could see that message, my issue became obvious.

I believe this should be mostly safe, because the verify_authenticity_token method name is the primary public API through which users interact with ActionController::RequestForgeryProtection (e.g: skip_before_action :verify_authenticity_token). There is some risk that rails may change its implementation such that we need to update to this class to continue supporting it. However, I believe that risk is small: this code should work for rails 4.2 through 8.0, even though ActionController::RequestForgeryProtection has been through significant changes and refactorings during that time.

In order to use verify_authenticity_token, we need a working #logger and we need to ensure that verify_authenticity_token raises an exception.

The #logger is simply delegated to OmniAuth.logger.

I converted TokenVerifier.config to work more similarly to the standard rails approach for inheriting a configuration object (should this be split into its own PR?). ActionController::RequestForgeryProtection adds its defaults to TokenVerifier's config when it is included. By deleting all of the keys on our local config object, we ensure that every config setting is inherited from the parent (which is is ActionController::Base.config). By using the inheritable config (rather than simply overriding the methods) we gain the ability to override the ActionController::Base config for TokenVerifier. I did this for two config values:

  • If forgery_protection_strategy isn't configured to raise exceptions, then verify_authenticity_token doesn't raise an exception, and OmniAuth can't detect any failure when it's called.
  • Both OmniAuth and ActionController::RequestForgeryProtection will log the failure, so it's should be safe to set log_warning_on_csrf_failure to false.

Additionally, I wrapped the exception in OmniAuth::AuthenticityError, so it would be caught by the appropriate OmniAuth rescue clause:

    rescue OmniAuth::AuthenticityError => e
      fail!(:authenticity_error, e)
    end

Also, edge rails requires ruby 3.2 now, so it's dropped from ruby 3.1.
By using an inheritable_copy of the config (rather than just redefining
the methods) we gain the ability to _override_ the config locally.
By adding a logger and setting the protection_strategy to raise an
exception, we can use verify_authenticity_token directly.  The main
benefit of this is that we will get a more helpful error message
attached to the exception.
This allows the exception to be handled by the appropriate OmniAuth
error handler.  The original exception will still be available from
the wrapping exceptions's `#cause`, for error reporting and diagnostics.
@nevans nevans force-pushed the use-verify_authenticity_token branch from c0e9b13 to 68b4ab2 Compare November 9, 2024 17:33
@nevans
Copy link
Contributor Author

nevans commented Nov 9, 2024

FYI: I rebased this on top of #22, so it would pass CI against all versions of rails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant