From bb235e72e58359f55f8aeccf2f22c671584fdbd7 Mon Sep 17 00:00:00 2001 From: Luca <102960244+Luca-Timo@users.noreply.github.com> Date: Tue, 7 Jul 2026 18:28:26 +0200 Subject: [PATCH] =?UTF-8?q?fix(messages):=20PR=20#769=20review=20=E2=80=94?= =?UTF-8?q?=20escape=20reply=20sender=20(XSS),=20gate=20backend=20routes,?= =?UTF-8?q?=20exact=20customer=20match?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - BLOCKER: stored XSS via inbound sender display name. The reply stub built raw HTML with the unsanitized From name and set it as innerHTML on the composer's contentEditable (admin origin) → onerror JS ran on Reply. Now HTML-escape from_address in the stub AND DOMPurify-sanitize the composer body before innerHTML (defense in depth). - Gate the NEW Messages routes with requireFeatureFlag('messaging') per-route (queue/:id, received/:id, item/*, identities, accounts, accounts/test, send) — NOT the shared /email mount, so the pre-existing email-config endpoints stay ungated. - DocumentActionModal auto-picks a customer only on an EXACT email match (customer search is prefix/fuzzy), else leaves the picker to the admin. --- backend/src/routes/adminEmail.js | 22 +++++++++++-------- .../admin/messages/DocumentActionModal.tsx | 10 ++++++++- .../pages/admin/messages/MessageComposer.tsx | 5 ++++- .../src/pages/admin/messages/MessagesPage.tsx | 8 ++++++- 4 files changed, 33 insertions(+), 12 deletions(-) diff --git a/backend/src/routes/adminEmail.js b/backend/src/routes/adminEmail.js index 41cc6866..f69702b3 100644 --- a/backend/src/routes/adminEmail.js +++ b/backend/src/routes/adminEmail.js @@ -4,6 +4,10 @@ const { body, query, validationResult } = require('express-validator'); const { db, logActivity } = require('../database/db'); const { adminAuth } = require('../middleware/auth'); const { requirePermission } = require('../middleware/permissions'); +// Gate the NEW Messages routes on the `messaging` flag (per-route, NOT the whole +// /email mount — the pre-existing config/queue/received endpoints stay ungated). +const { requireFeatureFlag } = require('../middleware/requireFeatureFlag'); +const messagingGate = requireFeatureFlag('messaging'); const { wrapEmailHtml, processEmailQueue } = require('../services/emailProcessor'); const { errorResponse } = require('../utils/routeHelpers'); const logger = require('../utils/logger'); @@ -287,7 +291,7 @@ router.get('/received', adminAuth, requirePermission('email.view'), async (req, // Single received email WITH its captured (server-sanitized) body — Messages // reading pane. body_html was already sanitized on ingest; the viewer renders // it in a script-less sandboxed iframe as well. -router.get('/received/:id', adminAuth, requirePermission('email.view'), async (req, res) => { +router.get('/received/:id', adminAuth, messagingGate, requirePermission('email.view'), async (req, res) => { try { const id = parseInt(req.params.id, 10); if (!Number.isInteger(id)) return res.status(400).json({ error: 'Invalid id' }); @@ -302,7 +306,7 @@ router.get('/received/:id', adminAuth, requirePermission('email.view'), async (r // Move an email between mailbox states: Archive / Delete (soft) or Restore // (back to active). kind = 'queue' | 'received'. Delete is a soft move to the // trash; the row is only removed for good by the DELETE handler below. -router.post('/item/:kind/:id/state', adminAuth, requirePermission('email.view'), async (req, res) => { +router.post('/item/:kind/:id/state', adminAuth, messagingGate, requirePermission('email.view'), async (req, res) => { try { const table = req.params.kind === 'received' ? 'received_emails' : req.params.kind === 'queue' ? 'email_queue' : null; if (!table) return res.status(400).json({ error: 'Invalid kind' }); @@ -319,7 +323,7 @@ router.post('/item/:kind/:id/state', adminAuth, requirePermission('email.view'), }); // Permanently delete an email row — only offered from the Deleted folder. -router.delete('/item/:kind/:id', adminAuth, requirePermission('email.edit'), async (req, res) => { +router.delete('/item/:kind/:id', adminAuth, messagingGate, requirePermission('email.edit'), async (req, res) => { try { const table = req.params.kind === 'received' ? 'received_emails' : req.params.kind === 'queue' ? 'email_queue' : null; if (!table) return res.status(400).json({ error: 'Invalid kind' }); @@ -334,7 +338,7 @@ router.delete('/item/:kind/:id', adminAuth, requirePermission('email.edit'), asy // Additional inbound mailboxes (beyond the primary accounting IMAP in // email_configs) — e.g. the customer hello@ box. Passwords are masked out. -router.get('/accounts', adminAuth, requirePermission('email.view'), async (req, res) => { +router.get('/accounts', adminAuth, messagingGate, requirePermission('email.view'), async (req, res) => { try { const rows = await db('mail_accounts').orderBy('id'); res.json({ items: rows.map((a) => ({ @@ -351,7 +355,7 @@ router.get('/accounts', adminAuth, requirePermission('email.view'), async (req, // the REAL configured addresses instead of hardcoded placeholders. Accounting = // the primary IMAP login (rechnungen@); customers = the hello@ mailbox; the // automated stream sends from the global SMTP from-address. -router.get('/identities', adminAuth, requirePermission('email.view'), async (req, res) => { +router.get('/identities', adminAuth, messagingGate, requirePermission('email.view'), async (req, res) => { try { const cfg = await db('email_configs').first(); let customers = null; @@ -371,7 +375,7 @@ router.get('/identities', adminAuth, requirePermission('email.view'), async (req // Upsert a mailbox by account_key. A masked password ('********') keeps the // stored value so the admin never has to re-type it. -router.post('/accounts', adminAuth, requirePermission('email.edit'), async (req, res) => { +router.post('/accounts', adminAuth, messagingGate, requirePermission('email.edit'), async (req, res) => { try { const b = req.body || {}; if (!b.account_key) return res.status(400).json({ error: 'account_key is required' }); @@ -423,7 +427,7 @@ router.post('/accounts', adminAuth, requirePermission('email.edit'), async (req, // Test an inbound mailbox's IMAP connection (before or after saving). Resolves // a masked/blank password from the stored row for the given account_key. -router.post('/accounts/test', adminAuth, requirePermission('email.view'), async (req, res) => { +router.post('/accounts/test', adminAuth, messagingGate, requirePermission('email.view'), async (req, res) => { try { const b = req.body || {}; const { isPrivateIP } = require('../utils/networkValidation'); @@ -710,7 +714,7 @@ router.get('/queue', adminAuth, requirePermission('email.view'), [ // 119); rows sent before that migration have none. Attachment disk paths in // `email_data` are never exposed — only the filenames, so the pane can list // attachments without leaking storage paths (same PII posture as the list). -router.get('/queue/:id', adminAuth, requirePermission('email.view'), async (req, res) => { +router.get('/queue/:id', adminAuth, messagingGate, requirePermission('email.view'), async (req, res) => { try { const id = parseInt(req.params.id, 10); if (!Number.isInteger(id)) return res.status(400).json({ error: 'Invalid id' }); @@ -760,7 +764,7 @@ router.get('/queue/:id', adminAuth, requirePermission('email.view'), async (req, // edited the body (reply or document message), so it is sent as-is — no // template render — after a sanitize pass. Recorded in email_queue as a // 'manual' send so it surfaces under Customers > Sent. -router.post('/send', adminAuth, requirePermission('email.send'), async (req, res) => { +router.post('/send', adminAuth, messagingGate, requirePermission('email.send'), async (req, res) => { try { const b = req.body || {}; const to = String(b.to || '').trim(); diff --git a/frontend/src/pages/admin/messages/DocumentActionModal.tsx b/frontend/src/pages/admin/messages/DocumentActionModal.tsx index 9cf5cde5..3a9f4df3 100644 --- a/frontend/src/pages/admin/messages/DocumentActionModal.tsx +++ b/frontend/src/pages/admin/messages/DocumentActionModal.tsx @@ -48,7 +48,15 @@ export const DocumentActionModal: React.FC<{ let cancelled = false; setResolving(true); customerAdminService.search(senderEmail) - .then((rows) => { if (!cancelled && rows.length) pick(rows[0]); }) + .then((rows) => { + if (cancelled) return; + // search matches email/name/company PREFIXES — only auto-pick on an + // EXACT email match so a spoofed/partial sender can't prefill the wrong + // customer. Otherwise leave the picker for the admin to choose. + const target = senderEmail.trim().toLowerCase(); + const exact = rows.find((r) => (r.email || '').toLowerCase() === target); + if (exact) pick(exact); + }) .catch(() => {}) .finally(() => { if (!cancelled) setResolving(false); }); return () => { cancelled = true; }; diff --git a/frontend/src/pages/admin/messages/MessageComposer.tsx b/frontend/src/pages/admin/messages/MessageComposer.tsx index 614b1440..c5cc7f0a 100644 --- a/frontend/src/pages/admin/messages/MessageComposer.tsx +++ b/frontend/src/pages/admin/messages/MessageComposer.tsx @@ -1,5 +1,6 @@ import React, { useEffect, useRef, useState } from 'react'; import { useMutation } from '@tanstack/react-query'; +import DOMPurify from 'dompurify'; import { X, Send as SendIcon } from 'lucide-react'; import { toast } from 'react-toastify'; import { emailService } from '../../../services/email.service'; @@ -35,7 +36,9 @@ export const MessageComposer: React.FC<{ const bodyRef = useRef(null); useEffect(() => { - if (bodyRef.current) bodyRef.current.innerHTML = init.html || ''; + // Sanitize before it hits the contentEditable innerHTML — the initial body + // can include untrusted text (e.g. an inbound sender name in a reply stub). + if (bodyRef.current) bodyRef.current.innerHTML = DOMPurify.sanitize(init.html || ''); // Load initial body exactly once; further edits are the admin's. // eslint-disable-next-line react-hooks/exhaustive-deps }, []); diff --git a/frontend/src/pages/admin/messages/MessagesPage.tsx b/frontend/src/pages/admin/messages/MessagesPage.tsx index 74850837..ea00cf6a 100644 --- a/frontend/src/pages/admin/messages/MessagesPage.tsx +++ b/frontend/src/pages/admin/messages/MessagesPage.tsx @@ -55,6 +55,12 @@ const fmt = (s?: string | null) => // narrow sidebar); full address stays in the hover title. const localPart = (addr?: string | null) => (addr ? `${addr.split('@')[0]}@` : ''); +// Escape untrusted text before it goes into an HTML string. The inbound From +// header carries an attacker-controlled display name; the reply stub builds raw +// HTML for the (contentEditable) composer, so this MUST be escaped there. +const escapeHtml = (s: string) => + s.replace(/[&<>"']/g, (c) => (({ '&': '&', '<': '<', '>': '>', '"': '"', "'": ''' } as Record)[c])); + const STATUS_STYLES: Record = { sent: 'bg-green-100 text-green-800 dark:bg-green-900/40 dark:text-green-300', ingested: 'bg-green-100 text-green-800 dark:bg-green-900/40 dark:text-green-300', @@ -527,7 +533,7 @@ const ReadingPane: React.FC<{ ? () => { const it = selection.item; const subj = /^re:/i.test(it.subject || '') ? (it.subject || '') : `Re: ${it.subject || ''}`; - const quoted = `


${t('messages.onWrote', 'On')} ${fmt(it.received_at)}, ${it.from_address}:

`; + const quoted = `


${t('messages.onWrote', 'On')} ${fmt(it.received_at)}, ${escapeHtml(it.from_address || '')}:

`; onCompose({ to: it.from_address || '', subject: subj, html: quoted, replyToReceivedId: it.id }, t('messages.reply', 'Reply')); } : undefined;