Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1
AIT-438: CLI should accept an API key from the environment#64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
c7a391d2a142300958ddf1055c7ab39b25146fec885c992706efdbb6af2826c539ef1ad5eeb6ec24d736d1b30810c53db46f38140File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, string>; | ||
| 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); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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'); | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -15,19 +15,26 @@ 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<typeof vi.spyOn>; | ||
| 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); | ||
| }); | ||
| 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the test runner has Useful? React with 👍 / 👎. | ||
| 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); | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.