diff --git a/apps/studio/components/interfaces/SQLEditor/SQLEditor.utils.test.ts b/apps/studio/components/interfaces/SQLEditor/SQLEditor.utils.test.ts index 0541700b8d..76fe45abe0 100644 --- a/apps/studio/components/interfaces/SQLEditor/SQLEditor.utils.test.ts +++ b/apps/studio/components/interfaces/SQLEditor/SQLEditor.utils.test.ts @@ -420,6 +420,108 @@ describe('SQLEditor.utils:getCreateTablesMissingRLS', () => { expect(result[0].tableName).toBe('foo') }) + it('does not flag `select ... into var` inside a plpgsql function body', () => { + // Regression: the SELECT..INTO detector used to match variable assignments + // inside $$...$$ function bodies and surface them as \"new tables\". + const sql = stripIndent` + create or replace function schema_checks() + returns jsonb + language plpgsql + as $$ + declare + ret jsonb; + begin + select jsonb_build_object('value', 'ok') + into ret; + return ret; + end; + $$; + ` + expect(getCreateTablesMissingRLS(sql)).toEqual([]) + }) + + it('does not flag `select ... into var` inside a DO block', () => { + const sql = stripIndent` + do $$ + declare + result int; + begin + select count(*) into result from information_schema.tables; + end + $$; + ` + expect(getCreateTablesMissingRLS(sql)).toEqual([]) + }) + + it('does not flag CREATE TABLE text that appears inside a function body', () => { + const sql = stripIndent` + create or replace function noop() + returns void + language plpgsql + as $$ + begin + -- create table foo (id int); + perform 1; + end; + $$; + ` + expect(getCreateTablesMissingRLS(sql)).toEqual([]) + }) + + it('flags top-level CREATE TABLE alongside a function with INTO assignments', () => { + const sql = stripIndent` + create table public.foo (id int8 primary key); + create or replace function bar() + returns int + language plpgsql + as $$ + declare + v int; + begin + select 1 into v; + return v; + end; + $$; + ` + const result = getCreateTablesMissingRLS(sql) + expect(result).toEqual([{ schema: 'public', tableName: 'foo' }]) + }) + + it('does not flag CREATE TABLE inside nested dollar-quoted dynamic SQL', () => { + // Regression: the `$sql$...$sql$` block inside the outer `$fn$...$fn$` + // body was previously pairing with the outer tag, letting the inner + // semicolon split the statement and exposing `create table fake` to the + // RLS warning. + const sql = stripIndent` + create function f() + returns void + language plpgsql + as $fn$ + begin + execute $sql$create table fake(id int);$sql$; + end; + $fn$; + ` + expect(getCreateTablesMissingRLS(sql)).toEqual([]) + }) + + it('handles custom dollar-quote tags (e.g. $body$...$body$)', () => { + const sql = stripIndent` + create or replace function f() + returns int + language plpgsql + as $body$ + declare + v int; + begin + select 1 into v; + return v; + end; + $body$; + ` + expect(getCreateTablesMissingRLS(sql)).toEqual([]) + }) + it('does not collide quoted identifiers that differ only by case', () => { // "MyTable" and "mytable" are distinct tables in Postgres, so the ALTER // here targets a different table than the CREATE — the warning must fire. diff --git a/apps/studio/lib/sql-event-parser.test.ts b/apps/studio/lib/sql-event-parser.test.ts index 44ddcd662a..6575d02dc1 100644 --- a/apps/studio/lib/sql-event-parser.test.ts +++ b/apps/studio/lib/sql-event-parser.test.ts @@ -405,7 +405,10 @@ describe('SQL Event Parser', () => { }) }) - it('handles dollar-quoted strings in SQL', () => { + it('does not scan inside dollar-quoted string literals', () => { + // $$...$$ is a string literal in Postgres — its contents must not be + // parsed as DDL, otherwise a user inserting SQL-shaped text into a log + // table would trigger false-positive table-created events. const sql = ` CREATE TABLE users (id INT); INSERT INTO logs VALUES ($$CREATE TABLE fake$$); @@ -413,11 +416,81 @@ describe('SQL Event Parser', () => { ` const results = sqlEventParser.getTableEvents(sql) expect(results).toHaveLength(3) - expect(results[0].type).toBe(TABLE_EVENT_ACTIONS.TableCreated) - expect(results[0]).toMatchObject({ tableName: 'users' }) - expect(results[1].type).toBe(TABLE_EVENT_ACTIONS.TableCreated) - expect(results[1]).toMatchObject({ tableName: 'fake' }) - expect(results[2].type).toBe(TABLE_EVENT_ACTIONS.TableDataAdded) + expect(results[0]).toMatchObject({ + type: TABLE_EVENT_ACTIONS.TableCreated, + tableName: 'users', + }) + expect(results[1]).toMatchObject({ + type: TABLE_EVENT_ACTIONS.TableDataAdded, + tableName: 'logs', + }) + expect(results[2]).toMatchObject({ + type: TABLE_EVENT_ACTIONS.TableDataAdded, + tableName: 'users', + }) + }) + + it('does not treat SELECT..INTO inside a plpgsql body as table creation', () => { + // Regression for the RLS warning modal false-positive: variable + // assignment inside a function body must not be reported as a new table. + const sql = ` + create or replace function schema_checks() + returns jsonb + language plpgsql + as $$ + declare + ret jsonb; + begin + select jsonb_build_object('value', 'ok') into ret; + return ret; + end; + $$; + ` + const results = sqlEventParser.getTableEvents(sql) + expect(results).toEqual([]) + }) + + it('does not leak nested dollar-quoted dynamic SQL through statement splitting', () => { + // Regression: an inner $sql$...$sql$ tag used inside an outer $fn$...$fn$ + // body was pairing with the outer opening tag (the splitStatements regex + // doesn't enforce matching tags), which caused the inner semicolon to + // split the statement and exposed `create table fake` to the detectors. + // The fix is that stripDollarQuoteBodies runs before splitStatements and + // uses a backreference to require matching tags. + const sql = ` + create function f() + returns void + language plpgsql + as $fn$ + begin + execute $sql$create table fake(id int);$sql$; + end; + $fn$; + ` + const results = sqlEventParser.getTableEvents(sql) + expect(results).toEqual([]) + }) + + it('still detects a real top-level CREATE TABLE next to a function with nested dollar tags', () => { + const sql = ` + create table public.real_table(id int); + create function f() + returns void + language plpgsql + as $fn$ + begin + execute $sql$create table fake(id int);$sql$; + end; + $fn$; + ` + const results = sqlEventParser.getTableEvents(sql) + expect(results).toEqual([ + { + type: TABLE_EVENT_ACTIONS.TableCreated, + schema: 'public', + tableName: 'real_table', + }, + ]) }) it('handles SQL injection attempts safely', () => { diff --git a/apps/studio/lib/sql-event-parser.ts b/apps/studio/lib/sql-event-parser.ts index 7b098d3ff8..4b01c1652a 100644 --- a/apps/studio/lib/sql-event-parser.ts +++ b/apps/studio/lib/sql-event-parser.ts @@ -49,6 +49,22 @@ export class SQLEventParser { return identifier?.replace(/["`']/g, '').replace(/\.$/, '') } + // Blank out the body of $tag$...$tag$ blocks (PL/pgSQL function bodies, DO + // blocks, dollar-quoted string literals) so their contents aren't scanned for + // DDL. A `select ... into var` inside a function body is variable assignment, + // not table creation, and would otherwise trip the SELECT..INTO detector. + // + // The backreference \1 forces opening and closing tags to match, so a nested + // inner block with a different tag (e.g. $fn$ containing $sql$...$sql$) is + // consumed as part of the outer body instead of being paired as the outer. + // + // Must run before statement splitting — splitStatements' dollar-quote regex + // doesn't enforce matching tags, so inner semicolons would otherwise leak + // out and fragment the function body across statements. + private stripDollarQuoteBodies(sql: string): string { + return sql.replace(/(\$[a-zA-Z0-9_]*\$)[\s\S]*?\1/g, '$1$1') + } + private match(sql: string): TableEventDetails | null { for (const { type, patterns } of SQLEventParser.DETECTORS) { for (const pattern of patterns) { @@ -113,7 +129,11 @@ export class SQLEventParser { } getTableEvents(sql: string): TableEventDetails[] { - const statements = this.splitStatements(this.removeComments(sql)) + // Order matters: strip dollar-quote bodies first so comment syntax inside + // a function body (which is just literal text in Postgres) isn't treated + // as a comment by removeComments, and so inner semicolons inside the body + // can't confuse splitStatements. + const statements = this.splitStatements(this.removeComments(this.stripDollarQuoteBodies(sql))) const results: TableEventDetails[] = [] for (const stmt of statements) { diff --git a/e2e/studio/features/sql-editor.spec.ts b/e2e/studio/features/sql-editor.spec.ts index e537f65c15..f5bf4f2ef1 100644 --- a/e2e/studio/features/sql-editor.spec.ts +++ b/e2e/studio/features/sql-editor.spec.ts @@ -343,6 +343,61 @@ test.describe('SQL Editor', () => { } }) + test('does not warn on CREATE FUNCTION with plpgsql SELECT..INTO variable assignment', async ({ + ref, + }) => { + // Regression for the parser false-positive where `select ... into var` + // inside a $$...$$ plpgsql body was mistaken for SELECT..INTO creating + // a new table, firing the CREATE-TABLE-without-RLS warning modal. + const fnName = 'pw_rls_regression_fn' + + // Clean up any leftover function from a previous run + await query(`drop function if exists public.${fnName}()`) + + try { + await expect(page.getByText('Loading...')).not.toBeVisible() + await page.locator('.view-lines').click() + await page.keyboard.press('ControlOrMeta+KeyA') + await page.keyboard.type( + `create or replace function ${fnName}() returns jsonb language plpgsql as $$ declare ret jsonb; begin select jsonb_build_object('ok', true) into ret; return ret; end; $$;` + ) + + const sqlMutationPromise = waitForApiResponse(page, 'pg-meta', ref, 'query?key=', { + method: 'POST', + }) + await page.getByTestId('sql-run-button').click() + await sqlMutationPromise + + // If the warning modal had fired, the query would have been blocked and + // the waiter above would have timed out. Belt-and-braces check that the + // modal is not visible. + await expect( + page.getByRole('heading', { name: 'Potential issue detected with' }), + 'RLS warning should not fire on CREATE FUNCTION with plpgsql SELECT..INTO' + ).not.toBeVisible() + + // Confirm the function was actually created + const rows = await query<{ exists: boolean }>( + `select exists ( + select 1 from pg_proc p + join pg_namespace n on n.oid = p.pronamespace + where n.nspname = 'public' and p.proname = $1 + ) as exists`, + [fnName] + ) + expect(rows[0]?.exists, 'Function should have been created').toBe(true) + } finally { + await query(`drop function if exists public.${fnName}()`) + + // clear SQL snippet + if (!isCLI()) { + await deleteSqlSnippet(page, ref, newSqlSnippetName) + } else { + await page.reload() + } + } + }) + test('should not show warning modal for safe alter database statement', async ({ ref }) => { await expect(page.getByText('Loading...')).not.toBeVisible() await page.locator('.view-lines').click()