Merge branch 'fix/cli-auth-preflight' into 'main'

fix(cli): validate auth before running checkup checks (fail fast on 401)

See merge request postgres-ai/postgresai!315

Author: Sarumyan

cli/bin/postgres-ai.ts

@@ -24,7 +24,7 @@ import { createInterface } from "readline";
 import * as childProcess from "child_process";
 import { REPORT_GENERATORS, CHECK_INFO, generateAllReports } from "../lib/checkup";
 import { getCheckupEntry } from "../lib/checkup-dictionary";
-import { createCheckupReport, uploadCheckupReportJson, convertCheckupReportJsonToMarkdown, RpcError, formatRpcErrorForDisplay, withRetry } from "../lib/checkup-api";
+import { createCheckupReport, uploadCheckupReportJson, convertCheckupReportJsonToMarkdown, RpcError, formatRpcErrorForDisplay, withRetry, verifyApiKey } from "../lib/checkup-api";
 import { generateCheckSummary } from "../lib/checkup-summary";
 import {
   type Instance,
@@ -334,7 +334,13 @@ function prepareUploadConfig(
       console.error("Tip: run 'postgresai auth' or pass --api-key / set PGAI_API_KEY");
       return null; // Signal to exit
     }
-    return undefined; // Skip upload silently
+    // No credentials and upload not explicitly requested: fall back to
+    // local-only mode, but say so prominently — skipping the upload silently
+    // hides the fact that results never reach the Console.
+    console.error("Notice: no API key configured — results will NOT be uploaded to PostgresAI.");
+    console.error("        To upload: run 'postgresai auth login' or pass --api-key / set PGAI_API_KEY.");
+    console.error("        To run locally without this notice, pass --no-upload.");
+    return undefined; // Skip upload, run checks locally
   }
 
   const cfg = config.readConfig();
@@ -2076,6 +2082,30 @@ program
     const projectWasGenerated = uploadResult?.projectWasGenerated ?? false;
     shouldUpload = !!uploadCfg;
 
+    // Preflight: validate the configured API key with a cheap authenticated
+    // call BEFORE connecting / running checks, so an invalid or expired token
+    // fails in seconds instead of after minutes of wasted work (previously the
+    // upload at the very end of the run was the first authenticated call).
+    // Only a definitive 401/403 stops the run; a transient pre-flight failure
+    // (network error, timeout, 5xx) warns and continues — the upload may still
+    // succeed.
+    if (uploadCfg) {
+      const verification = await verifyApiKey({
+        apiKey: uploadCfg.apiKey,
+        apiBaseUrl: uploadCfg.apiBaseUrl,
+      });
+      if (verification.status === "invalid") {
+        console.error(`Error: the configured API key was rejected by the PostgresAI API (HTTP ${verification.statusCode})`);
+        console.error("Tip: run 'postgresai auth login' to re-authenticate, or pass a valid --api-key / set PGAI_API_KEY");
+        console.error("Tip: pass --no-upload to run checks locally without uploading");
+        process.exitCode = 1;
+        return;
+      }
+      if (verification.status === "unknown") {
+        console.error(`Warning: could not verify API key before running checks (${verification.detail}); continuing — upload will still be attempted`);
+      }
+    }
+
     // Connect and run checks
     const adminConn = resolveAdminConnection({
       conn,

cli/lib/checkup-api.ts

@@ -320,6 +320,81 @@ async function postRpc<T>(params: {
   });
 }
 
+/**
+ * Result of an API key pre-flight verification.
+ * - "valid": the key was accepted by the API
+ * - "invalid": the API definitively rejected the key (HTTP 401/403)
+ * - "unknown": verification could not be completed (network error, timeout,
+ *   unexpected status) — callers should warn and continue, not block the run
+ */
+export type ApiKeyVerification =
+  | { status: "valid" }
+  | { status: "invalid"; statusCode: number }
+  | { status: "unknown"; detail: string };
+
+// Timeout for the auth pre-flight (shorter than regular RPC timeout: this is
+// an optional fast check and must not noticeably delay the run when the API
+// is slow or unreachable).
+const VERIFY_API_KEY_TIMEOUT_MS = 10_000;
+
+/**
+ * Verify an API key with a cheap, side-effect-free authenticated call
+ * (GET /checkup_reports?limit=1 — the same endpoint the `reports` command
+ * uses) so expensive work can fail fast on bad credentials.
+ *
+ * Only a definitive HTTP 401/403 is reported as "invalid". Network errors,
+ * timeouts, and unexpected statuses are reported as "unknown" so a transient
+ * pre-flight failure never blocks a run that might otherwise succeed.
+ */
+export async function verifyApiKey(params: {
+  apiKey: string;
+  apiBaseUrl: string;
+  timeoutMs?: number;
+}): Promise<ApiKeyVerification> {
+  const { apiKey, apiBaseUrl, timeoutMs = VERIFY_API_KEY_TIMEOUT_MS } = params;
+  const base = normalizeBaseUrl(apiBaseUrl);
+  const url = new URL(`${base}/checkup_reports`);
+  url.searchParams.set("limit", "1");
+
+  // Same plaintext-HTTP guard as postRpc: never send the API key over plain
+  // HTTP to a non-loopback host. Report "unknown" rather than aborting the
+  // run — the upload path raises the definitive, actionable error.
+  if (url.protocol === "http:") {
+    const hostname = url.hostname.replace(/^\[|\]$/g, "");
+    const isLoopback = ["localhost", "127.0.0.1", "::1"].includes(hostname);
+    if (!isLoopback && process.env.CHECKUP_ALLOW_HTTP !== "1") {
+      return {
+        status: "unknown",
+        detail: `refusing to send API key over plaintext HTTP to '${url.host}'`,
+      };
+    }
+  }
+
+  const controller = new AbortController();
+  const timer = setTimeout(() => controller.abort(), timeoutMs);
+  try {
+    const response = await fetch(url.toString(), {
+      method: "GET",
+      headers: { "access-token": apiKey },
+      signal: controller.signal,
+    });
+    // Drain the body so the connection is released cleanly.
+    await response.text().catch(() => "");
+    if (response.status === 401 || response.status === 403) {
+      return { status: "invalid", statusCode: response.status };
+    }
+    if (response.ok) {
+      return { status: "valid" };
+    }
+    return { status: "unknown", detail: `HTTP ${response.status}` };
+  } catch (err) {
+    const message = err instanceof Error ? err.message : String(err);
+    return { status: "unknown", detail: message };
+  } finally {
+    clearTimeout(timer);
+  }
+}
+
 /**
  * Create a new checkup report in the PostgresAI backend.
  * This creates the parent report container; individual check results

cli/test/checkup.test.ts

@@ -42,6 +42,24 @@ function runCli(args: string[], env: Record<string, string> = {}) {
   };
 }
 
+// Async variant for tests that need an in-process fake API (Bun.serve):
+// spawnSync would block the event loop and the fake server could never respond.
+async function runCliAsync(args: string[], env: Record<string, string> = {}) {
+  const cliPath = resolve(import.meta.dir, "..", "bin", "postgres-ai.ts");
+  const bunBin = typeof process.execPath === "string" && process.execPath.length > 0 ? process.execPath : "bun";
+  const proc = Bun.spawn([bunBin, cliPath, ...args], {
+    env: { ...process.env, ...env },
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+  const [status, stdout, stderr] = await Promise.all([
+    proc.exited,
+    new Response(proc.stdout).text(),
+    new Response(proc.stderr).text(),
+  ]);
+  return { status, stdout, stderr };
+}
+
 // Unit tests for parseVersionNum
 describe("parseVersionNum", () => {
   test("parses PG 16.3 version number", () => {
@@ -1636,6 +1654,113 @@ describe("CLI tests", () => {
   });
 });
 
+// CLI-level tests for the checkup auth pre-flight: missing/invalid credentials
+// must surface BEFORE checks run (previously the upload at the end of the run
+// was the first authenticated call, wasting minutes of work on a 401).
+describe("checkup auth pre-flight (CLI)", () => {
+  // Dead Postgres port: if the pre-flight correctly stops the run, the CLI
+  // never attempts this connection; if the run continues, the connection
+  // failure mentions this address.
+  const DEAD_DB = "postgresql://test:test@127.0.0.1:2/test";
+
+  test("no API key: prominent notice, run continues locally", () => {
+    const env = {
+      XDG_CONFIG_HOME: `/tmp/postgresai-test-preflight-nokey-${process.pid}`,
+      PGAI_API_KEY: "",
+    };
+    const r = runCli(["checkup", DEAD_DB], env);
+    expect(r.stderr).toMatch(/results will NOT be uploaded/i);
+    expect(r.stderr).toMatch(/auth login/);
+    expect(r.stderr).toMatch(/--no-upload/);
+    // Falls back to local-only mode instead of failing fast
+    expect(r.stderr).not.toMatch(/API key is required/i);
+    // Run continued to the database connection stage
+    expect(r.status).not.toBe(0);
+    expect(r.stderr).toMatch(/127\.0\.0\.1:2|ECONNREFUSED|connect/i);
+  });
+
+  test("--no-upload: no notice, pre-flight skipped entirely", () => {
+    const env = {
+      XDG_CONFIG_HOME: `/tmp/postgresai-test-preflight-noupload-${process.pid}`,
+      PGAI_API_KEY: "",
+    };
+    const r = runCli(["checkup", DEAD_DB, "--no-upload"], env);
+    expect(r.stderr).not.toMatch(/results will NOT be uploaded/i);
+    expect(r.stderr).not.toMatch(/could not verify API key/i);
+    expect(r.stderr).not.toMatch(/API key is required/i);
+  });
+
+  test("invalid API key (HTTP 401): fails fast before running checks", async () => {
+    const server = Bun.serve({
+      hostname: "127.0.0.1",
+      port: 0,
+      fetch() {
+        return new Response(JSON.stringify({ message: "Invalid token" }), {
+          status: 401,
+          headers: { "Content-Type": "application/json" },
+        });
+      },
+    });
+    try {
+      const env = {
+        XDG_CONFIG_HOME: `/tmp/postgresai-test-preflight-badkey-${process.pid}`,
+        PGAI_API_KEY: "bad-token",
+        PGAI_API_BASE_URL: `http://127.0.0.1:${server.port}`,
+      };
+      const r = await runCliAsync(["checkup", DEAD_DB, "--project", "preflight-test"], env);
+      expect(r.status).not.toBe(0);
+      expect(r.stderr).toMatch(/rejected by the PostgresAI API \(HTTP 401\)/);
+      expect(r.stderr).toMatch(/auth login/);
+      expect(r.stderr).toMatch(/--no-upload/);
+      // Stopped BEFORE connecting to the database / running checks
+      expect(r.stderr).not.toMatch(/127\.0\.0\.1:2|ECONNREFUSED/);
+    } finally {
+      server.stop(true);
+    }
+  });
+
+  test("--no-upload skips pre-flight even when an invalid key is configured", async () => {
+    const server = Bun.serve({
+      hostname: "127.0.0.1",
+      port: 0,
+      fetch() {
+        return new Response(JSON.stringify({ message: "Invalid token" }), {
+          status: 401,
+          headers: { "Content-Type": "application/json" },
+        });
+      },
+    });
+    try {
+      const env = {
+        XDG_CONFIG_HOME: `/tmp/postgresai-test-preflight-badkey-noupload-${process.pid}`,
+        PGAI_API_KEY: "bad-token",
+        PGAI_API_BASE_URL: `http://127.0.0.1:${server.port}`,
+      };
+      const r = await runCliAsync(["checkup", DEAD_DB, "--no-upload"], env);
+      expect(r.stderr).not.toMatch(/rejected by the PostgresAI API/);
+      expect(r.stderr).not.toMatch(/could not verify API key/i);
+      // Fails later at the database connection stage instead
+      expect(r.status).not.toBe(0);
+    } finally {
+      server.stop(true);
+    }
+  });
+
+  test("transient pre-flight failure (network error): warns and continues", () => {
+    const env = {
+      XDG_CONFIG_HOME: `/tmp/postgresai-test-preflight-netfail-${process.pid}`,
+      PGAI_API_KEY: "some-token",
+      PGAI_API_BASE_URL: "http://127.0.0.1:1", // connect refused — transient, not a 401/403
+    };
+    const r = runCli(["checkup", DEAD_DB, "--project", "preflight-test"], env);
+    expect(r.stderr).toMatch(/Warning: could not verify API key/i);
+    expect(r.stderr).not.toMatch(/rejected by the PostgresAI API/);
+    // Run continued to the database connection stage
+    expect(r.status).not.toBe(0);
+    expect(r.stderr).toMatch(/127\.0\.0\.1:2|ECONNREFUSED|connect/i);
+  });
+});
+
 // Tests for checkup-api module
 describe("checkup-api", () => {
   test("formatRpcErrorForDisplay formats details/hint nicely", () => {
@@ -1882,6 +2007,102 @@ describe("checkup-api", () => {
       }
     });
   });
+
+  // Auth pre-flight: verifyApiKey checks the key with a cheap authenticated
+  // GET before checkup runs expensive checks. Only definitive 401/403 is
+  // "invalid"; anything transient must come back "unknown" (warn + continue).
+  describe("verifyApiKey (auth pre-flight)", () => {
+    function serveStatus(status: number, body = "[]") {
+      return Bun.serve({
+        hostname: "127.0.0.1",
+        port: 0,
+        fetch() {
+          return new Response(body, { status, headers: { "Content-Type": "application/json" } });
+        },
+      });
+    }
+
+    test("returns valid on HTTP 200", async () => {
+      const server = serveStatus(200);
+      try {
+        const r = await api.verifyApiKey({ apiKey: "k", apiBaseUrl: `http://127.0.0.1:${server.port}` });
+        expect(r.status).toBe("valid");
+      } finally {
+        server.stop(true);
+      }
+    });
+
+    test("returns invalid on HTTP 401", async () => {
+      const server = serveStatus(401, JSON.stringify({ message: "Invalid token" }));
+      try {
+        const r = await api.verifyApiKey({ apiKey: "bad", apiBaseUrl: `http://127.0.0.1:${server.port}` });
+        expect(r.status).toBe("invalid");
+        expect((r as { statusCode: number }).statusCode).toBe(401);
+      } finally {
+        server.stop(true);
+      }
+    });
+
+    test("returns invalid on HTTP 403", async () => {
+      const server = serveStatus(403);
+      try {
+        const r = await api.verifyApiKey({ apiKey: "bad", apiBaseUrl: `http://127.0.0.1:${server.port}` });
+        expect(r.status).toBe("invalid");
+        expect((r as { statusCode: number }).statusCode).toBe(403);
+      } finally {
+        server.stop(true);
+      }
+    });
+
+    test("returns unknown on HTTP 500 (not a definitive rejection)", async () => {
+      const server = serveStatus(500);
+      try {
+        const r = await api.verifyApiKey({ apiKey: "k", apiBaseUrl: `http://127.0.0.1:${server.port}` });
+        expect(r.status).toBe("unknown");
+        expect((r as { detail: string }).detail).toMatch(/HTTP 500/);
+      } finally {
+        server.stop(true);
+      }
+    });
+
+    test("returns unknown on connection refused", async () => {
+      const r = await api.verifyApiKey({ apiKey: "k", apiBaseUrl: "http://127.0.0.1:1" }); // port 1 — connect refused
+      expect(r.status).toBe("unknown");
+    });
+
+    test("returns unknown on timeout", async () => {
+      const server = Bun.serve({
+        hostname: "127.0.0.1",
+        port: 0,
+        async fetch() {
+          await new Promise((resolve) => setTimeout(resolve, 5000));
+          return new Response("[]");
+        },
+      });
+      try {
+        const r = await api.verifyApiKey({
+          apiKey: "k",
+          apiBaseUrl: `http://127.0.0.1:${server.port}`,
+          timeoutMs: 100,
+        });
+        expect(r.status).toBe("unknown");
+      } finally {
+        server.stop(true);
+      }
+    });
+
+    test("does not send the key over plaintext HTTP to non-loopback hosts", async () => {
+      const saved = process.env.CHECKUP_ALLOW_HTTP;
+      delete process.env.CHECKUP_ALLOW_HTTP;
+      try {
+        const r = await api.verifyApiKey({ apiKey: "k", apiBaseUrl: "http://example.com/api" });
+        expect(r.status).toBe("unknown");
+        expect((r as { detail: string }).detail).toMatch(/plaintext HTTP/);
+      } finally {
+        if (saved !== undefined) process.env.CHECKUP_ALLOW_HTTP = saved;
+      }
+    });
+  });
 });
 
 // Tests for checkup-summary module