Skip to content

Commit aed4fd0

Browse files
marcelklehrmarcelklehr
authored
Merge pull request #63687 from nextcloud/fix/harden-task-processing-webhook
fix(TaskProcessing): Harden task scheduling with webhooks
2 parents 3794d9d + 0667068 commit aed4fd0

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
@@ -44,6 +44,7 @@
4444
use OCP\IUserSession;
4545
use OCP\L10N\IFactory;
4646
use OCP\Lock\LockedException;
47+
use OCP\Security\IRemoteHostValidator;
4748
use OCP\Server;
4849
use OCP\SpeechToText\ISpeechToTextProvider;
4950
use OCP\SpeechToText\ISpeechToTextProviderWithId;
@@ -167,6 +168,7 @@ public function __construct(
167168
ICacheFactory $cacheFactory,
168169
private IFactory $l10nFactory,
169170
private ITimeFactory $timeFactory,
171+
private IRemoteHostValidator $remoteHostValidator,
170172
) {
171173
$this->appData = $appDataFactory->get('core');
172174
$this->distributedCache = $cacheFactory->createDistributed('task_processing::');
@@ -1712,7 +1714,7 @@ public function setTaskStatus(Task $task, int $status): void {
17121714
}
17131715

17141716
/**
1715-
* Validate input, fill input default values, set completionExpectedAt, set scheduledAt
1717+
* Validate input and webhook, fill input default values, set completionExpectedAt, set scheduledAt
17161718
*
17171719
* @param Task $task
17181720
* @return void
@@ -1749,6 +1751,8 @@ private function prepareTask(Task $task): void {
17491751
$this->validateFileId($fileId);
17501752
$this->validateUserAccessToFile($fileId, $task->getUserId());
17511753
}
1754+
// validate the webhook configuration
1755+
$this->validateWebhook($task);
17521756
// remove superfluous keys and set input
17531757
$input = $this->removeSuperfluousArrayKeys($task->getInput(), $inputShape, $optionalInputShape);
17541758
$inputWithDefaults = $this->fillInputDefaults($input, $inputShapeDefaults, $optionalInputShapeDefaults);
@@ -1761,6 +1765,79 @@ private function prepareTask(Task $task): void {
17611765
$task->setCompletionExpectedAt($completionExpectedAt);
17621766
}
17631767

1768+
/**
1769+
* Validate the webhook URI and webhook method of a task
1770+
*
1771+
* Both values are optional, but if one is set, the other one has to be set as well.
1772+
* Supported methods are `HTTP:<GET|POST|PUT|DELETE>`, which require an absolute
1773+
* http(s) URI pointing at a non-local host, and `AppAPI:<exAppId>:<GET|POST|PUT|DELETE>`,
1774+
* which requires an absolute path as URI.
1775+
*
1776+
* @param Task $task
1777+
* @return void
1778+
* @throws ValidationException
1779+
*/
1780+
private function validateWebhook(Task $task): void {
1781+
$uri = $task->getWebhookUri();
1782+
$method = $task->getWebhookMethod();
1783+
1784+
if (($uri === null || $uri === '') && ($method === null || $method === '')) {
1785+
return;
1786+
}
1787+
if ($uri === null || $uri === '') {
1788+
throw new ValidationException('Webhook URI is required when a webhook method is set');
1789+
}
1790+
if ($method === null || $method === '') {
1791+
throw new ValidationException('Webhook method is required when a webhook URI is set');
1792+
}
1793+
if (mb_strlen($uri) > 4000) {
1794+
throw new ValidationException('Webhook URI is too long, maximum length is 4000 characters');
1795+
}
1796+
if (mb_strlen($method) > 64) {
1797+
throw new ValidationException('Webhook method is too long, maximum length is 64 characters');
1798+
}
1799+
1800+
if (str_starts_with($method, 'HTTP:')) {
1801+
if (!in_array($method, ['HTTP:GET', 'HTTP:POST', 'HTTP:PUT', 'HTTP:DELETE'], true)) {
1802+
throw new ValidationException('Invalid webhook method: ' . $method);
1803+
}
1804+
if (filter_var($uri, FILTER_VALIDATE_URL) === false) {
1805+
throw new ValidationException('Invalid webhook URI: ' . $uri);
1806+
}
1807+
$parsedUri = parse_url($uri);
1808+
if ($parsedUri === false || !isset($parsedUri['scheme']) || !isset($parsedUri['host']) || $parsedUri['host'] === '') {
1809+
throw new ValidationException('Invalid webhook URI: ' . $uri);
1810+
}
1811+
if (!in_array(strtolower($parsedUri['scheme']), ['http', 'https'], true)) {
1812+
throw new ValidationException('Invalid webhook URI scheme, only http and https are supported: ' . $uri);
1813+
}
1814+
if (!$this->remoteHostValidator->isValid($parsedUri['host'])) {
1815+
throw new ValidationException('Invalid webhook URI, the host is not allowed to be connected to: ' . $uri);
1816+
}
1817+
return;
1818+
}
1819+
1820+
if (str_starts_with($method, 'AppAPI:')) {
1821+
$parsedMethod = explode(':', $method);
1822+
if (count($parsedMethod) !== 3) {
1823+
throw new ValidationException('Invalid webhook method: ' . $method);
1824+
}
1825+
[, $exAppId, $httpMethod] = $parsedMethod;
1826+
if (preg_match('/^[a-z][a-z0-9_-]*$/', $exAppId) !== 1) {
1827+
throw new ValidationException('Invalid ExApp ID in webhook method: ' . $method);
1828+
}
1829+
if (!in_array($httpMethod, ['GET', 'POST', 'PUT', 'DELETE'], true)) {
1830+
throw new ValidationException('Invalid webhook method: ' . $method);
1831+
}
1832+
if (!str_starts_with($uri, '/')) {
1833+
throw new ValidationException('Invalid webhook URI, an absolute path is required for AppAPI webhooks: ' . $uri);
1834+
}
1835+
return;
1836+
}
1837+
1838+
throw new ValidationException('Invalid webhook method: ' . $method);
1839+
}
1840+
17641841
/**
17651842
* Store the task in the DB and set its ID in the \OCP\TaskProcessing\Task input param
17661843
*

‎lib/public/TaskProcessing/IManager.php‎

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

‎tests/lib/TaskProcessing/TaskProcessingTest.php‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
use OCP\IUserManager;
3333
use OCP\IUserSession;
3434
use OCP\L10N\IFactory;
35+
use OCP\Security\IRemoteHostValidator;
3536
use OCP\Server;
3637
use OCP\TaskProcessing\EShapeType;
3738
use OCP\TaskProcessing\Events\GetTaskProcessingProvidersEvent;
@@ -768,6 +769,12 @@ class TaskProcessingTest extends \Test\TestCase {
768769
private IJobList&MockObject $jobList;
769770
private IUserMountCache&MockObject $userMountCache;
770771
private RegistrationContext&MockObject $registrationContext;
772+
private IRemoteHostValidator&MockObject $remoteHostValidator;
773+
774+
/** @var list<string> hosts the mocked IRemoteHostValidator rejects */
775+
private array $invalidRemoteHosts = [];
776+
/** Makes the mocked IRemoteHostValidator reject every host */
777+
private bool $rejectAllRemoteHosts = false;
771778

772779
/** @var array<class-string, IProvider> */
773780
private array $providers;
@@ -836,6 +843,11 @@ protected function setUp(): void {
836843
);
837844

838845
$this->userMountCache = $this->createMock(IUserMountCache::class);
846+
$this->invalidRemoteHosts = [];
847+
$this->rejectAllRemoteHosts = false;
848+
$this->remoteHostValidator = $this->createMock(IRemoteHostValidator::class);
849+
$this->remoteHostValidator->expects($this->any())->method('isValid')
850+
->willReturnCallback(fn (string $host): bool => !$this->rejectAllRemoteHosts && !in_array($host, $this->invalidRemoteHosts, true));
839851
$this->config = Server::get(IConfig::class);
840852
$this->appConfig = Server::get(IAppConfig::class);
841853
$this->manager = new Manager(
@@ -857,6 +869,7 @@ protected function setUp(): void {
857869
Server::get(ICacheFactory::class),
858870
Server::get(IFactory::class),
859871
Server::get(ITimeFactory::class),
872+
$this->remoteHostValidator,
860873
);
861874
}
862875

@@ -906,6 +919,106 @@ public function testProviderShouldBeRegisteredAndTaskFailValidation(): void {
906919
$this->manager->scheduleTask($task);
907920
}
908921

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

0 commit comments

Comments
 (0)