diff --git a/backend/__tests__/utils/spreadsheetSafe.test.js b/backend/__tests__/utils/spreadsheetSafe.test.js new file mode 100644 index 00000000..3abe645d --- /dev/null +++ b/backend/__tests__/utils/spreadsheetSafe.test.js @@ -0,0 +1,37 @@ +const { neutralizeSpreadsheetFormula } = require('../../src/utils/spreadsheetSafe'); +const { _internal } = require('../../src/services/ledgerService'); + +describe('neutralizeSpreadsheetFormula — CSV/Banana formula-injection defence (PR #622 blocker 1)', () => { + it.each([ + ['=', '=cmd|"/C calc"!A1'], + ['+', '+1+1'], + ['-', '-2+3'], + ['@', '@SUM(1+1)'], + ['tab', '\tSUM(A1)'], + ['carriage-return', '\rSUM(A1)'], + ])('prefixes a single quote when the cell starts with %s', (_label, payload) => { + const out = neutralizeSpreadsheetFormula(payload); + expect(out).toBe(`'${payload}`); + expect(out[0]).toBe("'"); + }); + + it('leaves safe values untouched', () => { + expect(neutralizeSpreadsheetFormula('LBM-R-2026-0001')).toBe('LBM-R-2026-0001'); + expect(neutralizeSpreadsheetFormula('Acme GmbH')).toBe('Acme GmbH'); + expect(neutralizeSpreadsheetFormula('29.40')).toBe('29.40'); + // A minus only mid-string is fine — only a LEADING risky char matters. + expect(neutralizeSpreadsheetFormula('Q-2026-0001')).toBe('Q-2026-0001'); + }); + + it('coerces null/undefined to empty string', () => { + expect(neutralizeSpreadsheetFormula(null)).toBe(''); + expect(neutralizeSpreadsheetFormula(undefined)).toBe(''); + }); + + it('ledgerService.csvEscape applies the prefix AND the RFC-4180 quote wrap', () => { + // formula cell → prefixed then quote-wrapped + expect(_internal.csvEscape('=1+1')).toBe('"\'=1+1"'); + // embedded quotes still doubled; safe value not prefixed + expect(_internal.csvEscape('a"b')).toBe('"a""b"'); + }); +}); diff --git a/backend/migrations/core/128_incoming_mail.js b/backend/migrations/core/128_incoming_mail.js index b9a0f4f3..a0c81b40 100644 --- a/backend/migrations/core/128_incoming_mail.js +++ b/backend/migrations/core/128_incoming_mail.js @@ -42,7 +42,13 @@ exports.up = async function (knex) { table.integer('inbound_document_id').unsigned(); table.text('error'); table.timestamp('created_at').defaultTo(knex.fn.now()); - table.index(['message_id']); + // UNIQUE (not just INDEX): message_id is the dedup/claim key for the IMAP + // poller. The in-process `polling` lock serialises within one backend, but a + // second replica / rolling-deploy overlap would otherwise let two workers + // both pass the check-then-insert and double-ingest the same mail. NULLs stay + // distinct (Postgres + SQLite) so no-Message-ID rows aren't blocked. The + // intake claims this row BEFORE ingesting. + table.unique(['message_id']); table.index(['status']); }); } diff --git a/backend/src/services/emailIntakeService.js b/backend/src/services/emailIntakeService.js index 8763720b..2a3bd3ff 100644 --- a/backend/src/services/emailIntakeService.js +++ b/backend/src/services/emailIntakeService.js @@ -16,6 +16,7 @@ const { db } = require('../database/db'); const logger = require('../utils/logger'); const { getStoragePath } = require('../config/storage'); const expenseService = require('./expenseService'); +const { isUniqueViolation } = require('../utils/dbErrors'); const ALLOWED_MIME = ['application/pdf', 'image/jpeg', 'image/png']; let polling = false; @@ -269,14 +270,46 @@ async function pollOnce() { // 4) Download + process each fresh message. for (const cand of fresh) { let messageId = cand.messageId; + let claimKey = null; + let claimed = false; try { const one = await client.fetchOne(String(cand.uid), { source: true }, { uid: true }); if (!one || !one.source) continue; const parsed = await simpleParser(one.source); messageId = parsed.messageId || cand.messageId; - // Re-check with the parsed id (can differ from the envelope's). - const dup = await db('received_emails').where({ message_id: messageId }).first(); - if (dup) { await client.messageFlagsAdd(cand.uid, ['\\Seen'], { uid: true }); continue; } + // Claim key: a no-Message-ID mail still needs a non-null, per-message + // key so two pollers converge — fall back to the mailbox uid. + claimKey = messageId || `nomsgid-${cand.uid}`; + + // Fast-path: already processed. Recover a row left 'processing' by a + // worker that crashed mid-ingest (>10 min) so the attachment isn't + // orphaned — otherwise skip + mark seen. + const existing = await db('received_emails').where({ message_id: claimKey }).first(); + if (existing) { + const staleProcessing = existing.status === 'processing' + && existing.created_at + && (Date.now() - new Date(existing.created_at).getTime() > 10 * 60 * 1000); + if (!staleProcessing) { await client.messageFlagsAdd(cand.uid, ['\\Seen'], { uid: true }); continue; } + await db('received_emails').where({ id: existing.id }).del(); + } + + // CLAIM the message atomically BEFORE any ingest. The message_id UNIQUE + // index (migration 128) makes this the real guard: if a second poller + // (multi-replica / rolling deploy) already claimed it, the insert hits + // the unique constraint and we skip cleanly — no double-ingest. + try { + await db('received_emails').insert({ + message_id: claimKey, + status: 'processing', + attachment_count: 0, + received_at: new Date(), + created_at: new Date(), + }); + claimed = true; + } catch (ce) { + if (isUniqueViolation(ce)) { await client.messageFlagsAdd(cand.uid, ['\\Seen'], { uid: true }); continue; } + throw ce; + } // Ingest attachments. Isolate each so one bad file can't prevent the // audit row (the symptom: doc lands in Incoming invoices but the @@ -300,10 +333,9 @@ async function pollOnce() { // Postgres timestamp insert — coerce to now. const receivedAt = (parsed.date instanceof Date && !Number.isNaN(parsed.date.getTime())) ? parsed.date : new Date(); const status = count > 0 ? 'ingested' : (attErrors.length ? 'error' : 'no_attachment'); - // Audit EVERY processed message, even attachment-less ones, so the - // Received tab is a complete log. - await db('received_emails').insert({ - message_id: messageId, + // Finalise the claimed row — every processed message ends up in the + // Received tab, even attachment-less ones. + await db('received_emails').where({ message_id: claimKey }).update({ from_address: ((parsed.from && parsed.from.text) || '').slice(0, 512) || null, subject: parsed.subject || null, received_at: receivedAt, @@ -311,7 +343,6 @@ async function pollOnce() { status, inbound_document_id: inboundId, error: attErrors.length ? attErrors.join('; ').slice(0, 2000) : null, - created_at: new Date(), }); await client.messageFlagsAdd(cand.uid, ['\\Seen'], { uid: true }); processed += 1; @@ -320,7 +351,13 @@ async function pollOnce() { // Received row. logger.error?.(`emailIntake: message uid ${cand.uid} (${messageId}) failed: ${e.message}`); try { - await db('received_emails').insert({ message_id: `err-${cand.uid}-${Date.now()}`, status: 'error', error: e.message, attachment_count: 0, received_at: new Date(), created_at: new Date() }); + if (claimed && claimKey) { + // We already claimed the row — mark it errored rather than orphan it. + await db('received_emails').where({ message_id: claimKey }) + .update({ status: 'error', error: String(e.message).slice(0, 2000) }); + } else { + await db('received_emails').insert({ message_id: `err-${cand.uid}-${Date.now()}`, status: 'error', error: e.message, attachment_count: 0, received_at: new Date(), created_at: new Date() }); + } } catch (ie) { logger.error?.(`emailIntake: could not even write the error row (received_emails insert failing): ${ie.message}`); } diff --git a/backend/src/services/ledgerService.js b/backend/src/services/ledgerService.js index 35f0a560..a1b873ea 100644 --- a/backend/src/services/ledgerService.js +++ b/backend/src/services/ledgerService.js @@ -21,6 +21,7 @@ const { db, withRetry } = require('../database/db'); const { getAppSetting } = require('../utils/appSettings'); const { buildCustomerLabel } = require('./taxReportService')._internal; const { ensureInt } = require('../utils/numericHelpers'); +const { neutralizeSpreadsheetFormula } = require('../utils/spreadsheetSafe'); const ACCOUNT_TYPES = ['asset', 'liability', 'equity', 'revenue', 'expense']; const VAT_DIRECTIONS = ['output', 'input']; @@ -391,7 +392,8 @@ async function buildPostings({ from, to, currency } = {}) { // ── export formatters ──────────────────────────────────────────────── function csvEscape(cell) { - const s = cell === null || cell === undefined ? '' : String(cell); + // Formula-injection defence (Excel/Numbers/Banana) THEN RFC-4180 quote-wrap. + const s = neutralizeSpreadsheetFormula(cell === null || cell === undefined ? '' : String(cell)); return `"${s.replace(/"/g, '""')}"`; } function minorToDecimal(m) { return ((Number(m) || 0) / 100).toFixed(2); } @@ -473,7 +475,7 @@ async function exportPostings({ from, to, currency, format = 'generic' } = {}) { // Tab layout: strip any tab/newline from a cell so it can't split the row; // CSV cells go through the RFC-4180 quoter instead. const fmtCell = isTab - ? (v) => String(v == null ? '' : v).replace(/[\t\r\n]+/g, ' ') + ? (v) => neutralizeSpreadsheetFormula(String(v == null ? '' : v).replace(/[\t\r\n]+/g, ' ')) : csvEscape; const lines = [headers.map(fmtCell).join(sep)]; for (const p of postings) lines.push(rowOf(p).map(fmtCell).join(sep)); @@ -501,5 +503,5 @@ module.exports = { listVatCodes, createVatCode, updateVatCode, deleteVatCode, getMappings, setCategoryAccount, updateSettings, getConfig, buildPostings, exportPostings, - _internal: { rateKey, csvEscape, minorToDecimal }, + _internal: { rateKey, csvEscape, neutralizeSpreadsheetFormula, minorToDecimal }, }; diff --git a/backend/src/services/taxReportService.js b/backend/src/services/taxReportService.js index 1a1a013c..c08c2ce6 100644 --- a/backend/src/services/taxReportService.js +++ b/backend/src/services/taxReportService.js @@ -49,6 +49,7 @@ const REPORTABLE_STATUSES = ['sent', 'paid', 'overdue', 'pending_delivery', 'can // D.2 — `ensureInt` consolidated into utils/numericHelpers. const { ensureInt } = require('../utils/numericHelpers'); +const { neutralizeSpreadsheetFormula } = require('../utils/spreadsheetSafe'); const logger = require('../utils/logger'); function ensureRate(v) { @@ -997,9 +998,10 @@ async function renderTaxReportCsv({ from, to, currency, locale } = {}) { const useLocale = locale || 'en'; const escape = (cell) => { - const s = cell === null || cell === undefined ? '' : String(cell); - // RFC 4180: wrap in quotes when the value contains comma, quote, - // or newline. We always wrap, simpler + bulletproof for Excel. + // Formula-injection defence (Excel/Numbers) THEN RFC-4180 quote-wrap. The + // quote wrap alone does NOT stop formula evaluation — only the leading + // single-quote prefix does. + const s = neutralizeSpreadsheetFormula(cell === null || cell === undefined ? '' : String(cell)); return `"${s.replace(/"/g, '""')}"`; }; diff --git a/backend/src/utils/dbErrors.js b/backend/src/utils/dbErrors.js new file mode 100644 index 00000000..d4424598 --- /dev/null +++ b/backend/src/utils/dbErrors.js @@ -0,0 +1,15 @@ +/** + * Cross-driver detector for a unique-constraint violation. The error shape + * varies by driver: Postgres → SQLSTATE `23505`; better-sqlite3 → + * "UNIQUE constraint failed"; node-sqlite3 → `SQLITE_CONSTRAINT`. Used by the + * claim-then-work concurrency patterns (document_sequences, monthly-draft, the + * IMAP intake claim) to converge cleanly when a concurrent writer wins the race. + */ +function isUniqueViolation(err) { + if (!err) return false; + if (err.code === '23505' || err.code === 'SQLITE_CONSTRAINT') return true; + const msg = String(err.message || ''); + return /unique/i.test(msg) || /sqlite_constraint/i.test(msg); +} + +module.exports = { isUniqueViolation }; diff --git a/backend/src/utils/spreadsheetSafe.js b/backend/src/utils/spreadsheetSafe.js new file mode 100644 index 00000000..b8f7fcb5 --- /dev/null +++ b/backend/src/utils/spreadsheetSafe.js @@ -0,0 +1,19 @@ +/** + * Formula-injection defence for spreadsheet / accounting exports (CSV + Banana). + * + * A cell whose first character is one of `= + - @ TAB CR` is evaluated as a + * formula when the file is opened in Excel / Numbers / Banana. RFC-4180 + * quote-wrapping does NOT stop that evaluation — only prefixing a single quote + * does. Vectors in picpeak are real: supplier_name, invoice_number, + * payment_reference and description are admin-editable (and sender-controlled + * once incoming-mail ingestion is live). + * + * Apply to BOTH the quoted CSV and the unquoted tab-separated Banana export — + * the tab export has no surrounding quotes, so it's the more exposed of the two. + */ +function neutralizeSpreadsheetFormula(value) { + const s = value === null || value === undefined ? '' : String(value); + return /^[=+\-@\t\r]/.test(s) ? `'${s}` : s; +} + +module.exports = { neutralizeSpreadsheetFormula };