diff --git a/backend/src/__tests__/pilot-agent-fs-errors.test.ts b/backend/src/__tests__/pilot-agent-fs-errors.test.ts new file mode 100644 index 00000000..b832f9bd --- /dev/null +++ b/backend/src/__tests__/pilot-agent-fs-errors.test.ts @@ -0,0 +1,169 @@ +/** + * Unit tests for the pilot agent's filesystem-touching helpers. + * + * Both readPersistedToken and persistToken used to swallow every fs error + * silently; the audit found that an unwritable /app/data volume looked + * exactly like a fresh first boot to the rest of the agent, leaving the + * operator with no diagnostic signal when a node entered a re-enrollment + * loop. These tests lock the new behavior: + * + * - readPersistedToken treats ENOENT as silent (normal first boot) and + * surfaces every other errno at ERROR with the path and code. + * - persistToken logs at ERROR (not WARN) with an actionable + * "next restart will require re-enrollment" message and the failing + * errno, so the operator has a single log line that points at the disk. + * + * fs is mocked at the module-load layer per the existing pattern in + * filesystem.test.ts. The agent's top-level imports have no side effects, + * so importing the agent module after vi.mock is safe. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +const { mockReadFileSync, mockWriteFileSync, mockExistsSync, mockMkdirSync } = vi.hoisted(() => ({ + mockReadFileSync: vi.fn(), + mockWriteFileSync: vi.fn(), + mockExistsSync: vi.fn(), + mockMkdirSync: vi.fn(), +})); + +vi.mock('fs', () => { + const mock = { + readFileSync: mockReadFileSync, + writeFileSync: mockWriteFileSync, + existsSync: mockExistsSync, + mkdirSync: mockMkdirSync, + }; + return { ...mock, default: mock }; +}); + +// agent.ts is imported AFTER vi.mock so the mock is in place when the +// module's top-level fs import resolves. +import { readPersistedToken, persistToken } from '../pilot/agent'; + +let errorSpy: ReturnType; +let warnSpy: ReturnType; + +beforeEach(() => { + vi.clearAllMocks(); + errorSpy = vi.spyOn(console, 'error').mockImplementation(() => { /* swallow */ }); + warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => { /* swallow */ }); +}); + +afterEach(() => { + errorSpy.mockRestore(); + warnSpy.mockRestore(); +}); + +function fsError(code: string, message?: string): NodeJS.ErrnoException { + const err = new Error(message ?? code) as NodeJS.ErrnoException; + err.code = code; + return err; +} + +describe('readPersistedToken', () => { + it('returns the trimmed token when the file is present', () => { + mockReadFileSync.mockReturnValueOnce(' eyJhbGciOiJIUzI1NiJ9.payload.sig \n'); + const token = readPersistedToken(); + expect(token).toBe('eyJhbGciOiJIUzI1NiJ9.payload.sig'); + expect(errorSpy).not.toHaveBeenCalled(); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it('returns null and logs nothing on ENOENT (normal first boot)', () => { + mockReadFileSync.mockImplementationOnce(() => { throw fsError('ENOENT', 'no such file'); }); + const token = readPersistedToken(); + expect(token).toBeNull(); + expect(errorSpy).not.toHaveBeenCalled(); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it('returns null and logs at ERROR on EACCES (volume permission flip)', () => { + mockReadFileSync.mockImplementationOnce(() => { throw fsError('EACCES', 'permission denied'); }); + const token = readPersistedToken(); + expect(token).toBeNull(); + expect(errorSpy).toHaveBeenCalledOnce(); + const msg = String(errorSpy.mock.calls[0][0]); + expect(msg).toContain('Failed to read persisted tunnel token'); + expect(msg).toContain('EACCES'); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it('returns null and logs at ERROR on EIO (disk failure)', () => { + mockReadFileSync.mockImplementationOnce(() => { throw fsError('EIO', 'i/o error'); }); + const token = readPersistedToken(); + expect(token).toBeNull(); + expect(errorSpy).toHaveBeenCalledOnce(); + expect(String(errorSpy.mock.calls[0][0])).toContain('EIO'); + }); + + it('returns null and logs at ERROR even when the errno is missing', () => { + // Synthetic error with no .code attached: still classified as failure, + // not as ENOENT, so the operator gets a signal. + mockReadFileSync.mockImplementationOnce(() => { throw new Error('something weird'); }); + const token = readPersistedToken(); + expect(token).toBeNull(); + expect(errorSpy).toHaveBeenCalledOnce(); + expect(String(errorSpy.mock.calls[0][0])).toContain('unknown'); + }); + + it('returns null when the file is empty (avoid handing a blank string to the WS auth header)', () => { + mockReadFileSync.mockReturnValueOnce(' \n '); + const token = readPersistedToken(); + expect(token).toBeNull(); + expect(errorSpy).not.toHaveBeenCalled(); + }); +}); + +describe('persistToken', () => { + it('writes the token with mode 0o600 on the happy path', () => { + persistToken('test-token'); + expect(mockWriteFileSync).toHaveBeenCalledWith( + expect.stringContaining('pilot.jwt'), + 'test-token', + { mode: 0o600 }, + ); + expect(errorSpy).not.toHaveBeenCalled(); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it('creates the data directory recursively without an existsSync probe', () => { + persistToken('test-token'); + expect(mockMkdirSync).toHaveBeenCalledWith(expect.any(String), { recursive: true }); + expect(mockWriteFileSync).toHaveBeenCalled(); + // Lock the TOCTOU removal: an existsSync call here would re-introduce + // the race window between the probe and the mkdir/write. + expect(mockExistsSync).not.toHaveBeenCalled(); + }); + + it('logs at ERROR (not WARN) on ENOSPC and includes the actionable message', () => { + mockWriteFileSync.mockImplementationOnce(() => { throw fsError('ENOSPC', 'no space left'); }); + persistToken('test-token'); + expect(errorSpy).toHaveBeenCalledOnce(); + const msg = String(errorSpy.mock.calls[0][0]); + expect(msg).toContain('Failed to persist tunnel token'); + expect(msg).toContain('ENOSPC'); + expect(msg).toContain('next agent restart will require re-enrollment'); + // Critical: no console.warn fallback. The previous behavior was a + // silent warn that the operator missed. + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it('logs at ERROR on EACCES (read-only volume mount)', () => { + mockWriteFileSync.mockImplementationOnce(() => { throw fsError('EACCES', 'permission denied'); }); + persistToken('test-token'); + expect(errorSpy).toHaveBeenCalledOnce(); + expect(String(errorSpy.mock.calls[0][0])).toContain('EACCES'); + }); + + it('logs at ERROR on EROFS (read-only filesystem)', () => { + mockWriteFileSync.mockImplementationOnce(() => { throw fsError('EROFS', 'read-only file system'); }); + persistToken('test-token'); + expect(errorSpy).toHaveBeenCalledOnce(); + expect(String(errorSpy.mock.calls[0][0])).toContain('EROFS'); + }); + + it('does not throw, so the in-memory token stays usable for the current session', () => { + mockWriteFileSync.mockImplementationOnce(() => { throw fsError('EIO', 'i/o error'); }); + expect(() => persistToken('test-token')).not.toThrow(); + }); +}); diff --git a/backend/src/pilot/agent.ts b/backend/src/pilot/agent.ts index d2531921..7dcb5c30 100644 --- a/backend/src/pilot/agent.ts +++ b/backend/src/pilot/agent.ts @@ -603,13 +603,31 @@ type MeshResolveResult = | { ok: true; host: string; port: number } | { ok: false; err: MeshErrCode }; -function readPersistedToken(): string | null { +/** + * Read the persisted long-lived tunnel token from disk if present. ENOENT is + * the normal first-boot case and stays silent. Any other error class + * (EACCES, EIO, EISDIR, etc.) almost certainly means the volume is + * misconfigured or corrupt; log at ERROR with the path and the errno so the + * operator has an actionable signal, then return null. Returning null here + * lets the caller fall back to SENCHO_ENROLL_TOKEN if one is set, or exit + * with a clear "no credentials" message if not. + * + * Calls readFileSync directly rather than racing existsSync + readFileSync + * to avoid TOCTOU and to surface the actual errno on real failures. + * + * Exposed for unit tests. + */ +export function readPersistedToken(): string | null { try { - if (fs.existsSync(TOKEN_PATH)) { - return fs.readFileSync(TOKEN_PATH, 'utf8').trim() || null; - } - } catch { /* ignore */ } - return null; + return fs.readFileSync(TOKEN_PATH, 'utf8').trim() || null; + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code === 'ENOENT') return null; + console.error( + `[Pilot] Failed to read persisted tunnel token at ${sanitizeForLog(TOKEN_PATH)}: ${sanitizeForLog(code ?? 'unknown')} - ${sanitizeForLog((err as Error).message)}`, + ); + return null; + } } /** @@ -630,13 +648,28 @@ function readPilotCaBundle(): Buffer | null { } } -function persistToken(token: string): void { +/** + * Persist the long-lived tunnel token so the agent can reconnect after a + * container restart without re-enrolling. On failure we log at ERROR (not + * WARN) with an explicit "next agent restart will require re-enrollment" + * message: a silent warning here meant the operator saw the next-boot + * re-enrollment loop with no signal pointing at the disk. The current + * tunnel session continues with the in-memory token regardless. + * + * mkdirSync with recursive:true is idempotent on existing directories, so + * the prior existsSync guard was redundant and added a TOCTOU window. + * + * Exposed for unit tests. + */ +export function persistToken(token: string): void { try { - const dir = path.dirname(TOKEN_PATH); - if (!fs.existsSync(dir)) fs.mkdirSync(dir, { recursive: true }); + fs.mkdirSync(path.dirname(TOKEN_PATH), { recursive: true }); fs.writeFileSync(TOKEN_PATH, token, { mode: 0o600 }); } catch (err) { - console.warn('[Pilot] Failed to persist tunnel token:', (err as Error).message); + const code = (err as NodeJS.ErrnoException).code; + console.error( + `[Pilot] Failed to persist tunnel token at ${sanitizeForLog(TOKEN_PATH)} (${sanitizeForLog(code ?? 'unknown')}: ${sanitizeForLog((err as Error).message)}). Continuing with the in-memory token; the next agent restart will require re-enrollment until the volume is writable.`, + ); } }