fix(accounting): PR #622 blockers — CSV formula injection + IMAP double-ingest race
Blocker 1 — CSV/Banana formula injection. Neither csvEscape (ledgerService) nor the tax-report CSV escape nor the unquoted tab-separated Banana cell formatter prefixed risky leading chars, so an admin-/sender-controlled cell beginning with = + - @ TAB CR executes as a formula when the Treuhänder opens the export. New shared util neutralizeSpreadsheetFormula() prepends a single quote; wired into all three sinks (quoted CSV + unquoted Banana). Unit test pins one of each char. Blocker 2 — IMAP intake double-ingest race. received_emails.message_id was INDEX, not UNIQUE, and the poller ingested attachments BEFORE writing the audit row, so a second replica / rolling-deploy overlap double-ingested the same mail. Migration 128 makes message_id UNIQUE (nulls stay distinct); the intake now CLAIMS the message row (status='processing') BEFORE ingesting — a concurrent claim hits the unique constraint and skips cleanly (shared isUniqueViolation helper). Stale 'processing' rows (worker crashed mid-ingest) are reclaimed after 10 min so no attachment is orphaned. NOT done (deliberate): the suggested UNIQUE on inbound_documents.file_sha256 — that column is a SOFT dedup key by design (manual re-uploads are kept as flagged 'duplicate' rows + duplicate_of_id for the Duplikat disposition); a unique index would break that feature. The file race only yields an extra 'unsorted' row (a data-quality nit, caught by the existing manual Duplikat backstop), not a double-count. Rationale to be added to the PR reply.
This commit is contained in:
Luca
@@ -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"');
|
||||
});
|
||||
});
|
||||
@@ -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']);
|
||||
});
|
||||
}
|
||||
|
||||
@@ -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}`);
|
||||
}
|
||||
|
||||
@@ -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 },
|
||||
};
|
||||
|
||||
@@ -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, '""')}"`;
|
||||
};
|
||||
|
||||
|
||||
@@ -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 };
|
||||
@@ -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 };
|
||||
Reference in New Issue
Block a user