From b188b9c6f562cb6f76dc8a8ee204394667255fc2 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 7 Aug 2026 11:06:47 +0000 Subject: [PATCH 1/5] fix(sql-editor): critical data-migrate correctness (keys, identity, auth) - Encode composite migrate keys as JSON pairs so values containing |/= cannot collide and UPDATE/DELETE the wrong row - Strip identity columns on INSERT when Include identity is off (buildPeekInsert only skipped empty IDs) - Tighten trigger-column heuristic so bare created/updated/modified are not silently omitted - Block migrate when result grids have duplicate keys - Classify /data-migrate/execute SQL and require verb to match op (block DDL/GRANT and op/SQL mismatch) Co-authored-by: huy.phan9 --- apps/web/src/backend/api/routes.ts | 29 +++++++++++++++++ .../components/sql-editor/DataMigrateBar.tsx | 11 +++++++ .../src/frontend/lib/dataMigratePlans.test.ts | 21 ++++++++++++ apps/web/src/frontend/lib/dataMigratePlans.ts | 15 +++++++-- .../src/frontend/lib/resultRowDiff.test.ts | 31 ++++++++++++++++++ apps/web/src/frontend/lib/resultRowDiff.ts | 32 +++++++++++++++---- .../lib/triggerManagedColumns.test.ts | 20 ++++++++---- .../src/frontend/lib/triggerManagedColumns.ts | 9 ++++-- 8 files changed, 151 insertions(+), 17 deletions(-) diff --git a/apps/web/src/backend/api/routes.ts b/apps/web/src/backend/api/routes.ts index 143af751..7806f7d8 100644 --- a/apps/web/src/backend/api/routes.ts +++ b/apps/web/src/backend/api/routes.ts @@ -14,6 +14,7 @@ import { dialectSupportsIndexFragmentation, buildIndexFragmentationCustomTemplate, sqlStatementCategories, + statementVerb, type MigrationStep, type ConnectionOptions, type DbObjectType, @@ -826,10 +827,38 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt res.status(400).json({ error: 'Each op needs key and sql.' }); return; } + if (o.sql.length > MAX_STATEMENT_LENGTH) { + res.status(400).json({ error: `Each op.sql must be under ${MAX_STATEMENT_LENGTH} characters.` }); + return; + } if (o.params !== undefined && !Array.isArray(o.params)) { res.status(400).json({ error: 'op.params must be an array when set.' }); return; } + // Fail-closed like /sql/execute: classify the SQL itself so a client cannot + // label op=insert while sending DELETE/DDL/GRANT and bypass finer permissions. + const categories = sqlStatementCategories(o.sql); + if (categories.length === 0) { + res.status(400).json({ error: 'Could not classify op.sql.' }); + return; + } + for (const category of categories) { + const permission = CATEGORY_PERMISSION[category]; + if (permission) needed.add(permission); + if (category !== 'dml') { + res.status(400).json({ + error: `Data migrate op.sql must be DML (got ${category}).`, + }); + return; + } + } + const verb = statementVerb(o.sql); + if (verb !== o.op) { + res.status(400).json({ + error: `op.sql verb (${verb ?? 'unknown'}) must match op (${o.op}).`, + }); + return; + } needed.add(DATAGRID_ACTION_PERMISSION[o.op]); ops.push({ op: o.op, diff --git a/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx b/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx index 9a10ca54..a8ef50c9 100644 --- a/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx +++ b/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx @@ -183,6 +183,16 @@ export const DataMigrateBar: React.FC = ({ toast({ tone: 'info', title: 'Nothing to migrate', body: 'Grids match for the selected ops.' }); return; } + if (classification.duplicateKeys > 0) { + toast({ + tone: 'warning', + title: 'Duplicate keys in result grids', + body: + `${classification.duplicateKeys} duplicate key value(s) — migrate refuses to guess which row to write. ` + + 'Tighten the SELECT (DISTINCT / better keys) or pick a unique key set.', + }); + return; + } if (selected.uncappedCount > DATA_MIGRATE_ROW_CAP) { toast({ tone: 'warning', @@ -492,6 +502,7 @@ export const DataMigrateBar: React.FC = ({ applying || !canDml || selected.uncappedCount === 0 || + classification.duplicateKeys > 0 || selected.uncappedCount > DATA_MIGRATE_ROW_CAP } onClick={() => void apply()} diff --git a/apps/web/src/frontend/lib/dataMigratePlans.test.ts b/apps/web/src/frontend/lib/dataMigratePlans.test.ts index 6745a581..18804961 100644 --- a/apps/web/src/frontend/lib/dataMigratePlans.test.ts +++ b/apps/web/src/frontend/lib/dataMigratePlans.test.ts @@ -47,6 +47,27 @@ describe('buildDataMigratePlans', () => { expect(parsed.rows).toHaveLength(2); }); + it('omits identity columns from INSERT when includeIdentity is false', () => { + const { plans, errors } = buildDataMigratePlans({ + tableName: 'customers', + dialect: 'sqlite', + sourceColumns: cols, + destColumns: cols, + keyNames: ['id'], + ops: [{ op: 'insert', keyLabel: 'id=3', sourceRow: [3, 'New'] }], + includeIdentity: false, + identityColumns: new Set(['id']), + }); + expect(errors).toEqual([]); + expect(plans).toHaveLength(1); + const sql = plans[0]!.plan.sql.toLowerCase(); + expect(sql).toContain('insert'); + expect(sql).toContain('name'); + // Must not bind/preserve the source id when Include identity is off. + expect(sql).not.toMatch(/\bid\b/); + expect(plans[0]!.plan.params).toEqual(['New']); + }); + it('omits ignored trigger columns from INSERT and UPDATE SQL', () => { const auditCols = ['id', 'name', 'createdAt', 'updatedBy']; const auditOps: ClassifiedRowDiff[] = [ diff --git a/apps/web/src/frontend/lib/dataMigratePlans.ts b/apps/web/src/frontend/lib/dataMigratePlans.ts index bd0da320..b5cbe8dc 100644 --- a/apps/web/src/frontend/lib/dataMigratePlans.ts +++ b/apps/web/src/frontend/lib/dataMigratePlans.ts @@ -97,19 +97,28 @@ export function buildDataMigratePlans(opts: { const plans: DataMigratePlanItem[] = []; const errors: string[] = []; + // When Include identity is off, omit identity/autoincrement columns entirely + // so the destination generates them. buildPeekInsert only skips *empty* + // identity values (Peek UX fills those blank) — source migrate rows always + // carry real IDs, so we must strip here or IDs are preserved contrary to UI. + const insertIgnore = new Set(ignoreLower); + if (!includeIdentity) { + for (const name of identityColumns) insertIgnore.add(name.toLowerCase()); + } + for (const op of ops) { if (op.op === 'insert') { if (!op.sourceRow) { errors.push(`insert ${op.keyLabel}: missing source row`); continue; } - const stripped = stripIgnoredColumns(sourceColumns, op.sourceRow, ignoreLower); + const stripped = stripIgnoredColumns(sourceColumns, op.sourceRow, insertIgnore); const built = buildPeekInsert({ tableName, dialect, values: rowToValues(stripped.columns, stripped.row), - // Empty skip-set when includeIdentity — keep source ID values. - identityColumns: includeIdentity ? undefined : identityColumns, + // Identity already stripped when !includeIdentity; when on, keep values. + identityColumns: undefined, }); if ('error' in built) { errors.push(`insert ${op.keyLabel}: ${built.error}`); diff --git a/apps/web/src/frontend/lib/resultRowDiff.test.ts b/apps/web/src/frontend/lib/resultRowDiff.test.ts index 0e6c4254..78006dfc 100644 --- a/apps/web/src/frontend/lib/resultRowDiff.test.ts +++ b/apps/web/src/frontend/lib/resultRowDiff.test.ts @@ -66,6 +66,36 @@ describe('classifyRowsByKey', () => { expect(c.deletes).toHaveLength(0); }); + it('does not collide when key values contain delimiter characters', () => { + const source = { + columns: ['a', 'b', 'v'], + rows: [['foo|b=bar', 'baz', 1]], + }; + const dest = { + columns: ['a', 'b', 'v'], + rows: [['foo', 'bar|b=baz', 2]], + }; + const c = classifyRowsByKey({ source, dest, keyNames: ['a', 'b'] }); + expect(c.updates).toHaveLength(0); + expect(c.inserts).toHaveLength(1); + expect(c.deletes).toHaveLength(1); + }); + + it('counts duplicate keys instead of silently keeping the first row', () => { + const source = { + columns: ['id', 'name'], + rows: [ + [1, 'first'], + [1, 'second'], + ], + }; + const dest = { columns: ['id', 'name'], rows: [[1, 'dest']] }; + const c = classifyRowsByKey({ source, dest, keyNames: ['id'] }); + expect(c.duplicateKeys).toBe(1); + expect(c.updates).toHaveLength(1); + expect(c.updates[0]!.sourceRow).toEqual([1, 'first']); + }); + it('does not treat differing trigger columns as updates when ignored', () => { const columns = ['id', 'name', 'createdAt', 'updatedBy']; const source = { @@ -109,6 +139,7 @@ describe('selectMigrateOps', () => { updates, deletes: [], skippedNullKeys: 0, + duplicateKeys: 0, totalOps: 600, }; const selected = selectMigrateOps( diff --git a/apps/web/src/frontend/lib/resultRowDiff.ts b/apps/web/src/frontend/lib/resultRowDiff.ts index 4dd8b83b..053f5ae8 100644 --- a/apps/web/src/frontend/lib/resultRowDiff.ts +++ b/apps/web/src/frontend/lib/resultRowDiff.ts @@ -34,6 +34,8 @@ export interface RowDiffClassification { updates: ClassifiedRowDiff[]; deletes: ClassifiedRowDiff[]; skippedNullKeys: number; + /** Duplicate key values seen in source or dest (unsafe for migrate). */ + duplicateKeys: number; /** Total ops before cap. */ totalOps: number; } @@ -59,20 +61,27 @@ export function keyColumnsForGrid( })); } +/** + * Stable map key for composite PK matching. JSON-array encoding avoids + * collisions when a key value contains `|` / `=` (naive `a=x|b=y` join could + * treat distinct composite keys as the same row and UPDATE/DELETE the wrong one). + * Values are stringified so number `1` and string `"1"` still match across dialects. + */ function rowKey( row: unknown[], keys: PeekKeyColumn[] ): { ok: true; key: string; label: string } | { ok: false } { - const parts: string[] = []; + const wire: [string, string][] = []; const labels: string[] = []; for (const k of keys) { if (k.resultIndex < 0) return { ok: false }; const v = row[k.resultIndex]; if (v === null || v === undefined) return { ok: false }; - parts.push(`${k.name.toLowerCase()}=${String(v)}`); - labels.push(`${k.name}=${String(v)}`); + const asText = typeof v === 'bigint' ? v.toString() : String(v); + wire.push([k.name.toLowerCase(), asText]); + labels.push(`${k.name}=${asText}`); } - return { ok: true, key: parts.join('|'), label: labels.join(', ') }; + return { ok: true, key: JSON.stringify(wire), label: labels.join(', ') }; } function nonKeyColumnsDiffer( @@ -125,6 +134,7 @@ export function classifyRowsByKey(opts: { updates: [], deletes: [], skippedNullKeys: 0, + duplicateKeys: 0, totalOps: 0, }; } @@ -132,6 +142,7 @@ export function classifyRowsByKey(opts: { const sourceMap = new Map(); const destMap = new Map(); let skippedNullKeys = 0; + let duplicateKeys = 0; for (const row of source.rows) { const k = rowKey(row, sourceKeys); @@ -139,7 +150,11 @@ export function classifyRowsByKey(opts: { skippedNullKeys += 1; continue; } - if (!sourceMap.has(k.key)) sourceMap.set(k.key, { row, label: k.label }); + if (sourceMap.has(k.key)) { + duplicateKeys += 1; + continue; + } + sourceMap.set(k.key, { row, label: k.label }); } for (const row of dest.rows) { const k = rowKey(row, destKeys); @@ -147,7 +162,11 @@ export function classifyRowsByKey(opts: { skippedNullKeys += 1; continue; } - if (!destMap.has(k.key)) destMap.set(k.key, { row, label: k.label }); + if (destMap.has(k.key)) { + duplicateKeys += 1; + continue; + } + destMap.set(k.key, { row, label: k.label }); } const inserts: ClassifiedRowDiff[] = []; @@ -188,6 +207,7 @@ export function classifyRowsByKey(opts: { updates, deletes, skippedNullKeys, + duplicateKeys, totalOps: inserts.length + updates.length + deletes.length, }; } diff --git a/apps/web/src/frontend/lib/triggerManagedColumns.test.ts b/apps/web/src/frontend/lib/triggerManagedColumns.test.ts index 9ef5a131..5bda2a74 100644 --- a/apps/web/src/frontend/lib/triggerManagedColumns.test.ts +++ b/apps/web/src/frontend/lib/triggerManagedColumns.test.ts @@ -26,12 +26,20 @@ describe('isLikelyTriggerManagedColumn', () => { expect(isLikelyTriggerManagedColumn(name)).toBe(true); }); - it.each(['id', 'name', 'city', 'create_order', 'update_count', 'status'])( - 'does not flag %s', - (name) => { - expect(isLikelyTriggerManagedColumn(name)).toBe(false); - } - ); + it.each([ + 'id', + 'name', + 'city', + 'create_order', + 'update_count', + 'status', + // Bare names are often business columns — must not skip under migrate. + 'created', + 'updated', + 'modified', + ])('does not flag %s', (name) => { + expect(isLikelyTriggerManagedColumn(name)).toBe(false); + }); }); describe('detectTriggerManagedColumns', () => { diff --git a/apps/web/src/frontend/lib/triggerManagedColumns.ts b/apps/web/src/frontend/lib/triggerManagedColumns.ts index c47291d7..ed557105 100644 --- a/apps/web/src/frontend/lib/triggerManagedColumns.ts +++ b/apps/web/src/frontend/lib/triggerManagedColumns.ts @@ -39,9 +39,14 @@ const EXACT = new Set([ 'xmin', // Postgres system ]); -/** Suffix / contains patterns (lower-case, no separators normalized). */ +/** + * Audit-style names with an explicit time/actor suffix. + * Bare `created` / `updated` / `modified` are NOT matched — those are often + * business columns; skipping them under "Skip trigger cols" would silently + * drop real data from migrate INSERT/UPDATE. + */ const PATTERN = - /^(created|updated|modified|lastmodified)(at|on|by|date|time|timestamp)?$|^(created|updated|modified)_?(at|on|by|date|time|timestamp)$|_?(created|updated|modified)_?(at|on|by)$/; + /^(created|updated|modified|lastmodified)(at|on|by|date|time|timestamp)$|^(created|updated|modified)_?(at|on|by|date|time|timestamp)$|_(created|updated|modified)_?(at|on|by)$/; function normalize(name: string): string { return name.trim().toLowerCase().replace(/[^a-z0-9_]/g, ''); From b0ea8d53cb5a7a5bc7c19b09a72528e66e966b73 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 7 Aug 2026 11:09:57 +0000 Subject: [PATCH 2/5] fix(db): keep unsafe DuckDB BIGINTs as strings for grid PK writes Number(bigint) silently rounds past MAX_SAFE_INTEGER, so Peek/result UPDATE/DELETE WHERE clauses could target the wrong key. Safe integers still coerce to number for introspection and small PKs. Co-authored-by: huy.phan9 --- .../providers/duckDb/duckdb.adapter.test.ts | 21 +++++++++++++++++++ .../db/src/providers/duckDb/duckdb.adapter.ts | 18 ++++++++++++++-- 2 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 packages/db/src/providers/duckDb/duckdb.adapter.test.ts diff --git a/packages/db/src/providers/duckDb/duckdb.adapter.test.ts b/packages/db/src/providers/duckDb/duckdb.adapter.test.ts new file mode 100644 index 00000000..5eab2f14 --- /dev/null +++ b/packages/db/src/providers/duckDb/duckdb.adapter.test.ts @@ -0,0 +1,21 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + */ +import { describe, expect, it } from 'vitest'; +import { coerceDriverBigint } from './duckdb.adapter'; + +describe('coerceDriverBigint', () => { + it('keeps safe integers as numbers', () => { + expect(coerceDriverBigint(42n)).toBe(42); + expect(coerceDriverBigint(BigInt(Number.MAX_SAFE_INTEGER))).toBe(Number.MAX_SAFE_INTEGER); + }); + + it('stringifies integers past MAX_SAFE_INTEGER so PK WHERE keys are not rounded', () => { + const unsafe = 9007199254740993n; // Number(unsafe) === 9007199254740992 + expect(Number(unsafe)).toBe(9007199254740992); + expect(Number.isSafeInteger(Number(unsafe))).toBe(false); + expect(coerceDriverBigint(unsafe)).toBe('9007199254740993'); + }); +}); diff --git a/packages/db/src/providers/duckDb/duckdb.adapter.ts b/packages/db/src/providers/duckDb/duckdb.adapter.ts index fc956d18..158fb289 100644 --- a/packages/db/src/providers/duckDb/duckdb.adapter.ts +++ b/packages/db/src/providers/duckDb/duckdb.adapter.ts @@ -92,13 +92,27 @@ class DuckDbAdapter implements DriverAdapter { } } -/** Shallow-coerce bigint values in a row object to Number (introspection values are small). */ +/** + * Coerce driver bigint for JSON / grid use. + * Safe integers stay numbers (introspection + small PKs). Values beyond + * Number.MAX_SAFE_INTEGER stay digit strings so Peek / result-grid + * UPDATE/DELETE WHERE clauses are not silently rounded to the wrong key. + */ +export function coerceDriverBigint(v: bigint): number | string { + const asNumber = Number(v); + return Number.isSafeInteger(asNumber) ? asNumber : v.toString(); +} + +/** Shallow-coerce bigint values in a row object for callers that expect JSON-ish cells. */ function coerceBigints(row: Record): Record { let hasBig = false; for (const k in row) if (typeof row[k] === 'bigint') { hasBig = true; break; } if (!hasBig) return row; const out: Record = {}; - for (const k in row) out[k] = typeof row[k] === 'bigint' ? Number(row[k] as bigint) : row[k]; + for (const k in row) { + const v = row[k]; + out[k] = typeof v === 'bigint' ? coerceDriverBigint(v) : v; + } return out; } From f11fa563b4e824c024358f7ecddb29052fd36354 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 7 Aug 2026 11:09:57 +0000 Subject: [PATCH 3/5] fix(sql-editor): fail closed on page-window and non-unique migrate keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Data migrate classified only currently loaded grid rows, so paging one side (or hasNext) made Delete remove destination keys that still exist later in the source. Require both grids on page 1 with no remaining pages. Also stop defaulting keys to the first column / free checkboxes — use PK or unique index columns only, matching Peek editability. Co-authored-by: huy.phan9 --- .../components/sql-editor/DataMigrateBar.tsx | 84 ++++++++++++------- .../components/sql-editor/ResultsPanel.tsx | 15 +++- .../src/frontend/lib/resultRowDiff.test.ts | 32 +++++++ apps/web/src/frontend/lib/resultRowDiff.ts | 20 +++++ docs/USER_GUIDE.md | 8 +- 5 files changed, 125 insertions(+), 34 deletions(-) diff --git a/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx b/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx index a8ef50c9..75c2a824 100644 --- a/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx +++ b/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx @@ -6,7 +6,7 @@ * Side-by-side data migrate: key-based insert/update/delete onto a destination * grid (≤500 ops). Larger sets toast with Server Beam instructions. */ -import React, { useEffect, useMemo, useState } from 'react'; +import React, { useMemo, useState } from 'react'; import { createPortal } from 'react-dom'; import { ArrowRightLeft, History, Loader2, X } from 'lucide-react'; import { @@ -23,6 +23,7 @@ import { buildDataMigratePlans, buildDestSnapshotJson } from '../../lib/dataMigr import { classifyRowsByKey, DATA_MIGRATE_ROW_CAP, + migrateGridsAreComplete, selectMigrateOps, type ClassifiedRowDiff, } from '../../lib/resultRowDiff'; @@ -42,6 +43,10 @@ export interface DataMigrateGrid { columns: string[]; rows: unknown[][]; statementSql?: string; + /** 0-based page currently shown in the result grid. */ + pageIndex?: number; + /** True when more rows exist beyond this page (hasNext / truncated). */ + hasMore?: boolean; } type ProgressItem = { @@ -86,16 +91,16 @@ export const DataMigrateBar: React.FC = ({ const table: TableSchema | undefined = editTarget.ok ? editTarget.table : undefined; const tableName = table?.name ?? ''; - const defaultKeys = useMemo( - () => resolvePeekKeyColumns(table, source.columns).map((k) => k.name), + // Only PK / non-partial unique keys that appear in the result — never fall back + // to "first column" (non-unique WHERE would UPDATE/DELETE multiple rows). + const keyNames = useMemo( + () => + resolvePeekKeyColumns(table, source.columns) + .filter((k) => k.resultIndex >= 0) + .map((k) => k.name), [table, source.columns] ); - const [keyNames, setKeyNames] = useState([]); - useEffect(() => { - setKeyNames(defaultKeys.length ? defaultKeys : source.columns.slice(0, 1)); - }, [defaultKeys.join('\0'), source.columns.join('\0')]); - /** User opts into each op — nothing selected until they choose. */ const [doInsert, setDoInsert] = useState(false); const [doUpdate, setDoUpdate] = useState(false); @@ -138,14 +143,6 @@ export const DataMigrateBar: React.FC = ({ [classification, doInsert, doUpdate, doDelete] ); - const toggleKey = (name: string) => { - setKeyNames((prev) => - prev.some((k) => k.toLowerCase() === name.toLowerCase()) - ? prev.filter((k) => k.toLowerCase() !== name.toLowerCase()) - : [...prev, name] - ); - }; - const openHistory = async () => { setHistoryOpen(true); try { @@ -175,8 +172,33 @@ export const DataMigrateBar: React.FC = ({ }); return; } - if (keyNames.length === 0) { - toast({ tone: 'warning', title: 'Select at least one key column' }); + if (!editability.editable || keyNames.length === 0) { + toast({ + tone: 'warning', + title: 'Data migrate needs a unique key in the result', + body: + editability.reason || + 'Include the primary key (or a non-partial unique index) columns in the SELECT.', + }); + return; + } + if ( + !migrateGridsAreComplete({ + sourcePageIndex: source.pageIndex ?? 0, + destPageIndex: dest.pageIndex ?? 0, + sourceHasMore: Boolean(source.hasMore), + destHasMore: Boolean(dest.hasMore), + }) + ) { + toast({ + tone: 'warning', + title: 'Migrate needs the full result on page 1', + body: + 'Add / Edit / Delete classify only the rows currently loaded. ' + + 'Page both grids to page 1 with no “next page”, or tighten the SELECT ' + + `(LIMIT ≤ page size). Otherwise Delete can remove destination rows that ` + + 'still exist later in the source.', + }); return; } if (selected.uncappedCount === 0) { @@ -400,17 +422,21 @@ export const DataMigrateBar: React.FC = ({
Keys - {source.columns.map((c) => ( - - ))} + {keyNames.length > 0 ? ( + keyNames.map((c) => ( + + {c} + + )) + ) : ( + + {editability.reason || 'No PK/unique key in this result — migrate disabled.'} + + )}
diff --git a/apps/web/src/frontend/components/sql-editor/ResultsPanel.tsx b/apps/web/src/frontend/components/sql-editor/ResultsPanel.tsx index 5a1b9b9e..5590356d 100644 --- a/apps/web/src/frontend/components/sql-editor/ResultsPanel.tsx +++ b/apps/web/src/frontend/components/sql-editor/ResultsPanel.tsx @@ -883,6 +883,11 @@ const SideBySideStatementSection: React.FC<{ columns: sourceGrid.result.columns, rows: sourceGrid.result.rows, statementSql: sourceGrid.statementSql, + pageIndex: + pageState?.[`${sourceGrid.connectionId}:${statementIndex}`]?.pageIndex ?? 0, + hasMore: Boolean( + sourceGrid.result.hasNext || sourceGrid.result.truncated + ), }} dest={{ connectionId: destGrid.connectionId, @@ -891,6 +896,9 @@ const SideBySideStatementSection: React.FC<{ columns: destGrid.result.columns, rows: destGrid.result.rows, statementSql: destGrid.statementSql, + pageIndex: + pageState?.[`${destGrid.connectionId}:${statementIndex}`]?.pageIndex ?? 0, + hasMore: Boolean(destGrid.result.hasNext || destGrid.result.truncated), }} ignoreColumns={triggerIgnoreColumns} onAfterMigrate={() => onRefresh?.(destGrid.connectionId)} @@ -909,9 +917,10 @@ const SideBySideStatementSection: React.FC<{ /> {compareActive && (

- Cell colors align by row index. Choose Add / Edit / Delete yourself; Transaction and Stop / - Continue are safety assists. Skip trigger cols ignores createdAt / updatedBy. Same ORDER BY - when scanning. Cap: 500 ops — larger sets use Server Beam. + Cell colors align by row index. Migrate matches rows by PK/unique keys and only runs when + both grids show the full result on page 1. Choose Add / Edit / Delete yourself; Transaction + and Stop / Continue are safety assists. Skip trigger cols ignores createdAt / updatedBy. + Cap: 500 ops — larger sets use Server Beam.

)} diff --git a/apps/web/src/frontend/lib/resultRowDiff.test.ts b/apps/web/src/frontend/lib/resultRowDiff.test.ts index 78006dfc..7b27d682 100644 --- a/apps/web/src/frontend/lib/resultRowDiff.test.ts +++ b/apps/web/src/frontend/lib/resultRowDiff.test.ts @@ -7,6 +7,7 @@ import { describe, expect, it } from 'vitest'; import { classifyRowsByKey, DATA_MIGRATE_ROW_CAP, + migrateGridsAreComplete, selectMigrateOps, } from './resultRowDiff'; @@ -121,6 +122,37 @@ describe('classifyRowsByKey', () => { }); +describe('migrateGridsAreComplete', () => { + it('requires page 1 with no remaining pages on both sides', () => { + expect( + migrateGridsAreComplete({ + sourcePageIndex: 0, + destPageIndex: 0, + sourceHasMore: false, + destHasMore: false, + }) + ).toBe(true); + // Dest on page 2 while source stays on page 1 → Delete would drop dest keys + // that still exist later in the source. + expect( + migrateGridsAreComplete({ + sourcePageIndex: 0, + destPageIndex: 1, + sourceHasMore: false, + destHasMore: false, + }) + ).toBe(false); + expect( + migrateGridsAreComplete({ + sourcePageIndex: 0, + destPageIndex: 0, + sourceHasMore: true, + destHasMore: false, + }) + ).toBe(false); + }); +}); + describe('selectMigrateOps', () => { it('respects checkboxes and caps at 500', () => { const inserts = Array.from({ length: 300 }, (_, i) => ({ diff --git a/apps/web/src/frontend/lib/resultRowDiff.ts b/apps/web/src/frontend/lib/resultRowDiff.ts index 053f5ae8..2c8844fa 100644 --- a/apps/web/src/frontend/lib/resultRowDiff.ts +++ b/apps/web/src/frontend/lib/resultRowDiff.ts @@ -226,3 +226,23 @@ export function selectMigrateOps( if (all.length <= cap) return { ops: all, truncated: false, uncappedCount }; return { ops: all.slice(0, cap), truncated: true, uncappedCount }; } + +/** + * Data migrate classifies only the rows currently loaded in each grid. + * If either side is on a later page or still has more pages / truncated + * rows, "missing from this page" is not "missing from the table" — Delete + * would destroy real destination rows that exist later in the source. + */ +export function migrateGridsAreComplete(opts: { + sourcePageIndex: number; + destPageIndex: number; + sourceHasMore: boolean; + destHasMore: boolean; +}): boolean { + return ( + opts.sourcePageIndex === 0 && + opts.destPageIndex === 0 && + !opts.sourceHasMore && + !opts.destHasMore + ); +} diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index 24d007d5..a7419a9e 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -138,8 +138,12 @@ compare / migrate). It lives in the same local web UI you open with `foxschema`. index — use the same `ORDER BY` when scanning. 7. **Data migrate (≤500 row ops)** — with Compare on, **Data migrate** appears. - You choose **Add / Edit / Delete** (none selected until you check them). Safety - assists: **Transaction** (all-or-nothing when Stop is on) and **Stop on error** / + Rows match by the table’s primary key (or a non-partial unique index) present + in the SELECT. Migrate only runs when **both** grids show the **full** result + on **page 1** (no next page) — otherwise “missing on this page” is not “missing + from the table” and Delete could remove real destination rows. You choose + **Add / Edit / Delete** (none selected until you check them). Safety assists: + **Transaction** (all-or-nothing when Stop is on) and **Stop on error** / **Continue on error**. Optional **Include identity / IDs**. With **Skip trigger cols**, migrate does not treat audit columns as edits and omits them from INSERT/UPDATE so destination triggers can fill them. Progress lists each row; From 166579fda2b0a5e144c8717797ddc44ffd06e080 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 7 Aug 2026 11:09:57 +0000 Subject: [PATCH 4/5] fix(rbac): require datagridAction to match SQL verb on /sql/execute Mirror data-migrate hardening so a client cannot claim insert while sending UPDATE/DELETE under the grid-write permission path. Co-authored-by: huy.phan9 --- apps/web/src/backend/api/routes.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/apps/web/src/backend/api/routes.ts b/apps/web/src/backend/api/routes.ts index 7806f7d8..938a30f2 100644 --- a/apps/web/src/backend/api/routes.ts +++ b/apps/web/src/backend/api/routes.ts @@ -522,11 +522,22 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt } // Grid CRUD also needs the matching Data grid permission so Access control // can allow SQL DML without exposing Add/Edit/Delete on Peek / results. + // Require the SQL verb to match the claimed action so a client cannot label + // datagridAction=insert while sending UPDATE/DELETE (or DDL). if (datagridAction !== undefined) { if (!isDatagridAction(datagridAction)) { res.status(400).json({ error: 'datagridAction must be insert, update, or delete.' }); return; } + for (const sql of statements as string[]) { + const verb = statementVerb(sql); + if (verb !== datagridAction) { + res.status(400).json({ + error: `datagridAction (${datagridAction}) must match SQL verb (${verb ?? 'unknown'}).`, + }); + return; + } + } needed.add(DATAGRID_ACTION_PERMISSION[datagridAction]); } if (needed.size > 0 && denyUnless(authed, res, ...needed)) return; From b85e0172767b78c82f9b440c3ef6931a9e57bf94 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 7 Aug 2026 11:13:13 +0000 Subject: [PATCH 5/5] fix(sql): Safe Mode for upserts and MATERIALIZED/quoted write CTEs INSERT ... ON CONFLICT DO UPDATE / ON DUPLICATE KEY UPDATE / OR REPLACE were treated as insert-only and skipped confirmation. WITH AS MATERIALIZED and quoted CTE names also made walkWithCtes fail open so DELETE/UPDATE ran without Safe Mode. Fail closed on unscannable WITH; parse MATERIALIZED and quoted CTE names. Co-authored-by: huy.phan9 --- docs/USER_GUIDE.md | 6 +- packages/sql/src/modules/sql-splitter.test.ts | 47 +++++++++ packages/sql/src/modules/sql-splitter.ts | 99 +++++++++++++++---- 3 files changed, 130 insertions(+), 22 deletions(-) diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index a7419a9e..31d556bc 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -341,8 +341,10 @@ Tips: -- @end ``` -- **Safe mode** — when on, UPDATE / DELETE / MERGE and DDL need an extra - confirmation before run. Plain INSERT (including insert CTEs) does not. +- **Safe mode** — when on, UPDATE / DELETE / MERGE, upserts that can overwrite + rows (`ON CONFLICT DO UPDATE`, `ON DUPLICATE KEY UPDATE`, `INSERT OR REPLACE`), + and DDL need an extra confirmation before run. Plain INSERT (including insert + CTEs and `ON CONFLICT DO NOTHING`) does not. Writes and DDL are allowed when you confirm them. Some dialects (e.g. SQLite / ClickHouse adapters used for SELECT-only paths) may reject writes with a clear error diff --git a/packages/sql/src/modules/sql-splitter.test.ts b/packages/sql/src/modules/sql-splitter.test.ts index 38f29576..e1f75ae6 100644 --- a/packages/sql/src/modules/sql-splitter.test.ts +++ b/packages/sql/src/modules/sql-splitter.test.ts @@ -353,6 +353,10 @@ describe('isInsertWriteStatement', () => { isInsertWriteStatement('WITH i AS (INSERT INTO t VALUES (1) RETURNING id) SELECT id FROM i;') ).toBe(true); expect(isInsertWriteStatement('EXPLAIN ANALYZE INSERT INTO t VALUES (1);')).toBe(true); + // DO NOTHING cannot overwrite existing rows — still insert-only. + expect( + isInsertWriteStatement('INSERT INTO t(id,x) VALUES (1,2) ON CONFLICT (id) DO NOTHING;') + ).toBe(true); }); it('rejects non-insert writes and mixed mutating CTEs', () => { @@ -367,6 +371,49 @@ describe('isInsertWriteStatement', () => { ) ).toBe(false); }); + + it('rejects upserts that can overwrite existing rows (Safe Mode must confirm)', () => { + expect( + isInsertWriteStatement( + 'INSERT INTO t(id,x) VALUES (1,2) ON CONFLICT (id) DO UPDATE SET x = EXCLUDED.x;' + ) + ).toBe(false); + expect( + isInsertWriteStatement( + 'INSERT INTO t(id,x) VALUES (1,2) ON DUPLICATE KEY UPDATE x = VALUES(x);' + ) + ).toBe(false); + expect(isInsertWriteStatement('INSERT OR REPLACE INTO t(id,x) VALUES (1,2);')).toBe(false); + expect(isInsertWriteStatement('REPLACE INTO t(id,x) VALUES (1,2);')).toBe(false); + }); +}); + +describe('isWriteStatement — MATERIALIZED / quoted CTE names', () => { + it('detects DELETE/UPDATE inside AS MATERIALIZED CTEs (Safe Mode)', () => { + expect( + isWriteStatement('WITH d AS MATERIALIZED (DELETE FROM t RETURNING id) SELECT id FROM d;') + ).toBe(true); + expect( + isWriteStatement( + 'WITH u AS NOT MATERIALIZED (UPDATE t SET x = 1 RETURNING *) SELECT * FROM u;' + ) + ).toBe(true); + expect( + isWriteStatement('WITH d AS MATERIALIZED (SELECT id FROM t) SELECT id FROM d;') + ).toBe(false); + }); + + it('detects DELETE inside quoted / bracketed CTE names', () => { + expect( + isWriteStatement('WITH "i" AS (DELETE FROM t RETURNING id) SELECT id FROM "i";') + ).toBe(true); + expect( + isWriteStatement('WITH [i] AS (DELETE FROM t OUTPUT DELETED.id) SELECT * FROM [i];') + ).toBe(true); + expect( + isWriteStatement('WITH `i` AS (DELETE FROM t RETURNING id) SELECT id FROM `i`;') + ).toBe(true); + }); }); describe('isPageableStatement', () => { diff --git a/packages/sql/src/modules/sql-splitter.ts b/packages/sql/src/modules/sql-splitter.ts index 0744d1f4..82b69d3a 100644 --- a/packages/sql/src/modules/sql-splitter.ts +++ b/packages/sql/src/modules/sql-splitter.ts @@ -674,13 +674,16 @@ function sqlTextIsWrite(text: string): boolean { return inner !== null && sqlTextIsWrite(inner); } if (kw === 'with') { - for (const body of withCteBodies(text)) { + const bodies: string[] = []; + // Recurse into CTE bodies + the tail rather than matching its leading verb, so + // `WITH … SELECT * INTO t` and `WITH … EXPLAIN ANALYZE DELETE` still warn. + // Unscannable WITH (parser hole) fails closed → treat as write so Safe Mode confirms. + const tail = walkWithCtes(text, (body) => bodies.push(body)); + if (tail === null) return true; + for (const body of bodies) { if (sqlTextIsWrite(body)) return true; } - // Recurse into the tail rather than matching its leading verb, so - // `WITH … SELECT * INTO t` and `WITH … EXPLAIN ANALYZE DELETE` still warn. - const tail = walkWithCtes(text); - return tail !== null && sqlTextIsWrite(tail); + return sqlTextIsWrite(tail); } return false; } @@ -927,20 +930,25 @@ function sqlTextIsMutatingDml(text: string): boolean { } if (MUTATING_DML.has(kw)) return true; if (kw === 'with') { - for (const body of withCteBodies(text)) { + const bodies: string[] = []; + const tail = walkWithCtes(text, (body) => bodies.push(body)); + // Match isWriteStatement: unscannable WITH is treated as mutating for Safe Mode. + if (tail === null) return true; + for (const body of bodies) { if (sqlTextIsMutatingDml(body)) return true; } - const tail = walkWithCtes(text); - return tail !== null && sqlTextIsMutatingDml(tail); + return sqlTextIsMutatingDml(tail); } return false; } /** - * True when every write in the statement is INSERT (plain `INSERT …`, - * `WITH … INSERT …`, or `WITH i AS (INSERT … RETURNING …) SELECT …`). + * True when every write in the statement is a plain INSERT (additive only): + * `INSERT …`, `WITH … INSERT …`, or `WITH i AS (INSERT … RETURNING …) SELECT …`. * Safe mode skips confirmation for these; UPDATE/DELETE/MERGE, DDL, - * `SELECT … INTO`, and mixed insert+mutating CTEs still confirm. + * `SELECT … INTO`, upserts that can overwrite rows (`ON CONFLICT DO UPDATE`, + * `ON DUPLICATE KEY UPDATE`, `INSERT OR REPLACE`), and mixed insert+mutating + * CTEs still confirm. */ export function isInsertWriteStatement(text: string): boolean { const parts = splitSqlStatements(text); @@ -956,6 +964,19 @@ export function isInsertWriteStatement(text: string): boolean { return sawInsert; } +/** + * True when an INSERT can overwrite existing rows (not a pure append). + * Strings/comments are stripped so literals cannot fake the keywords. + */ +function insertMayUpdateExisting(text: string): boolean { + const stripped = stripSqlStringsAndComments(text); + if (/\bINSERT\s+OR\s+REPLACE\b/i.test(stripped)) return true; + if (/\bON\s+DUPLICATE\s+KEY\s+UPDATE\b/i.test(stripped)) return true; + // Postgres/SQLite upsert that assigns columns — DO NOTHING stays insert-only. + if (/\bON\s+CONFLICT\b/i.test(stripped) && /\bDO\s+UPDATE\b/i.test(stripped)) return true; + return false; +} + function sqlTextIsInsertOnlyWrite(text: string): boolean { if (!sqlTextIsWrite(text)) return false; const kw = firstKeyword(text); @@ -964,12 +985,13 @@ function sqlTextIsInsertOnlyWrite(text: string): boolean { const inner = peelExplainAnalyze(text); return inner !== null && sqlTextIsInsertOnlyWrite(inner); } - if (kw === 'insert') return true; + if (kw === 'insert') return !insertMayUpdateExisting(text); if (kw === 'with') { - const bodies = withCteBodies(text); - const tail = walkWithCtes(text); - const chunks = tail ? [...bodies, tail] : bodies; - const writes = chunks.filter((c) => sqlTextIsWrite(c)); + const bodies: string[] = []; + const tail = walkWithCtes(text, (body) => bodies.push(body)); + // Unscannable WITH is a write (fail closed) but not insert-only → confirm. + if (tail === null) return false; + const writes = [...bodies, tail].filter((c) => sqlTextIsWrite(c)); return writes.length > 0 && writes.every((c) => sqlTextIsInsertOnlyWrite(c)); } return false; @@ -1206,8 +1228,9 @@ function keywordAfterWithCtes(text: string): string | null { } /** - * Scan `WITH [RECURSIVE] cte AS (body) [, …]` then return the remainder after - * the CTE list (main statement text). Invokes `onBody` for each CTE body. + * Scan `WITH [RECURSIVE] cte AS [NOT] MATERIALIZED (body) [, …]` then return + * the remainder after the CTE list (main statement text). Invokes `onBody` for + * each CTE body. CTE names may be bare or quoted (`"i"`, `` `i` ``, `[i]`). */ function walkWithCtes(text: string, onBody?: (body: string) => void): string | null { let i = 0; @@ -1220,9 +1243,9 @@ function walkWithCtes(text: string, onBody?: (body: string) => void): string | n const rec = nextWord(text, i); if (rec && rec.word === 'recursive') i = rec.end; - // Walk `name [(cols)] AS (…)` [, …] + // Walk `name [(cols)] AS [NOT] MATERIALIZED (…)` [, …] while (i < text.length) { - const name = nextWord(text, i); + const name = nextCteName(text, i); if (!name) return null; i = name.end; @@ -1237,6 +1260,16 @@ function walkWithCtes(text: string, onBody?: (body: string) => void): string | n if (!asKw || asKw.word !== 'as') return null; i = asKw.end; + // Postgres: AS MATERIALIZED (…) / AS NOT MATERIALIZED (…) + const mat = nextWord(text, i); + if (mat && mat.word === 'not') { + const mat2 = nextWord(text, mat.end); + if (!mat2 || mat2.word !== 'materialized') return null; + i = mat2.end; + } else if (mat && mat.word === 'materialized') { + i = mat.end; + } + const open = skipWsAndComments(text, i); if (open >= text.length || text[open] !== '(') return null; const close = skipBalancedParens(text, open); @@ -1255,6 +1288,32 @@ function walkWithCtes(text: string, onBody?: (body: string) => void): string | n return null; } +/** Bare or quoted CTE name; advances past the identifier. */ +function nextCteName(text: string, from: number): { end: number } | null { + const i = skipWsAndComments(text, from); + if (i >= text.length) return null; + const ch = text[i]!; + if (ch === '"' || ch === '`' || ch === '[') { + const close = ch === '[' ? ']' : ch; + let j = i + 1; + while (j < text.length) { + const c = text[j]!; + if (c === close) { + // SQL escaped quotes double the closer. + if (j + 1 < text.length && text[j + 1] === close) { + j += 2; + continue; + } + return { end: j + 1 }; + } + j++; + } + return null; + } + const bare = nextWord(text, i); + return bare ? { end: bare.end } : null; +} + function firstKeywordSpan(text: string): { word: string; end: number } | null { let mode: Mode = 'code'; for (let i = 0; i < text.length; i++) {