File service uniformize not found behavior and stream management (#20891)

# Introduction

closes https://github.com/twentyhq/private-issues/issues/485
This commit is contained in:
Paul Rastoin
2026-05-26 11:23:48 +02:00
committed by GitHub
parent 82c565f7dc
commit 076c05cbd0
11 changed files with 514 additions and 94 deletions
@@ -10,11 +10,6 @@ jest.mock('node:stream/promises', () => ({
pipeline: jest.fn(),
}));
import {
FileStorageException,
FileStorageExceptionCode,
} from 'src/engine/core-modules/file-storage/interfaces/file-storage-exception';
import {
FileException,
FileExceptionCode,
@@ -32,7 +27,6 @@ const createMockStream = (): Readable => {
stream.push('file content');
stream.push(null);
stream.pipe = jest.fn();
return stream;
};
@@ -153,7 +147,7 @@ describe('FileController', () => {
'Content-Disposition',
'inline',
);
expect(mockStream.pipe).toHaveBeenCalledWith(mockResponse);
expect(mockPipeline).toHaveBeenCalledWith(mockStream, mockResponse);
});
it('should force attachment disposition for non-safe MIME types', async () => {
@@ -185,15 +179,8 @@ describe('FileController', () => {
);
});
it('should throw FileException with FILE_NOT_FOUND when file is not found', async () => {
jest
.spyOn(fileService, 'getFileResponseById')
.mockRejectedValue(
new FileStorageException(
'File not found',
FileStorageExceptionCode.FILE_NOT_FOUND,
),
);
it('should throw FILE_NOT_FOUND when the service yields null', async () => {
jest.spyOn(fileService, 'getFileResponseById').mockResolvedValue(null);
const mockRequest = { workspaceId: 'workspace-id' } as any;
const mockResponse = createMockResponse() as any;
@@ -210,27 +197,97 @@ describe('FileController', () => {
);
});
it('should throw FileException with INTERNAL_SERVER_ERROR for unexpected errors', async () => {
it('should throw INTERNAL_SERVER_ERROR without leaking the underlying message, and log the original error', async () => {
const loggerSpy = jest
.spyOn(Logger.prototype, 'error')
.mockImplementation(() => undefined);
const underlyingError = new Error(
'Storage unavailable: postgres://secret-host:5432',
);
jest
.spyOn(fileService, 'getFileResponseById')
.mockRejectedValue(new Error('Storage unavailable'));
.mockRejectedValue(underlyingError);
const mockRequest = { workspaceId: 'workspace-id' } as any;
const mockResponse = createMockResponse() as any;
const promise = controller.getFileById(
mockResponse,
mockRequest,
FileFolder.Workflow,
'file-456',
);
await expect(promise).rejects.toThrow(
new FileException(
'Error retrieving file',
FileExceptionCode.INTERNAL_SERVER_ERROR,
),
);
await expect(promise).rejects.not.toThrow(/secret-host/);
expect(loggerSpy).toHaveBeenCalledWith(
'getFileResponseById failed unexpectedly',
{ error: underlyingError },
);
});
it('should throw INTERNAL_SERVER_ERROR when the stream errors before headers are sent', async () => {
const mockStream = createMockStream();
jest.spyOn(fileService, 'getFileResponseById').mockResolvedValue({
type: 'stream',
stream: mockStream,
mimeType: 'image/png',
});
mockPipeline.mockRejectedValue(new Error('source backend exploded'));
const mockRequest = { workspaceId: 'workspace-id' } as any;
const mockResponse = createMockResponse({ headersSent: false }) as any;
await expect(
controller.getFileById(
mockResponse,
mockRequest,
FileFolder.Workflow,
'file-456',
FileFolder.CorePicture,
'file-123',
),
).rejects.toThrow(
new FileException(
'Error retrieving file: Storage unavailable',
'Error streaming file from storage',
FileExceptionCode.INTERNAL_SERVER_ERROR,
),
);
expect(mockResponse.destroy).not.toHaveBeenCalled();
});
it('should destroy the response without throwing when the stream errors after headers are sent', async () => {
const mockStream = createMockStream();
jest.spyOn(fileService, 'getFileResponseById').mockResolvedValue({
type: 'stream',
stream: mockStream,
mimeType: 'image/png',
});
mockPipeline.mockRejectedValue(new Error('socket reset mid-flight'));
const mockRequest = { workspaceId: 'workspace-id' } as any;
const mockResponse = createMockResponse({ headersSent: true }) as any;
// No throw expected — once headers are out, the controller cannot honestly
// switch to a 500 response, so it tears the socket down instead.
await controller.getFileById(
mockResponse,
mockRequest,
FileFolder.CorePicture,
'file-123',
);
expect(mockResponse.destroy).toHaveBeenCalledTimes(1);
});
});
@@ -15,10 +15,6 @@ import { join } from 'path';
import { Request, Response } from 'express';
import { FileFolder } from 'twenty-shared/types';
import {
FileStorageException,
FileStorageExceptionCode,
} from 'src/engine/core-modules/file-storage/interfaces/file-storage-exception';
import { validateFilePath } from 'src/engine/core-modules/file-storage/utils/validate-file-path.util';
import {
FileException,
@@ -120,45 +116,49 @@ export class FileController {
// oxlint-disable-next-line @typescripttypescript/no-explicit-any
const workspaceId = (req as any)?.workspaceId;
try {
const fileResponse = await this.fileService.getFileResponseById({
const fileResponse = await this.fileService
.getFileResponseById({
fileId,
workspaceId,
fileFolder,
});
})
.catch((error) => {
this.logger.error('getFileResponseById failed unexpectedly', {
error,
});
if (fileResponse.type === 'redirect') {
return res.redirect(fileResponse.presignedUrl);
}
setFileResponseHeaders(res, fileResponse.mimeType);
fileResponse.stream.on('error', () => {
if (!res.headersSent) {
res.status(500).send('Error streaming file from storage');
return;
}
res.destroy();
});
fileResponse.stream.pipe(res);
} catch (error) {
if (
error instanceof FileStorageException &&
error.code === FileStorageExceptionCode.FILE_NOT_FOUND
) {
throw new FileException(
'File not found',
FileExceptionCode.FILE_NOT_FOUND,
'Error retrieving file',
FileExceptionCode.INTERNAL_SERVER_ERROR,
);
});
if (fileResponse === null) {
throw new FileException(
'File not found',
FileExceptionCode.FILE_NOT_FOUND,
);
}
if (fileResponse.type === 'redirect') {
return res.redirect(fileResponse.presignedUrl);
}
setFileResponseHeaders(res, fileResponse.mimeType);
try {
await pipeline(fileResponse.stream, res);
} catch (error) {
this.logger.error('File-by-id stream failed mid-transfer', { error });
if (!res.headersSent) {
throw new FileException(
'Error streaming file from storage',
FileExceptionCode.INTERNAL_SERVER_ERROR,
);
}
throw new FileException(
`Error retrieving file: ${error.message}`,
FileExceptionCode.INTERNAL_SERVER_ERROR,
);
res.destroy();
}
}
}
@@ -1,4 +1,4 @@
import { Injectable } from '@nestjs/common';
import { Injectable, Logger } from '@nestjs/common';
import { InjectRepository } from '@nestjs/typeorm';
import { type Readable } from 'stream';
@@ -22,6 +22,8 @@ import { streamToBuffer } from 'src/utils/stream-to-buffer';
@Injectable()
export class FileService {
private readonly logger = new Logger(FileService.name);
constructor(
private readonly jwtWrapperService: JwtWrapperService,
private readonly fileStorageService: FileStorageService,
@@ -98,8 +100,8 @@ export class FileService {
fileId: string;
workspaceId: string;
fileFolder: FileFolder;
}): Promise<{ stream: Readable; mimeType: string }> {
const file = await this.fileRepository.findOneOrFail({
}): Promise<{ stream: Readable; mimeType: string } | null> {
const file = await this.fileRepository.findOne({
where: {
id: fileId,
workspaceId,
@@ -107,32 +109,55 @@ export class FileService {
},
});
const application = await this.applicationRepository.findOneOrFail({
if (file === null) {
return null;
}
const application = await this.applicationRepository.findOne({
where: {
id: file.applicationId,
workspaceId,
},
});
const stream = await this.fileStorageService.readFile({
resourcePath: removeFileFolderFromFileEntityPath(file.path),
fileFolder,
applicationUniversalIdentifier: application.universalIdentifier,
workspaceId,
});
if (application === null) {
this.logger.warn(
`File ${file.id} references missing application ${file.applicationId} in workspace ${workspaceId}`,
);
return {
stream,
mimeType: file.mimeType,
};
return null;
}
try {
const stream = await this.fileStorageService.readFile({
resourcePath: removeFileFolderFromFileEntityPath(file.path),
fileFolder,
applicationUniversalIdentifier: application.universalIdentifier,
workspaceId,
});
return {
stream,
mimeType: file.mimeType,
};
} catch (error) {
if (
error instanceof FileStorageException &&
error.code === FileStorageExceptionCode.FILE_NOT_FOUND
) {
return null;
}
throw error;
}
}
async getFileResponseById(params: {
fileId: string;
workspaceId: string;
fileFolder: FileFolder;
}): Promise<FileResponse> {
const file = await this.fileRepository.findOneOrFail({
}): Promise<FileResponse | null> {
const file = await this.fileRepository.findOne({
where: {
id: params.fileId,
workspaceId: params.workspaceId,
@@ -140,13 +165,25 @@ export class FileService {
},
});
const application = await this.applicationRepository.findOneOrFail({
if (file === null) {
return null;
}
const application = await this.applicationRepository.findOne({
where: {
id: file.applicationId,
workspaceId: params.workspaceId,
},
});
if (application === null) {
this.logger.warn(
`File ${file.id} references missing application ${file.applicationId} in workspace ${params.workspaceId}`,
);
return null;
}
const mimeType = file.mimeType ?? 'application/octet-stream';
const resourceIdentifier = {
resourcePath: removeFileFolderFromFileEntityPath(file.path),
@@ -168,9 +205,20 @@ export class FileService {
return { type: 'redirect', presignedUrl };
}
const stream = await this.fileStorageService.readFile(resourceIdentifier);
try {
const stream = await this.fileStorageService.readFile(resourceIdentifier);
return { type: 'stream', stream, mimeType };
return { type: 'stream', stream, mimeType };
} catch (error) {
if (
error instanceof FileStorageException &&
error.code === FileStorageExceptionCode.FILE_NOT_FOUND
) {
return null;
}
throw error;
}
}
async getFileContentById({
@@ -181,8 +229,8 @@ export class FileService {
fileId: string;
workspaceId: string;
fileFolder: FileFolder;
}): Promise<{ buffer: Buffer; mimeType: string }> {
const file = await this.fileRepository.findOneOrFail({
}): Promise<{ buffer: Buffer; mimeType: string } | null> {
const file = await this.fileRepository.findOne({
where: {
id: fileId,
workspaceId,
@@ -190,26 +238,49 @@ export class FileService {
},
});
const application = await this.applicationRepository.findOneOrFail({
if (file === null) {
return null;
}
const application = await this.applicationRepository.findOne({
where: {
id: file.applicationId,
workspaceId,
},
});
const stream = await this.fileStorageService.readFile({
resourcePath: removeFileFolderFromFileEntityPath(file.path),
fileFolder,
applicationUniversalIdentifier: application.universalIdentifier,
workspaceId,
});
if (application === null) {
this.logger.warn(
`File ${file.id} references missing application ${file.applicationId} in workspace ${workspaceId}`,
);
const buffer = await streamToBuffer(stream);
return null;
}
return {
buffer,
mimeType: file.mimeType ?? 'application/octet-stream',
};
try {
const stream = await this.fileStorageService.readFile({
resourcePath: removeFileFolderFromFileEntityPath(file.path),
fileFolder,
applicationUniversalIdentifier: application.universalIdentifier,
workspaceId,
});
const buffer = await streamToBuffer(stream);
return {
buffer,
mimeType: file.mimeType ?? 'application/octet-stream',
};
} catch (error) {
if (
error instanceof FileStorageException &&
error.code === FileStorageExceptionCode.FILE_NOT_FOUND
) {
return null;
}
throw error;
}
}
async deleteWorkspaceFolder(workspaceId: string) {
@@ -274,16 +274,23 @@ export class CodeInterpreterTool implements Tool {
continue;
}
const { buffer, mimeType } = await this.fileService.getFileContentById({
const fileContent = await this.fileService.getFileContentById({
fileId: file.fileId,
workspaceId,
fileFolder: FileFolder.AgentChat,
});
if (fileContent === null) {
this.logger.warn(
`File ${file.filename} no longer available (id=${file.fileId})`,
);
continue;
}
inputFiles.push({
filename: file.filename,
content: buffer,
mimeType,
content: fileContent.buffer,
mimeType: fileContent.mimeType,
});
} catch (error) {
this.logger.warn(`Failed to resolve file ${file.filename}`, error);
@@ -219,13 +219,20 @@ export class EmailComposerService {
for (const fileMetadata of files) {
const fileEntity = fileEntityMap.get(fileMetadata.id);
const { stream } = await this.fileService.getFileStreamById({
const fileStream = await this.fileService.getFileStreamById({
fileId: fileMetadata.id,
workspaceId,
fileFolder,
});
const buffer = await streamToBuffer(stream);
if (fileStream === null) {
throw new EmailToolException(
`Files not found: ${fileMetadata.name} (${fileMetadata.id})`,
EmailToolExceptionCode.FILE_NOT_FOUND,
);
}
const buffer = await streamToBuffer(fileStream.stream);
attachments.push({
filename: fileMetadata.name,
@@ -0,0 +1,10 @@
import gql from 'graphql-tag';
export const uploadWorkspaceLogoMutation = gql`
mutation UploadWorkspaceLogo($file: Upload!) {
uploadWorkspaceLogo(file: $file) {
id
url
}
}
`;
@@ -0,0 +1,15 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
exports[`File-by-id controller download should fail should respond 403 when the URL fileId does not match the token payload 1`] = `
{
"body": {},
"status": 403,
}
`;
exports[`File-by-id controller download should fail should respond 403 when the request has no token query parameter 1`] = `
{
"body": {},
"status": 403,
}
`;
@@ -0,0 +1,124 @@
import request from 'supertest';
import {
extractPathAndQueryFromUrl,
swapFileIdInUrl,
} from 'test/integration/metadata/suites/file/utils/file-by-id-url-helpers.util';
import { seedWorkspaceLogo } from 'test/integration/metadata/suites/file/utils/seed-workspace-logo.util';
import { expectOneNotInternalServerErrorHttpResponseSnapshot } from 'test/integration/utils/expect-one-not-internal-server-error-http-response-snapshot.util';
import { FileFolder } from 'twenty-shared/types';
import { v4 as uuidv4 } from 'uuid';
describe('File-by-id controller download should fail', () => {
let fileId: string;
let validUrlPath: string;
let cleanup: () => Promise<void>;
beforeAll(async () => {
jest.useRealTimers();
const seeded = await seedWorkspaceLogo();
fileId = seeded.fileId;
validUrlPath = extractPathAndQueryFromUrl(seeded.signedUrl);
cleanup = seeded.cleanup;
jest.useFakeTimers();
}, 60000);
afterAll(async () => {
await cleanup();
});
it('should respond 403 when the request has no token query parameter', async () => {
jest.useRealTimers();
const response = await request(global.app.getHttpServer()).get(
`/file/${FileFolder.CorePicture}/${fileId}`,
);
jest.useFakeTimers();
expect(response.text ?? '').not.toMatch(/postgres:|secret/i);
expectOneNotInternalServerErrorHttpResponseSnapshot({
status: response.status,
body: response.body,
});
}, 30000);
it('should respond 403 when the URL fileId does not match the token payload', async () => {
jest.useRealTimers();
const tamperedUrlPath = swapFileIdInUrl(validUrlPath, uuidv4());
const response = await request(global.app.getHttpServer()).get(
tamperedUrlPath,
);
jest.useFakeTimers();
expect(response.text ?? '').not.toMatch(/postgres:|secret/i);
expectOneNotInternalServerErrorHttpResponseSnapshot({
status: response.status,
body: response.body,
});
}, 30000);
it('should respond 404 when the file row no longer exists', async () => {
jest.useRealTimers();
await globalThis.testDataSource.query(
`UPDATE core."file" SET "deletedAt" = now() WHERE id = $1`,
[fileId],
);
try {
const response = await request(global.app.getHttpServer()).get(
validUrlPath,
);
expect(response.status).toBe(404);
expect(response.body.code).toBe('FILE_NOT_FOUND');
expect(response.text ?? '').not.toMatch(/postgres:|secret/i);
} finally {
await globalThis.testDataSource.query(
`UPDATE core."file" SET "deletedAt" = NULL WHERE id = $1`,
[fileId],
);
jest.useFakeTimers();
}
}, 30000);
it('should respond 404 when the storage object is missing while the DB row still exists', async () => {
jest.useRealTimers();
const [{ path: originalPath }] = (await globalThis.testDataSource.query(
`SELECT path FROM core."file" WHERE id = $1`,
[fileId],
)) as [{ path: string }];
await globalThis.testDataSource.query(
`UPDATE core."file" SET path = $1 WHERE id = $2`,
[`CorePicture/${uuidv4()}.png`, fileId],
);
try {
const response = await request(global.app.getHttpServer()).get(
validUrlPath,
);
expect(response.status).toBe(404);
expect(response.body.code).toBe('FILE_NOT_FOUND');
expect(response.text ?? '').not.toMatch(/postgres:|secret/i);
} finally {
await globalThis.testDataSource.query(
`UPDATE core."file" SET path = $1 WHERE id = $2`,
[originalPath, fileId],
);
jest.useFakeTimers();
}
}, 30000);
});
@@ -0,0 +1,51 @@
import request from 'supertest';
import { extractPathAndQueryFromUrl } from 'test/integration/metadata/suites/file/utils/file-by-id-url-helpers.util';
import {
ONE_BY_ONE_TRANSPARENT_PNG,
seedWorkspaceLogo,
} from 'test/integration/metadata/suites/file/utils/seed-workspace-logo.util';
describe('File-by-id controller download should succeed', () => {
let signedUrl: string;
let cleanup: () => Promise<void>;
beforeAll(async () => {
jest.useRealTimers();
const seeded = await seedWorkspaceLogo();
signedUrl = seeded.signedUrl;
cleanup = seeded.cleanup;
jest.useFakeTimers();
}, 60000);
afterAll(async () => {
await cleanup();
});
it('should stream the workspace logo with correct headers and a non-empty image body', async () => {
jest.useRealTimers();
const response = await request(global.app.getHttpServer())
.get(extractPathAndQueryFromUrl(signedUrl))
.buffer(true)
.parse((res, callback) => {
const chunks: Buffer[] = [];
res.on('data', (chunk) => chunks.push(chunk));
res.on('end', () => callback(null, Buffer.concat(chunks)));
});
jest.useFakeTimers();
expect(response.status).toBe(200);
expect(response.headers['content-type']).toContain('image/png');
expect(response.headers['x-content-type-options']).toBe('nosniff');
expect(response.headers['content-disposition']).toBe('inline');
const body = response.body as Buffer;
expect(body.equals(ONE_BY_ONE_TRANSPARENT_PNG)).toBe(true);
}, 30000);
});
@@ -0,0 +1,20 @@
// Strips host + port from an absolute signed file URL so `supertest` can hit
// the in-process Nest app via its bound port.
export const extractPathAndQueryFromUrl = (fullUrl: string): string => {
const parsed = new URL(fullUrl);
return parsed.pathname + parsed.search;
};
// `/file/<folder>/<fileId>?token=...` → swaps `<fileId>` for a different uuid;
// the embedded token (signed for the original fileId) stays untouched. Used to
// exercise the `payload.fileId !== URL.fileId` branch of `FileByIdGuard`.
export const swapFileIdInUrl = (
pathAndQuery: string,
newFileId: string,
): string => {
return pathAndQuery.replace(
/(\/file\/[^/]+\/)[^?]+(\?|$)/,
`$1${newFileId}$2`,
);
};
@@ -0,0 +1,58 @@
import { uploadWorkspaceLogoMutation } from 'test/integration/graphql/utils/upload-workspace-logo-mutation.util';
import { makeMetadataAPIRequestWithFileUpload } from 'test/integration/metadata/suites/utils/make-metadata-api-request-with-file-upload.util';
import { SEED_APPLE_WORKSPACE_ID } from 'src/engine/workspace-manager/dev-seeder/core/constants/seeder-workspaces.constant';
// 67-byte 1x1 transparent PNG. Big enough for `file-type` to detect a valid
// image header, small enough to keep the upload negligible. Exported so the
// success spec can assert round-trip byte-equality against the served body.
export const ONE_BY_ONE_TRANSPARENT_PNG = Buffer.from(
'89504E470D0A1A0A0000000D49484452000000010000000108060000001F15C4890000000D4944415478DA63000100000005000100200CB81000000000049454E44AE426082',
'hex',
);
type SeededWorkspaceLogo = {
fileId: string;
signedUrl: string;
workspaceId: string;
cleanup: () => Promise<void>;
};
export const seedWorkspaceLogo = async (): Promise<SeededWorkspaceLogo> => {
const response = await makeMetadataAPIRequestWithFileUpload(
{
query: uploadWorkspaceLogoMutation,
variables: { file: null },
},
{
field: 'file',
buffer: ONE_BY_ONE_TRANSPARENT_PNG,
filename: 'logo.png',
contentType: 'image/png',
},
);
if (response.body.errors !== undefined) {
throw new Error(
`uploadWorkspaceLogo failed: ${JSON.stringify(response.body.errors)}`,
);
}
const { id: fileId, url: signedUrl } = response.body.data
.uploadWorkspaceLogo as { id: string; url: string };
const workspaceId = SEED_APPLE_WORKSPACE_ID;
const cleanup = async (): Promise<void> => {
await globalThis.testDataSource.query(
`UPDATE core."workspace" SET "logoFileId" = NULL WHERE id = $1`,
[workspaceId],
);
await globalThis.testDataSource.query(
`DELETE FROM core."file" WHERE id = $1`,
[fileId],
);
};
return { fileId, signedUrl, workspaceId, cleanup };
};