Merge pull request #20138 from nextcloud/bugfix/noid/make-remote-checking-more-generic

Make remote checking more generic

2 files changed, 65 insertions, 43 deletions

diff --git a/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php b/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php
index fadf61fd7de..8883a5d353c 100644
--- a/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php
+++ b/apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php
@@ -32,6 +32,7 @@ use GuzzleHttp\HandlerStack;
 use GuzzleHttp\Middleware;
 use OCA\DAV\CalDAV\CalDavBackend;
 use OCP\Http\Client\IClientService;
+use OCP\Http\Client\LocalServerException;
 use OCP\IConfig;
 use OCP\ILogger;
 use Psr\Http\Message\RequestInterface;
@@ -215,48 +216,15 @@ class RefreshWebcalService {
 			return null;
 		}
 
-		if ($allowLocalAccess !== 'yes') {
-			$host = strtolower(parse_url($url, PHP_URL_HOST));
-			// remove brackets from IPv6 addresses
-			if (strpos($host, '[') === 0 && substr($host, -1) === ']') {
-				$host = substr($host, 1, -1);
-			}
-
-			// Disallow localhost and local network
-			if ($host === 'localhost' || substr($host, -6) === '.local' || substr($host, -10) === '.localhost') {
-				$this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules");
-				return null;
-			}
-
-			// Disallow hostname only
-			if (substr_count($host, '.') === 0) {
-				$this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules");
-				return null;
-			}
-
-			if ((bool)filter_var($host, FILTER_VALIDATE_IP) && !filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) {
-				$this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules");
-				return null;
-			}
-
-			// Also check for IPv6 IPv4 nesting, because that's not covered by filter_var
-			if ((bool)filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) && substr_count($host, '.') > 0) {
-				$delimiter = strrpos($host, ':'); // Get last colon
-				$ipv4Address = substr($host, $delimiter + 1);
-
-				if (!filter_var($ipv4Address, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) {
-					$this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules");
-					return null;
-				}
-			}
-		}
-
 		try {
 			$params = [
 				'allow_redirects' => [
 					'redirects' => 10
 				],
 				'handler' => $handlerStack,
+				'nextcloud' => [
+					'allow_local_address' => $allowLocalAccess === 'yes',
+				]
 			];
 
 			$user = parse_url($subscription['source'], PHP_URL_USER);
@@ -306,9 +274,18 @@ class RefreshWebcalService {
 					}
 					return $vCalendar->serialize();
 			}
+		} catch (LocalServerException $ex) {
+			$this->logger->logException($ex, [
+				'message' => "Subscription $subscriptionId was not refreshed because it violates local access rules",
+				'level' => ILogger::WARN,
+			]);
+
+			return null;
 		} catch (Exception $ex) {
-			$this->logger->logException($ex);
-			$this->logger->warning("Subscription $subscriptionId could not be refreshed due to a network error");
+			$this->logger->logException($ex, [
+				'message' => "Subscription $subscriptionId could not be refreshed due to a network error",
+				'level' => ILogger::WARN,
+			]);
 
 			return null;
 		}
diff --git a/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php b/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php
index 8de32ad8c35..e662f4651a0 100644
--- a/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php
+++ b/apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php
@@ -31,6 +31,7 @@ use OCA\DAV\CalDAV\WebcalCaching\RefreshWebcalService;
 use OCP\Http\Client\IClient;
 use OCP\Http\Client\IClientService;
 use OCP\Http\Client\IResponse;
+use OCP\Http\Client\LocalServerException;
 use OCP\IConfig;
 use OCP\ILogger;
 use PHPUnit\Framework\MockObject\MockObject;
@@ -170,8 +171,12 @@ class RefreshWebcalServiceTest extends TestCase {
 	 * @param string $source
 	 */
 	public function testRunLocalURL($source) {
-		$refreshWebcalService = new RefreshWebcalService($this->caldavBackend,
-			$this->clientService, $this->config, $this->logger);
+		$refreshWebcalService = new RefreshWebcalService(
+			$this->caldavBackend,
+			$this->clientService,
+			$this->config,
+			$this->logger
+		);
 
 		$this->caldavBackend->expects($this->once())
 			->method('getSubscriptionsForUser')
@@ -199,8 +204,13 @@ class RefreshWebcalServiceTest extends TestCase {
 			->with('dav', 'webcalAllowLocalAccess', 'no')
 			->willReturn('no');
 
-		$client->expects($this->never())
-			->method('get');
+		$client->expects($this->once())
+			->method('get')
+			->willThrowException(new LocalServerException());
+
+		$this->logger->expects($this->once())
+			->method('logException')
+			->with($this->isInstanceOf(LocalServerException::class), $this->anything());
 
 		$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
 	}
@@ -221,7 +231,42 @@ class RefreshWebcalServiceTest extends TestCase {
 			['10.0.0.1'],
 			['another-host.local'],
 			['service.localhost'],
-			['!@#$'], // test invalid url
 		];
 	}
+
+	public function testInvalidUrl() {
+		$refreshWebcalService = new RefreshWebcalService($this->caldavBackend,
+			$this->clientService, $this->config, $this->logger);
+
+		$this->caldavBackend->expects($this->once())
+			->method('getSubscriptionsForUser')
+			->with('principals/users/testuser')
+			->willReturn([
+				[
+					'id' => 42,
+					'uri' => 'sub123',
+					'refreshreate' => 'P1H',
+					'striptodos' => 1,
+					'stripalarms' => 1,
+					'stripattachments' => 1,
+					'source' => '!@#$'
+				],
+			]);
+
+		$client = $this->createMock(IClient::class);
+		$this->clientService->expects($this->once())
+			->method('newClient')
+			->with()
+			->willReturn($client);
+
+		$this->config->expects($this->once())
+			->method('getAppValue')
+			->with('dav', 'webcalAllowLocalAccess', 'no')
+			->willReturn('no');
+
+		$client->expects($this->never())
+			->method('get');
+
+		$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
+	}
 }