From 8f13a7faf3ad51ead5c288475fbe437c2061a03a Mon Sep 17 00:00:00 2001 From: Anso Date: Fri, 8 May 2026 03:48:35 -0400 Subject: [PATCH] fix(pilot): stop silently swallowing fs errors in agent token helpers (#985) * fix(pilot): stop silently swallowing fs errors in agent token helpers The pilot-agent audit found that both filesystem-touching helpers in the agent process discarded every fs error class: - readPersistedToken caught all errors and returned null. ENOENT (normal first boot) and EACCES / EIO / EROFS (a real disk or volume-permission failure) were indistinguishable; the latter silently fell back to "no persisted token", and on a node where the enrollment token had already been consumed the agent would enter a re-enrollment loop with no log signal pointing at the disk. - persistToken caught all errors and logged at console.warn. The operator saw the next-boot loop with the same diagnostic gap. Replace both with errno-aware handling: - readPersistedToken calls fs.readFileSync directly (no TOCTOU race against existsSync), treats ENOENT as silent, and logs every other errno at ERROR with the path. - persistToken logs at ERROR (not WARN) with the failing errno and an explicit "next agent restart will require re-enrollment until the volume is writable" message. The agent still continues with the in-memory token so the current session is unaffected. Both helpers are now exported (marked @internal) so the new test file can mock fs and assert the error-class-vs-log-level matrix. 12 unit cases in pilot-agent-fs-errors.test.ts cover ENOENT, EACCES, EIO, EROFS, ENOSPC, missing errno, empty file, and the happy path. Mock pattern follows backend/src/__tests__/filesystem.test.ts. * fix(pilot): address code-review on the fs-error branch Three findings from the review pass: - Em dash in a test description (Directive 18). Rewritten as "does not throw, so the in-memory token stays usable for the current session". - persistToken still had an existsSync + mkdirSync pair around the token-write. mkdirSync({ recursive: true }) is idempotent on existing directories, so the existsSync was redundant and added a TOCTOU window where the directory could be removed between the probe and the write. Dropped the existsSync; the test that previously primed mockExistsSync now asserts mockExistsSync is NOT called, locking the TOCTOU removal. - The two new exports used the @internal JSDoc tag, but this repo's existing pattern for "public-by-convention-for-tests" helpers (e.g. RegistryService.ts:481) is plain prose "Exposed for unit tests." Switched to that style. No behavior change beyond the TOCTOU removal. --- .../__tests__/pilot-agent-fs-errors.test.ts | 169 ++++++++++++++++++ backend/src/pilot/agent.ts | 53 ++++-- 2 files changed, 212 insertions(+), 10 deletions(-) create mode 100644 backend/src/__tests__/pilot-agent-fs-errors.test.ts 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.`, + ); } }