refactor(persistence): migrate credential-manager to canonical atomic-write util (DC-106) [glm-grade=B]
All three writeFileSync sites in the encrypted-credentials manager now delegate to src/utils/atomic-write.js atomicWriteJSON — rotateEncryptionKey save, _ensureFileExists bootstrap, and the _lockedUpdate commit under the proper-lockfile lock. A crash can no longer tear credentials.json mid-write: power loss through the old path could leave an empty/short file and silently drop every stored credential (DNS provider tokens etc). Lock-safety pre-study: proper-lockfile only stats its own sibling .lock dir, never the target file, so the rename swap cannot trip ECOMPROMISED. Test mock extended to the fd-level fs API (openSync/writeSync/ fsyncSync/closeSync/renameSync + fdMap/closedTmp state) so the canonical path is exercised end-to-end under the mock; write assertions moved from writeFileSync.mock.calls to destination-state reads. New DC-106 pins: wx+0600+fsync+rename discipline, plaintext-secret canary never on disk, fd-lifecycle order fsync->close->rename via invocationCallOrder, and atomic _ensureFileExists create at 0600. Eighth store migrated (DC-099..DC-105 preceded). Judge: GLM-5.3 cold read, round-1 B/ship (deleg_b3c038c2). URN urn:ump:vscequdet7wt5un7jhtl2nlg6cfkabsbazy5m2tjnyyguu5wstxa (readback verified: grade B, topic codex-judge-verdict). Sole finding is pre-existing and non-blocking: rotateEncryptionKey persists the new key before writing rotated creds (crash window) — queued as DC-107 follow-up. Full suite: 121 suites / 2783 tests green.
This commit is contained in:
@@ -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', {
|
||||
|
||||
Reference in New Issue
Block a user