Merge pull request #19907 from nextcloud/bugfix/noid/dont-allow-anchor-and-queries-in-remote-url

Don't allow anchors and queries in remote urls

2 files changed, 58 insertions, 31 deletions

diff --git a/apps/files_sharing/lib/Controller/ExternalSharesController.php b/apps/files_sharing/lib/Controller/ExternalSharesController.php
index d9be124b2ee..122ad0f7cf4 100644
--- a/apps/files_sharing/lib/Controller/ExternalSharesController.php
+++ b/apps/files_sharing/lib/Controller/ExternalSharesController.php
@@ -130,6 +130,10 @@ class ExternalSharesController extends Controller {
 	 * @return DataResponse
 	 */
 	public function testRemote($remote) {
+		if (strpos($remote, '#') !== false || strpos($remote, '?') !== false) {
+			return new DataResponse(false);
+		}
+
 		if (
 			$this->testUrl('https://' . $remote . '/ocs-provider/') ||
 			$this->testUrl('https://' . $remote . '/ocs-provider/index.php') ||
diff --git a/apps/files_sharing/tests/Controller/ExternalShareControllerTest.php b/apps/files_sharing/tests/Controller/ExternalShareControllerTest.php
index 8da037c3ba8..9d8ee9a9d42 100644
--- a/apps/files_sharing/tests/Controller/ExternalShareControllerTest.php
+++ b/apps/files_sharing/tests/Controller/ExternalShareControllerTest.php
@@ -29,6 +29,9 @@ use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\Http\Client\IClientService;
 use OCP\IRequest;
+use OCP\Http\Client\IResponse;
+use OCP\Http\Client\IClient;
+use OCA\Files_Sharing\External\Manager;
 
 /**
  * Class ExternalShareControllerTest
@@ -45,12 +48,9 @@ class ExternalShareControllerTest extends \Test\TestCase {
 
 	protected function setUp(): void {
 		parent::setUp();
-		$this->request = $this->getMockBuilder('\\OCP\\IRequest')
-			->disableOriginalConstructor()->getMock();
-		$this->externalManager = $this->getMockBuilder('\\OCA\\Files_Sharing\\External\\Manager')
-			->disableOriginalConstructor()->getMock();
-		$this->clientService = $this->getMockBuilder('\\OCP\Http\\Client\\IClientService')
-			->disableOriginalConstructor()->getMock();
+		$this->request = $this->createMock(IRequest::class);
+		$this->externalManager = $this->createMock(Manager::class);
+		$this->clientService = $this->createMock(IClientService::class);
 	}
 
 	/**
@@ -69,7 +69,7 @@ class ExternalShareControllerTest extends \Test\TestCase {
 		$this->externalManager
 			->expects($this->once())
 			->method('getOpenShares')
-			->will($this->returnValue(['MyDummyArray']));
+			->willReturn(['MyDummyArray']);
 
 		$this->assertEquals(new JSONResponse(['MyDummyArray']), $this->getExternalShareController()->index());
 	}
@@ -93,64 +93,87 @@ class ExternalShareControllerTest extends \Test\TestCase {
 	}
 
 	public function testRemoteWithValidHttps() {
-		$client = $this->getMockBuilder('\\OCP\\Http\\Client\\IClient')
-			->disableOriginalConstructor()->getMock();
-		$response = $this->getMockBuilder('\\OCP\\Http\\Client\\IResponse')
-			->disableOriginalConstructor()->getMock();
+		$client = $this->createMock(IClient::class);
+		$response = $this->createMock(IResponse::class);
 		$response
 			->expects($this->exactly(2))
 			->method('getBody')
-			->will($this->onConsecutiveCalls('Certainly not a JSON string', '{"installed":true,"maintenance":false,"version":"8.1.0.8","versionstring":"8.1.0","edition":""}'));
+			->willReturnOnConsecutiveCalls(
+				'Certainly not a JSON string',
+				'{"installed":true,"maintenance":false,"version":"8.1.0.8","versionstring":"8.1.0","edition":""}'
+			);
 		$client
 			->expects($this->any())
 			->method('get')
-			->will($this->returnValue($response));
+			->willReturn($response);
 
 		$this->clientService
 			->expects($this->exactly(2))
 			->method('newClient')
-			->will($this->returnValue($client));
+			->willReturn($client);
 
-		$this->assertEquals(new DataResponse('https'), $this->getExternalShareController()->testRemote('owncloud.org'));
+		$this->assertEquals(new DataResponse('https'), $this->getExternalShareController()->testRemote('nextcloud.com'));
 	}
 
 	public function testRemoteWithWorkingHttp() {
-		$client = $this->getMockBuilder('\\OCP\\Http\\Client\\IClient')
-			->disableOriginalConstructor()->getMock();
-		$response = $this->getMockBuilder('\\OCP\\Http\\Client\\IResponse')
-			->disableOriginalConstructor()->getMock();
+		$client = $this->createMock(IClient::class);
+		$response = $this->createMock(IResponse::class);
 		$client
 			->method('get')
-			->will($this->returnValue($response));
+			->willReturn($response);
 		$response
 			->expects($this->exactly(5))
 			->method('getBody')
-			->will($this->onConsecutiveCalls('Certainly not a JSON string', 'Certainly not a JSON string', 'Certainly not a JSON string', 'Certainly not a JSON string', '{"installed":true,"maintenance":false,"version":"8.1.0.8","versionstring":"8.1.0","edition":""}'));
+			->willReturnOnConsecutiveCalls(
+				'Certainly not a JSON string',
+				'Certainly not a JSON string',
+				'Certainly not a JSON string',
+				'Certainly not a JSON string',
+				'{"installed":true,"maintenance":false,"version":"8.1.0.8","versionstring":"8.1.0","edition":""}'
+			);
 		$this->clientService
 			->expects($this->exactly(5))
 			->method('newClient')
-			->will($this->returnValue($client));
+			->willReturn($client);
 
-		$this->assertEquals(new DataResponse('http'), $this->getExternalShareController()->testRemote('owncloud.org'));
+		$this->assertEquals(new DataResponse('http'), $this->getExternalShareController()->testRemote('nextcloud.com'));
 	}
 
 	public function testRemoteWithInvalidRemote() {
-		$client = $this->getMockBuilder('\\OCP\\Http\\Client\\IClient')
-			->disableOriginalConstructor()->getMock();
-		$response = $this->getMockBuilder('\\OCP\\Http\\Client\\IResponse')
-			->disableOriginalConstructor()->getMock();
+		$client = $this->createMock(IClient::class);
+		$response = $this->createMock(IResponse::class);
 		$client
+			->expects($this->exactly(6))
 			->method('get')
-			->will($this->returnValue($response));
+			->willReturn($response);
 		$response
 			->expects($this->exactly(6))
 			->method('getBody')
-			->will($this->returnValue('Certainly not a JSON string'));
+			->willReturn('Certainly not a JSON string');
 		$this->clientService
 			->expects($this->exactly(6))
 			->method('newClient')
-			->will($this->returnValue($client));
+			->willReturn($client);
+
+		$this->assertEquals(new DataResponse(false), $this->getExternalShareController()->testRemote('nextcloud.com'));
+	}
+
+	public function dataRemoteWithInvalidRemoteURLs(): array {
+		return [
+			['nextcloud.com?query'],
+			['nextcloud.com/#anchor'],
+		];
+	}
+
+	/**
+	 * @dataProvider dataRemoteWithInvalidRemoteURLs
+	 * @param string $remote
+	 */
+	public function testRemoteWithInvalidRemoteURLs(string $remote) {
+		$this->clientService
+			->expects($this->never())
+			->method('newClient');
 
-		$this->assertEquals(new DataResponse(false), $this->getExternalShareController()->testRemote('owncloud.org'));
+		$this->assertEquals(new DataResponse(false), $this->getExternalShareController()->testRemote($remote));
 	}
 }