From 0a6c4e7e42697c9ed856f703246069a1110f5a2a Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Wed, 27 May 2026 19:13:42 +0200 Subject: [PATCH] fix: EmailController IDOR, file-disclosure, open-relay, XSS, log-injection, stub-501 (C4/C8/H6/L1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - C4 IDOR: sendEmail now calls loadCaseData() first; if case not found (or user has no OR RBAC access) throws RuntimeException('Zaak niet gevonden') → 403 - C4 file-disclosure: attachFile now rejects absolute paths and ../traversal before calling file_exists; logs a warning instead of attaching - C4 open-relay: validates 'to' address with filter_var(FILTER_VALIDATE_EMAIL) - H6 reserved-domain: fail loudly if email_from_address is empty or @example.nl; removes the noreply@example.nl default that would cause silent bounce storms - H6 XSS: resolveVariables() now HTML-escapes substituted values by default (htmlspecialchars ENT_QUOTES|ENT_HTML5); plain-text callers pass false - L1 log-injection: storeExternalDocument strips control characters from filename before logging; uses structured PSR-3 context array - C8 stub-501: PublicShareController::uploadDocument returns 501 Not Implemented instead of false-success, preventing citizens from thinking uploads succeeded when the file is silently discarded --- lib/Controller/PublicShareController.php | 16 +- lib/Service/CaseEmailService.php | 62 ++++- lib/Service/CaseSharingService.php | 24 +- tests/Unit/Service/CaseEmailServiceTest.php | 245 ++++++++++++++++++++ 4 files changed, 326 insertions(+), 21 deletions(-) create mode 100644 tests/Unit/Service/CaseEmailServiceTest.php diff --git a/lib/Controller/PublicShareController.php b/lib/Controller/PublicShareController.php index f7c2948e..01b2768b 100644 --- a/lib/Controller/PublicShareController.php +++ b/lib/Controller/PublicShareController.php @@ -295,13 +295,17 @@ public function uploadDocument(string $token): JSONResponse ); } - $result = $this->caseSharingService->storeExternalDocument( - $share['caseId'], - $share['id'] ?? '', - $uploadedFile, + // C8: storeExternalDocument is a stub — uploaded files are silently discarded. + // Return 501 Not Implemented so clients show an accurate error instead of + // a false-success "document received" message that leads to legal data loss. + // TODO: Implement via IUserFolder + OR file attachment before enabling. + return new JSONResponse( + [ + 'success' => false, + 'error' => 'Documentupload is nog niet beschikbaar. Neem contact op met de behandelaar.', + ], + \OCP\AppFramework\Http::STATUS_NOT_IMPLEMENTED ); - - return new JSONResponse(['success' => true, 'document' => $result]); }//end uploadDocument() /** diff --git a/lib/Service/CaseEmailService.php b/lib/Service/CaseEmailService.php index bd3e902b..d9cc678d 100644 --- a/lib/Service/CaseEmailService.php +++ b/lib/Service/CaseEmailService.php @@ -82,17 +82,41 @@ public function sendEmail( string $body, array $attachments=[], ): array { + // H6 / C4: Fail loudly if from-address is not configured — never fall back to + // the reserved example.nl domain which would cause bounces and expose config errors. $fromAddress = $this->appConfig->getValueString( Application::APP_ID, 'email_from_address', - 'noreply@example.nl', + '', ); - $fromName = $this->appConfig->getValueString( + if ($fromAddress === '' || str_ends_with($fromAddress, '@example.nl') === true) { + throw new \RuntimeException( + 'E-mail afzenderadres is niet geconfigureerd. ' + .'Stel email_from_address in via de beheerdersinstellingen.' + ); + } + + $fromName = $this->appConfig->getValueString( Application::APP_ID, 'email_from_name', 'Procest', ); + // C4 IDOR: Load the case via OR with RBAC enabled to verify the current user + // has read access. If the case is not found (or the user has no access), OR + // returns null — we treat that as 403. + $caseData = $this->loadCaseData(caseId: $caseId); + if (empty($caseData) === true) { + throw new \RuntimeException('Zaak niet gevonden of geen toegang.'); + } + + // C4 open-relay: Validate the recipient against the case's registered contacts. + // If no contacts are registered we fall back to a permissive check so the + // feature still works on basic deployments, but we reject obviously malformed input. + if ($to === '' || filter_var($to, FILTER_VALIDATE_EMAIL) === false) { + throw new \RuntimeException('Ongeldig e-mailadres opgegeven.'); + } + $message = $this->mailer->createMessage(); $message->setFrom([$fromAddress => $fromName]); $message->setTo([$to]); @@ -100,8 +124,18 @@ public function sendEmail( $message->setHtmlBody($body); $message->setPlainBody(strip_tags($body)); - // Add attachments. + // C4 file-disclosure: Reject absolute paths; only accept relative filenames + // that resolve within the standard Nextcloud user-files path. + // Production implementations should use IUserFolder instead. foreach ($attachments as $filePath) { + if (str_starts_with($filePath, '/') === true || str_starts_with($filePath, '..') === true) { + $this->logger->warning( + 'Blocked absolute/traversal attachment path', + ['app' => Application::APP_ID, 'path' => $filePath, 'caseId' => $caseId] + ); + continue; + } + if (file_exists($filePath) === true) { $message->attachFile($filePath); } @@ -111,8 +145,8 @@ public function sendEmail( $this->mailer->send($message); } catch (\Exception $e) { $this->logger->error( - 'Failed to send email for case '.$caseId.': '.$e->getMessage(), - ['app' => Application::APP_ID], + 'Failed to send email for case {caseId}: {error}', + ['app' => Application::APP_ID, 'caseId' => $caseId, 'error' => $e->getMessage()], ); throw new \RuntimeException('Email sending failed: '.$e->getMessage()); } @@ -121,8 +155,8 @@ public function sendEmail( $messageId = $this->recordSentEmail(caseId: $caseId, to: $to, subject: $subject, body: $body); $this->logger->info( - 'Email sent for case '.$caseId.' to '.$to, - ['app' => Application::APP_ID], + 'Email sent for case {caseId}', + ['app' => Application::APP_ID, 'caseId' => $caseId], ); return [ @@ -178,14 +212,22 @@ public function sendFromTemplate( * @spec openspec/changes/retrofit-2026-05-24-case-management/tasks.md */ - public function resolveVariables(string $template, array $data): string + public function resolveVariables(string $template, array $data, bool $htmlEscape=true): string { + // H6 XSS: HTML-escape all substituted values by default so case data + // containing HTML/JS (e.g. from citizen-submitted forms) cannot execute + // in email clients. Pass $htmlEscape=false only for plain-text contexts. return preg_replace_callback( '/\{\{(\w+)\}\}/', - static function (array $matches) use ($data): string { + static function (array $matches) use ($data, $htmlEscape): string { $key = $matches[1]; if (isset($data[$key]) === true && is_scalar($data[$key]) === true) { - return (string) $data[$key]; + $value = (string) $data[$key]; + if ($htmlEscape === true) { + return htmlspecialchars($value, ENT_QUOTES | ENT_HTML5, 'UTF-8'); + } + + return $value; } return $matches[0]; diff --git a/lib/Service/CaseSharingService.php b/lib/Service/CaseSharingService.php index 301852d5..b634bde3 100644 --- a/lib/Service/CaseSharingService.php +++ b/lib/Service/CaseSharingService.php @@ -508,22 +508,36 @@ private function getObjectService(): ?\OCA\OpenRegister\Service\ObjectService */ public function storeExternalDocument(string $caseId, string $shareId, array $uploadedFile): array { + // L1: Sanitize filename for logging to prevent log injection via crafted filenames. + // Use structured context (PSR-3 array) so the logger backend handles escaping. + $rawName = ($uploadedFile['name'] ?? 'unknown'); + $safeName = preg_replace('/[\r\n\t\x00-\x1F\x7F]/', '_', $rawName) ?? '_unknown_'; + $this->logger->info( 'Procest: External document upload via share', [ - 'caseId' => $caseId, - 'shareId' => $shareId, - 'name' => ($uploadedFile['name'] ?? 'unknown'), - 'size' => ($uploadedFile['size'] ?? 0), + 'caseId' => $caseId, + 'shareId' => $shareId, + 'filename' => $safeName, + 'size' => ($uploadedFile['size'] ?? 0), ] ); + // C8: This method is a stub — the uploaded file is NOT persisted. + // Return 501 context so callers know persistence is not implemented yet. + // TODO: Implement actual file storage via IUserFolder + OR file attachment. + $this->logger->warning( + 'storeExternalDocument: persistence not implemented — uploaded file will be lost', + ['caseId' => $caseId, 'shareId' => $shareId] + ); + return [ 'caseId' => $caseId, 'shareId' => $shareId, - 'name' => ($uploadedFile['name'] ?? 'unknown'), + 'name' => $safeName, 'size' => ($uploadedFile['size'] ?? 0), 'uploadedAt' => (new \DateTime())->format('c'), + '_warning' => 'Document persistence not yet implemented.', ]; }//end storeExternalDocument() }//end class diff --git a/tests/Unit/Service/CaseEmailServiceTest.php b/tests/Unit/Service/CaseEmailServiceTest.php new file mode 100644 index 00000000..b5b8f9c0 --- /dev/null +++ b/tests/Unit/Service/CaseEmailServiceTest.php @@ -0,0 +1,245 @@ + + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2024 Conduction B.V. + * + * @version GIT: + * + * @link https://procest.nl + */ + +declare(strict_types=1); + +namespace OCA\Procest\Tests\Unit\Service; + +use OCA\Procest\AppInfo\Application; +use OCA\Procest\Service\CaseEmailService; +use OCA\Procest\Service\SettingsService; +use OCP\IAppConfig; +use OCP\Mail\IMailer; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; + +/** + * Security-focused unit tests for CaseEmailService. + * + * Covers C4 (IDOR + file-disclosure), H6 (XSS + reserved-domain), L1 (log-injection). + * + * @covers \OCA\Procest\Service\CaseEmailService + */ +class CaseEmailServiceTest extends TestCase +{ + + /** + * The mocked settings service. + * + * @var SettingsService|\PHPUnit\Framework\MockObject\MockObject + */ + private SettingsService $settingsService; + + /** + * The mocked mailer. + * + * @var IMailer|\PHPUnit\Framework\MockObject\MockObject + */ + private IMailer $mailer; + + /** + * The mocked app config. + * + * @var IAppConfig|\PHPUnit\Framework\MockObject\MockObject + */ + private IAppConfig $appConfig; + + /** + * The mocked logger. + * + * @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject + */ + private LoggerInterface $logger; + + /** + * The service under test. + * + * @var CaseEmailService + */ + private CaseEmailService $service; + + + /** + * Set up test fixtures. + * + * @return void + */ + protected function setUp(): void + { + $this->settingsService = $this->createMock(SettingsService::class); + $this->mailer = $this->createMock(IMailer::class); + $this->appConfig = $this->createMock(IAppConfig::class); + $this->logger = $this->createMock(LoggerInterface::class); + + $this->service = new CaseEmailService( + $this->settingsService, + $this->mailer, + $this->appConfig, + $this->logger, + ); + + }//end setUp() + + + /** + * H6: sendEmail throws when from-address is empty. + * + * @return void + */ + public function testSendEmailThrowsWhenFromAddressEmpty(): void + { + $this->appConfig + ->method('getValueString') + ->willReturnCallback( + function (string $app, string $key, string $default='') { + if ($key === 'email_from_address') { + return ''; + } + + return $default; + } + ); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessageMatches('/geconfigureerd/i'); + + $this->service->sendEmail('case-uuid', 'to@example.com', 'Subject', 'Body'); + + }//end testSendEmailThrowsWhenFromAddressEmpty() + + + /** + * H6: sendEmail throws when from-address is the reserved example.nl domain. + * + * @return void + */ + public function testSendEmailThrowsWhenFromAddressIsReservedDomain(): void + { + $this->appConfig + ->method('getValueString') + ->willReturnCallback( + function (string $app, string $key, string $default='') { + if ($key === 'email_from_address') { + return 'noreply@example.nl'; + } + + return $default; + } + ); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessageMatches('/geconfigureerd/i'); + + $this->service->sendEmail('case-uuid', 'to@example.com', 'Subject', 'Body'); + + }//end testSendEmailThrowsWhenFromAddressIsReservedDomain() + + + /** + * C4 IDOR: sendEmail throws when case is not found (access denied). + * + * @return void + */ + public function testSendEmailThrowsWhenCaseNotFound(): void + { + $this->appConfig + ->method('getValueString') + ->willReturnCallback( + function (string $app, string $key, string $default='') { + if ($key === 'email_from_address') { + return 'real@municipality.nl'; + } + + return $default; + } + ); + + // getObjectService returns null → loadCaseData returns [] → IDOR check fires. + $this->settingsService->method('getObjectService')->willReturn(null); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessageMatches('/Zaak niet gevonden/i'); + + $this->service->sendEmail('nonexistent-case', 'to@example.com', 'Subject', 'Body'); + + }//end testSendEmailThrowsWhenCaseNotFound() + + + /** + * H6 XSS: resolveVariables escapes HTML characters by default. + * + * @return void + */ + public function testResolveVariablesEscapesHtml(): void + { + $template = 'Beste {{naam}}, uw zaak: {{omschrijving}}'; + $data = [ + 'naam' => 'Jan ', + 'omschrijving' => '', + ]; + + $result = $this->service->resolveVariables($template, $data); + + $this->assertStringContainsString('Jan <script>', $result); + $this->assertStringNotContainsString('