Merge branch 'fix/supabase-checkup-issues' into 'main'

fix(cli): pick http/https transport from URL protocol

See merge request postgres-ai/postgresai!241

Author: Sarumyan

.pre-commit-config.yaml

@@ -26,10 +26,15 @@ repos:
 
   - repo: local
     hooks:
-      # Catch potential SQL injection patterns in TypeScript
+      # Catch potential SQL injection patterns in TypeScript.
+      # NOTE: `entry` MUST be a literal block scalar (|-). A plain YAML scalar
+      # breaks parsing because the bash command contains `: ` (e.g. in the
+      # "ERROR: Possible ..." echo), which YAML interprets as a mapping
+      # separator inside the value. Don't "simplify" back to a plain scalar.
       - id: sql-injection-check
         name: SQL injection check (TypeScript)
-        entry: bash -c 'grep -rn --include="*.ts" -E "(\`SELECT|\`INSERT|\`UPDATE|\`DELETE|\`DROP|\`ALTER|\`CREATE).*\$\{" "$@" && echo "ERROR: Possible SQL injection — use parameterized queries" && exit 1 || exit 0' --
+        entry: |-
+          bash -c 'grep -rn --include="*.ts" -E "(\`SELECT|\`INSERT|\`UPDATE|\`DELETE|\`DROP|\`ALTER|\`CREATE).*\$\{" "$@" && echo "ERROR: Possible SQL injection — use parameterized queries" && exit 1 || exit 0' --
         language: system
         files: '\.(ts|js)$'
         exclude: '(test|spec|__tests__)'

cli/.gitignore

@@ -6,3 +6,6 @@ coverage/
 # Auto-generated at build time (do not commit)
 lib/metrics-embedded.ts
 lib/checkup-dictionary-embedded.ts
+
+# Local-dev tarballs from `npm pack` (consumed by credential-service Dockerfile.dev)
+*.tgz

cli/lib/checkup-api.ts

@@ -1,3 +1,4 @@
+import * as http from "http";
 import * as https from "https";
 import { URL } from "url";
 import { normalizeBaseUrl } from "./util";
@@ -27,15 +28,15 @@ function isRetryableError(err: unknown): boolean {
     // Retry on server errors (5xx), not on client errors (4xx)
     return err.statusCode >= 500 && err.statusCode < 600;
   }
-  
+
   // Check for Node.js error codes (works on Error and Error-like objects)
   if (typeof err === "object" && err !== null && "code" in err) {
     const code = String((err as { code: unknown }).code);
     if (["ECONNRESET", "ECONNREFUSED", "ENOTFOUND", "ETIMEDOUT"].includes(code)) {
       return true;
     }
   }
-  
+
   if (err instanceof Error) {
     const msg = err.message.toLowerCase();
     // Retry on network-related errors based on message content
@@ -49,7 +50,7 @@ function isRetryableError(err: unknown): boolean {
       msg.includes("network")
     );
   }
-  
+
   return false;
 }
 
@@ -226,7 +227,25 @@ async function postRpc<T>(params: {
       resolve(value);
     };
 
-    const req = https.request(
+    // Transport is picked from the URL protocol so the CLI can talk to a
+    // local-dev PostgREST over plain HTTP. Production URLs are always HTTPS;
+    // to guard against typos (e.g. a missing 's' in 'https://') silently
+    // leaking the API key in cleartext, refuse HTTP to non-loopback hosts
+    // unless the operator explicitly opts in via CHECKUP_ALLOW_HTTP=1.
+    if (url.protocol === "http:") {
+      // WHATWG URL keeps IPv6 literals bracketed in .hostname
+      // (e.g. `[::1]`), so strip the brackets before matching the allowlist.
+      const hostname = url.hostname.replace(/^\[|\]$/g, "");
+      const isLoopback = ["localhost", "127.0.0.1", "::1"].includes(hostname);
+      if (!isLoopback && process.env.CHECKUP_ALLOW_HTTP !== "1") {
+        throw new Error(
+          `Refusing to send API key over plaintext HTTP to '${url.host}'. ` +
+          `Use https://, a loopback hostname, or set CHECKUP_ALLOW_HTTP=1.`
+        );
+      }
+    }
+    const transport = url.protocol === "http:" ? http : https;
+    const req = transport.request(
       url,
       {
         method: "POST",
@@ -277,7 +296,7 @@ async function postRpc<T>(params: {
       req.destroy();  // Backup: ensure request is terminated
       settledReject(new Error(`RPC ${rpcName} timed out after ${timeoutMs}ms (no response)`));
     }, timeoutMs);
-    
+
     req.on("error", (err: Error) => {
       // Handle abort as timeout (may already be rejected by timeout handler)
       if (err.name === "AbortError" || (err as any).code === "ABORT_ERR") {
@@ -295,7 +314,7 @@ async function postRpc<T>(params: {
         settledReject(err);
       }
     });
-    
+
     req.write(body);
     req.end();
   });

cli/test/checkup.test.ts

@@ -1290,6 +1290,90 @@ describe("checkup-api", () => {
     }
     expect(attempts).toBe(2); // Should retry on ECONNRESET
   });
+
+  // Transport selection — pick http/https by URL protocol, but refuse HTTP
+  // to non-loopback hosts unless CHECKUP_ALLOW_HTTP=1 is set (prevents
+  // typo-driven plaintext API-key leaks like http://api.postgres.ai/...).
+  describe("transport selection", () => {
+    test("https URL does not trip the guard (network error expected)", async () => {
+      let caught: Error | null = null;
+      try {
+        await api.createCheckupReport({
+          apiKey: "dummy",
+          apiBaseUrl: "https://127.0.0.1:1/api", // port 1 — connect refused
+          project: "p",
+        });
+      } catch (e) {
+        caught = e as Error;
+      }
+      expect(caught).not.toBeNull();
+      expect(caught!.message).not.toMatch(/Refusing to send API key/);
+    });
+
+    test("http on loopback does not trip the guard (network error expected)", async () => {
+      // IPv6 loopback is written as `[::1]` in URLs; WHATWG URL preserves
+      // the brackets in .hostname, so the guard must strip them before
+      // matching the allowlist.
+      for (const host of ["localhost", "127.0.0.1", "[::1]"]) {
+        let caught: Error | null = null;
+        try {
+          await api.createCheckupReport({
+            apiKey: "dummy",
+            apiBaseUrl: `http://${host}:1/api`, // port 1 — connect refused
+            project: "p",
+          });
+        } catch (e) {
+          caught = e as Error;
+        }
+        expect(caught).not.toBeNull();
+        expect(caught!.message).not.toMatch(/Refusing to send API key/);
+      }
+    });
+
+    test("http to non-loopback host is refused by the guard", async () => {
+      const saved = process.env.CHECKUP_ALLOW_HTTP;
+      delete process.env.CHECKUP_ALLOW_HTTP;
+      try {
+        let caught: Error | null = null;
+        try {
+          await api.createCheckupReport({
+            apiKey: "dummy",
+            apiBaseUrl: "http://example.com/api",
+            project: "p",
+          });
+        } catch (e) {
+          caught = e as Error;
+        }
+        expect(caught).not.toBeNull();
+        expect(caught!.message).toMatch(/Refusing to send API key over plaintext HTTP/);
+        expect(caught!.message).toMatch(/example\.com/);
+      } finally {
+        if (saved !== undefined) process.env.CHECKUP_ALLOW_HTTP = saved;
+      }
+    });
+
+    test("CHECKUP_ALLOW_HTTP=1 bypasses the guard for non-loopback hosts", async () => {
+      const saved = process.env.CHECKUP_ALLOW_HTTP;
+      process.env.CHECKUP_ALLOW_HTTP = "1";
+      try {
+        let caught: Error | null = null;
+        try {
+          await api.createCheckupReport({
+            apiKey: "dummy",
+            apiBaseUrl: "http://127.0.0.2:1/api", // non-loopback-match hostname, connect refused port
+            project: "p",
+          });
+        } catch (e) {
+          caught = e as Error;
+        }
+        expect(caught).not.toBeNull();
+        expect(caught!.message).not.toMatch(/Refusing to send API key/);
+      } finally {
+        if (saved === undefined) delete process.env.CHECKUP_ALLOW_HTTP;
+        else process.env.CHECKUP_ALLOW_HTTP = saved;
+      }
+    });
+  });
 });
 
 // Tests for checkup-summary module
@@ -1821,5 +1905,3 @@ describe("checkup-summary", () => {
     expect(result.message).toBe("No redundant indexes");
   });
 });
-
-