mirror of
https://github.com/supabase/supabase.git
synced 2026-05-06 08:56:46 -04:00
fix(studio): don't scan dollar-quoted bodies for DDL in SQL editor (#45050)
Fixes a false positive in the CREATE-TABLE-without-RLS warning modal added in #45008. The warning was firing on `CREATE FUNCTION` statements because the `SELECT..INTO` detector was matching plpgsql variable assignments inside `$$…$$` function bodies. Reported example that triggered the modal with no table actually being created: ```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; $$; ``` **Changed:** - `SQLEventParser.match()` now strips the body of `$tag$…$tag$` blocks before running detectors. Tags are kept as markers; content is blanked out so function bodies, DO blocks, and dollar-quoted string literals are never scanned as DDL. - Updated a pre-existing parser test that asserted the buggy behaviour (it expected `CREATE TABLE fake` inside a `$$…$$` string literal to be detected — `$$…$$` is a string literal in Postgres, not DDL). **Added:** - Regression tests in `SQLEditor.utils.test.ts` covering: the exact reported function, DO blocks with `select into`, `create table` text inside a function body, mixed top-level `CREATE TABLE` + function with `INTO` assignments, and custom `$body$…$body$` tags. - Parser-level regression test in `sql-event-parser.test.ts`. ## To test - In the SQL editor, paste the function from the Slack report and run it — the RLS warning modal should not appear. - Run `create table foo (id int8 primary key);` on its own — modal still appears as before. - Run `create table foo (id int8); create or replace function bar() returns int language plpgsql as $$ declare v int; begin select 1 into v; return v; end; $$;` — modal should flag only `foo`, not `v`. - Run an existing destructive query (`drop table x`) — unaffected, modal still works. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Parser no longer treats DDL/DML-like text inside PL/pgSQL functions, DO blocks, or dollar-quoted bodies (including nested/custom tags) as top-level CREATE TABLE/SELECT INTO, preventing false detections and UI warnings. * **Tests** * Added unit and e2e regression tests covering dollar-quoted blocks, nested dollar tags, DO blocks, SELECT INTO inside functions, and positive controls with a real top-level CREATE TABLE. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Alaister Young <10985857+alaister@users.noreply.github.com>
This commit is contained in:
Alaister Young
@@ -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.
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user