diff --git a/dashcaddy-api/__tests__/credential-manager.test.js b/dashcaddy-api/__tests__/credential-manager.test.js index ca19bb7..de42924 100644 --- a/dashcaddy-api/__tests__/credential-manager.test.js +++ b/dashcaddy-api/__tests__/credential-manager.test.js @@ -23,13 +23,65 @@ jest.mock('proper-lockfile', () => ({ check: jest.fn().mockResolvedValue(false), })); +// DC-106: fd-level mock exercising the canonical atomic-write path +// (openSync('wx') -> writeSync -> fsyncSync -> closeSync -> renameSync). +const mockFsState = { + files: {}, // path -> content (destination state after rename) + fdMap: new Map(), // open fd -> { p, content } + closedTmp: new Map(), // closed-but-not-yet-renamed tmp path -> content + openedWith: [], // { p, flags, mode } per openSync call + nextFd: 0, +}; + jest.mock('fs', () => ({ - existsSync: jest.fn().mockReturnValue(true), - readFileSync: jest.fn().mockReturnValue('{}'), - writeFileSync: jest.fn(), + existsSync: jest.fn((p) => mockFsState.files[p] !== undefined), + readFileSync: jest.fn((p) => { + if (mockFsState.files[p] === undefined) { + const e = new Error(`ENOENT: ${p}`); + e.code = 'ENOENT'; + throw e; + } + return mockFsState.files[p]; + }), mkdirSync: jest.fn(), + // DC-105/DC-106 canonical atomic-write path (atomic-write.js). + openSync: jest.fn((p, flags, mode) => { + mockFsState.openedWith.push({ p, flags, mode }); + mockFsState.nextFd += 1; + mockFsState.fdMap.set(mockFsState.nextFd, { p, content: '' }); + return mockFsState.nextFd; + }), + writeSync: jest.fn((fd, content) => { + const rec = mockFsState.fdMap.get(fd); + if (!rec) throw new Error(`EBADF: fd ${fd}`); + rec.content += content; + }), + fsyncSync: jest.fn(), + closeSync: jest.fn((fd) => { + const rec = mockFsState.fdMap.get(fd); + if (rec) { + mockFsState.closedTmp.set(rec.p, rec.content); + mockFsState.fdMap.delete(fd); + } + }), + renameSync: jest.fn((src, dst) => { + const content = mockFsState.closedTmp.has(src) + ? mockFsState.closedTmp.get(src) + : mockFsState.files[src]; + mockFsState.files[dst] = content; + mockFsState.closedTmp.delete(src); + delete mockFsState.files[src]; + }), + unlinkSync: jest.fn(), })); +// DC-106: mirror the production path resolution so assertions read the same +// destination the manager writes to, regardless of env overrides. +const path = require('path'); +const platformPaths = require('../platform-paths'); +const CREDENTIALS_FILE = process.env.CREDENTIALS_FILE + || path.join(platformPaths.dataDir, 'credentials.json'); + describe('CredentialManager', () => { let credentialManager; let fs, lockfile, keychainManager, cryptoUtils; @@ -43,10 +95,30 @@ describe('CredentialManager', () => { keychainManager = require('../src/security/keychain-manager'); cryptoUtils = require('../src/security/crypto-utils'); - // Reset mock implementations - fs.existsSync.mockReturnValue(true); - fs.readFileSync.mockReturnValue('{}'); - fs.writeFileSync.mockImplementation(() => {}); + // Reset mock implementations and fd-level atomic-write state + for (const k of Object.keys(mockFsState.files)) delete mockFsState.files[k]; + mockFsState.fdMap.clear(); + mockFsState.closedTmp.clear(); + mockFsState.openedWith.length = 0; + mockFsState.nextFd = 0; + // Default world: credentials.json exists with empty payload (the previous + // mock's existsSync=true / readFileSync='{}' semantics, now truthful). + mockFsState.files[CREDENTIALS_FILE] = '{}'; + fs.existsSync.mockImplementation((p) => mockFsState.files[p] !== undefined); + fs.readFileSync.mockImplementation((p) => { + if (mockFsState.files[p] === undefined) { + const e = new Error(`ENOENT: ${p}`); + e.code = 'ENOENT'; + throw e; + } + return mockFsState.files[p]; + }); + fs.openSync.mockClear(); + fs.writeSync.mockClear(); + fs.fsyncSync.mockClear(); + fs.closeSync.mockClear(); + fs.renameSync.mockClear(); + fs.unlinkSync.mockClear(); lockfile.lock.mockResolvedValue(jest.fn().mockResolvedValue()); keychainManager.available = false; @@ -59,7 +131,7 @@ describe('CredentialManager', () => { const result = await credentialManager.store('test.key', 'secret-value'); expect(result).toBe(true); expect(cryptoUtils.encrypt).toHaveBeenCalledWith('secret-value'); - expect(fs.writeFileSync).toHaveBeenCalled(); + expect(fs.renameSync).toHaveBeenCalled(); // DC-106: canonical write landed }); it('stores value in keychain when available', async () => { @@ -67,9 +139,10 @@ describe('CredentialManager', () => { // Need to get a fresh instance that sees available=true jest.resetModules(); fs = require('fs'); - fs.existsSync.mockReturnValue(true); - fs.readFileSync.mockReturnValue('{}'); - fs.writeFileSync.mockImplementation(() => {}); + for (const k of Object.keys(mockFsState.files)) delete mockFsState.files[k]; + mockFsState.fdMap.clear(); + mockFsState.closedTmp.clear(); + mockFsState.openedWith.length = 0; lockfile = require('proper-lockfile'); lockfile.lock.mockResolvedValue(jest.fn().mockResolvedValue()); keychainManager = require('../src/security/keychain-manager'); @@ -86,9 +159,10 @@ describe('CredentialManager', () => { keychainManager.available = true; jest.resetModules(); fs = require('fs'); - fs.existsSync.mockReturnValue(true); - fs.readFileSync.mockReturnValue('{}'); - fs.writeFileSync.mockImplementation(() => {}); + for (const k of Object.keys(mockFsState.files)) delete mockFsState.files[k]; + mockFsState.fdMap.clear(); + mockFsState.closedTmp.clear(); + mockFsState.openedWith.length = 0; lockfile = require('proper-lockfile'); lockfile.lock.mockResolvedValue(jest.fn().mockResolvedValue()); keychainManager = require('../src/security/keychain-manager'); @@ -226,8 +300,7 @@ describe('CredentialManager', () => { }); expect(lockfile.lock).toHaveBeenCalled(); - expect(fs.writeFileSync).toHaveBeenCalled(); - const writtenData = JSON.parse(fs.writeFileSync.mock.calls[0][1]); + const writtenData = JSON.parse(mockFsState.files[CREDENTIALS_FILE]); expect(writtenData).toEqual({ a: 1, b: 2 }); expect(releaseFn).toHaveBeenCalled(); }); @@ -264,7 +337,7 @@ describe('CredentialManager', () => { const result = await credentialManager.rotateEncryptionKey(); expect(result).toBe(true); expect(cryptoUtils.rotateKey).toHaveBeenCalled(); - expect(fs.writeFileSync).toHaveBeenCalled(); + expect(fs.renameSync).toHaveBeenCalled(); // DC-106: canonical write landed }); it('clears cache after rotation', async () => { @@ -326,6 +399,90 @@ describe('CredentialManager', () => { }); }); + + describe('DC-106 canonical atomic-write migration', () => { + it('writes credentials.json via wx tmp + fsync + rename, mode 0600', async () => { + await credentialManager.store('dc106.key', 'dc106-secret'); + + // fsyncDir also openSync()s the parent dir (flags 'r') — filter to the + // payload tmp opens to assert on the canonical write itself. + const wxOpens = mockFsState.openedWith.filter((o) => o.flags === 'wx'); + expect(wxOpens.length).toBe(1); // file pre-existed -> no ensure-create + expect(wxOpens[0].mode).toBe(0o600); // sensitive file mode preserved + expect(fs.fsyncSync).toHaveBeenCalled(); // bytes pinned before rename + expect(fs.renameSync).toHaveBeenCalled(); + + const [tmpSrc, dst] = fs.renameSync.mock.calls + .find((c) => c[1] === CREDENTIALS_FILE); + expect(tmpSrc).not.toBe(dst); + expect(tmpSrc).toMatch(/\.credentials\.json\.tmp-/); // canonical tmp prefix + expect(dst).toBe(CREDENTIALS_FILE); + expect(mockFsState.files[CREDENTIALS_FILE]).toBeDefined(); + + // No leftover tmp files: every payload tmp was renamed away + const renamedSrcs = fs.renameSync.mock.calls.map((c) => c[0]); + for (const o of wxOpens) { + expect(renamedSrcs).toContain(o.p); + } + }); + + it('never writes plaintext secret to disk', async () => { + await credentialManager.store('dc106b.key', 'plaintext-canary-9f1a'); + const raw = mockFsState.files[CREDENTIALS_FILE]; + expect(raw).toBeDefined(); + expect(raw).not.toContain('plaintext-canary-9f1a'); + expect(raw).toContain('enc:'); // crypto-utils mock prefix + }); + + it('_lockedUpdate closes fd before rename (torn-write window eliminated)', async () => { + const releaseFn = jest.fn().mockResolvedValue(); + lockfile.lock.mockResolvedValue(releaseFn); + mockFsState.files[CREDENTIALS_FILE] = '{}'; + + await credentialManager._lockedUpdate((creds) => { + creds.k = { value: 'enc:x' }; + return creds; + }); + + // fd lifecycle: open -> write -> fsync -> close -> rename. The dir + // fsync adds a second openSync/closeSync pair — so assert on counts of + // payload operations and the GLOBAL invocation order, which jest tracks + // across mocks (invocationCallOrder). + const wxOpens = mockFsState.openedWith.filter((o) => o.flags === 'wx'); + expect(wxOpens.length).toBe(1); // exactly one payload write + expect(fs.writeSync).toHaveBeenCalledTimes(1); // dir fsync writes nothing + expect(fs.renameSync).toHaveBeenCalledTimes(1); + const fsyncFirst = fs.fsyncSync.mock.invocationCallOrder[0]; + const closeFirst = fs.closeSync.mock.invocationCallOrder[0]; + const renameFirst = fs.renameSync.mock.invocationCallOrder[0]; + expect(fsyncFirst).toBeDefined(); + expect(closeFirst).toBeGreaterThan(fsyncFirst); // fsync before close + expect(renameFirst).toBeGreaterThan(closeFirst); // close before rename + expect(mockFsState.files[CREDENTIALS_FILE]).toContain('enc:x'); + }); + + it('_ensureFileExists creates initial {} atomically at 0600 when absent', async () => { + const releaseFn = jest.fn().mockResolvedValue(); + lockfile.lock.mockResolvedValue(releaseFn); + delete mockFsState.files[CREDENTIALS_FILE]; // absent on disk + + await credentialManager._lockedUpdate((c) => { + c.k = { value: 'enc:x' }; + return c; + }); + + const wxOpens = mockFsState.openedWith.filter((o) => o.flags === 'wx'); + expect(wxOpens.length).toBe(2); // ensure-created '{}' + the locked update + expect(wxOpens[0].mode).toBe(0o600); + // The ensure write staged its tmp FIRST and renamed it into place before + // the locked update renamed over it — creation itself was atomic. + expect(fs.renameSync.mock.calls[0][0]).toBe(wxOpens[0].p); + const final = JSON.parse(mockFsState.files[CREDENTIALS_FILE]); + expect(final.k.value).toBe('enc:x'); + }); + + }); + describe('cache TTL', () => { it('cache entries expire after TTL', async () => { credentialManager.cache.set('ttl.key', { diff --git a/dashcaddy-api/src/managers/credential-manager.js b/dashcaddy-api/src/managers/credential-manager.js index 2346806..d4f9e45 100644 --- a/dashcaddy-api/src/managers/credential-manager.js +++ b/dashcaddy-api/src/managers/credential-manager.js @@ -8,6 +8,7 @@ const keychainManager = require('../security/keychain-manager'); const cryptoUtils = require('../security/crypto-utils'); const lockfile = require('proper-lockfile'); const fs = require('fs'); +const { atomicWriteJSON } = require('../utils/atomic-write'); const { log } = require('../utils/logging'); const path = require('path'); const platformPaths = require('../../platform-paths'); @@ -214,8 +215,9 @@ class CredentialManager { }; } - // Save with new encryption - fs.writeFileSync(CREDENTIALS_FILE, JSON.stringify(rotated, null, 2), { mode: 0o600 }); + // Save with new encryption (DC-106: canonical atomic-write; proper-lockfile + // only tracks its own .lock dir, so the rename swap is lock-safe) + atomicWriteJSON(CREDENTIALS_FILE, rotated, { mode: 0o600 }); // Clear cache to force reload this.cache.clear(); @@ -278,7 +280,9 @@ class CredentialManager { if (!fs.existsSync(dir)) { fs.mkdirSync(dir, { recursive: true }); } - fs.writeFileSync(CREDENTIALS_FILE, '{}', { mode: 0o600 }); + // DC-106: canonical atomic-write — same wx/fsync/rename discipline as every + // other store. '{}' initial payload; 0600 mode is atomicWriteFile's default. + atomicWriteJSON(CREDENTIALS_FILE, {}); } } @@ -296,7 +300,10 @@ class CredentialManager { const data = fs.readFileSync(CREDENTIALS_FILE, 'utf8'); const credentials = JSON.parse(data); const updated = await updateFn(credentials); - fs.writeFileSync(CREDENTIALS_FILE, JSON.stringify(updated, null, 2), { mode: 0o600 }); + // DC-106: canonical atomic-write under the proper-lockfile lock — a crash + // can no longer tear credentials.json mid-write (the lockfile dir is a + // sibling of the target path, so the rename swap stays lock-safe). + atomicWriteJSON(CREDENTIALS_FILE, updated, { mode: 0o600 }); return updated; } catch (error) { if (error.code === 'ELOCKED') {