From 0b8368cd6c1a47711bf52972800f162bde0bbab9 Mon Sep 17 00:00:00 2001 From: Paul Rastoin <45004772+prastoin@users.noreply.github.com> Date: Mon, 22 Jun 2026 13:44:52 +0200 Subject: [PATCH] Refactor search vector field (#21947) # Introduction Refactoring the search vector field validation Review in cubic --- ...date-ts-vector-flat-field-metadata.util.ts | 12 +- .../utils/build-sql-column-definition.util.ts | 12 +- .../remove-sql-injection.util.spec.ts | 102 ++++++++++++ .../utils/remove-sql-injection.util.ts | 74 +++++++++ ...-expression-validation.integration-spec.ts | 74 +++++++++ ...-expression-validation.integration-spec.ts | 148 ++++++++++++++++++ 6 files changed, 418 insertions(+), 4 deletions(-) create mode 100644 packages/twenty-server/test/integration/metadata/suites/application/failing-sync-application-search-vector-expression-validation.integration-spec.ts create mode 100644 packages/twenty-server/test/integration/metadata/suites/field-metadata/update-one-field-metadata-search-vector-expression-validation.integration-spec.ts diff --git a/packages/twenty-server/src/engine/metadata-modules/flat-field-metadata/validators/utils/validate-ts-vector-flat-field-metadata.util.ts b/packages/twenty-server/src/engine/metadata-modules/flat-field-metadata/validators/utils/validate-ts-vector-flat-field-metadata.util.ts index bdf964289e..3cca34acee 100644 --- a/packages/twenty-server/src/engine/metadata-modules/flat-field-metadata/validators/utils/validate-ts-vector-flat-field-metadata.util.ts +++ b/packages/twenty-server/src/engine/metadata-modules/flat-field-metadata/validators/utils/validate-ts-vector-flat-field-metadata.util.ts @@ -1,9 +1,11 @@ import { msg } from '@lingui/core/macro'; +import { isNonEmptyString } from '@sniptt/guards'; import { type FieldMetadataType } from 'twenty-shared/types'; import { FieldMetadataExceptionCode } from 'src/engine/metadata-modules/field-metadata/field-metadata.exception'; import { type FlatFieldMetadataTypeValidationArgs } from 'src/engine/metadata-modules/flat-field-metadata/types/flat-field-metadata-type-validator.type'; import { type FlatFieldMetadataValidationError } from 'src/engine/metadata-modules/flat-field-metadata/types/flat-field-metadata-validation-error.type'; +import { isSafeTsVectorExpression } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; export const validateTsVectorFlatFieldMetadata = ({ flatEntityToValidate, @@ -28,13 +30,21 @@ export const validateTsVectorFlatFieldMetadata = ({ }); } - if (!flatEntityToValidate.universalSettings?.asExpression) { + const asExpression = flatEntityToValidate.universalSettings?.asExpression; + + if (!isNonEmptyString(asExpression)) { errors.push({ code: FieldMetadataExceptionCode.INVALID_FIELD_INPUT, message: 'Field type TS_VECTOR must have an expression. This may have failed to be built because record identifier field does not exist or is not of a searchable type.', userFriendlyMessage: msg`Field type TS_VECTOR must have an expression. This may have failed to be built because record identifier field does not exist or is not of a searchable type.`, }); + } else if (!isSafeTsVectorExpression(asExpression)) { + errors.push({ + code: FieldMetadataExceptionCode.INVALID_FIELD_INPUT, + message: 'Field type TS_VECTOR expression is invalid', + userFriendlyMessage: msg`The search field expression is invalid.`, + }); } return errors; 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 ebb00ed08b..41bd8bc7ec 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,7 +1,10 @@ import { isDefined } from 'twenty-shared/utils'; import { type WorkspaceSchemaColumnDefinition } from 'src/engine/twenty-orm/workspace-schema-manager/types/workspace-schema-column-definition.type'; -import { escapeIdentifier } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; +import { + assertSafeTsVectorExpression, + escapeIdentifier, +} from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; const ALLOWED_GENERATED_TYPES = new Set(['STORED', 'VIRTUAL']); @@ -14,9 +17,12 @@ export const buildSqlColumnDefinition = ( // (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. + // asExpression is normally built server-side by getTsVectorColumnExpressionFromFields, but it + // can technically reach here from user input (metadata API or app-sync manifest). The TS_VECTOR + // validator rejects corrupted expressions on every build path; this assert is the last-resort + // guard at the DDL sink so nothing can break out of the GENERATED ALWAYS AS (...) clause. if (column.asExpression && column.type === 'tsvector') { + assertSafeTsVectorExpression(column.asExpression); parts.push(`GENERATED ALWAYS AS (${column.asExpression})`); if ( column.generatedType && 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 index b923d4fc97..fd912d7789 100644 --- 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 @@ -1,6 +1,8 @@ import { + assertSafeTsVectorExpression, escapeIdentifier, escapeLiteral, + isSafeTsVectorExpression, removeSqlDDLInjection, } from 'src/engine/workspace-manager/workspace-migration/utils/remove-sql-injection.util'; @@ -86,3 +88,103 @@ describe('escapeLiteral', () => { expect(escapeLiteral('test"value')).toBe("'test\"value'"); }); }); + +describe('assertSafeTsVectorExpression', () => { + it('should accept a real server-generated tsvector expression', () => { + const generated = `to_tsvector('simple', COALESCE(public.unaccent_immutable("name"), '') || ' ' || COALESCE("emailsPrimaryEmail"::text, ''))`; + + expect(() => assertSafeTsVectorExpression(generated)).not.toThrow(); + }); + + it('should reject expressions containing a statement terminator', () => { + expect(() => + assertSafeTsVectorExpression( + `to_tsvector('simple', coalesce("name", ''))) STORED; CREATE TABLE core."x" (a text); --`, + ), + ).toThrow('Unsafe tsvector expression detected'); + }); + + it('should reject expressions containing a line comment', () => { + expect(() => + assertSafeTsVectorExpression(`to_tsvector('simple', '') -- comment`), + ).toThrow('Unsafe tsvector expression detected'); + }); + + it('should reject expressions containing a block comment', () => { + expect(() => + assertSafeTsVectorExpression(`to_tsvector('simple', '') /* comment */`), + ).toThrow('Unsafe tsvector expression detected'); + }); + + it('should reject null bytes', () => { + expect(() => + assertSafeTsVectorExpression(`to_tsvector('simple', '\0')`), + ).toThrow('Unsafe tsvector expression detected'); + }); + + it('should reject a parenthesis-balanced clause-injection that uses no forbidden token', () => { + // Closes the wrapping AS( early and injects a sibling ADD COLUMN clause - no ";" or comment. + expect(() => + assertSafeTsVectorExpression( + `to_tsvector('simple', coalesce("x",''))) STORED, ADD COLUMN "evil" text GENERATED ALWAYS AS (to_tsvector('simple', '')`, + ), + ).toThrow('Unsafe tsvector expression detected'); + }); + + it('should reject dollar-quoting used to smuggle a breakout parenthesis', () => { + // PostgreSQL treats the quotes inside $$...$$ as literal text, so a single-quote-only scanner + // would skip the real ) between the two dollar-quoted segments and miscount it as balanced. + expect(() => + assertSafeTsVectorExpression( + `to_tsvector('simple', $$'$$ ) STORED, ADD COLUMN "evil" text $$'$$`, + ), + ).toThrow('Unsafe tsvector expression detected'); + }); + + it('should reject a double-quoted-identifier desync that hides a breakout parenthesis', () => { + // PostgreSQL reads "'" as identifiers (named '), so the middle ) is real code that breaks out. + // A single-quote-only scanner instead treats that ) as inside a string literal and accepts it. + expect(() => assertSafeTsVectorExpression(`"'")"'"`)).toThrow( + 'Unsafe tsvector expression detected', + ); + }); +}); + +describe('isSafeTsVectorExpression', () => { + it('should return true for a safe generated expression', () => { + expect( + isSafeTsVectorExpression( + `to_tsvector('simple', COALESCE(public.unaccent_immutable("name"), ''))`, + ), + ).toBe(true); + }); + + it('should accept balanced parentheses that appear inside string literals', () => { + expect( + isSafeTsVectorExpression( + `to_tsvector('simple', regexp_replace("x"::text, '"(a|b)"\\s*:\\s*', '', 'g'))`, + ), + ).toBe(true); + expect(isSafeTsVectorExpression(`COALESCE("x", ')')`)).toBe(true); + }); + + it('should accept parentheses that appear inside a double-quoted identifier', () => { + expect(isSafeTsVectorExpression(`COALESCE("weird)name", '')`)).toBe(true); + }); + + it('should reject expressions with a forbidden token', () => { + expect(isSafeTsVectorExpression(`to_tsvector('simple', '') ; DROP`)).toBe( + false, + ); + }); + + it('should reject any expression containing a dollar sign', () => { + expect(isSafeTsVectorExpression(`to_tsvector('simple', $$x$$)`)).toBe( + false, + ); + }); + + it('should reject expressions with unbalanced parentheses', () => { + expect(isSafeTsVectorExpression(`coalesce("x", '')) STORED`)).toBe(false); + }); +}); 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 48e4977cfe..0955b11317 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 @@ -16,6 +16,80 @@ export const escapeIdentifier = (identifier: string): string => { return '"' + identifier.replace(/"/g, '""') + '"'; }; +const FORBIDDEN_TS_VECTOR_EXPRESSION_TOKENS = [ + '\0', + ';', + '--', + '/*', + '*/', + '$', +]; + +const hasBalancedParentheses = (expression: string): boolean => { + let depth = 0; + let context: 'code' | 'string' | 'identifier' = 'code'; + + for (let index = 0; index < expression.length; index++) { + const character = expression[index]; + + if (context === 'string') { + if (character === "'") { + if (expression[index + 1] === "'") { + index++; + } else { + context = 'code'; + } + } + continue; + } + + if (context === 'identifier') { + if (character === '"') { + if (expression[index + 1] === '"') { + index++; + } else { + context = 'code'; + } + } + continue; + } + + if (character === "'") { + context = 'string'; + } else if (character === '"') { + context = 'identifier'; + } else if (character === '(') { + depth++; + } else if (character === ')') { + depth--; + + if (depth < 0) { + return false; + } + } + } + + return depth === 0 && context === 'code'; +}; + +export const isSafeTsVectorExpression = (expression: string): boolean => { + const hasForbiddenToken = FORBIDDEN_TS_VECTOR_EXPRESSION_TOKENS.some( + (token) => expression.includes(token), + ); + + if (hasForbiddenToken) { + return false; + } + + return hasBalancedParentheses(expression); +}; + +export const assertSafeTsVectorExpression = (expression: string): void => { + if (!isSafeTsVectorExpression(expression)) { + throw new Error('Unsafe tsvector expression detected'); + } +}; + // 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). diff --git a/packages/twenty-server/test/integration/metadata/suites/application/failing-sync-application-search-vector-expression-validation.integration-spec.ts b/packages/twenty-server/test/integration/metadata/suites/application/failing-sync-application-search-vector-expression-validation.integration-spec.ts new file mode 100644 index 0000000000..0bcaca96d6 --- /dev/null +++ b/packages/twenty-server/test/integration/metadata/suites/application/failing-sync-application-search-vector-expression-validation.integration-spec.ts @@ -0,0 +1,74 @@ +import { buildBaseManifest } from 'test/integration/metadata/suites/application/utils/build-base-manifest.util'; +import { buildDefaultObjectManifest } from 'test/integration/metadata/suites/application/utils/build-default-object-manifest.util'; +import { cleanupApplicationAndAppRegistration } from 'test/integration/metadata/suites/application/utils/cleanup-application-and-app-registration.util'; +import { setupApplicationForSync } from 'test/integration/metadata/suites/application/utils/setup-application-for-sync.util'; +import { syncApplication } from 'test/integration/metadata/suites/application/utils/sync-application.util'; +import { jestExpectToBeDefined } from 'test/utils/jest-expect-to-be-defined.util.test'; +import { type Manifest, type ObjectManifest } from 'twenty-shared/application'; +import { v4 as uuidv4 } from 'uuid'; + +const TEST_APP_ID = uuidv4(); +const TEST_ROLE_ID = uuidv4(); + +describe('Sync application - search vector expression is validated', () => { + const RUN_SUFFIX = `${Date.now()}`; + + beforeAll(async () => { + await setupApplicationForSync({ + applicationUniversalIdentifier: TEST_APP_ID, + name: 'Test SearchVector Expression App', + description: 'App for testing search vector expression validation', + sourcePath: 'test-search-vector-expression', + }); + }, 60000); + + afterAll(async () => { + await cleanupApplicationAndAppRegistration({ + applicationUniversalIdentifier: TEST_APP_ID, + }); + }); + + it('rejects a manifest carrying a malformed searchVector expression', async () => { + const defaultObject = buildDefaultObjectManifest({ + nameSingular: `searchVectorManifestObject${RUN_SUFFIX}`, + namePlural: `searchVectorManifestObjects${RUN_SUFFIX}`, + labelSingular: 'Search Vector Manifest Object', + labelPlural: 'Search Vector Manifest Objects', + description: + 'Object whose manifest carries a malformed searchVector expression', + }); + + const invalidAsExpression = `to_tsvector('simple', coalesce("id"::text, '')`; + + const objectWithInvalidSearchVector: ObjectManifest = { + ...defaultObject, + fields: defaultObject.fields.map((field) => + field.name === 'searchVector' + ? ({ + ...field, + universalSettings: { + asExpression: invalidAsExpression, + generatedType: 'STORED', + }, + } as (typeof defaultObject.fields)[number]) + : field, + ), + }; + + const manifest: Manifest = buildBaseManifest({ + appId: TEST_APP_ID, + roleId: TEST_ROLE_ID, + overrides: { + objects: [objectWithInvalidSearchVector], + fields: [], + }, + }); + + const { errors } = await syncApplication({ + manifest, + expectToFail: true, + }); + + jestExpectToBeDefined(errors); + }, 60000); +}); diff --git a/packages/twenty-server/test/integration/metadata/suites/field-metadata/update-one-field-metadata-search-vector-expression-validation.integration-spec.ts b/packages/twenty-server/test/integration/metadata/suites/field-metadata/update-one-field-metadata-search-vector-expression-validation.integration-spec.ts new file mode 100644 index 0000000000..0b6fa44d08 --- /dev/null +++ b/packages/twenty-server/test/integration/metadata/suites/field-metadata/update-one-field-metadata-search-vector-expression-validation.integration-spec.ts @@ -0,0 +1,148 @@ +import { createOneFieldMetadata } from 'test/integration/metadata/suites/field-metadata/utils/create-one-field-metadata.util'; +import { updateOneFieldMetadata } from 'test/integration/metadata/suites/field-metadata/utils/update-one-field-metadata.util'; +import { createOneObjectMetadata } from 'test/integration/metadata/suites/object-metadata/utils/create-one-object-metadata.util'; +import { deleteOneObjectMetadata } from 'test/integration/metadata/suites/object-metadata/utils/delete-one-object-metadata.util'; +import { findManyObjectMetadata } from 'test/integration/metadata/suites/object-metadata/utils/find-many-object-metadata.util'; +import { updateOneObjectMetadata } from 'test/integration/metadata/suites/object-metadata/utils/update-one-object-metadata.util'; +import { jestExpectToBeDefined } from 'test/utils/jest-expect-to-be-defined.util.test'; +import { FieldMetadataType } from 'twenty-shared/types'; + +import { type FieldMetadataDTO } from 'src/engine/metadata-modules/field-metadata/dtos/field-metadata.dto'; + +describe('Field metadata update - search vector expression is validated', () => { + let testObjectMetadataId: string; + let labelIdentifierFieldMetadataId: string; + let searchVectorFieldMetadataId: string; + let originalAsExpression: string; + + const RUN_SUFFIX = `${Date.now()}`; + const OBJECT_NAME_SINGULAR = `searchVectorObject${RUN_SUFFIX}`; + const OBJECT_NAME_PLURAL = `searchVectorObjects${RUN_SUFFIX}`; + const LABEL_FIELD_NAME = `searchVectorTitle${RUN_SUFFIX}`; + + const getSearchVectorField = async () => { + const { objects } = await findManyObjectMetadata({ + expectToFail: false, + input: { + filter: { id: { eq: testObjectMetadataId } }, + paging: { first: 1 }, + }, + gqlFields: ` + id + nameSingular + fieldsList { + id + name + type + settings + } + `, + }); + + const testObject = objects[0]; + + jestExpectToBeDefined(testObject); + jestExpectToBeDefined(testObject.fieldsList); + + const searchVectorField = testObject.fieldsList.find( + (field: FieldMetadataDTO) => field.type === FieldMetadataType.TS_VECTOR, + ); + + jestExpectToBeDefined(searchVectorField); + + return searchVectorField as FieldMetadataDTO; + }; + + beforeAll(async () => { + const { + data: { + createOneObject: { id: objectMetadataId }, + }, + } = await createOneObjectMetadata({ + expectToFail: false, + input: { + nameSingular: OBJECT_NAME_SINGULAR, + namePlural: OBJECT_NAME_PLURAL, + labelSingular: 'Search Vector Object', + labelPlural: 'Search Vector Objects', + icon: 'IconSearch', + isLabelSyncedWithName: false, + }, + }); + + testObjectMetadataId = objectMetadataId; + + const { + data: { + createOneField: { id: fieldMetadataId }, + }, + } = await createOneFieldMetadata({ + expectToFail: false, + input: { + name: LABEL_FIELD_NAME, + label: 'Search Vector Title', + type: FieldMetadataType.TEXT, + objectMetadataId: testObjectMetadataId, + isLabelSyncedWithName: false, + }, + gqlFields: `id name label`, + }); + + labelIdentifierFieldMetadataId = fieldMetadataId; + + await updateOneObjectMetadata({ + input: { + idToUpdate: testObjectMetadataId, + updatePayload: { + labelIdentifierFieldMetadataId, + }, + }, + expectToFail: false, + }); + + const searchVectorField = await getSearchVectorField(); + + searchVectorFieldMetadataId = searchVectorField.id; + originalAsExpression = searchVectorField.settings?.asExpression ?? ''; + }); + + afterAll(async () => { + await updateOneObjectMetadata({ + expectToFail: false, + input: { + idToUpdate: testObjectMetadataId, + updatePayload: { isActive: false }, + }, + }); + await deleteOneObjectMetadata({ + expectToFail: false, + input: { idToDelete: testObjectMetadataId }, + }); + }); + + it('rejects a malformed searchVector expression and leaves the stored expression unchanged', async () => { + const invalidAsExpression = `to_tsvector('simple', coalesce("${LABEL_FIELD_NAME}", '')`; + + const { errors } = await updateOneFieldMetadata({ + input: { + idToUpdate: searchVectorFieldMetadataId, + updatePayload: { + settings: { + asExpression: invalidAsExpression, + generatedType: 'STORED', + }, + }, + }, + gqlFields: `id name settings`, + expectToFail: true, + }); + + jestExpectToBeDefined(errors); + + const searchVectorFieldAfter = await getSearchVectorField(); + + expect(searchVectorFieldAfter.settings?.asExpression).toBe( + originalAsExpression, + ); + }); +});