Skip to content

Commit cab0caf

Browse files
AndyScherzingerAndyScherzinger
authored
Merge pull request #63875 from nextcloud/backport/63687/stable32
[stable32] fix(TaskProcessing): Harden task scheduling with webhooks
2 parents f68348d + 484f8e9 commit cab0caf

3 files changed

Lines changed: 188 additions & 3 deletions

File tree

‎lib/private/TaskProcessing/Manager.php‎

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
use OCP\IUserSession;
4242
use OCP\L10N\IFactory;
4343
use OCP\Lock\LockedException;
44+
use OCP\Security\IRemoteHostValidator;
4445
use OCP\SpeechToText\ISpeechToTextProvider;
4546
use OCP\SpeechToText\ISpeechToTextProviderWithId;
4647
use OCP\TaskProcessing\EShapeType;
@@ -123,6 +124,7 @@ public function __construct(
123124
private IUserSession $userSession,
124125
ICacheFactory $cacheFactory,
125126
private IFactory $l10nFactory,
127+
private IRemoteHostValidator $remoteHostValidator,
126128
) {
127129
$this->appData = $appDataFactory->get('core');
128130
$this->distributedCache = $cacheFactory->createDistributed('task_processing::');
@@ -1389,7 +1391,7 @@ public function setTaskStatus(Task $task, int $status): void {
13891391
}
13901392

13911393
/**
1392-
* Validate input, fill input default values, set completionExpectedAt, set scheduledAt
1394+
* Validate input and webhook, fill input default values, set completionExpectedAt, set scheduledAt
13931395
*
13941396
* @param Task $task
13951397
* @return void
@@ -1426,6 +1428,8 @@ private function prepareTask(Task $task): void {
14261428
$this->validateFileId($fileId);
14271429
$this->validateUserAccessToFile($fileId, $task->getUserId());
14281430
}
1431+
// validate the webhook configuration
1432+
$this->validateWebhook($task);
14291433
// remove superfluous keys and set input
14301434
$input = $this->removeSuperfluousArrayKeys($task->getInput(), $inputShape, $optionalInputShape);
14311435
$inputWithDefaults = $this->fillInputDefaults($input, $inputShapeDefaults, $optionalInputShapeDefaults);
@@ -1438,6 +1442,79 @@ private function prepareTask(Task $task): void {
14381442
$task->setCompletionExpectedAt($completionExpectedAt);
14391443
}
14401444

1445+
/**
1446+
* Validate the webhook URI and webhook method of a task
1447+
*
1448+
* Both values are optional, but if one is set, the other one has to be set as well.
1449+
* Supported methods are `HTTP:<GET|POST|PUT|DELETE>`, which require an absolute
1450+
* http(s) URI pointing at a non-local host, and `AppAPI:<exAppId>:<GET|POST|PUT|DELETE>`,
1451+
* which requires an absolute path as URI.
1452+
*
1453+
* @param Task $task
1454+
* @return void
1455+
* @throws ValidationException
1456+
*/
1457+
private function validateWebhook(Task $task): void {
1458+
$uri = $task->getWebhookUri();
1459+
$method = $task->getWebhookMethod();
1460+
1461+
if (($uri === null || $uri === '') && ($method === null || $method === '')) {
1462+
return;
1463+
}
1464+
if ($uri === null || $uri === '') {
1465+
throw new ValidationException('Webhook URI is required when a webhook method is set');
1466+
}
1467+
if ($method === null || $method === '') {
1468+
throw new ValidationException('Webhook method is required when a webhook URI is set');
1469+
}
1470+
if (mb_strlen($uri) > 4000) {
1471+
throw new ValidationException('Webhook URI is too long, maximum length is 4000 characters');
1472+
}
1473+
if (mb_strlen($method) > 64) {
1474+
throw new ValidationException('Webhook method is too long, maximum length is 64 characters');
1475+
}
1476+
1477+
if (str_starts_with($method, 'HTTP:')) {
1478+
if (!in_array($method, ['HTTP:GET', 'HTTP:POST', 'HTTP:PUT', 'HTTP:DELETE'], true)) {
1479+
throw new ValidationException('Invalid webhook method: ' . $method);
1480+
}
1481+
if (filter_var($uri, FILTER_VALIDATE_URL) === false) {
1482+
throw new ValidationException('Invalid webhook URI: ' . $uri);
1483+
}
1484+
$parsedUri = parse_url($uri);
1485+
if ($parsedUri === false || !isset($parsedUri['scheme']) || !isset($parsedUri['host']) || $parsedUri['host'] === '') {
1486+
throw new ValidationException('Invalid webhook URI: ' . $uri);
1487+
}
1488+
if (!in_array(strtolower($parsedUri['scheme']), ['http', 'https'], true)) {
1489+
throw new ValidationException('Invalid webhook URI scheme, only http and https are supported: ' . $uri);
1490+
}
1491+
if (!$this->remoteHostValidator->isValid($parsedUri['host'])) {
1492+
throw new ValidationException('Invalid webhook URI, the host is not allowed to be connected to: ' . $uri);
1493+
}
1494+
return;
1495+
}
1496+
1497+
if (str_starts_with($method, 'AppAPI:')) {
1498+
$parsedMethod = explode(':', $method);
1499+
if (count($parsedMethod) !== 3) {
1500+
throw new ValidationException('Invalid webhook method: ' . $method);
1501+
}
1502+
[, $exAppId, $httpMethod] = $parsedMethod;
1503+
if (preg_match('/^[a-z][a-z0-9_-]*$/', $exAppId) !== 1) {
1504+
throw new ValidationException('Invalid ExApp ID in webhook method: ' . $method);
1505+
}
1506+
if (!in_array($httpMethod, ['GET', 'POST', 'PUT', 'DELETE'], true)) {
1507+
throw new ValidationException('Invalid webhook method: ' . $method);
1508+
}
1509+
if (!str_starts_with($uri, '/')) {
1510+
throw new ValidationException('Invalid webhook URI, an absolute path is required for AppAPI webhooks: ' . $uri);
1511+
}
1512+
return;
1513+
}
1514+
1515+
throw new ValidationException('Invalid webhook method: ' . $method);
1516+
}
1517+
14411518
/**
14421519
* Store the task in the DB and set its ID in the \OCP\TaskProcessing\Task input param
14431520
*

‎lib/public/TaskProcessing/IManager.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ public function getAvailableTaskTypeIds(bool $showDisabled = false, ?string $use
6767
/**
6868
* @param Task $task The task to run
6969
* @throws PreConditionNotMetException If no or not the requested provider was registered but this method was still called
70-
* @throws ValidationException the given task input didn't pass validation against the task type's input shape and/or the providers optional input shape specs
70+
* @throws ValidationException the given task input didn't pass validation against the task type's input shape and/or the providers optional input shape specs, or the specified webhook didn't pass validation
7171
* @throws Exception storing the task in the database failed
7272
* @throws UnauthorizedException the user scheduling the task does not have access to the files used in the input
7373
* @since 30.0.0
@@ -80,7 +80,7 @@ public function scheduleTask(Task $task): void;
8080
* @param Task $task The task to run
8181
* @return Task The result task
8282
* @throws PreConditionNotMetException If no or not the requested provider was registered but this method was still called
83-
* @throws ValidationException the given task input didn't pass validation against the task type's input shape and/or the providers optional input shape specs
83+
* @throws ValidationException the given task input didn't pass validation against the task type's input shape and/or the providers optional input shape specs, or the specified webhook didn't pass validation
8484
* @throws Exception storing the task in the database failed
8585
* @throws UnauthorizedException the user scheduling the task does not have access to the files used in the input
8686
* @since 30.0.0

‎tests/lib/TaskProcessing/TaskProcessingTest.php‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
use OCP\IUserManager;
3434
use OCP\IUserSession;
3535
use OCP\L10N\IFactory;
36+
use OCP\Security\IRemoteHostValidator;
3637
use OCP\Server;
3738
use OCP\TaskProcessing\EShapeType;
3839
use OCP\TaskProcessing\Events\GetTaskProcessingProvidersEvent;
@@ -602,6 +603,11 @@ protected function setUp(): void {
602603
);
603604

604605
$this->userMountCache = $this->createMock(IUserMountCache::class);
606+
$this->invalidRemoteHosts = [];
607+
$this->rejectAllRemoteHosts = false;
608+
$this->remoteHostValidator = $this->createMock(IRemoteHostValidator::class);
609+
$this->remoteHostValidator->expects($this->any())->method('isValid')
610+
->willReturnCallback(fn (string $host): bool => !$this->rejectAllRemoteHosts && !in_array($host, $this->invalidRemoteHosts, true));
605611
$this->config = Server::get(IConfig::class);
606612
$this->appConfig = Server::get(IAppConfig::class);
607613
$this->manager = new Manager(
@@ -622,6 +628,7 @@ protected function setUp(): void {
622628
Server::get(IUserSession::class),
623629
Server::get(ICacheFactory::class),
624630
Server::get(IFactory::class),
631+
Server::get(IRemoteHostValidator::class),
625632
);
626633
}
627634

@@ -672,6 +679,106 @@ public function testProviderShouldBeRegisteredAndTaskFailValidation(): void {
672679
$this->manager->scheduleTask($task);
673680
}
674681

682+
public static function invalidWebhookDataProvider(): array {
683+
return [
684+
'uri without method' => ['https://example.com/hook', null],
685+
'method without uri' => [null, 'HTTP:POST'],
686+
'empty uri with method' => ['', 'HTTP:POST'],
687+
'uri with empty method' => ['https://example.com/hook', ''],
688+
'unknown method prefix' => ['https://example.com/hook', 'FTP:GET'],
689+
'unknown http verb' => ['https://example.com/hook', 'HTTP:PATCH'],
690+
'lowercase http verb' => ['https://example.com/hook', 'HTTP:post'],
691+
'unsupported uri scheme' => ['file:///etc/passwd', 'HTTP:GET'],
692+
'relative uri for http method' => ['/some/path', 'HTTP:POST'],
693+
'malformed uri' => ['https://', 'HTTP:POST'],
694+
'appapi method without exapp id' => ['/some/path', 'AppAPI:POST'],
695+
'appapi method with too many parts' => ['/some/path', 'AppAPI:my_app:POST:extra'],
696+
'appapi method with invalid exapp id' => ['/some/path', 'AppAPI:My App:POST'],
697+
'appapi method with unknown http verb' => ['/some/path', 'AppAPI:my_app:PATCH'],
698+
'absolute uri for appapi method' => ['https://example.com/hook', 'AppAPI:my_app:POST'],
699+
'uri too long' => ['https://example.com/' . str_repeat('a', 4000), 'HTTP:POST'],
700+
'method too long' => ['/some/path', 'AppAPI:' . str_repeat('a', 64) . ':POST'],
701+
];
702+
}
703+
704+
#[\PHPUnit\Framework\Attributes\DataProvider('invalidWebhookDataProvider')]
705+
public function testProviderShouldBeRegisteredAndWebhookFailValidation(?string $webhookUri, ?string $webhookMethod): void {
706+
$this->registrationContext->expects($this->any())->method('getTaskProcessingProviders')->willReturn([
707+
new ServiceRegistration('test', SuccessfulSyncProvider::class)
708+
]);
709+
$task = new Task(TextToText::ID, ['input' => 'Hello'], 'test', null);
710+
$task->setWebhookUri($webhookUri);
711+
$task->setWebhookMethod($webhookMethod);
712+
self::expectException(ValidationException::class);
713+
$this->manager->scheduleTask($task);
714+
}
715+
716+
public static function validWebhookDataProvider(): array {
717+
return [
718+
'no webhook' => [null, null],
719+
'empty webhook' => ['', ''],
720+
'http get' => ['http://example.com/hook', 'HTTP:GET'],
721+
'https post' => ['https://example.com/hook?foo=bar', 'HTTP:POST'],
722+
'https put' => ['https://example.com/hook', 'HTTP:PUT'],
723+
'https delete' => ['https://example.com/hook', 'HTTP:DELETE'],
724+
'appapi post' => ['/some/path', 'AppAPI:my_app:POST'],
725+
'appapi get' => ['/', 'AppAPI:my-app2:GET'],
726+
];
727+
}
728+
729+
#[\PHPUnit\Framework\Attributes\DataProvider('validWebhookDataProvider')]
730+
public function testProviderShouldBeRegisteredAndWebhookPassValidation(?string $webhookUri, ?string $webhookMethod): void {
731+
$this->registrationContext->expects($this->any())->method('getTaskProcessingProviders')->willReturn([
732+
new ServiceRegistration('test', SuccessfulSyncProvider::class)
733+
]);
734+
$task = new Task(TextToText::ID, ['input' => 'Hello'], 'test', null);
735+
$task->setWebhookUri($webhookUri);
736+
$task->setWebhookMethod($webhookMethod);
737+
$this->manager->scheduleTask($task);
738+
self::assertNotNull($task->getId());
739+
self::assertEquals(Task::STATUS_SCHEDULED, $task->getStatus());
740+
// clean up so the scheduled task does not interfere with other tests
741+
$this->manager->deleteTask($task);
742+
}
743+
744+
public static function localWebhookHostDataProvider(): array {
745+
return [
746+
'localhost' => ['http://localhost/hook', 'localhost'],
747+
'ipv4 loopback' => ['http://127.0.0.1:8080/hook', '127.0.0.1'],
748+
'ipv6 loopback' => ['http://[::1]/hook', '[::1]'],
749+
'private network' => ['https://192.168.1.1/hook', '192.168.1.1'],
750+
'local hostname' => ['https://server.local/hook', 'server.local'],
751+
];
752+
}
753+
754+
#[\PHPUnit\Framework\Attributes\DataProvider('localWebhookHostDataProvider')]
755+
public function testProviderShouldBeRegisteredAndLocalWebhookHostFailValidation(string $webhookUri, string $host): void {
756+
$this->registrationContext->expects($this->any())->method('getTaskProcessingProviders')->willReturn([
757+
new ServiceRegistration('test', SuccessfulSyncProvider::class)
758+
]);
759+
$this->invalidRemoteHosts = [$host];
760+
$task = new Task(TextToText::ID, ['input' => 'Hello'], 'test', null);
761+
$task->setWebhookUri($webhookUri);
762+
$task->setWebhookMethod('HTTP:POST');
763+
self::expectException(ValidationException::class);
764+
$this->manager->scheduleTask($task);
765+
}
766+
767+
public function testProviderShouldBeRegisteredAndAppApiWebhookSkipsHostValidation(): void {
768+
$this->registrationContext->expects($this->any())->method('getTaskProcessingProviders')->willReturn([
769+
new ServiceRegistration('test', SuccessfulSyncProvider::class)
770+
]);
771+
// AppAPI webhooks use an absolute path, so no remote host is involved
772+
$this->rejectAllRemoteHosts = true;
773+
$task = new Task(TextToText::ID, ['input' => 'Hello'], 'test', null);
774+
$task->setWebhookUri('/some/path');
775+
$task->setWebhookMethod('AppAPI:my_app:POST');
776+
$this->manager->scheduleTask($task);
777+
self::assertEquals(Task::STATUS_SCHEDULED, $task->getStatus());
778+
// clean up so the scheduled task does not interfere with other tests
779+
$this->manager->deleteTask($task);
780+
}
781+
675782
public function testProviderShouldBeRegisteredAndTaskWithFilesFailValidation(): void {
676783
$this->registrationContext->expects($this->any())->method('getTaskProcessingTaskTypes')->willReturn([
677784
new ServiceRegistration('test', AudioToImage::class)
@@ -1296,6 +1403,7 @@ private function createManagerInstance(): Manager {
12961403
Server::get(IUserSession::class),
12971404
Server::get(ICacheFactory::class),
12981405
Server::get(IFactory::class),
1406+
Server::get(IRemoteHostValidator::class),
12991407
);
13001408
}
13011409

0 commit comments

Comments
 (0)