diff --git a/cli/sql/05.helpers.sql b/cli/sql/05.helpers.sql index 427dc47..8c9a47a 100644 --- a/cli/sql/05.helpers.sql +++ b/cli/sql/05.helpers.sql @@ -3,11 +3,16 @@ -- 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 + * - Input validation rejects queries with semicolons outside string literals + * * Usage examples: * -- Basic generic plan * select postgres_ai.explain_generic('select * from users where id = $1'); @@ -39,6 +44,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 +54,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: strip semicolons and anything after them (prevent statement chaining) + -- This is a defense-in-depth measure; EXPLAIN itself only accepts single statements + v_clean_query := trim(query); + if v_clean_query like '%;%' then + -- Allow semicolons inside string literals by checking if it's at the end + -- Simple heuristic: if query ends with ; optionally followed by whitespace, strip it + v_clean_query := regexp_replace(v_clean_query, ';\s*$', ''); + -- If there's still a semicolon, reject (likely multiple statements) + if v_clean_query like '%;%' then + raise exception 'query contains multiple statements (semicolon detected)'; + end if; + end if; + -- Check if HypoPG extension is available if hypopg_index is not null then select exists( @@ -64,15 +88,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'); diff --git a/cli/test/init.integration.test.cjs b/cli/test/init.integration.test.cjs index 3bf9fb0..b8e43c0 100644 --- a/cli/test/init.integration.test.cjs +++ b/cli/test/init.integration.test.cjs @@ -540,4 +540,77 @@ test("integration: table_describe works with different object types", { skip: !h } }); +test("integration: explain_generic validates input and prevents SQL injection", { skip: !havePostgresBinaries() }, async (t) => { + const pg = await withTempPostgres(t); + const { Client } = require("pg"); + + // Run init first + { + const r = await runCliInit([pg.adminUri, "--password", "pw1", "--skip-optional-permissions"]); + assert.equal(r.status, 0, r.stderr || r.stdout); + } + + 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 + t.skip("generic_plan requires PostgreSQL 16+"); + return; + } + + // Test 1: Empty query should be rejected + { + await assert.rejects( + c.query("select postgres_ai.explain_generic('')"), + /query cannot be empty/ + ); + } + + // Test 2: Null query should be rejected + { + await assert.rejects( + c.query("select postgres_ai.explain_generic(null)"), + /query cannot be empty/ + ); + } + + // Test 3: Multiple statements (semicolon in middle) should be rejected + { + await assert.rejects( + c.query("select postgres_ai.explain_generic('select 1; select 2')"), + /multiple statements|semicolon detected/i + ); + } + + // Test 4: Trailing semicolon should be stripped and work + { + const res = await c.query("select postgres_ai.explain_generic('select 1;') as result"); + assert.ok(res.rows[0].result, "Should return a query plan"); + assert.match(res.rows[0].result, /Result/i); + } + + // Test 5: Valid query should work + { + const res = await c.query("select postgres_ai.explain_generic('select $1::int', 'text') as result"); + assert.ok(res.rows[0].result, "Should return a query plan"); + } + + // 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); + assert.ok(Array.isArray(plan), "JSON result should be an array"); + assert.ok(plan[0].Plan, "JSON result should have a Plan"); + } + + } finally { + await c.end(); + } +});