diff --git a/src/index.ts b/src/index.ts index d2e3d6b..88f1994 100755 --- a/src/index.ts +++ b/src/index.ts @@ -50,7 +50,7 @@ import { isTelemetryOptedOut, recordOutcome, recordTelemetryExtras, - resolveTelemetryAuth, + tryResolveTelemetryAuth, type ResolvedTelemetryAuth, type WaitTimeoutTelemetry, } from './lib/telemetry.js'; @@ -304,13 +304,25 @@ program.hook('preAction', (_thisCommand, actionCommand) => { // Telemetry gate, same reasons as the update notice below: skip `completion` // (its stdout is eval'd by shells) and `--plan-template` (pure-local). Resolve // auth now, before the command's action runs, so `auth remove` still reports - // on the key it used; skip the read for opt-out / dry-run. + // on the key it used; skip the read for opt-out / dry-run. The read is + // best-effort: an unreadable credentials file must not end the process here, + // before the command could run — `doctor` exists to diagnose exactly that, and + // offline commands never need the key. On failure the command runs as if + // telemetry were off (no event, and no fallback read at exit either). telemetryEmit = commandPath !== 'completion' && !isPlanTemplate; if (telemetryEmit && !isTelemetryOptedOut(process.env) && globals.dryRun !== true) { - telemetryAuth = resolveTelemetryAuth({ - profile: globals.profile, - endpointUrl: globals.endpointUrl, - }); + telemetryAuth = tryResolveTelemetryAuth( + { profile: globals.profile, endpointUrl: globals.endpointUrl }, + { + onError: globals.debug + ? error => + process.stderr.write( + `[debug] telemetry skipped: ${error instanceof Error ? error.message : String(error)}\n`, + ) + : undefined, + }, + ); + if (telemetryAuth === undefined) telemetryEmit = false; } if (!isPlanTemplate) { maybeEmitSkillNudge({ diff --git a/src/lib/telemetry.spec.ts b/src/lib/telemetry.spec.ts index 6b51e59..d6d5622 100644 --- a/src/lib/telemetry.spec.ts +++ b/src/lib/telemetry.spec.ts @@ -20,6 +20,7 @@ import { recordOutcome, recordTelemetryExtras, resolveTelemetryAuth, + tryResolveTelemetryAuth, sanitizeTelemetryExtras, takeTelemetryExtras, } from './telemetry.js'; @@ -539,6 +540,47 @@ describe('resolveTelemetryAuth', () => { }); }); +// tryResolveTelemetryAuth — the pre-action hook's variant; must never throw +describe('tryResolveTelemetryAuth', () => { + /** A credentials path that exists but cannot be read as a file, on every OS. */ + function unreadableCredsPath(): string { + const dir = mkdtempSync(join(tmpdir(), 'ts-telemetry-unreadable-')); + const path = join(dir, 'credentials'); + // eslint-disable-next-line security/detect-non-literal-fs-filename -- test fixture path built from this test's own mkdtempSync() dir, never user input + mkdirSync(path); + return path; + } + + it('returns the same auth as resolveTelemetryAuth when the file reads', () => { + const credentialsPath = writeCreds(true); + expect(tryResolveTelemetryAuth({}, { env: {}, credentialsPath })).toEqual( + resolveTelemetryAuth({}, { env: {}, credentialsPath }), + ); + }); + + it('returns undefined instead of throwing when the credentials read fails', () => { + const credentialsPath = unreadableCredsPath(); + expect(() => resolveTelemetryAuth({}, { env: {}, credentialsPath })).toThrow(); + expect(tryResolveTelemetryAuth({}, { env: {}, credentialsPath })).toBeUndefined(); + }); + + it('hands the swallowed error to onError (the --debug line)', () => { + const onError = vi.fn(); + tryResolveTelemetryAuth({}, { env: {}, credentialsPath: unreadableCredsPath(), onError }); + expect(onError).toHaveBeenCalledOnce(); + expect(onError.mock.calls[0]?.[0]).toBeInstanceOf(Error); + }); + + it('stays silent when onError itself throws', () => { + const onError = vi.fn(() => { + throw new Error('stderr closed'); + }); + expect( + tryResolveTelemetryAuth({}, { env: {}, credentialsPath: unreadableCredsPath(), onError }), + ).toBeUndefined(); + }); +}); + describe('wait timeout event fields', () => { const base = { command: 'test run', diff --git a/src/lib/telemetry.ts b/src/lib/telemetry.ts index f16dd8f..461a3d8 100644 --- a/src/lib/telemetry.ts +++ b/src/lib/telemetry.ts @@ -645,6 +645,35 @@ export function resolveTelemetryAuth( return { apiKey: config.apiKey, apiUrl: config.apiUrl }; } +/** + * {@link resolveTelemetryAuth} for the pre-action hook, which runs before every + * command: never throws. An unreadable credentials file (EPERM/EACCES — the + * Windows ACL lock-out `doctor` diagnoses — or any other read/parse failure) + * returns `undefined` so the caller can skip the beacon and let the command run + * and report the problem its own way, as the module contract requires + * (telemetry never changes a command's behavior, output, or exit code). + * `onError` receives the swallowed error, for `--debug`. + */ +export function tryResolveTelemetryAuth( + opts: { profile?: string; endpointUrl?: string }, + deps: { + env?: NodeJS.ProcessEnv; + credentialsPath?: string; + onError?: (error: unknown) => void; + } = {}, +): ResolvedTelemetryAuth | undefined { + try { + return resolveTelemetryAuth(opts, deps); + } catch (error) { + try { + deps.onError?.(error); + } catch { + // A broken diagnostic sink must not turn a skipped beacon into a failure. + } + return undefined; + } +} + /** * Fire one command-outcome event to the beacon. Awaited by `index.ts` before * `process.exit` (the flush) — bounded and fully best-effort, so it can neither diff --git a/test/e2e/telemetry-unreadable-credentials.e2e.test.ts b/test/e2e/telemetry-unreadable-credentials.e2e.test.ts new file mode 100644 index 0000000..4bc37ea --- /dev/null +++ b/test/e2e/telemetry-unreadable-credentials.e2e.test.ts @@ -0,0 +1,173 @@ +/** + * An unreadable credentials file must not crash commands in the telemetry + * pre-action hook (#465). The hook resolves telemetry auth before every + * command's action; a raw EACCES/EPERM from that read used to end the process + * before `doctor` could diagnose the file or an offline command could run. + * Now the command runs as if telemetry were off: no beacon, and `doctor` + * reports the problem itself. + * + * Exercises the built entrypoint against a local server that records every + * request. `chmod 000` is how the read is made to fail, so the suite needs a + * POSIX host and a non-root user (root reads through mode bits); the hook's + * error handling itself is unit-tested cross-platform in `telemetry.spec.ts`. + */ + +import { chmodSync, existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { createServer, type Server } from 'node:http'; +import { tmpdir } from 'node:os'; +import { dirname, join, resolve } from 'node:path'; +import { spawn } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'vitest'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = resolve(__dirname, '../..'); +const BIN_PATH = join(REPO_ROOT, 'dist', 'index.js'); + +const canDenyRead = process.platform !== 'win32' && process.getuid?.() !== 0; + +let server: Server; +let baseUrl = ''; +const requests: string[] = []; +let home = ''; +let credentialsPath = ''; + +beforeAll(async () => { + if (!existsSync(BIN_PATH)) { + throw new Error('dist/index.js not found — run `npm run test:e2e` which builds first.'); + } + server = createServer((req, res) => { + const path = (req.url ?? '').split('?')[0]; + requests.push(`${req.method} ${path}`); + req.resume(); + if (req.method === 'POST' && path === '/api/cli/v1/telemetry') { + res.writeHead(204, { connection: 'close' }); + res.end(); + return; + } + if (req.method === 'GET' && path === '/api/cli/v1/me') { + res.writeHead(200, { 'content-type': 'application/json', connection: 'close' }); + res.end( + JSON.stringify({ + userId: 'user_e2e', + keyId: 'key_e2e', + scopes: ['tests:read'], + env: 'e2e', + }), + ); + return; + } + res.writeHead(404, { connection: 'close' }); + res.end(); + }); + await new Promise(resolveListen => server.listen(0, '127.0.0.1', () => resolveListen())); + const address = server.address(); + if (address === null || typeof address === 'string') throw new Error('no server address'); + baseUrl = `http://127.0.0.1:${address.port}`; +}); + +beforeEach(() => { + home = mkdtempSync(join(tmpdir(), 'ts-e2e-unreadable-creds-')); + mkdirSync(join(home, '.testsprite')); + credentialsPath = join(home, '.testsprite', 'credentials'); + writeFileSync(credentialsPath, '[default]\napi_key = sk-user-from-file\n', { mode: 0o600 }); +}); + +afterEach(() => { + requests.length = 0; + // Restore access first so the temp tree can be removed. + if (existsSync(credentialsPath)) chmodSync(credentialsPath, 0o600); + rmSync(home, { recursive: true, force: true }); +}); + +afterAll(async () => { + await new Promise(resolveClose => { + server.close(() => resolveClose()); + server.closeAllConnections(); + }); +}); + +async function runCli(args: string[]): Promise<{ status: number; stdout: string; stderr: string }> { + const child = spawn(process.execPath, [BIN_PATH, ...args], { + cwd: home, + stdio: ['ignore', 'pipe', 'pipe'], + timeout: 20_000, + env: { + ...process.env, + HOME: home, + USERPROFILE: home, + // A key in the environment: the file is still read (loadConfig always + // reads the profile), and without the fix that read is what crashes. + TESTSPRITE_API_KEY: 'sk-user-e2e-env-key', + TESTSPRITE_API_URL: baseUrl, + TESTSPRITE_PROFILE: '', + TESTSPRITE_NO_SKILL_WARNING: '1', + TESTSPRITE_NO_UPDATE_NOTIFIER: '1', + // Telemetry must be ON — that is the path under test. + TESTSPRITE_NO_TELEMETRY: '', + DO_NOT_TRACK: '', + CI: '', + }, + }); + let stdout = ''; + let stderr = ''; + child.stdout.setEncoding('utf8').on('data', (chunk: string) => (stdout += chunk)); + child.stderr.setEncoding('utf8').on('data', (chunk: string) => (stderr += chunk)); + const status = await new Promise((resolveExit, reject) => { + child.once('error', reject); + child.once('close', code => resolveExit(code ?? -1)); + }); + return { status, stdout, stderr }; +} + +const telemetryPosts = (): string[] => requests.filter(r => r === 'POST /api/cli/v1/telemetry'); + +describe('telemetry pre-action hook — readable credentials (control)', () => { + it('sends the beacon, so its absence below is meaningful', async () => { + const result = await runCli(['agent', 'list']); + expect(result.status).toBe(0); + expect(telemetryPosts()).toHaveLength(1); + }); +}); + +describe.skipIf(!canDenyRead)('telemetry pre-action hook — unreadable credentials file', () => { + beforeEach(() => { + chmodSync(credentialsPath, 0o000); + }); + + it('doctor runs and reports the Credentials problem itself; no beacon', async () => { + const result = await runCli(['doctor']); + const output = result.stdout + result.stderr; + // TESTSPRITE_API_KEY is set and /me answers, so the unreadable file is a + // WARN (commands still work) and doctor completes with exit 0. + expect(result.status).toBe(0); + expect(output).not.toMatch(/Error: (EACCES|EPERM)/); + expect(output).toContain('TestSprite doctor'); + expect(output).toMatch( + /\[WARN\]\s+Credentials\s+credentials file exists but cannot be read \((EACCES|EPERM)\)/, + ); + expect(telemetryPosts()).toEqual([]); + }); + + it('an offline command (agent list) runs and exits 0; no beacon', async () => { + const result = await runCli(['agent', 'list']); + expect(result.stderr).not.toMatch(/EACCES|EPERM/); + expect(result.status).toBe(0); + expect(result.stdout).toContain('AGENT'); + expect(telemetryPosts()).toEqual([]); + }); + + it('completion still prints its script and exits 0; no beacon', async () => { + const result = await runCli(['completion', 'bash']); + expect(result.status).toBe(0); + expect(result.stdout).toContain('_testsprite_completion'); + expect(telemetryPosts()).toEqual([]); + }); + + it('--debug names why the beacon was skipped', async () => { + const result = await runCli(['--debug', 'agent', 'list']); + expect(result.status).toBe(0); + expect(result.stderr).toMatch(/\[debug\] telemetry skipped: (EACCES|EPERM)/); + expect(telemetryPosts()).toEqual([]); + }); +});