Merge pull request #40856 from owncloud/fix/header-evaluation

fix: use empty() to check if header exists

Author: phil-davis

apps/files_sharing/lib/Controller/Share20OcsController.php

@@ -281,6 +281,7 @@ protected function formatShare(IShare $share, $received = false) {
 	 * Get a specific share by id
 	 *
 	 * @NoAdminRequired
+	 * @NoCSRFRequired
 	 *
 	 * @param string $id
 	 * @return Result
@@ -664,6 +665,7 @@ private function getSharedWithMe($node = null, $includeTags, $requestedShareType
 	 * the function will return an empty list.
 	 *
 	 * @NoAdminRequired
+	 * @NoCSRFRequired
 	 *
 	 * - Get shares by the current user
 	 * - Get shares by the current user and reshares (?reshares=true)

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

core/Controller/OcsController.php

@@ -110,6 +110,7 @@ public function checkPerson($login, $password) {
 	 * test: curl http://login:passwd@oc/core/ocs/v1.php/privatedata/getattribute
 	 *
 	 * @NoAdminRequired
+	 * @NoCSRFRequired
 	 *
 	 * @return Result
 	 */

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

@@ -143,7 +143,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 = ($this->request->getHeader("Authorization") === null || trim($this->request->getHeader("Authorization")) === '');
+			if (!$this->request->passesCSRFCheck() && $hasNoAuthHeader) {
 				throw new CrossSiteRequestForgeryException();
 			}
 		}

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

@@ -79,33 +79,33 @@ protected function setUp(): void {
 
 		$this->controller = $this->getMockBuilder(Controller::class)
 			->disableOriginalConstructor()
-				->getMock();
+			->getMock();
 		$this->reader = new ControllerMethodReflector();
 		$this->logger = $this->getMockBuilder(
 			ILogger::class
 		)
-				->disableOriginalConstructor()
-				->getMock();
+			->disableOriginalConstructor()
+			->getMock();
 		$this->navigationManager = $this->getMockBuilder(
 			INavigationManager::class
 		)
-				->disableOriginalConstructor()
-				->getMock();
+			->disableOriginalConstructor()
+			->getMock();
 		$this->urlGenerator = $this->getMockBuilder(
 			IURLGenerator::class
 		)
-				->disableOriginalConstructor()
-				->getMock();
+			->disableOriginalConstructor()
+			->getMock();
 		$this->request = $this->getMockBuilder(
 			IRequest::class
 		)
-				->disableOriginalConstructor()
-				->getMock();
+			->disableOriginalConstructor()
+			->getMock();
 		$this->contentSecurityPolicyManager = $this->getMockBuilder(
 			ContentSecurityPolicyManager::class
 		)
-				->disableOriginalConstructor()
-				->getMock();
+			->disableOriginalConstructor()
+			->getMock();
 		$this->middleware = $this->getMiddleware(true, true);
 		$this->secException = new SecurityException('hey', false);
 		$this->secAjaxException = new SecurityException('hey', true);
@@ -252,8 +252,8 @@ public function testAjaxStatusAllGood() {
 	 */
 	public function testNoChecks() {
 		$this->request->expects($this->never())
-				->method('passesCSRFCheck')
-				->will($this->returnValue(false));
+			->method('passesCSRFCheck')
+			->will($this->returnValue(false));
 
 		$sec = $this->getMiddleware(false, false);
 
@@ -388,30 +388,30 @@ public function testAfterExceptionNotCaughtThrowsItAgain() {
 	public function testAfterExceptionReturnsRedirectForNotLoggedInUser() {
 		$this->request = new Request(
 			[
-						'server' =>
-								[
-										'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
-										'REQUEST_URI' => 'owncloud/index.php/apps/specialapp'
-								]
-				],
+				'server' =>
+					[
+						'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
+						'REQUEST_URI' => 'owncloud/index.php/apps/specialapp'
+					]
+			],
 			$this->createMock(ISecureRandom::class),
 			$this->createMock(IConfig::class)
 		);
 		$this->middleware = $this->getMiddleware(false, false);
 		$this->urlGenerator
-				->expects($this->once())
-				->method('linkToRoute')
-				->with(
-					'core.login.showLoginForm',
-					[
-						'redirect_url' => 'owncloud%2Findex.php%2Fapps%2Fspecialapp',
-					]
-				)
-				->will($this->returnValue('http://localhost/index.php/login?redirect_url=owncloud%2Findex.php%2Fapps%2Fspecialapp'));
+			->expects($this->once())
+			->method('linkToRoute')
+			->with(
+				'core.login.showLoginForm',
+				[
+					'redirect_url' => 'owncloud%2Findex.php%2Fapps%2Fspecialapp',
+				]
+			)
+			->will($this->returnValue('http://localhost/index.php/login?redirect_url=owncloud%2Findex.php%2Fapps%2Fspecialapp'));
 		$this->logger
-				->expects($this->once())
-				->method('debug')
-				->with('Current user is not logged in');
+			->expects($this->once())
+			->method('debug')
+			->with('Current user is not logged in');
 		$response = $this->middleware->afterException(
 			$this->controller,
 			'test',
@@ -447,20 +447,20 @@ public function exceptionProvider() {
 	public function testAfterExceptionReturnsTemplateResponse(SecurityException $exception) {
 		$this->request = new Request(
 			[
-						'server' =>
-								[
-										'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
-										'REQUEST_URI' => 'owncloud/index.php/apps/specialapp'
-								]
-				],
+				'server' =>
+					[
+						'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
+						'REQUEST_URI' => 'owncloud/index.php/apps/specialapp'
+					]
+			],
 			$this->createMock(ISecureRandom::class),
 			$this->createMock(IConfig::class)
 		);
 		$this->middleware = $this->getMiddleware(false, false);
 		$this->logger
-				->expects($this->once())
-				->method('debug')
-				->with($exception->getMessage());
+			->expects($this->once())
+			->method('debug')
+			->with($exception->getMessage());
 		$response = $this->middleware->afterException(
 			$this->controller,
 			'test',
@@ -628,10 +628,10 @@ public function testAfterController() {
 			->method('getDefaultPolicy')
 			->willReturn($defaultPolicy);
 		$this->contentSecurityPolicyManager
-				->expects($this->once())
-				->method('mergePolicies')
-				->with($defaultPolicy, $currentPolicy)
-				->willReturn($mergedPolicy);
+			->expects($this->once())
+			->method('mergePolicies')
+			->with($defaultPolicy, $currentPolicy)
+			->willReturn($mergedPolicy);
 		$response->expects($this->once())
 			->method('setContentSecurityPolicy')
 			->with($mergedPolicy);