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 log.warn instead of debug for CSRF token warning #2972

Merged
merged 3 commits into from
Sep 10, 2011
Merged

Use log.warn instead of debug for CSRF token warning #2972

merged 3 commits into from
Sep 10, 2011

Conversation

md5
Copy link
Contributor

@md5 md5 commented Sep 10, 2011

We had a situation where the CSRF token additions in jquery_ujs were lost in a refactor and had a hell of a time tracking down why every post was logging the user out (we have a GET-heavy site and the pattern wasn't immediately clear since it manifested in the one broadly-used POST-based feature).

I poked around and saw the session clearing stuff in the CSRF protection code and realized what our problem was, but then I noticed that the big scary "WARNING" message was being logged with logger.debug. If we had seen that warning message in the development logs, it would have been immediately clear what was happening.

@sikachu
Copy link
Member

sikachu commented Sep 10, 2011

  1. Nice Github username :)
  2. It would be great to provide CHANGELOG entry for this.
  3. Is there currently a test coverage for warning?

@md5
Copy link
Contributor Author

md5 commented Sep 10, 2011

Sure, I can add a CHANGELOG entry.

There isn't an existing test for the warning; if you have a comparable example for testing a warning like this, I can try to add that too.

@md5
Copy link
Contributor Author

md5 commented Sep 10, 2011

@md5
Copy link
Contributor Author

md5 commented Sep 10, 2011

Ok, I added the test and a CHANGELOG entry. I added the entry in the 3.2.0 section since I did these changes off of master, but it seems like a reasonable idea for this to go into 3-1-stable too. Not sure how you guys decide that.

Also, I pushed these commits to a different csrf_token_warning branch under md5/rails. I'm not going to be pushing any other commits to my master any time soon, but I can redo the pull request from that branch if you like.

tenderlove added a commit that referenced this pull request Sep 10, 2011
Use log.warn instead of debug for CSRF token warning
@tenderlove tenderlove merged commit d0946fd into rails:master Sep 10, 2011
@md5
Copy link
Contributor Author

md5 commented Sep 10, 2011

I just noticed I used a "rescue" instead of "ensure" when putting the old logger back (I'm a bit of a Ruby/Rails noob). Not sure whether the logger is re-initialized for each test, so this may not have any effect on log output in other tests, but I thought I'd point it out.

@md5
Copy link
Contributor Author

md5 commented Sep 10, 2011

Here's the commit FWIW: https://github.com/md5/rails/commit/dbef311819c2c6475d156cc74611992ae45f1330

I'm going to continue about my business now and leave you fine people to yours.

@jonleighton
Copy link
Member

Please could you submit a PR for the rescue/ensure change and we will merge it. Thanks.

@jonleighton
Copy link
Member

Also, great change, I was bitten by this recently.

@spastorino
Copy link
Contributor

@tenderlove bro can you merge this and #2974 to 3-1-stable?

@tenderlove
Copy link
Member

@spastorino why do you want this in 3-1-stable? It is a new feature, not a bug fix.

@spastorino
Copy link
Contributor

I don't think it's a feature we were reporting in the wrong level. But anyway unsure if it worth backporting just pinging you to know your thoughts :)

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.

5 participants