mirror of
https://github.com/twentyhq/twenty
synced 2026-04-21 13:37:22 +00:00
fix google signup edge case (#18365)
Fixes an edge case when a user signs up with Google and the profile avatar network request times out, we crash instead of creating the user without an avatar. Added `axios-retry` to retry max 2 times and if it still fails we gracefully skip avatar image instead of crashing Fixes Sentry TWENTY-SERVER-FDQ Sonarly https://sonarly.com/issue/6564 --------- Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Co-authored-by: Charles Bochet <charles@twenty.com> Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
This commit is contained in:
parent
911a46aa45
commit
4c001778c2
9 changed files with 211 additions and 65 deletions
|
|
@ -85,6 +85,7 @@
|
|||
"apollo-server-core": "3.13.0",
|
||||
"archiver": "7.0.1",
|
||||
"axios": "^1.13.5",
|
||||
"axios-retry": "^4.5.0",
|
||||
"babel-plugin-module-resolver": "5.0.2",
|
||||
"bcrypt": "5.1.1",
|
||||
"bullmq": "5.40.0",
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { Injectable } from '@nestjs/common';
|
||||
import { Injectable, Logger } from '@nestjs/common';
|
||||
import { InjectRepository } from '@nestjs/typeorm';
|
||||
|
||||
import { buffer as streamToBuffer } from 'node:stream/consumers';
|
||||
|
|
@ -24,6 +24,8 @@ import { getImageBufferFromUrl } from 'src/utils/image';
|
|||
|
||||
@Injectable()
|
||||
export class FileCorePictureService {
|
||||
private readonly logger = new Logger(FileCorePictureService.name);
|
||||
|
||||
constructor(
|
||||
private readonly fileStorageService: FileStorageService,
|
||||
private readonly applicationService: ApplicationService,
|
||||
|
|
@ -186,7 +188,11 @@ export class FileCorePictureService {
|
|||
imageUrl: string,
|
||||
): Promise<{ buffer: Buffer; extension: string } | undefined> {
|
||||
try {
|
||||
const httpClient = this.secureHttpClientService.getHttpClient();
|
||||
const httpClient = this.secureHttpClientService.getHttpClient({
|
||||
retries: 2,
|
||||
shouldResetTimeout: true,
|
||||
});
|
||||
|
||||
const buffer = await getImageBufferFromUrl(imageUrl, httpClient);
|
||||
|
||||
const type = await FileType.fromBuffer(buffer);
|
||||
|
|
@ -196,7 +202,11 @@ export class FileCorePictureService {
|
|||
}
|
||||
|
||||
return { buffer, extension: type.ext };
|
||||
} catch {
|
||||
} catch (error) {
|
||||
this.logger.warn(
|
||||
`Failed to fetch image from URL: ${imageUrl} — ${error instanceof Error ? error.message : String(error)}`,
|
||||
);
|
||||
|
||||
return undefined;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { Injectable } from '@nestjs/common';
|
||||
import { Injectable, Logger } from '@nestjs/common';
|
||||
|
||||
import FileType from 'file-type';
|
||||
import sharp from 'sharp';
|
||||
|
|
@ -23,6 +23,8 @@ export type SignedFilesResult = {
|
|||
|
||||
@Injectable()
|
||||
export class FileUploadService {
|
||||
private readonly logger = new Logger(FileUploadService.name);
|
||||
|
||||
constructor(
|
||||
private readonly fileStorage: FileStorageService,
|
||||
private readonly fileService: FileService,
|
||||
|
|
@ -95,33 +97,54 @@ export class FileUploadService {
|
|||
fileFolder: FileFolder;
|
||||
workspaceId: string;
|
||||
}) {
|
||||
const httpClient = this.secureHttpClientService.getHttpClient();
|
||||
const imageData = await this.fetchImageBufferFromUrl(imageUrl).catch(
|
||||
(error) => {
|
||||
this.logger.warn(
|
||||
`Failed to fetch image from URL: ${imageUrl} — ${error instanceof Error ? error.message : String(error)}`,
|
||||
);
|
||||
|
||||
const buffer = await getImageBufferFromUrl(imageUrl, httpClient);
|
||||
return null;
|
||||
},
|
||||
);
|
||||
|
||||
const type = await FileType.fromBuffer(buffer);
|
||||
|
||||
if (!type || !type.ext || !type.mime) {
|
||||
throw new Error(
|
||||
'Unable to detect image type from buffer. The file may not be a valid image format.',
|
||||
);
|
||||
}
|
||||
|
||||
if (!type.mime.startsWith('image/')) {
|
||||
throw new Error(
|
||||
`Detected file type is not an image: ${type.mime}. Please provide a valid image URL.`,
|
||||
);
|
||||
if (!imageData) {
|
||||
return { name: '', mimeType: undefined, files: [] };
|
||||
}
|
||||
|
||||
return await this.uploadImage({
|
||||
file: buffer,
|
||||
filename: `${v4()}.${type.ext}`,
|
||||
mimeType: type.mime,
|
||||
file: imageData.buffer,
|
||||
filename: `${v4()}.${imageData.extension}`,
|
||||
mimeType: imageData.mimeType,
|
||||
fileFolder,
|
||||
workspaceId,
|
||||
});
|
||||
}
|
||||
|
||||
private async fetchImageBufferFromUrl(imageUrl: string): Promise<{
|
||||
buffer: Buffer;
|
||||
extension: string;
|
||||
mimeType: string;
|
||||
} | null> {
|
||||
const httpClient = this.secureHttpClientService.getHttpClient({
|
||||
retries: 2,
|
||||
shouldResetTimeout: true,
|
||||
});
|
||||
|
||||
const buffer = await getImageBufferFromUrl(imageUrl, httpClient);
|
||||
|
||||
if (!buffer || buffer.length === 0) {
|
||||
return null;
|
||||
}
|
||||
|
||||
const type = await FileType.fromBuffer(buffer);
|
||||
|
||||
if (!type || !type.ext || !type.mime || !type.mime.startsWith('image/')) {
|
||||
throw new Error(`Invalid image type for URL: ${imageUrl}`);
|
||||
}
|
||||
|
||||
return { buffer, extension: type.ext, mimeType: type.mime };
|
||||
}
|
||||
|
||||
async uploadImage({
|
||||
file,
|
||||
filename,
|
||||
|
|
|
|||
|
|
@ -1,9 +1,16 @@
|
|||
import * as http from 'http';
|
||||
import * as https from 'https';
|
||||
|
||||
import axiosRetry from 'axios-retry';
|
||||
|
||||
import { SecureHttpClientService } from 'src/engine/core-modules/secure-http-client/secure-http-client.service';
|
||||
import { type TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service';
|
||||
|
||||
jest.mock('axios-retry', () => ({
|
||||
__esModule: true,
|
||||
default: jest.fn(),
|
||||
}));
|
||||
|
||||
const createMockConfigService = (
|
||||
overrides: Record<string, unknown> = {},
|
||||
): TwentyConfigService => {
|
||||
|
|
@ -81,6 +88,43 @@ describe('SecureHttpClientService', () => {
|
|||
|
||||
expect(client.defaults.baseURL).toBe('https://example.com/api');
|
||||
});
|
||||
|
||||
it('should configure axios-retry when retries is greater than 0', () => {
|
||||
jest.mocked(axiosRetry).mockClear();
|
||||
const service = new SecureHttpClientService(createMockConfigService());
|
||||
const client = service.getHttpClient({
|
||||
retries: 2,
|
||||
shouldResetTimeout: true,
|
||||
});
|
||||
|
||||
expect(axiosRetry).toHaveBeenCalledWith(client, {
|
||||
retries: 2,
|
||||
shouldResetTimeout: true,
|
||||
retryCondition: expect.any(Function),
|
||||
});
|
||||
});
|
||||
|
||||
it('should not configure axios-retry when retries is 0', () => {
|
||||
jest.mocked(axiosRetry).mockClear();
|
||||
const service = new SecureHttpClientService(createMockConfigService());
|
||||
|
||||
service.getHttpClient({ retries: 0 });
|
||||
|
||||
expect(axiosRetry).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should not leak retry config into axios defaults', () => {
|
||||
const service = new SecureHttpClientService(createMockConfigService());
|
||||
const client = service.getHttpClient({
|
||||
retries: 2,
|
||||
shouldResetTimeout: true,
|
||||
baseURL: 'https://example.com',
|
||||
});
|
||||
|
||||
expect(client.defaults.baseURL).toBe('https://example.com');
|
||||
expect(client.defaults).not.toHaveProperty('retries');
|
||||
expect(client.defaults).not.toHaveProperty('shouldResetTimeout');
|
||||
});
|
||||
});
|
||||
|
||||
describe('getInternalHttpClient', () => {
|
||||
|
|
|
|||
|
|
@ -1,6 +1,8 @@
|
|||
import { Injectable, Logger } from '@nestjs/common';
|
||||
|
||||
import axios, { type AxiosInstance, type CreateAxiosDefaults } from 'axios';
|
||||
import axiosRetry from 'axios-retry';
|
||||
import { isDefined } from 'twenty-shared/utils';
|
||||
|
||||
import { createSsrfSafeAgent } from 'src/engine/core-modules/secure-http-client/utils/create-ssrf-safe-agent.util';
|
||||
import { resolveAndValidateHostname } from 'src/engine/core-modules/secure-http-client/utils/resolve-and-validate-hostname.util';
|
||||
|
|
@ -10,6 +12,11 @@ import { type OutboundRequestContext } from './outbound-request-context.type';
|
|||
|
||||
const MAX_REDIRECTS = 5;
|
||||
|
||||
type SecureHttpClientConfig = CreateAxiosDefaults & {
|
||||
retries?: number;
|
||||
shouldResetTimeout?: boolean;
|
||||
};
|
||||
|
||||
@Injectable()
|
||||
export class SecureHttpClientService {
|
||||
private readonly logger = new Logger(SecureHttpClientService.name);
|
||||
|
|
@ -22,24 +29,37 @@ export class SecureHttpClientService {
|
|||
// When context is provided, outbound requests are logged with
|
||||
// workspace/user info for GuardDuty correlation.
|
||||
getHttpClient(
|
||||
config?: CreateAxiosDefaults,
|
||||
config?: SecureHttpClientConfig,
|
||||
context?: OutboundRequestContext,
|
||||
): AxiosInstance {
|
||||
const { retries, shouldResetTimeout, ...axiosConfig } = config ?? {};
|
||||
|
||||
const isSafeModeEnabled = this.twentyConfigService.get(
|
||||
'OUTBOUND_HTTP_SAFE_MODE_ENABLED',
|
||||
);
|
||||
|
||||
const client = isSafeModeEnabled
|
||||
? axios.create({
|
||||
...config,
|
||||
...axiosConfig,
|
||||
httpAgent: createSsrfSafeAgent('http'),
|
||||
httpsAgent: createSsrfSafeAgent('https'),
|
||||
maxRedirects: Math.min(
|
||||
config?.maxRedirects ?? MAX_REDIRECTS,
|
||||
axiosConfig.maxRedirects ?? MAX_REDIRECTS,
|
||||
MAX_REDIRECTS,
|
||||
),
|
||||
})
|
||||
: axios.create(config);
|
||||
: axios.create(axiosConfig);
|
||||
|
||||
if (isDefined(retries) && retries > 0) {
|
||||
axiosRetry(client, {
|
||||
retries,
|
||||
shouldResetTimeout,
|
||||
retryCondition: (error) =>
|
||||
axiosRetry.isNetworkOrIdempotentRequestError(error) &&
|
||||
error.code !== 'ECONNABORTED' &&
|
||||
error.code !== 'ETIMEDOUT',
|
||||
});
|
||||
}
|
||||
|
||||
if (context) {
|
||||
client.interceptors.request.use((requestConfig) => {
|
||||
|
|
|
|||
|
|
@ -26,8 +26,8 @@ import { type WorkspaceEntity } from 'src/engine/core-modules/workspace/workspac
|
|||
import { DataSourceService } from 'src/engine/metadata-modules/data-source/data-source.service';
|
||||
import { ObjectMetadataEntity } from 'src/engine/metadata-modules/object-metadata/object-metadata.entity';
|
||||
import { PermissionsException } from 'src/engine/metadata-modules/permissions/permissions.exception';
|
||||
import { RoleValidationService } from 'src/engine/metadata-modules/role-validation/services/role-validation.service';
|
||||
import { RoleTargetEntity } from 'src/engine/metadata-modules/role-target/role-target.entity';
|
||||
import { RoleValidationService } from 'src/engine/metadata-modules/role-validation/services/role-validation.service';
|
||||
import { UserRoleService } from 'src/engine/metadata-modules/user-role/user-role.service';
|
||||
import { GlobalWorkspaceOrmManager } from 'src/engine/twenty-orm/global-workspace-datasource/global-workspace-orm.manager';
|
||||
import { type WorkspaceRepository } from 'src/engine/twenty-orm/repository/workspace.repository';
|
||||
|
|
@ -304,6 +304,50 @@ describe('UserWorkspaceService', () => {
|
|||
expect(userWorkspaceRepository.save).toHaveBeenCalledWith(userWorkspace);
|
||||
expect(result).toEqual(userWorkspace);
|
||||
});
|
||||
it('should create a user workspace without a default avatar url if image fetch fails', async () => {
|
||||
const userId = 'user-id';
|
||||
const workspaceId = 'workspace-id';
|
||||
const userWorkspace = {
|
||||
userId,
|
||||
workspaceId,
|
||||
} as UserWorkspaceEntity;
|
||||
|
||||
jest
|
||||
.spyOn(userWorkspaceRepository, 'create')
|
||||
.mockReturnValue(userWorkspace);
|
||||
jest
|
||||
.spyOn(userWorkspaceRepository, 'save')
|
||||
.mockResolvedValue(userWorkspace);
|
||||
|
||||
jest
|
||||
.spyOn(fileUploadService, 'uploadImageFromUrl')
|
||||
.mockRejectedValue(
|
||||
new Error(
|
||||
'Failed to fetch image from https://lh3.googleusercontent.com/a/invalid: Request failed with status code 404',
|
||||
),
|
||||
);
|
||||
|
||||
const result = await service.create({
|
||||
userId,
|
||||
workspaceId,
|
||||
isExistingUser: false,
|
||||
pictureUrl: 'https://lh3.googleusercontent.com/a/invalid',
|
||||
});
|
||||
|
||||
expect(fileUploadService.uploadImageFromUrl).toHaveBeenCalledTimes(1);
|
||||
expect(fileUploadService.uploadImageFromUrl).toHaveBeenCalledWith({
|
||||
imageUrl: 'https://lh3.googleusercontent.com/a/invalid',
|
||||
fileFolder: FileFolder.ProfilePicture,
|
||||
workspaceId,
|
||||
});
|
||||
expect(userWorkspaceRepository.create).toHaveBeenCalledWith({
|
||||
userId,
|
||||
workspaceId,
|
||||
defaultAvatarUrl: undefined,
|
||||
});
|
||||
expect(userWorkspaceRepository.save).toHaveBeenCalledWith(userWorkspace);
|
||||
expect(result).toEqual(userWorkspace);
|
||||
});
|
||||
it("should create a user workspace without a default avatar url if it's a new user without a picture url", async () => {
|
||||
const userId = 'user-id';
|
||||
const workspaceId = 'workspace-id';
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
import { Logger } from '@nestjs/common';
|
||||
import { InjectRepository } from '@nestjs/typeorm';
|
||||
|
||||
import { TypeOrmQueryService } from '@ptc-org/nestjs-query-typeorm';
|
||||
|
|
@ -44,6 +45,8 @@ import { assert } from 'src/utils/assert';
|
|||
import { getDomainNameByEmail } from 'src/utils/get-domain-name-by-email';
|
||||
|
||||
export class UserWorkspaceService extends TypeOrmQueryService<UserWorkspaceEntity> {
|
||||
private readonly logger = new Logger(UserWorkspaceService.name);
|
||||
|
||||
constructor(
|
||||
@InjectRepository(UserWorkspaceEntity)
|
||||
private readonly userWorkspaceRepository: Repository<UserWorkspaceEntity>,
|
||||
|
|
@ -482,17 +485,25 @@ export class UserWorkspaceService extends TypeOrmQueryService<UserWorkspaceEntit
|
|||
|
||||
if (!isDefined(pictureUrl) || pictureUrl === '') return;
|
||||
|
||||
const { files } = await this.fileUploadService.uploadImageFromUrl({
|
||||
imageUrl: pictureUrl,
|
||||
fileFolder: FileFolder.ProfilePicture,
|
||||
workspaceId,
|
||||
});
|
||||
try {
|
||||
const { files } = await this.fileUploadService.uploadImageFromUrl({
|
||||
imageUrl: pictureUrl,
|
||||
fileFolder: FileFolder.ProfilePicture,
|
||||
workspaceId,
|
||||
});
|
||||
|
||||
if (!files.length) {
|
||||
throw new Error('Failed to upload avatar');
|
||||
if (!files.length) {
|
||||
return;
|
||||
}
|
||||
|
||||
return files[0].path;
|
||||
} catch (error) {
|
||||
this.logger.warn(
|
||||
`Failed to upload profile picture from URL: ${pictureUrl} — ${error instanceof Error ? error.message : String(error)}`,
|
||||
);
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
return files[0].path;
|
||||
}
|
||||
|
||||
private async computeDefaultAvatarUrlMigrated(
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { type AxiosError, type AxiosInstance } from 'axios';
|
||||
import { type AxiosInstance } from 'axios';
|
||||
|
||||
const cropRegex = /([w|h])([0-9]+)/;
|
||||
|
||||
|
|
@ -35,7 +35,7 @@ export const getImageBufferFromUrl = async (
|
|||
responseType: 'arraybuffer',
|
||||
validateStatus: (status) => status >= 200 && status < 300,
|
||||
maxRedirects: 5,
|
||||
timeout: 30000,
|
||||
timeout: 10000,
|
||||
});
|
||||
|
||||
if (!response.data) {
|
||||
|
|
@ -60,34 +60,8 @@ export const getImageBufferFromUrl = async (
|
|||
|
||||
return Buffer.from(response.data, 'binary');
|
||||
} catch (error) {
|
||||
const axiosError = error as AxiosError;
|
||||
const axiosResponse = axiosError.response;
|
||||
const message = error instanceof Error ? error.message : 'Unknown error';
|
||||
|
||||
if (axiosResponse) {
|
||||
throw new Error(
|
||||
`Failed to fetch image: HTTP ${axiosResponse.status} from ${url}`,
|
||||
);
|
||||
}
|
||||
|
||||
if (error instanceof Error) {
|
||||
if (
|
||||
axiosError.code === 'ECONNABORTED' ||
|
||||
error.message.includes('timeout')
|
||||
) {
|
||||
throw new Error(
|
||||
`Request timeout while fetching image from URL: ${url}`,
|
||||
);
|
||||
}
|
||||
if (
|
||||
axiosError.code === 'ENOTFOUND' ||
|
||||
axiosError.code === 'ECONNREFUSED' ||
|
||||
error.message.includes('ENOTFOUND') ||
|
||||
error.message.includes('ECONNREFUSED')
|
||||
) {
|
||||
throw new Error(`Failed to connect to image URL: ${url}`);
|
||||
}
|
||||
throw new Error(`Failed to fetch image from URL: ${error.message}`);
|
||||
}
|
||||
throw new Error(`Failed to fetch image from URL: ${url}`);
|
||||
throw new Error(`Failed to fetch image from ${url}: ${message}`);
|
||||
}
|
||||
};
|
||||
|
|
|
|||
19
yarn.lock
19
yarn.lock
|
|
@ -29122,6 +29122,17 @@ __metadata:
|
|||
languageName: node
|
||||
linkType: hard
|
||||
|
||||
"axios-retry@npm:^4.5.0":
|
||||
version: 4.5.0
|
||||
resolution: "axios-retry@npm:4.5.0"
|
||||
dependencies:
|
||||
is-retry-allowed: "npm:^2.2.0"
|
||||
peerDependencies:
|
||||
axios: 0.x || 1.x
|
||||
checksum: 10c0/574e7b1bf24aad99b560042d232a932d51bfaa29b5a6d4612d748ed799a6f11a5afb2582792492c55d95842200cbdfbe3454027a8c1b9a2d3e895d13c3d03c10
|
||||
languageName: node
|
||||
linkType: hard
|
||||
|
||||
"axios@npm:^1.12.0, axios@npm:^1.13.5, axios@npm:^1.6.1, axios@npm:^1.7.7, axios@npm:^1.8.3":
|
||||
version: 1.13.5
|
||||
resolution: "axios@npm:1.13.5"
|
||||
|
|
@ -41270,6 +41281,13 @@ __metadata:
|
|||
languageName: node
|
||||
linkType: hard
|
||||
|
||||
"is-retry-allowed@npm:^2.2.0":
|
||||
version: 2.2.0
|
||||
resolution: "is-retry-allowed@npm:2.2.0"
|
||||
checksum: 10c0/013be4f8a0a06a49ed1fe495242952e898325d496202a018f6f9fb3fb9ac8fe3b957a9bd62463d68299ae35dbbda680473c85a9bcefca731b49d500d3ccc08ff
|
||||
languageName: node
|
||||
linkType: hard
|
||||
|
||||
"is-scoped@npm:^2.1.0":
|
||||
version: 2.1.0
|
||||
resolution: "is-scoped@npm:2.1.0"
|
||||
|
|
@ -57907,6 +57925,7 @@ __metadata:
|
|||
apollo-server-core: "npm:3.13.0"
|
||||
archiver: "npm:7.0.1"
|
||||
axios: "npm:^1.13.5"
|
||||
axios-retry: "npm:^4.5.0"
|
||||
babel-plugin-module-resolver: "npm:5.0.2"
|
||||
bcrypt: "npm:5.1.1"
|
||||
bullmq: "npm:5.40.0"
|
||||
|
|
|
|||
Loading…
Reference in a new issue