From c10f5815613fb699825a0b5d9fe71ca5826fa0e3 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Thu, 28 May 2026 06:16:10 +0200 Subject: [PATCH] fix(email): prevent SMTP-cred round-trip, XSS in templates, DSN encoding C2: skip masked placeholder in updateEmailSettings so stored secrets are never overwritten by the redacted display value. C4: HTML-escape all user-controlled template variables before substitution. H5: remove {{ user.password }} substitution from processTemplate. M1: use rawurlencode for SMTP DSN userinfo in SettingsService. M2: fix empty-if encSuffix branch so logged DSN pattern is correct. L2: remove duplicate backward-compat block in processTemplate (dead code). bonus: fix rawrawurlencode typo in SymfonyEmailService::createSmtpTransport. --- lib/Controller/SettingsController.php | 8 +++--- lib/Service/SettingsService.php | 19 +++++++++------ lib/Service/SymfonyEmailService.php | 35 +++++++-------------------- 3 files changed, 24 insertions(+), 38 deletions(-) diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 6ca1b52a..1fbf9dfb 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -2014,13 +2014,13 @@ public function updateEmailSettings(): JSONResponse $data = $this->request->getParams(); $emailSettings = $data['emailSettings'] ?? $data; - $result = $this->settingsService->updateEmailSettings($emailSettings); + $updatedSettings = $this->settingsService->updateEmailSettings($emailSettings); return new JSONResponse( [ - 'success' => $result['success'], - 'message' => $result['message'] ?? 'Email settings updated successfully', - 'emailSettings' => $result['emailSettings'] ?? null, + 'success' => true, + 'message' => 'Email settings updated successfully', + 'emailSettings' => $updatedSettings, ] ); } catch (\Exception $e) { diff --git a/lib/Service/SettingsService.php b/lib/Service/SettingsService.php index 2b2c2f6e..c95bcd49 100644 --- a/lib/Service/SettingsService.php +++ b/lib/Service/SettingsService.php @@ -2085,17 +2085,22 @@ public function updateEmailSettings(array $emailSettings): array 'mailjetApiKey' => 'email_mailjet_api_key', 'mailjetSecretKey' => 'email_mailjet_secret_key', ]; + // Secret fields that are masked in GET responses — skip any value that is the mask placeholder. + $secretFields = ['smtpPassword', 'sendgridApiKey', 'mailgunApiKey', 'postmarkApiKey', 'sesSecretKey', 'mailjetSecretKey']; $updatedSettings = []; foreach ($allowedSettings as $settingKey => $configKey) { if (array_key_exists($settingKey, $emailSettings) === true) { $value = $emailSettings[$settingKey]; + // Skip masked placeholder — the client is echoing back the redacted value; preserve the real stored secret. + if (in_array($settingKey, $secretFields, true) === true && $value === '••••••••') { + continue; + } + // Convert boolean values to strings. if (is_bool($value) === true) { - $value = 'false'; - if ($value === true) { - } + $value = ($value === true) ? 'true' : 'false'; } $this->config->setValueString($this->appName, $configKey, (string) $value); @@ -2808,8 +2813,8 @@ private function createSmtpTransport(array $settings): \Symfony\Component\Mailer $dsn = sprintf( 'smtp://%s:%s@%s:%d', - urlencode($username), - urlencode($password), + rawurlencode($username), + rawurlencode($password), $host, $port ); @@ -2818,9 +2823,7 @@ private function createSmtpTransport(array $settings): \Symfony\Component\Mailer $dsn .= '?encryption='.$encryption; } - $encSuffix = ''; - if (empty($encryption) === false && $encryption !== 'none') { - } + $encSuffix = (empty($encryption) === false && $encryption !== 'none') ? '?encryption='.$encryption : ''; $dsnPattern = sprintf('smtp://***:***@%s:%d%s', $host, $port, $encSuffix); diff --git a/lib/Service/SymfonyEmailService.php b/lib/Service/SymfonyEmailService.php index 404560cc..8d333160 100644 --- a/lib/Service/SymfonyEmailService.php +++ b/lib/Service/SymfonyEmailService.php @@ -329,8 +329,8 @@ private function createSmtpTransport(array $settings): TransportInterface $dsn = sprintf( 'smtp://%s:%s@%s:%d', - rawrawurlencode($username), - rawrawurlencode($password), + rawurlencode($username), + rawurlencode($password), $host, $port ); @@ -998,44 +998,27 @@ private function processTemplate(string $template, array $templateData): string $org = $templateData['organization']; $processed = str_replace( search: '{{ organization.name }}', - replace: ($org['name'] ?? ''), + replace: htmlspecialchars((string) ($org['name'] ?? ''), ENT_QUOTES | ENT_HTML5, 'UTF-8'), subject: $processed ); } - // Replace user variables. + // Replace user variables — never include passwords in email output. if (isset($templateData['user']) === true) { $user = $templateData['user']; $processed = str_replace( search: '{{ user.name }}', - replace: ($user['name'] ?? ''), + replace: htmlspecialchars((string) ($user['name'] ?? ''), ENT_QUOTES | ENT_HTML5, 'UTF-8'), subject: $processed ); $processed = str_replace( search: '{{ user.email }}', - replace: ($user['email'] ?? ''), - subject: $processed - ); - $processed = str_replace( - search: '{{ user.password }}', - replace: ($user['password'] ?? ''), - subject: $processed - ); - } - - // Replace contact variables (backward compatibility). - if (isset($templateData['user']) === true) { - $user = $templateData['user']; - $processed = str_replace( - search: '{{ user.name }}', - replace: ($user['name'] ?? ''), - subject: $processed - ); - $processed = str_replace( - search: '{{ user.email }}', - replace: ($user['email'] ?? ''), + replace: htmlspecialchars((string) ($user['email'] ?? ''), ENT_QUOTES | ENT_HTML5, 'UTF-8'), subject: $processed ); + // Remove any {{ user.password }} placeholder that may appear in custom templates + // to prevent accidental credential exposure. + $processed = str_replace(search: '{{ user.password }}', replace: '', subject: $processed); } return $processed;