Use proper PostgreSQL identifier/literal escaping in workspace DDL (#18024)

## Summary - Replace the character-stripping approach (`removeSqlDDLInjection`) with standard PostgreSQL `escapeIdentifier` and `escapeLiteral` functions across all workspace schema manager services - Add missing identifier escaping to `createForeignKey` (was the only method in the FK manager without it) - Add allowlist validation for index WHERE clauses and FK action types - Harden tsvector expression builder with proper identifier quoting ## Context The workspace schema managers build DDL dynamically from metadata (table names, column names, enum values, etc.). The previous approach stripped all non-alphanumeric characters — safe but lossy (silently corrupts values with legitimate special characters). The new approach uses PostgreSQL's standard escaping: - **Identifiers**: double internal `"` and wrap → `"my""table"` (same algorithm as `pg` driver's `escapeIdentifier`) - **Literals**: double internal `'` and wrap → `'it''s a value'` (same algorithm as `pg` driver's `escapeLiteral`) `removeSqlDDLInjection` is kept only for name generation (e.g., `computePostgresEnumName`) where stripping to `[a-zA-Z0-9_]` is the correct behavior. ## Files changed | File | What | |------|------| | `remove-sql-injection.util.ts` | Added `escapeIdentifier` + `escapeLiteral` | | `validate-index-where-clause.util.ts` | New — allowlist for partial index WHERE clauses | | 5 schema manager services | Replaced strip+manual-quote with `escapeIdentifier`/`escapeLiteral` | | `build-sql-column-definition.util.ts` | `escapeIdentifier` for column names, validated `generatedType` | | `sanitize-default-value.util.ts` | `escapeLiteral` instead of stripping | | `serialize-default-value.util.ts` | `escapeLiteral` for values, `escapeIdentifier` for enum casts | | `get-ts-vector-column-expression.util.ts` | `escapeIdentifier` for field names in expressions | | `sanitize-default-value.util.spec.ts` | Updated tests for escape behavior | ## Test plan - [x] All 64 existing tests pass across 6 test suites - [x] `lint:diff-with-main` passes - [x] TypeScript typecheck — no new errors - [ ] Verify workspace sync-metadata still works end-to-end - [ ] Verify custom object/field creation works - [ ] Verify enum field option changes work Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor <cursoragent@cursor.com>
2026-02-18 15:24:10 +01:00
parent f4a61f26c0
commit 3bd431e95d
14 changed files with 410 additions and 321 deletions
@@ -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:
@@ -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'");
+  });
+});
@@ -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');
+  });
+});
@@ -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;
+};
@@ -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.',
+  );
+};
@@ -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(