Skip to content

Commit 40aef4e

Browse files
marcelklehrmarcelklehr
committed
fix(TaskProcessing): Harden task scheduling with webhooks
Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Marcel Klehr <mklehr@gmx.net>
1 parent 1c88b39 commit 40aef4e

2 files changed

Lines changed: 192 additions & 1 deletion

File tree

‎lib/private/TaskProcessing/Manager.php‎

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
use OCP\IUserSession;
4444
use OCP\L10N\IFactory;
4545
use OCP\Lock\LockedException;
46+
use OCP\Security\IRemoteHostValidator;
4647
use OCP\Server;
4748
use OCP\SpeechToText\ISpeechToTextProvider;
4849
use OCP\SpeechToText\ISpeechToTextProviderWithId;
@@ -163,6 +164,7 @@ public function __construct(
163164
ICacheFactory $cacheFactory,
164165
private IFactory $l10nFactory,
165166
private ITimeFactory $timeFactory,
167+
private IRemoteHostValidator $remoteHostValidator,
166168
) {
167169
$this->appData = $appDataFactory->get('core');
168170
$this->distributedCache = $cacheFactory->createDistributed('task_processing::');
@@ -1669,7 +1671,7 @@ public function setTaskStatus(Task $task, int $status): void {
16691671
}
16701672

16711673
/**
1672-
* Validate input, fill input default values, set completionExpectedAt, set scheduledAt
1674+
* Validate input and webhook, fill input default values, set completionExpectedAt, set scheduledAt
16731675
*
16741676
* @param Task $task
16751677
* @return void
@@ -1706,6 +1708,8 @@ private function prepareTask(Task $task): void {
17061708
$this->validateFileId($fileId);
17071709
$this->validateUserAccessToFile($fileId, $task->getUserId());
17081710
}
1711+
// validate the webhook configuration
1712+
$this->validateWebhook($task);
17091713
// remove superfluous keys and set input
17101714
$input = $this->removeSuperfluousArrayKeys($task->getInput(), $inputShape, $optionalInputShape);
17111715
$inputWithDefaults = $this->fillInputDefaults($input, $inputShapeDefaults, $optionalInputShapeDefaults);
@@ -1718,6 +1722,79 @@ private function prepareTask(Task $task): void {
17181722
$task->setCompletionExpectedAt($completionExpectedAt);
17191723
}
17201724

1725+
/**
1726+
* Validate the webhook URI and webhook method of a task
1727+
*
1728+
* Both values are optional, but if one is set, the other one has to be set as well.
1729+
* Supported methods are `HTTP:<GET|POST|PUT|DELETE>`, which require an absolute
1730+
* http(s) URI pointing at a non-local host, and `AppAPI:<exAppId>:<GET|POST|PUT|DELETE>`,
1731+
* which requires an absolute path as URI.
1732+
*
1733+
* @param Task $task
1734+
* @return void
1735+
* @throws ValidationException
1736+
*/
1737+
private function validateWebhook(Task $task): void {
1738+
$uri = $task->getWebhookUri();
1739+
$method = $task->getWebhookMethod();
1740+
1741+
if (($uri === null || $uri === '') && ($method === null || $method === '')) {
1742+
return;
1743+
}
1744+
if ($uri === null || $uri === '') {
1745+
throw new ValidationException('Webhook URI is required when a webhook method is set');
1746+
}
1747+
if ($method === null || $method === '') {
1748+
throw new ValidationException('Webhook method is required when a webhook URI is set');
1749+
}
1750+
if (mb_strlen($uri) > 4000) {
1751+
throw new ValidationException('Webhook URI is too long, maximum length is 4000 characters');
1752+
}
1753+
if (mb_strlen($method) > 64) {
1754+
throw new ValidationException('Webhook method is too long, maximum length is 64 characters');
1755+
}
1756+
1757+
if (str_starts_with($method, 'HTTP:')) {
1758+
if (!in_array($method, ['HTTP:GET', 'HTTP:POST', 'HTTP:PUT', 'HTTP:DELETE'], true)) {
1759+
throw new ValidationException('Invalid webhook method: ' . $method);
1760+
}
1761+
if (filter_var($uri, FILTER_VALIDATE_URL) === false) {
1762+
throw new ValidationException('Invalid webhook URI: ' . $uri);
1763+
}
1764+
$parsedUri = parse_url($uri);
1765+
if ($parsedUri === false || !isset($parsedUri['scheme']) || !isset($parsedUri['host']) || $parsedUri['host'] === '') {
1766+
throw new ValidationException('Invalid webhook URI: ' . $uri);
1767+
}
1768+
if (!in_array(strtolower($parsedUri['scheme']), ['http', 'https'], true)) {
1769+
throw new ValidationException('Invalid webhook URI scheme, only http and https are supported: ' . $uri);
1770+
}
1771+
if (!$this->remoteHostValidator->isValid($parsedUri['host'])) {
1772+
throw new ValidationException('Invalid webhook URI, the host is not allowed to be connected to: ' . $uri);
1773+
}
1774+
return;
1775+
}
1776+
1777+
if (str_starts_with($method, 'AppAPI:')) {
1778+
$parsedMethod = explode(':', $method);
1779+
if (count($parsedMethod) !== 3) {
1780+
throw new ValidationException('Invalid webhook method: ' . $method);
1781+
}
1782+
[, $exAppId, $httpMethod] = $parsedMethod;
1783+
if (preg_match('/^[a-z][a-z0-9_-]*$/', $exAppId) !== 1) {
1784+
throw new ValidationException('Invalid ExApp ID in webhook method: ' . $method);
1785+
}
1786+
if (!in_array($httpMethod, ['GET', 'POST', 'PUT', 'DELETE'], true)) {
1787+
throw new ValidationException('Invalid webhook method: ' . $method);
1788+
}
1789+
if (!str_starts_with($uri, '/')) {
1790+
throw new ValidationException('Invalid webhook URI, an absolute path is required for AppAPI webhooks: ' . $uri);
1791+
}
1792+
return;
1793+
}
1794+
1795+
throw new ValidationException('Invalid webhook method: ' . $method);
1796+
}
1797+
17211798
/**
17221799
* Store the task in the DB and set its ID in the \OCP\TaskProcessing\Task input param
17231800
*

‎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;
@@ -767,6 +768,12 @@ class TaskProcessingTest extends \Test\TestCase {
767768
private IJobList&MockObject $jobList;
768769
private IUserMountCache&MockObject $userMountCache;
769770
private RegistrationContext&MockObject $registrationContext;
771+
private IRemoteHostValidator&MockObject $remoteHostValidator;
772+
773+
/** @var list<string> hosts the mocked IRemoteHostValidator rejects */
774+
private array $invalidRemoteHosts = [];
775+
/** Makes the mocked IRemoteHostValidator reject every host */
776+
private bool $rejectAllRemoteHosts = false;
770777

771778
/** @var array<class-string, IProvider> */
772779
private array $providers;
@@ -835,6 +842,11 @@ protected function setUp(): void {
835842
);
836843

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

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

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

0 commit comments

Comments
 (0)