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

Log a warning when a CSRF error occurs #1200

Merged
merged 2 commits into from Mar 29, 2016

Conversation

Projects
None yet
3 participants
@MaxGabriel
Copy link
Member

MaxGabriel commented Mar 28, 2016

(Closes #1192)

So, this is a breaking change because it adds the MonadLogger constraint to checkCsrfHeaderOrParam. However, I find it exceptionally unlikely that would cause issues, since this function is mainly used by higher-level middleware, is pretty new, and would probably be called from a Handler that already has logging capabilities.

The current log message is the same as what's returned in the response body:

28/Mar/2016:12:15:41 -0700 [Warn#yesod-core] A valid CSRF token wasn't present in HTTP headers or POST parameters. Check the Yesod.Core.Handler docs of the yesod-core package for details on CSRF protection. @(yesod_LSS9gnOrLHV9K8GzIkReQP:Yesod.Core.Handler ./Yesod/Core/Handler.hs:1445:5)

  • Thoughts on the error message?
  • Thoughts a breaking change being ok?

cc @bitemyapp

@bitemyapp

This comment has been minimized.

Copy link
Contributor

bitemyapp commented Mar 28, 2016

Include a stable URL that documents the issue perhaps? A lot people (myself included) use terminals that make it easy to open recognizable URLs.

Other than that, looks good. You're not adding this at the level where any provenance/cause is known, so this is as good as it's going to get for now. As long as the route/method will be proximal to this message, I'm fine with it.

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Mar 28, 2016

Can I ask where you were running into this error? I noticed that the default test environment in the yesod-scaffolding wouldn't log something like this and wondered if that was contributing to the problem

@bitemyapp

This comment has been minimized.

Copy link
Contributor

bitemyapp commented Mar 28, 2016

We had logs turned up.

Happened in dev, yesod-test has some stuff that fails at runtime if you don't make a GET before-hand. I already added logging for that particular case.

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Mar 28, 2016

@bitemyapp any suggestions for a stable URL? Ideally I'd link to this: http://haddock.stackage.org/lts-5.10/yesod-core-1.4.19/Yesod-Core-Handler.html#g:28 but I don't know of a way to get a stable link to it, besides hardcoding an specific version.

@bitemyapp

This comment has been minimized.

Copy link
Contributor

bitemyapp commented Mar 28, 2016

@MaxGabriel stackage needs the latest version defaulting behavior Hackage has, but for LTS, it looks like.

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Mar 28, 2016

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Mar 28, 2016

@snoyberg Do you think this is enough of a breaking change to warrant 1.5 or could this be snuck in as 1.4.20?

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented Mar 29, 2016

To be clear, the breaking change you're referring to is the introduction of the MonadLogger constraint? I have no objection to that going into 1.4, since all instances of MonadHandler are also instances of MonadLogger.

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Mar 29, 2016

Ah my bad, didn't realize that.

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented Mar 29, 2016

It's a mistake in my original design, MonadLogger should have been a superclass of MonadHandler (unless history itself defeated me here). Either way, I see no problem with this being in the 1.4 series.

@MaxGabriel MaxGabriel force-pushed the MaxGabriel:logCSRFErrors branch from ea6680a to 5a5cfd6 Mar 29, 2016

@MaxGabriel MaxGabriel merged commit 09087c9 into yesodweb:master Mar 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@snoyberg snoyberg removed the in progress label Mar 29, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.