fix(TaskProcessing): Harden task scheduling with webhooks - #63687
fix(TaskProcessing): Harden task scheduling with webhooks#63687marcelklehr wants to merge 1 commit into
Conversation
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
|
/backport to stable32 |
|
/backport to stable33 |
|
/backport to stable34 |
|
/backport to stable35 |
| @@ -168,6 +169,18 @@ private function handleScheduleTaskInternal( | |||
| bool $preferStreaming = false, | |||
| ): DataResponse { | |||
| $task = new Task($type, $input, $appId, $this->userId, $customId); | |||
| if ($webhookUri !== null && $webhookUri !== '' | |||
| && str_starts_with((string)$webhookMethod, 'HTTP:')) { | |||
There was a problem hiding this comment.
Should we also check when the method is lowercase? I mean, are we sure the method is systematically uppercase?
There was a problem hiding this comment.
Yes, if it's not uppercase, nothing will happen: https://github.com/nextcloud/server/blob/master/lib/private/TaskProcessing/Manager.php#L1946
joshtrichards
left a comment
joshtrichards
left a comment
There was a problem hiding this comment.
Should this validation be moved into the manager to also cover callers that schedule tasks directly? e.g., could a shared validator be invoked from prepareTask() to cover all scenarios / both scheduleTask() and runTask()?
| @@ -168,6 +169,18 @@ private function handleScheduleTaskInternal( | |||
| bool $preferStreaming = false, | |||
| ): DataResponse { | |||
| $task = new Task($type, $input, $appId, $this->userId, $customId); | |||
| if ($webhookUri !== null && $webhookUri !== '' | |||
There was a problem hiding this comment.
Maybe a dedicated validator or shared constants/helper would keep scheduling and execution behavior aligned (i.e. with Manager::runWebhook()?
| if (!is_string($host) || $host === '' | ||
| || !in_array($scheme, ['http', 'https'], true) | ||
| || !$this->remoteHostValidator->isValid($host)) { | ||
| return new DataResponse(['message' => 'Bad webhook URI'], |
There was a problem hiding this comment.
| return new DataResponse(['message' => 'Bad webhook URI'], | |
| return new DataResponse(['message' => 'Invalid webhook URI, remote host, or method.'], |
| @@ -168,6 +169,18 @@ private function handleScheduleTaskInternal( | |||
| bool $preferStreaming = false, | |||
| ): DataResponse { | |||
| $task = new Task($type, $input, $appId, $this->userId, $customId); | |||
| if ($webhookUri !== null && $webhookUri !== '' | |||
| && str_starts_with((string)$webhookMethod, 'HTTP:')) { | |||
There was a problem hiding this comment.
Doesn't this mean validation is bypassed entirely for anything that doesn't start with HTTP:?
I think the validation needs to be more explicit regarding URI and method combos:
- Neither supplied: valid non-webhook task
- Both supplied validate according to
HTTP:orAppAPI:. - Only one supplied: return error response
There was a problem hiding this comment.
Doesn't this mean validation is bypassed entirely for anything that doesn't start with HTTP:?
yes, because if the method doesn't start with HTTP, we don't take URLs at all
3 participants
Checklist
3. to review, feature component)stable32)AI (if applicable)