Make CLI doctor fail on unvalidated or ambiguous credentials

README.md

@@ -105,18 +105,21 @@ polymorfa doctor
 polymorfa doctor --profile work --json
 ```
 
-`doctor` runs five checks and exits with code 1 if any fails:
-
-| Check        | What it does                                                                                                                                                                                                                         |
-| ------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
-| `node`       | Requires Node.js 22 or newer.                                                                                                                                                                                                        |
-| `config`     | Loads and validates the profile config and reports the active profile.                                                                                                                                                               |
-| `keyring`    | Reads the selected profile's entry from the operating-system keyring. A locked or unavailable keyring fails, or warns when an environment credential is in use.                                                                      |
-| `api`        | Calls the unauthenticated `/ping` on the configured base URL, without retries.                                                                                                                                                       |
-| `credential` | For a server API key, lists projects as an authenticated ping. `401` fails; `403` warns that the key authenticated but project listing was denied. Project and client tokens are reported as present but are not validated remotely. |
-
-Environment credentials take precedence over the active profile, as for other
-commands. Output never contains credential values.
+`doctor` runs five checks and exits with code 1 if any fails or credential
+validation is skipped. A successful run needs one server API key. It does not
+change credentials or configuration.
+
+| Check        | What it does                                                                                                                                                                                                                                                                              |
+| ------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `node`       | Requires Node.js 22 or newer.                                                                                                                                                                                                                                                             |
+| `config`     | Loads and validates the profile config and reports the active profile.                                                                                                                                                                                                                    |
+| `keyring`    | Reads the selected profile's entry from the operating-system keyring. A locked or unavailable keyring fails, or warns when an environment credential is in use.                                                                                                                           |
+| `api`        | Calls the unauthenticated `/ping` on the configured base URL, without retries.                                                                                                                                                                                                            |
+| `credential` | For a server API key, lists projects as an authenticated ping. `401` fails; `403` warns that the key authenticated but project listing was denied. Missing credentials fail. Project and client tokens are reported as present, but remote validation is skipped and the command exits 1. |
+
+An explicit `--profile` selects that profile. Otherwise, one environment
+credential takes precedence over the active profile; multiple credential
+environment variables fail as ambiguous. Output never contains credential values.
 
 ## QuickLinks
 

src/commands/doctor.ts

@@ -5,7 +5,7 @@ import {runDoctor} from '../services/doctor.js'
 
 export default class Doctor extends BaseCommand {
   static override description =
-    'Check Node.js, config, keyring access, API reachability, and API key validity.'
+    'Check Node.js, config, keyring access, API reachability, and server API key validity. Exits 1 if validation fails or is skipped.'
 
   static override examples = [
     '<%= config.bin %> doctor',

src/services/doctor.ts

@@ -64,12 +64,12 @@ export async function runDoctor(
   }
 
   const envKey = nonEmpty(deps.env.POLYMORFA_API_KEY)
-  const envOther =
-    nonEmpty(deps.env.POLYMORFA_PROJECT_TOKEN) ??
-    nonEmpty(deps.env.POLYMORFA_CLIENT_TOKEN)
-  const useEnv =
-    nonEmpty(deps.profile) === undefined &&
-    (envKey !== undefined || envOther !== undefined)
+  const envProjectToken = nonEmpty(deps.env.POLYMORFA_PROJECT_TOKEN)
+  const envClientToken = nonEmpty(deps.env.POLYMORFA_CLIENT_TOKEN)
+  const envCredentialCount = [envKey, envProjectToken, envClientToken].filter(
+    (value) => value !== undefined,
+  ).length
+  const useEnv = nonEmpty(deps.profile) === undefined && envCredentialCount > 0
   const profileName = nonEmpty(deps.profile) ?? config?.activeProfile
   const profile =
     profileName === undefined ? undefined : config?.profiles[profileName]
@@ -120,11 +120,11 @@ export async function runDoctor(
     })
   }
   if (useEnv) {
-    credential = envKey ?? envOther
+    credential = envKey ?? envProjectToken ?? envClientToken
     kind =
       envKey !== undefined
         ? 'apiKey'
-        : nonEmpty(deps.env.POLYMORFA_PROJECT_TOKEN) !== undefined
+        : envProjectToken !== undefined
           ? 'projectToken'
           : 'clientToken'
   }
@@ -151,18 +151,30 @@ export async function runDoctor(
   }
 
   checks.push(
-    await credentialCheck({
-      baseUrl,
-      credential,
-      kind,
-      lookup: deps.lookup ?? listProjectsPing,
-      reachable,
-      source: useEnv ? 'environment' : `profile ${profileName ?? '-'}`,
-      timeoutMs: deps.timeoutMs,
-    }),
+    useEnv && envCredentialCount > 1
+      ? {
+          name: 'credential',
+          status: 'fail',
+          detail:
+            'Environment credentials are ambiguous. Set only one of POLYMORFA_API_KEY, POLYMORFA_PROJECT_TOKEN, or POLYMORFA_CLIENT_TOKEN, or select --profile.',
+        }
+      : await credentialCheck({
+          baseUrl,
+          credential,
+          kind,
+          lookup: deps.lookup ?? listProjectsPing,
+          reachable,
+          source: useEnv ? 'environment' : `profile ${profileName ?? '-'}`,
+          timeoutMs: deps.timeoutMs,
+        }),
   )
 
-  return {checks, ok: checks.every((check) => check.status !== 'fail')}
+  return {
+    checks,
+    ok: checks.every(
+      (check) => check.status !== 'fail' && check.status !== 'skip',
+    ),
+  }
 }
 
 function nodeCheck(version: string): DoctorCheck {
@@ -188,7 +200,7 @@ async function credentialCheck(input: {
   if (input.credential === undefined || input.kind === undefined) {
     return {
       name: 'credential',
-      status: 'warn',
+      status: 'fail',
       detail:
         'No credential is configured. Set POLYMORFA_API_KEY or run polymorfa auth login --with-api-key.',
     }
@@ -204,7 +216,7 @@ async function credentialCheck(input: {
     return {
       name: 'credential',
       status: 'skip',
-      detail: `A ${input.kind === 'projectToken' ? 'project token' : 'client token'} from ${input.source} is present; doctor validates only server API keys remotely.`,
+      detail: `A ${input.kind === 'projectToken' ? 'project token' : 'client token'} from ${input.source} is present but was not validated. Doctor needs a server API key to verify authentication; this token's command access depends on the command.`,
     }
   }
   try {

test/commands/doctor.test.ts

@@ -0,0 +1,92 @@
+import {runCommand} from '@oclif/test'
+import {mkdtemp, rm} from 'node:fs/promises'
+import {tmpdir} from 'node:os'
+import {join} from 'node:path'
+import {
+  afterAll,
+  afterEach,
+  beforeAll,
+  beforeEach,
+  describe,
+  expect,
+  test,
+} from 'vitest'
+
+import {
+  startHttpServer,
+  withEnvironment,
+  type TestHttpServer,
+} from '../helpers/http-server.js'
+
+const API_KEY =
+  'pmfa_AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA'
+
+describe('doctor command exit status', () => {
+  let server: TestHttpServer
+  let directory: string
+
+  beforeAll(async () => {
+    server = await startHttpServer({body: {success: true, data: []}})
+    directory = await mkdtemp(join(tmpdir(), 'polymorfa-cli-doctor-'))
+  })
+
+  afterAll(async () => {
+    await server.close()
+    await rm(directory, {force: true, recursive: true})
+  })
+
+  beforeEach(() => {
+    process.exitCode = undefined
+  })
+
+  afterEach(() => {
+    process.exitCode = undefined
+  })
+
+  async function doctor(env: Record<string, string | undefined>) {
+    return withEnvironment(
+      {
+        POLYMORFA_API_KEY: undefined,
+        POLYMORFA_CLIENT_TOKEN: undefined,
+        POLYMORFA_PROJECT_TOKEN: undefined,
+        POLYMORFA_BASE_URL: server.baseUrl,
+        POLYMORFA_CONFIG_DIR: directory,
+        ...env,
+      },
+      () => runCommand('doctor --json'),
+    )
+  }
+
+  test('returns success for an authenticated server API key', async () => {
+    const result = await doctor({POLYMORFA_API_KEY: API_KEY})
+    expect(result.error, result.stderr).toBeUndefined()
+    expect(JSON.parse(result.stdout)).toMatchObject({ok: true})
+    expect(process.exitCode).toBeUndefined()
+    expect(server.requests.map((request) => request.url)).toContain(
+      '/platform/projects',
+    )
+  })
+
+  test('returns failure when credential validation is skipped', async () => {
+    const result = await doctor({POLYMORFA_CLIENT_TOKEN: 'pmfa_ct_example'})
+    expect(result.error, result.stderr).toBeUndefined()
+    expect(JSON.parse(result.stdout)).toMatchObject({ok: false})
+    expect(process.exitCode).toBe(1)
+  })
+
+  test('returns failure for ambiguous credentials without using either', async () => {
+    const requestsBefore = server.requests.length
+    const result = await doctor({
+      POLYMORFA_API_KEY: API_KEY,
+      POLYMORFA_PROJECT_TOKEN: 'private-project-token',
+    })
+    expect(result.error, result.stderr).toBeUndefined()
+    expect(JSON.parse(result.stdout)).toMatchObject({ok: false})
+    expect(process.exitCode).toBe(1)
+    expect(
+      server.requests.slice(requestsBefore).map((request) => request.url),
+    ).toEqual(['/ping'])
+    expect(result.stdout).not.toContain(API_KEY)
+    expect(result.stdout).not.toContain('private-project-token')
+  })
+})

test/services/doctor.test.ts

@@ -84,7 +84,7 @@ describe('doctor', () => {
         },
       }),
     )
-    expect(status(report)).toMatchObject({credential: 'warn', keyring: 'fail'})
+    expect(status(report)).toMatchObject({credential: 'fail', keyring: 'fail'})
     expect(report.ok).toBe(false)
   })
 
@@ -195,10 +195,76 @@ describe('doctor', () => {
     expect(report.ok).toBe(true)
   })
 
+  test('fails when no credential can be used', async () => {
+    const lookup = vi.fn()
+    const report = await runDoctor(
+      deps({
+        credentials: {
+          delete: () => Promise.resolve(),
+          get: () => Promise.resolve(undefined),
+          set: () => Promise.resolve(),
+        },
+        lookup,
+        profiles: {
+          load: () => Promise.resolve({profiles: {}, version: 2}),
+          save: () => Promise.resolve(),
+        },
+      }),
+    )
+    expect(status(report)).toMatchObject({credential: 'fail', keyring: 'ok'})
+    expect(report.ok).toBe(false)
+    expect(lookup).not.toHaveBeenCalled()
+  })
+
+  test('rejects ambiguous environment credentials without remote validation', async () => {
+    const lookup = vi.fn()
+    const report = await runDoctor(
+      deps({
+        env: {
+          POLYMORFA_API_KEY: KEY,
+          POLYMORFA_PROJECT_TOKEN: 'secret-project',
+        },
+        lookup,
+      }),
+    )
+    expect(status(report).credential).toBe('fail')
+    expect(report.ok).toBe(false)
+    expect(lookup).not.toHaveBeenCalled()
+    expect(JSON.stringify(report)).not.toContain(KEY)
+    expect(JSON.stringify(report)).not.toContain('secret-project')
+  })
+
+  test('an explicit profile ignores ambient environment credentials', async () => {
+    const lookup = vi.fn(() => Promise.resolve([]))
+    const report = await runDoctor(
+      deps({
+        env: {
+          POLYMORFA_API_KEY: 'secret-api',
+          POLYMORFA_CLIENT_TOKEN: 'secret-client',
+        },
+        lookup,
+        profile: 'work',
+      }),
+    )
+    expect(report.ok).toBe(true)
+    expect(lookup).toHaveBeenCalledWith(
+      expect.objectContaining({credential: KEY}),
+    )
+  })
+
   test('does not remotely validate client tokens', async () => {
     const report = await runDoctor(
       deps({env: {POLYMORFA_CLIENT_TOKEN: 'pmfa_ct_x'}}),
     )
     expect(status(report).credential).toBe('skip')
+    expect(report.ok).toBe(false)
+  })
+
+  test('does not report success for an unvalidated project token', async () => {
+    const report = await runDoctor(
+      deps({env: {POLYMORFA_PROJECT_TOKEN: 'pmfa_pt_x'}}),
+    )
+    expect(status(report).credential).toBe('skip')
+    expect(report.ok).toBe(false)
   })
 })