Avoid "CSRF failed" pages

[MorrisJobke]

• create a password protected share
• open the link and get a password prompt
• send your machine to sleep and open it on the next day
• enter the password
• press the "access link" button
• expected: I get access
• actual: "Access forbidden. CSRF check failed"

We should make the access forbidden page a bit less technical and make the error inline, so that the user just get an "Login failed - please try again" instead of the pure error page.

cc @nextcloud/security @nextcloud/designers @rullzer Does that make sense?

[rullzer]

Ah the CSRF joys.

So a few things:

1. Yeah probably good to have a less technical error. Even if it means lying to the user about the reason ;)
2. We discussed in Austria with @LukasReschke and @ChristophWurst, that maybe we could abuse the heartbeat mechanism to update the CSRF token?
3. Or we would use some dedicated storage (just drop it also in the distributed memcache?) so it survives the default session timeout a bit better.

1 is not to hard to to. 2. Already a bit trickier. And 3 will need serious design and implementation effort (as you don't want to mess it up).

[ChristophWurst]

will need serious design and implementation effort

Whatever path we take, would be great to have a solid solution. The way we currently handle it is fragile. Let's see how other platforms/frameworks handle this.

As far as I understood the problem we need

• A mechanism on the client that catches outdated CSRF tokens and updates them
• A mechanism on the server-side to retrieve a new CSRF token

I could look into this for NC14.

[raimund-schluessler]

I guess the same thing also happens for apps. For calendar and tasks I always get a

<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotAuthenticated</s:exception>
  <s:message>CSRF check not passed.</s:message>
</d:error>

if the apps were not used for a while.

[jancborchardt]

For best UX it would of course be best to not have an error message of any kind, but just have it work. ;) (So I guess the heartbeat update, or what Christoph mentioned?)

[nickvergessen]

Not sure what happens after suspend though. Because when the heartbeat didn't run for a long time and your session expired there is nothing we can do.

If the session is still valid and the heart beat continues, that might fix it.

[ChristophWurst]

Not sure what happens after suspend though. Because when the heartbeat didn't run for a long time and your session expired there is nothing we can do.

There is: fetch a new now token and replace the currently used one.

[MorrisJobke]

The underlying issue for the page during logout is maybe fixed with #6544 in master

[MorrisJobke]

See also #1075

[oparoz]

For best UX it would of course be best to not have an error message of any kind, but just have it work. ;) (So I guess the heartbeat update, or what Christoph mentioned?)

How about having a production mode which removes these errors from the front-end, redirecting the user to the login or Files app?
admins would still get notified via the logs.

[ChristophWurst]

A mechanism on the server-side to retrieve a new CSRF token

I've given this a quick test:

server/core/Controller/CSRFTokenController.php

Lines 48 to 62 in 68fa0d4

	/**
	* @NoAdminRequired
	* @return JSONResponse
	*/
	public function index() {
	$requestToken = $this->session->get('requesttoken');
	if (is_null($requestToken)) {
	return new JSONResponse(null, Http::STATUS_NOT_FOUND);
	}
	return new JSONResponse([
	'token' => $requestToken,
	]);
	}

… and that seems to be usable on the client-side. As far as I can tell and have tested this (as a non-expert in security) other sites are unable to access this route because it's not a CORS route. @nextcloud/security is there anything I've forgot to add?

Of course, the client side is still missing. And this approach will only work for requests sent from JS and not for static HTML forms.