From 570c57563a077a826f5c769ff91f68c2c73e3789 Mon Sep 17 00:00:00 2001 From: Paul Rastoin <45004772+prastoin@users.noreply.github.com> Date: Thu, 21 May 2026 14:45:54 +0200 Subject: [PATCH] Upload application file resolver exception management and integration coverage (#20803) # Introduction Earlier and better exception handling of the upload application file resolver + coverage --- .../application-development.resolver.ts | 20 ++- .../dtos/upload-application-file.input.ts | 6 + .../is-safe-relative-path.validator.ts | 30 ++++ ...le-path-traversal.integration-spec.ts.snap | 91 ++++++++++++ ...on-file-path-traversal.integration-spec.ts | 126 +++++++++++++++++ ...pload-application-file.integration-spec.ts | 132 ++++++++++++++++++ 6 files changed, 402 insertions(+), 3 deletions(-) create mode 100644 packages/twenty-server/src/engine/core-modules/file-storage/validators/is-safe-relative-path.validator.ts create mode 100644 packages/twenty-server/test/integration/metadata/suites/application/__snapshots__/failing-upload-application-file-path-traversal.integration-spec.ts.snap create mode 100644 packages/twenty-server/test/integration/metadata/suites/application/failing-upload-application-file-path-traversal.integration-spec.ts create mode 100644 packages/twenty-server/test/integration/metadata/suites/application/successful-upload-application-file.integration-spec.ts diff --git a/packages/twenty-server/src/engine/core-modules/application/application-development/application-development.resolver.ts b/packages/twenty-server/src/engine/core-modules/application/application-development/application-development.resolver.ts index 283ec3bc0d..1ca37a0a82 100644 --- a/packages/twenty-server/src/engine/core-modules/application/application-development/application-development.resolver.ts +++ b/packages/twenty-server/src/engine/core-modules/application/application-development/application-development.resolver.ts @@ -22,9 +22,9 @@ import { UploadApplicationFileInput } from 'src/engine/core-modules/application/ import { WorkspaceMigrationDTO } from 'src/engine/core-modules/application/application-development/dtos/workspace-migration.dto'; import { ApplicationExceptionFilter } from 'src/engine/core-modules/application/application-exception-filter'; import { ApplicationSyncService } from 'src/engine/core-modules/application/application-manifest/application-sync.service'; +import { resolveManifestAssetUrls } from 'src/engine/core-modules/application/application-marketplace/utils/resolve-manifest-asset-urls.util'; import { ApplicationTokenPairDTO } from 'src/engine/core-modules/application/application-oauth/dtos/application-token-pair.dto'; import { ApplicationRegistrationVariableService } from 'src/engine/core-modules/application/application-registration-variable/application-registration-variable.service'; -import { resolveManifestAssetUrls } from 'src/engine/core-modules/application/application-marketplace/utils/resolve-manifest-asset-urls.util'; import { ApplicationRegistrationService } from 'src/engine/core-modules/application/application-registration/application-registration.service'; import { ApplicationRegistrationSourceType } from 'src/engine/core-modules/application/application-registration/enums/application-registration-source-type.enum'; import { @@ -37,14 +37,14 @@ import { FileStorageService } from 'src/engine/core-modules/file-storage/file-st import { FileDTO } from 'src/engine/core-modules/file/dtos/file.dto'; import { ResolverValidationPipe } from 'src/engine/core-modules/graphql/pipes/resolver-validation.pipe'; import { SdkClientGenerationService } from 'src/engine/core-modules/sdk-client/sdk-client-generation.service'; +import { ThrottlerService } from 'src/engine/core-modules/throttler/throttler.service'; import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service'; import { type WorkspaceEntity } from 'src/engine/core-modules/workspace/workspace.entity'; -import { AuthUser } from 'src/engine/decorators/auth/auth-user.decorator'; import { AuthUserWorkspaceId } from 'src/engine/decorators/auth/auth-user-workspace-id.decorator'; +import { AuthUser } from 'src/engine/decorators/auth/auth-user.decorator'; import { AuthWorkspace } from 'src/engine/decorators/auth/auth-workspace.decorator'; import { SettingsPermissionGuard } from 'src/engine/guards/settings-permission.guard'; import { WorkspaceAuthGuard } from 'src/engine/guards/workspace-auth.guard'; -import { ThrottlerService } from 'src/engine/core-modules/throttler/throttler.service'; import { WorkspaceMigrationGraphqlApiExceptionInterceptor } from 'src/engine/workspace-manager/workspace-migration/interceptors/workspace-migration-graphql-api-exception.interceptor'; import { streamToBuffer } from 'src/utils/stream-to-buffer'; @@ -219,6 +219,20 @@ export class ApplicationDevelopmentResolver { ); } + const application = await this.applicationService.findByUniversalIdentifier( + { + universalIdentifier: applicationUniversalIdentifier, + workspaceId, + }, + ); + + if (!isDefined(application)) { + throw new ApplicationException( + 'Application not found in workspace.', + ApplicationExceptionCode.APPLICATION_NOT_FOUND, + ); + } + const buffer = await streamToBuffer(createReadStream()); return await this.fileStorageService.writeFile({ diff --git a/packages/twenty-server/src/engine/core-modules/application/application-development/dtos/upload-application-file.input.ts b/packages/twenty-server/src/engine/core-modules/application/application-development/dtos/upload-application-file.input.ts index d21069b00d..1e69a52e2d 100644 --- a/packages/twenty-server/src/engine/core-modules/application/application-development/dtos/upload-application-file.input.ts +++ b/packages/twenty-server/src/engine/core-modules/application/application-development/dtos/upload-application-file.input.ts @@ -1,15 +1,21 @@ import { ArgsType, Field } from '@nestjs/graphql'; +import { IsNotEmpty } from 'class-validator'; import { FileFolder } from 'twenty-shared/types'; +import { IsSafeRelativePath } from 'src/engine/core-modules/file-storage/validators/is-safe-relative-path.validator'; + @ArgsType() export class UploadApplicationFileInput { @Field(() => String) + @IsNotEmpty() applicationUniversalIdentifier: string; @Field(() => FileFolder) fileFolder: FileFolder; @Field(() => String) + @IsNotEmpty() + @IsSafeRelativePath() filePath: string; } diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/validators/is-safe-relative-path.validator.ts b/packages/twenty-server/src/engine/core-modules/file-storage/validators/is-safe-relative-path.validator.ts new file mode 100644 index 0000000000..9f5b7e873a --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file-storage/validators/is-safe-relative-path.validator.ts @@ -0,0 +1,30 @@ +import { + registerDecorator, + type ValidationArguments, + type ValidationOptions, +} from 'class-validator'; + +import { isSafeRelativePath } from 'src/engine/core-modules/file-storage/utils/is-safe-relative-path.util'; + +export function IsSafeRelativePath(validationOptions?: ValidationOptions) { + return function (object: object, propertyName: string) { + registerDecorator({ + name: 'IsSafeRelativePath', + target: object.constructor, + propertyName, + options: validationOptions, + validator: { + validate(value: unknown) { + if (typeof value !== 'string') { + return false; + } + + return isSafeRelativePath(value); + }, + defaultMessage(args: ValidationArguments) { + return `${args.property} contains unsafe characters or path traversal`; + }, + }, + }); + }; +} diff --git a/packages/twenty-server/test/integration/metadata/suites/application/__snapshots__/failing-upload-application-file-path-traversal.integration-spec.ts.snap b/packages/twenty-server/test/integration/metadata/suites/application/__snapshots__/failing-upload-application-file-path-traversal.integration-spec.ts.snap new file mode 100644 index 0000000000..8914852ea4 --- /dev/null +++ b/packages/twenty-server/test/integration/metadata/suites/application/__snapshots__/failing-upload-application-file-path-traversal.integration-spec.ts.snap @@ -0,0 +1,91 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`Upload application file should fail when applicationUniversalIdentifier does not match any installed application 1`] = ` +{ + "extensions": { + "code": "NOT_FOUND", + "subCode": "APPLICATION_NOT_FOUND", + "userFriendlyMessage": "Application not found.", + }, + "message": "Application not found in workspace.", + "name": "NotFoundError", +} +`; + +exports[`Upload application file should fail when applicationUniversalIdentifier is empty 1`] = ` +{ + "extensions": { + "code": "BAD_USER_INPUT", + "userFriendlyMessage": "An error occurred.", + }, + "message": "applicationUniversalIdentifier should not be empty", + "name": "UserInputError", +} +`; + +exports[`Upload application file should fail when fileFolder is not an allowed application file folder 1`] = ` +{ + "extensions": { + "code": "BAD_USER_INPUT", + "subCode": "INVALID_INPUT", + "userFriendlyMessage": "Invalid input provided.", + }, + "message": "Invalid fileFolder for application file upload. Allowed values: built-logic-function, built-front-component, public-asset, source, dependencies", + "name": "UserInputError", +} +`; + +exports[`Upload application file should fail when filePath contains backslash path traversal 1`] = ` +{ + "extensions": { + "code": "BAD_USER_INPUT", + "userFriendlyMessage": "An error occurred.", + }, + "message": "filePath contains unsafe characters or path traversal", + "name": "UserInputError", +} +`; + +exports[`Upload application file should fail when filePath contains relative path traversal (../) 1`] = ` +{ + "extensions": { + "code": "BAD_USER_INPUT", + "userFriendlyMessage": "An error occurred.", + }, + "message": "filePath contains unsafe characters or path traversal", + "name": "UserInputError", +} +`; + +exports[`Upload application file should fail when filePath contains upward traversal (../../) 1`] = ` +{ + "extensions": { + "code": "BAD_USER_INPUT", + "userFriendlyMessage": "An error occurred.", + }, + "message": "filePath contains unsafe characters or path traversal", + "name": "UserInputError", +} +`; + +exports[`Upload application file should fail when filePath is an absolute path 1`] = ` +{ + "extensions": { + "code": "BAD_USER_INPUT", + "userFriendlyMessage": "An error occurred.", + }, + "message": "filePath contains unsafe characters or path traversal", + "name": "UserInputError", +} +`; + +exports[`Upload application file should fail when filePath is empty 1`] = ` +{ + "extensions": { + "code": "BAD_USER_INPUT", + "userFriendlyMessage": "An error occurred.", + }, + "message": "filePath contains unsafe characters or path traversal, filePath should not be empty", + "name": "UserInputError", +} +`; diff --git a/packages/twenty-server/test/integration/metadata/suites/application/failing-upload-application-file-path-traversal.integration-spec.ts b/packages/twenty-server/test/integration/metadata/suites/application/failing-upload-application-file-path-traversal.integration-spec.ts new file mode 100644 index 0000000000..d42533347f --- /dev/null +++ b/packages/twenty-server/test/integration/metadata/suites/application/failing-upload-application-file-path-traversal.integration-spec.ts @@ -0,0 +1,126 @@ +import { expectOneNotInternalServerErrorSnapshot } from 'test/integration/graphql/utils/expect-one-not-internal-server-error-snapshot.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 { uploadApplicationFile } from 'test/integration/metadata/suites/application/utils/upload-application-file.util'; +import { + type EachTestingContext, + eachTestingContextFilter, +} from 'twenty-shared/testing'; +import { v4 as uuidv4 } from 'uuid'; + +const TEST_APP_ID = uuidv4(); +const UNKNOWN_APP_ID = uuidv4(); + +type TestContext = { + applicationUniversalIdentifier: string; + fileFolder: string; + filePath: string; +}; + +const FAILING_TEST_CASES: EachTestingContext[] = [ + { + title: 'when filePath contains relative path traversal (../)', + context: { + applicationUniversalIdentifier: TEST_APP_ID, + fileFolder: 'BuiltFrontComponent', + filePath: + '../../../other-workspace/other-app/BuiltFrontComponent/stolen.mjs', + }, + }, + { + title: 'when filePath contains upward traversal (../../)', + context: { + applicationUniversalIdentifier: TEST_APP_ID, + fileFolder: 'BuiltFrontComponent', + filePath: '../../etc/passwd', + }, + }, + { + title: 'when filePath is an absolute path', + context: { + applicationUniversalIdentifier: TEST_APP_ID, + fileFolder: 'BuiltFrontComponent', + filePath: '/etc/passwd', + }, + }, + { + title: 'when filePath contains backslash path traversal', + context: { + applicationUniversalIdentifier: TEST_APP_ID, + fileFolder: 'BuiltFrontComponent', + filePath: '..\\..\\..\\etc\\passwd', + }, + }, + { + title: 'when filePath is empty', + context: { + applicationUniversalIdentifier: TEST_APP_ID, + fileFolder: 'BuiltFrontComponent', + filePath: '', + }, + }, + { + title: + 'when applicationUniversalIdentifier does not match any installed application', + context: { + applicationUniversalIdentifier: UNKNOWN_APP_ID, + fileFolder: 'BuiltFrontComponent', + filePath: 'src/components/legit.mjs', + }, + }, + { + title: 'when applicationUniversalIdentifier is empty', + context: { + applicationUniversalIdentifier: '', + fileFolder: 'BuiltFrontComponent', + filePath: 'src/components/legit.mjs', + }, + }, + { + title: 'when fileFolder is not an allowed application file folder', + context: { + applicationUniversalIdentifier: TEST_APP_ID, + fileFolder: 'CorePicture', + filePath: 'src/components/legit.mjs', + }, + }, +]; + +describe('Upload application file should fail', () => { + beforeAll(async () => { + await setupApplicationForSync({ + applicationUniversalIdentifier: TEST_APP_ID, + name: 'Test Upload Path Traversal App', + description: 'App for testing path traversal on file upload', + sourcePath: 'test-upload-path-traversal', + }); + }, 60000); + + afterAll(async () => { + await cleanupApplicationAndAppRegistration({ + applicationUniversalIdentifier: TEST_APP_ID, + }); + }); + + it.each(eachTestingContextFilter(FAILING_TEST_CASES))( + '$title', + async ({ context }) => { + jest.useRealTimers(); + + const { errors } = await uploadApplicationFile({ + applicationUniversalIdentifier: context.applicationUniversalIdentifier, + fileFolder: context.fileFolder, + filePath: context.filePath, + fileBuffer: Buffer.from('content'), + filename: 'test-file.mjs', + contentType: 'application/javascript', + expectToFail: true, + }); + + jest.useFakeTimers(); + + expectOneNotInternalServerErrorSnapshot({ errors }); + }, + 60000, + ); +}); diff --git a/packages/twenty-server/test/integration/metadata/suites/application/successful-upload-application-file.integration-spec.ts b/packages/twenty-server/test/integration/metadata/suites/application/successful-upload-application-file.integration-spec.ts new file mode 100644 index 0000000000..0bf650239c --- /dev/null +++ b/packages/twenty-server/test/integration/metadata/suites/application/successful-upload-application-file.integration-spec.ts @@ -0,0 +1,132 @@ +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 { uploadApplicationFile } from 'test/integration/metadata/suites/application/utils/upload-application-file.util'; +import { + type EachTestingContext, + eachTestingContextFilter, +} from 'twenty-shared/testing'; +import { FileFolder } from 'twenty-shared/types'; +import { v4 as uuidv4 } from 'uuid'; + +const TEST_APP_ID = uuidv4(); + +type TestContext = { + fileFolder: string; + fileFolderValue: FileFolder; + filePath: string; + filename: string; + contentType: string; + fileContent: string; +}; + +const SUCCESSFUL_TEST_CASES: EachTestingContext[] = [ + { + title: 'when uploading a built front component', + context: { + fileFolder: 'BuiltFrontComponent', + fileFolderValue: FileFolder.BuiltFrontComponent, + filePath: 'src/components/my-component.mjs', + filename: 'my-component.mjs', + contentType: 'application/javascript', + fileContent: 'export default function MyComponent() {}', + }, + }, + { + title: 'when uploading a built logic function', + context: { + fileFolder: 'BuiltLogicFunction', + fileFolderValue: FileFolder.BuiltLogicFunction, + filePath: 'src/handlers/my-handler.mjs', + filename: 'my-handler.mjs', + contentType: 'application/javascript', + fileContent: 'export default async function handler() {}', + }, + }, + { + title: 'when uploading a source file', + context: { + fileFolder: 'Source', + fileFolderValue: FileFolder.Source, + filePath: 'src/index.tsx', + filename: 'index.tsx', + contentType: 'text/plain', + fileContent: 'export const App = () =>
Hello
;', + }, + }, + { + title: 'when uploading a public asset', + context: { + fileFolder: 'PublicAsset', + fileFolderValue: FileFolder.PublicAsset, + filePath: 'assets/logo.svg', + filename: 'logo.svg', + contentType: 'image/svg+xml', + fileContent: '', + }, + }, + { + title: 'when uploading a dependencies file', + context: { + fileFolder: 'Dependencies', + fileFolderValue: FileFolder.Dependencies, + filePath: 'yarn.lock', + filename: 'yarn.lock', + contentType: 'text/plain', + fileContent: '# yarn lockfile v1', + }, + }, + { + title: 'when uploading a file in a nested directory path', + context: { + fileFolder: 'Source', + fileFolderValue: FileFolder.Source, + filePath: 'src/modules/auth/login/login.component.tsx', + filename: 'login.component.tsx', + contentType: 'text/plain', + fileContent: 'export const Login = () => null;', + }, + }, +]; + +describe('Upload application file should succeed', () => { + beforeAll(async () => { + await setupApplicationForSync({ + applicationUniversalIdentifier: TEST_APP_ID, + name: 'Test Successful Upload App', + description: 'App for testing successful file uploads', + sourcePath: 'test-successful-upload', + }); + }, 60000); + + afterAll(async () => { + await cleanupApplicationAndAppRegistration({ + applicationUniversalIdentifier: TEST_APP_ID, + }); + }); + + it.each(eachTestingContextFilter(SUCCESSFUL_TEST_CASES))( + '$title', + async ({ context }) => { + jest.useRealTimers(); + + const { data, errors } = await uploadApplicationFile({ + applicationUniversalIdentifier: TEST_APP_ID, + fileFolder: context.fileFolder, + filePath: context.filePath, + fileBuffer: Buffer.from(context.fileContent), + filename: context.filename, + contentType: context.contentType, + expectToFail: false, + }); + + jest.useFakeTimers(); + + expect(errors).toBeUndefined(); + expect(data.uploadApplicationFile).toEqual({ + id: expect.any(String), + path: `${context.fileFolderValue}/${context.filePath}`, + }); + }, + 60000, + ); +});