From 32d745b57507915e8c8674ae3b56ff8d950e4712 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 5 Sep 2026 23:16:48 +0200 Subject: [PATCH] fix(usage): stop local backups implying S3 use, and make the protocol-error branch reachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the review of the current head. Local backups no longer imply S3. markUsed derived an s3_storage marker from "a backup ran while backup_destination_type is s3" — but the middleware also counts /database-backup/* and /backup/picpeak/export as backups, and those write a local file wherever scheduled backups go. So configuring S3 and downloading a local export reported s3_storage as USED. The middleware now tells markUsed whether the operation writes to the configured destination, and only then is the marker derived. A wrong `true` in this dataset is worse than a missing signal: it is a claim about an install that nobody can check. The ProtocolError branch was dead code. adminUsage matched on `error.name === 'ProtocolError'`, but the class extends Error without setting `name`, so every instance reports 'Error' — verified — and a malformed vote or feedback payload fell through to the global handler, which logs it as an unhandled programming error and answers INTERNAL_ERROR in production, losing the validation code the caller needs. Now matched with instanceof. protocol.cjs is byte-identical with picpeak-usage (diffed against the companion repo), so the fix belongs here rather than in the class. An existing assertion needed updating for the new markUsed argument, and the path split is pinned: /backup/run is destination-driven, /database-backup/backup and /backup/picpeak/export are not. Refs #1110 --- backend/__tests__/routes/adminUsage.test.js | 27 +++++++++++- .../services/usageSnapshotSignals.test.js | 42 +++++++++++++++++++ backend/src/middleware/productUsage.js | 12 +++++- backend/src/routes/adminUsage.js | 10 ++++- backend/src/usage/UsageService.js | 9 +++- 5 files changed, 94 insertions(+), 6 deletions(-) diff --git a/backend/__tests__/routes/adminUsage.test.js b/backend/__tests__/routes/adminUsage.test.js index 75fba5f0..187f4d4b 100644 --- a/backend/__tests__/routes/adminUsage.test.js +++ b/backend/__tests__/routes/adminUsage.test.js @@ -171,8 +171,33 @@ test('public/gallery paths and failed/unauthenticated admin operations never set simulate('/quotes', { id: 1 }, 403); expect(service.markUsed).not.toHaveBeenCalled(); simulate('/customers/42/hour-entries', { id: 1 }, 200); + // The second argument tells markUsed whether this operation writes to the + // configured backup destination; a CRM route never does. expect(service.markUsed).toHaveBeenCalledWith( - expect.arrayContaining(['crm', 'crm_hours']) + expect.arrayContaining(['crm', 'crm_hours']), + expect.objectContaining({ destinationBackup: false }) ); expect(JSON.stringify(service.markUsed.mock.calls)).not.toContain('42'); }); + +test('only a backup that writes to the configured destination flags S3', () => { + // /database-backup/* and /backup/picpeak/export produce a local file, so + // they must not imply S3 use just because S3 is the configured destination. + const seen = []; + const simulate = (pathname) => { + service.markUsed.mockClear(); + const res = new (require('events').EventEmitter)(); + res.statusCode = 200; + productUsage({ path: pathname, admin: { id: 1 } }, res, () => {}); + res.emit('finish'); + seen.push([pathname, service.markUsed.mock.calls[0]?.[1]?.destinationBackup]); + }; + simulate('/backup/run'); + simulate('/database-backup/backup'); + simulate('/backup/picpeak/export'); + expect(seen).toEqual([ + ['/backup/run', true], + ['/database-backup/backup', false], + ['/backup/picpeak/export', false], + ]); +}); diff --git a/backend/__tests__/services/usageSnapshotSignals.test.js b/backend/__tests__/services/usageSnapshotSignals.test.js index b31efcf4..c575bdf1 100644 --- a/backend/__tests__/services/usageSnapshotSignals.test.js +++ b/backend/__tests__/services/usageSnapshotSignals.test.js @@ -150,3 +150,45 @@ describe('status survives a misconfigured collector URL', () => { expect(status.collector_url).toBe('https://usage.picpeak.app'); }); }); + +describe('S3 use is only implied by backups that write to the destination', () => { + let db; + afterEach(async () => { if (db) await db.destroy(); db = null; }); + + const withS3Destination = async (database) => { + await database('app_settings').insert({ + setting_key: 'backup_destination_type', + setting_value: JSON.stringify('s3'), + }); + await database('product_usage_state').where({ id: 1 }).update({ status: 'active' }); + }; + + it('marks S3 for a backup that uses the configured destination', async () => { + db = await bootDb(); + await withS3Destination(db); + await service(db).markUsed(['backup'], { destinationBackup: true }); + expect((await db('product_usage_markers').pluck('feature')).sort()) + .toEqual(['backup', 's3_storage']); + }); + + it('does NOT mark S3 for a local backup, even with S3 configured', async () => { + // /database-backup/* and /backup/picpeak/export produce a local file. They + // count as `backup`, but claiming S3 was used for them made merely + // configuring S3 and downloading an export report s3_storage.used. + db = await bootDb(); + await withS3Destination(db); + await service(db).markUsed(['backup']); + expect(await db('product_usage_markers').pluck('feature')).toEqual(['backup']); + }); + + it('does not mark S3 when the destination is not S3', async () => { + db = await bootDb(); + await db('app_settings').insert({ + setting_key: 'backup_destination_type', + setting_value: JSON.stringify('local'), + }); + await db('product_usage_state').where({ id: 1 }).update({ status: 'active' }); + await service(db).markUsed(['backup'], { destinationBackup: true }); + expect(await db('product_usage_markers').pluck('feature')).toEqual(['backup']); + }); +}); diff --git a/backend/src/middleware/productUsage.js b/backend/src/middleware/productUsage.js index 7bbd7eb7..a8d41c14 100644 --- a/backend/src/middleware/productUsage.js +++ b/backend/src/middleware/productUsage.js @@ -28,6 +28,12 @@ const RULES = [ [/^\/email\/(?:test|send)(?:\/|$)/, ['smtp']], [/^\/external-media(?:\/|$)/, ['share_mounts']] ]; +// Backup operations that write to the CONFIGURED destination, and so imply +// S3 use when that destination is S3. Deliberately excludes +// /backup/picpeak/export and everything under /database-backup/, which +// produce a local file regardless of where scheduled backups go. +const DESTINATION_BACKUP = /^\/backup\/(?:run|backup|create|start|test)(?:\/|$)/; + function productUsage(req, res, next) { const pathname = req.path; res.once('finish', () => { @@ -42,9 +48,11 @@ function productUsage(req, res, next) { features.push('s3_storage'); if (features.length) service - .markUsed(features) + .markUsed(features, { + destinationBackup: DESTINATION_BACKUP.test(pathname) + }) .catch(() => logger.warn('Product usage marker could not be recorded')); }); next(); } -module.exports = { productUsage, RULES }; +module.exports = { productUsage, RULES, DESTINATION_BACKUP }; diff --git a/backend/src/routes/adminUsage.js b/backend/src/routes/adminUsage.js index 20cb1b38..9506dddc 100644 --- a/backend/src/routes/adminUsage.js +++ b/backend/src/routes/adminUsage.js @@ -4,10 +4,18 @@ const { adminAuth } = require('../middleware/auth'); const { requirePermission } = require('../middleware/permissions'); const { ValidationError } = require('../utils/errors'); const service = require('../services/productUsageService'); +const { ProtocolError } = require('../usage/protocol.cjs'); const router = express.Router(); const wrap = (fn) => (req, res, next) => Promise.resolve(fn(req, res)).catch((error) => { - if (error.name === 'ProtocolError') + // instanceof, not `error.name`: ProtocolError extends Error without + // setting `name`, so every instance reports 'Error' and this branch never + // ran. A malformed vote or feedback payload fell through to the global + // handler, which logs it as an unhandled programming error and answers + // INTERNAL_ERROR in production — losing the validation code the caller + // needs. protocol.cjs is vendored byte-identical with picpeak-usage, so + // the fix belongs here rather than in the class. + if (error instanceof ProtocolError) return res .status(400) .json({ error: 'Invalid usage request', code: error.code }); diff --git a/backend/src/usage/UsageService.js b/backend/src/usage/UsageService.js index d34568b4..c87584e7 100644 --- a/backend/src/usage/UsageService.js +++ b/backend/src/usage/UsageService.js @@ -572,7 +572,7 @@ class UsageService { return this.status(); } - async markUsed(features) { + async markUsed(features, { destinationBackup = false } = {}) { const allowed = [...new Set(features)].filter((f) => FEATURE_KEYS.includes(f) ); @@ -583,7 +583,12 @@ class UsageService { if (this.db.client.config.client === 'pg') query.forUpdate(); const state = await query.first(); if (!state || state.status !== 'active') return; - if (allowed.includes('backup')) { + // Only when the operation actually writes to the configured backup + // destination. Deriving this from "a backup ran while S3 is configured" + // marked S3 as USED for a local database backup or a .picpeak export, + // which the middleware also counts as `backup` — so merely configuring + // S3 and downloading a local export claimed S3 was in use. + if (destinationBackup && allowed.includes('backup')) { const destination = await tx('app_settings') .where({ setting_key: 'backup_destination_type' }) .first();