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.

Sign up for GitHub

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

Jump to bottom

Give better error messages when CSRF validation fails #1455

Merged
merged 1 commit into from
Nov 26, 2017
Merged

Give better error messages when CSRF validation fails #1455

merged 1 commit into from
Nov 26, 2017

Conversation

MaxGabriel
Copy link
Member

@MaxGabriel MaxGabriel commented Nov 17, 2017 •
edited
Loading

This PR modifies the AJAX CSRF checking code to provide a clearer error message, based on my experience integrating a frontend with a Yesod app with @devonzuegel. The error message now says:

A valid CSRF token wasn't present. Because the request could have been forged, it's been rejected altogether. Check the Yesod.Core.Handler docs of the yesod-core package for details on CSRF protection.
Tip: By default, the CSRF token is sent to the client in a cookie named XSRF-TOKEN.
Tip: The server is looking for the token in the following locations:

  • An HTTP header named X-XSRF-TOKEN (which is not currently set)
  • A POST parameter named _token (which has the current, incorrect value: 'PpjJoRnZPmfoo')

compared to the previous, static message:

A valid CSRF token wasn't present in HTTP headers or POST parameters. Because the request could have been forged, it's been rejected altogether. Check the Yesod.Core.Handler docs of the yesod-core package for details on CSRF protection.

  • This is important because historically these errors have tripped people up
  • Making security as easy as possible is important so that it doesn't just get turned off
  • Giving clear directions about where to get the CSRF token (a cookie) and where to send it (a header/param) is especially helpful to frontend developers not necessarily familiar with the backend codebase

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts) (Passed on most versions, just timed out on GHC 8.0.2)

@MaxGabriel MaxGabriel force-pushed the csrfBetterErrors branch 3 times, most recently from 1ad4dc3 to c344f16 Compare November 17, 2017 07:51
sbditto85
sbditto85 reviewed Nov 17, 2017
View reviewed changes
yesod-core/Yesod/Core/Handler.hs Outdated

csrfErrorMessage :: [CSRFExpectation]
-> Text -- ^ Error message
csrfErrorMessage expectedLocations = "A valid CSRF token wasn't present. Because the request could have been forged, it's been rejected altogether. Check the Yesod.Core.Handler docs of the yesod-core package for details on CSRF protection.\nTip: By default, the CSRF token is sent to the client in a cookie named " `mappend` (decodeUtf8 defaultCsrfCookieName) `mappend` ".\nTip: The server is looking for the token in the following locations:\n" `mappend` T.intercalate "\n" (map csrfLocation expectedLocations)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: is there any reason to have Check the Yesod.Core.Handler docs of the yesod-core package for details on CSRF in the message. Its not something I would want being shown to anyone viewing my site. Is there some other way to get that message to developers and not viewers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that yesod-core has a way of knowing if it's being used for development or production, so I don't think so.

I agree that it's not an ideal error message for a user, but if your users encounter a CSRF error your site is probably broken, which is also a pretty bad user experience. Potential compromise: the sentence could be prefaced with: "If you're a developer of this site"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or possibly a link to a wiki page. Then the information is available and "normal" people will probably just ignore it.

@MaxGabriel
Give better error messages when CSRF validation fails
1275cce 
* This is important because historically these errors have tripped people up
* Making security as easy as possible is important so that it doesn't just get turned off
* Giving clear directions about where to get the CSRF token (a cookie) and where to send it (a header/param) is especially helpful to frontend developers not necessarily familiar with the backend codebase
@MaxGabriel MaxGabriel merged commit c81ad91 into master Nov 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbditto85 sbditto85 sbditto85 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@MaxGabriel @sbditto85