diff --git a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-column-manager.service.ts b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-column-manager.service.ts index 3b71e9553c9..6022e611e59 100644 --- a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-column-manager.service.ts +++ b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-column-manager.service.ts @@ -2,7 +2,7 @@ import { type ColumnType, type QueryRunner } from 'typeorm'; import { type WorkspaceSchemaColumnDefinition } from 'src/engine/twenty-orm/workspace-schema-manager/types/workspace-schema-column-definition.type'; import { buildSqlColumnDefinition } from 'src/engine/twenty-orm/workspace-schema-manager/utils/build-sql-column-definition.util'; -import { removeSqlDDLInjection } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; +import { escapeIdentifier } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; export class WorkspaceSchemaColumnManagerService { async addColumns({ @@ -18,12 +18,10 @@ export class WorkspaceSchemaColumnManagerService { }): Promise { if (columnDefinitions.length === 0) return; - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); const addColumnClauses = columnDefinitions.map( (column) => `ADD COLUMN ${buildSqlColumnDefinition(column)}`, ); - const sql = `ALTER TABLE "${safeSchemaName}"."${safeTableName}" ${addColumnClauses.join(', ')}`; + const sql = `ALTER TABLE ${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)} ${addColumnClauses.join(', ')}`; await queryRunner.query(sql); } @@ -43,15 +41,12 @@ export class WorkspaceSchemaColumnManagerService { }): Promise { if (columnNames.length === 0) return; - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); const cascadeClause = cascade ? ' CASCADE' : ''; - const dropClauses = columnNames.map((name) => { - const safeName = removeSqlDDLInjection(name); - - return `DROP COLUMN IF EXISTS "${safeName}"${cascadeClause}`; - }); - const sql = `ALTER TABLE "${safeSchemaName}"."${safeTableName}" ${dropClauses.join(', ')}`; + const dropClauses = columnNames.map( + (name) => + `DROP COLUMN IF EXISTS ${escapeIdentifier(name)}${cascadeClause}`, + ); + const sql = `ALTER TABLE ${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)} ${dropClauses.join(', ')}`; await queryRunner.query(sql); } @@ -69,11 +64,7 @@ export class WorkspaceSchemaColumnManagerService { oldColumnName: string; newColumnName: string; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); - const safeOldColumnName = removeSqlDDLInjection(oldColumnName); - const safeNewColumnName = removeSqlDDLInjection(newColumnName); - const sql = `ALTER TABLE "${safeSchemaName}"."${safeTableName}" RENAME COLUMN "${safeOldColumnName}" TO "${safeNewColumnName}"`; + const sql = `ALTER TABLE ${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)} RENAME COLUMN ${escapeIdentifier(oldColumnName)} TO ${escapeIdentifier(newColumnName)}`; await queryRunner.query(sql); } @@ -92,20 +83,21 @@ export class WorkspaceSchemaColumnManagerService { defaultValue?: string | number | boolean | null; columnType?: ColumnType; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); - const safeColumnName = removeSqlDDLInjection(columnName); + const tableRef = `${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)}`; + const columnRef = escapeIdentifier(columnName); const computeDefaultValueSqlQuery = () => { if (defaultValue === undefined) { - return `ALTER TABLE "${safeSchemaName}"."${safeTableName}" ALTER COLUMN "${safeColumnName}" DROP DEFAULT`; + return `ALTER TABLE ${tableRef} ALTER COLUMN ${columnRef} DROP DEFAULT`; } if (defaultValue === null) { - return `ALTER TABLE "${safeSchemaName}"."${safeTableName}" ALTER COLUMN "${safeColumnName}" SET DEFAULT NULL`; + return `ALTER TABLE ${tableRef} ALTER COLUMN ${columnRef} SET DEFAULT NULL`; } - return `ALTER TABLE "${safeSchemaName}"."${safeTableName}" ALTER COLUMN "${safeColumnName}" SET DEFAULT ${defaultValue}`; + // defaultValue here is pre-serialized by serializeDefaultValue which + // already applies escaping/sanitization to the value. + return `ALTER TABLE ${tableRef} ALTER COLUMN ${columnRef} SET DEFAULT ${defaultValue}`; }; const sql = computeDefaultValueSqlQuery(); diff --git a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-enum-manager.service.ts b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-enum-manager.service.ts index 7f0d5022e2c..e490bb45c4b 100644 --- a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-enum-manager.service.ts +++ b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-enum-manager.service.ts @@ -7,9 +7,11 @@ import { import { type WorkspaceSchemaColumnDefinition } from 'src/engine/twenty-orm/workspace-schema-manager/types/workspace-schema-column-definition.type'; import { buildSqlColumnDefinition } from 'src/engine/twenty-orm/workspace-schema-manager/utils/build-sql-column-definition.util'; import { computePostgresEnumName } from 'src/engine/workspace-manager/workspace-migration/utils/compute-postgres-enum-name.util'; -import { removeSqlDDLInjection } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; +import { + escapeIdentifier, + escapeLiteral, +} from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; -// TODO: upstream does not guarantee transactionality, implement IF EXISTS or equivalent for idempotency export class WorkspaceSchemaEnumManagerService { async createEnum({ queryRunner, @@ -30,13 +32,10 @@ export class WorkspaceSchemaEnumManagerService { } const sanitizedValues = values - .map((value) => removeSqlDDLInjection(value.toString())) - .map((value) => `'${value}'`) + .map((value) => escapeLiteral(value.toString())) .join(', '); - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeEnumName = removeSqlDDLInjection(enumName); - const sql = `CREATE TYPE "${safeSchemaName}"."${safeEnumName}" AS ENUM (${sanitizedValues})`; + const sql = `CREATE TYPE ${escapeIdentifier(schemaName)}.${escapeIdentifier(enumName)} AS ENUM (${sanitizedValues})`; await queryRunner.query(sql); } @@ -50,9 +49,7 @@ export class WorkspaceSchemaEnumManagerService { schemaName: string; enumName: string; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeEnumName = removeSqlDDLInjection(enumName); - const sql = `DROP TYPE IF EXISTS "${safeSchemaName}"."${safeEnumName}"`; + const sql = `DROP TYPE IF EXISTS ${escapeIdentifier(schemaName)}.${escapeIdentifier(enumName)}`; await queryRunner.query(sql); } @@ -68,10 +65,7 @@ export class WorkspaceSchemaEnumManagerService { oldEnumName: string; newEnumName: string; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeOldEnumName = removeSqlDDLInjection(oldEnumName); - const safeNewEnumName = removeSqlDDLInjection(newEnumName); - const sql = `ALTER TYPE "${safeSchemaName}"."${safeOldEnumName}" RENAME TO "${safeNewEnumName}"`; + const sql = `ALTER TYPE ${escapeIdentifier(schemaName)}.${escapeIdentifier(oldEnumName)} RENAME TO ${escapeIdentifier(newEnumName)}`; await queryRunner.query(sql); } @@ -91,19 +85,12 @@ export class WorkspaceSchemaEnumManagerService { beforeValue?: string; afterValue?: string; }): Promise { - const sanitizedValue = removeSqlDDLInjection(value); - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeEnumName = removeSqlDDLInjection(enumName); - let sql = `ALTER TYPE "${safeSchemaName}"."${safeEnumName}" ADD VALUE '${sanitizedValue}'`; + let sql = `ALTER TYPE ${escapeIdentifier(schemaName)}.${escapeIdentifier(enumName)} ADD VALUE ${escapeLiteral(value)}`; if (beforeValue) { - const sanitizedBeforeValue = removeSqlDDLInjection(beforeValue); - - sql += ` BEFORE '${sanitizedBeforeValue}'`; + sql += ` BEFORE ${escapeLiteral(beforeValue)}`; } else if (afterValue) { - const sanitizedAfterValue = removeSqlDDLInjection(afterValue); - - sql += ` AFTER '${sanitizedAfterValue}'`; + sql += ` AFTER ${escapeLiteral(afterValue)}`; } await queryRunner.query(sql); @@ -122,16 +109,11 @@ export class WorkspaceSchemaEnumManagerService { oldValue: string; newValue: string; }): Promise { - const sanitizedOldValue = removeSqlDDLInjection(oldValue); - const sanitizedNewValue = removeSqlDDLInjection(newValue); - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeEnumName = removeSqlDDLInjection(enumName); - const sql = `ALTER TYPE "${safeSchemaName}"."${safeEnumName}" RENAME VALUE '${sanitizedOldValue}' TO '${sanitizedNewValue}'`; + const sql = `ALTER TYPE ${escapeIdentifier(schemaName)}.${escapeIdentifier(enumName)} RENAME VALUE ${escapeLiteral(oldValue)} TO ${escapeLiteral(newValue)}`; await queryRunner.query(sql); } - // TODO: optimize this to not create a temp enum and column if not necessary (e.g. using ADD VALUE) async alterEnumValues({ queryRunner, schemaName, @@ -255,11 +237,7 @@ export class WorkspaceSchemaEnumManagerService { oldColumnName: string; newColumnName: string; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); - const safeOldColumnName = removeSqlDDLInjection(oldColumnName); - const safeNewColumnName = removeSqlDDLInjection(newColumnName); - const sql = `ALTER TABLE "${safeSchemaName}"."${safeTableName}" RENAME COLUMN "${safeOldColumnName}" TO "${safeNewColumnName}"`; + const sql = `ALTER TABLE ${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)} RENAME COLUMN ${escapeIdentifier(oldColumnName)} TO ${escapeIdentifier(newColumnName)}`; await queryRunner.query(sql); } @@ -277,15 +255,12 @@ export class WorkspaceSchemaEnumManagerService { columnDefinition: WorkspaceSchemaColumnDefinition; enumTypeName: string; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); - const columnDef = buildSqlColumnDefinition({ ...columnDefinition, - type: `"${safeSchemaName}"."${enumTypeName}"`, + type: `${escapeIdentifier(schemaName)}.${escapeIdentifier(enumTypeName)}`, }); - const sql = `ALTER TABLE "${safeSchemaName}"."${safeTableName}" ADD COLUMN ${columnDef}`; + const sql = `ALTER TABLE ${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)} ADD COLUMN ${columnDef}`; await queryRunner.query(sql); } @@ -301,15 +276,11 @@ export class WorkspaceSchemaEnumManagerService { tableName: string; columnName: string; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); - const safeColumnName = removeSqlDDLInjection(columnName); - const sql = `ALTER TABLE "${safeSchemaName}"."${safeTableName}" DROP COLUMN "${safeColumnName}"`; + const sql = `ALTER TABLE ${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)} DROP COLUMN ${escapeIdentifier(columnName)}`; await queryRunner.query(sql); } - // TODO: explore USING clause to avoid the need for this function private async migrateEnumData({ queryRunner, schemaName, @@ -334,36 +305,38 @@ export class WorkspaceSchemaEnumManagerService { columnName: newColumnName, }); - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); - const safeOldColumnName = removeSqlDDLInjection(oldColumnName); - const safeNewColumnName = removeSqlDDLInjection(newColumnName); + const escapedSchema = escapeIdentifier(schemaName); + const escapedTable = escapeIdentifier(tableName); + const escapedOldColumn = escapeIdentifier(oldColumnName); + const escapedNewColumn = escapeIdentifier(newColumnName); + const escapedNewEnumType = `${escapedSchema}.${escapeIdentifier(newEnumTypeName)}`; + const caseStatements = Object.entries(oldToNewEnumOptionMap) .map( ([oldEnumOption, newEnumOption]) => - `WHEN '${removeSqlDDLInjection(oldEnumOption)}' THEN '${removeSqlDDLInjection(newEnumOption)}'::"${safeSchemaName}"."${newEnumTypeName}"`, + `WHEN ${escapeLiteral(oldEnumOption)} THEN ${escapeLiteral(newEnumOption)}::${escapedNewEnumType}`, ) .join(' '); const mappedValuesCondition = Object.keys(oldToNewEnumOptionMap) - .map((oldValue) => `'${removeSqlDDLInjection(oldValue)}'`) + .map((oldValue) => escapeLiteral(oldValue)) .join(', '); const sqlQuery = columnDefinition.isArray ? this.updateArrayEnum({ - safeSchemaName, - safeTableName, - safeOldColumnName, - safeNewColumnName, - newEnumTypeName, - oldEnumTypeName, + escapedSchema, + escapedTable, + escapedOldColumn, + escapedNewColumn, + escapedNewEnumType, + escapedOldEnumType: `${escapedSchema}.${escapeIdentifier(oldEnumTypeName)}`, caseStatements, mappedValuesCondition, }) : this.updateAtomicEnum({ - safeSchemaName, - safeTableName, - safeOldColumnName, - safeNewColumnName, + escapedSchema, + escapedTable, + escapedOldColumn, + escapedNewColumn, caseStatements, mappedValuesCondition, }); @@ -372,61 +345,61 @@ export class WorkspaceSchemaEnumManagerService { } private updateArrayEnum({ - safeNewColumnName, - safeOldColumnName, - safeSchemaName, - safeTableName, - newEnumTypeName, - oldEnumTypeName, + escapedNewColumn, + escapedOldColumn, + escapedSchema, + escapedTable, + escapedNewEnumType, + escapedOldEnumType, caseStatements, mappedValuesCondition, }: { - safeSchemaName: string; - safeTableName: string; - safeOldColumnName: string; - safeNewColumnName: string; - newEnumTypeName: string; - oldEnumTypeName: string; + escapedSchema: string; + escapedTable: string; + escapedOldColumn: string; + escapedNewColumn: string; + escapedNewEnumType: string; + escapedOldEnumType: string; caseStatements: string; mappedValuesCondition: string; }) { return ` - UPDATE "${safeSchemaName}"."${safeTableName}" - SET "${safeNewColumnName}" = ( + UPDATE ${escapedSchema}.${escapedTable} + SET ${escapedNewColumn} = ( SELECT array_agg( CASE unnest_value::text ${caseStatements} - ELSE unnest_value::text::"${safeSchemaName}"."${newEnumTypeName}" + ELSE unnest_value::text::${escapedNewEnumType} END ) - FROM unnest("${safeOldColumnName}") AS unnest_value + FROM unnest(${escapedOldColumn}) AS unnest_value ) - WHERE "${safeOldColumnName}" IS NOT NULL - AND "${safeOldColumnName}" && ARRAY[${mappedValuesCondition}]::"${safeSchemaName}"."${oldEnumTypeName}"[]`; + WHERE ${escapedOldColumn} IS NOT NULL + AND ${escapedOldColumn} && ARRAY[${mappedValuesCondition}]::${escapedOldEnumType}[]`; } private updateAtomicEnum({ - safeNewColumnName, - safeOldColumnName, - safeSchemaName, - safeTableName, + escapedNewColumn, + escapedOldColumn, + escapedSchema, + escapedTable, caseStatements, mappedValuesCondition, }: { caseStatements: string; mappedValuesCondition: string; - safeSchemaName: string; - safeTableName: string; - safeOldColumnName: string; - safeNewColumnName: string; + escapedSchema: string; + escapedTable: string; + escapedOldColumn: string; + escapedNewColumn: string; }) { return ` - UPDATE "${safeSchemaName}"."${safeTableName}" - SET "${safeNewColumnName}" = - CASE "${safeOldColumnName}"::text + UPDATE ${escapedSchema}.${escapedTable} + SET ${escapedNewColumn} = + CASE ${escapedOldColumn}::text ${caseStatements} END - WHERE "${safeOldColumnName}" IS NOT NULL - AND "${safeOldColumnName}"::text IN (${mappedValuesCondition})`; + WHERE ${escapedOldColumn} IS NOT NULL + AND ${escapedOldColumn}::text IN (${mappedValuesCondition})`; } } diff --git a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-foreign-key-manager.service.ts b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-foreign-key-manager.service.ts index b502a7e4534..af76789bca5 100644 --- a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-foreign-key-manager.service.ts +++ b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-foreign-key-manager.service.ts @@ -1,7 +1,15 @@ import { type QueryRunner } from 'typeorm'; import { type WorkspaceSchemaForeignKeyDefinition } from 'src/engine/twenty-orm/workspace-schema-manager/types/workspace-schema-foreign-key-definition.type'; -import { removeSqlDDLInjection } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; +import { escapeIdentifier } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; + +const ALLOWED_FK_ACTIONS = new Set([ + 'CASCADE', + 'SET NULL', + 'RESTRICT', + 'NO ACTION', + 'SET DEFAULT', +]); export class WorkspaceSchemaForeignKeyManagerService { async createForeignKey({ @@ -20,13 +28,19 @@ export class WorkspaceSchemaForeignKeyManagerService { [foreignKey.referencedColumnName], ); - let sql = `ALTER TABLE "${schemaName}"."${foreignKey.tableName}" ADD CONSTRAINT "${foreignKeyName}" FOREIGN KEY ("${foreignKey.columnName}") REFERENCES "${schemaName}"."${foreignKey.referencedTableName}" ("${foreignKey.referencedColumnName}")`; + let sql = `ALTER TABLE ${escapeIdentifier(schemaName)}.${escapeIdentifier(foreignKey.tableName)} ADD CONSTRAINT ${escapeIdentifier(foreignKeyName)} FOREIGN KEY (${escapeIdentifier(foreignKey.columnName)}) REFERENCES ${escapeIdentifier(schemaName)}.${escapeIdentifier(foreignKey.referencedTableName)} (${escapeIdentifier(foreignKey.referencedColumnName)})`; if (foreignKey.onDelete) { + if (!ALLOWED_FK_ACTIONS.has(foreignKey.onDelete)) { + throw new Error(`Unsupported ON DELETE action: ${foreignKey.onDelete}`); + } sql += ` ON DELETE ${foreignKey.onDelete}`; } if (foreignKey.onUpdate) { + if (!ALLOWED_FK_ACTIONS.has(foreignKey.onUpdate)) { + throw new Error(`Unsupported ON UPDATE action: ${foreignKey.onUpdate}`); + } sql += ` ON UPDATE ${foreignKey.onUpdate}`; } @@ -44,10 +58,7 @@ export class WorkspaceSchemaForeignKeyManagerService { tableName: string; foreignKeyName: string; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); - const safeForeignKeyName = removeSqlDDLInjection(foreignKeyName); - const sql = `ALTER TABLE "${safeSchemaName}"."${safeTableName}" DROP CONSTRAINT IF EXISTS "${safeForeignKeyName}"`; + const sql = `ALTER TABLE ${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)} DROP CONSTRAINT IF EXISTS ${escapeIdentifier(foreignKeyName)}`; await queryRunner.query(sql); } @@ -63,10 +74,7 @@ export class WorkspaceSchemaForeignKeyManagerService { tableName: string; foreignKeyName: string; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); - const safeForeignKeyName = removeSqlDDLInjection(foreignKeyName); - const sql = `ALTER TABLE "${safeSchemaName}"."${safeTableName}" ALTER CONSTRAINT "${safeForeignKeyName}" NOT DEFERRABLE`; + const sql = `ALTER TABLE ${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)} ALTER CONSTRAINT ${escapeIdentifier(foreignKeyName)} NOT DEFERRABLE`; await queryRunner.query(sql); } @@ -82,10 +90,7 @@ export class WorkspaceSchemaForeignKeyManagerService { tableName: string; foreignKeyName: string; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); - const safeForeignKeyName = removeSqlDDLInjection(foreignKeyName); - const sql = `ALTER TABLE "${safeSchemaName}"."${safeTableName}" ALTER CONSTRAINT "${safeForeignKeyName}" DEFERRABLE`; + const sql = `ALTER TABLE ${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)} ALTER CONSTRAINT ${escapeIdentifier(foreignKeyName)} DEFERRABLE`; await queryRunner.query(sql); } @@ -101,10 +106,7 @@ export class WorkspaceSchemaForeignKeyManagerService { tableName: string; columnName: string; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); - const safeColumnName = removeSqlDDLInjection(columnName); - + // Uses parameterized query ($1, $2, $3) — safe against injection const foreignKeys = await queryRunner.query( ` SELECT @@ -121,7 +123,7 @@ export class WorkspaceSchemaForeignKeyManagerService { AND tc.table_name = $2 AND kcu.column_name = $3 `, - [safeSchemaName, safeTableName, safeColumnName], + [schemaName, tableName, columnName], ); return foreignKeys[0]?.constraint_name; diff --git a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-index-manager.service.ts b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-index-manager.service.ts index 01e18f0b21b..b81cd5f4543 100644 --- a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-index-manager.service.ts +++ b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-index-manager.service.ts @@ -1,7 +1,17 @@ import { type QueryRunner } from 'typeorm'; import { type WorkspaceSchemaIndexDefinition } from 'src/engine/twenty-orm/workspace-schema-manager/types/workspace-schema-index-definition.type'; -import { removeSqlDDLInjection } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; +import { escapeIdentifier } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; +import { validateAndReturnIndexWhereClause } from 'src/engine/workspace-manager/workspace-migration/utils/validate-index-where-clause.util'; + +const ALLOWED_INDEX_TYPES = new Set([ + 'BTREE', + 'HASH', + 'GIST', + 'SPGIST', + 'GIN', + 'BRIN', +]); export class WorkspaceSchemaIndexManagerService { async createIndex({ @@ -15,25 +25,32 @@ export class WorkspaceSchemaIndexManagerService { tableName: string; index: WorkspaceSchemaIndexDefinition; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); - const safeIndexName = removeSqlDDLInjection(index.name); - - const quotedColumns = index.columns.map( - (column) => `"${removeSqlDDLInjection(column)}"`, + const quotedColumns = index.columns.map((column) => + escapeIdentifier(column), ); const isUnique = index.isUnique ? 'UNIQUE' : ''; - const indexType = - index.type && index.type !== 'BTREE' ? `USING ${index.type}` : ''; - const whereClause = index.where ? `WHERE ${index.where}` : ''; // TODO: to sanitize -> might search for a lib to sanitize sql queries + + let indexType = ''; + + if (index.type && index.type !== 'BTREE') { + if (!ALLOWED_INDEX_TYPES.has(index.type)) { + throw new Error(`Unsupported index type: ${index.type}`); + } + indexType = `USING ${index.type}`; + } + + const validatedWhereClause = validateAndReturnIndexWhereClause(index.where); + const whereClause = validatedWhereClause + ? `WHERE ${validatedWhereClause}` + : ''; const sql = [ 'CREATE', isUnique && 'UNIQUE', 'INDEX IF NOT EXISTS', - `"${safeIndexName}"`, + escapeIdentifier(index.name), 'ON', - `"${safeSchemaName}"."${safeTableName}"`, + `${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)}`, indexType, `(${quotedColumns.join(', ')})`, whereClause, @@ -54,9 +71,7 @@ export class WorkspaceSchemaIndexManagerService { schemaName: string; indexName: string; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeIndexName = removeSqlDDLInjection(indexName); - const sql = `DROP INDEX IF EXISTS "${safeSchemaName}"."${safeIndexName}"`; + const sql = `DROP INDEX IF EXISTS ${escapeIdentifier(schemaName)}.${escapeIdentifier(indexName)}`; await queryRunner.query(sql); } diff --git a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-table-manager.service.ts b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-table-manager.service.ts index e59e276ddd8..690a35859a2 100644 --- a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-table-manager.service.ts +++ b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/services/workspace-schema-table-manager.service.ts @@ -2,7 +2,7 @@ import { type QueryRunner } from 'typeorm'; import { type WorkspaceSchemaColumnDefinition } from 'src/engine/twenty-orm/workspace-schema-manager/types/workspace-schema-column-definition.type'; import { buildSqlColumnDefinition } from 'src/engine/twenty-orm/workspace-schema-manager/utils/build-sql-column-definition.util'; -import { removeSqlDDLInjection } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; +import { escapeIdentifier } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; export class WorkspaceSchemaTableManagerService { async createTable({ @@ -21,16 +21,13 @@ export class WorkspaceSchemaTableManagerService { buildSqlColumnDefinition(columnDefinition), ) || []; - // Add default columns if no columns specified if (sqlColumnDefinitions.length === 0) { sqlColumnDefinitions.push( '"id" uuid PRIMARY KEY DEFAULT gen_random_uuid()', ); } - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); - const sql = `CREATE TABLE IF NOT EXISTS "${safeSchemaName}"."${safeTableName}" (${sqlColumnDefinitions.join(', ')})`; + const sql = `CREATE TABLE IF NOT EXISTS ${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)} (${sqlColumnDefinitions.join(', ')})`; await queryRunner.query(sql); } @@ -46,10 +43,8 @@ export class WorkspaceSchemaTableManagerService { tableName: string; cascade?: boolean; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); const cascadeClause = cascade ? ' CASCADE' : ''; - const sql = `DROP TABLE IF EXISTS "${safeSchemaName}"."${safeTableName}"${cascadeClause}`; + const sql = `DROP TABLE IF EXISTS ${escapeIdentifier(schemaName)}.${escapeIdentifier(tableName)}${cascadeClause}`; await queryRunner.query(sql); } @@ -65,10 +60,7 @@ export class WorkspaceSchemaTableManagerService { oldTableName: string; newTableName: string; }): Promise { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeOldTableName = removeSqlDDLInjection(oldTableName); - const safeNewTableName = removeSqlDDLInjection(newTableName); - const sql = `ALTER TABLE "${safeSchemaName}"."${safeOldTableName}" RENAME TO "${safeNewTableName}"`; + const sql = `ALTER TABLE ${escapeIdentifier(schemaName)}.${escapeIdentifier(oldTableName)} RENAME TO ${escapeIdentifier(newTableName)}`; await queryRunner.query(sql); } diff --git a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/utils/__tests__/sanitize-default-value.util.spec.ts b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/utils/__tests__/sanitize-default-value.util.spec.ts index 1246d92bd39..8af489552f5 100644 --- a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/utils/__tests__/sanitize-default-value.util.spec.ts +++ b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/utils/__tests__/sanitize-default-value.util.spec.ts @@ -3,243 +3,147 @@ import { sanitizeDefaultValue } from 'src/engine/twenty-orm/workspace-schema-man describe('sanitizeDefaultValue', () => { describe('allowed functions', () => { it('should allow uuid_generate_v4() function', () => { - // Prepare const input = 'public.uuid_generate_v4()'; - - // Act const result = sanitizeDefaultValue(input); - // Assert expect(result).toBe('public.uuid_generate_v4()'); }); it('should allow now() function', () => { - // Prepare const input = 'now()'; - - // Act const result = sanitizeDefaultValue(input); - // Assert expect(result).toBe('now()'); }); it('should be case insensitive for allowed functions', () => { - // Act & Assert - expect(sanitizeDefaultValue('NOW()')).toBe('NOW()'); }); }); - describe('SQL injection prevention', () => { - it('should sanitize potential SQL injection in string values', () => { - // Prepare + describe('SQL injection prevention via escapeLiteral', () => { + it('should escape single quotes to prevent SQL injection', () => { const maliciousInput = "'; DROP TABLE users; --"; - - // Act const result = sanitizeDefaultValue(maliciousInput); - // Assert - expect(result).not.toContain('DROP TABLE'); - expect(result).not.toContain(';'); - expect(result).not.toContain('--'); - expect(result).toBe("'DROPTABLEusers'"); + // Single quotes are doubled, making injection impossible + expect(result).toBe("'''; DROP TABLE users; --'"); }); - it('should sanitize quotes in string values', () => { - // Prepare + it('should preserve double quotes inside string literals (safe in SQL strings)', () => { const inputWithQuotes = 'test"value'; - - // Act const result = sanitizeDefaultValue(inputWithQuotes); - // Assert - expect(result).not.toContain('"'); - expect(result).toBe("'testvalue'"); + expect(result).toBe("'test\"value'"); }); - it('should sanitize parentheses in non-function values', () => { - // Prepare + it('should preserve parentheses inside string literals (safe in SQL strings)', () => { const inputWithParens = 'test(value)'; - - // Act const result = sanitizeDefaultValue(inputWithParens); - // Assert - expect(result).not.toContain('('); - expect(result).not.toContain(')'); - expect(result).toBe("'testvalue'"); + expect(result).toBe("'test(value)'"); }); - it('should sanitize backslashes', () => { - // Prepare + it('should escape backslashes with E-string syntax', () => { const inputWithBackslash = 'test\\value'; - - // Act const result = sanitizeDefaultValue(inputWithBackslash); - // Assert - expect(result).not.toContain('\\'); - expect(result).toBe("'testvalue'"); + expect(result).toBe("E'test\\\\value'"); }); - it('should sanitize SQL comment patterns', () => { - // Prepare + it('should preserve comment-like patterns inside string literals (safe in SQL strings)', () => { const inputWithComments = 'value/*comment*/test'; - - // Act const result = sanitizeDefaultValue(inputWithComments); - // Assert - expect(result).not.toContain('/*'); - expect(result).not.toContain('*/'); - expect(result).toBe("'valuecommenttest'"); - }); - - it('should remove non-alphanumeric characters but preserve SQL keywords in alphanumeric form', () => { - // Prepare - const inputWithKeywords = 'SELECT * FROM users'; - - // Act - const result = sanitizeDefaultValue(inputWithKeywords); - - // Assert - expect(result).toBe("'SELECTFROMusers'"); - expect(result).not.toContain('*'); - expect(result).not.toContain(' '); + expect(result).toBe("'value/*comment*/test'"); }); }); describe('regular values', () => { - it('should preserve underscores and alphanumeric characters in simple string values', () => { - // Prepare + it('should preserve simple string values', () => { const input = 'simple_value'; - - // Act const result = sanitizeDefaultValue(input); - // Assert expect(result).toBe("'simple_value'"); }); it('should preserve numeric values', () => { - // Prepare - const input = 12345; - - // Act - const result = sanitizeDefaultValue(input); - - // Assert - expect(result).toBe(12345); + expect(sanitizeDefaultValue(12345)).toBe(12345); }); it('should preserve boolean values', () => { - // Act & Assert expect(sanitizeDefaultValue(true)).toBe(true); expect(sanitizeDefaultValue(false)).toBe(false); }); it('should handle empty string', () => { - // Prepare - const input = ''; - - // Act - const result = sanitizeDefaultValue(input); - - // Assert - expect(result).toBe("''"); + expect(sanitizeDefaultValue('')).toBe("''"); }); - it('should remove whitespace but preserve alphanumeric and underscores', () => { - // Prepare + it('should preserve whitespace in string values', () => { const input = ' test '; - - // Act const result = sanitizeDefaultValue(input); - // Assert - expect(result).toBe("'test'"); + expect(result).toBe("' test '"); }); it('should preserve alphanumeric values with underscores', () => { - // Prepare const input = 'test_value_123'; - - // Act const result = sanitizeDefaultValue(input); - // Assert expect(result).toBe("'test_value_123'"); }); }); describe('mixed cases', () => { it('should distinguish between allowed functions and similar strings', () => { - // Act & Assert expect(sanitizeDefaultValue('now()')).toBe('now()'); expect(sanitizeDefaultValue('now_test')).toBe("'now_test'"); - expect(sanitizeDefaultValue('not_now()')).toBe("'not_now'"); + expect(sanitizeDefaultValue('not_now()')).toBe("'not_now()'"); }); - it('should handle functions with different casing but sanitize non-functions normally', () => { - // Act & Assert + it('should handle functions with different casing but escape non-functions', () => { expect(sanitizeDefaultValue('NOW()')).toBe('NOW()'); expect(sanitizeDefaultValue('now_function')).toBe("'now_function'"); }); - it('should handle complex mixed input', () => { - // Prepare + it('should properly escape complex mixed input', () => { const complexInput = 'test"value; DROP TABLE users; /* comment */ now()'; - - // Act const result = sanitizeDefaultValue(complexInput); - // Assert - expect(result).toBe("'testvalueDROPTABLEuserscommentnow'"); - expect(result).not.toContain(';'); - expect(result).not.toContain('"'); - expect(result).not.toContain('/*'); - expect(result).not.toContain('*/'); - expect(result).not.toContain(' '); + expect(result).toBe( + "'test\"value; DROP TABLE users; /* comment */ now()'", + ); }); }); describe('edge cases', () => { it('should handle null', () => { - // Act & Assert expect(sanitizeDefaultValue(null)).toBe('NULL'); }); it('should handle strings that start with allowed function names', () => { - // Act & Assert expect(sanitizeDefaultValue('now_extended')).toBe("'now_extended'"); expect(sanitizeDefaultValue('gen_random_uuid_custom')).toBe( "'gen_random_uuid_custom'", ); }); - it('should handle strings with special characters', () => { - // Prepare + it('should escape all special characters properly', () => { const specialChars = '!@#$%^&*()+=[]{}|\\:";\'<>?,.'; - - // Act const result = sanitizeDefaultValue(specialChars); - // Assert - expect(result).toBe("''"); + // Backslash triggers E-string, single quotes are doubled + expect(result).toContain('E'); + expect(result).toContain("''"); }); - it('should handle very long strings', () => { - // Prepare - const longString = 'a'.repeat(1000) + '; DROP TABLE users;'; - - // Act + it('should handle very long strings with injection attempts', () => { + const longString = 'a'.repeat(1000) + "'; DROP TABLE users;"; const result = sanitizeDefaultValue(longString); - // Assert - expect(result).toBe(`'${'a'.repeat(1000)}DROPTABLEusers'`); - expect(result).not.toContain(';'); - expect(result).not.toContain(' '); + // The single quote in the injection attempt is properly escaped + expect(result).toBe(`'${'a'.repeat(1000)}''; DROP TABLE users;'`); }); }); }); diff --git a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/utils/build-sql-column-definition.util.ts b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/utils/build-sql-column-definition.util.ts index 1cd0526f684..ebb00ed08be 100644 --- a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/utils/build-sql-column-definition.util.ts +++ b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/utils/build-sql-column-definition.util.ts @@ -1,19 +1,27 @@ import { isDefined } from 'twenty-shared/utils'; import { type WorkspaceSchemaColumnDefinition } from 'src/engine/twenty-orm/workspace-schema-manager/types/workspace-schema-column-definition.type'; -import { removeSqlDDLInjection } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; +import { escapeIdentifier } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; + +const ALLOWED_GENERATED_TYPES = new Set(['STORED', 'VIRTUAL']); export const buildSqlColumnDefinition = ( column: WorkspaceSchemaColumnDefinition, ): string => { - const safeName = removeSqlDDLInjection(column.name); - const parts = [`"${safeName}"`]; + const parts = [escapeIdentifier(column.name)]; + // column.type is either a PostgreSQL type name from fieldMetadataTypeToColumnType + // (safe enum-mapped), or a schema-qualified enum type pre-escaped by the caller. parts.push(column.isArray ? `${column.type}[]` : column.type); + // asExpression is built internally by getTsVectorColumnExpressionFromFields + // (never user-provided). Field names within are escaped at the source. if (column.asExpression && column.type === 'tsvector') { - parts.push(`GENERATED ALWAYS AS (${column.asExpression})`); // TODO: to sanitize - if (column.generatedType) { + parts.push(`GENERATED ALWAYS AS (${column.asExpression})`); + if ( + column.generatedType && + ALLOWED_GENERATED_TYPES.has(column.generatedType) + ) { parts.push(column.generatedType); } } @@ -26,6 +34,8 @@ export const buildSqlColumnDefinition = ( parts.push('NOT NULL'); } + // column.default is pre-serialized by serializeDefaultValue which + // applies escapeLiteral/removeSqlDDLInjection to the value. if (isDefined(column.default) && column.type !== 'tsvector') { parts.push(`DEFAULT ${column.default}`); } diff --git a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/utils/sanitize-default-value.util.ts b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/utils/sanitize-default-value.util.ts index 4a0fdbdc795..607a3972745 100644 --- a/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/utils/sanitize-default-value.util.ts +++ b/packages/twenty-server/src/engine/twenty-orm/workspace-schema-manager/utils/sanitize-default-value.util.ts @@ -1,4 +1,9 @@ -import { removeSqlDDLInjection } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; +import { escapeLiteral } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; + +const ALLOWED_DEFAULT_FUNCTIONS = new Set([ + 'public.uuid_generate_v4()', + 'now()', +]); export const sanitizeDefaultValue = ( defaultValue: string | number | boolean | null, @@ -7,14 +12,12 @@ export const sanitizeDefaultValue = ( return 'NULL'; } - const allowedFunctions = ['public.uuid_generate_v4()', 'now()']; - if (typeof defaultValue === 'string') { - if (allowedFunctions.includes(defaultValue.toLowerCase())) { + if (ALLOWED_DEFAULT_FUNCTIONS.has(defaultValue.toLowerCase())) { return defaultValue; } - return `'${removeSqlDDLInjection(defaultValue)}'`; + return escapeLiteral(defaultValue); } return defaultValue; diff --git a/packages/twenty-server/src/engine/workspace-manager/utils/get-ts-vector-column-expression.util.ts b/packages/twenty-server/src/engine/workspace-manager/utils/get-ts-vector-column-expression.util.ts index 5f55e731898..c24412a0add 100644 --- a/packages/twenty-server/src/engine/workspace-manager/utils/get-ts-vector-column-expression.util.ts +++ b/packages/twenty-server/src/engine/workspace-manager/utils/get-ts-vector-column-expression.util.ts @@ -8,6 +8,7 @@ import { computeCompositeColumnName, } from 'src/engine/metadata-modules/field-metadata/utils/compute-column-name.util'; import { isCompositeFieldMetadataType } from 'src/engine/metadata-modules/field-metadata/utils/is-composite-field-metadata-type.util'; +import { escapeIdentifier } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; import { type SearchableFieldType } from 'src/engine/workspace-manager/utils/is-searchable-field.util'; import { isSearchableSubfield } from 'src/engine/workspace-manager/utils/is-searchable-subfield.util'; @@ -27,7 +28,6 @@ export const getTsVectorColumnExpressionFromFields = ( ? columnExpressions.join(" || ' ' || ") : 'NULL'; - // Note: changing this expression requires reindexing/backfilling existing searchVector values. return `to_tsvector('simple', ${concatenatedExpression})`; }; @@ -59,9 +59,15 @@ const getColumnExpressionsFromField = ( }); if (fieldMetadataTypeAndName.type === FieldMetadataType.PHONES) { - const phoneNumberColumn = `"${fieldMetadataTypeAndName.name}PrimaryPhoneNumber"`; - const callingCodeColumn = `"${fieldMetadataTypeAndName.name}PrimaryPhoneCallingCode"`; - const additionalPhonesColumn = `"${fieldMetadataTypeAndName.name}AdditionalPhones"`; + const phoneNumberColumn = escapeIdentifier( + `${fieldMetadataTypeAndName.name}PrimaryPhoneNumber`, + ); + const callingCodeColumn = escapeIdentifier( + `${fieldMetadataTypeAndName.name}PrimaryPhoneCallingCode`, + ); + const additionalPhonesColumn = escapeIdentifier( + `${fieldMetadataTypeAndName.name}AdditionalPhones`, + ); const internationalFormats = [ `COALESCE(${callingCodeColumn} || ${phoneNumberColumn}, '')`, @@ -79,7 +85,9 @@ const getColumnExpressionsFromField = ( } if (fieldMetadataTypeAndName.type === FieldMetadataType.LINKS) { - const secondaryLinksColumn = `"${fieldMetadataTypeAndName.name}SecondaryLinks"`; + const secondaryLinksColumn = escapeIdentifier( + `${fieldMetadataTypeAndName.name}SecondaryLinks`, + ); const secondaryLinksExpression = `COALESCE(public.unaccent_immutable(TRANSLATE(regexp_replace(${secondaryLinksColumn}::text, '"(label|url)"\\s*:\\s*', '', 'g'), '[]{}",:', ' ')), '')`; @@ -87,7 +95,9 @@ const getColumnExpressionsFromField = ( } if (fieldMetadataTypeAndName.type === FieldMetadataType.EMAILS) { - const additionalEmailsColumn = `"${fieldMetadataTypeAndName.name}AdditionalEmails"`; + const additionalEmailsColumn = escapeIdentifier( + `${fieldMetadataTypeAndName.name}AdditionalEmails`, + ); const additionalEmailsExpression = `COALESCE(public.unaccent_immutable(TRANSLATE(${additionalEmailsColumn}::text, '[]",', ' ')), '') || ' ' || COALESCE(public.unaccent_immutable(TRANSLATE(REPLACE(${additionalEmailsColumn}::text, '@', ' '), '[]",', ' ')), '')`; @@ -105,7 +115,7 @@ const getColumnExpression = ( columnName: string, fieldType: FieldMetadataType, ): string => { - const quotedColumnName = `"${columnName}"`; + const quotedColumnName = escapeIdentifier(columnName); switch (fieldType) { case FieldMetadataType.EMAILS: diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/__tests__/remove-sql-injection.util.spec.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/__tests__/remove-sql-injection.util.spec.ts new file mode 100644 index 00000000000..b923d4fc978 --- /dev/null +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/__tests__/remove-sql-injection.util.spec.ts @@ -0,0 +1,88 @@ +import { + escapeIdentifier, + escapeLiteral, + removeSqlDDLInjection, +} from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; + +describe('removeSqlDDLInjection', () => { + it('should strip non-alphanumeric/underscore characters', () => { + expect(removeSqlDDLInjection('my_table')).toBe('my_table'); + expect(removeSqlDDLInjection('table"name')).toBe('tablename'); + expect(removeSqlDDLInjection('drop;--')).toBe('drop'); + }); +}); + +describe('escapeIdentifier', () => { + it('should wrap identifier in double quotes', () => { + expect(escapeIdentifier('myTable')).toBe('"myTable"'); + }); + + it('should double internal double-quote characters', () => { + expect(escapeIdentifier('my"table')).toBe('"my""table"'); + expect(escapeIdentifier('a""b')).toBe('"a""""b"'); + }); + + it('should handle empty string', () => { + expect(escapeIdentifier('')).toBe('""'); + }); + + it('should handle single-quote characters without modification', () => { + expect(escapeIdentifier("it's")).toBe('"it\'s"'); + }); + + it('should reject null bytes', () => { + expect(() => escapeIdentifier('my\0table')).toThrow( + 'Null bytes are not allowed in PostgreSQL identifiers', + ); + }); + + it('should handle SQL injection attempts in identifiers', () => { + expect(escapeIdentifier('"; DROP TABLE users; --')).toBe( + '"""; DROP TABLE users; --"', + ); + }); +}); + +describe('escapeLiteral', () => { + it('should wrap value in single quotes', () => { + expect(escapeLiteral('hello')).toBe("'hello'"); + }); + + it('should double internal single-quote characters', () => { + expect(escapeLiteral("it's")).toBe("'it''s'"); + expect(escapeLiteral("a''b")).toBe("'a''''b'"); + }); + + it('should handle empty string', () => { + expect(escapeLiteral('')).toBe("''"); + }); + + it('should escape backslashes and add E prefix', () => { + expect(escapeLiteral('test\\value')).toBe("E'test\\\\value'"); + }); + + it('should handle both single quotes and backslashes', () => { + expect(escapeLiteral("it's a \\path")).toBe("E'it''s a \\\\path'"); + }); + + it('should not add E prefix when no backslashes present', () => { + expect(escapeLiteral('simple')).toBe("'simple'"); + expect(escapeLiteral("it's")).toBe("'it''s'"); + }); + + it('should reject null bytes', () => { + expect(() => escapeLiteral('my\0value')).toThrow( + 'Null bytes are not allowed in PostgreSQL string literals', + ); + }); + + it('should handle SQL injection attempts in literals', () => { + expect(escapeLiteral("'; DROP TABLE users; --")).toBe( + "'''; DROP TABLE users; --'", + ); + }); + + it('should handle double quotes without modification', () => { + expect(escapeLiteral('test"value')).toBe("'test\"value'"); + }); +}); diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/__tests__/validate-index-where-clause.util.spec.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/__tests__/validate-index-where-clause.util.spec.ts new file mode 100644 index 00000000000..b17f0668932 --- /dev/null +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/__tests__/validate-index-where-clause.util.spec.ts @@ -0,0 +1,27 @@ +import { validateAndReturnIndexWhereClause } from 'src/engine/workspace-manager/workspace-migration/utils/validate-index-where-clause.util'; + +describe('validateAndReturnIndexWhereClause', () => { + it('should return undefined for null/undefined/empty input', () => { + expect(validateAndReturnIndexWhereClause(null)).toBeUndefined(); + expect(validateAndReturnIndexWhereClause(undefined)).toBeUndefined(); + expect(validateAndReturnIndexWhereClause('')).toBeUndefined(); + }); + + it('should return the clause when it is in the allowlist', () => { + expect(validateAndReturnIndexWhereClause('"deletedAt" IS NULL')).toBe( + '"deletedAt" IS NULL', + ); + }); + + it('should throw for clauses not in the allowlist', () => { + expect(() => + validateAndReturnIndexWhereClause('1=1; DROP TABLE users;'), + ).toThrow('Unsupported index WHERE clause'); + }); + + it('should throw for subtle variants of allowed clauses', () => { + expect(() => + validateAndReturnIndexWhereClause('"deletedAt" IS NOT NULL'), + ).toThrow('Unsupported index WHERE clause'); + }); +}); diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util.ts index 42a890c7e46..48e4977cfe3 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util.ts @@ -1,3 +1,49 @@ +// Strips all characters except [a-zA-Z0-9_]. +// Use ONLY for generating safe identifier names (e.g. enum names from table+column). +// For SQL escaping, use escapeIdentifier or escapeLiteral instead. export const removeSqlDDLInjection = (value: string): string => { return value.replace(/[^a-zA-Z0-9_]/g, ''); }; + +// PostgreSQL standard identifier quoting: wraps in double quotes and +// doubles any internal double-quote characters. +// e.g. my"table → "my""table" +export const escapeIdentifier = (identifier: string): string => { + if (identifier.includes('\0')) { + throw new Error('Null bytes are not allowed in PostgreSQL identifiers'); + } + + return '"' + identifier.replace(/"/g, '""') + '"'; +}; + +// PostgreSQL standard literal quoting: wraps in single quotes and +// doubles any internal single-quote characters. Prefixes with E when +// backslashes are present (standard_conforming_strings safety). +// e.g. it's → 'it''s' +export const escapeLiteral = (value: string): string => { + if (value.includes('\0')) { + throw new Error('Null bytes are not allowed in PostgreSQL string literals'); + } + + let hasBackslash = false; + let escaped = "'"; + + for (const char of value) { + if (char === "'") { + escaped += "''"; + } else if (char === '\\') { + escaped += '\\\\'; + hasBackslash = true; + } else { + escaped += char; + } + } + + escaped += "'"; + + if (hasBackslash) { + escaped = 'E' + escaped; + } + + return escaped; +}; diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/validate-index-where-clause.util.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/validate-index-where-clause.util.ts new file mode 100644 index 00000000000..fa1439341fb --- /dev/null +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/validate-index-where-clause.util.ts @@ -0,0 +1,21 @@ +// Allowlist of safe WHERE clause patterns for partial indexes. +// Any new pattern must be reviewed for SQL injection safety before being added. +const ALLOWED_INDEX_WHERE_CLAUSES = new Set(['"deletedAt" IS NULL']); + +export const validateAndReturnIndexWhereClause = ( + clause: string | null | undefined, +): string | undefined => { + if (!clause) { + return undefined; + } + + if (ALLOWED_INDEX_WHERE_CLAUSES.has(clause)) { + return clause; + } + + throw new Error( + `Unsupported index WHERE clause: "${clause}". ` + + 'Only allowlisted patterns are permitted to prevent SQL injection. ' + + 'Add the pattern to ALLOWED_INDEX_WHERE_CLAUSES after security review.', + ); +}; diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/utils/serialize-default-value.util.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/utils/serialize-default-value.util.ts index bafb58532b7..c69bcd42f75 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/utils/serialize-default-value.util.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/utils/serialize-default-value.util.ts @@ -1,5 +1,5 @@ -import { type ColumnType } from 'typeorm'; import { type FieldMetadataDefaultValueForAnyType } from 'twenty-shared/types'; +import { type ColumnType } from 'typeorm'; import { FieldMetadataException, @@ -7,7 +7,16 @@ import { } from 'src/engine/metadata-modules/field-metadata/field-metadata.exception'; import { isFunctionDefaultValue } from 'src/engine/metadata-modules/field-metadata/utils/is-function-default-value.util'; import { serializeFunctionDefaultValue } from 'src/engine/metadata-modules/field-metadata/utils/serialize-function-default-value.util'; -import { removeSqlDDLInjection } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; +import { + escapeIdentifier, + escapeLiteral, + removeSqlDDLInjection, +} from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; + +// Default values arrive pre-quoted with single quotes (e.g. "'OPTION_1'"). +// Strip them so escapeLiteral can re-quote properly. +const stripSurroundingQuotes = (value: string): string => + value.startsWith("'") && value.endsWith("'") ? value.slice(1, -1) : value; type SerializeDefaultValueArgs = { defaultValue?: FieldMetadataDefaultValueForAnyType; @@ -23,15 +32,10 @@ export const serializeDefaultValue = ({ tableName, columnName, }: SerializeDefaultValueArgs) => { - const safeSchemaName = removeSqlDDLInjection(schemaName); - const safeTableName = removeSqlDDLInjection(tableName); - const safeColumnName = removeSqlDDLInjection(columnName); - if (defaultValue === undefined || defaultValue === null) { return 'NULL'; } - // Function default values if (isFunctionDefaultValue(defaultValue)) { const serializedTypeDefaultValue = serializeFunctionDefaultValue(defaultValue); @@ -46,13 +50,16 @@ export const serializeDefaultValue = ({ return serializedTypeDefaultValue; } + // Enum types need a schema-qualified cast; others use the column type directly. + // Enum name is built from sanitized table+column (removeSqlDDLInjection strips + // to [a-zA-Z0-9_]) to match computePostgresEnumName. const castSuffix = columnType === 'enum' - ? `::${safeSchemaName}."${safeTableName}_${safeColumnName}_enum"` + ? `::${escapeIdentifier(schemaName)}.${escapeIdentifier(`${removeSqlDDLInjection(tableName)}_${removeSqlDDLInjection(columnName)}_enum`)}` : `::${columnType}`; - const sanitizeAndAddCastPrefix = (defaultValue: string) => - `'${removeSqlDDLInjection(defaultValue)}'` + castSuffix; + const escapeAndCast = (rawValue: string) => + escapeLiteral(rawValue) + castSuffix; switch (typeof defaultValue) { case 'string': { @@ -63,27 +70,26 @@ export const serializeDefaultValue = ({ ); } - return sanitizeAndAddCastPrefix(defaultValue); + return escapeAndCast(stripSurroundingQuotes(defaultValue)); } case 'boolean': case 'number': { - return sanitizeAndAddCastPrefix(`${defaultValue}`); + return escapeAndCast(`${defaultValue}`); } case 'object': { if (defaultValue instanceof Date) { - return sanitizeAndAddCastPrefix(`'${defaultValue.toISOString()}'`); + return escapeAndCast(defaultValue.toISOString()); } if (Array.isArray(defaultValue)) { const arrayValues = defaultValue - .map((val) => `'${removeSqlDDLInjection(val)}'`) + .map((val) => escapeLiteral(stripSurroundingQuotes(String(val)))) .join(','); return `ARRAY[${arrayValues}]${castSuffix}[]`; } - // Default value for objects won't work with sanitization here - return sanitizeAndAddCastPrefix(`'${JSON.stringify(defaultValue)}'`); + return escapeAndCast(JSON.stringify(defaultValue)); } default: { throw new FieldMetadataException(