From 17336d8cc42d62844817320b07f0167ddc1ca63d Mon Sep 17 00:00:00 2001
From: Ferdinand Thiessen <rpm@fthiessen.de>
Date: Thu, 26 Jan 2023 21:08:10 +0100
Subject: [PATCH] fix(CORS): CORS should only be bypassed on `PublicPage` if
 not logged in to prevent CSRF attack vectors

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
---
 .../Middleware/Security/CORSMiddleware.php    |  2 +-
 .../Security/CORSMiddlewareTest.php           | 36 ++++++++++++++++++-
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php
index 2476f4ec9b31a..30ba8d8d6e4fb 100644
--- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php
+++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php
@@ -83,7 +83,7 @@ public function __construct(IRequest $request,
 	public function beforeController($controller, $methodName) {
 		// ensure that @CORS annotated API routes are not used in conjunction
 		// with session authentication since this enables CSRF attack vectors
-		if ($this->reflector->hasAnnotation('CORS') && !$this->reflector->hasAnnotation('PublicPage')) {
+		if ($this->reflector->hasAnnotation('CORS') && (!$this->reflector->hasAnnotation('PublicPage') || $this->session->isLoggedIn())) {
 			$user = array_key_exists('PHP_AUTH_USER', $this->request->server) ? $this->request->server['PHP_AUTH_USER'] : null;
 			$pass = array_key_exists('PHP_AUTH_PW', $this->request->server) ? $this->request->server['PHP_AUTH_PW'] : null;
 
diff --git a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php
index f3c1f7934efaa..986d0e577b7fa 100644
--- a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php
+++ b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php
@@ -123,10 +123,12 @@ public function testCorsIgnoredIfWithCredentialsHeaderPresent() {
 	}
 
 	/**
+	 * CORS must not be enforced for anonymous users on public pages
+	 *
 	 * @CORS
 	 * @PublicPage
 	 */
-	public function testNoCORSShouldAllowCookieAuth() {
+	public function testNoCORSOnAnonymousPublicPage() {
 		$request = new Request(
 			[],
 			$this->createMock(IRequestId::class),
@@ -134,6 +136,9 @@ public function testNoCORSShouldAllowCookieAuth() {
 		);
 		$this->reflector->reflect($this, __FUNCTION__);
 		$middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler);
+		$this->session->expects($this->once())
+			->method('isLoggedIn')
+			->willReturn(false);
 		$this->session->expects($this->never())
 			->method('logout');
 		$this->session->expects($this->never())
@@ -145,6 +150,35 @@ public function testNoCORSShouldAllowCookieAuth() {
 		$middleware->beforeController($this->controller, __FUNCTION__);
 	}
 
+	/**
+	 * Even on public pages users logged in using session cookies,
+	 * that do not provide a valid CSRF token are disallowed
+	 *
+	 * @CORS
+	 * @PublicPage
+	 */
+	public function testCORSShouldNeverAllowCookieAuth() {
+		$request = new Request(
+			[],
+			$this->createMock(IRequestId::class),
+			$this->createMock(IConfig::class)
+		);
+		$this->reflector->reflect($this, __FUNCTION__);
+		$middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler);
+		$this->session->expects($this->once())
+			->method('isLoggedIn')
+			->willReturn(true);
+		$this->session->expects($this->once())
+			->method('logout');
+		$this->session->expects($this->never())
+			->method('logClientIn')
+			->with($this->equalTo('user'), $this->equalTo('pass'))
+			->willReturn(true);
+
+		$this->expectException(SecurityException::class);
+		$middleware->beforeController($this->controller, __FUNCTION__);
+	}
+
 	/**
 	 * @CORS
 	 */