fix: use empty() to check if header exists

Author: DeepDiver1975

changelog/unreleased/40856

@@ -0,0 +1,6 @@
+Bugfix: Request header can hold an empty string if not set
+
+Due to Apache rewrite rules originally not existing headers can hold an empty
+string.
+
+https://github.com/owncloud/core/pull/40856

lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php

@@ -142,7 +142,8 @@ public function beforeController($controller, $methodName) {
 		// CSRF check - also registers the CSRF token since the session may be closed later
 		Util::callRegister();
 		if (!$this->reflector->hasAnnotation('NoCSRFRequired')) {
-			if (!$this->request->passesCSRFCheck() && $this->request->getHeader("Authorization") === null) {
+			$hasNoAuthHeader = empty($this->request->getHeader("Authorization"));
+			if (!$this->request->passesCSRFCheck() && $hasNoAuthHeader) {
 				throw new CrossSiteRequestForgeryException();
 			}
 		}

tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php

@@ -40,6 +40,7 @@
 use OCP\ISession;
 use OCP\AppFramework\Controller;
 use OCP\IUserSession;
+use ReflectionException;
 use Test\TestCase;
 use OCP\AppFramework\Http\Response;
 use OCP\IConfig;
@@ -135,7 +136,7 @@ private function getMiddleware($isLoggedIn, $isAdminUser) {
 	 * @PublicPage
 	 * @NoCSRFRequired
 	 * @throws SecurityException
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	public function testSetNavigationEntry() {
 		$this->navigationManager->expects($this->once())
@@ -150,7 +151,7 @@ public function testSetNavigationEntry() {
 	 * @param string $method
 	 * @param string $test
 	 * @param $status
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	private function ajaxExceptionStatus($method, $test, $status) {
 		$isLoggedIn = false;
@@ -178,7 +179,7 @@ private function ajaxExceptionStatus($method, $test, $status) {
 	}
 
 	/**
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	public function testAjaxStatusLoggedInCheck() {
 		$this->ajaxExceptionStatus(
@@ -190,7 +191,7 @@ public function testAjaxStatusLoggedInCheck() {
 
 	/**
 	 * @NoCSRFRequired
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	public function testAjaxNotAdminCheck() {
 		$this->ajaxExceptionStatus(
@@ -202,7 +203,7 @@ public function testAjaxNotAdminCheck() {
 
 	/**
 	 * @PublicPage
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	public function testAjaxStatusCSRFCheck() {
 		$this->ajaxExceptionStatus(
@@ -215,10 +216,10 @@ public function testAjaxStatusCSRFCheck() {
 	/**
 	 * @PublicPage
 	 * @NoCSRFRequired
-	 * @throws \ReflectionException
-	 * @throws \ReflectionException
-	 * @throws \ReflectionException
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
+	 * @throws ReflectionException
+	 * @throws ReflectionException
+	 * @throws ReflectionException
 	 */
 	public function testAjaxStatusAllGood() {
 		$this->ajaxExceptionStatus(
@@ -247,7 +248,7 @@ public function testAjaxStatusAllGood() {
 	 * @PublicPage
 	 * @NoCSRFRequired
 	 * @throws SecurityException
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	public function testNoChecks() {
 		$this->request->expects($this->never())
@@ -265,7 +266,7 @@ public function testNoChecks() {
 	 * @param string $expects
 	 * @param bool $shouldFail
 	 * @throws SecurityException
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	private function securityCheck($method, $expects, $shouldFail=false) {
 		// admin check requires login
@@ -292,10 +293,10 @@ private function securityCheck($method, $expects, $shouldFail=false) {
 	/**
 	 * @PublicPage
 	 * @throws SecurityException
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	public function testCsrfCheck() {
-		$this->expectException(\OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException::class);
+		$this->expectException(CrossSiteRequestForgeryException::class);
 
 		$this->request->expects($this->once())
 			->method('passesCSRFCheck')
@@ -309,7 +310,7 @@ public function testCsrfCheck() {
 	 * @PublicPage
 	 * @NoCSRFRequired
 	 * @throws SecurityException
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	public function testNoCsrfCheck() {
 		$this->request->expects($this->never())
@@ -323,7 +324,7 @@ public function testNoCsrfCheck() {
 	/**
 	 * @PublicPage
 	 * @throws SecurityException
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	public function testFailCsrfCheck() {
 		$this->request->expects($this->once())
@@ -334,11 +335,29 @@ public function testFailCsrfCheck() {
 		$this->middleware->beforeController(__CLASS__, __FUNCTION__);
 	}
 
+	/**
+	 * @PublicPage
+	 * @throws SecurityException
+	 * @throws ReflectionException
+	 */
+	public function testFailCsrfCheckWithoutAuthHeader(): void {
+		$this->expectException(CrossSiteRequestForgeryException::class);
+		$this->request->expects($this->once())
+			->method('passesCSRFCheck')
+			->willReturn(false);
+		$this->request->expects($this->once())
+			->method('getHeader')
+			->willReturn('');
+
+		$this->reader->reflect(__CLASS__, __FUNCTION__);
+		$this->middleware->beforeController(__CLASS__, __FUNCTION__);
+	}
+
 	/**
 	 * @NoCSRFRequired
 	 * @NoAdminRequired
 	 * @throws SecurityException
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	public function testLoggedInCheck() {
 		$this->securityCheck(__FUNCTION__, 'isLoggedIn');
@@ -348,7 +367,7 @@ public function testLoggedInCheck() {
 	 * @NoCSRFRequired
 	 * @NoAdminRequired
 	 * @throws SecurityException
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	public function testFailLoggedInCheck() {
 		$this->securityCheck(__FUNCTION__, 'isLoggedIn', true);
@@ -357,7 +376,7 @@ public function testFailLoggedInCheck() {
 	/**
 	 * @NoCSRFRequired
 	 * @throws SecurityException
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	public function testIsAdminCheck() {
 		$this->securityCheck(__FUNCTION__, 'isAdminUser');
@@ -366,7 +385,7 @@ public function testIsAdminCheck() {
 	/**
 	 * @NoCSRFRequired
 	 * @throws SecurityException
-	 * @throws \ReflectionException
+	 * @throws ReflectionException
 	 */
 	public function testFailIsAdminCheck() {
 		$this->securityCheck(__FUNCTION__, 'isAdminUser', true);