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>
summersab · Jun 26, 2023 · 17336d8 · 17336d8
1 parent 83a8a63
commit 17336d8
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 2 deletions.
diff --git 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
@@ -123,17 +123,22 @@ 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),
 			$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(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
 	 */