Skip to content

Commit d5129a3

Browse files
marcelklehrmarcelklehr
authored
Merge pull request #63873 from nextcloud/backport/63687/stable34
[stable34] fix(TaskProcessing): Harden task scheduling with webhooks
2 parents ff8285d + 45ab37b commit d5129a3

3 files changed

Lines changed: 194 additions & 3 deletions

File tree

‎lib/private/TaskProcessing/Manager.php‎

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
use OCP\IUserSession;
4343
use OCP\L10N\IFactory;
4444
use OCP\Lock\LockedException;
45+
use OCP\Security\IRemoteHostValidator;
4546
use OCP\Server;
4647
use OCP\SpeechToText\ISpeechToTextProvider;
4748
use OCP\SpeechToText\ISpeechToTextProviderWithId;
@@ -156,6 +157,7 @@ public function __construct(
156157
private IUserSession $userSession,
157158
ICacheFactory $cacheFactory,
158159
private IFactory $l10nFactory,
160+
private IRemoteHostValidator $remoteHostValidator,
159161
) {
160162
$this->appData = $appDataFactory->get('core');
161163
$this->distributedCache = $cacheFactory->createDistributed('task_processing::');
@@ -1553,7 +1555,7 @@ public function setTaskStatus(Task $task, int $status): void {
15531555
}
15541556

15551557
/**
1556-
* Validate input, fill input default values, set completionExpectedAt, set scheduledAt
1558+
* Validate input and webhook, fill input default values, set completionExpectedAt, set scheduledAt
15571559
*
15581560
* @param Task $task
15591561
* @return void
@@ -1590,6 +1592,8 @@ private function prepareTask(Task $task): void {
15901592
$this->validateFileId($fileId);
15911593
$this->validateUserAccessToFile($fileId, $task->getUserId());
15921594
}
1595+
// validate the webhook configuration
1596+
$this->validateWebhook($task);
15931597
// remove superfluous keys and set input
15941598
$input = $this->removeSuperfluousArrayKeys($task->getInput(), $inputShape, $optionalInputShape);
15951599
$inputWithDefaults = $this->fillInputDefaults($input, $inputShapeDefaults, $optionalInputShapeDefaults);
@@ -1602,6 +1606,79 @@ private function prepareTask(Task $task): void {
16021606
$task->setCompletionExpectedAt($completionExpectedAt);
16031607
}
16041608

1609+
/**
1610+
* Validate the webhook URI and webhook method of a task
1611+
*
1612+
* Both values are optional, but if one is set, the other one has to be set as well.
1613+
* Supported methods are `HTTP:<GET|POST|PUT|DELETE>`, which require an absolute
1614+
* http(s) URI pointing at a non-local host, and `AppAPI:<exAppId>:<GET|POST|PUT|DELETE>`,
1615+
* which requires an absolute path as URI.
1616+
*
1617+
* @param Task $task
1618+
* @return void
1619+
* @throws ValidationException
1620+
*/
1621+
private function validateWebhook(Task $task): void {
1622+
$uri = $task->getWebhookUri();
1623+
$method = $task->getWebhookMethod();
1624+
1625+
if (($uri === null || $uri === '') && ($method === null || $method === '')) {
1626+
return;
1627+
}
1628+
if ($uri === null || $uri === '') {
1629+
throw new ValidationException('Webhook URI is required when a webhook method is set');
1630+
}
1631+
if ($method === null || $method === '') {
1632+
throw new ValidationException('Webhook method is required when a webhook URI is set');
1633+
}
1634+
if (mb_strlen($uri) > 4000) {
1635+
throw new ValidationException('Webhook URI is too long, maximum length is 4000 characters');
1636+
}
1637+
if (mb_strlen($method) > 64) {
1638+
throw new ValidationException('Webhook method is too long, maximum length is 64 characters');
1639+
}
1640+
1641+
if (str_starts_with($method, 'HTTP:')) {
1642+
if (!in_array($method, ['HTTP:GET', 'HTTP:POST', 'HTTP:PUT', 'HTTP:DELETE'], true)) {
1643+
throw new ValidationException('Invalid webhook method: ' . $method);
1644+
}
1645+
if (filter_var($uri, FILTER_VALIDATE_URL) === false) {
1646+
throw new ValidationException('Invalid webhook URI: ' . $uri);
1647+
}
1648+
$parsedUri = parse_url($uri);
1649+
if ($parsedUri === false || !isset($parsedUri['scheme']) || !isset($parsedUri['host']) || $parsedUri['host'] === '') {
1650+
throw new ValidationException('Invalid webhook URI: ' . $uri);
1651+
}
1652+
if (!in_array(strtolower($parsedUri['scheme']), ['http', 'https'], true)) {
1653+
throw new ValidationException('Invalid webhook URI scheme, only http and https are supported: ' . $uri);
1654+
}
1655+
if (!$this->remoteHostValidator->isValid($parsedUri['host'])) {
1656+
throw new ValidationException('Invalid webhook URI, the host is not allowed to be connected to: ' . $uri);
1657+
}
1658+
return;
1659+
}
1660+
1661+
if (str_starts_with($method, 'AppAPI:')) {
1662+
$parsedMethod = explode(':', $method);
1663+
if (count($parsedMethod) !== 3) {
1664+
throw new ValidationException('Invalid webhook method: ' . $method);
1665+
}
1666+
[, $exAppId, $httpMethod] = $parsedMethod;
1667+
if (preg_match('/^[a-z][a-z0-9_-]*$/', $exAppId) !== 1) {
1668+
throw new ValidationException('Invalid ExApp ID in webhook method: ' . $method);
1669+
}
1670+
if (!in_array($httpMethod, ['GET', 'POST', 'PUT', 'DELETE'], true)) {
1671+
throw new ValidationException('Invalid webhook method: ' . $method);
1672+
}
1673+
if (!str_starts_with($uri, '/')) {
1674+
throw new ValidationException('Invalid webhook URI, an absolute path is required for AppAPI webhooks: ' . $uri);
1675+
}
1676+
return;
1677+
}
1678+
1679+
throw new ValidationException('Invalid webhook method: ' . $method);
1680+
}
1681+
16051682
/**
16061683
* Store the task in the DB and set its ID in the \OCP\TaskProcessing\Task input param
16071684
*

‎lib/public/TaskProcessing/IManager.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ public function getAvailableTaskTypeIds(bool $showDisabled = false, ?string $use
7070
/**
7171
* @param Task $task The task to run
7272
* @throws PreConditionNotMetException If no or not the requested provider was registered but this method was still called
73-
* @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
73+
* @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
7474
* @throws Exception storing the task in the database failed
7575
* @throws UnauthorizedException the user scheduling the task does not have access to the files used in the input
7676
* @since 30.0.0
@@ -83,7 +83,7 @@ public function scheduleTask(Task $task): void;
8383
* @param Task $task The task to run
8484
* @return Task The result task
8585
* @throws PreConditionNotMetException If no or not the requested provider was registered but this method was still called
86-
* @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
86+
* @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
8787
* @throws Exception storing the task in the database failed
8888
* @throws UnauthorizedException the user scheduling the task does not have access to the files used in the input
8989
* @since 30.0.0

‎tests/lib/TaskProcessing/TaskProcessingTest.php‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
use OCP\IUserManager;
3232
use OCP\IUserSession;
3333
use OCP\L10N\IFactory;
34+
use OCP\Security\IRemoteHostValidator;
3435
use OCP\Server;
3536
use OCP\TaskProcessing\EShapeType;
3637
use OCP\TaskProcessing\Events\GetTaskProcessingProvidersEvent;
@@ -770,6 +771,12 @@ class TaskProcessingTest extends \Test\TestCase {
770771
private IJobList&MockObject $jobList;
771772
private IUserMountCache&MockObject $userMountCache;
772773
private RegistrationContext&MockObject $registrationContext;
774+
private IRemoteHostValidator&MockObject $remoteHostValidator;
775+
776+
/** @var list<string> hosts the mocked IRemoteHostValidator rejects */
777+
private array $invalidRemoteHosts = [];
778+
/** Makes the mocked IRemoteHostValidator reject every host */
779+
private bool $rejectAllRemoteHosts = false;
773780

774781
/** @var array<class-string, IProvider> */
775782
private array $providers;
@@ -838,6 +845,11 @@ protected function setUp(): void {
838845
);
839846

840847
$this->userMountCache = $this->createMock(IUserMountCache::class);
848+
$this->invalidRemoteHosts = [];
849+
$this->rejectAllRemoteHosts = false;
850+
$this->remoteHostValidator = $this->createMock(IRemoteHostValidator::class);
851+
$this->remoteHostValidator->expects($this->any())->method('isValid')
852+
->willReturnCallback(fn (string $host): bool => !$this->rejectAllRemoteHosts && !in_array($host, $this->invalidRemoteHosts, true));
841853
$this->config = Server::get(IConfig::class);
842854
$this->appConfig = Server::get(IAppConfig::class);
843855
$this->manager = new Manager(
@@ -858,6 +870,7 @@ protected function setUp(): void {
858870
Server::get(IUserSession::class),
859871
Server::get(ICacheFactory::class),
860872
Server::get(IFactory::class),
873+
Server::get(IRemoteHostValidator::class),
861874
);
862875
}
863876

@@ -908,6 +921,106 @@ public function testProviderShouldBeRegisteredAndTaskFailValidation(): void {
908921
$this->manager->scheduleTask($task);
909922
}
910923

924+
public static function invalidWebhookDataProvider(): array {
925+
return [
926+
'uri without method' => ['https://example.com/hook', null],
927+
'method without uri' => [null, 'HTTP:POST'],
928+
'empty uri with method' => ['', 'HTTP:POST'],
929+
'uri with empty method' => ['https://example.com/hook', ''],
930+
'unknown method prefix' => ['https://example.com/hook', 'FTP:GET'],
931+
'unknown http verb' => ['https://example.com/hook', 'HTTP:PATCH'],
932+
'lowercase http verb' => ['https://example.com/hook', 'HTTP:post'],
933+
'unsupported uri scheme' => ['file:///etc/passwd', 'HTTP:GET'],
934+
'relative uri for http method' => ['/some/path', 'HTTP:POST'],
935+
'malformed uri' => ['https://', 'HTTP:POST'],
936+
'appapi method without exapp id' => ['/some/path', 'AppAPI:POST'],
937+
'appapi method with too many parts' => ['/some/path', 'AppAPI:my_app:POST:extra'],
938+
'appapi method with invalid exapp id' => ['/some/path', 'AppAPI:My App:POST'],
939+
'appapi method with unknown http verb' => ['/some/path', 'AppAPI:my_app:PATCH'],
940+
'absolute uri for appapi method' => ['https://example.com/hook', 'AppAPI:my_app:POST'],
941+
'uri too long' => ['https://example.com/' . str_repeat('a', 4000), 'HTTP:POST'],
942+
'method too long' => ['/some/path', 'AppAPI:' . str_repeat('a', 64) . ':POST'],
943+
];
944+
}
945+
946+
#[\PHPUnit\Framework\Attributes\DataProvider('invalidWebhookDataProvider')]
947+
public function testProviderShouldBeRegisteredAndWebhookFailValidation(?string $webhookUri, ?string $webhookMethod): void {
948+
$this->registrationContext->expects($this->any())->method('getTaskProcessingProviders')->willReturn([
949+
new ServiceRegistration('test', SuccessfulSyncProvider::class)
950+
]);
951+
$task = new Task(TextToText::ID, ['input' => 'Hello'], 'test', null);
952+
$task->setWebhookUri($webhookUri);
953+
$task->setWebhookMethod($webhookMethod);
954+
self::expectException(ValidationException::class);
955+
$this->manager->scheduleTask($task);
956+
}
957+
958+
public static function validWebhookDataProvider(): array {
959+
return [
960+
'no webhook' => [null, null],
961+
'empty webhook' => ['', ''],
962+
'http get' => ['http://example.com/hook', 'HTTP:GET'],
963+
'https post' => ['https://example.com/hook?foo=bar', 'HTTP:POST'],
964+
'https put' => ['https://example.com/hook', 'HTTP:PUT'],
965+
'https delete' => ['https://example.com/hook', 'HTTP:DELETE'],
966+
'appapi post' => ['/some/path', 'AppAPI:my_app:POST'],
967+
'appapi get' => ['/', 'AppAPI:my-app2:GET'],
968+
];
969+
}
970+
971+
#[\PHPUnit\Framework\Attributes\DataProvider('validWebhookDataProvider')]
972+
public function testProviderShouldBeRegisteredAndWebhookPassValidation(?string $webhookUri, ?string $webhookMethod): void {
973+
$this->registrationContext->expects($this->any())->method('getTaskProcessingProviders')->willReturn([
974+
new ServiceRegistration('test', SuccessfulSyncProvider::class)
975+
]);
976+
$task = new Task(TextToText::ID, ['input' => 'Hello'], 'test', null);
977+
$task->setWebhookUri($webhookUri);
978+
$task->setWebhookMethod($webhookMethod);
979+
$this->manager->scheduleTask($task);
980+
self::assertNotNull($task->getId());
981+
self::assertEquals(Task::STATUS_SCHEDULED, $task->getStatus());
982+
// clean up so the scheduled task does not interfere with other tests
983+
$this->manager->deleteTask($task);
984+
}
985+
986+
public static function localWebhookHostDataProvider(): array {
987+
return [
988+
'localhost' => ['http://localhost/hook', 'localhost'],
989+
'ipv4 loopback' => ['http://127.0.0.1:8080/hook', '127.0.0.1'],
990+
'ipv6 loopback' => ['http://[::1]/hook', '[::1]'],
991+
'private network' => ['https://192.168.1.1/hook', '192.168.1.1'],
992+
'local hostname' => ['https://server.local/hook', 'server.local'],
993+
];
994+
}
995+
996+
#[\PHPUnit\Framework\Attributes\DataProvider('localWebhookHostDataProvider')]
997+
public function testProviderShouldBeRegisteredAndLocalWebhookHostFailValidation(string $webhookUri, string $host): void {
998+
$this->registrationContext->expects($this->any())->method('getTaskProcessingProviders')->willReturn([
999+
new ServiceRegistration('test', SuccessfulSyncProvider::class)
1000+
]);
1001+
$this->invalidRemoteHosts = [$host];
1002+
$task = new Task(TextToText::ID, ['input' => 'Hello'], 'test', null);
1003+
$task->setWebhookUri($webhookUri);
1004+
$task->setWebhookMethod('HTTP:POST');
1005+
self::expectException(ValidationException::class);
1006+
$this->manager->scheduleTask($task);
1007+
}
1008+
1009+
public function testProviderShouldBeRegisteredAndAppApiWebhookSkipsHostValidation(): void {
1010+
$this->registrationContext->expects($this->any())->method('getTaskProcessingProviders')->willReturn([
1011+
new ServiceRegistration('test', SuccessfulSyncProvider::class)
1012+
]);
1013+
// AppAPI webhooks use an absolute path, so no remote host is involved
1014+
$this->rejectAllRemoteHosts = true;
1015+
$task = new Task(TextToText::ID, ['input' => 'Hello'], 'test', null);
1016+
$task->setWebhookUri('/some/path');
1017+
$task->setWebhookMethod('AppAPI:my_app:POST');
1018+
$this->manager->scheduleTask($task);
1019+
self::assertEquals(Task::STATUS_SCHEDULED, $task->getStatus());
1020+
// clean up so the scheduled task does not interfere with other tests
1021+
$this->manager->deleteTask($task);
1022+
}
1023+
9111024
public function testProviderShouldBeRegisteredAndTaskWithFilesFailValidation(): void {
9121025
$this->registrationContext->expects($this->any())->method('getTaskProcessingTaskTypes')->willReturn([
9131026
new ServiceRegistration('test', AudioToImage::class)
@@ -1600,6 +1713,7 @@ private function createManagerInstance(): Manager {
16001713
Server::get(IUserSession::class),
16011714
Server::get(ICacheFactory::class),
16021715
Server::get(IFactory::class),
1716+
Server::get(IRemoteHostValidator::class),
16031717
);
16041718
}
16051719

0 commit comments

Comments
 (0)