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

Allow both encrypted + unencrypted CSRF header token #7528

Merged
merged 1 commit into from Feb 20, 2015

Conversation

@barryvdh
Copy link
Contributor

barryvdh commented Feb 20, 2015

I know something similar has been submitted before, but currently CSRF works for these situations:

  • CSRF works for hidden _token input
  • CSRF works automatically for Angular, which uses the encrypted value from the cookie (+ X-XSRF-TOKEN header)

But it doesn't for other common use-cases:

  • Plain text token header
  • Scripts that use X-CSRF convention

Proposal: Also check the X-CSRF-TOKEN header for plain-text token. This make it easier to add a meta-tag to the page which javascript checks.
<meta name="csrf-token" content="<?= csrf_token ?>" />

Some frameworks/scripts already use this convention. like Jquery UJS

CSRFProtection: function(xhr) {
      var token = $('meta[name="csrf-token"]').attr('content');
      if (token) xhr.setRequestHeader('X-CSRF-Token', token);
    },

We see a lot of issues by people not understaning the decryption or need to remove it. This will allow all cases:

  1. Check the _token input, plain text
  2. Check the X-CSRF-TOKEN, plain text
  3. Check the X-XSRF-TOKEN, encrypted

And a section would needed to be added to the docs, but I'm willing to write that, with a few simple examples.

Fixes #7418 #7437 #7436 #7373 #7435 #7288 #7287

@JoostK

This comment has been minimized.

Copy link
Contributor

JoostK commented Feb 20, 2015

Now this seems like a proper universal fix. Clears up the apparently unknown convention of XSRF vs. CSRF.

@barryvdh

This comment has been minimized.

Copy link
Contributor Author

barryvdh commented Feb 20, 2015

Well I don't know if that is really the convention. I think the token being encrypted is more of a side-effect of laravel's cookies always being encrypted, which just doesn't matter for Angular bot does for other ways to pass the token.

taylorotwell added a commit that referenced this pull request Feb 20, 2015
Allow both encrypted + unencrypted CSRF header token
@taylorotwell taylorotwell merged commit 8687d42 into laravel:5.0 Feb 20, 2015
2 checks passed
2 checks passed
Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Feb 20, 2015

Can you submit a documentation PR?

@barryvdh

This comment has been minimized.

Copy link
Contributor Author

barryvdh commented Feb 20, 2015

Docs PR in laravel/docs#1179

@barryvdh barryvdh deleted the barryvdh:patch-9 branch Feb 20, 2015
@barryvdh barryvdh referenced this pull request Feb 20, 2015
@OzanKurt

This comment has been minimized.

Copy link
Contributor

OzanKurt commented on bd41d7f Feb 21, 2015

What is the difference between CSRF and XSRF?

This comment has been minimized.

Copy link
Contributor

OzanKurt replied Feb 21, 2015

What is the difference between CSRF and XSRF?

This comment has been minimized.

Copy link
Contributor Author

barryvdh replied Feb 21, 2015

Check the corresponding docs change: https://github.com/laravel/docs/pull/1179/files

This comment has been minimized.

Copy link
Contributor

OzanKurt replied Feb 21, 2015

@OzanKurt

This comment has been minimized.

Copy link
Contributor

OzanKurt commented on bd41d7f Feb 21, 2015

What is the difference between CSRF and XSRF?

This comment has been minimized.

Copy link
Contributor

OzanKurt replied Feb 21, 2015

What is the difference between CSRF and XSRF?

This comment has been minimized.

Copy link
Contributor Author

barryvdh replied Feb 21, 2015

Check the corresponding docs change: https://github.com/laravel/docs/pull/1179/files

This comment has been minimized.

Copy link
Contributor

OzanKurt replied Feb 21, 2015

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