Merge branch 'claude/fix-explain-generic-sql-injection-boJKG' into 'main'

fix(prepare-db): add SQL injection protection to explain_generic

Closes #70

See merge request postgres-ai/postgres_ai!121

Author: NikolayS

cli/sql/05.helpers.sql

@@ -3,11 +3,17 @@
 -- operations they don't have direct permissions for.
 
 /*
- * pgai_explain_generic
+ * explain_generic
  *
  * Function to get generic explain plans with optional HypoPG index testing.
  * Requires: PostgreSQL 16+ (for generic_plan option), HypoPG extension (optional).
  *
+ * Security notes:
+ * - EXPLAIN without ANALYZE is read-only (plans but doesn't execute the query)
+ * - PostgreSQL's EXPLAIN only accepts a single statement (primary protection)
+ * - Input validation uses a simple heuristic to detect multiple statements
+ *   (Note: may reject valid queries containing semicolons in string literals)
+ *
  * Usage examples:
  *   -- Basic generic plan
  *   select postgres_ai.explain_generic('select * from users where id = $1');
@@ -39,6 +45,7 @@ declare
   v_hypo_result record;
   v_version int;
   v_hypopg_available boolean;
+  v_clean_query text;
 begin
   -- Check PostgreSQL version (generic_plan requires 16+)
   select current_setting('server_version_num')::int into v_version;
@@ -48,6 +55,24 @@ begin
       current_setting('server_version');
   end if;
 
+  -- Input validation: reject empty queries
+  if query is null or trim(query) = '' then
+    raise exception 'query cannot be empty';
+  end if;
+
+  -- Input validation: detect multiple statements (defense-in-depth)
+  -- Note: This is a simple heuristic - EXPLAIN itself only accepts single statements
+  -- Limitation: Queries with semicolons inside string literals will be rejected
+  v_clean_query := trim(query);
+  if v_clean_query like '%;%' then
+    -- Strip trailing semicolon if present (common user convenience)
+    v_clean_query := regexp_replace(v_clean_query, ';\s*$', '');
+    -- If there's still a semicolon, reject (likely multiple statements or semicolon in string)
+    if v_clean_query like '%;%' then
+      raise exception 'query contains semicolon (multiple statements not allowed; note: semicolons in string literals are also not supported)';
+    end if;
+  end if;
+
   -- Check if HypoPG extension is available
   if hypopg_index is not null then
     select exists(
@@ -64,15 +89,14 @@ begin
       v_hypo_result.indexname, v_hypo_result.indexrelid;
   end if;
 
-  -- Build and execute explain query based on format
-  -- Output is preserved exactly as EXPLAIN returns it
+  -- Build and execute EXPLAIN query
+  -- Note: EXPLAIN is read-only (plans but doesn't execute), making this safe
   begin
     if lower(format) = 'json' then
-      v_explain_query := 'explain (verbose, settings, generic_plan, format json) ' || query;
-      execute v_explain_query into result;
+      execute 'explain (verbose, settings, generic_plan, format json) ' || v_clean_query
+        into result;
     else
-      v_explain_query := 'explain (verbose, settings, generic_plan) ' || query;
-      for v_line in execute v_explain_query loop
+      for v_line in execute 'explain (verbose, settings, generic_plan) ' || v_clean_query loop
         v_lines := array_append(v_lines, v_line."QUERY PLAN");
       end loop;
       result := array_to_string(v_lines, e'\n');

cli/test/init.integration.test.ts

@@ -396,4 +396,102 @@ describe.skipIf(skipTests)("integration: prepare-db", () => {
       await pg.cleanup();
     }
   });
+
+  test("explain_generic validates input and prevents SQL injection", async () => {
+    pg = await createTempPostgres();
+
+    try {
+      // Run init first
+      {
+        const r = runCliInit([pg.adminUri, "--password", "pw1", "--skip-optional-permissions"]);
+        expect(r.status).toBe(0);
+      }
+
+      const c = new Client({ connectionString: pg.adminUri });
+      await c.connect();
+
+      try {
+        // Check PostgreSQL version - generic_plan requires 16+
+        const versionRes = await c.query("show server_version_num");
+        const version = parseInt(versionRes.rows[0].server_version_num, 10);
+
+        if (version < 160000) {
+          // Skip this test on older PostgreSQL versions
+          console.log("Skipping explain_generic tests: requires PostgreSQL 16+");
+          return;
+        }
+
+        // Test 1: Empty query should be rejected
+        await expect(
+          c.query("select postgres_ai.explain_generic('')")
+        ).rejects.toThrow(/query cannot be empty/);
+
+        // Test 2: Null query should be rejected
+        await expect(
+          c.query("select postgres_ai.explain_generic(null)")
+        ).rejects.toThrow(/query cannot be empty/);
+
+        // Test 3: Multiple statements (semicolon in middle) should be rejected
+        await expect(
+          c.query("select postgres_ai.explain_generic('select 1; select 2')")
+        ).rejects.toThrow(/semicolon|multiple statements/i);
+
+        // Test 4: Trailing semicolon should be stripped and work
+        {
+          const res = await c.query("select postgres_ai.explain_generic('select 1;') as result");
+          expect(res.rows[0].result).toBeTruthy();
+          expect(res.rows[0].result).toMatch(/Result/i);
+        }
+
+        // Test 5: Valid query should work
+        {
+          const res = await c.query("select postgres_ai.explain_generic('select $1::int', 'text') as result");
+          expect(res.rows[0].result).toBeTruthy();
+        }
+
+        // Test 6: JSON format should work
+        {
+          const res = await c.query("select postgres_ai.explain_generic('select 1', 'json') as result");
+          const plan = JSON.parse(res.rows[0].result);
+          expect(Array.isArray(plan)).toBe(true);
+          expect(plan[0].Plan).toBeTruthy();
+        }
+
+        // Test 7: Whitespace-only query should be rejected
+        await expect(
+          c.query("select postgres_ai.explain_generic('   ')")
+        ).rejects.toThrow(/query cannot be empty/);
+
+        // Test 8: Semicolon in string literal is rejected (documented limitation)
+        // Note: This is a known limitation - the simple heuristic cannot parse SQL strings
+        await expect(
+          c.query("select postgres_ai.explain_generic('select ''hello;world''')")
+        ).rejects.toThrow(/semicolon/i);
+
+        // Test 9: SQL comments should work (no semicolons)
+        {
+          const res = await c.query("select postgres_ai.explain_generic('select 1 -- comment') as result");
+          expect(res.rows[0].result).toBeTruthy();
+        }
+
+        // Test 10: Escaped quotes should work (no semicolons)
+        {
+          const res = await c.query("select postgres_ai.explain_generic('select ''test''''s value''') as result");
+          expect(res.rows[0].result).toBeTruthy();
+        }
+
+        // Test 11: Case-insensitive format parameter
+        {
+          const res = await c.query("select postgres_ai.explain_generic('select 1', 'JSON') as result");
+          const plan = JSON.parse(res.rows[0].result);
+          expect(Array.isArray(plan)).toBe(true);
+        }
+
+      } finally {
+        await c.end();
+      }
+    } finally {
+      await pg.cleanup();
+    }
+  });
 });