From eeacb973f8c85577d0c7978b96419b624d286276 Mon Sep 17 00:00:00 2001 From: Joshen Lim Date: Wed, 11 Mar 2026 15:34:54 +0700 Subject: [PATCH] Add warning in SQL editor when running alter db commands which prevents connections to the DB (#43584) ## Context Adds a warning if running an `ALTER DATABASE` command that prevents connections to the database. This would lock the dashboard out of the database, and re-configuring the setting will require a direct connection to the DB image ## To test - Could try running the following command in the SQL editor to check the warning, this will show all the warnings ``` ALTER TABLE colors2 drop column sss; update colors set name = 'test'; alter database postgres connection limit 0; ``` --- .../SQLEditor/RunQueryWarningModal.tsx | 31 ++-- .../SQLEditor/SQLEditor.constants.ts | 8 + .../interfaces/SQLEditor/SQLEditor.tsx | 51 +++--- .../interfaces/SQLEditor/SQLEditor.types.ts | 6 + .../SQLEditor/SQLEditor.utils.test.ts | 54 ++++++ .../interfaces/SQLEditor/SQLEditor.utils.ts | 17 +- e2e/studio/features/sql-editor.spec.ts | 155 ++++++++++++++++++ 7 files changed, 282 insertions(+), 40 deletions(-) diff --git a/apps/studio/components/interfaces/SQLEditor/RunQueryWarningModal.tsx b/apps/studio/components/interfaces/SQLEditor/RunQueryWarningModal.tsx index e902c67a99..fa27e55bc6 100644 --- a/apps/studio/components/interfaces/SQLEditor/RunQueryWarningModal.tsx +++ b/apps/studio/components/interfaces/SQLEditor/RunQueryWarningModal.tsx @@ -1,21 +1,24 @@ -import { Separator } from 'ui' +import { DialogSectionSeparator, Separator } from 'ui' import ConfirmationModal from 'ui-patterns/Dialogs/ConfirmationModal' +import { PotentialIssues } from './SQLEditor.types' + interface RunQueryWarningModalProps { visible: boolean - hasDestructiveOperations: boolean - hasUpdateWithoutWhere: boolean + potentialIssues: PotentialIssues | undefined onCancel: () => void onConfirm: () => void } export const RunQueryWarningModal = ({ visible, - hasDestructiveOperations, - hasUpdateWithoutWhere, + potentialIssues, onCancel, onConfirm, }: RunQueryWarningModalProps) => { + const { hasDestructiveOperations, hasUpdateWithoutWhere, hasAlterDatabasePreventConnection } = + potentialIssues || {} + return (
-
    +
      {hasDestructiveOperations && (
    • - Query has destructive operation - + Query has destructive operations + Make sure you are not accidentally removing something important.
      • )} - {hasDestructiveOperations && hasUpdateWithoutWhere && } {hasUpdateWithoutWhere && (
    • Query uses update without a where clause - + Without a where clause, this could update all rows in the table.
      • )} + {hasAlterDatabasePreventConnection && ( +
    • + Query will prevent connections to your database + + The dashboard will no longer have access to your database, and you will need a + direct connection to your database to reconfigure this setting + +
      • + )}

diff --git a/apps/studio/components/interfaces/SQLEditor/SQLEditor.constants.ts b/apps/studio/components/interfaces/SQLEditor/SQLEditor.constants.ts index 09ba0e4fb7..6a3b8e597d 100644 --- a/apps/studio/components/interfaces/SQLEditor/SQLEditor.constants.ts +++ b/apps/studio/components/interfaces/SQLEditor/SQLEditor.constants.ts @@ -39,6 +39,14 @@ export const destructiveSqlRegex = [ /^(.*;)?\s*(drop|delete|truncate|alter\s+table\s+.*\s+drop\s+column)\s/is, ] +export const updateWithoutWhereRegex = + /(?:^|;)\s*update\s+(?:"[\w.]+"\."[\w.]+"|[\w.]+)\s+set\s+[\w\W]+?(?!\s*where\s)/is + +export const alterDatabasePreventConnectionStatements = [ + 'alter database postgres connection limit 0', + 'alter database postgres allow_connections false', +] + export const ASSISTANT_TEMPLATES = [ { name: 'Twitter clone', diff --git a/apps/studio/components/interfaces/SQLEditor/SQLEditor.tsx b/apps/studio/components/interfaces/SQLEditor/SQLEditor.tsx index 718463124b..7f8898d5a6 100644 --- a/apps/studio/components/interfaces/SQLEditor/SQLEditor.tsx +++ b/apps/studio/components/interfaces/SQLEditor/SQLEditor.tsx @@ -64,8 +64,14 @@ import { sqlAiDisclaimerComment, untitledSnippetTitle, } from './SQLEditor.constants' -import { DiffType, IStandaloneCodeEditor, IStandaloneDiffEditor } from './SQLEditor.types' import { + DiffType, + IStandaloneCodeEditor, + IStandaloneDiffEditor, + type PotentialIssues, +} from './SQLEditor.types' +import { + checkAlterDatabaseConnection, checkDestructiveQuery, checkIfAppendLimitRequired, createSqlSnippetSkeletonV2, @@ -123,9 +129,8 @@ export const SQLEditor = () => { const [hasSelection, setHasSelection] = useState(false) const [lineHighlights, setLineHighlights] = useState([]) const [isDiffEditorMounted, setIsDiffEditorMounted] = useState(false) - const [showPotentialIssuesModal, setShowPotentialIssuesModal] = useState(false) - const [queryHasDestructiveOperations, setQueryHasDestructiveOperations] = useState(false) - const [queryHasUpdateWithoutWhere, setQueryHasUpdateWithoutWhere] = useState(false) + const [potentialIssues, setPotentialIssues] = useState() + const [showWidget, setShowWidget] = useState(false) const [activeUtilityTab, setActiveUtilityTab] = useState('results') @@ -316,23 +321,20 @@ export const SQLEditor = () => { ? (selectedValue || editorRef.current?.getValue()) ?? snippet.snippet.content?.sql : selectedValue || editorRef.current?.getValue() - let queryHasIssues = false + const hasDestructiveOperations = checkDestructiveQuery(sql) + const hasUpdateWithoutWhere = isUpdateWithoutWhere(sql) + const hasAlterDatabasePreventConnection = checkAlterDatabaseConnection(sql) - const destructiveOperations = checkDestructiveQuery(sql) - if (!force && destructiveOperations) { - setShowPotentialIssuesModal(true) - setQueryHasDestructiveOperations(true) - queryHasIssues = true - } - - const updateWithoutWhereClause = isUpdateWithoutWhere(sql) - if (!force && updateWithoutWhereClause) { - setShowPotentialIssuesModal(true) - setQueryHasUpdateWithoutWhere(true) - queryHasIssues = true - } + const queryHasIssues = + !force && + (hasDestructiveOperations || hasUpdateWithoutWhere || hasAlterDatabasePreventConnection) if (queryHasIssues) { + setPotentialIssues({ + hasDestructiveOperations, + hasUpdateWithoutWhere, + hasAlterDatabasePreventConnection, + }) return } @@ -801,21 +803,16 @@ export const SQLEditor = () => { return ( <> { clearPendingRunRefocus() - setShowPotentialIssuesModal(false) - setQueryHasDestructiveOperations(false) - setQueryHasUpdateWithoutWhere(false) + setPotentialIssues(undefined) refocusEditor() }} onConfirm={() => { shouldRefocusAfterRunRef.current = true - setShowPotentialIssuesModal(false) - setQueryHasDestructiveOperations(false) - setQueryHasUpdateWithoutWhere(false) + setPotentialIssues(undefined) refocusEditor() void executeQuery(true) }} diff --git a/apps/studio/components/interfaces/SQLEditor/SQLEditor.types.ts b/apps/studio/components/interfaces/SQLEditor/SQLEditor.types.ts index 3988a93a7c..092bd1a642 100644 --- a/apps/studio/components/interfaces/SQLEditor/SQLEditor.types.ts +++ b/apps/studio/components/interfaces/SQLEditor/SQLEditor.types.ts @@ -30,3 +30,9 @@ export enum DiffType { Addition = 'addition', NewSnippet = 'new-snippet', } + +export type PotentialIssues = { + hasDestructiveOperations?: boolean + hasUpdateWithoutWhere?: boolean + hasAlterDatabasePreventConnection?: boolean +} diff --git a/apps/studio/components/interfaces/SQLEditor/SQLEditor.utils.test.ts b/apps/studio/components/interfaces/SQLEditor/SQLEditor.utils.test.ts index b8736ded8e..b14c2d46e5 100644 --- a/apps/studio/components/interfaces/SQLEditor/SQLEditor.utils.test.ts +++ b/apps/studio/components/interfaces/SQLEditor/SQLEditor.utils.test.ts @@ -1,5 +1,6 @@ import { stripIndent } from 'common-tags' import { + checkAlterDatabaseConnection, checkDestructiveQuery, checkIfAppendLimitRequired, isUpdateWithoutWhere, @@ -260,3 +261,56 @@ describe('SQLEditor.utils:updateWithoutWhere', () => { }) }) }) + +describe('SQLEditor.utils:checkAlterDatabaseConnection', () => { + it('detects connection limit 0', () => { + const match = checkAlterDatabaseConnection('alter database postgres connection limit 0;') + expect(match).toBe(true) + }) + + it('detects allow_connections false', () => { + const match = checkAlterDatabaseConnection('alter database postgres allow_connections false;') + expect(match).toBe(true) + }) + + it('detects case-insensitive match', () => { + const match = checkAlterDatabaseConnection('ALTER DATABASE postgres CONNECTION LIMIT 0;') + expect(match).toBe(true) + }) + + it('detects statement among multiple statements', () => { + const match = checkAlterDatabaseConnection(stripIndent` + select * from countries; + alter database postgres connection limit 0; + `) + expect(match).toBe(true) + }) + + it('does not flag unrelated alter database statement', () => { + const match = checkAlterDatabaseConnection( + 'alter database postgres set statement_timeout = 60000;' + ) + expect(match).toBe(false) + }) + + it('does not flag non-alter statements', () => { + const match = checkAlterDatabaseConnection('select * from countries;') + expect(match).toBe(false) + }) + + it('ignores statements inside comments', () => { + const match = checkAlterDatabaseConnection(stripIndent` + -- alter database postgres connection limit 0; + select 1; + `) + expect(match).toBe(false) + }) + + it('detects both dangerous statements in same query', () => { + const match = checkAlterDatabaseConnection(stripIndent` + alter database postgres connection limit 0; + alter database postgres allow_connections false; + `) + expect(match).toBe(true) + }) +}) diff --git a/apps/studio/components/interfaces/SQLEditor/SQLEditor.utils.ts b/apps/studio/components/interfaces/SQLEditor/SQLEditor.utils.ts index f18eb79c5e..23ded23477 100644 --- a/apps/studio/components/interfaces/SQLEditor/SQLEditor.utils.ts +++ b/apps/studio/components/interfaces/SQLEditor/SQLEditor.utils.ts @@ -1,10 +1,13 @@ import { generateUuid } from 'lib/api/snippets.browser' import { removeCommentsFromSql } from 'lib/helpers' import type { SnippetWithContent } from 'state/sql-editor-v2' + import { - NEW_SQL_SNIPPET_SKELETON, + alterDatabasePreventConnectionStatements, destructiveSqlRegex, + NEW_SQL_SNIPPET_SKELETON, sqlAiDisclaimerComment, + updateWithoutWhereRegex, } from './SQLEditor.constants' import { ContentDiff } from './SQLEditor.types' @@ -55,8 +58,6 @@ export function checkDestructiveQuery(sql: string) { // Function to check for UPDATE queries without WHERE clause export function isUpdateWithoutWhere(sql: string): boolean { - const updateWithoutWhereRegex = - /(?:^|;)\s*update\s+(?:"[\w.]+"\."[\w.]+"|[\w.]+)\s+set\s+[\w\W]+?(?!\s*where\s)/is const updateStatements = sql .split(';') .filter((statement) => statement.trim().toLowerCase().startsWith('update')) @@ -65,6 +66,16 @@ export function isUpdateWithoutWhere(sql: string): boolean { ) } +export function checkAlterDatabaseConnection(sql: string): boolean { + const cleanedSql = removeCommentsFromSql(sql) + const statements = cleanedSql + .split(';') + .filter((statement) => statement.trim().toLowerCase().startsWith('alter database')) + return statements.some((statement) => + alterDatabasePreventConnectionStatements.some((x) => statement.toLowerCase().includes(x)) + ) +} + export const generateMigrationCliCommand = (id: string, name: string, isNpx = false) => ` ${isNpx ? 'npx ' : ''}supabase snippets download ${id} | diff --git a/e2e/studio/features/sql-editor.spec.ts b/e2e/studio/features/sql-editor.spec.ts index db689ed1b5..ac6c8f98a8 100644 --- a/e2e/studio/features/sql-editor.spec.ts +++ b/e2e/studio/features/sql-editor.spec.ts @@ -167,6 +167,161 @@ test.describe('SQL Editor', () => { } }) + test('should block execution for alter database connection limit 0', async ({ ref }) => { + await expect(page.getByText('Loading...')).not.toBeVisible() + await page.locator('.view-lines').click() + await page.keyboard.press('ControlOrMeta+KeyA') + await page.keyboard.type(`alter database postgres connection limit 0;`) + + // Track whether the SQL editor dispatches this specific query to pg-meta + let queryDispatched = false + const listener = (request: any) => { + if ( + request.url().includes('query?key=') && + request.method() === 'POST' && + request.postData()?.includes('connection limit 0') + ) { + queryDispatched = true + } + } + page.on('request', listener) + + await page.getByTestId('sql-run-button').click() + + // verify warning modal blocks execution + await expect( + page.getByRole('heading', { name: 'Potential issue detected with' }) + ).toBeVisible() + await expect( + page.getByText('Query will prevent connections to your database') + ).toBeVisible() + expect(queryDispatched).toBe(false) + + // cancel should dismiss without executing + await page.getByRole('button', { name: 'Cancel' }).click() + expect(queryDispatched).toBe(false) + + page.removeListener('request', listener) + + // clear SQL snippet + if (!isCLI()) { + await deleteSqlSnippet(page, ref, newSqlSnippetName) + } else { + await page.reload() + } + }) + + test('should block execution for alter database allow_connections false', async ({ ref }) => { + await expect(page.getByText('Loading...')).not.toBeVisible() + await page.locator('.view-lines').click() + await page.keyboard.press('ControlOrMeta+KeyA') + await page.keyboard.type(`ALTER DATABASE postgres ALLOW_CONNECTIONS false;`) + + // Track whether the SQL editor dispatches this specific query to pg-meta + let queryDispatched = false + const listener = (request: any) => { + if ( + request.url().includes('query?key=') && + request.method() === 'POST' && + request.postData()?.includes('ALLOW_CONNECTIONS false') + ) { + queryDispatched = true + } + } + page.on('request', listener) + + await page.getByTestId('sql-run-button').click() + + // verify warning modal blocks execution + await expect( + page.getByRole('heading', { name: 'Potential issue detected with' }) + ).toBeVisible() + await expect( + page.getByText('Query will prevent connections to your database') + ).toBeVisible() + expect(queryDispatched).toBe(false) + + // cancel should dismiss without executing + await page.getByRole('button', { name: 'Cancel' }).click() + expect(queryDispatched).toBe(false) + + page.removeListener('request', listener) + + // clear SQL snippet + if (!isCLI()) { + await deleteSqlSnippet(page, ref, newSqlSnippetName) + } else { + await page.reload() + } + }) + + test('should block execution for update without where clause', async ({ ref }) => { + await expect(page.getByText('Loading...')).not.toBeVisible() + await page.locator('.view-lines').click() + await page.keyboard.press('ControlOrMeta+KeyA') + await page.keyboard.type(`update countries set name = 'test';`) + + // Track whether the SQL editor dispatches this specific query to pg-meta + let queryDispatched = false + const listener = (request: any) => { + if ( + request.url().includes('query?key=') && + request.method() === 'POST' && + request.postData()?.includes("set name = 'test'") + ) { + queryDispatched = true + } + } + page.on('request', listener) + + await page.getByTestId('sql-run-button').click() + + // verify warning modal blocks execution + await expect( + page.getByRole('heading', { name: 'Potential issue detected with' }) + ).toBeVisible() + await expect(page.getByText('Query uses update without a where clause')).toBeVisible() + expect(queryDispatched).toBe(false) + + // cancel should dismiss without executing + await page.getByRole('button', { name: 'Cancel' }).click() + expect(queryDispatched).toBe(false) + + page.removeListener('request', listener) + + // 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() + await page.keyboard.press('ControlOrMeta+KeyA') + await page.keyboard.type(`alter database postgres set statement_timeout = 60000;`) + + const sqlMutationPromise = waitForApiResponse(page, 'pg-meta', ref, 'query?key=', { + method: 'POST', + }) + await page.getByTestId('sql-run-button').click() + await sqlMutationPromise + + // verify warning modal is NOT visible - query should execute directly + await expect( + page.getByRole('heading', { name: 'Potential issue detected with' }) + ).not.toBeVisible() + + // clear SQL snippet + if (!isCLI()) { + await deleteSqlSnippet(page, ref, newSqlSnippetName) + } else { + await page.reload() + } + }) + test('exporting works as expected', async ({ ref }) => { await expect(page.getByText('Loading...')).not.toBeVisible() await page.locator('.view-lines').click()