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

Make CSRF failure logging optional/configurable. #14280

Merged
merged 1 commit into from Mar 8, 2014
Merged

Make CSRF failure logging optional/configurable. #14280

merged 1 commit into from Mar 8, 2014

Conversation

@joho
Copy link
Contributor

@joho joho commented Mar 5, 2014

Added the log_warning_on_csrf_failure option to ActionController::RequestForgeryProtection
which is on by default.

My reasoning being that I'm using papertrailapp on an app that is maybe 80% API and I'm explicitly using null_session to ignore CSRF problems on my API endpoints safely, but am getting a lot of log noise which isn't very helpful. I thought about overriding verify_authenticity_token in my app code, but the comments suggest that is a bad idea and I agree, but as the logging happens in there rather than in any of the protection method classes, I can't override the behaviour the preferred way.

The other implementation options I've thought of:

  • push all the logging down into the classes in ProtectionMethods and then I re-implement NullSession in my own app code as QuietNullSession
  • add a QuietNullSession to ProtectionMethods

I prefer it to be a config option because that way I don't have to worry about keeping my CSRF stuff in sync with rails over time, because honestly I'm unlikely to, and then security will happen to me.

Added the log_warning_on_csrf_failure option to ActionController::RequestForgeryProtection
which is on by default.
@dnch
Copy link

@dnch dnch commented Mar 5, 2014

👍

@robin850 robin850 added this to the 4.2.0 milestone Mar 5, 2014
@joho
Copy link
Contributor Author

@joho joho commented Mar 7, 2014

@NZKoz as the committer that had a bunch to do with #7616 have you any thoughts?

@NZKoz
Copy link
Member

@NZKoz NZKoz commented Mar 7, 2014

a config option to silence the log message sounds simplest and I have no objection to it being included.

@joho
Copy link
Contributor Author

@joho joho commented Mar 7, 2014

So... last time I put in a patch everything was still lighthouse and patchfiles.

What now?

spastorino added a commit that referenced this pull request Mar 8, 2014
Make CSRF failure logging optional/configurable.
@spastorino spastorino merged commit 2af7a7b into rails:master Mar 8, 2014
1 check passed
1 check passed
@jeremy
default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants