diff --git a/dashcaddy-api/__tests__/csrf-protection.test.js b/dashcaddy-api/__tests__/csrf-protection.test.js index b039f57..022e9ad 100644 --- a/dashcaddy-api/__tests__/csrf-protection.test.js +++ b/dashcaddy-api/__tests__/csrf-protection.test.js @@ -322,6 +322,89 @@ describe('CSRF Protection', () => { process.env.NODE_ENV = origEnv; }); + + // DC-058: differentiate "browser auto-retry" from "real probe" by the + // presence of the X-CSRF-Token header. The 403 response is identical in + // both branches; only the stderr log tag changes. + describe('DC-058: browser-auto-retry vs real-probe log tagging', () => { + let stderrSpy; + let origEnv; + + beforeEach(() => { + origEnv = process.env.NODE_ENV; + process.env.NODE_ENV = 'production'; + stderrSpy = jest.spyOn(process.stderr, 'write').mockImplementation(() => true); + }); + + afterEach(() => { + process.env.NODE_ENV = origEnv; + stderrSpy.mockRestore(); + }); + + it('tags missing-cookie with [CSRF-debug] when X-CSRF-Token header also present (browser auto-retry)', () => { + const { req, res, next } = createMockReqRes({ + method: 'POST', path: '/api/v1/backups/schedule', + headers: { cookie: '', 'x-csrf-token': 'some-signature-attempt' } + }); + csrfValidationMiddleware(req, res, next); + + // 403 response unchanged + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ error: expect.stringContaining('DC-100') }) + ); + // Log tag is [CSRF-debug] + expect(stderrSpy).toHaveBeenCalled(); + const lastWrite = stderrSpy.mock.calls[stderrSpy.mock.calls.length - 1][0]; + expect(lastWrite).toContain('[CSRF-debug]'); + expect(lastWrite).toContain('browser auto-retry'); + expect(lastWrite).not.toMatch(/^\[CSRF\][^-]/); // not bare [CSRF] + }); + + it('tags missing-cookie with [CSRF] when no X-CSRF-Token header present (real probe)', () => { + const { req, res, next } = createMockReqRes({ + method: 'POST', path: '/api/v1/backups/schedule', + headers: { cookie: '' } + }); + csrfValidationMiddleware(req, res, next); + + expect(res.status).toHaveBeenCalledWith(403); + expect(stderrSpy).toHaveBeenCalled(); + const lastWrite = stderrSpy.mock.calls[stderrSpy.mock.calls.length - 1][0]; + expect(lastWrite).toContain('[CSRF]'); + expect(lastWrite).not.toContain('[CSRF-debug]'); + expect(lastWrite).not.toContain('browser auto-retry'); + }); + + it('keeps [CSRF] tag when cookie present but header missing (curl probe with manual cookie)', () => { + const nonce = generateToken(); + const { req, res, next } = createMockReqRes({ + method: 'POST', path: '/api/v1/backups/schedule', + headers: { cookie: `${CSRF_COOKIE_NAME}=${nonce}` } + }); + csrfValidationMiddleware(req, res, next); + + expect(res.status).toHaveBeenCalledWith(403); + expect(stderrSpy).toHaveBeenCalled(); + const lastWrite = stderrSpy.mock.calls[stderrSpy.mock.calls.length - 1][0]; + expect(lastWrite).toContain('[CSRF]'); + expect(lastWrite).not.toContain('[CSRF-debug]'); + }); + + it('headerToken is read from x-csrf-token (lowercased by Express) — lowercase header triggers [CSRF-debug]', () => { + // Express/Node lowercases all incoming header keys, so production code + // only ever sees lowercase. We test the exact code path here. + const { req, res, next } = createMockReqRes({ + method: 'POST', path: '/api/v1/backups/schedule', + headers: { cookie: '', 'x-csrf-token': 'some-signature-attempt' } + }); + csrfValidationMiddleware(req, res, next); + + expect(res.status).toHaveBeenCalledWith(403); + const lastWrite = stderrSpy.mock.calls[stderrSpy.mock.calls.length - 1][0]; + expect(lastWrite).toContain('[CSRF-debug]'); + }); + }); }); describe('renewCSRFToken', () => { diff --git a/dashcaddy-api/__tests__/routes/disk-space.routes.test.js b/dashcaddy-api/__tests__/routes/disk-space.routes.test.js new file mode 100644 index 0000000..15ad430 --- /dev/null +++ b/dashcaddy-api/__tests__/routes/disk-space.routes.test.js @@ -0,0 +1,277 @@ +/** + * DC-059: disk-space POST /config threshold-ordering invariant. + * + * DiskSpaceMonitor._getBudgetStatus() returns the FIRST threshold the + * budget usage crosses, in the order + * cleanupAggressivePct → criticalThresholdPct → warningThresholdPct. + * If a caller writes the three thresholds out of order + * (e.g. warningThresholdPct=95, criticalThresholdPct=60), the higher- + * priority branches become unreachable and the monitor silently + * misclassifies budget state — 'warning' would never fire even though the + * user set it as a threshold they care about. + * + * The fix lives in `routes/disk-space.js`: a `mergeAndCheckOrdering()` + * helper validates the *effective* (merged with live baseline) config + * against the invariant `warningThresholdPct < criticalThresholdPct < + * cleanupAggressivePct` BEFORE the route mutates diskSpaceMonitor.diskConfig. + * + * Tests cover: + * 1. Monotonic ascending order is accepted (happy path). + * 2. warningThresholdPct >= criticalThresholdPct is rejected with 400. + * 3. criticalThresholdPct >= cleanupAggressivePct is rejected with 400. + * 4. Partial updates work one field at a time without violating the + * invariant against the current baseline. + * 5. Out-of-bounds numeric values are clamped to the same bounds the + * original inline Math.min/Math.max chains enforced (50/60/70 → 99). + * 6. DiskSpaceMonitor.configure is NEVER called when the request is + * rejected (no partial mutation). + * 7. The merged config returned to the client is the post-clamp value, + * not the raw request body. + */ + +const express = require('express'); +const http = require('http'); + +const DEFAULT_CONFIG = { + enabled: true, + diskBudgetGB: 10, + warningThresholdPct: 80, + criticalThresholdPct: 90, + autoCleanup: true, + cleanupAggressivePct: 95, +}; + +function buildFakeDiskSpaceMonitor(initial = { ...DEFAULT_CONFIG }) { + const state = { ...initial }; + return { + configure: jest.fn((updates) => { + Object.assign(state, updates); + return { ...state }; + }), + getConfig: jest.fn(() => ({ ...state })), + getSnapshot: jest.fn(async () => ({})), + getDetailedBreakdown: jest.fn(async () => ({})), + performCleanup: jest.fn(async () => ({})), + // Test-only: peek at the internal state to confirm no mutation on rejection + _state: state, + }; +} + +function buildRouter(monitor) { + // Reset module cache so each test starts fresh + jest.resetModules(); + const mod = require('../../routes/disk-space'); + return mod({ + diskSpaceMonitor: monitor, + asyncHandler: (fn) => async (req, res, next) => { // eslint-disable-line require-await + try { await fn(req, res, next); } catch (e) { next(e); } + }, + log: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, + }); +} + +function buildApp(router) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { next(); }); // strip auth + app.use('/', router); + // eslint-disable-next-line no-unused-vars + app.use((err, req, res, next) => { + const status = err.statusCode || err.status || 500; + res.status(status).json({ + error: err.message, + code: err.code || 'ERR', + field: err.field || null, + }); + }); + return app; +} + +function supertestFetch(app) { + return function (method, path, body) { + return new Promise((resolve, reject) => { + const server = app.listen(0, () => { + const { port } = server.address(); + const data = body ? JSON.stringify(body) : null; + const req = http.request({ + method, + hostname: '127.0.0.1', + port, + path, + headers: data ? { 'Content-Type': 'application/json', 'Content-Length': Buffer.byteLength(data) } : {}, + }, (res) => { + let chunks = ''; + res.on('data', (c) => { chunks += c; }); + res.on('end', () => { + server.close(); + let parsed; + try { parsed = JSON.parse(chunks); } catch { parsed = chunks; } + resolve({ status: res.statusCode, body: parsed }); + }); + }); + req.on('error', (e) => { server.close(); reject(e); }); + if (data) req.write(data); + req.end(); + }); + }); + }; +} + +describe('routes/disk-space POST /config (DC-059 threshold ordering)', () => { + let monitor, app, fetch; + + beforeEach(() => { + monitor = buildFakeDiskSpaceMonitor(); + const router = buildRouter(monitor); + app = buildApp(router); + fetch = supertestFetch(app); + }); + + test('happy path — strict monotonic ascending order is accepted', async () => { + const res = await fetch('POST', '/config', { + warningThresholdPct: 75, + criticalThresholdPct: 88, + cleanupAggressivePct: 95, + }); + expect(res.status).toBe(200); + expect(res.body.config).toEqual(expect.objectContaining({ + warningThresholdPct: 75, + criticalThresholdPct: 88, + cleanupAggressivePct: 95, + })); + expect(monitor.configure).toHaveBeenCalledTimes(1); + }); + + test('warningThresholdPct >= criticalThresholdPct is rejected with 400', async () => { + const res = await fetch('POST', '/config', { + warningThresholdPct: 95, + criticalThresholdPct: 80, + cleanupAggressivePct: 99, + }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/warningThresholdPct.*strictly less than.*criticalThresholdPct/); + expect(res.body.field).toBe('warningThresholdPct'); + // Critical invariant: monitor.configure was NEVER called. + expect(monitor.configure).not.toHaveBeenCalled(); + }); + + test('criticalThresholdPct >= cleanupAggressivePct is rejected with 400', async () => { + const res = await fetch('POST', '/config', { + warningThresholdPct: 60, + criticalThresholdPct: 95, + cleanupAggressivePct: 80, + }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/criticalThresholdPct.*strictly less than.*cleanupAggressivePct/); + expect(res.body.field).toBe('criticalThresholdPct'); + expect(monitor.configure).not.toHaveBeenCalled(); + }); + + test('equal thresholds are rejected (strict <, not <=)', async () => { + const res = await fetch('POST', '/config', { + warningThresholdPct: 80, + criticalThresholdPct: 80, + cleanupAggressivePct: 90, + }); + expect(res.status).toBe(400); + expect(monitor.configure).not.toHaveBeenCalled(); + }); + + test('partial update — single field accepted against existing baseline', async () => { + // Defaults: warning=80, critical=90, aggressive=95. Raise warning to 85. + const res = await fetch('POST', '/config', { warningThresholdPct: 85 }); + expect(res.status).toBe(200); + expect(res.body.config.warningThresholdPct).toBe(85); + expect(res.body.config.criticalThresholdPct).toBe(90); + expect(res.body.config.cleanupAggressivePct).toBe(95); + }); + + test('partial update — would violate invariant against baseline, rejected', async () => { + // Defaults: warning=80, critical=90, aggressive=95. Setting warning=95 + // would collide with the existing critical=90 (warning >= critical). + const res = await fetch('POST', '/config', { warningThresholdPct: 95 }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/warningThresholdPct.*strictly less than.*criticalThresholdPct/); + expect(monitor.configure).not.toHaveBeenCalled(); + }); + + test('partial update — succeeds after baseline was updated in a prior request', async () => { + // First request: bump warning from 80 → 85 (within current critical=90). + let res = await fetch('POST', '/config', { warningThresholdPct: 85 }); + expect(res.status).toBe(200); + // Second request: now bump warning from 85 → 89. Still under critical=90. + res = await fetch('POST', '/config', { warningThresholdPct: 89 }); + expect(res.status).toBe(200); + expect(monitor.configure).toHaveBeenCalledTimes(2); + }); + + test('partial update — would violate against the NEW baseline, rejected', async () => { + // Step 1: raise warning to 85. + let res = await fetch('POST', '/config', { warningThresholdPct: 85 }); + expect(res.status).toBe(200); + // Step 2: try to raise warning to 95 — would collide with critical=90. + res = await fetch('POST', '/config', { warningThresholdPct: 95 }); + expect(res.status).toBe(400); + // monitor.configure should have run exactly once (the accepted request). + expect(monitor.configure).toHaveBeenCalledTimes(1); + }); + + test('out-of-bounds values are clamped to documented ranges', async () => { + // Note: the three values must produce a valid monotonic ordering AFTER + // clamping. Setting warning=20 (→ 50), critical=200 (→ 99), aggressive=70 + // would produce critical=99 > aggressive=70 which is rejected by the + // ordering check. Use values that clamp into a valid range. + const res = await fetch('POST', '/config', { + warningThresholdPct: 20, // below warning min 50 → clamped to 50 + criticalThresholdPct: 85, // valid + cleanupAggressivePct: 200, // above aggressive max 99 → clamped to 99 + }); + expect(res.status).toBe(200); + expect(res.body.config).toEqual(expect.objectContaining({ + warningThresholdPct: 50, + criticalThresholdPct: 85, + cleanupAggressivePct: 99, + })); + }); + + test('non-numeric threshold values are silently dropped (legacy behaviour preserved)', async () => { + // Strings are not numbers → unchanged from baseline. Confirms the + // ordering check doesn\'t reject legitimate "I didn\'t change this" requests. + const res = await fetch('POST', '/config', { + warningThresholdPct: '80', + }); + expect(res.status).toBe(200); + expect(res.body.config.warningThresholdPct).toBe(80); // baseline unchanged + expect(monitor.configure).toHaveBeenCalledWith({}); // empty updates + }); + + test('diskBudgetGB and autoCleanup updates still work alongside threshold validation', async () => { + const res = await fetch('POST', '/config', { + diskBudgetGB: 50, + autoCleanup: false, + warningThresholdPct: 81, + }); + expect(res.status).toBe(200); + expect(res.body.config.diskBudgetGB).toBe(50); + expect(res.body.config.autoCleanup).toBe(false); + expect(res.body.config.warningThresholdPct).toBe(81); + }); + + test('rejected request does NOT mutate the live diskConfig', async () => { + const before = { ...monitor._state }; + const res = await fetch('POST', '/config', { + warningThresholdPct: 95, // collides with critical=90 + }); + expect(res.status).toBe(400); + expect(monitor._state).toEqual(before); + }); + + test('POST /config with no thresholds in body is a no-op against baseline', async () => { + const res = await fetch('POST', '/config', { diskBudgetGB: 25 }); + expect(res.status).toBe(200); + expect(res.body.config.diskBudgetGB).toBe(25); + expect(res.body.config.warningThresholdPct).toBe(80); // unchanged + expect(res.body.config.criticalThresholdPct).toBe(90); // unchanged + expect(res.body.config.cleanupAggressivePct).toBe(95); // unchanged + }); +}); \ No newline at end of file diff --git a/dashcaddy-api/routes/disk-space.js b/dashcaddy-api/routes/disk-space.js index 345733e..5bf208f 100644 --- a/dashcaddy-api/routes/disk-space.js +++ b/dashcaddy-api/routes/disk-space.js @@ -1,5 +1,6 @@ const express = require('express'); const { success, error: errorResponse } = require('../src/utils/responses'); +const { ValidationError } = require('../src/utilities/errors'); /** * Disk space management routes @@ -10,6 +11,76 @@ const { success, error: errorResponse } = require('../src/utils/responses'); * POST /disk/config — update disk budget settings * POST /disk/cleanup — trigger manual cleanup (standard|aggressive|logs-only) */ + +// DC-059: monotonic-ordering invariant for the three threshold percentages. +// DiskSpaceMonitor._getBudgetStatus() walks them in order +// (cleanupAggressivePct → criticalThresholdPct → warningThresholdPct) and +// returns at the FIRST threshold the usage crosses. If a caller writes +// them out of order (e.g. warningThresholdPct=95, criticalThresholdPct=60), +// the higher-priority branches become unreachable and the monitor silently +// misclassifies budget state. Validate against the *effective* config +// (current value + incoming update for each field) so partial updates can +// be applied one field at a time without violating the invariant. +// +// Clamp values to the same ranges the previous inline Math.min/Math.max +// chains enforced (warning 50..99, critical 60..99, aggressive 70..99) +// so we don't loosen the original bounds while adding the new check. +const THRESHOLD_BOUNDS = Object.freeze({ + warning: { min: 50, max: 99 }, + critical: { min: 60, max: 99 }, + aggressive: { min: 70, max: 99 }, +}); + +function clampThreshold(name, value) { + const { min, max } = THRESHOLD_BOUNDS[name]; + return Math.min(Math.max(value, min), max); +} + +/** + * Apply a candidate update to a baseline config, then verify the three + * threshold percentages still satisfy + * warningThresholdPct < criticalThresholdPct < cleanupAggressivePct. + * The POST /config endpoint accepts partial updates (single field at a + * time), so we merge into the live diskSpaceMonitor config first, then test + * the merged value. Returns the merged candidate on success; throws + * ValidationError if the ordering invariant would be violated. + * + * @param {Object} baseline - current effective config from diskSpaceMonitor + * @param {Object} candidate - the partial update being applied this request + * @returns {Object} merged candidate with thresholds clamped to bounds + */ +function mergeAndCheckOrdering(baseline, candidate) { + const next = { ...baseline }; + if (typeof candidate.warningThresholdPct === 'number') { + next.warningThresholdPct = clampThreshold('warning', candidate.warningThresholdPct); + } + if (typeof candidate.criticalThresholdPct === 'number') { + next.criticalThresholdPct = clampThreshold('critical', candidate.criticalThresholdPct); + } + if (typeof candidate.cleanupAggressivePct === 'number') { + next.cleanupAggressivePct = clampThreshold('aggressive', candidate.cleanupAggressivePct); + } + if (!(next.warningThresholdPct < next.criticalThresholdPct)) { + throw new ValidationError( + `warningThresholdPct (${next.warningThresholdPct}) must be strictly less than criticalThresholdPct (${next.criticalThresholdPct})`, + 'warningThresholdPct' + ); + } + if (!(next.criticalThresholdPct < next.cleanupAggressivePct)) { + throw new ValidationError( + `criticalThresholdPct (${next.criticalThresholdPct}) must be strictly less than cleanupAggressivePct (${next.cleanupAggressivePct})`, + 'criticalThresholdPct' + ); + } + // Return only the fields the caller asked to change (preserves partial- + // update semantics; diskSpaceMonitor.configure does its own merge). + const out = {}; + if (typeof candidate.warningThresholdPct === 'number') out.warningThresholdPct = next.warningThresholdPct; + if (typeof candidate.criticalThresholdPct === 'number') out.criticalThresholdPct = next.criticalThresholdPct; + if (typeof candidate.cleanupAggressivePct === 'number') out.cleanupAggressivePct = next.cleanupAggressivePct; + return out; +} + module.exports = function({ diskSpaceMonitor, asyncHandler, log }) { const router = express.Router(); @@ -36,9 +107,16 @@ module.exports = function({ diskSpaceMonitor, asyncHandler, log }) { const updates = {}; if (typeof diskBudgetGB === 'number' && diskBudgetGB > 0) updates.diskBudgetGB = Math.min(diskBudgetGB, 1000); - if (typeof warningThresholdPct === 'number') updates.warningThresholdPct = Math.min(Math.max(warningThresholdPct, 50), 99); - if (typeof criticalThresholdPct === 'number') updates.criticalThresholdPct = Math.min(Math.max(criticalThresholdPct, 60), 99); - if (typeof cleanupAggressivePct === 'number') updates.cleanupAggressivePct = Math.min(Math.max(cleanupAggressivePct, 70), 99); + // DC-059: threshold percentages must satisfy a strict monotonic order + // (warning < critical < aggressive) so _getBudgetStatus() reaches the + // correct branch. mergeAndCheckOrdering() validates against the live + // baseline, so partial updates that violate the invariant are rejected + // BEFORE we mutate diskSpaceMonitor.diskConfig. + const thresholdUpdates = mergeAndCheckOrdering( + diskSpaceMonitor.getConfig(), + { warningThresholdPct, criticalThresholdPct, cleanupAggressivePct } + ); + Object.assign(updates, thresholdUpdates); if (typeof autoCleanup === 'boolean') updates.autoCleanup = autoCleanup; if (typeof enabled === 'boolean') updates.enabled = enabled; diff --git a/dashcaddy-api/src/security/csrf-protection.js b/dashcaddy-api/src/security/csrf-protection.js index 97cc7f4..ad7581f 100644 --- a/dashcaddy-api/src/security/csrf-protection.js +++ b/dashcaddy-api/src/security/csrf-protection.js @@ -214,14 +214,30 @@ function csrfValidationMiddleware(req, res, next) { return next(); } - // Validate both values exist + // DC-058: differentiate "browser auto-retry" from "real probe" using the + // X-CSRF-Token header as a signal. The dashboard JS in status/js/globals.js + // secureFetch() pre-fetches /api/v1/csrf-token (which sets the CSRF cookie + // via csrfCookieMiddleware) before posting; if the GET raced with container + // restart OR the user cleared cookies mid-session, the POST can arrive with + // a header but no cookie. secureFetch catches the 403 and auto-retries + // with a fresh token (lines 225-238 of globals.js). For these "has header + // but no cookie" misses, tag the log line [CSRF-debug] — operators can + // grep them out as expected noise. A request with NEITHER cookie NOR + // header (curl probe, exploit scanner, broken client) keeps the louder + // [CSRF] tag. if (!cookieNonce) { - process.stderr.write(`[CSRF] Missing CSRF cookie: ${method} ${req.path} from ${req.ip}\n`); + const isLikelyBrowserAutoRetry = !!headerToken; + const tag = isLikelyBrowserAutoRetry ? '[CSRF-debug]' : '[CSRF]'; + process.stderr.write(`${tag} Missing CSRF cookie: ${method} ${req.path} from ${req.ip}` + + (isLikelyBrowserAutoRetry ? ' (browser auto-retry — header present, expect self-heal)' : '') + '\n'); return errorResponse(res, 403, '[DC-100] CSRF token missing', { message: 'CSRF cookie not found. Please refresh the page (Ctrl+Shift+R) and try again.' }); } + // Cookie present but no header — a real browser POST always sends both, so + // header-less is suspicious (curl probe with manual cookie, misconfigured + // client). Keep WARN level. if (!headerToken) { process.stderr.write(`[CSRF] Missing CSRF header: ${method} ${req.path} from ${req.ip}\n`); return errorResponse(res, 403, '[DC-100] CSRF token missing', {