Use verify_authenticity_token directly#20
Open
nevans wants to merge 4 commits into
Open
Conversation
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.
c0e9b13 to
68b4ab2
Compare
Contributor
Author
|
FYI: I rebased this on top of #22, so it would pass CI against all versions of rails. |
Collaborator
|
Thank you for your patch. I'm sorry for responding to this PR a year later or so. I think your approach looks great. However, since I fixed #23, I think the code no longer applies cleanly and hence needs to be updated. If you still want to get this in, would you mind rebasing this PR and update it with the fix that work for all versions of Rails? I've recently updated the CI matrix as well, so you should be able to rely on it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a significant refactoring, in order to use
verify_authenticity_tokendirectly.This was motivated by issues with GitHub CodeSpaces: their proxy mangles the
Originheader (infuriating). That can be hard to debug already, but unfortunately this gem lost all of the context for whyverified_request?returned false. Fortunately,verify_authenticity_tokendoes 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_tokenmethod name is the primary public API through which users interact withActionController::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 thoughActionController::RequestForgeryProtectionhas been through significant changes and refactorings during that time.In order to use
verify_authenticity_token, we need a working#loggerand we need to ensure thatverify_authenticity_tokenraises an exception.The
#loggeris simply delegated toOmniAuth.logger.I converted
TokenVerifier.configto work more similarly to the standard rails approach for inheriting a configuration object (should this be split into its own PR?).ActionController::RequestForgeryProtectionadds 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 isActionController::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:forgery_protection_strategyisn't configured to raise exceptions, thenverify_authenticity_tokendoesn't raise an exception, and OmniAuth can't detect any failure when it's called.log_warning_on_csrf_failuretofalse.Additionally, I wrapped the exception in OmniAuth::AuthenticityError, so it would be caught by the appropriate OmniAuth rescue clause: