From 67074a75815dd59fcda5c17108f6682e2a87a4c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Malfait?= Date: Wed, 18 Feb 2026 21:41:01 +0100 Subject: [PATCH] Harden server-side input validation and auth defaults (#18018) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - **File storage (LocalDriver):** Add realpath resolution and symlink rejection to `writeFile`, `downloadFile`, and `downloadFolder` — brings them in line with the existing `readFile` protections. Includes unit tests. - **JWT:** Pin signing/verification to HS256 explicitly. - **Auth:** Revoke active refresh tokens when a user changes their password. - **Logic functions:** Validate `handlerName` as a safe JS identifier at both DTO and runtime level, preventing injection into the generated runner script. - **User entity:** Remove `passwordHash` from the GraphQL schema (`@Field` decorator removed, column stays). - **Query params:** Use `crypto.randomBytes` instead of `Math.random` for SQL parameter name generation. - **Exception filter:** Mirror the request `Origin` header instead of sending `Access-Control-Allow-Origin: *`. ## Test plan - [x] `local.driver.spec.ts` — writeFile rejects symlinks, downloadFile rejects paths outside storage - [ ] Verify JWT auth flow still works (login, token refresh) - [ ] Verify password change invalidates existing sessions - [ ] Verify logic function creation with valid/invalid handler names - [ ] Verify file upload/download in dev environment Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> --- .../src/generated-metadata/graphql.ts | 1 - .../utils/compute-where-condition-parts.ts | 78 +++++++++--------- .../auth/services/auth.service.ts | 14 +++- .../drivers/__tests__/local.driver.spec.ts | 81 +++++++++++++++++++ .../file-storage/drivers/local.driver.ts | 79 ++++++++++++++++-- .../src/engine/core-modules/jwt/jwt.module.ts | 4 + .../drivers/local.driver.ts | 6 ++ .../engine/core-modules/user/user.entity.ts | 1 - .../dtos/create-logic-function.input.ts | 4 + .../dtos/logic-function-source.input.ts | 5 +- ...update-logic-function-from-source.input.ts | 4 + 11 files changed, 227 insertions(+), 50 deletions(-) create mode 100644 packages/twenty-server/src/engine/core-modules/file-storage/drivers/__tests__/local.driver.spec.ts diff --git a/packages/twenty-front/src/generated-metadata/graphql.ts b/packages/twenty-front/src/generated-metadata/graphql.ts index 3eb5ef11e5..e1e02f7971 100644 --- a/packages/twenty-front/src/generated-metadata/graphql.ts +++ b/packages/twenty-front/src/generated-metadata/graphql.ts @@ -4874,7 +4874,6 @@ export type User = { lastName: Scalars['String']; locale: Scalars['String']; onboardingStatus?: Maybe; - passwordHash?: Maybe; supportUserHash?: Maybe; updatedAt: Scalars['DateTime']; userVars?: Maybe; diff --git a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts index d20bf040fd..b755df97da 100644 --- a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts +++ b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/utils/compute-where-condition-parts.ts @@ -1,3 +1,5 @@ +import { randomBytes } from 'crypto'; + import { type FieldMetadataType } from 'twenty-shared/types'; import { isDefined } from 'twenty-shared/utils'; import { type ObjectLiteral } from 'typeorm'; @@ -33,9 +35,9 @@ export const computeWhereConditionParts = ({ fieldMetadataType: FieldMetadataType; useDirectTableReference?: boolean; }): WhereConditionParts => { - const uuid = Math.random().toString(36).slice(2, 7); + const paramSuffix = randomBytes(5).toString('hex'); - const secondUuid = Math.random().toString(36).slice(2, 7); + const secondParamSuffix = randomBytes(5).toString('hex'); const fieldReference = useDirectTableReference ? `"${key}"` @@ -58,99 +60,99 @@ export const computeWhereConditionParts = ({ }; case 'eq': return { - sql: `${fieldReference} = :${key}${uuid}${hasNullEquivalentFieldValue ? ` OR ${fieldReference} IS NULL` : ''}`, - params: { [`${key}${uuid}`]: value }, + sql: `${fieldReference} = :${key}${paramSuffix}${hasNullEquivalentFieldValue ? ` OR ${fieldReference} IS NULL` : ''}`, + params: { [`${key}${paramSuffix}`]: value }, }; case 'neq': return { - sql: `${fieldReference} != :${key}${uuid}${hasNullEquivalentFieldValue ? ` OR ${fieldReference} IS NOT NULL` : ''}`, - params: { [`${key}${uuid}`]: value }, + sql: `${fieldReference} != :${key}${paramSuffix}${hasNullEquivalentFieldValue ? ` OR ${fieldReference} IS NOT NULL` : ''}`, + params: { [`${key}${paramSuffix}`]: value }, }; case 'gt': return { - sql: `${fieldReference} > :${key}${uuid}`, - params: { [`${key}${uuid}`]: value }, + sql: `${fieldReference} > :${key}${paramSuffix}`, + params: { [`${key}${paramSuffix}`]: value }, }; case 'gte': return { - sql: `${fieldReference} >= :${key}${uuid}`, - params: { [`${key}${uuid}`]: value }, + sql: `${fieldReference} >= :${key}${paramSuffix}`, + params: { [`${key}${paramSuffix}`]: value }, }; case 'lt': return { - sql: `${fieldReference} < :${key}${uuid}`, - params: { [`${key}${uuid}`]: value }, + sql: `${fieldReference} < :${key}${paramSuffix}`, + params: { [`${key}${paramSuffix}`]: value }, }; case 'lte': return { - sql: `${fieldReference} <= :${key}${uuid}`, - params: { [`${key}${uuid}`]: value }, + sql: `${fieldReference} <= :${key}${paramSuffix}`, + params: { [`${key}${paramSuffix}`]: value }, }; case 'in': return { - sql: `${fieldReference} IN (:...${key}${uuid})`, - params: { [`${key}${uuid}`]: value }, + sql: `${fieldReference} IN (:...${key}${paramSuffix})`, + params: { [`${key}${paramSuffix}`]: value }, }; case 'is': return { - sql: `${fieldReference} IS ${value === 'NULL' ? 'NULL' : 'NOT NULL'}${hasNullEquivalentFieldValue ? ` OR ${fieldReference} = :${key}${secondUuid}` : ''}`, + sql: `${fieldReference} IS ${value === 'NULL' ? 'NULL' : 'NOT NULL'}${hasNullEquivalentFieldValue ? ` OR ${fieldReference} = :${key}${secondParamSuffix}` : ''}`, params: hasNullEquivalentFieldValue - ? { [`${key}${secondUuid}`]: nullEquivalentFieldValue } + ? { [`${key}${secondParamSuffix}`]: nullEquivalentFieldValue } : {}, }; case 'like': return { - sql: `${fieldReference}::text LIKE :${key}${uuid}${hasNullEquivalentFieldValue ? ` OR ${fieldReference} IS NULL` : ''}`, - params: { [`${key}${uuid}`]: `${value}` }, + sql: `${fieldReference}::text LIKE :${key}${paramSuffix}${hasNullEquivalentFieldValue ? ` OR ${fieldReference} IS NULL` : ''}`, + params: { [`${key}${paramSuffix}`]: `${value}` }, }; case 'ilike': return { - sql: `${fieldReference}::text ILIKE :${key}${uuid}${hasNullEquivalentFieldValue ? ` OR ${fieldReference} IS NULL` : ''}`, - params: { [`${key}${uuid}`]: `${value}` }, + sql: `${fieldReference}::text ILIKE :${key}${paramSuffix}${hasNullEquivalentFieldValue ? ` OR ${fieldReference} IS NULL` : ''}`, + params: { [`${key}${paramSuffix}`]: `${value}` }, }; case 'startsWith': return { - sql: `${fieldReference}::text ^@ :${key}${uuid}`, - params: { [`${key}${uuid}`]: `${value}` }, + sql: `${fieldReference}::text ^@ :${key}${paramSuffix}`, + params: { [`${key}${paramSuffix}`]: `${value}` }, }; case 'endsWith': return { - sql: `RIGHT(${fieldReference}::text, LENGTH(:${key}${uuid})) = :${key}${uuid}`, - params: { [`${key}${uuid}`]: `${value}` }, + sql: `RIGHT(${fieldReference}::text, LENGTH(:${key}${paramSuffix})) = :${key}${paramSuffix}`, + params: { [`${key}${paramSuffix}`]: `${value}` }, }; case 'contains': return { - sql: `${fieldReference} @> ARRAY[:...${key}${uuid}]`, - params: { [`${key}${uuid}`]: value }, + sql: `${fieldReference} @> ARRAY[:...${key}${paramSuffix}]`, + params: { [`${key}${paramSuffix}`]: value }, }; case 'search': { const tsQuery = formatSearchTerms(value, 'and'); return { sql: `( - ${fieldReference} @@ to_tsquery('simple', public.unaccent_immutable(:${key}${uuid}Ts)) OR - public.unaccent_immutable(${fieldReference}::text) ILIKE public.unaccent_immutable(:${key}${uuid}Like) + ${fieldReference} @@ to_tsquery('simple', public.unaccent_immutable(:${key}${paramSuffix}Ts)) OR + public.unaccent_immutable(${fieldReference}::text) ILIKE public.unaccent_immutable(:${key}${paramSuffix}Like) )`, params: { - [`${key}${uuid}Ts`]: tsQuery, - [`${key}${uuid}Like`]: `%${value}%`, + [`${key}${paramSuffix}Ts`]: tsQuery, + [`${key}${paramSuffix}Like`]: `%${value}%`, }, }; } case 'notContains': return { - sql: `NOT (${fieldReference}::text[] && ARRAY[:...${key}${uuid}]::text[])`, - params: { [`${key}${uuid}`]: value }, + sql: `NOT (${fieldReference}::text[] && ARRAY[:...${key}${paramSuffix}]::text[])`, + params: { [`${key}${paramSuffix}`]: value }, }; case 'containsAny': return { - sql: `${fieldReference}::text[] && ARRAY[:...${key}${uuid}]::text[]`, - params: { [`${key}${uuid}`]: value }, + sql: `${fieldReference}::text[] && ARRAY[:...${key}${paramSuffix}]::text[]`, + params: { [`${key}${paramSuffix}`]: value }, }; case 'containsIlike': return { - sql: `EXISTS (SELECT 1 FROM unnest(${fieldReference}) AS elem WHERE elem ILIKE :${key}${uuid})`, - params: { [`${key}${uuid}`]: value }, + sql: `EXISTS (SELECT 1 FROM unnest(${fieldReference}) AS elem WHERE elem ILIKE :${key}${paramSuffix})`, + params: { [`${key}${paramSuffix}`]: value }, }; default: throw new GraphqlQueryRunnerException( diff --git a/packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts b/packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts index ed6fc6dc9d..b75ddf1d84 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts @@ -11,7 +11,7 @@ import { PasswordUpdateNotifyEmail } from 'twenty-emails'; import { PermissionFlagType } from 'twenty-shared/constants'; import { AppPath } from 'twenty-shared/types'; import { assertIsDefinedOrThrow, isDefined } from 'twenty-shared/utils'; -import { Repository } from 'typeorm'; +import { IsNull, Repository } from 'typeorm'; import { NodeEnvironment } from 'src/engine/core-modules/twenty-config/interfaces/node-environment.interface'; @@ -618,6 +618,18 @@ export class AuthService { passwordHash: newPasswordHash, }); + // Invalidate all existing refresh tokens for this user across all workspaces + await this.appTokenRepository.update( + { + userId, + type: AppTokenType.RefreshToken, + revokedAt: IsNull(), + }, + { + revokedAt: new Date(), + }, + ); + const emailTemplate = PasswordUpdateNotifyEmail({ userName: `${user.firstName} ${user.lastName}`, email: user.email, diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/drivers/__tests__/local.driver.spec.ts b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/__tests__/local.driver.spec.ts new file mode 100644 index 0000000000..7c100e0e21 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/__tests__/local.driver.spec.ts @@ -0,0 +1,81 @@ +import { mkdtemp, mkdir, readFile, rm, symlink, writeFile } from 'fs/promises'; +import { tmpdir } from 'os'; +import path from 'path'; + +import { FileStorageExceptionCode } from 'src/engine/core-modules/file-storage/interfaces/file-storage-exception'; + +import { LocalDriver } from 'src/engine/core-modules/file-storage/drivers/local.driver'; + +describe('LocalDriver security hardening', () => { + const cleanupPaths: string[] = []; + + const createTempDirectory = async (prefix: string) => { + const dir = await mkdtemp(path.join(tmpdir(), prefix)); + + cleanupPaths.push(dir); + + return dir; + }; + + afterAll(async () => { + await Promise.all( + cleanupPaths.map(async (directoryPath) => { + await rm(directoryPath, { recursive: true, force: true }); + }), + ); + }); + + it('should reject writeFile when target is a symlink', async () => { + const storagePath = await createTempDirectory('local-driver-storage-'); + const outsidePath = await createTempDirectory('local-driver-outside-'); + const outsideFilePath = path.join(outsidePath, 'outside.txt'); + const symlinkFolderPath = path.join(storagePath, 'workspace', 'app'); + const symlinkFilePath = path.join(symlinkFolderPath, 'target.txt'); + + await mkdir(symlinkFolderPath, { recursive: true }); + await writeFile(outsideFilePath, 'outside'); + await symlink(outsideFilePath, symlinkFilePath); + + const driver = new LocalDriver({ storagePath }); + + await expect( + driver.writeFile({ + filePath: 'workspace/app/target.txt', + sourceFile: Buffer.from('new-content'), + mimeType: undefined, + }), + ).rejects.toMatchObject({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }); + + await expect(readFile(outsideFilePath, 'utf8')).resolves.toBe('outside'); + }); + + it('should reject downloadFile when path resolves outside storage', async () => { + const storagePath = await createTempDirectory('local-driver-storage-'); + const outsidePath = await createTempDirectory('local-driver-outside-'); + const outsideFilePath = path.join(outsidePath, 'outside.txt'); + const symlinkFolderPath = path.join(storagePath, 'workspace', 'app'); + const symlinkFilePath = path.join(symlinkFolderPath, 'target.txt'); + const downloadDestinationPath = path.join( + storagePath, + 'download', + 'file.txt', + ); + + await mkdir(symlinkFolderPath, { recursive: true }); + await writeFile(outsideFilePath, 'outside'); + await symlink(outsideFilePath, symlinkFilePath); + + const driver = new LocalDriver({ storagePath }); + + await expect( + driver.downloadFile({ + onStoragePath: 'workspace/app/target.txt', + localPath: downloadDestinationPath, + }), + ).rejects.toMatchObject({ + code: FileStorageExceptionCode.ACCESS_DENIED, + }); + }); +}); diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/drivers/local.driver.ts b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/local.driver.ts index 6995fe0ec7..f21b1017bb 100644 --- a/packages/twenty-server/src/engine/core-modules/file-storage/drivers/local.driver.ts +++ b/packages/twenty-server/src/engine/core-modules/file-storage/drivers/local.driver.ts @@ -74,17 +74,49 @@ export class LocalDriver implements StorageDriver { await this.createFolder(folderPath); - await fs.writeFile(filePath, params.sourceFile); + const realFolderPath = realpathSync(folderPath); + const realFilePath = path.join(realFolderPath, path.basename(filePath)); + + this.assertRealPathIsWithinStorage(realFilePath); + + try { + const stats = await fs.lstat(realFilePath); + + if (stats.isSymbolicLink()) { + throw new FileStorageException( + 'Access denied', + FileStorageExceptionCode.ACCESS_DENIED, + ); + } + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + throw error; + } + } + + await fs.writeFile(realFilePath, params.sourceFile); } async downloadFile(params: { onStoragePath: string; localPath: string; }): Promise { - const filePath = path.resolve( + const resolvedPath = path.resolve( this.options.storagePath, params.onStoragePath, ); + let filePath: string; + + try { + filePath = realpathSync(resolvedPath); + } catch { + throw new FileStorageException( + 'File not found', + FileStorageExceptionCode.FILE_NOT_FOUND, + ); + } + + this.assertRealPathIsWithinStorage(filePath); await this.createFolder(dirname(params.localPath)); @@ -97,26 +129,57 @@ export class LocalDriver implements StorageDriver { onStoragePath: string; localPath: string; }): Promise { - const rootFolderPath = path.resolve( + const resolvedPath = path.resolve( this.options.storagePath, params.onStoragePath, ); + let rootFolderPath: string; + + try { + rootFolderPath = realpathSync(resolvedPath); + } catch { + throw new FileStorageException( + 'File not found', + FileStorageExceptionCode.FILE_NOT_FOUND, + ); + } + + this.assertRealPathIsWithinStorage(rootFolderPath); await this.createFolder(params.localPath); - const resources = await fs.readdir(rootFolderPath); + await this.downloadFolderFromRealPath({ + rootFolderPath, + localPath: params.localPath, + }); + } + + private async downloadFolderFromRealPath(params: { + rootFolderPath: string; + localPath: string; + }): Promise { + await this.createFolder(params.localPath); + + const resources = await fs.readdir(params.rootFolderPath); for (const resource of resources) { - const resourcePath = path.join(rootFolderPath, resource); - const stats = await fs.stat(resourcePath); + const resourcePath = path.join(params.rootFolderPath, resource); + const stats = await fs.lstat(resourcePath); + + if (stats.isSymbolicLink()) { + throw new FileStorageException( + 'Access denied', + FileStorageExceptionCode.ACCESS_DENIED, + ); + } if (stats.isFile()) { const content = await fs.readFile(resourcePath); await fs.writeFile(path.join(params.localPath, resource), content); } else { - await this.downloadFolder({ - onStoragePath: path.join(params.onStoragePath, resource), + await this.downloadFolderFromRealPath({ + rootFolderPath: resourcePath, localPath: path.join(params.localPath, resource), }); } diff --git a/packages/twenty-server/src/engine/core-modules/jwt/jwt.module.ts b/packages/twenty-server/src/engine/core-modules/jwt/jwt.module.ts index d021aee35d..43e67c006f 100644 --- a/packages/twenty-server/src/engine/core-modules/jwt/jwt.module.ts +++ b/packages/twenty-server/src/engine/core-modules/jwt/jwt.module.ts @@ -10,8 +10,12 @@ const InternalJwtModule = NestJwtModule.registerAsync({ return { secret: twentyConfigService.get('APP_SECRET'), signOptions: { + algorithm: 'HS256', expiresIn: twentyConfigService.get('ACCESS_TOKEN_EXPIRES_IN'), }, + verifyOptions: { + algorithms: ['HS256'], + }, }; }, inject: [TwentyConfigService], diff --git a/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-drivers/drivers/local.driver.ts b/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-drivers/drivers/local.driver.ts index 094989abe8..bce2968edd 100644 --- a/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-drivers/drivers/local.driver.ts +++ b/packages/twenty-server/src/engine/core-modules/logic-function/logic-function-drivers/drivers/local.driver.ts @@ -215,6 +215,12 @@ export class LocalDriver implements LogicFunctionDriver { builtFileAbsPath: string; handlerName: string; }) { + if (!/^[a-zA-Z_$][a-zA-Z0-9_$]*$/.test(handlerName)) { + throw new Error( + `Invalid handlerName "${handlerName}": must be a valid JavaScript identifier`, + ); + } + const runnerPath = join(dir, '__runner.cjs'); const code = ` // Auto-generated. Do not edit. diff --git a/packages/twenty-server/src/engine/core-modules/user/user.entity.ts b/packages/twenty-server/src/engine/core-modules/user/user.entity.ts index d4870b646a..61d0c13902 100644 --- a/packages/twenty-server/src/engine/core-modules/user/user.entity.ts +++ b/packages/twenty-server/src/engine/core-modules/user/user.entity.ts @@ -70,7 +70,6 @@ export class UserEntity { @Column({ default: false }) disabled: boolean; - @Field({ nullable: true }) @Column({ nullable: true }) passwordHash: string; diff --git a/packages/twenty-server/src/engine/metadata-modules/logic-function/dtos/create-logic-function.input.ts b/packages/twenty-server/src/engine/metadata-modules/logic-function/dtos/create-logic-function.input.ts index 0e5f097b0e..d00e16f721 100644 --- a/packages/twenty-server/src/engine/metadata-modules/logic-function/dtos/create-logic-function.input.ts +++ b/packages/twenty-server/src/engine/metadata-modules/logic-function/dtos/create-logic-function.input.ts @@ -8,6 +8,7 @@ import { IsOptional, IsString, IsUUID, + Matches, Max, Min, } from 'class-validator'; @@ -71,6 +72,9 @@ export class CreateLogicFunction { checksum?: string; @IsString() + @Matches(/^[a-zA-Z_$][a-zA-Z0-9_$]*$/, { + message: 'handlerName must be a valid JavaScript identifier', + }) @Field({ nullable: false }) handlerName: string; diff --git a/packages/twenty-server/src/engine/metadata-modules/logic-function/dtos/logic-function-source.input.ts b/packages/twenty-server/src/engine/metadata-modules/logic-function/dtos/logic-function-source.input.ts index f287e7b8c6..1973ca38d1 100644 --- a/packages/twenty-server/src/engine/metadata-modules/logic-function/dtos/logic-function-source.input.ts +++ b/packages/twenty-server/src/engine/metadata-modules/logic-function/dtos/logic-function-source.input.ts @@ -1,6 +1,6 @@ import { Field, InputType } from '@nestjs/graphql'; -import { IsObject, IsString } from 'class-validator'; +import { IsObject, IsString, Matches } from 'class-validator'; import graphqlTypeJson from 'graphql-type-json'; @InputType() @@ -14,6 +14,9 @@ export class LogicFunctionSourceInput { toolInputSchema: object; @IsString() + @Matches(/^[a-zA-Z_$][a-zA-Z0-9_$]*$/, { + message: 'handlerName must be a valid JavaScript identifier', + }) @Field({ nullable: false }) handlerName: string; } diff --git a/packages/twenty-server/src/engine/metadata-modules/logic-function/dtos/update-logic-function-from-source.input.ts b/packages/twenty-server/src/engine/metadata-modules/logic-function/dtos/update-logic-function-from-source.input.ts index f188ab21aa..c1de0b29d7 100644 --- a/packages/twenty-server/src/engine/metadata-modules/logic-function/dtos/update-logic-function-from-source.input.ts +++ b/packages/twenty-server/src/engine/metadata-modules/logic-function/dtos/update-logic-function-from-source.input.ts @@ -9,6 +9,7 @@ import { IsOptional, IsString, IsUUID, + Matches, Max, Min, ValidateNested, @@ -53,6 +54,9 @@ class UpdateLogicFunctionFromSourceInputUpdates { toolInputSchema?: object; @IsString() + @Matches(/^[a-zA-Z_$][a-zA-Z0-9_$]*$/, { + message: 'handlerName must be a valid JavaScript identifier', + }) @Field({ nullable: true }) @IsOptional() handlerName?: string;