Refactor search vector field (#21947)
# Introduction Refactoring the search vector field validation <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/twentyhq/twenty/pull/21947?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
This commit is contained in:
+11
-1
@@ -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;
|
||||
|
||||
+9
-3
@@ -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 &&
|
||||
|
||||
+102
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
+74
@@ -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).
|
||||
|
||||
+74
@@ -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);
|
||||
});
|
||||
+148
@@ -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<FieldMetadataType.TS_VECTOR>;
|
||||
};
|
||||
|
||||
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,
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user