diff --git a/src/api/__tests__/env-key-bearer.test.ts b/src/api/__tests__/env-key-bearer.test.ts new file mode 100644 index 00000000..34c304fe --- /dev/null +++ b/src/api/__tests__/env-key-bearer.test.ts @@ -0,0 +1,79 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { apiClient } from '../client.js'; +import { API_KEY_ENV_VAR } from '../../config/env-vars.js'; +import * as secrets from '../../storage/secrets.js'; +import { AuthError, ForbiddenError } from '../../output/error.js'; +import { setWorkspaceContext } from '../client.js'; + +// The env key must reach the wire as the bearer token, exactly as a stored +// agent credential does — no refresh attempt, no rewriting (AIT-438). +describe('apiClient with HOOKMYAPP_API_KEY (AIT-438)', () => { + const original = process.env[API_KEY_ENV_VAR]; + + beforeEach(() => { + vi.restoreAllMocks(); + vi.spyOn(secrets, 'readSecrets').mockResolvedValue(null); + }); + afterEach(() => { + if (original === undefined) delete process.env[API_KEY_ENV_VAR]; + else process.env[API_KEY_ENV_VAR] = original; + vi.unstubAllGlobals(); + }); + + // A revoked key 401s. "Session expired. Run: login" is wrong twice over — + // there is no session, and login refuses while the variable is set. + it('names the variable when the key is rejected, not "session expired"', async () => { + process.env[API_KEY_ENV_VAR] = 'hmok_revoked'; + vi.stubGlobal('fetch', vi.fn(async () => ({ + ok: false, + status: 401, + headers: new Headers(), + json: async () => ({ message: 'Unauthorized' }), + }))); + + await expect(apiClient('/workspaces')).rejects.toThrow(AuthError); + await expect(apiClient('/workspaces')).rejects.toThrow( + /HOOKMYAPP_API_KEY was rejected/, + ); + }); + + // A bare 403 maps to PermissionError, whose stock message says "run: + // hookmyapp login" — which login now refuses to do while the key is set. + it('gives permission guidance, not login advice, on a bare 403', async () => { + process.env[API_KEY_ENV_VAR] = 'hmok_lowperm'; + setWorkspaceContext({ workspaceId: 'ws_abc12345' }); + vi.stubGlobal('fetch', vi.fn(async () => ({ + ok: false, + status: 403, + headers: new Headers(), + json: async () => ({ message: 'Forbidden' }), + }))); + + await expect(apiClient('/channels')).rejects.toThrow(ForbiddenError); + await expect(apiClient('/channels')).rejects.toThrow( + /HOOKMYAPP_API_KEY lacks permission for workspace ws_abc12345/, + ); + await expect(apiClient('/channels')).rejects.not.toThrow(/hookmyapp login/); + setWorkspaceContext({ workspaceId: null }); + }); + + it('sends the env key as the Authorization bearer', async () => { + process.env[API_KEY_ENV_VAR] = 'hmok_livekey123'; + const fetchMock = vi.fn(async (_url: string, _init: RequestInit) => ({ + ok: true, + status: 200, + headers: new Headers(), + json: async () => [{ id: 'ws_abc12345' }], + })); + vi.stubGlobal('fetch', fetchMock); + + const res = await apiClient('/workspaces'); + + expect(res).toEqual([{ id: 'ws_abc12345' }]); + const headers = fetchMock.mock.calls[0]![1].headers as Record; + expect(headers.Authorization).toBe('Bearer hmok_livekey123'); + // One call only: an agent credential has no refresh token, so nothing + // should have tried to reach WorkOS first. + expect(fetchMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/api/client.ts b/src/api/client.ts index 7820eba6..461af485 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -20,6 +20,7 @@ import { getEffectiveWorkosClientId, } from '../config/env-profiles.js'; import { buildVersionHeaders } from './version-headers.js'; +import { API_KEY_ENV_VAR } from '../config/env-vars.js'; // Module-level workspace context populated by the top-level CLI entry after // parsing --workspace. Explicit options.workspaceId on a specific apiClient() @@ -312,9 +313,14 @@ export function describeFetchError(err: unknown): string { export async function apiClient( path: string, - options?: RequestInit & { workspaceId?: string }, + // `bearerToken` pins the request to one credential instead of the resolved + // one. Only logout needs it: it must authenticate as the STORED key to + // revoke it, which the env key would otherwise shadow (AIT-438). + options?: RequestInit & { workspaceId?: string; bearerToken?: string }, ): Promise { - const creds = await readCredentials(); + const creds = options?.bearerToken + ? ({ accessToken: options.bearerToken, refreshToken: '', expiresAt: 0, kind: 'agent' } as const) + : await readCredentials(); if (!creds) { throw new AuthError('Not logged in. Run: hookmyapp login'); } @@ -335,7 +341,7 @@ export async function apiClient( const baseUrl = getEffectiveApiUrl(); - const { workspaceId, ...fetchOptions } = options ?? {}; + const { workspaceId, bearerToken: _bearerToken, ...fetchOptions } = options ?? {}; const headers: Record = { Authorization: `Bearer ${accessToken}`, @@ -383,7 +389,31 @@ export async function apiClient( } if (!res.ok) { - throw await mapApiError(res); + const err = await mapApiError(res); + // A 401 on an env credential must not say "Session expired. Run: login": + // there is no session, and `login` refuses to run while the variable is + // set, so that guidance is a loop (AIT-438). + if ('source' in creds && creds.source === 'env') { + if (err instanceof AuthError) { + throw new AuthError( + `The API key in ${API_KEY_ENV_VAR} was rejected (invalid or revoked). Replace it or unset the variable.`, + ); + } + // A bare 403 maps to PermissionError, whose message names the persisted + // workspace slug and says "run: hookmyapp login" — wrong on both counts + // here: login refuses while the variable is set, and the workspace in + // play may have come from the environment too. + if (err instanceof PermissionError) { + // resolvedWsId already folds in options.workspaceId, which beats the + // shared context for this specific call. + const ws = resolvedWsId ?? '(unresolved)'; + throw new ForbiddenError( + `The API key in ${API_KEY_ENV_VAR} lacks permission for workspace ${ws}. Use a key with the required role, or unset the variable to use your stored login.`, + 'AGENT_KEY_FORBIDDEN', + ); + } + } + throw err; } // 204 No Content (and other empty-body 2xx responses) have no JSON to parse. diff --git a/src/auth/__tests__/env-api-key.test.ts b/src/auth/__tests__/env-api-key.test.ts new file mode 100644 index 00000000..afe559e9 --- /dev/null +++ b/src/auth/__tests__/env-api-key.test.ts @@ -0,0 +1,112 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { readCredentials, readEnvCredential } from '../store.js'; +import { API_KEY_ENV_VAR } from '../../config/env-vars.js'; +import * as secrets from '../../storage/secrets.js'; +import { AuthError } from '../../output/error.js'; + +const STORED = { + accessToken: 'stored-token', + refreshToken: 'r', + expiresAt: 0, +} as const; + +describe('HOOKMYAPP_API_KEY credential (AIT-438)', () => { + const original = process.env[API_KEY_ENV_VAR]; + + beforeEach(() => { + delete process.env[API_KEY_ENV_VAR]; + vi.restoreAllMocks(); + }); + afterEach(() => { + if (original === undefined) delete process.env[API_KEY_ENV_VAR]; + else process.env[API_KEY_ENV_VAR] = original; + }); + + it('is used when no credential is stored', async () => { + vi.spyOn(secrets, 'readSecrets').mockResolvedValue(null); + process.env[API_KEY_ENV_VAR] = 'hmok_abc123'; + const creds = await readCredentials(); + expect(creds?.accessToken).toBe('hmok_abc123'); + // Shaped as an agent credential so refresh/rescope stay no-ops. + expect(creds?.kind).toBe('agent'); + expect(creds?.source).toBe('env'); + }); + + it('outranks a stored credential', async () => { + vi.spyOn(secrets, 'readSecrets').mockResolvedValue({ ...STORED }); + process.env[API_KEY_ENV_VAR] = 'hmok_abc123'; + expect((await readCredentials())?.accessToken).toBe('hmok_abc123'); + }); + + it('leaves the stored credential in charge when unset', async () => { + vi.spyOn(secrets, 'readSecrets').mockResolvedValue({ ...STORED }); + const creds = await readCredentials(); + expect(creds?.accessToken).toBe('stored-token'); + expect(creds?.source).toBeUndefined(); + }); + + it('accepts legacy ac_ keys, which the backend still resolves', () => { + process.env[API_KEY_ENV_VAR] = 'ac_legacy'; + expect(readEnvCredential()?.accessToken).toBe('ac_legacy'); + }); + + it('ignores an empty or whitespace-only value', () => { + process.env[API_KEY_ENV_VAR] = ' '; + expect(readEnvCredential()).toBeNull(); + }); + + // `set HOOKMYAPP_API_KEY=""` on cmd.exe stores the two quote characters. + // Every caller must agree that is "unset", or login refuses while no + // credential exists. + it.each(['""', "''", '" "'])('treats %s as unset everywhere', async (value) => { + process.env[API_KEY_ENV_VAR] = value; + expect(readEnvCredential()).toBeNull(); + const { envApiKey } = await import('../../config/env-vars.js'); + expect(envApiKey()).toBe(''); + }); + + // cmd.exe keeps the quotes in `set HOOKMYAPP_API_KEY="hmok_abc"`, unlike + // PowerShell and POSIX shells — without stripping, Windows users get + // "not a valid API key" for a key that plainly starts with hmok_. + it('accepts a value quoted the way cmd.exe stores it', () => { + process.env[API_KEY_ENV_VAR] = '"hmok_abc123"'; + expect(readEnvCredential()?.accessToken).toBe('hmok_abc123'); + process.env[API_KEY_ENV_VAR] = "'hmok_abc123'"; + expect(readEnvCredential()?.accessToken).toBe('hmok_abc123'); + }); + + it('leaves an unbalanced quote alone, so it still fails as malformed', () => { + process.env[API_KEY_ENV_VAR] = '"hmok_abc123'; + expect(() => readEnvCredential()).toThrow(AuthError); + }); + + it('gives each env key its own notification-cache fingerprint', async () => { + const { credentialFingerprint } = await import('../../notifications-nudge.js'); + const a = credentialFingerprint({ + accessToken: 'hmok_keyA', refreshToken: '', expiresAt: 0, kind: 'agent', source: 'env', + }); + const b = credentialFingerprint({ + accessToken: 'hmok_keyB', refreshToken: '', expiresAt: 0, kind: 'agent', source: 'env', + }); + expect(a).not.toBe(b); + expect(a).not.toBe('unknown'); + // Derived, never the secret itself. + expect(a).not.toContain('hmok_keyA'); + }); + + it('rejects a malformed value, naming the variable', () => { + process.env[API_KEY_ENV_VAR] = 'not-a-key'; + expect(() => readEnvCredential()).toThrow(AuthError); + expect(() => readEnvCredential()).toThrow(/HOOKMYAPP_API_KEY/); + }); + + it('never echoes the key in the malformed-value error', () => { + process.env[API_KEY_ENV_VAR] = 'not-a-key-supersecret'; + try { + readEnvCredential(); + throw new Error('expected a throw'); + } catch (err) { + expect((err as Error).message).not.toContain('supersecret'); + } + }); +}); diff --git a/src/auth/__tests__/logout.test.ts b/src/auth/__tests__/logout.test.ts index 9320dc4d..58d276d4 100644 --- a/src/auth/__tests__/logout.test.ts +++ b/src/auth/__tests__/logout.test.ts @@ -15,12 +15,17 @@ vi.mock('../../commands/mcp.js', () => ({ removeClaudeMcp: removeClaudeMcpMock } let DIR: string; const SAVED = process.env.HOOKMYAPP_CONFIG_DIR; +// HOOKMYAPP_API_KEY is a supported auth mechanism now, so a CI runner may well +// have one exported. Every baseline expectation here assumes no env +// credential; clear it per test and let the env-specific cases set their own. +const SAVED_KEY = process.env.HOOKMYAPP_API_KEY; let logSpy: ReturnType; beforeEach(() => { removeClaudeMcpMock.mockReset().mockReturnValue({ ok: true }); DIR = mkdtempSync(join(tmpdir(), 'hma-logout-')); process.env.HOOKMYAPP_CONFIG_DIR = DIR; + delete process.env.HOOKMYAPP_API_KEY; logSpy = vi.spyOn(console, 'log').mockReturnValue(undefined); }); @@ -28,6 +33,8 @@ afterEach(() => { rmSync(DIR, { recursive: true, force: true }); if (SAVED) process.env.HOOKMYAPP_CONFIG_DIR = SAVED; else delete process.env.HOOKMYAPP_CONFIG_DIR; + if (SAVED_KEY === undefined) delete process.env.HOOKMYAPP_API_KEY; + else process.env.HOOKMYAPP_API_KEY = SAVED_KEY; vi.restoreAllMocks(); }); @@ -69,12 +76,94 @@ describe('logout', () => { expect(JSON.parse(written.trim())).toEqual({ status: 'logged_out', revoked: false, + envKeyActive: false, mcpCleanup: { ok: true }, }); // The human check line must NOT be printed in --json mode. expect(logSpy.mock.calls.flat().join('')).not.toMatch(/Logged out/); }); + // AIT-438: an env key keeps authenticating after logout. Automation reads + // the payload, not the stderr warning, so the signal has to be in the JSON. + test('--json flags a still-active HOOKMYAPP_API_KEY', async () => { + process.env.HOOKMYAPP_API_KEY = 'hmok_stillhere'; + const credsPath = join(DIR, 'credentials.json'); + writeFileSync(credsPath, JSON.stringify({ accessToken: 'a', refreshToken: 'r', expiresAt: 1 })); + const stdoutSpy = vi.spyOn(process.stdout, 'write').mockReturnValue(true); + + { + await runLogout(['--json']); + const written = stdoutSpy.mock.calls.map((c) => String(c[0])).join(''); + expect(JSON.parse(written.trim())).toMatchObject({ + status: 'logged_out_with_warning', + envKeyActive: true, + envKeyVar: 'HOOKMYAPP_API_KEY', + }); + } + }); + + // AIT-438: the revoke goes through apiClient, which authenticates with the + // env key while it is set — a "self-revoke" would kill the credential every + // other process is sharing. + test('skips the server-side revoke while HOOKMYAPP_API_KEY is set', async () => { + process.env.HOOKMYAPP_API_KEY = 'hmok_stillhere'; + writeFileSync( + join(DIR, 'credentials.json'), + JSON.stringify({ + accessToken: 'hmok_stillhere', + refreshToken: '', + expiresAt: 0, + kind: 'agent', + credentialPublicId: 'ac_self0001', + }), + ); + const stdoutSpy = vi.spyOn(process.stdout, 'write').mockReturnValue(true); + + { + await runLogout(['--json']); + const payload = JSON.parse( + stdoutSpy.mock.calls.map((c) => String(c[0])).join('').trim(), + ); + expect(payload.revoked).toBe(false); + expect(payload.envKeyActive).toBe(true); + expect(payload.envKeyIsStoredKey).toBe(true); + expect(existsSync(join(DIR, 'credentials.json'))).toBe(false); + } + }); + + // A different env key must not stop logout from revoking the stored one: + // the request is pinned to the stored token, so it is a real self-revoke. + test('still revokes a stored key when the env holds a DIFFERENT key', async () => { + process.env.HOOKMYAPP_API_KEY = 'hmok_otherkey'; + writeFileSync( + join(DIR, 'credentials.json'), + JSON.stringify({ + accessToken: 'hmok_storedkey', + refreshToken: '', + expiresAt: 0, + kind: 'agent', + credentialPublicId: 'ac_stored01', + }), + ); + const stdoutSpy = vi.spyOn(process.stdout, 'write').mockReturnValue(true); + const fetchMock = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); + vi.stubGlobal('fetch', fetchMock); + + { + await runLogout(['--json']); + const payload = JSON.parse( + stdoutSpy.mock.calls.map((c) => String(c[0])).join('').trim(), + ); + expect(payload.envKeyIsStoredKey).toBe(false); + const call = fetchMock.mock.calls.find(([url]) => + String(url).includes('/agent/credentials/ac_stored01'), + ); + expect(call).toBeDefined(); + // Pinned to the stored key, NOT the env key that would otherwise win. + expect(call![1].headers.Authorization).toBe('Bearer hmok_storedkey'); + } + }); + test('reports MCP cleanup failure after credentials are removed', async () => { removeClaudeMcpMock.mockReturnValue({ ok: false, detail: 'Claude MCP cleanup timed out' }); const stdoutSpy = vi.spyOn(process.stdout, 'write').mockReturnValue(true); diff --git a/src/auth/login.ts b/src/auth/login.ts index ae5807b0..46a9f9b1 100644 --- a/src/auth/login.ts +++ b/src/auth/login.ts @@ -1,5 +1,6 @@ import { Command } from 'commander'; import { saveCredentials, peekIdentity } from './store.js'; +import { API_KEY_ENV_VAR, envApiKey } from '../config/env-vars.js'; import { AuthError, NetworkError, ValidationError } from '../output/error.js'; import { addExamples } from '../output/help.js'; import { c, icon } from '../output/color.js'; @@ -692,6 +693,16 @@ export function loginCommand(program: Command): void { registrationId?: string; scope?: string[]; }) => { + // AIT-438: HOOKMYAPP_API_KEY outranks the stored credential, so a + // login completed now would be authenticated over and have no effect. + // Refuse with the fix rather than let the user sign in for nothing + // (same contract as `gh auth login` under GH_TOKEN). + if (envApiKey()) { + throw new ValidationError( + `${API_KEY_ENV_VAR} is set, and it takes precedence over a stored login — signing in now would have no effect. ` + + `Unset ${API_KEY_ENV_VAR} first, then run: ${cliCommandPrefix()} login`, + ); + } // Reject an invalid --next locally (exit 2) instead of silently // coercing it to undefined — `login --next bogus` previously ran the // default flow and exited 0, hiding the typo. diff --git a/src/auth/logout.ts b/src/auth/logout.ts index eeb8db42..b453dbe1 100644 --- a/src/auth/logout.ts +++ b/src/auth/logout.ts @@ -1,6 +1,7 @@ import { Command } from 'commander'; -import { readCredentials, deleteCredentials } from './store.js'; -import { isAgentCredential } from '../storage/secrets.js'; +import { deleteCredentials } from './store.js'; +import { API_KEY_ENV_VAR, envApiKey } from '../config/env-vars.js'; +import { isAgentCredential, readSecrets } from '../storage/secrets.js'; import { addExamples } from '../output/help.js'; import { removeClaudeMcp } from '../commands/mcp.js'; @@ -10,6 +11,10 @@ export function logoutCommand(program: Command): void { .description('Remove stored credentials') .action(async () => { const json = !!program.opts().json; + // AIT-438: an env key keeps authenticating after logout. Humans get the + // warning below; --json callers need the same signal in the payload, or + // automation reads status "logged_out" and assumes it is signed out. + const envKeyActive = Boolean(envApiKey()); // AIT-153: for an agent credential (org API key), also revoke it // server-side so it can't keep being used after logout. Best-effort — an @@ -17,12 +22,23 @@ export function logoutCommand(program: Command): void { // credentials. WorkOS sessions carry no CLI-side revoke, so this only // fires for agent credentials. let revoked = false; - const creds = await readCredentials(); - if (creds && isAgentCredential(creds) && creds.credentialPublicId) { + // Stored credential only (AIT-438): logout manages credentials.json and + // must never revoke a key that came from HOOKMYAPP_API_KEY — the + // environment is not ours to clear, and revoking it server-side would + // break every other process sharing that key. + const creds = await readSecrets(); + // The revoke goes through apiClient, which prefers the env key. Pin it + // to the stored token so logout revokes the credential it is actually + // clearing. The one case to skip: the env holds that SAME key — revoking + // it would break every other process sharing it, and the user did not + // ask to invalidate their environment (AIT-438). + const envIsSameKey = envKeyActive && creds?.accessToken === envApiKey(); + if (creds && isAgentCredential(creds) && creds.credentialPublicId && !envIsSameKey) { try { const { apiClient } = await import('../api/client.js'); await apiClient(`/agent/credentials/${creds.credentialPublicId}`, { method: 'DELETE', + bearerToken: creds.accessToken, }); revoked = true; } catch { @@ -36,8 +52,13 @@ export function logoutCommand(program: Command): void { if (json) { process.stdout.write( JSON.stringify({ - status: mcpCleanup.ok ? 'logged_out' : 'logged_out_with_warning', + status: + !mcpCleanup.ok || envKeyActive + ? 'logged_out_with_warning' + : 'logged_out', revoked, + envKeyActive, + ...(envKeyActive ? { envKeyVar: API_KEY_ENV_VAR, envKeyIsStoredKey: envIsSameKey } : {}), mcpCleanup, }) + '\n', ); @@ -47,6 +68,13 @@ export function logoutCommand(program: Command): void { ? '\n✓ Logged out\n' : `\n✓ Logged out\n⚠ ${mcpCleanup.detail}\n`, ); + if (envKeyActive) { + console.log( + envIsSameKey + ? `⚠ ${API_KEY_ENV_VAR} holds this same key, so it was not revoked and commands stay authenticated with it. Unset it to sign out fully.\n` + : `⚠ ${API_KEY_ENV_VAR} is still set — commands stay authenticated with it. Unset it to sign out fully.\n`, + ); + } } }); diff --git a/src/auth/store.ts b/src/auth/store.ts index 0c5c1e73..ccef1a53 100644 --- a/src/auth/store.ts +++ b/src/auth/store.ts @@ -1,5 +1,7 @@ import { readFileSync } from 'node:fs'; import { getConfigFile } from '../storage/path.js'; +import { AuthError } from '../output/error.js'; +import { API_KEY_ENV_VAR, envApiKey } from '../config/env-vars.js'; import { writeSecrets, readSecrets, @@ -17,8 +19,55 @@ export async function saveCredentials(creds: Credentials): Promise { await writeSecrets(creds); } +/** + * Build a credential from HOOKMYAPP_API_KEY, or null when the variable is + * unset/empty. + * + * Shaped as `kind: 'agent'` on purpose: an org API key is exactly what the + * auth.md flow (`login --email … --scope …`) persists, so every existing + * agent-credential path (no refresh, no rescope, bearer sent verbatim) + * applies unchanged. + * + * Accepted prefixes mirror the backend's `isAgentToken` — `hmok_` is the + * current mint, `ac_` is legacy and still resolves server-side. Rejecting + * `ac_` here would fail keys the API accepts. + * + * Throws rather than returning null on a malformed value: a set-but-wrong + * variable is a configuration mistake, and silently falling through to + * "Not logged in. Run: hookmyapp login" hides where the bad key came from. + */ +export function readEnvCredential(): Credentials | null { + const raw = envApiKey(); + if (!raw) return null; + if (!raw.startsWith('hmok_') && !raw.startsWith('ac_')) { + throw new AuthError( + `${API_KEY_ENV_VAR} is not a valid API key (expected it to start with "hmok_"). ` + + `Fix or unset the variable, then retry.`, + ); + } + return { + accessToken: raw, + refreshToken: '', + expiresAt: 0, + kind: 'agent', + source: 'env', + }; +} + +/** + * Resolve the credential every authenticated code path uses. + * + * Precedence: HOOKMYAPP_API_KEY beats the stored credential, matching gh + * (GH_TOKEN), aws, vercel and stripe — an explicitly exported key is the more + * deliberate signal, and the reverse order silently ignores the key an agent + * or CI job was handed. The override is reported by `doctor`, and `login` + * refuses to run while the variable is set, so it is never invisible. + * + * `login`/`logout` deliberately bypass this and use readSecrets() directly: + * they manage the stored credential and must not act on the environment. + */ export async function readCredentials(): Promise { - return readSecrets(); + return readEnvCredential() ?? (await readSecrets()); } export async function deleteCredentials(): Promise { diff --git a/src/commands/__tests__/credentials.test.ts b/src/commands/__tests__/credentials.test.ts index 535b7477..e1fba548 100644 --- a/src/commands/__tests__/credentials.test.ts +++ b/src/commands/__tests__/credentials.test.ts @@ -83,6 +83,27 @@ test('revoking the currently stored credential clears it from disk', async () => expect(existsSync(join(DIR, 'credentials.json'))).toBe(false); }); +// AIT-438: with the same key also in HOOKMYAPP_API_KEY, reading the resolved +// credential returns the synthesized env one, which has no credentialPublicId +// — the match fails and a revoked token stays on disk, 401ing the moment the +// variable is unset. +test('clears the stored credential even when the env holds the same key', async () => { + const original = process.env.HOOKMYAPP_API_KEY; + process.env.HOOKMYAPP_API_KEY = 'hmok_same'; + writeFileSync( + join(DIR, 'credentials.json'), + JSON.stringify({ accessToken: 'hmok_same', refreshToken: '', expiresAt: 0, kind: 'agent', credentialPublicId: 'ac_pub1', scopes: [] }), + ); + apiClientMock.mockResolvedValue(undefined); + try { + await run(['credentials', 'revoke', 'ac_pub1', '-y', '--json']); + expect(existsSync(join(DIR, 'credentials.json'))).toBe(false); + } finally { + if (original === undefined) delete process.env.HOOKMYAPP_API_KEY; + else process.env.HOOKMYAPP_API_KEY = original; + } +}); + test('revoking a different credential leaves the stored one intact', async () => { writeFileSync( join(DIR, 'credentials.json'), diff --git a/src/commands/__tests__/doctor.test.ts b/src/commands/__tests__/doctor.test.ts index edce84ad..72d4c36b 100644 --- a/src/commands/__tests__/doctor.test.ts +++ b/src/commands/__tests__/doctor.test.ts @@ -45,10 +45,41 @@ describe('doctor — auth probe uses the real authenticated request path', () => expect(apiClient).toHaveBeenCalledWith('/workspaces'); expect(report.loggedIn).toBe(true); expect(report.checks.find((c) => c.id === 'auth')!.detail).toBe( - 'credentials valid for this env', + 'credentials valid for this env — stored credentials', ); }); + // AIT-438: which credential is in play must be visible — an env key and a + // stored login produce identical output otherwise. + it('names HOOKMYAPP_API_KEY when the credential came from the environment', async () => { + vi.mocked(apiClient).mockResolvedValue([]); + vi.mocked(readCredentials).mockResolvedValue({ + accessToken: 'hmok_abc', + refreshToken: '', + expiresAt: 0, + kind: 'agent', + source: 'env', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + + const report = await collectDoctorReport({ checkTools: false }); + + expect(report.checks.find((c) => c.id === 'auth')!.detail).toBe( + 'credentials valid for this env — HOOKMYAPP_API_KEY (environment)', + ); + }); + + it('surfaces a malformed env key instead of "not logged in"', async () => { + vi.mocked(readCredentials).mockRejectedValue( + new AuthError('HOOKMYAPP_API_KEY is not a valid API key (expected it to start with "hmok_").'), + ); + + const report = await collectDoctorReport({ checkTools: false }); + + expect(report.loggedIn).toBe(false); + expect(report.checks.find((c) => c.id === 'auth')!.detail).toContain('HOOKMYAPP_API_KEY'); + }); + it('fails auth when apiClient throws AuthError (genuinely invalid credentials)', async () => { vi.mocked(apiClient).mockRejectedValue(new AuthError()); @@ -67,7 +98,9 @@ describe('doctor — auth probe uses the real authenticated request path', () => const report = await collectDoctorReport({ checkTools: false }); expect(report.loggedIn).toBe(true); - expect(report.checks.find((c) => c.id === 'auth')!.detail).toBe('credentials present'); + expect(report.checks.find((c) => c.id === 'auth')!.detail).toBe( + 'credentials present — stored credentials', + ); }); }); diff --git a/src/commands/_helpers.ts b/src/commands/_helpers.ts index e2bf597b..02cafa42 100644 --- a/src/commands/_helpers.ts +++ b/src/commands/_helpers.ts @@ -110,7 +110,8 @@ export async function getDefaultWorkspaceId(): Promise { if (Array.isArray(workspaces) && workspaces.length > 1) { throw new ValidationError( - `You're a member of ${workspaces.length} workspaces. Pick one first:\n hookmyapp workspace use `, + `You're a member of ${workspaces.length} workspaces. Pick one first:\n hookmyapp workspace use \n` + + ' (non-interactive: pass --workspace )', ); } diff --git a/src/commands/credentials.ts b/src/commands/credentials.ts index 1a898f80..615e1d42 100644 --- a/src/commands/credentials.ts +++ b/src/commands/credentials.ts @@ -1,7 +1,7 @@ import { Command } from 'commander'; import { apiClient } from '../api/client.js'; -import { readCredentials, deleteCredentials } from '../auth/store.js'; -import { isAgentCredential } from '../storage/secrets.js'; +import { deleteCredentials } from '../auth/store.js'; +import { isAgentCredential, readSecrets } from '../storage/secrets.js'; import { addExamples } from '../output/help.js'; interface AgentCredentialRow { @@ -60,7 +60,12 @@ export function registerCredentialsCommand(program: Command): void { }); // If this is the credential we're currently authenticated with, drop the // now-dead token from disk so the next command doesn't send a 401. - const creds = await readCredentials(); + // Read the STORED credential, not the resolved one: with the same key + // also in HOOKMYAPP_API_KEY, the synthesized env credential carries no + // credentialPublicId, the comparison never matches, and credentials.json + // keeps a revoked token that 401s as soon as the variable is unset + // (AIT-438). + const creds = await readSecrets(); if (creds && isAgentCredential(creds) && creds.credentialPublicId === publicId) { await deleteCredentials(); } diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 0cd848fd..1c6bde24 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -1,6 +1,7 @@ import type { Command } from 'commander'; import { runTool } from '../lib/spawn-tool.js'; import { readCredentials } from '../auth/store.js'; +import { API_KEY_ENV_VAR } from '../config/env-vars.js'; import { apiClient } from '../api/client.js'; import { AuthError, ForbiddenError, PermissionError } from '../output/error.js'; import { isJsonMode } from '../output/format.js'; @@ -56,7 +57,17 @@ export async function collectDoctorReport( let loggedIn = false; let creds: Awaited> = null; - try { creds = await readCredentials(); loggedIn = creds !== null; } catch { loggedIn = false; } + // A malformed HOOKMYAPP_API_KEY throws here (AIT-438). Keep its message — + // "not logged in" would send the user to `login` when the real fix is to + // correct the variable. + let credsError: string | null = null; + try { + creds = await readCredentials(); + loggedIn = creds !== null; + } catch (err) { + loggedIn = false; + credsError = err instanceof Error ? err.message : String(err); + } // Credentials-present alone is a false positive when the stored token is // expired or belongs to another env (2026-07-07 audit: doctor said OK, the // next command 401'd). When network checks are on, prove the token works @@ -64,7 +75,10 @@ export async function collectDoctorReport( // command uses, including the token-refresh attempt. A raw fetch with the // stored accessToken false-FAILs on tokens that are merely expired but // refreshable (2026-07-08 audit). - let authDetail = loggedIn ? 'credentials present' : 'not logged in — run: hookmyapp login'; + const credSource = creds?.source === 'env' ? `${API_KEY_ENV_VAR} (environment)` : 'stored credentials'; + let authDetail = loggedIn + ? `credentials present — ${credSource}` + : credsError ?? 'not logged in — run: hookmyapp login'; // Kept from the auth probe so the workspace check below can validate the // persisted activeWorkspaceId against the backend instead of trusting the // cache (AIT-51: after a DB re-seed doctor said OK while every scoped @@ -74,13 +88,23 @@ export async function collectDoctorReport( try { const res = await apiClient('/workspaces'); if (Array.isArray(res)) workspaces = res; - authDetail = 'credentials valid for this env'; + authDetail = `credentials valid for this env — ${credSource}`; } catch (err) { // ForbiddenError is what coded 403s map to since AIT-151 — a denial is // still a rejected credential, not a network flake. if (err instanceof AuthError || err instanceof PermissionError || err instanceof ForbiddenError) { loggedIn = false; - authDetail = 'credentials present but rejected by this env — run: hookmyapp login'; + // Only a 401 means the credential itself is bad. A 403 is an + // authenticated principal without permission — telling the user their + // key is revoked would send them to replace a perfectly good key. + const denied = !(err instanceof AuthError); + authDetail = creds?.source === 'env' + ? denied + ? `credentials present but denied on this env — the key in ${API_KEY_ENV_VAR} lacks permission for this workspace` + : `credentials present but rejected by this env — the key in ${API_KEY_ENV_VAR} is invalid or revoked` + : denied + ? 'credentials present but denied on this env — this account lacks permission for this workspace' + : 'credentials present but rejected by this env — run: hookmyapp login'; } // Anything else (network flake, 5xx) — leave the presence-based verdict. } diff --git a/src/config/env-vars.ts b/src/config/env-vars.ts new file mode 100644 index 00000000..6a3dd903 --- /dev/null +++ b/src/config/env-vars.ts @@ -0,0 +1,46 @@ +import { createHash } from 'node:crypto'; + +/** + * Name of the environment variable the CLI reads for credentials. Kept in a leaf module (no imports) so consumers can + * pull the name without dragging in the auth store — several test suites + * mock `auth/store.js` wholesale, and a constant exported from there would + * break every one of them. + */ +export const API_KEY_ENV_VAR = 'HOOKMYAPP_API_KEY'; + +/** + * Drop one layer of matching surrounding quotes from an env-var value. + * + * cmd.exe stores `set VAR="value"` with the quotes included, unlike PowerShell + * and POSIX shells, so a Windows user following the documented instructions + * ends up with a value no prefix/shape check would accept. + */ +export function stripEnvQuotes(value: string): string { + if (value.length >= 2) { + const first = value[0]; + if ((first === '"' || first === "'") && value[value.length - 1] === first) { + return value.slice(1, -1).trim(); + } + } + return value; +} + +/** + * The active org API key from the environment, normalized ('' when unset). + * Every caller must go through this: `HOOKMYAPP_API_KEY='""'` strips to empty, + * so a raw `process.env[...]` truthiness test would refuse a login (or claim a + * key is active) when no credential exists at all. + */ +export function envApiKey(): string { + return stripEnvQuotes(process.env[API_KEY_ENV_VAR]?.trim() ?? ''); +} + +/** + * Stable, non-secret id derived from an opaque API key. Used wherever a key + * needs an identity — telemetry distinct-id, notification cache key — because + * an hmok_ token carries no publicId, no JWT claims and no email. Never + * logged, transmitted, or reversible to the key. + */ +export function keyFingerprint(token: string): string { + return createHash('sha256').update(token).digest('hex').slice(0, 16); +} diff --git a/src/notifications-nudge.ts b/src/notifications-nudge.ts index 3b0a5b72..64422c15 100644 --- a/src/notifications-nudge.ts +++ b/src/notifications-nudge.ts @@ -28,6 +28,7 @@ import { getValidAccessToken } from './api/client.js'; import { readCredentials } from './auth/store.js'; import type { Secrets } from './storage/secrets.js'; import { ENV_PROFILES, getEffectiveApiUrl, isValidEnv } from './config/env-profiles.js'; +import { keyFingerprint } from './config/env-vars.js'; import { getConfigDir } from './storage/path.js'; const DAY_MS = 24 * 60 * 60 * 1000; @@ -45,6 +46,13 @@ export interface NotificationsCache { /** Non-secret identity for cache namespacing. Never a raw token. */ export function credentialFingerprint(creds: Secrets): string { if (creds.credentialPublicId) return creds.credentialPublicId; + // An opaque env key (AIT-438) carries no publicId, no JWT claims and no + // email, so every such key would collapse to 'unknown' and share one cache + // file — one key's unread state and 24h throttle shown for another. Hash the + // token into a stable, non-secret id instead. Never logged or transmitted. + if (creds.source === 'env') { + return `env:${keyFingerprint(creds.accessToken)}`; + } try { // WorkOS sessions: `sub` (stable user id) + `org_id` (session org scope) // — both non-secret. org_id matters: the same user re-scoped to another diff --git a/src/observability/__tests__/env-key-identity.test.ts b/src/observability/__tests__/env-key-identity.test.ts new file mode 100644 index 00000000..85d531c6 --- /dev/null +++ b/src/observability/__tests__/env-key-identity.test.ts @@ -0,0 +1,34 @@ +import { describe, it, expect, afterEach } from 'vitest'; +import { getDistinctId } from '../posthog.js'; +import { API_KEY_ENV_VAR } from '../../config/env-vars.js'; + +// AIT-438: lastWorkosSub outlives a human's session, so without a key-specific +// identity every CI/agent event authenticated by HOOKMYAPP_API_KEY is merged +// into whoever last logged in on that machine. +describe('telemetry identity under HOOKMYAPP_API_KEY', () => { + afterEach(() => { + delete process.env[API_KEY_ENV_VAR]; + }); + + it('uses a key-specific distinct id, not the persisted human sub', () => { + const withoutKey = getDistinctId(); + process.env[API_KEY_ENV_VAR] = 'hmok_agentkey'; + const withKey = getDistinctId(); + + expect(withKey).not.toBe(withoutKey); + expect(withKey).toMatch(/^key_[0-9a-f]{16}$/); + // Derived, never the secret itself. + expect(withKey).not.toContain('agentkey'); + }); + + it('is stable for one key and distinct across keys', () => { + process.env[API_KEY_ENV_VAR] = 'hmok_a'; + const a1 = getDistinctId(); + const a2 = getDistinctId(); + process.env[API_KEY_ENV_VAR] = 'hmok_b'; + const b = getDistinctId(); + + expect(a1).toBe(a2); + expect(a1).not.toBe(b); + }); +}); diff --git a/src/observability/posthog.ts b/src/observability/posthog.ts index c22df578..2448f25f 100644 --- a/src/observability/posthog.ts +++ b/src/observability/posthog.ts @@ -43,6 +43,7 @@ import { readActiveWorkspacePublicId, } from '../config/index.js'; import { resolveEnv } from '../config/env-profiles.js'; +import { envApiKey, keyFingerprint } from '../config/env-vars.js'; import { nanoid } from 'nanoid'; import type { EventName, EventProperties } from '../analytics/events.js'; @@ -149,6 +150,11 @@ export function getOrCreateMachineId(): string { * captures. */ export function getDistinctId(): string { + // An environment key is a different principal from whoever last logged in on + // this machine. Without this, CI and agent activity merges into that human's + // PostHog profile, because lastWorkosSub outlives their session (AIT-438). + const envKey = envApiKey(); + if (envKey) return `key_${keyFingerprint(envKey)}`; const cfg = readPosthogConfig(); return cfg.lastWorkosSub ?? getOrCreateMachineId(); } diff --git a/src/storage/secrets.ts b/src/storage/secrets.ts index da4ecb7a..c99f3604 100644 --- a/src/storage/secrets.ts +++ b/src/storage/secrets.ts @@ -33,6 +33,9 @@ export interface Secrets { /** Agent credentials only: login email — the hmok_ token is opaque, so this * is the only human identity available for crash attribution (AIT-278). */ email?: string; + /** Where the credential came from. Undefined = read from credentials.json. + * 'env' = synthesized from HOOKMYAPP_API_KEY and never persisted (AIT-438). */ + source?: 'env'; } /** True for an auth.md-issued org-scoped agent credential (no refresh token). */ diff --git a/vitest.setup.ts b/vitest.setup.ts index 22070a8a..6af57e40 100644 --- a/vitest.setup.ts +++ b/vitest.setup.ts @@ -10,6 +10,13 @@ if (!process.env.HOOKMYAPP_CONFIG_DIR) { process.env.HOOKMYAPP_CONFIG_DIR = mkdtempSync(join(tmpdir(), 'hookmyapp-cli-test-')); } +// Unset the credential env var for the whole suite. It is a supported auth +// mechanism (AIT-438), so a developer or CI runner may legitimately have one +// exported — and it outranks the stored credential, which silently flips every +// test that assumes "not logged in" or writes its own credentials.json. Tests +// that exercise the variable set it themselves. +delete process.env.HOOKMYAPP_API_KEY; + // Pin color output OFF for the whole suite. picocolors latches color support // from process.stdout.isTTY at first import; several tests toggle isTTY to // exercise interactive paths, which — combined with lazy dynamic imports —