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

Give better error messages when CSRF validation fails #1455

Merged
merged 1 commit into from Nov 26, 2017

Conversation

Projects
None yet
2 participants
@MaxGabriel
Copy link
Member

MaxGabriel commented Nov 17, 2017

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 Nov 17, 2017


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)

This comment has been minimized.

Copy link
@sbditto85

sbditto85 Nov 17, 2017

Contributor

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?

This comment has been minimized.

Copy link
@MaxGabriel

MaxGabriel Nov 17, 2017

Author Member

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"

This comment has been minimized.

Copy link
@sbditto85

sbditto85 Nov 17, 2017

Contributor

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

Give better error messages when CSRF validation fails
* 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 force-pushed the csrfBetterErrors branch from 54daba6 to 1275cce Nov 26, 2017

@MaxGabriel MaxGabriel merged commit c81ad91 into master Nov 26, 2017

2 of 4 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
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.