diff --git a/apps/sim/app/api/v1/logs/filters.ts b/apps/sim/app/api/v1/logs/filters.ts index 0e409e4d53f..70b89ae5824 100644 --- a/apps/sim/app/api/v1/logs/filters.ts +++ b/apps/sim/app/api/v1/logs/filters.ts @@ -32,11 +32,11 @@ export function buildLogFilters(filters: LogFilters): SQL { const cursorDate = new Date(filters.cursor.startedAt) if (filters.order === 'desc') { conditions.push( - sql`(${workflowExecutionLogs.startedAt}, ${workflowExecutionLogs.id}) < (${cursorDate}, ${filters.cursor.id})` + sql`(${workflowExecutionLogs.startedAt}, ${workflowExecutionLogs.id}) < (${sql.param(cursorDate, workflowExecutionLogs.startedAt)}, ${filters.cursor.id})` ) } else { conditions.push( - sql`(${workflowExecutionLogs.startedAt}, ${workflowExecutionLogs.id}) > (${cursorDate}, ${filters.cursor.id})` + sql`(${workflowExecutionLogs.startedAt}, ${workflowExecutionLogs.id}) > (${sql.param(cursorDate, workflowExecutionLogs.startedAt)}, ${filters.cursor.id})` ) } } diff --git a/apps/sim/lib/execution/payloads/large-value-metadata.ts b/apps/sim/lib/execution/payloads/large-value-metadata.ts index 95d4f43ea98..cc0fc07f6f6 100644 --- a/apps/sim/lib/execution/payloads/large-value-metadata.ts +++ b/apps/sim/lib/execution/payloads/large-value-metadata.ts @@ -390,9 +390,9 @@ async function pruneStaleReferences( batchSize: number, dbClient: LargeValueMetadataClient ): Promise { - // `IN ()` is a syntax error; callers chunk a non-empty list, but never rely on that. + // Empty input is a valid no-op, and `IN ()` is a syntax error whose failure the + // cleanup job swallows — keep these total rather than relying on the caller. if (workspaceIds.length === 0) return 0 - const rows = await dbClient.execute<{ count: number }>(sql` WITH deleted AS ( DELETE FROM ${executionLargeValueReferences} AS ref @@ -434,9 +434,9 @@ async function pruneDeletedParentDependencies( batchSize: number, dbClient: LargeValueMetadataClient ): Promise { - // `IN ()` is a syntax error; callers chunk a non-empty list, but never rely on that. + // Empty input is a valid no-op, and `IN ()` is a syntax error whose failure the + // cleanup job swallows — keep these total rather than relying on the caller. if (workspaceIds.length === 0) return 0 - const rows = await dbClient.execute<{ count: number }>(sql` WITH deleted AS ( DELETE FROM ${executionLargeValueDependencies} AS dependency @@ -472,9 +472,9 @@ async function pruneDeletedLargeValueTombstones( batchSize: number, dbClient: LargeValueMetadataClient ): Promise { - // `IN ()` is a syntax error; callers chunk a non-empty list, but never rely on that. + // Empty input is a valid no-op, and `IN ()` is a syntax error whose failure the + // cleanup job swallows — keep these total rather than relying on the caller. if (workspaceIds.length === 0) return 0 - const rows = await dbClient.execute<{ count: number }>(sql` WITH deleted AS ( DELETE FROM ${executionLargeValues} AS value @@ -483,7 +483,7 @@ async function pruneDeletedLargeValueTombstones( FROM ${executionLargeValues} AS value WHERE value.workspace_id IN ${workspaceIds} AND value.deleted_at IS NOT NULL - AND value.deleted_at < ${deletedBefore} + AND value.deleted_at < ${sql.param(deletedBefore, executionLargeValues.deletedAt)} AND NOT EXISTS ( SELECT 1 FROM ${executionLargeValueDependencies} AS dependency diff --git a/apps/sim/lib/execution/payloads/live-paused-statuses-sql.test.ts b/apps/sim/lib/execution/payloads/live-paused-statuses-sql.test.ts index 812b1d64944..0c6c1ba70a5 100644 --- a/apps/sim/lib/execution/payloads/live-paused-statuses-sql.test.ts +++ b/apps/sim/lib/execution/payloads/live-paused-statuses-sql.test.ts @@ -37,4 +37,13 @@ describe('unreferencedLargeValuePredicate SQL', () => { // `status = IN (...)` is a syntax error Postgres only reports at execution time. expect(text).not.toMatch(/=\s*IN\s*\(/) }) + + it('never binds an array as a parameter', () => { + // The invariant that matters: the app pools set `fetch_types: false`, so an + // array bound as one parameter fails at execution even though the rendered + // SQL (`ANY($1::text[])`) is valid. Only the params can reveal it. + for (const param of params) { + expect(Array.isArray(param)).toBe(false) + } + }) }) diff --git a/apps/sim/lib/execution/payloads/prune-metadata-sql.test.ts b/apps/sim/lib/execution/payloads/prune-metadata-sql.test.ts index 0205ee7b460..f4387965a08 100644 --- a/apps/sim/lib/execution/payloads/prune-metadata-sql.test.ts +++ b/apps/sim/lib/execution/payloads/prune-metadata-sql.test.ts @@ -67,12 +67,16 @@ describe('pruneLargeValueMetadata SQL', () => { } }) - it('never binds an array as a parameter', async () => { + it('binds only scalar parameters', async () => { const statements = await renderPruneStatements([...ids]) for (const { params } of statements) { for (const param of params) { + // Arrays fail under fetch_types: false (no serializer, 22P02); Date + // objects fail in postgres-js's unsafe path outright + // (ERR_INVALID_ARG_TYPE) — both must be pre-encoded to strings. expect(Array.isArray(param)).toBe(false) + expect(param instanceof Date).toBe(false) } } }) diff --git a/apps/sim/lib/webhooks/polling/config-sql.test.ts b/apps/sim/lib/webhooks/polling/config-sql.test.ts new file mode 100644 index 00000000000..f77e2710694 --- /dev/null +++ b/apps/sim/lib/webhooks/polling/config-sql.test.ts @@ -0,0 +1,54 @@ +/** + * @vitest-environment node + */ + +// Renders the real key-removal expression against the real drizzle dialect. +// utils.test.ts mocks drizzle wholesale, so its assertions passed against BOTH +// previously-broken forms (`- ${keys}::text[]` and `- ${sql.param(keys)}::text[]`). +// Only a real render can tell them apart, and only the params reveal an array bind: +// the app pools set `fetch_types: false` (packages/db/db.ts), which leaves +// postgres-js unable to serialize an array bound as a single parameter. +import { describe, expect, it, vi } from 'vitest' + +vi.unmock('drizzle-orm') +vi.unmock('@sim/db') +vi.unmock('@sim/db/schema') + +process.env.DATABASE_URL ??= 'postgresql://user:pass@localhost:5432/test' + +const { sql } = await import('drizzle-orm') +const { PgDialect } = await import('drizzle-orm/pg-core') + +/** Mirrors the expression built in `updateWebhookProviderConfig`. */ +function renderKeyRemoval(removedKeys: string[]) { + const merged = sql`COALESCE("provider_config"::jsonb, '{}'::jsonb) || ${JSON.stringify({ a: 1 })}::jsonb` + return new PgDialect().sqlToQuery( + sql`(${merged}) - ARRAY[${sql.join( + removedKeys.map((key) => sql`${key}`), + sql`, ` + )}]::text[]` + ) +} + +describe('provider-config key removal SQL', () => { + // Production only ever removes one key (google-drive's pageToken reset), so the + // single-element case is the one that actually runs. + for (const keys of [['pageToken'], ['a', 'b'], ['a', 'b', 'c']]) { + it(`binds ${keys.length} key(s) as scalars inside an ARRAY constructor`, () => { + const { sql: text, params } = renderKeyRemoval(keys) + + expect(text).toContain(`ARRAY[${keys.map((_, i) => `$${i + 2}`).join(', ')}]::text[]`) + expect(params).toEqual([JSON.stringify({ a: 1 }), ...keys]) + for (const param of params) { + expect(Array.isArray(param)).toBe(false) + } + }) + } + + it('never renders a row constructor cast to text[]', () => { + // `- ($1, $2)::text[]` — what interpolating the raw JS array produced. Postgres + // rejects it: "cannot cast type record to text[]" (and 22P02 at one element). + const { sql: text } = renderKeyRemoval(['a', 'b']) + expect(text).not.toMatch(/\(\$\d+(, \$\d+)+\)::text\[\]/) + }) +}) diff --git a/packages/db/db.ts b/packages/db/db.ts index ef66a2ef912..8cdc4d477e3 100644 --- a/packages/db/db.ts +++ b/packages/db/db.ts @@ -43,18 +43,33 @@ if (!connectionString) { } /** - * `fetch_types: false` skips postgres.js's `pg_catalog.pg_type` roundtrip, which - * it otherwise runs on every new connection before that connection's first query. - * It builds array parsers only — scalar types (including `jsonb` and pgvector) - * are unaffected — and Drizzle already parses this schema's `text[]` columns - * itself, so the fetch is pure connection-setup latency. + * `fetch_types: false` skips postgres.js's `pg_catalog.pg_type` roundtrip, which it + * otherwise runs on every new connection before that connection's first query. * - * The one behavior this changes: a raw `db.execute` that projects an array-typed - * column yields the wire form `'{a,b}'` rather than `['a','b']`, since nothing maps - * the result. Verified by differential test that Drizzle-typed selects and - * `.returning()` are unaffected — `PgArray.mapFromDriverValue` parses the wire form - * itself, and writes never reach the array serializer because Drizzle serializes - * arrays before binding. + * That roundtrip populates the array type map, which postgres.js uses in BOTH + * directions, for every array type — not just `text[]`. Disabling it has two + * consequences on every client built from these options (the primary, the replica, + * and every `dbFor()` sub-pool): + * + * 1. Reads: a raw `db.execute` projecting an array column yields the wire form + * `'{a,b}'`, not `['a','b']`. Drizzle-typed selects and `.returning()` are + * unaffected — `PgArray.mapFromDriverValue` parses the wire form itself. + * 2. Writes: a JS array bound as a SINGLE parameter fails at execution with + * `22P02 Array value must start with "{"` — there is no serializer to build the + * literal, and neither `prepare: false` nor `sql.array()` avoids it. Drizzle-typed + * column writes ARE safe (`PgArray.mapToDriverValue` stringifies first), as are + * `inArray`/`${jsArray}` in a drizzle `sql` template (both expand to scalar binds). + * Raw binds are NOT: never write `sql.param(someArray)`. Pass an expanded list + * (`IN ${ids}`) or an explicit `ARRAY[${sql.join(...)}]::text[]` of scalars. + * + * Scalar types — including `jsonb`, pgvector, enums and ranges — are unaffected. + * + * Note postgres.js's OWN `sql` tag has the opposite semantics: it binds `${array}` + * as one array parameter. `packages/db/scripts/migrate.ts` deliberately omits + * `fetch_types` for that reason; see the note there before sharing these options. + * + * Pinned by apps/sim/lib/execution/payloads/prune-metadata-sql.test.ts, which renders + * the real statements and asserts no bind parameter is an array. */ const poolOptions = { prepare: false, diff --git a/packages/db/scripts/migrate.ts b/packages/db/scripts/migrate.ts index d08bb7a32a1..b66f0484760 100644 --- a/packages/db/scripts/migrate.ts +++ b/packages/db/scripts/migrate.ts @@ -45,6 +45,14 @@ const hasDirectMigrationUrl = Boolean(process.env.MIGRATION_DATABASE_URL) * `max_lifetime: null` pins the session for the whole run: the postgres-js * default recycles the connection after 30–60 min, silently dropping the * session advisory lock and `SET`s. + * + * Deliberately does NOT set `fetch_types: false`, unlike the app pools in + * `packages/db/db.ts`. Script migrations bind JS arrays directly through + * postgres-js's own `sql` tag (`= ANY(${ids}::text[])`, `unnest(${ids}::text[])`), + * and that binding is powered by the `pg_catalog.pg_type` fetch this option would + * skip. Copying the app's options here for consistency fails every registered + * script migration with `22P02`, aborting the migration container and blocking + * startup on every deploy and every fresh self-hosted install. */ const client = postgres(url, { max: 1, diff --git a/packages/testing/src/mocks/database.mock.ts b/packages/testing/src/mocks/database.mock.ts index d9e405b5cd0..e8a79a6bd0d 100644 --- a/packages/testing/src/mocks/database.mock.ts +++ b/packages/testing/src/mocks/database.mock.ts @@ -25,12 +25,29 @@ export function createMockSql() { }), }) - // Binds a value as a single parameter — drizzle's escape hatch for passing an - // array to Postgres as an array rather than expanding it into a value list. - sqlFn.param = (value: any) => ({ - value, - toSQL: () => ({ sql: '?', params: [value] }), - }) + // Binds a value as a single parameter. Rejecting arrays here is the only place + // the unit suite can see this bug class: the app pools set `fetch_types: false` + // (packages/db/db.ts), which leaves postgres-js with no array serializer, so an + // array bound as ONE parameter fails at execution with `22P02`. Rendered SQL + // looks perfect (`ANY($1::text[])`), so no assertion on query text can catch it. + sqlFn.param = (value: any, encoder?: any) => { + if (encoder === undefined && Array.isArray(value)) { + throw new Error( + 'sql.param(array) binds an array as one parameter, which fails under ' + + 'fetch_types: false (packages/db/db.ts). Interpolate the array directly ' + + 'for an expanded IN list, or build an ARRAY[...]::text[] constructor of ' + + 'scalar binds via sql.join.' + ) + } + if (encoder === undefined && value instanceof Date) { + throw new Error( + 'sql.param(date) without an encoder reaches postgres-js as a Date object, ' + + 'which its unsafe path cannot serialize (ERR_INVALID_ARG_TYPE). Bind ' + + 'through the matching column: sql.param(date, table.timestampColumn).' + ) + } + return { value, toSQL: () => ({ sql: '?', params: [value] }) } + } return sqlFn }