From 3930ab8e8a72190933931b256aea78c3cd239953 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 12 Mar 2020 13:43:29 +0100 Subject: [PATCH] Don't allow anchors and queries in remote urls Signed-off-by: Joas Schilling --- .../Controller/ExternalSharesController.php | 3 + .../ExternalShareControllerTest.php | 77 +++++++++++-------- 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)); } } -- 2.39.5