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

RFC: Consider ditching CSRF tokens #1681

Closed
Bloke opened this issue Apr 29, 2021 · 3 comments
Closed

RFC: Consider ditching CSRF tokens #1681

Bloke opened this issue Apr 29, 2021 · 3 comments

Comments

@Bloke
Copy link
Member

Bloke commented Apr 29, 2021

Background:

In theory, we could get rid of all our token/CSRF/bouncer token validation stuff in core since we set SameSite=Lax by default and set the httponly header on the login authentication cookie. Removing the CSRF protection would be a nice thing to do as it's less work on each page request (faster), fewer bytes over the wire (less bloat, faster admin panel delivery) and we only need to validate the step names, not then do the additional check of whether the tokens match - which is less faff for plugin authors too.

I'm slightly nervous removing it, but maybe CSRF really is unnecessary. In which case, we can remove it from the 4.9 UI library and get rid of all the form_token() rubbish. Presumably if somebody created a browser that didn't respect the headers, it wouldn't last long in the marketplace! At the time of writing three minor browsers don't support SameSite so if we ripped out the CSRF protection, they'd be exposed. Not sure if that's anything to worry about given the development state of those browsers.

Thoughts?

@bloatware
Copy link
Member

Theoretically, one txp user could throw a CSRF against another user, so SameSite wouldn't help. Should we adhere to 'trust your users' principle once again?

@Bloke
Copy link
Member Author

Bloke commented Apr 29, 2021

That's a good point. The articles I cited are approaching this from a man-in-the-middle perspective - an external attack. For multi-user systems, we still have a duty of care to protect users from each other - especially since some of those users may be Publishers with keys to the castle.

In that case, CSRF still has value and we should continue to honour it. I'm fine with keeping it; just didn't want to leave it in if it served no purpose.

We do need to boost the security of the public login tokens, however.

@Bloke Bloke changed the title Ditch CSRF tokens RFC: Consider ditching CSRF tokens Apr 29, 2021
@Bloke Bloke removed this from the v4.9 milestone Apr 29, 2021
@Bloke Bloke added the rfc label Apr 29, 2021
@Bloke
Copy link
Member Author

Bloke commented May 29, 2021

Let's park this. It's still valid to use them to protect users from each other.

@Bloke Bloke closed this as completed May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants