From 1dd168488554def9db33a2abbc3697332bb4bb65 Mon Sep 17 00:00:00 2001
From: Philipp Holzer <admin@philipp.info>
Date: Wed, 10 Apr 2019 20:38:39 +0200
Subject: [PATCH] Hardening save method in BaseURL

---
 src/Util/BaseURL.php           | 43 +++++++++++++++++-------
 tests/src/Util/BaseURLTest.php | 61 ++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+), 12 deletions(-)

diff --git a/src/Util/BaseURL.php b/src/Util/BaseURL.php
index 66526ff2db..cd703a9177 100644
--- a/src/Util/BaseURL.php
+++ b/src/Util/BaseURL.php
@@ -137,35 +137,54 @@ class BaseURL
 	 */
 	public function save($hostname = null, $sslPolicy = null, $urlPath = null)
 	{
-		$success = true;
+		$currHostname  = $this->hostname;
+		$currSSLPolicy = $this->sslPolicy;
+		$currURLPath   = $this->urlPath;
 
 		if (!empty($hostname) && $hostname !== $this->hostname) {
-			$this->hostname  = $hostname;
-			if (!$this->config->set('config', 'hostname', $this->hostname)) {
-				$success = false;
+			if ($this->config->set('config', 'hostname', $hostname)) {
+				$this->hostname  = $hostname;
+			} else {
+				return false;
 			}
 		}
 
 		if (isset($sslPolicy) && $sslPolicy !== $this->sslPolicy) {
-			$this->sslPolicy = $sslPolicy;
-			if (!$this->config->set('system', 'ssl_policy', $this->sslPolicy)) {
-				$success = false;
+			if ($this->config->set('system', 'ssl_policy', $sslPolicy)) {
+				$this->sslPolicy = $sslPolicy;
+			} else {
+				$this->hostname  = $currHostname;
+				$this->config->set('config', 'hostname', $this->hostname);
+				return false;
 			}
 		}
 
 		if (isset($urlPath) && $urlPath !== $this->urlPath) {
-			$this->urlPath   = $urlPath;
-			if (!$this->config->set('system', 'urlpath', $this->urlPath)) {
-				$success = false;
+			if ($this->config->set('system', 'urlpath', $urlPath)) {
+				$this->urlPath = $urlPath;
+			} else {
+				$this->hostname  = $currHostname;
+				$this->sslPolicy = $currSSLPolicy;
+				$this->config->set('config', 'hostname', $this->hostname);
+				$this->config->set('system', 'ssl_policy', $this->sslPolicy);
+				return false;
 			}
 		}
 
 		$this->determineBaseUrl();
 		if (!$this->config->set('system', 'url', $this->url)) {
-			$success = false;
+			$this->hostname  = $currHostname;
+			$this->sslPolicy = $currSSLPolicy;
+			$this->urlPath   = $currURLPath;
+			$this->determineBaseUrl();
+
+			$this->config->set('config', 'hostname', $this->hostname);
+			$this->config->set('system', 'ssl_policy', $this->sslPolicy);
+			$this->config->set('system', 'urlpath', $this->urlPath);
+			return false;
 		}
 
-		return $success;
+		return true;
 	}
 
 	/**
diff --git a/tests/src/Util/BaseURLTest.php b/tests/src/Util/BaseURLTest.php
index c819a22e5f..ee88bd9804 100644
--- a/tests/src/Util/BaseURLTest.php
+++ b/tests/src/Util/BaseURLTest.php
@@ -470,4 +470,65 @@ class BaseURLTest extends MockedTest
 
 		$this->assertEquals($redirect, $baseUrl->checkRedirectHttps());
 	}
+
+	public function dataWrongSave()
+	{
+		return [
+			'wrongHostname' => [
+				'fail' => 'hostname',
+			],
+			'wrongSSLPolicy' => [
+				'fail' => 'sslPolicy',
+			],
+			'wrongURLPath' => [
+				'fail' => 'urlPath',
+			],
+			'wrongURL' => [
+				'fail' => 'url',
+			],
+		];
+	}
+
+	/**
+	 * Test the save() method with wrong parameters
+	 * @dataProvider dataWrongSave
+	 */
+	public function testWrongSave($fail)
+	{
+		$configMock = \Mockery::mock(Configuration::class);
+		$configMock->shouldReceive('get')->with('config', 'hostname')->andReturn('friendica.local');
+		$configMock->shouldReceive('get')->with('system', 'urlpath')->andReturn('new/test');
+		$configMock->shouldReceive('get')->with('system', 'ssl_policy')->andReturn(BaseURL::DEFAULT_SSL_SCHEME);
+		$configMock->shouldReceive('get')->with('system', 'url')->andReturn('http://friendica.local/new/test');
+
+		switch ($fail) {
+			case 'hostname':
+				$configMock->shouldReceive('set')->with('config', 'hostname', \Mockery::any())->andReturn(false)->once();
+				break;
+			case 'sslPolicy':
+				$configMock->shouldReceive('set')->with('config', 'hostname', \Mockery::any())->andReturn(true)->twice();
+				$configMock->shouldReceive('set')->with('system', 'ssl_policy', \Mockery::any())->andReturn(false)->once();
+				break;
+			case 'urlPath':
+				$configMock->shouldReceive('set')->with('config', 'hostname', \Mockery::any())->andReturn(true)->twice();
+				$configMock->shouldReceive('set')->with('system', 'ssl_policy', \Mockery::any())->andReturn(true)->twice();
+				$configMock->shouldReceive('set')->with('system', 'urlpath', \Mockery::any())->andReturn(false)->once();
+				break;
+			case 'url':
+				$configMock->shouldReceive('set')->with('config', 'hostname', \Mockery::any())->andReturn(true)->twice();
+				$configMock->shouldReceive('set')->with('system', 'ssl_policy', \Mockery::any())->andReturn(true)->twice();
+				$configMock->shouldReceive('set')->with('system', 'urlpath', \Mockery::any())->andReturn(true)->twice();
+				$configMock->shouldReceive('set')->with('system', 'url', \Mockery::any())->andReturn(false)->once();
+				break;
+		}
+
+		$baseUrl = new BaseURL($configMock, []);
+		$this->assertFalse($baseUrl->save('test', 10, 'nope'));
+
+		// nothing should have changed because we never successfully saved anything
+		$this->assertEquals($baseUrl->getHostname(), 'friendica.local');
+		$this->assertEquals($baseUrl->getUrlPath(), 'new/test');
+		$this->assertEquals($baseUrl->getSSLPolicy(), BaseURL::DEFAULT_SSL_SCHEME);
+		$this->assertEquals($baseUrl->get(), 'http://friendica.local/new/test');
+	}
 }