Skip to content

Commit e3742bb

Browse files
committed
fix(security): ReportRenderJob — refuse 'admin' fallback + reject traversal in filesFolder
Two converging persistence-shaped issues: 1. `writeToFiles()` defaulted the dashboard owner to the literal string `'admin'` when `$dashboard->getOwner()` was null, then wrote attacker-controlled bytes into `admin`'s home folder. Combined with NC's link-share endpoints, that's a re-anchor for re-compromise. 2. `$delivery['filesFolder']` is taken straight from the dashboard payload — user-controlled JSON — with no validation. The job runs as the dashboard owner (not the user who configured the schedule), so the configurer effectively borrows owner-fs permissions. Fixes: - Skip delivery (with a warning log) when owner is null/empty rather than falling back to admin. - Reject folder paths containing `..` (traversal) or that resolve to empty after stripping leading slashes. Persisting a separate `configurer` UID and validating the folder against an allowlist of paths the *configurer* explicitly owns is the right longer-term fix — left as a follow-up so this commit stays small. Refs: #1419 review (blocker 7) — discussion_r3187494444
1 parent e1ba44c commit e3742bb

1 file changed

Lines changed: 29 additions & 1 deletion

File tree

lib/BackgroundJob/ReportRenderJob.php

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,22 @@ private function renderAndDeliver(ObjectEntity $dashboard, array $payload): void
271271
*/
272272
private function writeToFiles(ObjectEntity $dashboard, array $payload, array $rendered, array $delivery): void
273273
{
274-
$owner = (string) ($dashboard->getOwner() ?? 'admin');
274+
// SECURITY: refuse to fall back to 'admin' when the dashboard
275+
// owner is null. The previous default dropped attacker-controlled
276+
// bytes (rendered HTML/PDF derived from a user-writable
277+
// ObjectEntity) into admin's Files share, where admin would see
278+
// them on next login — a phishing/redirect persistence vector.
279+
// Without an owner we have no honest "who runs this job", so
280+
// skip delivery and surface the misconfiguration in logs.
281+
$owner = $dashboard->getOwner();
282+
if ($owner === null || $owner === '') {
283+
$this->logger->warning(
284+
message: '[ReportRenderJob] Dashboard owner missing — skipping Files delivery',
285+
context: ['dashboardUuid' => $dashboard->getUuid()]
286+
);
287+
return;
288+
}
289+
275290
try {
276291
$userFolder = $this->rootFolder->getUserFolder(userId: $owner);
277292
} catch (NotFoundException $e) {
@@ -288,7 +303,20 @@ private function writeToFiles(ObjectEntity $dashboard, array $payload, array $re
288303
$folderPath = sprintf('Reports/%s', $slug);
289304
}
290305

306+
// SECURITY: $delivery['filesFolder'] comes from the dashboard
307+
// payload (user-controlled JSON). Restrict to a relative path
308+
// under the owner's home — strip leading slashes and reject any
309+
// segment that escapes the home root (`..`) or addresses an
310+
// absolute filesystem path.
291311
$folderPath = ltrim($folderPath, '/');
312+
if ($folderPath === '' || str_contains($folderPath, '..') === true) {
313+
$this->logger->warning(
314+
message: '[ReportRenderJob] Rejected delivery folder containing path traversal',
315+
context: ['folder' => $folderPath, 'owner' => $owner]
316+
);
317+
return;
318+
}
319+
292320
if ($userFolder->nodeExists(path: $folderPath) === false) {
293321
$userFolder->newFolder(path: $folderPath);
294322
}

0 commit comments

Comments
 (0)