OAuth uses the Authentication header, not cookies, so it's not vulnerable to CSRF attacks. Requiring extra token lookup requests from apps using OAuth is unnecessary extra complexity.
Conditions of acceptance
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| api: don't require CSRF token if using a CSRF-safe SessionProvider | mediawiki/core | master | +17 -12 |
I'm inclined to decline this: requiring the CSRF tokens doesn't hurt anything besides an extra round-trip once to fetch the token and a few bytes to send it, and it's likely to be much more complex[1] to sometimes require them and sometimes not than it is to just always require them.
I also note that clients that support both OAuth and non-OAuth authentication (e.g. anything that wants to support non-WMF wikis too) will have to already have to have the code for handling the tokens, so it's not even likely to save them anything.
[1]: Specifically, much more complex in the code that actually checks that the token is valid. And unnecessary complexity in security code is generally considered bad.
I have spend a day debugging CSRF failures when making edits using OAuth... it seems that for some reason MediaWiki doesn't persist those sessions. In any case, if there was no CSRF token requirement, this wouldn't be an issue.
Just briefly not to derail this task: My problem was fixed by removing $wgPHPSessionHandling = 'disable'; (I suppose I had it enabled in earlier attempts to resolve lost session issues). Not sure why PHP session handling is deprecated in the 1.27 release notes.
In theory PHP session handling is not needed (assuming none of your other code tries to directly interact with PHP sessions - there's a warning mode for the variable to log when that happens) and degrades performance. We never tried to disable it for Wikimedia wikis so that should be taken with a grain of salt though.
@BPirkle this was fixed for rMW804842910317: Allow SessionProviderInterface to say if it is safe against CSRF for the REST API, right?
I think the action API still requires a CSRF token though, even if SessionProvider::safeAgainstCsrf() is set.
Sounds like this is easy to fix for the Action API based on SessionProvider::safeAgainstCsrf().
Change #1165975 had a related patch set uploaded (by SD0001; author: SD0001):
[mediawiki/core@master] api: don't require CSRF token if using a CSRF-safe SessionProvider
@BPirkle this was fixed for rMW804842910317: Allow SessionProviderInterface to say if it is safe against CSRF for the REST API, right?
Correct.
No objection to this change. Looks like this has a proposed patch and reviewers, so moving it to our Radar column. Let us know if we can help.
I can't get my patch to work. This is an interesting task and I shouldn't have jumped on it with limited time available. If anyone wants to take this over, please do.
Change #1165975 abandoned by SD0001:
[mediawiki/core@master] api: don't require CSRF token if using a CSRF-safe SessionProvider
Reason:
inactive WIP patch