Skip to content

Commit ff45796

Browse files
committed
fix(security): ReportsController — log internal errors, return generic messages
`render()` previously surfaced `$e->getMessage()` directly in the response — useful for diagnostics, hostile when echoed to a hostile caller. The mapper layer + ReportRenderService chain (PhpSpreadsheet / Dompdf) routinely raise exceptions that include file paths, SQL fragments, or library-internal state. Inject `LoggerInterface`, log the original exception detail at error level (with identifier + format context for correlation), and return a generic message in the response body. Keep `InvalidArgumentException` verbatim — those are validation messages the caller controls and needs to act on. Refs: #1419 review (concern 11) — discussion_r3187494514 Same pattern applies in `AggregationController`, `MetricsController`, `RealtimeController`; left for follow-up since they're separate hunks reviewed off-line.
1 parent 545d148 commit ff45796

1 file changed

Lines changed: 16 additions & 2 deletions

File tree

lib/Controller/ReportsController.php

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
use OCP\AppFramework\Http\DataDownloadResponse;
4141
use OCP\AppFramework\Http\JSONResponse;
4242
use OCP\IRequest;
43+
use Psr\Log\LoggerInterface;
4344

4445
/**
4546
* REST endpoint for on-demand dashboard rendering.
@@ -59,6 +60,7 @@ public function __construct(
5960
IRequest $request,
6061
private readonly ReportRenderService $renderService,
6162
private readonly MagicMapper $objectMapper,
63+
private readonly LoggerInterface $logger,
6264
) {
6365
parent::__construct(appName: $appName, request: $request);
6466

@@ -87,8 +89,14 @@ public function render(string $id)
8789
statusCode: Http::STATUS_NOT_FOUND
8890
);
8991
} catch (\Throwable $e) {
92+
// SECURITY: log internal exception detail (file paths, SQL
93+
// fragments, library state) and return a generic message.
94+
$this->logger->error(
95+
message: '[ReportsController] Failed to load dashboard',
96+
context: ['identifier' => $id, 'error' => $e->getMessage()]
97+
);
9098
return new JSONResponse(
91-
data: ['error' => 'Failed to load dashboard: '.$e->getMessage()],
99+
data: ['error' => 'Failed to load dashboard'],
92100
statusCode: Http::STATUS_INTERNAL_SERVER_ERROR
93101
);
94102
}
@@ -99,13 +107,19 @@ public function render(string $id)
99107
format: $format
100108
);
101109
} catch (InvalidArgumentException $e) {
110+
// Validation errors are safe to return verbatim (the caller
111+
// controls the input that triggered them).
102112
return new JSONResponse(
103113
data: ['error' => $e->getMessage()],
104114
statusCode: Http::STATUS_UNPROCESSABLE_ENTITY
105115
);
106116
} catch (\Throwable $e) {
117+
$this->logger->error(
118+
message: '[ReportsController] Render failed',
119+
context: ['identifier' => $id, 'format' => $format, 'error' => $e->getMessage()]
120+
);
107121
return new JSONResponse(
108-
data: ['error' => 'Render failed: '.$e->getMessage()],
122+
data: ['error' => 'Render failed; see server logs for details'],
109123
statusCode: Http::STATUS_INTERNAL_SERVER_ERROR
110124
);
111125
}

0 commit comments

Comments
 (0)