From 6466e76c06723d181e984d2c185c67eafea68f8a Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Tue, 27 May 2025 15:50:44 +0200 Subject: [PATCH] fix(core): Don't allow creating more projects than allowed by exploiting a race condition (#15218) --- .../configs/__tests__/database.config.test.ts | 39 +++++++++++++ .../config/src/configs/database.config.ts | 9 +++ packages/@n8n/config/test/config.test.ts | 14 ++++- packages/@n8n/config/tsconfig.build.json | 2 +- .../cli/src/services/project.service.ee.ts | 58 ++++++++++++++----- .../cli/test/integration/project.api.test.ts | 29 ++++++++++ .../workflow-sharing.service.test.ts | 13 ++--- 7 files changed, 137 insertions(+), 27 deletions(-) create mode 100644 packages/@n8n/config/src/configs/__tests__/database.config.test.ts diff --git a/packages/@n8n/config/src/configs/__tests__/database.config.test.ts b/packages/@n8n/config/src/configs/__tests__/database.config.test.ts new file mode 100644 index 0000000000..c620fc49c2 --- /dev/null +++ b/packages/@n8n/config/src/configs/__tests__/database.config.test.ts @@ -0,0 +1,39 @@ +import { Container } from '@n8n/di'; + +import { DatabaseConfig } from '../database.config'; + +describe('DatabaseConfig', () => { + beforeEach(() => { + Container.reset(); + jest.clearAllMocks(); + }); + + test('`isLegacySqlite` defaults to true', () => { + const databaseConfig = Container.get(DatabaseConfig); + expect(databaseConfig.isLegacySqlite).toBe(true); + }); + + test.each(['mariadb', 'mysqldb', 'postgresdb'] satisfies Array)( + '`isLegacySqlite` returns false if dbType is `%s`', + (dbType) => { + const databaseConfig = Container.get(DatabaseConfig); + databaseConfig.sqlite.poolSize = 0; + databaseConfig.type = dbType; + expect(databaseConfig.isLegacySqlite).toBe(false); + }, + ); + + test('`isLegacySqlite` returns false if dbType is `sqlite` and `poolSize` > 0', () => { + const databaseConfig = Container.get(DatabaseConfig); + databaseConfig.sqlite.poolSize = 1; + databaseConfig.type = 'sqlite'; + expect(databaseConfig.isLegacySqlite).toBe(false); + }); + + test('`isLegacySqlite` returns true if dbType is `sqlite` and `poolSize` is 0', () => { + const databaseConfig = Container.get(DatabaseConfig); + databaseConfig.sqlite.poolSize = 0; + databaseConfig.type = 'sqlite'; + expect(databaseConfig.isLegacySqlite).toBe(true); + }); +}); diff --git a/packages/@n8n/config/src/configs/database.config.ts b/packages/@n8n/config/src/configs/database.config.ts index 75dda6ae3b..52b4a842e2 100644 --- a/packages/@n8n/config/src/configs/database.config.ts +++ b/packages/@n8n/config/src/configs/database.config.ts @@ -145,6 +145,15 @@ export class DatabaseConfig { @Env('DB_TYPE', dbTypeSchema) type: DbType = 'sqlite'; + /** + * Is true if the default sqlite data source of TypeORM is used, as opposed + * to any other (e.g. postgres) + * This also returns false if n8n's new pooled sqlite data source is used. + */ + get isLegacySqlite() { + return this.type === 'sqlite' && this.sqlite.poolSize === 0; + } + /** Prefix for table names */ @Env('DB_TABLE_PREFIX') tablePrefix: string = ''; diff --git a/packages/@n8n/config/test/config.test.ts b/packages/@n8n/config/test/config.test.ts index 02e30e0387..99fd56c36d 100644 --- a/packages/@n8n/config/test/config.test.ts +++ b/packages/@n8n/config/test/config.test.ts @@ -71,6 +71,7 @@ describe('GlobalConfig', () => { }, tablePrefix: '', type: 'sqlite', + isLegacySqlite: true, }, credentials: { defaultName: 'My credentials', @@ -309,7 +310,10 @@ describe('GlobalConfig', () => { it('should use all default values when no env variables are defined', () => { process.env = {}; const config = Container.get(GlobalConfig); - expect(structuredClone(config)).toEqual(defaultConfig); + // Makes sure the objects are structurally equal while respecting getters, + // which `toEqual` and `toBe` does not do. + expect(defaultConfig).toMatchObject(config); + expect(config).toMatchObject(defaultConfig); expect(mockFs.readFileSync).not.toHaveBeenCalled(); }); @@ -365,7 +369,7 @@ describe('GlobalConfig', () => { mockFs.readFileSync.calledWith(passwordFile, 'utf8').mockReturnValueOnce('password-from-file'); const config = Container.get(GlobalConfig); - expect(structuredClone(config)).toEqual({ + const expected = { ...defaultConfig, database: { ...defaultConfig.database, @@ -374,7 +378,11 @@ describe('GlobalConfig', () => { password: 'password-from-file', }, }, - }); + }; + // Makes sure the objects are structurally equal while respecting getters, + // which `toEqual` and `toBe` does not do. + expect(config).toMatchObject(expected); + expect(expected).toMatchObject(config); expect(mockFs.readFileSync).toHaveBeenCalled(); }); diff --git a/packages/@n8n/config/tsconfig.build.json b/packages/@n8n/config/tsconfig.build.json index 0794319028..ad06174279 100644 --- a/packages/@n8n/config/tsconfig.build.json +++ b/packages/@n8n/config/tsconfig.build.json @@ -7,5 +7,5 @@ "tsBuildInfoFile": "dist/build.tsbuildinfo" }, "include": ["src/**/*.ts"], - "exclude": ["test/**"] + "exclude": ["test/**", "src/**/__tests__/**"] } diff --git a/packages/cli/src/services/project.service.ee.ts b/packages/cli/src/services/project.service.ee.ts index ac46a574d1..b051241fc4 100644 --- a/packages/cli/src/services/project.service.ee.ts +++ b/packages/cli/src/services/project.service.ee.ts @@ -1,4 +1,6 @@ import type { CreateProjectDto, ProjectType, UpdateProjectDto } from '@n8n/api-types'; +import { LicenseState } from '@n8n/backend-common'; +import { DatabaseConfig } from '@n8n/config'; import { UNLIMITED_LICENSE_QUOTA } from '@n8n/constants'; import type { User } from '@n8n/db'; import { @@ -20,7 +22,6 @@ import { UserError } from 'n8n-workflow'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; -import { License } from '@/license'; import { CacheService } from './cache/cache.service'; @@ -46,7 +47,8 @@ export class ProjectService { private readonly projectRelationRepository: ProjectRelationRepository, private readonly sharedCredentialsRepository: SharedCredentialsRepository, private readonly cacheService: CacheService, - private readonly license: License, + private readonly licenseState: LicenseState, + private readonly databaseConfig: DatabaseConfig, ) {} private get workflowService() { @@ -181,25 +183,48 @@ export class ProjectService { return await this.projectRelationRepository.getPersonalProjectOwners(projectIds); } - async createTeamProject(adminUser: User, data: CreateProjectDto): Promise { - const limit = this.license.getTeamProjectLimit(); - if ( - limit !== UNLIMITED_LICENSE_QUOTA && - limit <= (await this.projectRepository.count({ where: { type: 'team' } })) - ) { - throw new TeamProjectOverQuotaError(limit); + private async createTeamProjectWithEntityManager( + adminUser: User, + data: CreateProjectDto, + trx: EntityManager, + ) { + const limit = this.licenseState.getMaxTeamProjects(); + if (limit !== UNLIMITED_LICENSE_QUOTA) { + const teamProjectCount = await trx.count(Project, { where: { type: 'team' } }); + if (teamProjectCount >= limit) { + throw new TeamProjectOverQuotaError(limit); + } } - const project = await this.projectRepository.save( + const project = await trx.save( + Project, this.projectRepository.create({ ...data, type: 'team' }), ); // Link admin - await this.addUser(project.id, adminUser.id, 'project:admin'); + await this.addUser(project.id, adminUser.id, 'project:admin', trx); return project; } + async createTeamProject(adminUser: User, data: CreateProjectDto): Promise { + if (this.databaseConfig.isLegacySqlite) { + // Using transaction in the sqlite legacy driver can cause data loss, so + // we avoid this here. + return await this.createTeamProjectWithEntityManager( + adminUser, + data, + this.projectRepository.manager, + ); + } else { + // This needs to be SERIALIZABLE otherwise the count would not block a + // concurrent transaction and we could insert multiple projects. + return await this.projectRepository.manager.transaction('SERIALIZABLE', async (trx) => { + return await this.createTeamProjectWithEntityManager(adminUser, data, trx); + }); + } + } + async updateProject( projectId: string, data: Pick, @@ -252,11 +277,11 @@ export class ProjectService { private isProjectRoleLicensed(role: ProjectRole) { switch (role) { case 'project:admin': - return this.license.isProjectRoleAdminLicensed(); + return this.licenseState.isProjectRoleAdminLicensed(); case 'project:editor': - return this.license.isProjectRoleEditorLicensed(); + return this.licenseState.isProjectRoleEditorLicensed(); case 'project:viewer': - return this.license.isProjectRoleViewerLicensed(); + return this.licenseState.isProjectRoleViewerLicensed(); default: return true; } @@ -326,8 +351,9 @@ export class ProjectService { }); } - async addUser(projectId: string, userId: string, role: ProjectRole) { - return await this.projectRelationRepository.save({ + async addUser(projectId: string, userId: string, role: ProjectRole, trx?: EntityManager) { + trx = trx ?? this.projectRelationRepository.manager; + return await trx.save(ProjectRelation, { projectId, userId, role, diff --git a/packages/cli/test/integration/project.api.test.ts b/packages/cli/test/integration/project.api.test.ts index 02b12eb57c..2c488409f7 100644 --- a/packages/cli/test/integration/project.api.test.ts +++ b/packages/cli/test/integration/project.api.test.ts @@ -1,3 +1,4 @@ +import { GlobalConfig } from '@n8n/config'; import type { Project } from '@n8n/db'; import { FolderRepository } from '@n8n/db'; import { ProjectRelationRepository } from '@n8n/db'; @@ -432,6 +433,34 @@ describe('POST /projects/', () => { expect(await Container.get(ProjectRepository).count({ where: { type: 'team' } })).toBe(2); }); + + const globalConfig = Container.get(GlobalConfig); + // Preventing this relies on transactions and we can't use them with the + // sqlite legacy driver due to data loss risks. + if (!globalConfig.database.isLegacySqlite) { + test('should respect the quota when trying to create multiple projects in parallel (no race conditions)', async () => { + expect(await Container.get(ProjectRepository).count({ where: { type: 'team' } })).toBe(0); + testServer.license.setQuota('quota:maxTeamProjects', 3); + const ownerUser = await createOwner(); + const ownerAgent = testServer.authAgentFor(ownerUser); + await expect( + Container.get(ProjectRepository).count({ where: { type: 'team' } }), + ).resolves.toBe(0); + + await Promise.all([ + ownerAgent.post('/projects/').send({ name: 'Test Team Project 1' }), + ownerAgent.post('/projects/').send({ name: 'Test Team Project 2' }), + ownerAgent.post('/projects/').send({ name: 'Test Team Project 3' }), + ownerAgent.post('/projects/').send({ name: 'Test Team Project 4' }), + ownerAgent.post('/projects/').send({ name: 'Test Team Project 5' }), + ownerAgent.post('/projects/').send({ name: 'Test Team Project 6' }), + ]); + + await expect( + Container.get(ProjectRepository).count({ where: { type: 'team' } }), + ).resolves.toBe(3); + }); + } }); describe('PATCH /projects/:projectId', () => { diff --git a/packages/cli/test/integration/workflows/workflow-sharing.service.test.ts b/packages/cli/test/integration/workflows/workflow-sharing.service.test.ts index 2a491c6504..a88ba352a2 100644 --- a/packages/cli/test/integration/workflows/workflow-sharing.service.test.ts +++ b/packages/cli/test/integration/workflows/workflow-sharing.service.test.ts @@ -1,13 +1,13 @@ +import { LicenseState } from '@n8n/backend-common'; import type { User } from '@n8n/db'; import { Container } from '@n8n/di'; +import { mock } from 'jest-mock-extended'; -import { License } from '@/license'; import { ProjectService } from '@/services/project.service.ee'; import { WorkflowSharingService } from '@/workflows/workflow-sharing.service'; import { createUser } from '../shared/db/users'; import { createWorkflow, shareWorkflowWithUsers } from '../shared/db/workflows'; -import { LicenseMocker } from '../shared/license'; import * as testDb from '../shared/test-db'; let owner: User; @@ -21,11 +21,10 @@ beforeAll(async () => { owner = await createUser({ role: 'global:owner' }); member = await createUser({ role: 'global:member' }); anotherMember = await createUser({ role: 'global:member' }); - let license: LicenseMocker; - license = new LicenseMocker(); - license.mock(Container.get(License)); - license.enable('feat:sharing'); - license.setQuota('quota:maxTeamProjects', -1); + const licenseMock = mock(); + licenseMock.isSharingLicensed.mockReturnValue(true); + licenseMock.getMaxTeamProjects.mockReturnValue(-1); + Container.set(LicenseState, licenseMock); workflowSharingService = Container.get(WorkflowSharingService); projectService = Container.get(ProjectService); });