Don't allow anchors and queries in remote urls

Signed-off-by: Joas Schilling <coding@schilljs.com>

2 files changed, 50 insertions, 30 deletions

diff --git a/apps/files_sharing/lib/Controller/ExternalSharesController.php b/apps/files_sharing/lib/Controller/ExternalSharesController.php
index d9be124b2ee..f903871ffd6 100644
--- a/apps/files_sharing/lib/Controller/ExternalSharesController.php
+++ b/apps/files_sharing/lib/Controller/ExternalSharesController.php
@@ -130,6 +130,9 @@ 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..9ef508a3d0a 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,35 +93,34 @@ 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')
@@ -129,28 +128,46 @@ class ExternalShareControllerTest extends \Test\TestCase {
 		$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));
 	}
 }