Skip to content

Commit 5c0637b

Browse files
nickvergessennickvergessen
authored
Merge pull request #20138 from nextcloud/bugfix/noid/make-remote-checking-more-generic
Make remote checking more generic
2 parents 95ad9ab + 4e846dd commit 5c0637b

File tree

14 files changed

+300
-65
lines changed

14 files changed

+300
-65
lines changed

‎apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php‎

Lines changed: 15 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
use GuzzleHttp\Middleware;
3333
use OCA\DAV\CalDAV\CalDavBackend;
3434
use OCP\Http\Client\IClientService;
35+
use OCP\Http\Client\LocalServerException;
3536
use OCP\IConfig;
3637
use OCP\ILogger;
3738
use Psr\Http\Message\RequestInterface;
@@ -215,48 +216,15 @@ private function queryWebcalFeed(array $subscription, array &$mutations) {
215216
return null;
216217
}
217218

218-
if ($allowLocalAccess !== 'yes') {
219-
$host = strtolower(parse_url($url, PHP_URL_HOST));
220-
// remove brackets from IPv6 addresses
221-
if (strpos($host, '[') === 0 && substr($host, -1) === ']') {
222-
$host = substr($host, 1, -1);
223-
}
224-
225-
// Disallow localhost and local network
226-
if ($host === 'localhost' || substr($host, -6) === '.local' || substr($host, -10) === '.localhost') {
227-
$this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules");
228-
return null;
229-
}
230-
231-
// Disallow hostname only
232-
if (substr_count($host, '.') === 0) {
233-
$this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules");
234-
return null;
235-
}
236-
237-
if ((bool)filter_var($host, FILTER_VALIDATE_IP) && !filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) {
238-
$this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules");
239-
return null;
240-
}
241-
242-
// Also check for IPv6 IPv4 nesting, because that's not covered by filter_var
243-
if ((bool)filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) && substr_count($host, '.') > 0) {
244-
$delimiter = strrpos($host, ':'); // Get last colon
245-
$ipv4Address = substr($host, $delimiter + 1);
246-
247-
if (!filter_var($ipv4Address, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) {
248-
$this->logger->warning("Subscription $subscriptionId was not refreshed because it violates local access rules");
249-
return null;
250-
}
251-
}
252-
}
253-
254219
try {
255220
$params = [
256221
'allow_redirects' => [
257222
'redirects' => 10
258223
],
259224
'handler' => $handlerStack,
225+
'nextcloud' => [
226+
'allow_local_address' => $allowLocalAccess === 'yes',
227+
]
260228
];
261229

262230
$user = parse_url($subscription['source'], PHP_URL_USER);
@@ -306,9 +274,18 @@ private function queryWebcalFeed(array $subscription, array &$mutations) {
306274
}
307275
return $vCalendar->serialize();
308276
}
277+
} catch (LocalServerException $ex) {
278+
$this->logger->logException($ex, [
279+
'message' => "Subscription $subscriptionId was not refreshed because it violates local access rules",
280+
'level' => ILogger::WARN,
281+
]);
282+
283+
return null;
309284
} catch (Exception $ex) {
310-
$this->logger->logException($ex);
311-
$this->logger->warning("Subscription $subscriptionId could not be refreshed due to a network error");
285+
$this->logger->logException($ex, [
286+
'message' => "Subscription $subscriptionId could not be refreshed due to a network error",
287+
'level' => ILogger::WARN,
288+
]);
312289

313290
return null;
314291
}

‎apps/dav/tests/unit/CalDAV/WebcalCaching/RefreshWebcalServiceTest.php‎

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
use OCP\Http\Client\IClient;
3232
use OCP\Http\Client\IClientService;
3333
use OCP\Http\Client\IResponse;
34+
use OCP\Http\Client\LocalServerException;
3435
use OCP\IConfig;
3536
use OCP\ILogger;
3637
use PHPUnit\Framework\MockObject\MockObject;
@@ -170,8 +171,12 @@ public function runDataProvider():array {
170171
* @param string $source
171172
*/
172173
public function testRunLocalURL($source) {
173-
$refreshWebcalService = new RefreshWebcalService($this->caldavBackend,
174-
$this->clientService, $this->config, $this->logger);
174+
$refreshWebcalService = new RefreshWebcalService(
175+
$this->caldavBackend,
176+
$this->clientService,
177+
$this->config,
178+
$this->logger
179+
);
175180

176181
$this->caldavBackend->expects($this->once())
177182
->method('getSubscriptionsForUser')
@@ -199,8 +204,13 @@ public function testRunLocalURL($source) {
199204
->with('dav', 'webcalAllowLocalAccess', 'no')
200205
->willReturn('no');
201206

202-
$client->expects($this->never())
203-
->method('get');
207+
$client->expects($this->once())
208+
->method('get')
209+
->willThrowException(new LocalServerException());
210+
211+
$this->logger->expects($this->once())
212+
->method('logException')
213+
->with($this->isInstanceOf(LocalServerException::class), $this->anything());
204214

205215
$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
206216
}
@@ -221,7 +231,42 @@ public function runLocalURLDataProvider():array {
221231
['10.0.0.1'],
222232
['another-host.local'],
223233
['service.localhost'],
224-
['!@#$'], // test invalid url
225234
];
226235
}
236+
237+
public function testInvalidUrl() {
238+
$refreshWebcalService = new RefreshWebcalService($this->caldavBackend,
239+
$this->clientService, $this->config, $this->logger);
240+
241+
$this->caldavBackend->expects($this->once())
242+
->method('getSubscriptionsForUser')
243+
->with('principals/users/testuser')
244+
->willReturn([
245+
[
246+
'id' => 42,
247+
'uri' => 'sub123',
248+
'refreshreate' => 'P1H',
249+
'striptodos' => 1,
250+
'stripalarms' => 1,
251+
'stripattachments' => 1,
252+
'source' => '!@#$'
253+
],
254+
]);
255+
256+
$client = $this->createMock(IClient::class);
257+
$this->clientService->expects($this->once())
258+
->method('newClient')
259+
->with()
260+
->willReturn($client);
261+
262+
$this->config->expects($this->once())
263+
->method('getAppValue')
264+
->with('dav', 'webcalAllowLocalAccess', 'no')
265+
->willReturn('no');
266+
267+
$client->expects($this->never())
268+
->method('get');
269+
270+
$refreshWebcalService->refreshSubscription('principals/users/testuser', 'sub123');
271+
}
227272
}

‎apps/files_sharing/lib/Controller/ExternalSharesController.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ protected function testUrl($remote, $checkVersion = false) {
131131
* @return DataResponse
132132
*/
133133
public function testRemote($remote) {
134-
if (strpos($remote, '#') !== false || strpos($remote, '?') !== false) {
134+
if (strpos($remote, '#') !== false || strpos($remote, '?') !== false || strpos($remote, ';') !== false) {
135135
return new DataResponse(false);
136136
}
137137

‎apps/files_sharing/tests/Controller/ExternalShareControllerTest.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,7 @@ public function dataRemoteWithInvalidRemoteURLs(): array {
162162
return [
163163
['nextcloud.com?query'],
164164
['nextcloud.com/#anchor'],
165+
['nextcloud.com/;tomcat'],
165166
];
166167
}
167168

‎build/integration/run.sh‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ INSTALLED=$($OCC status | grep installed: | cut -d " " -f 5)
1616
if [ "$INSTALLED" == "true" ]; then
1717
# Disable bruteforce protection because the integration tests do trigger them
1818
$OCC config:system:set auth.bruteforce.protection.enabled --value false --type bool
19+
# Allow local remote urls otherwise we can not share
20+
$OCC config:system:set allow_local_remote_servers --value true --type bool
1921
else
2022
if [ "$SCENARIO_TO_RUN" != "setup_features/setup.feature" ]; then
2123
echo "Nextcloud instance needs to be installed" >&2

‎config/config.sample.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -558,6 +558,13 @@
558558
*/
559559
'proxyexclude' => [],
560560

561+
/**
562+
* Allow remote servers with local addresses e.g. in federated shares, webcal services and more
563+
*
564+
* Defaults to false
565+
*/
566+
'allow_local_remote_servers' => true,
567+
561568
/**
562569
* Deleted Items (trash bin)
563570
*

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,7 @@
323323
'OCP\\Http\\Client\\IClient' => $baseDir . '/lib/public/Http/Client/IClient.php',
324324
'OCP\\Http\\Client\\IClientService' => $baseDir . '/lib/public/Http/Client/IClientService.php',
325325
'OCP\\Http\\Client\\IResponse' => $baseDir . '/lib/public/Http/Client/IResponse.php',
326+
'OCP\\Http\\Client\\LocalServerException' => $baseDir . '/lib/public/Http/Client/LocalServerException.php',
326327
'OCP\\IAddressBook' => $baseDir . '/lib/public/IAddressBook.php',
327328
'OCP\\IAppConfig' => $baseDir . '/lib/public/IAppConfig.php',
328329
'OCP\\IAvatar' => $baseDir . '/lib/public/IAvatar.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
352352
'OCP\\Http\\Client\\IClient' => __DIR__ . '/../../..' . '/lib/public/Http/Client/IClient.php',
353353
'OCP\\Http\\Client\\IClientService' => __DIR__ . '/../../..' . '/lib/public/Http/Client/IClientService.php',
354354
'OCP\\Http\\Client\\IResponse' => __DIR__ . '/../../..' . '/lib/public/Http/Client/IResponse.php',
355+
'OCP\\Http\\Client\\LocalServerException' => __DIR__ . '/../../..' . '/lib/public/Http/Client/LocalServerException.php',
355356
'OCP\\IAddressBook' => __DIR__ . '/../../..' . '/lib/public/IAddressBook.php',
356357
'OCP\\IAppConfig' => __DIR__ . '/../../..' . '/lib/public/IAppConfig.php',
357358
'OCP\\IAvatar' => __DIR__ . '/../../..' . '/lib/public/IAvatar.php',

‎lib/private/Http/Client/Client.php‎

Lines changed: 60 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,10 @@
3434
use GuzzleHttp\RequestOptions;
3535
use OCP\Http\Client\IClient;
3636
use OCP\Http\Client\IResponse;
37+
use OCP\Http\Client\LocalServerException;
3738
use OCP\ICertificateManager;
3839
use OCP\IConfig;
40+
use OCP\ILogger;
3941

4042
/**
4143
* Class Client
@@ -47,20 +49,19 @@ class Client implements IClient {
4749
private $client;
4850
/** @var IConfig */
4951
private $config;
52+
/** @var ILogger */
53+
private $logger;
5054
/** @var ICertificateManager */
5155
private $certificateManager;
5256

53-
/**
54-
* @param IConfig $config
55-
* @param ICertificateManager $certificateManager
56-
* @param GuzzleClient $client
57-
*/
5857
public function __construct(
5958
IConfig $config,
59+
ILogger $logger,
6060
ICertificateManager $certificateManager,
6161
GuzzleClient $client
6262
) {
6363
$this->config = $config;
64+
$this->logger = $logger;
6465
$this->client = $client;
6566
$this->certificateManager = $certificateManager;
6667
}
@@ -144,6 +145,53 @@ private function getProxyUri(): ?array {
144145
return $proxy;
145146
}
146147

148+
protected function preventLocalAddress(string $uri, array $options): void {
149+
if (($options['nextcloud']['allow_local_address'] ?? false) ||
150+
$this->config->getSystemValueBool('allow_local_remote_servers', false)) {
151+
return;
152+
}
153+
154+
$host = parse_url($uri, PHP_URL_HOST);
155+
if ($host === false) {
156+
$this->logger->warning("Could not detect any host in $uri");
157+
throw new LocalServerException('Could not detect any host');
158+
}
159+
160+
$host = strtolower($host);
161+
// remove brackets from IPv6 addresses
162+
if (strpos($host, '[') === 0 && substr($host, -1) === ']') {
163+
$host = substr($host, 1, -1);
164+
}
165+
166+
// Disallow localhost and local network
167+
if ($host === 'localhost' || substr($host, -6) === '.local' || substr($host, -10) === '.localhost') {
168+
$this->logger->warning("Host $host was not connected to because it violates local access rules");
169+
throw new LocalServerException('Host violates local access rules');
170+
}
171+
172+
// Disallow hostname only
173+
if (substr_count($host, '.') === 0) {
174+
$this->logger->warning("Host $host was not connected to because it violates local access rules");
175+
throw new LocalServerException('Host violates local access rules');
176+
}
177+
178+
if ((bool)filter_var($host, FILTER_VALIDATE_IP) && !filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) {
179+
$this->logger->warning("Host $host was not connected to because it violates local access rules");
180+
throw new LocalServerException('Host violates local access rules');
181+
}
182+
183+
// Also check for IPv6 IPv4 nesting, because that's not covered by filter_var
184+
if ((bool)filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) && substr_count($host, '.') > 0) {
185+
$delimiter = strrpos($host, ':'); // Get last colon
186+
$ipv4Address = substr($host, $delimiter + 1);
187+
188+
if (!filter_var($ipv4Address, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) {
189+
$this->logger->warning("Host $host was not connected to because it violates local access rules");
190+
throw new LocalServerException('Host violates local access rules');
191+
}
192+
}
193+
}
194+
147195
/**
148196
* Sends a GET request
149197
*
@@ -174,6 +222,7 @@ private function getProxyUri(): ?array {
174222
* @throws \Exception If the request could not get completed
175223
*/
176224
public function get(string $uri, array $options = []): IResponse {
225+
$this->preventLocalAddress($uri, $options);
177226
$response = $this->client->request('get', $uri, $this->buildRequestOptions($options));
178227
$isStream = isset($options['stream']) && $options['stream'];
179228
return new Response($response, $isStream);
@@ -204,6 +253,7 @@ public function get(string $uri, array $options = []): IResponse {
204253
* @throws \Exception If the request could not get completed
205254
*/
206255
public function head(string $uri, array $options = []): IResponse {
256+
$this->preventLocalAddress($uri, $options);
207257
$response = $this->client->request('head', $uri, $this->buildRequestOptions($options));
208258
return new Response($response);
209259
}
@@ -238,6 +288,8 @@ public function head(string $uri, array $options = []): IResponse {
238288
* @throws \Exception If the request could not get completed
239289
*/
240290
public function post(string $uri, array $options = []): IResponse {
291+
$this->preventLocalAddress($uri, $options);
292+
241293
if (isset($options['body']) && is_array($options['body'])) {
242294
$options['form_params'] = $options['body'];
243295
unset($options['body']);
@@ -276,6 +328,7 @@ public function post(string $uri, array $options = []): IResponse {
276328
* @throws \Exception If the request could not get completed
277329
*/
278330
public function put(string $uri, array $options = []): IResponse {
331+
$this->preventLocalAddress($uri, $options);
279332
$response = $this->client->request('put', $uri, $this->buildRequestOptions($options));
280333
return new Response($response);
281334
}
@@ -310,6 +363,7 @@ public function put(string $uri, array $options = []): IResponse {
310363
* @throws \Exception If the request could not get completed
311364
*/
312365
public function delete(string $uri, array $options = []): IResponse {
366+
$this->preventLocalAddress($uri, $options);
313367
$response = $this->client->request('delete', $uri, $this->buildRequestOptions($options));
314368
return new Response($response);
315369
}
@@ -344,6 +398,7 @@ public function delete(string $uri, array $options = []): IResponse {
344398
* @throws \Exception If the request could not get completed
345399
*/
346400
public function options(string $uri, array $options = []): IResponse {
401+
$this->preventLocalAddress($uri, $options);
347402
$response = $this->client->request('options', $uri, $this->buildRequestOptions($options));
348403
return new Response($response);
349404
}

‎lib/private/Http/Client/ClientService.php‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
use OCP\Http\Client\IClientService;
3333
use OCP\ICertificateManager;
3434
use OCP\IConfig;
35+
use OCP\ILogger;
3536

3637
/**
3738
* Class ClientService
@@ -41,23 +42,23 @@
4142
class ClientService implements IClientService {
4243
/** @var IConfig */
4344
private $config;
45+
/** @var ILogger */
46+
private $logger;
4447
/** @var ICertificateManager */
4548
private $certificateManager;
4649

47-
/**
48-
* @param IConfig $config
49-
* @param ICertificateManager $certificateManager
50-
*/
5150
public function __construct(IConfig $config,
51+
ILogger $logger,
5252
ICertificateManager $certificateManager) {
5353
$this->config = $config;
54+
$this->logger = $logger;
5455
$this->certificateManager = $certificateManager;
5556
}
5657

5758
/**
5859
* @return Client
5960
*/
6061
public function newClient(): IClient {
61-
return new Client($this->config, $this->certificateManager, new GuzzleClient());
62+
return new Client($this->config, $this->logger, $this->certificateManager, new GuzzleClient());
6263
}
6364
}

0 commit comments

Comments
 (0)