Don't allow anchors and queries in remote urls

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

diff --git a/apps/files_sharing/lib/Controller/ExternalSharesController.php b/apps/files_sharing/lib/Controller/ExternalSharesController.php
index d9be124b2ee53c3203be5629d30a0ed5752078be..f903871ffd6adf5cdab819129078a80547941e7b 100644 (file)
--- 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 8da037c3ba8e33fda3a2c2a184f018257d4a1d57..9ef508a3d0a327486dcb8a2fc1a5c91a859aaf7c 100644 (file)
--- 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));
        }
 }