From 4007709590a2b2d54bdc1102e7f20a72655a7ff4 Mon Sep 17 00:00:00 2001 From: Anso Date: Thu, 7 May 2026 13:41:10 -0400 Subject: [PATCH] fix(fleet-sync): hygiene pass on receiver behavior and cleanup (#972) A bundle of small file-local fixes to the receiver path and node-deletion flow. Changes: - F4 receiver audit log: applyIncomingSync now writes a system audit entry on every applied push so mirrored security-rule changes show up in the replica's audit panel with a clear control-side origin. - F7 pilot-agent skip: pushResource explicitly excludes pilot-agent nodes (they have no api_url for HTTP push) and warns once per node id so the operator sees they will not receive replicated policies. - B4 identity-drift notification: when targetIdentity differs from the cached fleet_self_identity, dispatch a warning so the operator can audit any identity-scoped policies that may need re-targeting. - B6 stack_pattern ReDoS guard: reject patterns with 4+ consecutive wildcards or more than 8 wildcards total. Both control-side validators (POST/PUT scan policies) and the receiver-side row validator share the helper. - B9 deleteNode cascade: clear fleet_sync_status rows for the node inside the existing transaction so the sync-status panel does not render ghost entries after a node is removed. - S6 last_error redaction: formatError strips Bearer tokens and JWT-shaped values from error messages and caps at 500 chars before storing in fleet_sync_status.last_error or logging. Tests: - 8 new vitest cases covering audit-log entry, identity-drift alert, pilot-agent warn-once, formatError redaction (Bearer + JWT), ReDoS validator rejection, and a backtracking-time smoke test. - New database-fleet-sync-cascade.test.ts: deleteNode removes fleet_sync_status rows for the deleted node and leaves siblings untouched. - Full backend suite: 1792 pass / 5 skipped. --- .../database-fleet-sync-cascade.test.ts | 60 ++++++++++++ .../src/__tests__/fleet-sync-routes.test.ts | 41 ++++++++ .../src/__tests__/fleet-sync-service.test.ts | 93 +++++++++++++++++++ backend/src/routes/fleet.ts | 23 ++++- backend/src/routes/security.ts | 21 ++++- backend/src/services/DatabaseService.ts | 1 + backend/src/services/FleetSyncService.ts | 72 +++++++++++++- 7 files changed, 303 insertions(+), 8 deletions(-) create mode 100644 backend/src/__tests__/database-fleet-sync-cascade.test.ts diff --git a/backend/src/__tests__/database-fleet-sync-cascade.test.ts b/backend/src/__tests__/database-fleet-sync-cascade.test.ts new file mode 100644 index 00000000..4dac5a79 --- /dev/null +++ b/backend/src/__tests__/database-fleet-sync-cascade.test.ts @@ -0,0 +1,60 @@ +/** + * Pins the fleet_sync_status cascade behavior of DatabaseService.deleteNode. + * + * Without this cleanup, deleting a node from Settings → Nodes leaves orphaned + * sync-status rows behind that the UI then renders as ghost entries. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { setupTestDb, cleanupTestDb } from './helpers/setupTestDb'; + +let tmpDir: string; +let DatabaseService: typeof import('../services/DatabaseService').DatabaseService; + +beforeAll(async () => { + tmpDir = await setupTestDb(); + ({ DatabaseService } = await import('../services/DatabaseService')); +}); + +afterAll(() => { + cleanupTestDb(tmpDir); +}); + +describe('deleteNode fleet_sync_status cascade', () => { + it('removes fleet_sync_status rows for the deleted node', () => { + const db = DatabaseService.getInstance(); + const nodeId = db.addNode({ + name: 'cascade-target', + type: 'remote', + compose_dir: '/app/compose', + is_default: false, + api_url: 'https://cascade.example', + api_token: 'tok', + mode: 'proxy', + }); + + // Sibling node so we can confirm its rows survive. + const siblingId = db.addNode({ + name: 'cascade-sibling', + type: 'remote', + compose_dir: '/app/compose', + is_default: false, + api_url: 'https://sibling.example', + api_token: 'tok', + mode: 'proxy', + }); + + db.recordFleetSyncFailure(nodeId, 'scan_policies', 'timeout'); + db.recordFleetSyncFailure(nodeId, 'cve_suppressions', 'timeout'); + db.recordFleetSyncSuccess(siblingId, 'scan_policies'); + + const before = db.getFleetSyncStatuses(); + expect(before.filter((s) => s.node_id === nodeId)).toHaveLength(2); + expect(before.filter((s) => s.node_id === siblingId)).toHaveLength(1); + + db.deleteNode(nodeId); + + const after = db.getFleetSyncStatuses(); + expect(after.filter((s) => s.node_id === nodeId)).toHaveLength(0); + expect(after.filter((s) => s.node_id === siblingId)).toHaveLength(1); + }); +}); diff --git a/backend/src/__tests__/fleet-sync-routes.test.ts b/backend/src/__tests__/fleet-sync-routes.test.ts index 0e43cf0a..d5e037ef 100644 --- a/backend/src/__tests__/fleet-sync-routes.test.ts +++ b/backend/src/__tests__/fleet-sync-routes.test.ts @@ -274,6 +274,47 @@ describe('POST /api/fleet/role/reanchor', () => { }); }); +describe('POST /api/fleet/sync/:resource stack_pattern ReDoS guard', () => { + it('rejects 4+ consecutive wildcards in a stack_pattern', async () => { + const res = await request(app) + .post('/api/fleet/sync/scan_policies') + .set('Authorization', nodeProxyAuthHeader) + .send({ + rows: [{ + name: 'redos', node_identity: '', stack_pattern: 'foo****bar', + max_severity: 'CRITICAL', block_on_deploy: 0, enabled: 1, + }], + }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/4\+ consecutive wildcards/); + }); + + it('rejects more than 8 wildcards anywhere in a stack_pattern', async () => { + const res = await request(app) + .post('/api/fleet/sync/scan_policies') + .set('Authorization', nodeProxyAuthHeader) + .send({ + rows: [{ + name: 'too-many-stars', node_identity: '', + stack_pattern: '*a*b*c*d*e*f*g*h*i*', + max_severity: 'CRITICAL', block_on_deploy: 0, enabled: 1, + }], + }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/too many wildcards/); + }); + + it('regex compiled from a max-allowed pattern matches in under 50ms on a worst-case input', () => { + const pattern = 'a*b*c*d*e*f*g*h'; // 7 stars, allowed. + const escaped = pattern.replace(/[.+?^${}()|[\]\\]/g, '\\$&').replace(/\*/g, '.*'); + const re = new RegExp('^' + escaped + '$'); + const target = 'a' + 'a'.repeat(2000); + const start = Date.now(); + re.test(target); + expect(Date.now() - start).toBeLessThan(50); + }); +}); + describe('POST /api/fleet/role/demote', () => { it('returns 401 without auth', async () => { const res = await request(app).post('/api/fleet/role/demote').send({ confirm: true }); diff --git a/backend/src/__tests__/fleet-sync-service.test.ts b/backend/src/__tests__/fleet-sync-service.test.ts index eae8ebc1..1615fe86 100644 --- a/backend/src/__tests__/fleet-sync-service.test.ts +++ b/backend/src/__tests__/fleet-sync-service.test.ts @@ -13,6 +13,7 @@ const { mockReplaceReplicatedCveSuppressions, mockClearOrphanPolicyEvaluations, mockClearReplicatedRows, + mockInsertAuditLog, mockRecordFleetSyncSuccess, mockRecordFleetSyncFailure, mockGetSystemState, @@ -29,6 +30,7 @@ const { mockReplaceReplicatedCveSuppressions: vi.fn(), mockClearOrphanPolicyEvaluations: vi.fn(), mockClearReplicatedRows: vi.fn(), + mockInsertAuditLog: vi.fn(), mockRecordFleetSyncSuccess: vi.fn(), mockRecordFleetSyncFailure: vi.fn(), mockGetSystemState: vi.fn().mockReturnValue(null), @@ -48,6 +50,7 @@ vi.mock('../services/DatabaseService', () => ({ replaceReplicatedCveSuppressions: mockReplaceReplicatedCveSuppressions, clearOrphanPolicyEvaluations: mockClearOrphanPolicyEvaluations, clearReplicatedRows: mockClearReplicatedRows, + insertAuditLog: mockInsertAuditLog, recordFleetSyncSuccess: mockRecordFleetSyncSuccess, recordFleetSyncFailure: mockRecordFleetSyncFailure, getSystemState: mockGetSystemState, @@ -501,3 +504,93 @@ describe('FleetSyncService.getControlIdentity', () => { expect(FleetSyncService.getControlIdentity()).toBe(''); }); }); + +describe('FleetSyncService incoming-sync side effects', () => { + it('writes a system audit log entry on every applied sync', () => { + FleetSyncService.getInstance().applyIncomingSync( + 'scan_policies', + [], + 'https://me.example', + 1_700_000_000_000, + 'fingerprint-aaa', + ); + expect(mockInsertAuditLog).toHaveBeenCalledWith(expect.objectContaining({ + username: 'system', + method: 'POST', + path: '/api/fleet/sync/scan_policies', + ip_address: 'control', + summary: expect.stringContaining('Replicated scan_policies from fingerprint-aaa'), + })); + }); + + it('emits an identity-drift notification when the targetIdentity changes', () => { + mockGetSystemState.mockImplementation((key: string) => { + if (key === 'fleet_self_identity') return 'https://old.example'; + return null; + }); + FleetSyncService.getInstance().applyIncomingSync( + 'scan_policies', + [], + 'https://new.example', + ); + expect(mockDispatchAlert).toHaveBeenCalledWith( + 'warning', + 'system', + expect.stringContaining('Fleet self-identity changed'), + ); + }); + + it('does not emit identity-drift when there is no prior identity (first sync)', () => { + mockGetSystemState.mockImplementation(() => null); + FleetSyncService.getInstance().applyIncomingSync( + 'scan_policies', + [], + 'https://first.example', + ); + const driftCalls = mockDispatchAlert.mock.calls.filter((c) => + typeof c[2] === 'string' && c[2].includes('self-identity changed'), + ); + expect(driftCalls).toHaveLength(0); + }); +}); + +describe('FleetSyncService.pushResource pilot-agent skip', () => { + it('skips pilot-agent nodes and logs warn-once per node id', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + mockGetNodes.mockReturnValue([ + { id: 50, type: 'remote', mode: 'pilot_agent', api_url: '', api_token: '', name: 'PilotNode' }, + ]); + await FleetSyncService.getInstance().pushResource('scan_policies'); + await FleetSyncService.getInstance().pushResource('scan_policies'); + const pilotWarns = warnSpy.mock.calls.filter((c) => + typeof c[0] === 'string' && c[0].includes('pilot-agent') && c[0].includes('PilotNode'), + ); + expect(pilotWarns).toHaveLength(1); + expect(mockAxiosPost).not.toHaveBeenCalled(); + warnSpy.mockRestore(); + }); +}); + +describe('FleetSyncService.formatError redaction', () => { + it('redacts Bearer tokens and JWT-shaped values from error messages', async () => { + mockGetNodes.mockReturnValue([ + { id: 51, type: 'remote', api_url: 'https://leak.example', api_token: 'tok', name: 'leak' }, + ]); + mockGetLocalScanPolicies.mockReturnValue([]); + mockAxiosPost.mockImplementation(async () => { + const { AxiosError } = await import('axios'); + const err = new AxiosError('Request failed'); + (err as unknown as { response: unknown }).response = { + status: 500, + statusText: 'Server Error', + data: { error: 'Authorization: Bearer abc.def-_~+/=secrettoken denied; jwt eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiJ1c2VyIn0.signaturePart' }, + }; + throw err; + }); + await FleetSyncService.getInstance().pushResource('scan_policies'); + const failure = mockRecordFleetSyncFailure.mock.calls[0]; + expect(failure[2]).not.toMatch(/Bearer\s+[A-Za-z0-9]/); + expect(failure[2]).toContain('[redacted]'); + expect(failure[2]).toContain('[redacted-jwt]'); + }); +}); diff --git a/backend/src/routes/fleet.ts b/backend/src/routes/fleet.ts index 3993985b..45530383 100644 --- a/backend/src/routes/fleet.ts +++ b/backend/src/routes/fleet.ts @@ -48,7 +48,10 @@ function validateScanPolicyRow(row: unknown): string | null { if (typeof r.name !== 'string' || r.name.length === 0 || r.name.length > 200) return 'name must be a non-empty string'; if (typeof r.max_severity !== 'string' || !POLICY_SEVERITIES.has(r.max_severity)) return 'max_severity must be CRITICAL, HIGH, MEDIUM, or LOW'; if (r.stack_pattern !== null && typeof r.stack_pattern !== 'string') return 'stack_pattern must be a string or null'; - if (typeof r.stack_pattern === 'string' && r.stack_pattern.length > 200) return 'stack_pattern is too long'; + if (typeof r.stack_pattern === 'string') { + const patternError = validateStackPatternForRedos(r.stack_pattern); + if (patternError) return patternError; + } if (typeof r.node_identity !== 'string') return 'node_identity must be a string'; if (r.node_identity.length > 500) return 'node_identity is too long'; if (!isIntFlag(r.block_on_deploy)) return 'block_on_deploy must be 0 or 1'; @@ -56,6 +59,24 @@ function validateScanPolicyRow(row: unknown): string | null { return null; } +/** + * Reject `stack_pattern` inputs that would compile to a backtracking-prone + * regex. The matcher in `getMatchingPolicy` substitutes `*` with `.*`, so a + * pattern like `***...` becomes a chain of adjacent `.*` runs that exhibit + * catastrophic backtracking on long inputs. + * + * Caps mirror the limit in routes/security.ts so a control creating a policy + * sees the same error as a replica receiving one. Length is gated at 200 by + * the surrounding row validator. + */ +export function validateStackPatternForRedos(pattern: string): string | null { + if (pattern.length > 200) return 'stack_pattern is too long'; + const stars = (pattern.match(/\*/g) ?? []).length; + if (stars > 8) return 'stack_pattern has too many wildcards (max 8)'; + if (/\*{4,}/.test(pattern)) return 'stack_pattern must not contain 4+ consecutive wildcards'; + return null; +} + function validateCveSuppressionRow(row: unknown): string | null { if (!row || typeof row !== 'object') return 'row must be an object'; const r = row as Record; diff --git a/backend/src/routes/security.ts b/backend/src/routes/security.ts index 413c2c3f..94468b44 100644 --- a/backend/src/routes/security.ts +++ b/backend/src/routes/security.ts @@ -14,6 +14,7 @@ import { sanitizeForLog } from '../utils/safeLog'; import { getErrorMessage } from '../utils/errors'; import { isDebugEnabled } from '../utils/debug'; import { blockIfReplica } from '../middleware/fleetSyncGuards'; +import { validateStackPatternForRedos } from './fleet'; import { FINDING_SEVERITIES, POLICY_SEVERITIES } from '../utils/severity'; const CVE_ID_RE = /^(CVE-\d{4}-\d{4,}|GHSA-[\w-]{14,})$/; @@ -423,13 +424,20 @@ securityRouter.post('/policies', authMiddleware, (req: Request, res: Response): if (!POLICY_SEVERITIES.has(max_severity)) { res.status(400).json({ error: 'max_severity must be CRITICAL, HIGH, MEDIUM, or LOW' }); return; } + const normalizedPattern = stack_pattern ? String(stack_pattern) : null; + if (normalizedPattern !== null) { + const patternError = validateStackPatternForRedos(normalizedPattern); + if (patternError) { + res.status(400).json({ error: patternError }); return; + } + } try { const resolvedNodeId = node_id != null ? Number(node_id) : null; const policy = DatabaseService.getInstance().createScanPolicy({ name: name.trim(), node_id: resolvedNodeId, node_identity: FleetSyncService.resolveIdentityForNodeId(resolvedNodeId), - stack_pattern: stack_pattern ? String(stack_pattern) : null, + stack_pattern: normalizedPattern, max_severity, block_on_deploy: block_on_deploy ? 1 : 0, enabled: enabled === false ? 0 : 1, @@ -459,7 +467,16 @@ securityRouter.put('/policies/:id', authMiddleware, (req: Request, res: Response updates.node_id = resolvedNodeId; updates.node_identity = FleetSyncService.resolveIdentityForNodeId(resolvedNodeId); } - if (body.stack_pattern !== undefined) updates.stack_pattern = body.stack_pattern ? String(body.stack_pattern) : null; + if (body.stack_pattern !== undefined) { + const normalizedPattern = body.stack_pattern ? String(body.stack_pattern) : null; + if (normalizedPattern !== null) { + const patternError = validateStackPatternForRedos(normalizedPattern); + if (patternError) { + res.status(400).json({ error: patternError }); return; + } + } + updates.stack_pattern = normalizedPattern; + } if (body.max_severity !== undefined) { if (!POLICY_SEVERITIES.has(body.max_severity)) { res.status(400).json({ error: 'max_severity must be CRITICAL, HIGH, MEDIUM, or LOW' }); return; diff --git a/backend/src/services/DatabaseService.ts b/backend/src/services/DatabaseService.ts index 134978ac..77eaeed8 100644 --- a/backend/src/services/DatabaseService.ts +++ b/backend/src/services/DatabaseService.ts @@ -2097,6 +2097,7 @@ export class DatabaseService { this.db.prepare('DELETE FROM stack_labels WHERE node_id = ?').run(id); this.db.prepare('UPDATE blueprints SET pinned_node_id = NULL WHERE pinned_node_id = ?').run(id); this.deleteRoleAssignmentsByResource('node', String(id)); + this.db.prepare('DELETE FROM fleet_sync_status WHERE node_id = ?').run(id); this.db.prepare('DELETE FROM nodes WHERE id = ?').run(id); })(); } diff --git a/backend/src/services/FleetSyncService.ts b/backend/src/services/FleetSyncService.ts index a8be203b..279b45c4 100644 --- a/backend/src/services/FleetSyncService.ts +++ b/backend/src/services/FleetSyncService.ts @@ -175,6 +175,9 @@ export class FleetSyncService { private static cachedControlIdentity: string | null = null; + /** Node ids already warned about as pilot-agent skip candidates. */ + private static readonly warnedPilotAgents = new Set(); + private nextPushedAt(): number { const now = Date.now(); const next = now > FleetSyncService.lastPushedAt ? now : FleetSyncService.lastPushedAt + 1; @@ -191,8 +194,21 @@ export class FleetSyncService { return; } const db = DatabaseService.getInstance(); - const nodes = db.getNodes().filter((n): n is Node & { id: number } => { - return n.type === 'remote' && Boolean(n.api_url) && Boolean(n.api_token) && n.id != null; + const allNodes = db.getNodes(); + // Pilot-agent nodes do not accept fleet-sync HTTP pushes; security + // rules for them are out of scope until pilot tunneling is built. Warn + // once per node id so the operator knows their pilot remote will not + // receive replicated policies. + for (const n of allNodes) { + if (n.mode === 'pilot_agent' && n.id != null && !FleetSyncService.warnedPilotAgents.has(n.id)) { + console.warn( + `[FleetSync] Skipping pilot-agent node "${n.name}" (id=${n.id}); fleet sync over pilot tunnel is out of scope.`, + ); + FleetSyncService.warnedPilotAgents.add(n.id); + } + } + const nodes = allNodes.filter((n): n is Node & { id: number } => { + return n.type === 'remote' && n.mode !== 'pilot_agent' && Boolean(n.api_url) && Boolean(n.api_token) && n.id != null; }); if (nodes.length === 0) { if (isDebugEnabled()) { @@ -299,6 +315,21 @@ export class FleetSyncService { } db.setSystemState(SYNC_STATE_KEYS.fleetRole, 'replica'); if (targetIdentity) { + const cachedSelf = db.getSystemState(SYNC_STATE_KEYS.fleetSelfIdentity); + if (cachedSelf && cachedSelf !== targetIdentity) { + // Operator changed how the control sees this node (e.g. an + // api_url switch). Notify so they can audit any + // identity-scoped policies that may need re-targeting. + void NotificationService.getInstance() + .dispatchAlert( + 'warning', + 'system', + `Fleet self-identity changed from "${cachedSelf}" to "${targetIdentity}". Identity-scoped policies are reapplied on the next sync.`, + ) + .catch((err) => { + console.warn('[FleetSync] Failed to dispatch identity-drift alert:', err); + }); + } db.setSystemState(SYNC_STATE_KEYS.fleetSelfIdentity, targetIdentity); } if (resource === 'scan_policies') { @@ -306,6 +337,20 @@ export class FleetSyncService { } else if (resource === 'cve_suppressions') { db.replaceReplicatedCveSuppressions(rows as Array>); } + // F4: persist an audit-log entry for the operator on the replica + // side. Without this, mirrored security-rule changes happen + // silently from the replica's perspective. Username 'system' and + // ip 'control' make the source unambiguous in the audit panel. + db.insertAuditLog({ + timestamp: Date.now(), + username: 'system', + method: 'POST', + path: `/api/fleet/sync/${resource}`, + status_code: 200, + node_id: 0, + ip_address: 'control', + summary: `Replicated ${resource} from ${controlIdentity || 'legacy control'}: replaced ${rows.length} row(s)`, + }); }); } @@ -504,16 +549,33 @@ export class FleetSyncService { } private formatError(err: unknown): string { + let raw: string; if (err instanceof AxiosError) { if (err.response) { const data = err.response.data; const detail = typeof data === 'object' && data && 'error' in data ? String((data as { error: unknown }).error) : err.response.statusText; - return `HTTP ${err.response.status}: ${detail}`; + raw = `HTTP ${err.response.status}: ${detail}`; + } else { + raw = err.message; } - return err.message; + } else { + raw = err instanceof Error ? err.message : String(err); } - return err instanceof Error ? err.message : String(err); + return FleetSyncService.redactSensitive(raw); + } + + /** + * Strip Bearer tokens and JWT-shaped substrings from an error message + * before it lands on disk in `fleet_sync_status.last_error` (visible in the + * UI sync-status panel) or in any log line. Caps the result at 500 chars + * to bound storage growth on pathological remote responses. + */ + private static redactSensitive(message: string): string { + const redacted = message + .replace(/Bearer\s+[A-Za-z0-9\-._~+/=]+/gi, 'Bearer [redacted]') + .replace(/[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}/g, '[redacted-jwt]'); + return redacted.length > 500 ? redacted.slice(0, 497) + '...' : redacted; } }