fix(TaskProcessing): Harden task scheduling with webhooks - #63687
Conversation
|
/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()?
3331ebc to
40aef4e
Compare
|
Reimplemented this with a a separate validation method that is called from prepareTask |
There was a problem hiding this comment.
🟡 Changes recommended
The public manager contract does not document the newly introduced webhook validation failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens task scheduling by validating webhook methods, URIs, lengths, and remote hosts.
Changes:
- Adds HTTP and AppAPI webhook validation.
- Adds unit coverage for valid, invalid, and local-host webhooks.
File summaries
| File | Description |
|---|---|
lib/private/TaskProcessing/Manager.php |
Validates webhook configuration before scheduling. |
tests/lib/TaskProcessing/TaskProcessingTest.php |
Tests webhook validation behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Marcel Klehr <mklehr@gmx.net>
40aef4e to
0667068
Compare
Checklist
3. to review, feature component)stable32)AI (if applicable)