fix(accounting): PR #622 concerns — flag-cache, customer master gate, VAT-unconfigured, helpers, page cap

1. requireFeatureFlag now caches each flag for 10s (the accounting area is 10+ gated endpoints); PUT /admin/feature-flags invalidates the cache so toggles still take effect immediately. 2. Customer routes (/quotes, /invoices, /contracts + their PDFs) now gate via getEffectiveFeaturesForCustomer — the global MASTER flag AND the per-customer override — instead of the per-customer column alone, via a shared customerFeatureAllowed() helper. Admin disabling a feature globally is now honoured for customers too. 4. Tax-report VAT-payable: when accounting_vat_registered is UNSET, stop guessing from grandTotalVat>0 (a zero-output-VAT quarter silently flipped to "not registered" and hid the reclaim). Treat null as "not configured": vatPayableMinor=null + vatRegistrationConfigured=false; the UI renders "—" and a "configure VAT registration" warning. Tests updated. 5. Shared upsertAppSetting() in utils/appSettings — the two adminSettings upsert loops use it, so the app_settings created_at class can't be re-introduced. 6. PDF rasterise per-file bound: getRenderedPagePath refuses pages beyond MAX_RENDERABLE_PAGES (200); page_count is capped to match at ingest, so a hostile high-page PDF can't drive an unbounded pager. 7. (no code) original_filename is only rendered via auto-escaped JSX; the two dangerouslySetInnerHTML sites are admin-authored content — paranoia pass clean. Concerns 3 (foreign-VAT reclaim-country) and 8 (imap_pass plaintext) are PR-reply / doc items, addressed in the PR response, not code.
2026-06-16 18:33:47 +02:00
parent cd6d57839b
commit a93b6dc232
13 changed files with 134 additions and 80 deletions
@@ -371,7 +371,9 @@ describe('getTaxReport', () => {
    expect(out.summary).toMatchObject({
      incomeNetMinor: 10000, incomeVatMinor: 770, incomeGrossMinor: 10770,
      costNetMinor: 0, costVatMinor: 0, costGrossMinor: 0,
-      resultNetMinor: 10000, resultGrossMinor: 10770, vatPayableMinor: 770,
+      resultNetMinor: 10000, resultGrossMinor: 10770,
+      // VAT registration unconfigured in the test DB → refuse to compute payable.
+      vatRegistrationConfigured: false, vatPayableMinor: null,
    });
  });

@@ -433,7 +435,8 @@ describe('getTaxReport', () => {
    expect(out.summary).toMatchObject({
      incomeNetMinor: 100000, incomeVatMinor: 7700, incomeGrossMinor: 107700,
      costNetMinor: 25000, costVatMinor: 1540, costGrossMinor: 26540,
-      resultNetMinor: 75000, resultGrossMinor: 81160, vatPayableMinor: 6160,
+      resultNetMinor: 75000, resultGrossMinor: 81160,
+      vatRegistrationConfigured: false, vatPayableMinor: null,
    });
  });

@@ -6,27 +6,50 @@
 * hit must still be refused so a disabled feature is never actable. Mirrors the
 * truthy logic feature_flags uses everywhere (true | 1 | '1').
 *
+ * Cached: the accounting area alone is 10+ gated endpoints and the dashboard
+ * polls several, so a per-request DB read is wasteful. Flags change rarely and
+ * only via `PUT /admin/feature-flags`, which calls invalidateFeatureFlagCache()
+ * — so a short TTL is belt-and-braces against any other mutation path.
+ *
 * Several route files (adminLedger, adminExpenses) predate this and define an
 * identical local `requireFlag`; new gates should import this instead.
 */
 const { db } = require('../database/db');

+const TTL_MS = 10_000;
+const cache = new Map(); // key -> { enabled, expires }
+
+function flagEnabledFromRow(row) {
+  return !!(row && (row.value === true || row.value === 1 || row.value === '1'));
+}
+
+async function isFeatureEnabled(key) {
+  const now = Date.now();
+  const hit = cache.get(key);
+  if (hit && hit.expires > now) return hit.enabled;
+  const row = await db('feature_flags').where({ key }).first();
+  const enabled = flagEnabledFromRow(row);
+  cache.set(key, { enabled, expires: now + TTL_MS });
+  return enabled;
+}
+
+/** Clear the flag cache — call after any write to feature_flags. */
+function invalidateFeatureFlagCache() {
+  cache.clear();
+}
+
 function requireFeatureFlag(key, code) {
  return async (req, res, next) => {
    try {
-      const row = await db('feature_flags').where({ key }).first();
-      const enabled = row && (row.value === true || row.value === 1 || row.value === '1');
-      if (!enabled) {
-        return res.status(403).json({
-          error: `${key} feature is disabled`,
-          code: code || `${key.replace(/([a-z])([A-Z])/g, '$1_$2').toUpperCase()}_DISABLED`,
-        });
-      }
-      return next();
+      if (await isFeatureEnabled(key)) return next();
+      return res.status(403).json({
+        error: `${key} feature is disabled`,
+        code: code || `${key.replace(/([a-z])([A-Z])/g, '$1_$2').toUpperCase()}_DISABLED`,
+      });
    } catch (err) {
      return next(err);
    }
  };
 }

-module.exports = { requireFeatureFlag };
+module.exports = { requireFeatureFlag, isFeatureEnabled, invalidateFeatureFlagCache };
@@ -18,6 +18,7 @@ const router = express.Router();
 const { db, logActivity } = require('../database/db');
 const { adminAuth } = require('../middleware/auth');
 const { requirePermission } = require('../middleware/permissions');
+const { invalidateFeatureFlagCache } = require('../middleware/requireFeatureFlag');
 const logger = require('../utils/logger');

 // Canonical flag list. Keep in sync with frontend
@@ -226,6 +227,10 @@ router.put('/', adminAuth, requirePermission('settings.edit'), async (req, res)
      }
    });

+    // Drop the requireFeatureFlag middleware's short-TTL cache so a toggle takes
+    // effect immediately instead of after ≤10s.
+    invalidateFeatureFlagCache();
+
    await logActivity(
      'feature_flags_updated',
      { changed, actor: adminUsername },
@@ -19,6 +19,7 @@ const {
  getRawPublicSiteSettings,
 } = require('../services/publicSiteService');
 const { sanitizeCss } = require('../utils/cssSanitizer');
+const { upsertAppSetting } = require('../utils/appSettings');
 const { clearShareLinkSettingsCache } = require('../services/shareLinkService');
 const { resetSecurityConfigCache } = require('../utils/authSecurity');
 const router = express.Router();
@@ -211,19 +212,7 @@ router.put('/customer-surface', adminAuth, requirePermission('settings.edit'), a
    }

    for (const u of updates) {
-      const existing = await db('app_settings').where('setting_key', u.setting_key).first();
-      if (existing) {
-        await db('app_settings').where('setting_key', u.setting_key).update({
-          setting_value: u.setting_value,
-          setting_type: u.setting_type,
-          updated_at: new Date(),
-        });
-      } else {
-        // app_settings has no created_at column (see src/database/db.js: only
-        // setting_key/value/type + updated_at). Inserting created_at errors —
-        // which broke first-time keys like the VAT-registration toggle.
-        await db('app_settings').insert({ ...u, updated_at: new Date() });
-      }
+      await upsertAppSetting(u.setting_key, u.setting_value, u.setting_type);
    }

    // Clear the public-site cache so any consumer relying on it
@@ -282,17 +271,7 @@ router.put('/accounting', adminAuth, requirePermission('settings.edit'), async (
      });
    }
    for (const u of updates) {
-      const existing = await db('app_settings').where('setting_key', u.setting_key).first();
-      if (existing) {
-        await db('app_settings').where('setting_key', u.setting_key).update({
-          setting_value: u.setting_value, setting_type: u.setting_type, updated_at: new Date(),
-        });
-      } else {
-        // app_settings has no created_at column (see src/database/db.js: only
-        // setting_key/value/type + updated_at). Inserting created_at errors —
-        // which broke first-time keys like the VAT-registration toggle.
-        await db('app_settings').insert({ ...u, updated_at: new Date() });
-      }
+      await upsertAppSetting(u.setting_key, u.setting_value, u.setting_type);
    }
    res.json({ message: 'Accounting settings updated', updated: updates.map((u) => u.setting_key) });
  } catch (error) {
@@ -24,6 +24,19 @@ const { customerAuth } = require('../middleware/customerAuth');
 const { setGalleryAuthCookies } = require('../utils/tokenUtils');
 const customerAccountsService = require('../services/customerAccountsService');

+// Gate a customer-facing route on BOTH the global master flag AND the
+// per-customer override — getEffectiveFeaturesForCustomer combines them, so an
+// admin disabling e.g. Bills globally is honoured even when feature_bills=true
+// on the row. Sends the 403 and returns false on denial; true if allowed.
+async function customerFeatureAllowed(req, res, featureKey, label) {
+  const eff = await customerAccountsService.getEffectiveFeaturesForCustomer(req.customer.id);
+  if (!eff || !eff[featureKey]) {
+    res.status(403).json({ error: `${label} are disabled for this account`, code: 'CUSTOMER_FEATURE_DISABLED' });
+    return false;
+  }
+  return true;
+}
+
 /**
 * Customer-side password policy mirrors the one in customerAuth.js — kept
 * deliberately simple (8 chars, one uppercase, one digit) since a customer
@@ -380,11 +393,8 @@ router.post('/profile/password', [
 router.get('/quotes', customerAuth, async (req, res) => {
  try {
    const { db: dbi } = require('../database/db');
-    // Customer-feature gate. is_active is enforced by customerAuth.
-    const customer = await dbi('customer_accounts').where({ id: req.customer.id }).first();
-    if (!customer || customer.feature_quotes === false || customer.feature_quotes === 0) {
-      return res.status(403).json({ error: 'Quotes are disabled for this account', code: 'CUSTOMER_FEATURE_DISABLED' });
-    }
+    // Customer-feature gate — master flag AND per-customer override.
+    if (!(await customerFeatureAllowed(req, res, 'quotes', 'Quotes'))) return;
    const rows = await dbi('quotes')
      .where({ customer_account_id: req.customer.id })
      // Hide drafts — they're admin scratch work; nothing has been
@@ -459,10 +469,8 @@ router.get('/quotes', customerAuth, async (req, res) => {
 router.get('/invoices', customerAuth, async (req, res) => {
  try {
    const { db: dbi } = require('../database/db');
-    const customer = await dbi('customer_accounts').where({ id: req.customer.id }).first();
-    if (!customer || customer.feature_bills === false || customer.feature_bills === 0) {
-      return res.status(403).json({ error: 'Invoices are disabled for this account', code: 'CUSTOMER_FEATURE_DISABLED' });
-    }
+    // Customer-feature gate — master flag AND per-customer override.
+    if (!(await customerFeatureAllowed(req, res, 'bills', 'Invoices'))) return;
    // Visibility rules for the customer-facing list:
    //   - Hide `scheduled` always (drafts the admin is still tweaking).
    //   - Show `sent`, `overdue`, `paid` always (the customer's
@@ -551,14 +559,8 @@ router.get('/invoices', customerAuth, async (req, res) => {
 router.get('/quotes/:id/pdf', customerAuth, async (req, res) => {
  try {
    const { db: dbi } = require('../database/db');
-    // Feature-gate identically to /quotes (list endpoint). req.customer does
-    // NOT carry feature_* columns (customerAuth only selects identity), so we
-    // read the row here — the previous req.customer.feature_quotes check was a
-    // silent no-op (always undefined).
-    const account = await dbi('customer_accounts').where({ id: req.customer.id }).first();
-    if (!account || account.feature_quotes === false || account.feature_quotes === 0) {
-      return res.status(403).json({ error: 'Quotes are disabled for this account', code: 'CUSTOMER_FEATURE_DISABLED' });
-    }
+    // Feature-gate — master flag AND per-customer override.
+    if (!(await customerFeatureAllowed(req, res, 'quotes', 'Quotes'))) return;
    const quote = await dbi('quotes')
      .where({ id: parseInt(req.params.id, 10), customer_account_id: req.customer.id })
      .first();
@@ -588,12 +590,8 @@ router.get('/quotes/:id/pdf', customerAuth, async (req, res) => {
 router.get('/invoices/:id/pdf', customerAuth, async (req, res) => {
  try {
    const { db: dbi } = require('../database/db');
-    // Feature-gate identically to /invoices (list endpoint) — a direct hit must
-    // not download an invoice PDF when Bills is disabled for the account.
-    const account = await dbi('customer_accounts').where({ id: req.customer.id }).first();
-    if (!account || account.feature_bills === false || account.feature_bills === 0) {
-      return res.status(403).json({ error: 'Invoices are disabled for this account', code: 'CUSTOMER_FEATURE_DISABLED' });
-    }
+    // Feature-gate — master flag AND per-customer override.
+    if (!(await customerFeatureAllowed(req, res, 'bills', 'Invoices'))) return;
    const invoice = await dbi('invoices')
      .where({ id: parseInt(req.params.id, 10), customer_account_id: req.customer.id })
      .first();
@@ -633,12 +631,8 @@ router.get('/contracts', customerAuth, async (req, res) => {
      // Feature not migrated on this install yet.
      return res.json({ contracts: [] });
    }
-    // Per-customer contracts gate (migration 131) — mirrors /quotes + /invoices
-    // so a direct hit is refused when Contracts is off for the account.
-    const account = await dbi('customer_accounts').where({ id: req.customer.id }).first();
-    if (!account || account.feature_contracts === false || account.feature_contracts === 0) {
-      return res.status(403).json({ error: 'Contracts are disabled for this account', code: 'CUSTOMER_FEATURE_DISABLED' });
-    }
+    // Contracts gate — master flag AND per-customer override (migration 131).
+    if (!(await customerFeatureAllowed(req, res, 'contracts', 'Contracts'))) return;
    const rows = await dbi('contracts')
      .where({ customer_account_id: req.customer.id })
      .whereNotIn('status', ['draft'])
@@ -696,10 +690,8 @@ router.get('/contracts/:id/pdf', customerAuth, async (req, res) => {
    if (!(await dbi.schema.hasTable('contracts'))) {
      return res.status(404).json({ error: 'Contract not found' });
    }
-    const account = await dbi('customer_accounts').where({ id: req.customer.id }).first();
-    if (!account || account.feature_contracts === false || account.feature_contracts === 0) {
-      return res.status(403).json({ error: 'Contracts are disabled for this account', code: 'CUSTOMER_FEATURE_DISABLED' });
-    }
+    // Contracts gate — master flag AND per-customer override.
+    if (!(await customerFeatureAllowed(req, res, 'contracts', 'Contracts'))) return;
    const contract = await dbi('contracts')
      .where({ id: parseInt(req.params.id, 10), customer_account_id: req.customer.id })
      .first();
@@ -153,7 +153,10 @@ async function recordInboundDocument({ source, filePath, originalFilename, mimeT
    status: duplicateOfId ? 'duplicate' : 'unsorted',
    parse_status: 'pending',
    parse_method: 'none',
-    page_count: pageCount,
+    // Cap stored page_count to the renderable max (rasterizeService
+    // MAX_RENDERABLE_PAGES) so a hostile high-page PDF can't drive an
+    // unbounded inbox pager (PR #622 concern 6).
+    page_count: pageCount != null ? Math.min(pageCount, 200) : null,
    duplicate_of_id: duplicateOfId,
    created_by_admin_id: adminId || null,
    created_at: now,
@@ -22,6 +22,10 @@ const logger = require('../utils/logger');

 const RENDER_TIMEOUT_MS = 25000;
 const RENDER_DPI = 150;
+// Per-file resource bound (PR #622 concern 6): pages render one-per-request, so
+// a 1000-page hostile PDF could otherwise be walked page-by-page. Refuse to
+// render beyond this — the inbox pager is capped to match.
+const MAX_RENDERABLE_PAGES = 200;

 function renderedDir(docId) {
  return path.join(getStoragePath(), 'business-docs', 'inbound', 'rendered', String(docId));
@@ -41,6 +45,9 @@ function execFileAsync(cmd, args, opts) {
 * @throws AppError 503 when pdftoppm is unavailable, 500 on render failure.
 */
 async function getRenderedPagePath(docId, pdfPath, pageNum) {
+  if (!Number.isInteger(pageNum) || pageNum < 1 || pageNum > MAX_RENDERABLE_PAGES) {
+    throw new AppError(`Page out of range (1–${MAX_RENDERABLE_PAGES})`, 400, 'PAGE_OUT_OF_RANGE');
+  }
  const dir = renderedDir(docId);
  const outPng = path.join(dir, `page-${pageNum}.png`);
  if (fs.existsSync(outPng)) return outPng;
@@ -502,10 +502,14 @@ async function getTaxReport({ from, to, currency, includeCosts = true } = {}) {
    // business doesn't file VAT (payable = 0); when registered it's output VAT
    // minus the RECLAIMABLE input VAT only (foreign non-reclaimable cost VAT is
    // not deducted). Guideline figure — verify with your Treuhänder.
-    let vatRegistered = await getVatRegisteredSetting();
-    // Unset → preserve prior behaviour: if the business charged output VAT this
-    // period it's effectively registered; otherwise treat as small-business.
-    if (vatRegistered === null) vatRegistered = grandTotalVat > 0;
+    // PR #622 concern 4: when VAT registration is UNSET we must NOT guess from
+    // `grandTotalVat > 0` — a quarter with all-exempt cross-border sales has zero
+    // output VAT and would silently flip to "not registered", hiding the reclaim.
+    // Treat null as "not configured": refuse to compute a payable, surface a
+    // warning in the UI instead.
+    const vatRegisteredSetting = await getVatRegisteredSetting();
+    const vatRegistrationConfigured = vatRegisteredSetting !== null;
+    const vatRegistered = vatRegisteredSetting === true;
    const reclaimableInputVat = costs.reclaimableVat != null ? costs.reclaimableVat : costs.totalVat;

    // Summary: income vs cost vs result. Result = a simplified
@@ -520,7 +524,12 @@ async function getTaxReport({ from, to, currency, includeCosts = true } = {}) {
      resultNetMinor: grandTotalNet - costs.totalNet,
      resultGrossMinor: grandTotal - costs.totalGross,
      vatRegistered,
-      vatPayableMinor: vatRegistered ? (grandTotalVat - reclaimableInputVat) : 0,
+      vatRegistrationConfigured,
+      // null (not 0) when registration is unconfigured — the UI renders "—" + a
+      // "configure VAT registration" warning rather than a misleading number.
+      vatPayableMinor: !vatRegistrationConfigured
+        ? null
+        : (vatRegistered ? (grandTotalVat - reclaimableInputVat) : 0),
    };

    // Unified ledger (#5 — one typed, signed, sortable list). Outgoing
@@ -35,4 +35,22 @@ async function getAppSetting(key, defaultValue = null) {
  }
 }

-module.exports = { getAppSetting };
+/**
+ * Schema-correct upsert into app_settings. The table has NO `created_at`
+ * column (only setting_key/setting_value/setting_type + updated_at — see
+ * src/database/db.js), so inserting created_at throws and silently breaks the
+ * FIRST save of any new key. Centralised here so route authors can't
+ * re-introduce that bug (PR #622 concern 5). `setting_value` is expected to be
+ * already JSON-stringified, matching getAppSetting's JSON.parse on read.
+ */
+async function upsertAppSetting(setting_key, setting_value, setting_type, conn = db) {
+  const existing = await conn('app_settings').where({ setting_key }).first();
+  if (existing) {
+    await conn('app_settings').where({ setting_key })
+      .update({ setting_value, setting_type, updated_at: new Date() });
+  } else {
+    await conn('app_settings').insert({ setting_key, setting_value, setting_type, updated_at: new Date() });
+  }
+}
+
+module.exports = { getAppSetting, upsertAppSetting };