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

CSRF rejections should be logged by default #1192

Closed
bitemyapp opened this issue Mar 18, 2016 · 4 comments
Closed

CSRF rejections should be logged by default #1192

bitemyapp opened this issue Mar 18, 2016 · 4 comments
Assignees

Comments

@bitemyapp
Copy link
Contributor

I've made a custom CSRF middleware privately that logs that a request was rejected. This is needed because the user will never see any of the log output from their handler and the body seems to be empty when this happens.

I could respin this with a parameterization that takes a logLevel and then update the scaffolds if people are open to this.

@MaxGabriel
Copy link
Member

Logging CSRF errors sounds useful. I like that idea. I thought that a 403 was logged when CSRF failure happened though. Maybe I'm misremembering.

Currently the scaffolding doesn't have CSRF middleware anymore (most of Yesod-auth was breaking under it). But I think all that's been fixed, so it may be ready to be added back.

@MaxGabriel
Copy link
Member

So you will get a 403 logged, but it won't obviously be a CSRF error:

POST /comments
  Request Body: {"message":"test"}
  Accept: */*
  Status: 403 Forbidden 0.000439s

Do you think more is needed?

I could respin this with a parameterization that takes a logLevel and then update the scaffolds if people are open to this.

Another potential option—not sure if this is too much—but we could allow running an arbitrary handler action on CSRF failure. This gives a little more flexibility (you could raise an exception, or log to graphite, or log to a file, or clear the session but allow the request, or whatever), but is slightly more complex.

@bitemyapp
Copy link
Contributor Author

Do you think more is needed?

I've lost hours to this recently and I've been using Yesod for 3+ years, so yeah, I think more is needed.

Another potential option—not sure if this is too much—but we could allow running an arbitrary handler action on CSRF failure.

I'm fine with that, but I'd still like the default to log something as logWarn whatever we do with the extensibility of the code.

@MaxGabriel
Copy link
Member

@bitemyapp Ok, I think having written the CSRF middleware stuff and intentionally triggered it so many times that I didn't have a good perspective on what it's like for other developers. Thanks for reporting this. Thoughts on #1200?

MaxGabriel added a commit to MaxGabriel/yesod that referenced this issue Mar 29, 2016
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

No branches or pull requests

3 participants