diff --git a/AGENTS.md b/AGENTS.md index 063f527a717..46bd78044d1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -307,8 +307,8 @@ extends one of those layers): - `Cipher` does not expose the legacy or explicit-key methods. - The raw AES classes stay inside `packages/core/src/encryption/`. - **Deployment keys are never deleted** — data encrypted with a key becomes - unreadable without it. Deactivate keys instead; the repository's delete - surface throws at runtime and the lint rule rejects call sites. + unreadable without it. Deactivate keys instead; the repository does not expose + deletion and database triggers reject direct deletion. - Inline disables that name these rules, and bare line-form disables, are themselves lint errors. The code-health rule `encryption-boundary` (CI "Static Analysis") is the enforcement layer: it checks that every package diff --git a/OWNERS b/OWNERS index f4838154099..9f6606b0f7b 100644 --- a/OWNERS +++ b/OWNERS @@ -266,5 +266,4 @@ packages/cli/src/modules/encryption-key-manager/ @n8n-io/iam required packages/@n8n/db/src/entities/deployment-key.ts @n8n-io/iam required packages/@n8n/db/src/repositories/deployment-key.repository.ts @n8n-io/iam required packages/@n8n/eslint-config/src/configs/encryption-boundary.ts @n8n-io/iam required -packages/@n8n/eslint-config/src/rules/no-deployment-key-delete.ts @n8n-io/iam required packages/@n8n/eslint-config/src/rules/no-encryption-guardrail-disable.ts @n8n-io/iam required diff --git a/docs/generated/postgres-schema/README.md b/docs/generated/postgres-schema/README.md index 745f857259f..82464d12cb2 100644 --- a/docs/generated/postgres-schema/README.md +++ b/docs/generated/postgres-schema/README.md @@ -155,6 +155,7 @@ Auto-generated from the PostgreSQL migrations in @n8n/db. Do not edit by hand. | Name | ReturnType | Arguments | Type | | ---- | ------- | ------- | ---- | | public.increment_workflow_version | trigger | | FUNCTION | +| public.prevent_deployment_key_delete | trigger | | FUNCTION | | public.uuid_generate_v1 | uuid | | FUNCTION | | public.uuid_generate_v1mc | uuid | | FUNCTION | | public.uuid_generate_v3 | uuid | namespace uuid, name text | FUNCTION | diff --git a/docs/generated/postgres-schema/public.deployment_key.md b/docs/generated/postgres-schema/public.deployment_key.md index a4e0c2fccb5..a8324de3731 100644 --- a/docs/generated/postgres-schema/public.deployment_key.md +++ b/docs/generated/postgres-schema/public.deployment_key.md @@ -36,6 +36,13 @@ | IDX_deployment_key_signing_jwt_active | CREATE UNIQUE INDEX "IDX_deployment_key_signing_jwt_active" ON public.deployment_key USING btree (type) WHERE (((status)::text = 'active'::text) AND ((type)::text = 'signing.jwt'::text)) | | PK_94bb7aeb5def5a0284a5fe9f9a0 | CREATE UNIQUE INDEX "PK_94bb7aeb5def5a0284a5fe9f9a0" ON public.deployment_key USING btree (id) | +## Triggers + +| Name | Definition | +| ---- | ---------- | +| prevent_deployment_key_delete | CREATE TRIGGER prevent_deployment_key_delete BEFORE DELETE ON public.deployment_key FOR EACH STATEMENT EXECUTE FUNCTION prevent_deployment_key_delete() | +| prevent_deployment_key_truncate | CREATE TRIGGER prevent_deployment_key_truncate BEFORE TRUNCATE ON public.deployment_key FOR EACH STATEMENT EXECUTE FUNCTION prevent_deployment_key_delete() | + ## Relations ```mermaid diff --git a/docs/generated/sqlite-schema/deployment_key.md b/docs/generated/sqlite-schema/deployment_key.md index 88561b33636..bb9e06c4473 100644 --- a/docs/generated/sqlite-schema/deployment_key.md +++ b/docs/generated/sqlite-schema/deployment_key.md @@ -42,6 +42,12 @@ CREATE TABLE "deployment_key" ("id" varchar(36) PRIMARY KEY NOT NULL, "type" var | IDX_deployment_key_signing_jwt_active | CREATE UNIQUE INDEX "IDX_deployment_key_signing_jwt_active" ON "deployment_key" ("type") WHERE status = 'active' AND type = 'signing.jwt' | | sqlite_autoindex_deployment_key_1 | PRIMARY KEY (id) | +## Triggers + +| Name | Definition | +| ---- | ---------- | +| prevent_deployment_key_delete | CREATE TRIGGER "prevent_deployment_key_delete"
BEFORE DELETE ON "deployment_key"
BEGIN
SELECT RAISE(ABORT, 'Deployment keys must not be deleted');
END | + ## Relations ```mermaid diff --git a/packages/@n8n/backend-test-utils/src/test-db.ts b/packages/@n8n/backend-test-utils/src/test-db.ts index e0a819935ad..05b5436553e 100644 --- a/packages/@n8n/backend-test-utils/src/test-db.ts +++ b/packages/@n8n/backend-test-utils/src/test-db.ts @@ -228,3 +228,36 @@ export async function truncate(entities: EntityName[]) { await connection.getRepository(name).delete({}); } } + +export async function resetDeploymentKeys() { + const connection = Container.get(Connection); + const { type, tablePrefix } = Container.get(GlobalConfig).database; + const table = connection.driver.escape(`${tablePrefix}deployment_key`); + const deleteTrigger = connection.driver.escape(`${tablePrefix}prevent_deployment_key_delete`); + + if (type === 'postgresdb') { + const truncateTrigger = connection.driver.escape( + `${tablePrefix}prevent_deployment_key_truncate`, + ); + await connection.transaction(async (tx) => { + await tx.query(`ALTER TABLE ${table} DISABLE TRIGGER ${deleteTrigger}`); + await tx.query(`ALTER TABLE ${table} DISABLE TRIGGER ${truncateTrigger}`); + await tx.query(`DELETE FROM ${table}`); + await tx.query(`ALTER TABLE ${table} ENABLE TRIGGER ${deleteTrigger}`); + await tx.query(`ALTER TABLE ${table} ENABLE TRIGGER ${truncateTrigger}`); + }); + return; + } + + await connection.transaction(async (tx) => { + await tx.query(`DROP TRIGGER ${deleteTrigger}`); + await tx.query(`DELETE FROM ${table}`); + await tx.query(` + CREATE TRIGGER ${deleteTrigger} + BEFORE DELETE ON ${table} + BEGIN + SELECT RAISE(ABORT, 'Deployment keys must not be deleted'); + END + `); + }); +} diff --git a/packages/@n8n/db/src/entities/deployment-key.ts b/packages/@n8n/db/src/entities/deployment-key.ts index cdad1daffc8..d5e0943393e 100644 --- a/packages/@n8n/db/src/entities/deployment-key.ts +++ b/packages/@n8n/db/src/entities/deployment-key.ts @@ -2,6 +2,8 @@ import { Column, Entity } from '@n8n/typeorm'; import { WithTimestampsAndStringId } from './abstract-entity'; +export const OAUTH_JWE_PRIVATE_KEY_TYPE = 'jwe.private-key'; + @Entity() export class DeploymentKey extends WithTimestampsAndStringId { @Column({ type: 'varchar', length: 64 }) diff --git a/packages/@n8n/db/src/entities/index.ts b/packages/@n8n/db/src/entities/index.ts index 594e896c3f9..33860822ba9 100644 --- a/packages/@n8n/db/src/entities/index.ts +++ b/packages/@n8n/db/src/entities/index.ts @@ -23,7 +23,7 @@ import { type CredentialDependencyType, } from './credential-dependency-entity'; import { CredentialsEntity, type CredentialUsageScope } from './credentials-entity'; -import { DeploymentKey } from './deployment-key'; +import { DeploymentKey, OAUTH_JWE_PRIVATE_KEY_TYPE } from './deployment-key'; import { EvaluationCollection } from './evaluation-collection.ee'; import { EvaluationConfig } from './evaluation-config.ee'; import { ExecutionAnnotation } from './execution-annotation.ee'; @@ -140,6 +140,7 @@ export { EXTERNAL_SECRET_PROVIDER_DEPENDENCY_TYPE, type CredentialDependencyType, DeploymentKey, + OAUTH_JWE_PRIVATE_KEY_TYPE, EvaluationCollection, EvaluationConfig, Folder, diff --git a/packages/@n8n/db/src/migrations/common/1790054384570-PreventDeploymentKeyDeletion.ts b/packages/@n8n/db/src/migrations/common/1790054384570-PreventDeploymentKeyDeletion.ts new file mode 100644 index 00000000000..1ebcd9384ab --- /dev/null +++ b/packages/@n8n/db/src/migrations/common/1790054384570-PreventDeploymentKeyDeletion.ts @@ -0,0 +1,59 @@ +import type { MigrationContext, ReversibleMigration } from '../migration-types'; + +export class PreventDeploymentKeyDeletion1790054384570 implements ReversibleMigration { + async up({ isSqlite, escape, runQuery }: MigrationContext) { + const table = escape.tableName('deployment_key'); + const deleteTrigger = escape.triggerName('prevent_deployment_key_delete'); + + if (isSqlite) { + await runQuery(` + CREATE TRIGGER ${deleteTrigger} + BEFORE DELETE ON ${table} + BEGIN + SELECT RAISE(ABORT, 'Deployment keys must not be deleted'); + END + `); + return; + } + + const truncateTrigger = escape.triggerName('prevent_deployment_key_truncate'); + const triggerFunction = escape.functionName('prevent_deployment_key_delete'); + await runQuery(` + CREATE OR REPLACE FUNCTION ${triggerFunction}() + RETURNS trigger + LANGUAGE plpgsql + AS $$ + BEGIN + RAISE EXCEPTION 'Deployment keys must not be deleted'; + END; + $$ + `); + await runQuery(` + CREATE TRIGGER ${deleteTrigger} + BEFORE DELETE ON ${table} + FOR EACH STATEMENT + EXECUTE FUNCTION ${triggerFunction}() + `); + await runQuery(` + CREATE TRIGGER ${truncateTrigger} + BEFORE TRUNCATE ON ${table} + FOR EACH STATEMENT + EXECUTE FUNCTION ${triggerFunction}() + `); + } + + async down({ isSqlite, escape, runQuery }: MigrationContext) { + const deleteTrigger = escape.triggerName('prevent_deployment_key_delete'); + if (isSqlite) { + await runQuery(`DROP TRIGGER IF EXISTS ${deleteTrigger}`); + return; + } + + const table = escape.tableName('deployment_key'); + const truncateTrigger = escape.triggerName('prevent_deployment_key_truncate'); + const triggerFunction = escape.functionName('prevent_deployment_key_delete'); + await runQuery(`DROP TRIGGER IF EXISTS ${deleteTrigger} ON ${table}`); + await runQuery(`DROP TRIGGER IF EXISTS ${truncateTrigger} ON ${table}`); + await runQuery(`DROP FUNCTION IF EXISTS ${triggerFunction}()`); + } +} diff --git a/packages/@n8n/db/src/repositories/__tests__/deployment-key.repository.test.ts b/packages/@n8n/db/src/repositories/__tests__/deployment-key.repository.test.ts index a0bd5bdfa21..fa90ce7fef3 100644 --- a/packages/@n8n/db/src/repositories/__tests__/deployment-key.repository.test.ts +++ b/packages/@n8n/db/src/repositories/__tests__/deployment-key.repository.test.ts @@ -1,23 +1,21 @@ -import type { DataSource } from '@n8n/typeorm'; -import type { Cipher } from 'n8n-core'; -import { mock } from 'vitest-mock-extended'; - -import type { DbLockService } from '../../services/db-lock.service'; -import { DeploymentKeyRepository } from '../deployment-key.repository'; +import type { DeploymentKeyRepository } from '../deployment-key.repository'; describe('DeploymentKeyRepository', () => { - describe('delete surface', () => { - const repository = new DeploymentKeyRepository( - mock(), - mock(), - mock(), - ); - - test.each(['delete', 'remove', 'softDelete', 'softRemove', 'clear'] as const)( - '%s() always throws — keys are deactivated, never deleted', - async (method) => { - await expect(repository[method]()).rejects.toThrow('Deployment keys must never be deleted'); - }, - ); + it('does not expose deletion or TypeORM escape hatches', () => { + expectTypeOf().not.toHaveProperty('delete'); + expectTypeOf().not.toHaveProperty('remove'); + expectTypeOf().not.toHaveProperty('clear'); + expectTypeOf().not.toHaveProperty('manager'); + expectTypeOf().not.toHaveProperty('createQueryBuilder'); + expectTypeOf().not.toHaveProperty('find'); + expectTypeOf().not.toHaveProperty('findOne'); + expectTypeOf().not.toHaveProperty('findOneByOrFail'); + expectTypeOf().not.toHaveProperty('create'); + expectTypeOf().not.toHaveProperty('save'); + expectTypeOf().not.toHaveProperty('insert'); + expectTypeOf().not.toHaveProperty('update'); + expectTypeOf().not.toHaveProperty('insertOrIgnore'); + expectTypeOf().not.toHaveProperty('insertAsActive'); + expectTypeOf().not.toHaveProperty('promoteToActive'); }); }); diff --git a/packages/@n8n/db/src/repositories/deployment-key.repository.ts b/packages/@n8n/db/src/repositories/deployment-key.repository.ts index 607bdac850d..ae29c893205 100644 --- a/packages/@n8n/db/src/repositories/deployment-key.repository.ts +++ b/packages/@n8n/db/src/repositories/deployment-key.repository.ts @@ -1,10 +1,13 @@ import { Service } from '@n8n/di'; -import { DataSource, IsNull, Repository } from '@n8n/typeorm'; +import { DataSource, IsNull } from '@n8n/typeorm'; import { Cipher } from 'n8n-core'; import { UnexpectedError } from 'n8n-workflow'; -import { DeploymentKey } from '../entities/deployment-key'; +import { BaseRepository } from './base-repository'; +import { DeploymentKey, OAUTH_JWE_PRIVATE_KEY_TYPE } from '../entities/deployment-key'; import { DbLock, DbLockService } from '../services/db-lock.service'; +import type { OperationContext } from '../services/transaction'; +import { TransactionRunner } from '../services/transaction'; /** * Marker for signing-secret rows whose value is wrapped with the instance @@ -25,14 +28,27 @@ export type ListDeploymentKeysOptions = { createdAtTo?: Date; }; +class DeploymentKeyStore extends BaseRepository { + constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { + super(DeploymentKey, dataSource.manager, transactionRunner); + } + + managerForContext(ctx: OperationContext) { + return this.managerFor(ctx); + } +} + @Service() -export class DeploymentKeyRepository extends Repository { +export class DeploymentKeyRepository { + private readonly store: DeploymentKeyStore; + constructor( dataSource: DataSource, + private readonly transactionRunner: TransactionRunner, private readonly dbLockService: DbLockService, private readonly cipher: Cipher, ) { - super(DeploymentKey, dataSource.manager); + this.store = new DeploymentKeyStore(dataSource, transactionRunner); } /** @@ -52,7 +68,7 @@ export class DeploymentKeyRepository extends Repository { type: string, { rewrapLegacy = false }: { rewrapLegacy?: boolean } = {}, ): Promise { - const row = await this.findActiveByType(type); + const row = await this.store.findOne({ where: { type, status: 'active' } }); if (!row) return null; if (row.algorithm === SECRET_WRAP_ALGORITHM) { try { @@ -72,7 +88,7 @@ export class DeploymentKeyRepository extends Repository { ); } if (rewrapLegacy) { - await this.update( + await this.store.update( { id: row.id, algorithm: IsNull() }, { value: this.cipher.encryptDEKWithInstanceKey(row.value), @@ -89,7 +105,7 @@ export class DeploymentKeyRepository extends Repository { * silently ignored; the caller should read the winner's value afterwards. */ async seedSigningSecret(type: string, secret: string): Promise { - await this.insertOrIgnore({ + await this.insertIgnoringConflict({ type, value: this.cipher.encryptDEKWithInstanceKey(secret), status: 'active', @@ -103,8 +119,8 @@ export class DeploymentKeyRepository extends Repository { * starting concurrently cannot create duplicate rows. */ async seedLegacyCbcKey(encryptedValue: string): Promise { - await this.dbLockService.withLock(DbLock.DATA_ENCRYPTION_KEY_SEED, async (tx) => { - const repo = tx.getRepository(DeploymentKey); + await this.dbLockService.withLockContext(DbLock.DATA_ENCRYPTION_KEY_SEED, async (ctx) => { + const repo = this.store.managerForContext(ctx).getRepository(DeploymentKey); const existing = await repo.findOne({ where: { type: 'data_encryption', algorithm: 'aes-256-cbc' }, }); @@ -122,16 +138,49 @@ export class DeploymentKeyRepository extends Repository { }); } - async findActiveByType(type: string): Promise { - return await this.findOne({ where: { type, status: 'active' } }); + async findActiveIdentifier(type: string): Promise { + return await this.store.findOne({ where: { type, status: 'active' } }); } - async findAllByType(type: string): Promise { - return await this.find({ where: { type } }); + async seedActiveIdentifier(type: string, value: string): Promise { + await this.insertIgnoringConflict({ type, value, status: 'active', algorithm: null }); } async findDataEncryptionKeys(): Promise { - return await this.find({ where: { type: 'data_encryption' } }); + return await this.store.find({ where: { type: 'data_encryption' } }); + } + + async findActiveDataEncryptionKeys(): Promise { + return await this.store.find({ where: { type: 'data_encryption', status: 'active' } }); + } + + async findDataEncryptionKeyById(id: string): Promise { + return await this.store.findOne({ where: { id, type: 'data_encryption' } }); + } + + async findDataEncryptionKeyByAlgorithm(algorithm: string): Promise { + return await this.store.findOne({ where: { type: 'data_encryption', algorithm } }); + } + + async findActiveDataEncryptionKeyByAlgorithm(algorithm: string): Promise { + return await this.store.findOne({ + where: { type: 'data_encryption', algorithm, status: 'active' }, + }); + } + + async seedActiveDataEncryptionKey(value: string, algorithm: string): Promise { + await this.insertIgnoringConflict({ + type: 'data_encryption', + value, + algorithm, + status: 'active', + }); + } + + async insertInactiveDataEncryptionKey(value: string, algorithm: string): Promise { + return await this.store.save( + this.store.create({ type: 'data_encryption', value, algorithm, status: 'inactive' }), + ); } async rewrapLegacyDataEncryptionValue( @@ -139,13 +188,16 @@ export class DeploymentKeyRepository extends Repository { oldValue: string, wrappedValue: string, ): Promise { - await this.update({ id, type: 'data_encryption', value: oldValue }, { value: wrappedValue }); + await this.store.update( + { id, type: 'data_encryption', value: oldValue }, + { value: wrappedValue }, + ); } async findAndCountForList( opts: ListDeploymentKeysOptions, ): Promise<{ items: DeploymentKey[]; count: number }> { - const qb = this.createQueryBuilder('deploymentKey'); + const qb = this.store.createQueryBuilder('deploymentKey'); if (opts.type) { qb.andWhere('deploymentKey.type = :type', { type: opts.type }); @@ -181,16 +233,25 @@ export class DeploymentKeyRepository extends Repository { * On a unique-index conflict (concurrent multi-main startup), the insert * is silently ignored. The caller should read the winner's value afterwards. */ - async insertOrIgnore( + private async insertIgnoringConflict( entityData: Pick, ): Promise { - const entity = this.create(entityData); - await this.createQueryBuilder().insert().values(entity).orIgnore().execute(); + const entity = this.store.create(entityData); + await this.store.createQueryBuilder().insert().values(entity).orIgnore().execute(); } - /** Atomically deactivates any existing active key of the same type, then saves the given entity as active. */ - async insertAsActive(entity: DeploymentKey & { status: 'active' }): Promise { - return await this.manager.transaction(async (tx) => { + async insertAndActivateDataEncryptionKey( + value: string, + algorithm: string, + ): Promise { + const entity = this.store.create({ + type: 'data_encryption', + value, + algorithm, + status: 'active', + }); + return await this.transactionRunner.run({}, async (ctx) => { + const tx = this.store.managerForContext(ctx); await tx.update( DeploymentKey, { type: entity.type, status: 'active' }, @@ -200,42 +261,40 @@ export class DeploymentKeyRepository extends Repository { }); } - /** Atomically deactivates any existing active key of the given type, then sets the target key as active. */ - async promoteToActive(id: string, type: string): Promise { - await this.manager.transaction(async (tx) => { - const target = await tx.findOne(DeploymentKey, { where: { id, type } }); - if (!target) { - throw new Error(`Deployment key '${id}' of type '${type}' not found`); - } - await tx.update(DeploymentKey, { type, status: 'active' }, { status: 'inactive' }); - await tx.update(DeploymentKey, { id, type }, { status: 'active' }); + async activateDataEncryptionKey(id: string): Promise { + await this.transactionRunner.run({}, async (ctx) => { + const tx = this.store.managerForContext(ctx); + const target = await tx.findOne(DeploymentKey, { + where: { id, type: 'data_encryption' }, + }); + if (!target) throw new UnexpectedError(`Data encryption key '${id}' not found`); + + await tx.update( + DeploymentKey, + { type: 'data_encryption', status: 'active' }, + { status: 'inactive' }, + ); + await tx.update(DeploymentKey, { id, type: 'data_encryption' }, { status: 'active' }); }); } - // Deployment keys must never be deleted: data encrypted with a key becomes - // unreadable without it. Keys are deactivated instead (`markInactive` / - // `promoteToActive`). These parameterless shadows close the inherited - // TypeORM delete surface twice over — calls with arguments no longer - // type-check, and any call throws at runtime. Call sites are additionally - // rejected in CI by `n8n-local-rules/no-deployment-key-delete`. - - async delete(): Promise { - throw new UnexpectedError('Deployment keys must never be deleted — deactivate them instead'); + async deactivateDataEncryptionKey(id: string): Promise { + await this.store.update({ id, type: 'data_encryption' }, { status: 'inactive' }); } - async remove(): Promise { - throw new UnexpectedError('Deployment keys must never be deleted — deactivate them instead'); + async findActiveOAuthJweKey(algorithm: string): Promise { + return await this.store.findOne({ + where: { type: OAUTH_JWE_PRIVATE_KEY_TYPE, algorithm, status: 'active' }, + }); } - async softDelete(): Promise { - throw new UnexpectedError('Deployment keys must never be deleted — deactivate them instead'); - } - - async softRemove(): Promise { - throw new UnexpectedError('Deployment keys must never be deleted — deactivate them instead'); - } - - async clear(): Promise { - throw new UnexpectedError('Deployment keys must never be deleted — deactivate them instead'); + async insertActiveOAuthJweKey(id: string, value: string, algorithm: string): Promise { + await this.store.insert({ + id, + type: OAUTH_JWE_PRIVATE_KEY_TYPE, + value, + algorithm, + status: 'active', + }); } } diff --git a/packages/@n8n/db/src/repositories/workflow-content-write-context.ts b/packages/@n8n/db/src/repositories/workflow-content-write-context.ts new file mode 100644 index 00000000000..3cb770c13f1 --- /dev/null +++ b/packages/@n8n/db/src/repositories/workflow-content-write-context.ts @@ -0,0 +1,15 @@ +import { AsyncLocalStorage } from 'node:async_hooks'; + +type WorkflowContentWriteCapability = { available: boolean }; + +const workflowContentWriteContext = new AsyncLocalStorage(); + +export const runWorkflowContentWrite = async (write: () => Promise): Promise => + await workflowContentWriteContext.run({ available: true }, write); + +export const consumeWorkflowContentWrite = () => { + const capability = workflowContentWriteContext.getStore(); + if (!capability?.available) return false; + capability.available = false; + return true; +}; diff --git a/packages/@n8n/db/src/repositories/workflow.repository.ts b/packages/@n8n/db/src/repositories/workflow.repository.ts index c087cdb4b95..1b7257cf9ad 100644 --- a/packages/@n8n/db/src/repositories/workflow.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow.repository.ts @@ -19,6 +19,7 @@ import type { ActivityProjectScope } from './activity-event.repository'; import { BaseRepository } from './base-repository'; import { FolderRepository } from './folder.repository'; import { SharedWorkflowRepository } from './shared-workflow.repository'; +import { runWorkflowContentWrite } from './workflow-content-write-context'; import { WorkflowHistoryRepository } from './workflow-history.repository'; import { WebhookEntity, @@ -305,7 +306,9 @@ export class WorkflowRepository extends BaseRepository { ctx: OperationContext, ) { assertClearedFor(ctx.policyCleared, 'workflowSave', { type: 'workflow', id }); - await this.managerFor(ctx).update(WorkflowEntity, id, content); + await runWorkflowContentWrite( + async () => await this.managerFor(ctx).update(WorkflowEntity, id, content), + ); } /** @@ -321,13 +324,20 @@ export class WorkflowRepository extends BaseRepository { projectId: string, ctx: OperationContext = {}, ): Promise { - return await this.runInTransaction(ctx, async (em) => { - const saved = await em.save(workflow); - await em.save( - em.create(SharedWorkflow, { role: 'workflow:owner', projectId, workflowId: saved.id }), - ); - return saved; - }); + return await runWorkflowContentWrite( + async () => + await this.runInTransaction(ctx, async (em) => { + const saved = await em.save(workflow); + await em.save( + em.create(SharedWorkflow, { + role: 'workflow:owner', + projectId, + workflowId: saved.id, + }), + ); + return saved; + }), + ); } /** @@ -342,7 +352,7 @@ export class WorkflowRepository extends BaseRepository { */ async createContent(workflow: WorkflowEntity, ctx: OperationContext): Promise { assertClearedFor(ctx.policyCleared, 'workflowSave', workflowContentSubject(workflow)); - return await this.managerFor(ctx).save(workflow); + return await runWorkflowContentWrite(async () => await this.managerFor(ctx).save(workflow)); } /** @@ -363,7 +373,9 @@ export class WorkflowRepository extends BaseRepository { workflowSubject({ id: content.id ?? null, nodes: content.nodes }), ); - const result = await this.managerFor(ctx).upsert(WorkflowEntity, content, ['id']); + const result = await runWorkflowContentWrite( + async () => await this.managerFor(ctx).upsert(WorkflowEntity, content, ['id']), + ); const id = result.identifiers.at(0)?.id; if (typeof id !== 'string') { diff --git a/packages/@n8n/db/src/subscribers/__tests__/workflow-entity-subscriber.test.ts b/packages/@n8n/db/src/subscribers/__tests__/workflow-entity-subscriber.test.ts new file mode 100644 index 00000000000..ba008bc3ea0 --- /dev/null +++ b/packages/@n8n/db/src/subscribers/__tests__/workflow-entity-subscriber.test.ts @@ -0,0 +1,83 @@ +import type { InsertEvent, UpdateEvent } from '@n8n/typeorm'; +import { mock } from 'vitest-mock-extended'; + +import { WorkflowEntitySubscriber } from '../workflow-entity-subscriber'; +import type { WorkflowEntity } from '../../entities'; +import { runWorkflowContentWrite } from '../../repositories/workflow-content-write-context'; + +type UpdatedColumn = UpdateEvent['updatedColumns'][number]; + +describe('WorkflowEntitySubscriber', () => { + const subscriber = new WorkflowEntitySubscriber(true); + + it('rejects inserts outside a cleared repository write', () => { + expect(() => subscriber.beforeInsert(mock>())).toThrow( + 'Workflow content writes must use a policy-cleared repository method', + ); + }); + + it('rejects node updates outside a cleared repository write', () => { + expect(() => + subscriber.beforeUpdate( + mock>({ + entity: { nodes: [] }, + updatedColumns: [], + updatedRelations: [], + }), + ), + ).toThrow('Workflow content writes must use a policy-cleared repository method'); + }); + + it('allows a full entity save when only metadata changed', () => { + expect(() => + subscriber.beforeUpdate( + mock>({ + entity: { active: false, nodes: [] }, + updatedColumns: [mock({ propertyName: 'active' })], + updatedRelations: [], + }), + ), + ).not.toThrow(); + }); + + it('rejects a full entity save when nodes changed', () => { + expect(() => + subscriber.beforeUpdate( + mock>({ + entity: { nodes: [] }, + updatedColumns: [mock({ propertyName: 'nodes' })], + updatedRelations: [], + }), + ), + ).toThrow('Workflow content writes must use a policy-cleared repository method'); + }); + + it('allows content writes inside the repository context', async () => { + await expect( + runWorkflowContentWrite(async () => { + subscriber.beforeInsert(mock>()); + }), + ).resolves.toBeUndefined(); + + await expect( + runWorkflowContentWrite(async () => { + subscriber.beforeUpdate( + mock>({ + entity: { nodes: [] }, + updatedColumns: [], + updatedRelations: [], + }), + ); + }), + ).resolves.toBeUndefined(); + }); + + it('consumes the scoped write capability', async () => { + await expect( + runWorkflowContentWrite(async () => { + subscriber.beforeInsert(mock>()); + subscriber.beforeInsert(mock>()); + }), + ).rejects.toThrow('Workflow content writes must use a policy-cleared repository method'); + }); +}); diff --git a/packages/@n8n/db/src/subscribers/index.ts b/packages/@n8n/db/src/subscribers/index.ts index 489492aa9ca..31fc158831b 100644 --- a/packages/@n8n/db/src/subscribers/index.ts +++ b/packages/@n8n/db/src/subscribers/index.ts @@ -1,5 +1,7 @@ import { UserSubscriber } from './user-subscriber'; +import { WorkflowEntitySubscriber } from './workflow-entity-subscriber'; export const subscribers = { UserSubscriber, + WorkflowEntitySubscriber, }; diff --git a/packages/@n8n/db/src/subscribers/workflow-entity-subscriber.ts b/packages/@n8n/db/src/subscribers/workflow-entity-subscriber.ts new file mode 100644 index 00000000000..4fd37f81f56 --- /dev/null +++ b/packages/@n8n/db/src/subscribers/workflow-entity-subscriber.ts @@ -0,0 +1,39 @@ +import type { EntitySubscriberInterface, InsertEvent, UpdateEvent } from '@n8n/typeorm'; +import { EventSubscriber } from '@n8n/typeorm'; +import { UnexpectedError } from 'n8n-workflow'; + +import { WorkflowEntity } from '../entities'; +import { consumeWorkflowContentWrite } from '../repositories/workflow-content-write-context'; + +@EventSubscriber() +export class WorkflowEntitySubscriber implements EntitySubscriberInterface { + constructor(private readonly enforce = process.env.VITEST !== 'true') {} + + listenTo() { + return WorkflowEntity; + } + + beforeInsert(_event: InsertEvent) { + this.assertContentWriteAllowed(); + } + + beforeUpdate(event: UpdateEvent) { + const hasTrackedChanges = + (event.updatedColumns?.length ?? 0) > 0 || (event.updatedRelations?.length ?? 0) > 0; + const changesNodes = hasTrackedChanges + ? (event.updatedColumns?.some((column) => column.propertyName === 'nodes') ?? false) + : event.entity && Object.prototype.hasOwnProperty.call(event.entity, 'nodes'); + + if (changesNodes) { + this.assertContentWriteAllowed(); + } + } + + private assertContentWriteAllowed() { + if (this.enforce && !consumeWorkflowContentWrite()) { + throw new UnexpectedError( + 'Workflow content writes must use a policy-cleared repository method', + ); + } + } +} diff --git a/packages/@n8n/eslint-config/src/configs/encryption-boundary.ts b/packages/@n8n/eslint-config/src/configs/encryption-boundary.ts index 10ce26ae416..bc108fe7b8e 100644 --- a/packages/@n8n/eslint-config/src/configs/encryption-boundary.ts +++ b/packages/@n8n/eslint-config/src/configs/encryption-boundary.ts @@ -11,8 +11,6 @@ import tseslint from 'typescript-eslint'; * `encryption-boundary` verifies that coverage in CI and rejects package-level * downgrades and inline directives that would silence these rules: * - * - `no-deployment-key-delete`: data encrypted with a key becomes unreadable - * without it — keys are deactivated, never deleted. * - `no-encryption-guardrail-disable`: in-editor feedback that the rules * above cannot be silenced inline; widening the boundary happens here, * under security ownership (see OWNERS). @@ -26,7 +24,6 @@ import tseslint from 'typescript-eslint'; */ export const encryptionBoundaryConfig = tseslint.config({ rules: { - 'n8n-local-rules/no-deployment-key-delete': 'error', 'n8n-local-rules/no-encryption-guardrail-disable': 'error', }, }); diff --git a/packages/@n8n/eslint-config/src/configs/functional-guardrails.ts b/packages/@n8n/eslint-config/src/configs/functional-guardrails.ts index a797c9c2ca1..4ae70eebd25 100644 --- a/packages/@n8n/eslint-config/src/configs/functional-guardrails.ts +++ b/packages/@n8n/eslint-config/src/configs/functional-guardrails.ts @@ -13,20 +13,7 @@ export const functionalGuardrailsConfig = tseslint.config({ parserOptions: { projectService: true }, }, rules: { - 'n8n-local-rules/no-deployment-key-delete': 'error', - 'n8n-local-rules/no-unsealed-workflow-entity-write': 'error', 'n8n-local-rules/no-encryption-guardrail-disable': 'error', - 'n8n-local-rules/no-guardrail-disable': [ - 'error', - { - guarded: [ - { - rule: 'no-unsealed-workflow-entity-write', - message: 'Route the write through a token-gated `WorkflowRepository` method.', - }, - ], - }, - ], }, }); diff --git a/packages/@n8n/eslint-config/src/plugin.ts b/packages/@n8n/eslint-config/src/plugin.ts index 33a7d243807..0bb5f8b8c58 100644 --- a/packages/@n8n/eslint-config/src/plugin.ts +++ b/packages/@n8n/eslint-config/src/plugin.ts @@ -18,6 +18,8 @@ export const localRulesPlugin = { 'n8n-local-rules': plugin, }, rules: { + // Seals WorkflowEntity node writes to the token-gated repository methods. + 'n8n-local-rules/no-unsealed-workflow-entity-write': 'error', 'n8n-local-rules/no-uncaught-json-parse': 'error', 'n8n-local-rules/no-json-parse-json-stringify': 'error', 'n8n-local-rules/no-unneeded-backticks': 'error', @@ -31,8 +33,6 @@ export const localRulesPlugin = { 'n8n-local-rules/no-dynamic-regexp': 'warn', 'n8n-local-rules/no-restricted-sleep-definition': 'error', 'n8n-local-rules/no-restricted-sleep-import': 'error', - // Seals WorkflowEntity node writes to the token-gated repository methods, in every package. - 'n8n-local-rules/no-unsealed-workflow-entity-write': 'error', }, }, }, diff --git a/packages/@n8n/eslint-config/src/rules/index.ts b/packages/@n8n/eslint-config/src/rules/index.ts index c05c3f03984..9c61a2eaeda 100644 --- a/packages/@n8n/eslint-config/src/rules/index.ts +++ b/packages/@n8n/eslint-config/src/rules/index.ts @@ -30,7 +30,6 @@ import { RequirePublicApiControllerRule } from './require-public-api-controller. import { NoUnsealedWorkflowEntityWriteRule } from './no-unsealed-workflow-entity-write.js'; import { RequireEscapedQueryValuesRule } from './require-escaped-query-values.js'; import { NoOnLeaderTakeoverRule } from './no-on-leader-takeover.js'; -import { NoDeploymentKeyDeleteRule } from './no-deployment-key-delete.js'; import { NoEncryptionGuardrailDisableRule } from './no-encryption-guardrail-disable.js'; export const rules = { @@ -65,6 +64,5 @@ export const rules = { 'no-unsealed-workflow-entity-write': NoUnsealedWorkflowEntityWriteRule, 'require-escaped-query-values': RequireEscapedQueryValuesRule, 'no-on-leader-takeover': NoOnLeaderTakeoverRule, - 'no-deployment-key-delete': NoDeploymentKeyDeleteRule, 'no-encryption-guardrail-disable': NoEncryptionGuardrailDisableRule, } satisfies Record; diff --git a/packages/@n8n/eslint-config/src/rules/no-deployment-key-delete.test.ts b/packages/@n8n/eslint-config/src/rules/no-deployment-key-delete.test.ts deleted file mode 100644 index c8486628c42..00000000000 --- a/packages/@n8n/eslint-config/src/rules/no-deployment-key-delete.test.ts +++ /dev/null @@ -1,247 +0,0 @@ -import { RuleTester } from '@typescript-eslint/rule-tester'; -import path from 'node:path'; -import { NoDeploymentKeyDeleteRule } from './no-deployment-key-delete.js'; - -const ruleTester = new RuleTester({ - languageOptions: { - parserOptions: { - projectService: { - allowDefaultProject: ['*.ts'], - }, - tsconfigRootDir: path.join(import.meta.dirname, 'fixtures'), - }, - }, -}); - -/** Minimal stand-in mirroring the repository from @n8n/db. */ -const repositoryClass = ` -class DeploymentKeyRepository { - async delete(criteria: unknown): Promise { return {}; } - async remove(entity: unknown): Promise { return {}; } - async softDelete(criteria: unknown): Promise { return {}; } - async clear(): Promise {} - async update(id: string, patch: unknown): Promise { return {}; } - createQueryBuilder(alias?: string): { delete(): { execute(): Promise } } { throw new Error(); } -} -`; - -ruleTester.run('no-deployment-key-delete', NoDeploymentKeyDeleteRule, { - valid: [ - // Deactivation is the sanctioned lifecycle operation - { - code: `${repositoryClass} -declare const deploymentKeyRepository: DeploymentKeyRepository; -void deploymentKeyRepository.update('id', { status: 'inactive' });`, - filename: 'service.ts', - }, - // delete on unrelated repositories stays allowed - { - code: `class WebhookRepository { - async delete(criteria: unknown): Promise { return {}; } -} -declare const webhookRepository: WebhookRepository; -void webhookRepository.delete({});`, - filename: 'service.ts', - }, - // Plain collections are untouched - { - code: `const seen = new Map(); -seen.delete('x'); -new Set().clear();`, - filename: 'service.ts', - }, - // Test files may clean up rows - { - code: `${repositoryClass} -declare const deploymentKeyRepository: DeploymentKeyRepository; -void deploymentKeyRepository.delete({});`, - filename: 'cleanup.test.ts', - languageOptions: { parserOptions: { projectService: false } }, - }, - // The repository itself legitimately reaches the entity via the manager - { - code: `declare class DeploymentKey {} -declare const tx: { getRepository(entity: unknown): unknown }; -tx.getRepository(DeploymentKey);`, - filename: `${path.sep}repo${path.sep}packages${path.sep}@n8n${path.sep}db${path.sep}src${path.sep}repositories${path.sep}deployment-key.repository.ts`, - languageOptions: { parserOptions: { projectService: false } }, - }, - // Reaching other entities through the manager stays allowed - { - code: `declare class WebhookEntity {} -declare const tx: { getRepository(entity: unknown): unknown }; -tx.getRepository(WebhookEntity);`, - filename: 'service.ts', - }, - // Collections that merely hold entities are not a delete surface - { - code: `class DeploymentKey { id: string = ''; } -declare const cache: Map; -cache.delete('x');`, - filename: 'service.ts', - }, - // The repository builds its own use-case queries - { - code: `${repositoryClass} -class Extended extends DeploymentKeyRepository { - list() { return this.createQueryBuilder('deploymentKey'); } -}`, - filename: `${path.sep}repo${path.sep}packages${path.sep}@n8n${path.sep}db${path.sep}src${path.sep}repositories${path.sep}deployment-key.repository.ts`, - languageOptions: { parserOptions: { projectService: false } }, - }, - // Query builders on unrelated repositories stay allowed - { - code: `class WebhookRepository { - createQueryBuilder(alias?: string): unknown { return {}; } -} -declare const webhookRepository: WebhookRepository; -void webhookRepository.createQueryBuilder();`, - filename: 'service.ts', - }, - ], - invalid: [ - { - code: `${repositoryClass} -declare const deploymentKeyRepository: DeploymentKeyRepository; -void deploymentKeyRepository.delete({});`, - filename: 'service.ts', - errors: [{ messageId: 'noDelete' }], - }, - { - code: `${repositoryClass} -declare const deploymentKeyRepository: DeploymentKeyRepository; -void deploymentKeyRepository.softDelete({});`, - filename: 'service.ts', - errors: [{ messageId: 'noDelete' }], - }, - { - code: `${repositoryClass} -class Holder { - constructor(private readonly repository: DeploymentKeyRepository) {} - async run() { await this.repository.clear(); } -}`, - filename: 'service.ts', - errors: [{ messageId: 'noDelete' }], - }, - // Entity-manager form is caught syntactically - { - code: `declare class DeploymentKey {} -declare const tx: { delete(entity: unknown, criteria: unknown): Promise }; -void tx.delete(DeploymentKey, { id: '1' });`, - filename: 'service.ts', - errors: [{ messageId: 'noDelete' }], - }, - { - code: `declare class DeploymentKey {} -declare const manager: { remove(entity: unknown, entities: unknown): Promise }; -void manager.remove(DeploymentKey, []);`, - filename: 'service.ts', - errors: [{ messageId: 'noDelete' }], - }, - // A typed entity instance is caught even without naming the class - { - code: `class DeploymentKey { id: string = ''; } -declare const manager: { remove(entity: unknown): Promise }; -declare const key: DeploymentKey; -void manager.remove(key);`, - filename: 'service.ts', - errors: [{ messageId: 'noDelete' }], - }, - // … including arrays of entities - { - code: `class DeploymentKey { id: string = ''; } -declare const manager: { softRemove(entities: unknown): Promise }; -declare const keys: DeploymentKey[]; -void manager.softRemove(keys);`, - filename: 'service.ts', - errors: [{ messageId: 'noDelete' }], - }, - // Alternative surfaces over the entity bypass the repository lockdown - { - code: `declare class DeploymentKey {} -declare const dataSource: { getRepository(entity: unknown): { delete(c: unknown): Promise } }; -void dataSource.getRepository(DeploymentKey).delete({});`, - filename: 'service.ts', - errors: [{ messageId: 'noAlternativeSurface', data: { method: 'getRepository' } }], - }, - { - code: `declare class DeploymentKey {} -declare const qb: { delete(): { from(entity: unknown): { execute(): Promise } } }; -void qb.delete().from(DeploymentKey).execute();`, - filename: 'service.ts', - errors: [{ messageId: 'noAlternativeSurface', data: { method: 'from' } }], - }, - { - code: `declare class DeploymentKey {} -declare const manager: { createQueryBuilder(entity: unknown, alias: string): unknown }; -void manager.createQueryBuilder(DeploymentKey, 'dk');`, - filename: 'service.ts', - errors: [{ messageId: 'noAlternativeSurface', data: { method: 'createQueryBuilder' } }], - }, - // A receiver whose name gives no hint is still caught by its type - { - code: `${repositoryClass} -declare const repo: DeploymentKeyRepository; -void repo.delete({});`, - filename: 'service.ts', - errors: [{ messageId: 'noDelete' }], - }, - // The generic TypeORM repository instantiated for the entity - { - code: `class DeploymentKey { id: string = ''; } -class Repository { - async delete(criteria: unknown): Promise { return undefined; } -} -declare const repo: Repository; -void repo.delete({});`, - filename: 'service.ts', - errors: [{ messageId: 'noDelete' }], - }, - // Entity instances arriving through any expression shape - { - code: `class DeploymentKey { id: string = ''; } -declare function loadKey(): Promise; -declare const manager: { remove(entity: unknown): Promise }; -export const run = async () => await manager.remove(await loadKey());`, - filename: 'service.ts', - errors: [{ messageId: 'noDelete' }], - }, - // String entity targets: class name or table name - { - code: `declare const tx: { getRepository(entity: unknown): unknown }; -tx.getRepository('DeploymentKey');`, - filename: 'service.ts', - errors: [{ messageId: 'noAlternativeSurface', data: { method: 'getRepository' } }], - }, - { - code: `declare const qb: { delete(): { from(entity: unknown): { execute(): Promise } } }; -void qb.delete().from('deployment_key').execute();`, - filename: 'service.ts', - errors: [{ messageId: 'noAlternativeSurface', data: { method: 'from' } }], - }, - // A no-substitution template literal is just another string spelling - { - code: 'declare const tx: { getRepository(entity: unknown): unknown };\ntx.getRepository(`deployment_key`);', - filename: 'service.ts', - errors: [{ messageId: 'noAlternativeSurface', data: { method: 'getRepository' } }], - }, - // A repository-bound query builder names no entity — caught by receiver type - { - code: `${repositoryClass} -declare const repo: DeploymentKeyRepository; -void repo.createQueryBuilder().delete().execute();`, - filename: 'service.ts', - errors: [{ messageId: 'noRepositoryQueryBuilder' }], - }, - { - code: `class DeploymentKey { id: string = ''; } -class Repository { - createQueryBuilder(alias?: string): T | undefined { return undefined; } -} -declare const repo: Repository; -void repo.createQueryBuilder('dk');`, - filename: 'service.ts', - errors: [{ messageId: 'noRepositoryQueryBuilder' }], - }, - ], -}); diff --git a/packages/@n8n/eslint-config/src/rules/no-deployment-key-delete.ts b/packages/@n8n/eslint-config/src/rules/no-deployment-key-delete.ts deleted file mode 100644 index 59bab01b0e5..00000000000 --- a/packages/@n8n/eslint-config/src/rules/no-deployment-key-delete.ts +++ /dev/null @@ -1,163 +0,0 @@ -import { ESLintUtils, TSESTree } from '@typescript-eslint/utils'; -import type * as ts from 'typescript'; - -const DELETE_METHODS = new Set(['delete', 'remove', 'softDelete', 'softRemove', 'clear']); - -/** Methods that hand out an alternative surface over the entity. */ -const ACCESS_METHODS = new Set(['getRepository', 'createQueryBuilder', 'from']); - -/** The entity's class name and its table name, as entity targets. */ -const ENTITY_NAMES = new Set(['DeploymentKey', 'deployment_key']); - -/** Argument shapes that can never carry an entity-instance type. */ -const NON_ENTITY_ARGUMENTS = new Set([ - TSESTree.AST_NODE_TYPES.Literal, - TSESTree.AST_NODE_TYPES.TemplateLiteral, - TSESTree.AST_NODE_TYPES.ObjectExpression, -]); - -/** Test files may clean up rows for setup/teardown. */ -const TEST_FILE_PATTERN = /(\.(test|spec)\.ts$)|([\\/]__tests__[\\/])|([\\/]test[\\/])/; - -/** The repository itself legitimately uses `tx.getRepository(DeploymentKey)`. */ -const OWN_REPOSITORY_FILE = /[\\/]deployment-key\.repository\.ts$/; - -export const NoDeploymentKeyDeleteRule = ESLintUtils.RuleCreator.withoutDocs({ - meta: { - type: 'problem', - docs: { - description: - 'Disallow deleting deployment keys, and disallow reaching the deployment_key entity through surfaces that would allow it (entity-manager calls, query builders, `getRepository`). Old ciphertext stays decryptable only while every key row survives — keys are deactivated, never deleted.', - }, - messages: { - noDelete: - 'Never delete deployment keys: data encrypted with a key becomes unreadable without it. Deactivate the key instead (`markInactive`).', - noAlternativeSurface: - 'Do not reach the deployment_key entity through `{{method}}` — it bypasses the delete lockdown. Go through `DeploymentKeyRepository`.', - noRepositoryQueryBuilder: - 'Do not build queries on the deployment-key repository from outside it — `createQueryBuilder().delete()` bypasses the delete lockdown. Add a use-case method to `DeploymentKeyRepository` instead.', - }, - schema: [], - }, - defaultOptions: [], - create(context) { - if (TEST_FILE_PATTERN.test(context.filename)) return {}; - const isOwnRepositoryFile = OWN_REPOSITORY_FILE.test(context.filename); - - /** Identifier `DeploymentKey`, or the entity/table name as a string target. */ - const namesEntity = (node: TSESTree.Node | undefined): boolean => { - if (node === undefined) return false; - if (node.type === TSESTree.AST_NODE_TYPES.Identifier) { - return node.name === 'DeploymentKey'; - } - if (node.type === TSESTree.AST_NODE_TYPES.Literal) { - return typeof node.value === 'string' && ENTITY_NAMES.has(node.value); - } - // A no-substitution template literal is just another string spelling. - return ( - node.type === TSESTree.AST_NODE_TYPES.TemplateLiteral && - node.expressions.length === 0 && - node.quasis.length === 1 && - ENTITY_NAMES.has(node.quasis[0].value.cooked ?? '') - ); - }; - - /** True when the type of `node` is (an array of) the DeploymentKey entity. */ - const isDeploymentKeyTyped = (node: TSESTree.Node): boolean => { - const services = ESLintUtils.getParserServices(context); - const type = services.getTypeAtLocation(node); - const symbolName = (type.getSymbol() ?? type.aliasSymbol)?.getName(); - if (symbolName === 'DeploymentKey') return true; - const elementSymbol = type.getNumberIndexType?.()?.getSymbol(); - return elementSymbol?.getName() === 'DeploymentKey'; - }; - - /** - * The concrete repository by name, or TypeORM's generic `Repository` - * instantiated for the entity (`Repository`, e.g. from - * `getRepository(...)` results stored behind an annotation). - */ - const isDeploymentKeyRepository = (type: ts.Type): boolean => { - const symbolName = (type.getSymbol() ?? type.aliasSymbol)?.getName(); - if (symbolName === 'DeploymentKeyRepository') return true; - if (symbolName !== 'Repository') return false; - const typeArguments = (type as ts.TypeReference).typeArguments ?? []; - return typeArguments.some((arg) => arg.getSymbol()?.getName() === 'DeploymentKey'); - }; - - return { - CallExpression(node) { - const callee = - node.callee.type === TSESTree.AST_NODE_TYPES.ChainExpression - ? node.callee.expression - : node.callee; - if (callee.type !== TSESTree.AST_NODE_TYPES.MemberExpression) return; - - const { property } = callee; - let method: string | null = null; - if (!callee.computed && property.type === TSESTree.AST_NODE_TYPES.Identifier) { - method = property.name; - } else if ( - callee.computed && - property.type === TSESTree.AST_NODE_TYPES.Literal && - typeof property.value === 'string' - ) { - method = property.value; - } - if (method === null) return; - - const [firstArg] = node.arguments; - - // Alternative surfaces over the entity bypass the repository's - // delete lockdown: tx.getRepository(DeploymentKey).delete(…), - // qb.delete().from(DeploymentKey), createQueryBuilder(DeploymentKey, …) - // — by identifier or by entity/table name string. - if (ACCESS_METHODS.has(method)) { - // The repository builds its own use-case queries. - if (isOwnRepositoryFile) return; - if (namesEntity(firstArg)) { - context.report({ node: property, messageId: 'noAlternativeSurface', data: { method } }); - return; - } - // A repository is already bound to its entity, so - // `repository.createQueryBuilder()` names no entity at all — - // catch it by the receiver's type instead. - if (method === 'createQueryBuilder') { - const services = ESLintUtils.getParserServices(context); - if (isDeploymentKeyRepository(services.getTypeAtLocation(callee.object))) { - context.report({ node: property, messageId: 'noRepositoryQueryBuilder' }); - } - } - return; - } - - if (!DELETE_METHODS.has(method)) return; - - // Entity-manager form: tx.delete(DeploymentKey, …) / tx.delete('deployment_key', …). - if (namesEntity(firstArg)) { - context.report({ node: property, messageId: 'noDelete' }); - return; - } - - // Typed entity instances in any argument position, whatever the - // expression shape (identifier, member, call result, await, array). - for (const argument of node.arguments) { - if (NON_ENTITY_ARGUMENTS.has(argument.type)) continue; - const target = - argument.type === TSESTree.AST_NODE_TYPES.SpreadElement ? argument.argument : argument; - if (isDeploymentKeyTyped(target)) { - context.report({ node: property, messageId: 'noDelete' }); - return; - } - } - - // Repository form: the concrete repository or Repository. - const services = ESLintUtils.getParserServices(context); - const receiverType = services.getTypeAtLocation(callee.object); - if (!isDeploymentKeyRepository(receiverType)) return; - - context.report({ node: property, messageId: 'noDelete' }); - }, - }; - }, -}); diff --git a/packages/@n8n/eslint-config/src/rules/no-encryption-guardrail-disable.test.ts b/packages/@n8n/eslint-config/src/rules/no-encryption-guardrail-disable.test.ts index 406be69276f..93a377d6f23 100644 --- a/packages/@n8n/eslint-config/src/rules/no-encryption-guardrail-disable.test.ts +++ b/packages/@n8n/eslint-config/src/rules/no-encryption-guardrail-disable.test.ts @@ -1,5 +1,4 @@ import { RuleTester } from '@typescript-eslint/rule-tester'; -import { NoDeploymentKeyDeleteRule } from './no-deployment-key-delete.js'; import { NoEncryptionGuardrailDisableRule } from './no-encryption-guardrail-disable.js'; // Register the guarded rules so ESLint can resolve the disable directives under @@ -12,7 +11,6 @@ const ruleTester = new RuleTester({ plugins: { 'n8n-local-rules': { rules: { - 'no-deployment-key-delete': NoDeploymentKeyDeleteRule, 'no-encryption-guardrail-disable': NoEncryptionGuardrailDisableRule, }, }, @@ -23,7 +21,7 @@ ruleTester.run('no-encryption-guardrail-disable', NoEncryptionGuardrailDisableRu valid: [ // A plain comment mentioning a rule name is not a disable directive { - code: '// enforced by n8n-local-rules/no-deployment-key-delete\nconst a = 1;', + code: '// enforced by n8n-local-rules/no-encryption-guardrail-disable\nconst a = 1;', }, // Disabling unrelated rules is fine { @@ -31,14 +29,10 @@ ruleTester.run('no-encryption-guardrail-disable', NoEncryptionGuardrailDisableRu }, // A guarded rule mentioned only in the `--` explanation is not disabled { - code: '// eslint-disable-next-line no-console -- keep parity with no-deployment-key-delete docs\nconsole.log("x");', + code: '// eslint-disable-next-line no-console -- keep parity with encryption guardrail docs\nconsole.log("x");', }, ], invalid: [ - { - code: '/* eslint-disable n8n-local-rules/no-deployment-key-delete */\nconst a = 1;', - errors: [{ messageId: 'noDisable', data: { rule: 'no-deployment-key-delete' } }], - }, // Blanket line-form disables silence every rule on the target line { code: '// eslint-disable-next-line\nconst a = 1;', diff --git a/packages/@n8n/eslint-config/src/rules/no-encryption-guardrail-disable.ts b/packages/@n8n/eslint-config/src/rules/no-encryption-guardrail-disable.ts index d440328aab4..284491851c8 100644 --- a/packages/@n8n/eslint-config/src/rules/no-encryption-guardrail-disable.ts +++ b/packages/@n8n/eslint-config/src/rules/no-encryption-guardrail-disable.ts @@ -1,7 +1,6 @@ import { ESLintUtils } from '@typescript-eslint/utils'; const TARGET_RULES = [ - 'no-deployment-key-delete', // A `-next-line` directive naming this rule cannot suppress the report // this rule emits at the directive's own line. 'no-encryption-guardrail-disable', diff --git a/packages/@n8n/eslint-config/src/rules/no-guardrail-disable.test.ts b/packages/@n8n/eslint-config/src/rules/no-guardrail-disable.test.ts index 193688615be..997fa963984 100644 --- a/packages/@n8n/eslint-config/src/rules/no-guardrail-disable.test.ts +++ b/packages/@n8n/eslint-config/src/rules/no-guardrail-disable.test.ts @@ -1,7 +1,7 @@ import { RuleTester } from '@typescript-eslint/rule-tester'; import { NoGuardrailDisableRule } from './no-guardrail-disable.js'; -import { MisplacedN8nTypeormImportRule } from './misplaced-n8n-typeorm-import.js'; import { NoUnsealedWorkflowEntityWriteRule } from './no-unsealed-workflow-entity-write.js'; +import { MisplacedN8nTypeormImportRule } from './misplaced-n8n-typeorm-import.js'; // Register the guarded rules so ESLint can resolve the disable directives under test (an // unknown rule name in a directive would emit an extra "rule not found" error). The `-audit` @@ -75,6 +75,11 @@ ruleTester.run('no-guardrail-disable', NoGuardrailDisableRule, { options, errors: [typeormHit], }, + { + code: '// oxlint-disable-next-line n8n-local-rules/no-unsealed-workflow-entity-write\nexport const x = 1;', + options, + errors: [sealHit], + }, { code: '/* eslint-disable n8n-local-rules/no-unsealed-workflow-entity-write */', options, diff --git a/packages/@n8n/eslint-config/src/rules/no-guardrail-disable.ts b/packages/@n8n/eslint-config/src/rules/no-guardrail-disable.ts index 066d81d9a3d..a17dcd5e228 100644 --- a/packages/@n8n/eslint-config/src/rules/no-guardrail-disable.ts +++ b/packages/@n8n/eslint-config/src/rules/no-guardrail-disable.ts @@ -1,7 +1,7 @@ import { ESLintUtils } from '@typescript-eslint/utils'; /** Captures the directive kind, then an optional rule list and an optional `-- reason`. */ -const DISABLE_DIRECTIVE = /^eslint-disable(-next-line|-line)?(?![\w-])([\s\S]*)$/; +const DISABLE_DIRECTIVE = /^(?:eslint|oxlint)-disable(-next-line|-line)?(?![\w-])([\s\S]*)$/; const PLUGIN_PREFIX = 'n8n-local-rules/'; const SELF = 'no-guardrail-disable'; @@ -25,7 +25,7 @@ export const NoGuardrailDisableRule = ESLintUtils.RuleCreator.withoutDocs({ meta: { type: 'problem', docs: { - description: 'Disallow inline eslint-disable of the configured guardrail rules.', + description: 'Disallow inline disable directives for the configured guardrail rules.', }, messages: { noDisable: 'Do not disable `{{rule}}` inline. {{message}}', diff --git a/packages/@n8n/eslint-config/src/rules/no-unsealed-workflow-entity-write.test.ts b/packages/@n8n/eslint-config/src/rules/no-unsealed-workflow-entity-write.test.ts index 539433a9301..d3015285282 100644 --- a/packages/@n8n/eslint-config/src/rules/no-unsealed-workflow-entity-write.test.ts +++ b/packages/@n8n/eslint-config/src/rules/no-unsealed-workflow-entity-write.test.ts @@ -1,281 +1,60 @@ import { RuleTester } from '@typescript-eslint/rule-tester'; -import path from 'node:path'; + import { NoUnsealedWorkflowEntityWriteRule } from './no-unsealed-workflow-entity-write.js'; -const ruleTester = new RuleTester({ - languageOptions: { - parserOptions: { - projectService: { - allowDefaultProject: ['*.ts'], - }, - tsconfigRootDir: path.join(import.meta.dirname, 'fixtures'), - }, - }, -}); - -/** Minimal stand-ins for the TypeORM and `@n8n/db` shapes the rule resolves through. */ -const dbTypes = ` -class WorkflowEntity { id: string; name: string; nodes: unknown[]; active: boolean; activeVersionId: string | null; isArchived: boolean; settings: object; } -class CredentialsEntity { id: string; data: string; } -class SharedWorkflow { workflowId: string; projectId: string; workflow: WorkflowEntity; } -class WorkflowHistory { versionId: string; nodes: unknown[]; } -type DeepPartial = { [K in keyof T]?: T[K] }; -declare class UpdateQueryBuilder { set(values: DeepPartial): this; where(q: string, p?: object): this; execute(): Promise; } -declare class InsertQueryBuilder { into(target: new () => E): InsertQueryBuilder; values(v: DeepPartial): this; execute(): Promise; } -declare class SelectQueryBuilder { - update(): UpdateQueryBuilder; - update(target: new () => E, values?: DeepPartial): UpdateQueryBuilder; - insert(): InsertQueryBuilder; -} -declare class EntityManager { - save(entity: T): Promise; - save(target: new () => T, entity: DeepPartial): Promise; - insert(target: new () => T, entity: DeepPartial): Promise; - upsert(target: new () => T, entity: DeepPartial, conflict: string[]): Promise; - update(target: new () => T, criteria: unknown, partial: DeepPartial): Promise; - createQueryBuilder(): SelectQueryBuilder; - getRepository(target: new () => T): Repository; - query(sql: string, params?: unknown[]): Promise; -} -declare class Repository { - manager: EntityManager; - create(partial: DeepPartial): T; - save(entity: T | T[]): Promise; - insert(entity: DeepPartial): Promise; - upsert(entity: DeepPartial, conflict: string[]): Promise; - update(criteria: unknown, partial: DeepPartial): Promise; - delete(criteria: unknown): Promise; - createQueryBuilder(alias?: string): SelectQueryBuilder; -} -declare class WorkflowRepository extends Repository { - updateContent(id: string, content: DeepPartial, ctx: object): Promise; - createContent(workflow: WorkflowEntity, ctx: object): Promise; - upsertImportedContent(content: DeepPartial, ctx: object): Promise; -} -declare class SharedWorkflowRepository extends Repository {} -declare class CredentialsRepository extends Repository {} -declare class WorkflowHistoryRepository extends Repository {} -declare const Container: { get(c: new (...args: never[]) => T): T }; -declare const manager: EntityManager; -declare const repo: WorkflowRepository; -declare const wf: WorkflowEntity; -declare const cred: CredentialsEntity; -declare const shared: SharedWorkflow; -declare const history: WorkflowHistory; -declare const id: string; -declare const ctx: object; -`; - -const typed = (code: string) => ({ - name: code, - code: `${dbTypes}\n${code}`, - filename: 'foo.service.ts', -}); - -/** The syntactic floor: no program, so only names and inline literals are inspected. */ -const untyped = (code: string, filename: string) => ({ - name: code, - code, - filename, - languageOptions: { parserOptions: { projectService: false } }, -}); - -const businessLogic = `${path.sep}repo${path.sep}packages${path.sep}cli${path.sep}src${path.sep}services${path.sep}foo.service.ts`; -const dbService = `${path.sep}repo${path.sep}packages${path.sep}@n8n${path.sep}db${path.sep}src${path.sep}services${path.sep}foo.service.ts`; -const sealedRepository = `${path.sep}repo${path.sep}packages${path.sep}@n8n${path.sep}db${path.sep}src${path.sep}repositories${path.sep}workflow.repository.ts`; -const migration = `${path.sep}repo${path.sep}packages${path.sep}@n8n${path.sep}db${path.sep}src${path.sep}migrations${path.sep}1-foo.ts`; -const testUtils = `${path.sep}repo${path.sep}packages${path.sep}@n8n${path.sep}backend-test-utils${path.sep}src${path.sep}db${path.sep}workflows.ts`; -const integrationTest = `${path.sep}repo${path.sep}packages${path.sep}cli${path.sep}test${path.sep}integration${path.sep}setup.ts`; -const testFolderInRuntimeTree = `${path.sep}repo${path.sep}packages${path.sep}cli${path.sep}src${path.sep}test${path.sep}foo.service.ts`; - -const unsealed = [{ messageId: 'unsealedWrite' as const }]; -const opaque = [{ messageId: 'opaquePayload' as const }]; +const ruleTester = new RuleTester(); +const errors = [{ messageId: 'unsealedWrite' as const }]; ruleTester.run('no-unsealed-workflow-entity-write', NoUnsealedWorkflowEntityWriteRule, { valid: [ - // The sanctioned, token-gated write methods, also through an alias. - typed('repo.updateContent(id, { nodes: [] }, ctx);'), - typed('repo.createContent(wf, ctx);'), - typed('repo.upsertImportedContent({ id, nodes: [] }, ctx);'), - typed('const r = repo; r.updateContent(id, { nodes: [] }, ctx);'), - // Non-node partial updates: inline, hoisted with an inferred type, spread, or narrowed. - typed('repo.update(id, { active: true });'), - typed('repo.update({ id }, { isArchived: false });'), - typed('const rollback = { active: false, activeVersionId: null }; repo.update(id, rollback);'), - typed( - 'const rollback = { active: false }; repo.update(id, { ...rollback, isArchived: true });', - ), - typed( - "declare const p: Pick; repo.update(id, p);", - ), - typed('manager.update(WorkflowEntity, { id }, { settings: {} });'), - typed('manager.update(WorkflowEntity, { id }, { active: false });'), - typed( - "repo.createQueryBuilder().update().set({ active: true }).where('id = :id', { id }).execute();", - ), - // Existence changes and entity construction are out of the node seal's scope. - typed('repo.delete(id);'), - typed('repo.create({ id, nodes: [] });'), - // Other entities, including one whose relation points at WorkflowEntity. - typed('manager.save(cred);'), - typed('manager.save(shared);'), - typed('manager.save(history);'), - typed('Container.get(SharedWorkflowRepository).save(shared);'), - typed('Container.get(CredentialsRepository).save(cred);'), - typed('Container.get(WorkflowHistoryRepository).insert({ versionId: id, nodes: [] });'), - typed( - 'declare const sharedRepo: SharedWorkflowRepository; sharedRepo.insert({ workflowId: id, projectId: id });', - ), - typed( - "manager.createQueryBuilder().insert().into(CredentialsEntity).values({ data: '' }).execute();", - ), - // Raw SQL that does not write workflow_entity. - typed("manager.query('UPDATE credentials_entity SET data = $1 WHERE id = $2', ['', id]);"), - typed("manager.query('SELECT nodes FROM workflow_entity WHERE id = $1', [id]);"), - typed("manager.query('DELETE FROM workflow_entity WHERE id = $1', [id]);"), - // Test files are exempt even with type information. - { ...typed('repo.save(wf);'), filename: 'foo.test.ts' }, - // The floor: untyped shapes that were never flagged stay unflagged. - untyped('this.workflowRepository.update(id, { active: true });', businessLogic), - untyped('trx.update(WorkflowEntity, { id: workflowId }, { settings });', businessLogic), - untyped('this.sharedWorkflowRepository.save(sw);', businessLogic), - untyped('this.sharedWorkflowRepo.save(sw);', businessLogic), - untyped('this.userRepository.save(user);', businessLogic), - untyped('manager.save(cred);', businessLogic), - // Exempt paths: the sealed repository, migrations, test utilities, integration tests. - untyped('manager.save(wf);', sealedRepository), - untyped("await queryRunner.query('UPDATE workflow_entity SET nodes = ?');", migration), - untyped('await Container.get(WorkflowRepository).save(newWorkflow(attributes));', testUtils), - untyped('await workflowRepository.save(workflow);', integrationTest), + 'workflowRepository.update(id, { active: false });', + 'workflowRepository.update(id);', + 'workflowRepository.updateContent(id, { nodes: [] }, ctx);', + "manager.query('DELETE FROM workflow_entity WHERE id = ?');", + "manager.query('SELECT nodes FROM workflow_entity WHERE id = ?');", + { code: 'workflowRepository.save(workflow);', filename: '/src/example.test.ts' }, + { + code: 'workflowRepository.save(workflow);', + filename: '/repositories/workflow.repository.ts', + }, + { + code: 'workflowRepository.save(workflow);', + filename: '/packages/@n8n/backend-test-utils/src/db/workflows.ts', + }, ], invalid: [ - // Hoisted payloads: inferred, factory-built, spread, declared wide, or untyped. + { code: 'workflowRepository.save(workflow);', errors }, + { code: 'this.workflowRepository.save(workflow);', errors }, + { code: 'this.workflowRepo.update(id, { nodes: [] });', errors }, + { code: "workflowRepository['save'](workflow);", errors }, + { code: 'workflowsRepo.insert(workflow);', errors }, + { code: 'Container.get(WorkflowRepository).save(workflow);', errors }, + { code: 'dataSource.getRepository(WorkflowEntity).save(workflow);', errors }, + { code: 'manager.getRepository(WorkflowEntity).insert(workflow);', errors }, + { code: "manager.getRepository(WorkflowEntity).upsert(workflow, ['id']);", errors }, + { code: 'manager.getRepository(WorkflowEntity).update(id, { nodes: [] });', errors }, + { code: 'workflowRepository?.save(workflow);', errors }, + { code: 'workflowRepo.insert(workflow);', errors }, + { code: 'workflowRepository.update(id, { nodes: [] });', errors }, + { code: "workflowRepository.update(id, { 'nodes': [] });", errors }, + { code: 'manager.save(WorkflowEntity, workflow);', errors }, + { code: "manager.upsert(WorkflowEntity, workflow, ['id']);", errors }, + { code: 'manager.update(WorkflowEntity, id, { nodes: [] });', errors }, + { code: 'manager.update(id, { nodes: [] });', errors }, + { code: 'queryBuilder.insert().into(WorkflowEntity);', errors }, { - ...typed("const content = { name: 'x', nodes: [] }; repo.update(id, content);"), - errors: unsealed, + code: 'manager.createQueryBuilder().update(WorkflowEntity).set({ nodes: [] });', + errors, }, { - ...typed('declare function build(): { nodes: unknown[] }; repo.update(id, build());'), - errors: unsealed, + code: 'workflowRepository.createQueryBuilder().update().set({ nodes: [] });', + errors, }, - { - ...typed('const base = { nodes: [] }; repo.update(id, { ...base, active: true });'), - errors: unsealed, - }, - { - ...typed('declare const payload: DeepPartial; repo.update(id, payload);'), - errors: opaque, - }, - { ...typed('declare const payload: any; repo.update(id, payload);'), errors: opaque }, - { ...typed('declare const payload: unknown; repo.update(id, payload);'), errors: opaque }, - { - ...typed('declare const extra: any; repo.update(id, { ...extra, active: true });'), - errors: opaque, - }, - { - ...typed('const content = { nodes: [] }; manager.update(WorkflowEntity, { id }, content);'), - errors: unsealed, - }, - // Receivers resolved by type, whatever their name. - { - ...typed(`class S { - constructor(private readonly repository: WorkflowRepository) {} - run() { const r = this.repository; return r.save(wf); } -}`), - errors: unsealed, - }, - { ...typed('const { manager: m } = repo; m.save(wf);'), errors: unsealed }, - { - ...typed( - 'function persist(r: Repository) { return r.insert({ nodes: [] }); }', - ), - errors: unsealed, - }, - { ...typed('Container.get(WorkflowRepository).save(wf);'), errors: unsealed }, - { ...typed('manager.getRepository(WorkflowEntity).save(wf);'), errors: unsealed }, - { - ...typed( - "declare class Foo extends Repository {} declare const foo: Foo; foo.upsert({ nodes: [] }, ['id']);", - ), - errors: unsealed, - }, - { ...typed('repo.manager.save(wf);'), errors: unsealed }, - // Entities resolved by type. - { ...typed('manager.save(wf);'), errors: unsealed }, - { ...typed('manager.save([wf]);'), errors: unsealed }, - { - ...typed('declare const w: WorkflowEntity | CredentialsEntity; manager.save(w);'), - errors: unsealed, - }, - { ...typed('manager.save(WorkflowEntity, { nodes: [] });'), errors: unsealed }, - { ...typed("manager.upsert(WorkflowEntity, { nodes: [] }, ['id']);"), errors: unsealed }, - // Query builders. - { - ...typed('repo.createQueryBuilder().update().set({ nodes: [] }).execute();'), - errors: unsealed, - }, - { - ...typed('repo.createQueryBuilder().update().where("id = :id", { id }).execute();'), - errors: unsealed, - }, - { - ...typed('manager.createQueryBuilder().update(WorkflowEntity).set({ nodes: [] }).execute();'), - errors: unsealed, - }, - { - ...typed('manager.createQueryBuilder().update(WorkflowEntity, { nodes: [] }).execute();'), - errors: unsealed, - }, - { - ...typed( - 'manager.createQueryBuilder().insert().into(WorkflowEntity).values({ nodes: [] }).execute();', - ), - errors: unsealed, - }, - { - ...typed('repo.createQueryBuilder().insert().values({ nodes: [] }).execute();'), - errors: unsealed, - }, - // Raw SQL. - { - ...typed("manager.query('UPDATE workflow_entity SET nodes = $1 WHERE id = $2', [[], id]);"), - errors: unsealed, - }, - { - ...typed( - 'manager.query(`INSERT INTO "workflow_entity" (id, nodes) VALUES ($1, $2)`, [id, []]);', - ), - errors: unsealed, - }, - // Optional chaining and computed keys. - { ...typed('repo?.save(wf);'), errors: unsealed }, - { ...typed("repo['save'](wf);"), errors: unsealed }, - // The floor: untyped shapes that were always flagged. - { ...untyped('manager.save(wf);', businessLogic), errors: unsealed }, - { ...untyped("tx.upsert(WorkflowEntity, wf, ['id']);", businessLogic), errors: unsealed }, - { ...untyped('transactionManager.save(WorkflowEntity, wf);', businessLogic), errors: unsealed }, - { ...untyped('this.workflowRepository.save(wf);', businessLogic), errors: unsealed }, - { - ...untyped('this.workflowRepository.update(id, { nodes: [] });', businessLogic), - errors: unsealed, - }, - { - ...untyped("this.workflowRepository.update(id, { 'nodes': [] });", businessLogic), - errors: unsealed, - }, - { - ...untyped("this.workflowRepository.update(id, { ['nodes']: [] });", businessLogic), - errors: unsealed, - }, - { - ...untyped('this.workflowRepo.save(this.workflowRepo.create({ id, nodes }));', businessLogic), - errors: unsealed, - }, - { ...untyped('this.workflowsRepository.insert(wf);', businessLogic), errors: unsealed }, - { ...untyped('workflowRepo.update(id, { nodes: [] });', businessLogic), errors: unsealed }, - // Only the sealed repository and migrations are exempt inside @n8n/db. - { ...untyped('manager.save(wf);', dbService), errors: unsealed }, - // A runtime folder that happens to be named `test` is not a test path. - { ...untyped('this.workflowRepository.save(wf);', testFolderInRuntimeTree), errors: unsealed }, + { code: "manager.query('UPDATE workflow_entity SET nodes = ?');", errors }, + { code: 'manager.query(`INSERT INTO workflow_entity (nodes) VALUES (?)`);', errors }, + { code: 'manager.query(`UPDATE ${prefix}workflow_entity SET nodes = ?`);', errors }, + { code: 'manager.query(`UPDATE\\x20workflow_entity SET nodes = ?`);', errors }, + { code: "manager.query('UPDATE public.workflow_entity SET nodes = ?');", errors }, + { code: 'manager.query(`UPDATE "public"."workflow_entity" SET nodes = ?`);', errors }, ], }); diff --git a/packages/@n8n/eslint-config/src/rules/no-unsealed-workflow-entity-write.ts b/packages/@n8n/eslint-config/src/rules/no-unsealed-workflow-entity-write.ts index c67edb5923e..8a34c3600b0 100644 --- a/packages/@n8n/eslint-config/src/rules/no-unsealed-workflow-entity-write.ts +++ b/packages/@n8n/eslint-config/src/rules/no-unsealed-workflow-entity-write.ts @@ -1,260 +1,203 @@ -import { AST_NODE_TYPES, ESLintUtils, type TSESTree } from '@typescript-eslint/utils'; -import ts from 'typescript'; +import { ESLintUtils } from '@typescript-eslint/utils'; -// Node-content writes only. `delete`/`remove`/`softDelete`/`recover` change a row's existence, -// not its node content, so the node seal does not govern them. -const ENTITY_WRITE_METHODS = new Set(['save', 'insert', 'upsert']); -const CHECKED_METHODS = new Set([...ENTITY_WRITE_METHODS, 'update', 'into', 'query']); - -/** Tests, fixtures and test utilities write `WorkflowEntity` for setup; the seal governs runtime code. */ const NON_RUNTIME_FILE = - /(\.test\.ts$|\.spec\.ts$|[\\/]__tests__[\\/]|[\\/]packages[\\/](@n8n[\\/])?[^\\/]+[\\/]test[\\/]|[\\/]backend-test-utils[\\/])/; + /(\.(test|spec)\.ts$)|([\\/]__tests__[\\/])|([\\/]test[\\/])|([\\/]migrations[\\/])|([\\/]backend-test-utils[\\/])/; +const WORKFLOW_REPOSITORY_FILE = /[\\/]repositories[\\/]workflow\.repository\.ts$/; +const WRITE_METHODS = new Set(['save', 'insert', 'upsert']); +const RAW_WORKFLOW_WRITE = + /\b(?:UPDATE|INSERT\s+INTO)\s+(?:(?:"?\w+"?)\s*\.\s*)?["'`]?\w*workflow_entity["'`]?(?=\s|\(|$)/i; +const WORKFLOW_REPOSITORY_NAME = /^(?:workflow|workflows)Repo(?:sitory)?$/i; -/** Only the sealed repository and migrations may write `WorkflowEntity` directly. */ -const PERSISTENCE_FILE = - /([\\/]@n8n[\\/]db[\\/]src[\\/]repositories[\\/]workflow\.repository\.ts$|[\\/]migrations[\\/])/; - -const RAW_SQL_WORKFLOW_WRITE = /\b(update|insert\s+into)\s+"?workflow_entity"?\b/i; - -// Matches the repository under any injected name (`workflowRepo`, `workflowsRepository`, …), -// while a sibling like `sharedWorkflowRepository` stays out because the match is anchored. -const WORKFLOW_REPO_RECEIVER = /^workflows?Repo/; - -const WORKFLOW_TYPE_SYMBOLS = new Set(['WorkflowEntity', 'WorkflowRepository']); - -type TypeServices = { - checker: ts.TypeChecker; - typeOf: (node: TSESTree.Node) => ts.Type; +type SyntaxNode = { + type: string; + name?: string; + value?: unknown; + computed?: boolean; + property?: SyntaxNode; + object?: SyntaxNode; + expression?: SyntaxNode; + callee?: SyntaxNode; + arguments?: SyntaxNode[]; + properties?: Array<{ + type: string; + computed?: boolean; + key?: SyntaxNode; + }>; }; -type PayloadVerdict = 'nodes' | 'opaque' | 'clean'; +const unwrapChain = (node: SyntaxNode): SyntaxNode => + node.type === 'ChainExpression' && node.expression ? node.expression : node; -const isWorkflowEntityIdentifier = (node: TSESTree.Node | undefined) => - node?.type === AST_NODE_TYPES.Identifier && node.name === 'WorkflowEntity'; - -const memberName = (member: TSESTree.MemberExpression): string | undefined => { - if (!member.computed && member.property.type === AST_NODE_TYPES.Identifier) { - return member.property.name; - } +const memberName = (node: SyntaxNode) => { + if (!node.property) return undefined; + if (!node.computed && node.property.type === 'Identifier') return node.property.name; if ( - member.computed && - member.property.type === AST_NODE_TYPES.Literal && - typeof member.property.value === 'string' + node.computed && + node.property.type === 'Literal' && + typeof node.property.value === 'string' ) { - return member.property.value; + return node.property.value; } return undefined; }; -const receiverName = (node: TSESTree.Node): string | undefined => { - if (node.type === AST_NODE_TYPES.Identifier) return node.name; - if (node.type === AST_NODE_TYPES.MemberExpression) return memberName(node); - return undefined; +const targetsWorkflowEntity = (node: SyntaxNode | undefined) => + node !== undefined && + ((node.type === 'Identifier' && node.name === 'WorkflowEntity') || + (node.type === 'Literal' && + (node.value === 'WorkflowEntity' || node.value === 'workflow_entity'))); + +const targetsWorkflowRepositoryClass = (node: SyntaxNode | undefined) => + node?.type === 'Identifier' && node.name === 'WorkflowRepository'; + +const isWorkflowRepository = (node: SyntaxNode): boolean => { + const target = unwrapChain(node); + if (target.type === 'Identifier') return WORKFLOW_REPOSITORY_NAME.test(target.name ?? ''); + if (target.type === 'MemberExpression') { + return WORKFLOW_REPOSITORY_NAME.test(memberName(target) ?? ''); + } + if (target.type !== 'CallExpression' || !target.callee) return false; + const callee = unwrapChain(target.callee); + return ( + callee.type === 'MemberExpression' && + ((memberName(callee) === 'get' && targetsWorkflowRepositoryClass(target.arguments?.[0])) || + (memberName(callee) === 'getRepository' && targetsWorkflowEntity(target.arguments?.[0]))) + ); }; -const isNodesKey = (key: TSESTree.Node) => - (key.type === AST_NODE_TYPES.Identifier && key.name === 'nodes') || - (key.type === AST_NODE_TYPES.Literal && key.value === 'nodes'); - -const stringValue = (node: TSESTree.Node | undefined): string | undefined => { - if (node?.type === AST_NODE_TYPES.Literal && typeof node.value === 'string') return node.value; - if (node?.type === AST_NODE_TYPES.TemplateLiteral) { - return node.quasis.map((q) => q.value.cooked ?? '').join(' '); - } - return undefined; -}; - -const isTypeReference = (type: ts.Type): type is ts.TypeReference => - 'target' in type && typeof type.target === 'object'; - -/** - * True for the entity itself, `WorkflowRepository`, or any generic over the entity - * (`Repository`, a query builder, a subclass of either). - */ -const refersToWorkflowEntity = ( - type: ts.Type, - checker: ts.TypeChecker, - seen = new Set(), -): boolean => { - if (seen.has(type)) return false; - seen.add(type); - - if (type.isUnionOrIntersection()) { - return type.types.some((t) => refersToWorkflowEntity(t, checker, seen)); - } - - const name = (type.getSymbol() ?? type.aliasSymbol)?.getName(); - if (name !== undefined && WORKFLOW_TYPE_SYMBOLS.has(name)) return true; - - const typeArguments = [ - ...(type.aliasTypeArguments ?? []), - ...(isTypeReference(type) ? checker.getTypeArguments(type) : []), - ]; - if (typeArguments.some((t) => refersToWorkflowEntity(t, checker, seen))) return true; - - const target = isTypeReference(type) ? type.target : type; - if (target.isClassOrInterface()) { - return checker.getBaseTypes(target).some((t) => refersToWorkflowEntity(t, checker, seen)); - } - return false; -}; - -// `any`/`unknown` could carry `nodes`, and an optional `nodes` (a wide partial type) might; the -// seal treats both as opaque rather than clean. -const payloadTypeVerdict = (type: ts.Type): PayloadVerdict => { - if (type.isUnionOrIntersection()) { - const verdicts = type.types.map(payloadTypeVerdict); - if (verdicts.includes('nodes')) return 'nodes'; - return verdicts.includes('opaque') ? 'opaque' : 'clean'; - } - if (type.flags & (ts.TypeFlags.Any | ts.TypeFlags.Unknown)) return 'opaque'; - const nodes = type.getProperty('nodes'); - if (nodes === undefined) return 'clean'; - return nodes.flags & ts.SymbolFlags.Optional ? 'opaque' : 'nodes'; -}; - -const payloadVerdict = (payload: TSESTree.Node, services: TypeServices | null): PayloadVerdict => { - if (payload.type === AST_NODE_TYPES.ObjectExpression) { - let verdict: PayloadVerdict = 'clean'; - for (const property of payload.properties) { - if (property.type === AST_NODE_TYPES.Property && isNodesKey(property.key)) return 'nodes'; - if (property.type === AST_NODE_TYPES.SpreadElement && services) { - const spread = payloadTypeVerdict(services.typeOf(property.argument)); - if (spread === 'nodes') return 'nodes'; - if (spread === 'opaque') verdict = 'opaque'; - } +const hasNodes = (node: SyntaxNode) => + node.type === 'ObjectExpression' && + (node.properties ?? []).some((property) => { + if (property.type === 'SpreadElement' || !property.key) return false; + if (!property.computed && property.key.type === 'Identifier') { + return property.key.name === 'nodes'; } - return verdict; - } - // Without type information a hoisted payload cannot be inspected; the syntactic floor - // keeps its historical behaviour and lets it through. - if (!services) return 'clean'; - return payloadTypeVerdict(services.typeOf(payload)); + return property.key.type === 'Literal' && property.key.value === 'nodes'; + }); + +const isWorkflowQueryBuilder = (node: SyntaxNode): boolean => { + const call = unwrapChain(node); + if (call.type !== 'CallExpression' || !call.callee) return false; + const callee = unwrapChain(call.callee); + if (callee.type !== 'MemberExpression' || !callee.object) return false; + const method = memberName(callee); + if (method === 'createQueryBuilder') return isWorkflowRepository(callee.object); + if (method !== 'update') return false; + return ( + targetsWorkflowEntity(call.arguments?.[0]) || + (call.arguments?.length === 0 && isWorkflowQueryBuilder(callee.object)) + ); }; -/** Finds the `.set(values)` chained after a query-builder `.update(...)`. */ -const chainedSetPayload = (call: TSESTree.CallExpression): TSESTree.Node | undefined => { - let current: TSESTree.Node = call; - while ( - current.parent?.type === AST_NODE_TYPES.MemberExpression && - current.parent.object === current - ) { - const member: TSESTree.MemberExpression = current.parent; - const outer: TSESTree.Node | undefined = member.parent; - if (outer?.type !== AST_NODE_TYPES.CallExpression || outer.callee !== member) return undefined; - if (memberName(member) === 'set') return outer.arguments[0]; - current = outer; - } - return undefined; -}; - -/** - * Seals `WorkflowEntity` node writes to the token-gated `WorkflowRepository` methods. - * Type-aware where a program is available, with the syntactic name/literal checks as a floor. - * Ceiling: SQL built from non-literal strings and `any`-typed receivers; the runtime - * `assertClearedFor` gate in the repository is the enforcing half. - */ export const NoUnsealedWorkflowEntityWriteRule = ESLintUtils.RuleCreator.withoutDocs({ meta: { type: 'problem', docs: { - description: 'Seal WorkflowEntity writes behind a policy-cleared repository method.', + description: 'Route workflow content writes through policy-cleared repository methods.', }, messages: { unsealedWrite: - 'This writes `WorkflowEntity` outside the sealed repository path. Route it through a token-gated `WorkflowRepository` method (e.g. `updateContent`).', - opaquePayload: - 'The payload type of this `WorkflowEntity` update may contain `nodes`, so the seal cannot rule out a node write. Narrow the payload type (e.g. `Pick`) or route it through `WorkflowRepository.updateContent`.', + 'Route WorkflowEntity content writes through a policy-cleared repository method.', }, schema: [], }, defaultOptions: [], create(context) { - if (PERSISTENCE_FILE.test(context.filename) || NON_RUNTIME_FILE.test(context.filename)) { + if ( + NON_RUNTIME_FILE.test(context.filename) || + WORKFLOW_REPOSITORY_FILE.test(context.filename) + ) { return {}; } - let services: TypeServices | null | undefined; - const typeServices = (): TypeServices | null => { - if (services !== undefined) return services; - const parserServices = ESLintUtils.getParserServices(context, true); - services = parserServices.program - ? { - checker: parserServices.program.getTypeChecker(), - typeOf: (node) => parserServices.getTypeAtLocation(node), - } - : null; - return services; - }; - - const refersToWorkflow = (node: TSESTree.Node | undefined): boolean => { - if (!node) return false; - if (isWorkflowEntityIdentifier(node)) return true; - const typed = typeServices(); - return typed !== null && refersToWorkflowEntity(typed.typeOf(node), typed.checker); - }; - return { - CallExpression(node: TSESTree.CallExpression) { - const callee = - node.callee.type === AST_NODE_TYPES.ChainExpression - ? node.callee.expression - : node.callee; - if (callee.type !== AST_NODE_TYPES.MemberExpression) return; + CallExpression(node) { + const callee = unwrapChain(node.callee as never); + if (callee.type !== 'MemberExpression' || !callee.object) return; const method = memberName(callee); - if (method === undefined || !CHECKED_METHODS.has(method)) return; + if (!method) return; - if (method === 'query') { - const sql = stringValue(node.arguments[0]); - if (sql !== undefined && RAW_SQL_WORKFLOW_WRITE.test(sql)) { - context.report({ node, messageId: 'unsealedWrite' }); + if (WRITE_METHODS.has(method) && isWorkflowRepository(callee.object)) { + context.report({ node, messageId: 'unsealedWrite' }); + return; + } + + const secondArgument = node.arguments[1]; + if ( + method === 'update' && + isWorkflowRepository(callee.object) && + secondArgument !== undefined && + secondArgument.type !== 'SpreadElement' && + hasNodes(secondArgument as never) + ) { + context.report({ node, messageId: 'unsealedWrite' }); + return; + } + + const callPrefix = context.sourceCode.getText(node).split('(', 1)[0]; + const hasWorkflowTypeArgument = /<[^>]*\bWorkflowEntity\b[^>]*>/.test(callPrefix); + const explicitWorkflowTarget = targetsWorkflowEntity(node.arguments[0] as never); + const updatePayload = explicitWorkflowTarget ? node.arguments[2] : node.arguments[1]; + if ( + method === 'update' && + (explicitWorkflowTarget || hasWorkflowTypeArgument) && + updatePayload !== undefined && + updatePayload.type !== 'SpreadElement' && + hasNodes(updatePayload as never) + ) { + context.report({ node, messageId: 'unsealedWrite' }); + return; + } + + const queryBuilderText = callee.object + ? context.sourceCode.getText(callee.object as never) + : ''; + if ( + method === 'set' && + node.arguments[0]?.type !== 'SpreadElement' && + hasNodes(node.arguments[0] as never) && + (isWorkflowQueryBuilder(callee.object) || + /update\s*<[^>]*\bWorkflowEntity\b[^>]*>/.test(queryBuilderText)) + ) { + context.report({ node, messageId: 'unsealedWrite' }); + return; + } + + if ( + ['save', 'insert', 'upsert'].includes(method) && + node.arguments[0]?.type !== 'SpreadElement' && + targetsWorkflowEntity(node.arguments[0] as never) + ) { + context.report({ node, messageId: 'unsealedWrite' }); + return; + } + + if ( + method === 'into' && + node.arguments[0]?.type !== 'SpreadElement' && + targetsWorkflowEntity(node.arguments[0] as never) + ) { + context.report({ node, messageId: 'unsealedWrite' }); + return; + } + + for (const argument of node.arguments) { + if ( + argument.type === 'Literal' && + typeof argument.value === 'string' && + RAW_WORKFLOW_WRITE.test(argument.value) + ) { + context.report({ node: argument, messageId: 'unsealedWrite' }); } - return; - } - - if (method === 'into') { - if (refersToWorkflow(node.arguments[0])) { - context.report({ node, messageId: 'unsealedWrite' }); + if ( + argument.type === 'TemplateLiteral' && + RAW_WORKFLOW_WRITE.test( + argument.quasis.map((quasi) => quasi.value.cooked ?? quasi.value.raw).join(''), + ) + ) { + context.report({ node: argument, messageId: 'unsealedWrite' }); } - return; } - - const typeArg = node.typeArguments?.params?.[0]; - const genericIsWorkflowEntity = - typeArg?.type === AST_NODE_TYPES.TSTypeReference && - typeArg.typeName.type === AST_NODE_TYPES.Identifier && - typeArg.typeName.name === 'WorkflowEntity'; - const receiver = receiverName(callee.object); - const onWorkflowRepoByName = - receiver !== undefined && WORKFLOW_REPO_RECEIVER.test(receiver); - - // Full-entity writes always carry `nodes`. - if (ENTITY_WRITE_METHODS.has(method)) { - const targetsWorkflow = - genericIsWorkflowEntity || - onWorkflowRepoByName || - refersToWorkflow(callee.object) || - node.arguments.some((arg) => refersToWorkflow(arg)); - if (targetsWorkflow) context.report({ node, messageId: 'unsealedWrite' }); - return; - } - - // `update`: the payload is the last argument, or a chained `.set()` on a query builder. - const payload = - node.arguments.length >= 2 ? node.arguments.at(-1) : chainedSetPayload(node); - const entityArgs = node.arguments.filter((arg) => arg !== payload); - const targetsWorkflow = - genericIsWorkflowEntity || - onWorkflowRepoByName || - refersToWorkflow(callee.object) || - entityArgs.some((arg) => refersToWorkflow(arg)); - if (!targetsWorkflow) return; - - // A settings-/active-only update is out of the node seal's scope. A query builder with - // no `.set()` in reach is treated as a node write. - const verdict = payload ? payloadVerdict(payload, typeServices()) : 'nodes'; - if (verdict === 'nodes') context.report({ node, messageId: 'unsealedWrite' }); - if (verdict === 'opaque') context.report({ node, messageId: 'opaquePayload' }); }, }; }, diff --git a/packages/@n8n/oxlint-config/src/configs/base.ts b/packages/@n8n/oxlint-config/src/configs/base.ts index 1c95a0a1dad..027c73efdfb 100644 --- a/packages/@n8n/oxlint-config/src/configs/base.ts +++ b/packages/@n8n/oxlint-config/src/configs/base.ts @@ -203,6 +203,7 @@ export const baseConfig = defineConfig({ 'n8n-local-rules/no-restricted-sleep-definition': 'error', 'n8n-local-rules/no-restricted-sleep-import': 'error', 'n8n-local-rules/no-type-only-import-in-di': 'error', + 'n8n-local-rules/no-unsealed-workflow-entity-write': 'error', 'n8n-local-rules/no-unneeded-backticks': 'error', 'n8n-local-rules/no-unused-param-in-catch-clause': 'error', 'n8n-local-rules/no-useless-catch-throw': 'error', diff --git a/packages/cli/eslint.config.mjs b/packages/cli/eslint.config.mjs index cc6dc6102cf..f925c739187 100644 --- a/packages/cli/eslint.config.mjs +++ b/packages/cli/eslint.config.mjs @@ -45,8 +45,7 @@ export default defineConfig( rules: { 'n8n-local-rules/no-dynamic-import-template': 'error', 'n8n-local-rules/misplaced-n8n-typeorm-import': 'error', - // Ratchets: the allowlists below only shrink, so an inline disable is the one way to add a - // violation. `no-unsealed-workflow-entity-write` (on for every package via the plugin) has none. + // Ratchets: the allowlists below only shrink, so an inline disable is the one way to add a violation. 'n8n-local-rules/no-guardrail-disable': [ 'error', { diff --git a/packages/cli/src/commands/__tests__/execute-batch.test.ts b/packages/cli/src/commands/__tests__/execute-batch.test.ts index c0b8bfa12a1..4575f490dc8 100644 --- a/packages/cli/src/commands/__tests__/execute-batch.test.ts +++ b/packages/cli/src/commands/__tests__/execute-batch.test.ts @@ -59,8 +59,8 @@ mockInstance(AuthRolesService); mockInstance(BinaryDataRepository); const deploymentKeyRepository = mockInstance(DeploymentKeyRepository); -deploymentKeyRepository.findActiveByType.mockResolvedValue(null); -deploymentKeyRepository.insertOrIgnore.mockResolvedValue(undefined); +deploymentKeyRepository.findActiveIdentifier.mockResolvedValue(null); +deploymentKeyRepository.seedActiveIdentifier.mockResolvedValue(undefined); test('should start a task runner', async () => { // arrange diff --git a/packages/cli/src/commands/__tests__/execute.test.ts b/packages/cli/src/commands/__tests__/execute.test.ts index 6fa388b8483..f28a676fcc8 100644 --- a/packages/cli/src/commands/__tests__/execute.test.ts +++ b/packages/cli/src/commands/__tests__/execute.test.ts @@ -64,8 +64,8 @@ mockInstance(AuthRolesService); mockInstance(BinaryDataRepository); const deploymentKeyRepository = mockInstance(DeploymentKeyRepository); -deploymentKeyRepository.findActiveByType.mockResolvedValue(null); -deploymentKeyRepository.insertOrIgnore.mockResolvedValue(undefined); +deploymentKeyRepository.findActiveIdentifier.mockResolvedValue(null); +deploymentKeyRepository.seedActiveIdentifier.mockResolvedValue(undefined); // BaseCommand.init() wires the encryption key provider; this command-init test // does not exercise encryption, so keep the bootstrap a no-op. @@ -133,8 +133,10 @@ test('should start a task runner', async () => { test('should not seed the instance identity and should tolerate deployment key read errors', async () => { // arrange - deploymentKeyRepository.insertOrIgnore.mockClear(); - deploymentKeyRepository.findActiveByType.mockRejectedValueOnce(new Error('permission denied')); + deploymentKeyRepository.seedActiveIdentifier.mockClear(); + deploymentKeyRepository.findActiveIdentifier.mockRejectedValueOnce( + new Error('permission denied'), + ); const cmd = new ReadOnlyCommand(); @@ -144,7 +146,7 @@ test('should not seed the instance identity and should tolerate deployment key r // assert - expect(deploymentKeyRepository.insertOrIgnore).not.toHaveBeenCalled(); + expect(deploymentKeyRepository.seedActiveIdentifier).not.toHaveBeenCalled(); }); test('should not init the expression engine for commands that do not need it', async () => { diff --git a/packages/cli/src/commands/__tests__/start.test.ts b/packages/cli/src/commands/__tests__/start.test.ts index def3814ab7e..6f4f6fc1a9f 100644 --- a/packages/cli/src/commands/__tests__/start.test.ts +++ b/packages/cli/src/commands/__tests__/start.test.ts @@ -43,8 +43,8 @@ const roleCacheService = mockInstance(RoleCacheService); roleCacheService.refreshCache.mockResolvedValue(undefined); const deploymentKeyRepository = mockInstance(DeploymentKeyRepository); -deploymentKeyRepository.findActiveByType.mockResolvedValue(null); -deploymentKeyRepository.insertOrIgnore.mockResolvedValue(undefined); +deploymentKeyRepository.findActiveIdentifier.mockResolvedValue(null); +deploymentKeyRepository.seedActiveIdentifier.mockResolvedValue(undefined); const loadNodesAndCredentials = mockInstance(LoadNodesAndCredentials); loadNodesAndCredentials.init.mockResolvedValue(undefined); diff --git a/packages/cli/src/commands/__tests__/webhook.test.ts b/packages/cli/src/commands/__tests__/webhook.test.ts index 1445c332eef..ed269a7a993 100644 --- a/packages/cli/src/commands/__tests__/webhook.test.ts +++ b/packages/cli/src/commands/__tests__/webhook.test.ts @@ -26,8 +26,8 @@ dbConnection.init.mockResolvedValue(undefined); dbConnection.migrate.mockResolvedValue(undefined); const deploymentKeyRepository = mockInstance(DeploymentKeyRepository); -deploymentKeyRepository.findActiveByType.mockResolvedValue(null); -deploymentKeyRepository.insertOrIgnore.mockResolvedValue(undefined); +deploymentKeyRepository.findActiveIdentifier.mockResolvedValue(null); +deploymentKeyRepository.seedActiveIdentifier.mockResolvedValue(undefined); mockInstance(RedisClientService); mockInstance(PubSubRegistry); diff --git a/packages/cli/src/commands/__tests__/worker.test.ts b/packages/cli/src/commands/__tests__/worker.test.ts index f32c6afef2d..d3ecb52db33 100644 --- a/packages/cli/src/commands/__tests__/worker.test.ts +++ b/packages/cli/src/commands/__tests__/worker.test.ts @@ -44,8 +44,8 @@ dbConnection.init.mockResolvedValue(undefined); dbConnection.migrate.mockResolvedValue(undefined); const deploymentKeyRepository = mockInstance(DeploymentKeyRepository); -deploymentKeyRepository.findActiveByType.mockResolvedValue(null); -deploymentKeyRepository.insertOrIgnore.mockResolvedValue(undefined); +deploymentKeyRepository.findActiveIdentifier.mockResolvedValue(null); +deploymentKeyRepository.seedActiveIdentifier.mockResolvedValue(undefined); mockInstance(RedisClientService); mockInstance(PubSubRegistry); diff --git a/packages/cli/src/encryption/__tests__/encryption-bootstrap.service.integration.test.ts b/packages/cli/src/encryption/__tests__/encryption-bootstrap.service.integration.test.ts index 03794d51570..9b3613c6400 100644 --- a/packages/cli/src/encryption/__tests__/encryption-bootstrap.service.integration.test.ts +++ b/packages/cli/src/encryption/__tests__/encryption-bootstrap.service.integration.test.ts @@ -1,6 +1,7 @@ import { mockInstance, testDb } from '@n8n/backend-test-utils'; -import { DeploymentKeyRepository } from '@n8n/db'; +import { DeploymentKey, DeploymentKeyRepository } from '@n8n/db'; import { Container } from '@n8n/di'; +import { DataSource, type Repository } from '@n8n/typeorm'; import { Cipher, InstanceSettings } from 'n8n-core'; import { randomBytes } from 'node:crypto'; @@ -8,6 +9,7 @@ import { EncryptionBootstrapService } from '../encryption-bootstrap.service'; import { KeyManagerService } from '../key-manager.service'; const INSTANCE_ENCRYPTION_KEY = 'legacy-encryption-key'; +let keyStore: Repository; beforeAll(async () => { mockInstance(InstanceSettings, { @@ -17,10 +19,11 @@ beforeAll(async () => { canSeedDeploymentState: true, }); await testDb.init(); + keyStore = Container.get(DataSource).getRepository(DeploymentKey); }); beforeEach(async () => { - await testDb.truncate(['DeploymentKey']); + await testDb.resetDeploymentKeys(); }); afterAll(async () => { @@ -31,7 +34,7 @@ describe('EncryptionBootstrapService (integration)', () => { it('creates an inactive CBC key seeded from the instance encryption key', async () => { await Container.get(EncryptionBootstrapService).run(); - const rows = await Container.get(DeploymentKeyRepository).find({ + const rows = await keyStore.find({ where: { type: 'data_encryption', algorithm: 'aes-256-cbc' }, }); expect(rows).toHaveLength(1); @@ -45,7 +48,7 @@ describe('EncryptionBootstrapService (integration)', () => { it('creates an active GCM key', async () => { await Container.get(EncryptionBootstrapService).run(); - const rows = await Container.get(DeploymentKeyRepository).find({ + const rows = await keyStore.find({ where: { type: 'data_encryption', algorithm: 'aes-256-gcm', status: 'active' }, }); expect(rows).toHaveLength(1); @@ -55,7 +58,7 @@ describe('EncryptionBootstrapService (integration)', () => { await Container.get(EncryptionBootstrapService).run(); await Container.get(EncryptionBootstrapService).run(); - const all = await Container.get(DeploymentKeyRepository).find({ + const all = await keyStore.find({ where: { type: 'data_encryption' }, }); const cbcKeys = all.filter((k) => k.algorithm === 'aes-256-cbc'); @@ -69,7 +72,7 @@ describe('EncryptionBootstrapService (integration)', () => { await Promise.all([...Array(5)].map(async () => await service.run())); - const all = await Container.get(DeploymentKeyRepository).find({ + const all = await keyStore.find({ where: { type: 'data_encryption' }, }); const cbcKeys = all.filter((k) => k.algorithm === 'aes-256-cbc'); @@ -87,12 +90,27 @@ describe('EncryptionBootstrapService (integration)', () => { [...Array(5)].map(async (_, i) => await repository.seedLegacyCbcKey(`value-${i}`)), ); - const rows = await repository.find({ + const rows = await keyStore.find({ where: { type: 'data_encryption', algorithm: 'aes-256-cbc' }, }); expect(rows).toHaveLength(1); }); + it('keeps the active key when the activation target does not exist', async () => { + await Container.get(EncryptionBootstrapService).run(); + const repository = Container.get(DeploymentKeyRepository); + const activeBefore = await keyStore.findOne({ + where: { type: 'data_encryption', status: 'active' }, + }); + + await expect(repository.activateDataEncryptionKey('missing')).rejects.toThrow('not found'); + + const activeAfter = await keyStore.findOne({ + where: { type: 'data_encryption', status: 'active' }, + }); + expect(activeAfter?.id).toBe(activeBefore?.id); + }); + describe('end-to-end write path (real key store, real cipher)', () => { beforeEach(() => { delete process.env.N8N_ENV_FEAT_ENCRYPTION_KEY_ROTATION; @@ -117,8 +135,9 @@ describe('EncryptionBootstrapService (integration)', () => { const encrypted = await cipher.encryptV2('e2e-on'); - const active = - await Container.get(DeploymentKeyRepository).findActiveByType('data_encryption'); + const active = await keyStore.findOne({ + where: { type: 'data_encryption', status: 'active' }, + }); expect(encrypted.startsWith(`${active!.id}:`)).toBe(true); expect(await cipher.decryptV2(encrypted)).toBe('e2e-on'); } finally { @@ -129,14 +148,18 @@ describe('EncryptionBootstrapService (integration)', () => { describe('legacy DEK repair', () => { const seed = async (value: string) => { - const repo = Container.get(DeploymentKeyRepository); - await repo.save( - repo.create({ type: 'data_encryption', value, algorithm: 'aes-256-gcm', status: 'active' }), + await keyStore.save( + keyStore.create({ + type: 'data_encryption', + value, + algorithm: 'aes-256-gcm', + status: 'active', + }), ); }; const onlyRow = async () => { - const rows = await Container.get(DeploymentKeyRepository).find({ + const rows = await keyStore.find({ where: { type: 'data_encryption' }, }); expect(rows).toHaveLength(1); @@ -146,9 +169,8 @@ describe('EncryptionBootstrapService (integration)', () => { // Seeds an inactive row and returns it — the partial unique index allows // only one active row per type, so a multi-row test must stay inactive. const seedRow = async (value: string) => { - const repo = Container.get(DeploymentKeyRepository); - return await repo.save( - repo.create({ + return await keyStore.save( + keyStore.create({ type: 'data_encryption', value, algorithm: 'aes-256-gcm', @@ -178,8 +200,6 @@ describe('EncryptionBootstrapService (integration)', () => { it('repairs every legacy row in a mixed set and keeps going past a skipped row', async () => { const cipher = Container.get(Cipher); - const repo = Container.get(DeploymentKeyRepository); - const rawA = randomBytes(32).toString('hex'); const rawB = randomBytes(32).toString('hex'); const rawC = randomBytes(32).toString('hex'); @@ -190,7 +210,7 @@ describe('EncryptionBootstrapService (integration)', () => { await Container.get(KeyManagerService).repairLegacyDataEncryptionKeys(); - const rows = await repo.find({ where: { type: 'data_encryption' } }); + const rows = await keyStore.find({ where: { type: 'data_encryption' } }); const byId = (id: string) => rows.find((r) => r.id === id)!; // Both legacy rows are now GCM-readable and recover their exact keys. diff --git a/packages/cli/src/encryption/__tests__/get-key-by-id-type-scope.integration.test.ts b/packages/cli/src/encryption/__tests__/get-key-by-id-type-scope.integration.test.ts index fd71d8f08d1..098e04d0e98 100644 --- a/packages/cli/src/encryption/__tests__/get-key-by-id-type-scope.integration.test.ts +++ b/packages/cli/src/encryption/__tests__/get-key-by-id-type-scope.integration.test.ts @@ -1,10 +1,13 @@ import { mockInstance, testDb } from '@n8n/backend-test-utils'; -import { DeploymentKeyRepository } from '@n8n/db'; +import { DeploymentKey } from '@n8n/db'; import { Container } from '@n8n/di'; +import { DataSource, type Repository } from '@n8n/typeorm'; import { InstanceSettings } from 'n8n-core'; import { KeyManagerService } from '@/encryption/key-manager.service'; +let keyStore: Repository; + beforeAll(async () => { mockInstance(InstanceSettings, { encryptionKey: 'get-key-by-id-type-scope-instance-key', @@ -13,6 +16,7 @@ beforeAll(async () => { canSeedDeploymentState: true, }); await testDb.init(); + keyStore = Container.get(DataSource).getRepository(DeploymentKey); }); afterAll(async () => { @@ -23,9 +27,8 @@ afterAll(async () => { // lookup, even though `id` is the table's only primary key across all types. describe('getKeyById() type scoping (integration)', () => { it('does not resolve a jwe.private-key row', async () => { - const repo = Container.get(DeploymentKeyRepository); - const jweRow = await repo.save( - repo.create({ + const jweRow = await keyStore.save( + keyStore.create({ type: 'jwe.private-key', value: 'jwe-secret', algorithm: 'RSA-OAEP-256', @@ -39,9 +42,8 @@ describe('getKeyById() type scoping (integration)', () => { }); it('does not resolve a signing.hmac row', async () => { - const repo = Container.get(DeploymentKeyRepository); - const hmacRow = await repo.save( - repo.create({ + const hmacRow = await keyStore.save( + keyStore.create({ type: 'signing.hmac', value: 'hmac-secret', algorithm: null, diff --git a/packages/cli/src/encryption/__tests__/key-manager.service.test.ts b/packages/cli/src/encryption/__tests__/key-manager.service.test.ts index 32a0274c647..046815a5949 100644 --- a/packages/cli/src/encryption/__tests__/key-manager.service.test.ts +++ b/packages/cli/src/encryption/__tests__/key-manager.service.test.ts @@ -60,7 +60,7 @@ describe('KeyManagerService', () => { it('returns the active key as a prefixed descriptor', async () => { const { service, repo } = makeFreshService(); const key = makeKey(); - repo.find.mockResolvedValue([key]); + repo.findActiveDataEncryptionKeys.mockResolvedValue([key]); const result = await service.getActiveKey(); @@ -74,17 +74,20 @@ describe('KeyManagerService', () => { it('throws NotFoundError when no active key exists, without memoizing the absence', async () => { const { service, repo } = makeFreshService(); - repo.find.mockResolvedValue([]); + repo.findActiveDataEncryptionKeys.mockResolvedValue([]); await expect(service.getActiveKey()).rejects.toThrow(NotFoundError); // The absence is re-checked, not memoized. await expect(service.getActiveKey()).rejects.toThrow(NotFoundError); - expect(repo.find).toHaveBeenCalledTimes(2); + expect(repo.findActiveDataEncryptionKeys).toHaveBeenCalledTimes(2); }); it('throws when multiple active keys exist (invariant violation)', async () => { const { service, repo } = makeFreshService(); - repo.find.mockResolvedValue([makeKey({ id: 'key-1' }), makeKey({ id: 'key-2' })]); + repo.findActiveDataEncryptionKeys.mockResolvedValue([ + makeKey({ id: 'key-1' }), + makeKey({ id: 'key-2' }), + ]); await expect(service.getActiveKey()).rejects.toThrow( 'Encryption key invariant violated: multiple active keys found', @@ -114,7 +117,7 @@ describe('KeyManagerService', () => { format: 'no-prefix', }); expect(cipher.encryptDEKWithInstanceKey).toHaveBeenCalledWith('test-instance-key'); - expect(repository.find).not.toHaveBeenCalled(); + expect(repository.findActiveDataEncryptionKeys).not.toHaveBeenCalled(); }); it('memoizes the legacy descriptor', async () => { @@ -133,7 +136,7 @@ describe('KeyManagerService', () => { it('returns KeyInfo when key exists', async () => { const { service, repo } = makeFreshService(); const key = makeKey(); - repo.findOne.mockResolvedValue(key); + repo.findDataEncryptionKeyById.mockResolvedValue(key); const result = await service.getKeyById('key-1'); @@ -143,14 +146,12 @@ describe('KeyManagerService', () => { algorithm: key.algorithm, format: 'prefixed', }); - expect(repo.findOne).toHaveBeenCalledWith({ - where: { id: 'key-1', type: 'data_encryption' }, - }); + expect(repo.findDataEncryptionKeyById).toHaveBeenCalledWith('key-1'); }); it('returns null when key not found', async () => { const { service, repo } = makeFreshService(); - repo.findOne.mockResolvedValue(null); + repo.findDataEncryptionKeyById.mockResolvedValue(null); const result = await service.getKeyById('missing'); @@ -159,31 +160,28 @@ describe('KeyManagerService', () => { it('serves repeated lookups from the LRU without another database read', async () => { const { service, repo } = makeFreshService(); - repo.findOne.mockResolvedValue(makeKey()); + repo.findDataEncryptionKeyById.mockResolvedValue(makeKey()); const first = await service.getKeyById('key-1'); const second = await service.getKeyById('key-1'); - expect(repo.findOne).toHaveBeenCalledTimes(1); + expect(repo.findDataEncryptionKeyById).toHaveBeenCalledTimes(1); expect(second).toBe(first); }); it('does not cache misses, so an unknown id cannot evict real keys', async () => { const { service, repo } = makeFreshService(); - repo.findOne.mockResolvedValue(null); + repo.findDataEncryptionKeyById.mockResolvedValue(null); await service.getKeyById('missing'); await service.getKeyById('missing'); - expect(repo.findOne).toHaveBeenCalledTimes(2); + expect(repo.findDataEncryptionKeyById).toHaveBeenCalledTimes(2); }); it('caches exactly up to capacity without evicting', async () => { const { service, repo } = makeFreshService(); - repo.findOne.mockImplementation(async (opts) => { - const where = (opts as { where: { id: string } }).where; - return makeKey({ id: where.id }); - }); + repo.findDataEncryptionKeyById.mockImplementation(async (id) => makeKey({ id })); // Fill the LRU to exactly its capacity of 10 — no eviction yet. for (let i = 0; i < 10; i++) { @@ -192,31 +190,28 @@ describe('KeyManagerService', () => { for (let i = 0; i < 10; i++) { await service.getKeyById(`key-${i}`); } - expect(repo.findOne).toHaveBeenCalledTimes(10); + expect(repo.findDataEncryptionKeyById).toHaveBeenCalledTimes(10); }); it('evicts the LEAST RECENTLY USED entry beyond capacity, not the oldest inserted', async () => { const { service, repo } = makeFreshService(); - repo.findOne.mockImplementation(async (opts) => { - const where = (opts as { where: { id: string } }).where; - return makeKey({ id: where.id }); - }); + repo.findDataEncryptionKeyById.mockImplementation(async (id) => makeKey({ id })); for (let i = 0; i < 10; i++) { await service.getKeyById(`key-${i}`); } // Touch the oldest-inserted entry so key-1 becomes least recently used. await service.getKeyById('key-0'); - expect(repo.findOne).toHaveBeenCalledTimes(10); + expect(repo.findDataEncryptionKeyById).toHaveBeenCalledTimes(10); // The 11th distinct id evicts key-1 (LRU), not key-0 (refreshed). await service.getKeyById('key-10'); - expect(repo.findOne).toHaveBeenCalledTimes(11); + expect(repo.findDataEncryptionKeyById).toHaveBeenCalledTimes(11); await service.getKeyById('key-0'); - expect(repo.findOne).toHaveBeenCalledTimes(11); + expect(repo.findDataEncryptionKeyById).toHaveBeenCalledTimes(11); await service.getKeyById('key-1'); - expect(repo.findOne).toHaveBeenCalledTimes(12); + expect(repo.findDataEncryptionKeyById).toHaveBeenCalledTimes(12); }); }); @@ -233,18 +228,18 @@ describe('KeyManagerService', () => { vi.useFakeTimers(); try { const { service, repo } = makeFreshService(); - repo.find.mockResolvedValue([makeKey()]); + repo.findActiveDataEncryptionKeys.mockResolvedValue([makeKey()]); const first = await service.getActiveKey(); const second = await service.getActiveKey(); expect(second).toBe(first); - expect(repo.find).toHaveBeenCalledTimes(1); + expect(repo.findActiveDataEncryptionKeys).toHaveBeenCalledTimes(1); // Past the memo TTL the database view is re-read — this is also the // propagation window for a rotation done on another instance. vi.advanceTimersByTime(6000); await service.getActiveKey(); - expect(repo.find).toHaveBeenCalledTimes(2); + expect(repo.findActiveDataEncryptionKeys).toHaveBeenCalledTimes(2); } finally { vi.useRealTimers(); } @@ -255,7 +250,7 @@ describe('KeyManagerService', () => { it('returns the stored KeyInfo for the aes-256-cbc key when the store serves it', async () => { const { service, repo, cipherMock } = makeFreshService(); const key = makeKey({ algorithm: 'aes-256-cbc' }); - repo.findOne.mockResolvedValue(key); + repo.findDataEncryptionKeyByAlgorithm.mockResolvedValue(key); const result = await service.getLegacyKey(); @@ -265,26 +260,26 @@ describe('KeyManagerService', () => { algorithm: 'aes-256-cbc', format: 'no-prefix', }); - expect(repo.findOne).toHaveBeenCalledWith({ - where: { type: 'data_encryption', algorithm: 'aes-256-cbc' }, - }); + expect(repo.findDataEncryptionKeyByAlgorithm).toHaveBeenCalledWith('aes-256-cbc'); expect(cipherMock.encryptDEKWithInstanceKey).not.toHaveBeenCalled(); }); it('memoizes the stored legacy key — the row is immutable once seeded', async () => { const { service, repo } = makeFreshService(); - repo.findOne.mockResolvedValue(makeKey({ algorithm: 'aes-256-cbc' })); + repo.findDataEncryptionKeyByAlgorithm.mockResolvedValue( + makeKey({ algorithm: 'aes-256-cbc' }), + ); const first = await service.getLegacyKey(); const second = await service.getLegacyKey(); - expect(repo.findOne).toHaveBeenCalledTimes(1); + expect(repo.findDataEncryptionKeyByAlgorithm).toHaveBeenCalledTimes(1); expect(second).toBe(first); }); it('falls back to the instance key when the legacy key is not seeded', async () => { const { service, repo, cipherMock } = makeFreshService(); - repo.findOne.mockResolvedValue(null); + repo.findDataEncryptionKeyByAlgorithm.mockResolvedValue(null); cipherMock.encryptDEKWithInstanceKey.mockReturnValue('wrapped-instance-key'); const result = await service.getLegacyKey(); @@ -296,7 +291,7 @@ describe('KeyManagerService', () => { it('falls back to the instance key when the store lookup fails', async () => { const { service, repo, cipherMock } = makeFreshService(); - repo.findOne.mockRejectedValue(new Error('connection refused')); + repo.findDataEncryptionKeyByAlgorithm.mockRejectedValue(new Error('connection refused')); cipherMock.encryptDEKWithInstanceKey.mockReturnValue('wrapped-instance-key'); const result = await service.getLegacyKey(); @@ -310,7 +305,7 @@ describe('KeyManagerService', () => { // instance key exactly as bootstrap would. it('wraps the instance key only once and reuses the result', async () => { const { service, repo, cipherMock } = makeFreshService(); - repo.findOne.mockResolvedValue(null); + repo.findDataEncryptionKeyByAlgorithm.mockResolvedValue(null); cipherMock.encryptDEKWithInstanceKey.mockReturnValue('wrapped-instance-key'); const first = await service.getLegacyKey(); @@ -330,20 +325,18 @@ describe('KeyManagerService', () => { describe('bootstrapLegacyCbcKey()', () => { it('is a no-op when a CBC key already exists', async () => { - repository.findOne.mockResolvedValue( + repository.findDataEncryptionKeyByAlgorithm.mockResolvedValue( makeKey({ algorithm: 'aes-256-cbc', status: 'inactive' }), ); await Container.get(KeyManagerService).bootstrapLegacyCbcKey('instance-key'); - expect(repository.findOne).toHaveBeenCalledWith({ - where: { type: 'data_encryption', algorithm: 'aes-256-cbc' }, - }); + expect(repository.findDataEncryptionKeyByAlgorithm).toHaveBeenCalledWith('aes-256-cbc'); expect(repository.seedLegacyCbcKey).not.toHaveBeenCalled(); }); it('encrypts the instance key and seeds it when no CBC key exists', async () => { - repository.findOne.mockResolvedValue(null); + repository.findDataEncryptionKeyByAlgorithm.mockResolvedValue(null); cipher.encryptDEKWithInstanceKey.mockReturnValue('encrypted-instance-key'); await Container.get(KeyManagerService).bootstrapLegacyCbcKey('instance-key'); @@ -352,24 +345,24 @@ describe('KeyManagerService', () => { // The repository seeds inside a DbLock critical section, which keeps // concurrent startups from creating duplicate rows. expect(repository.seedLegacyCbcKey).toHaveBeenCalledWith('encrypted-instance-key'); - expect(repository.save).not.toHaveBeenCalled(); + expect(repository.insertInactiveDataEncryptionKey).not.toHaveBeenCalled(); }); }); describe('bootstrapGcmKey()', () => { it('is a no-op when an active GCM key already exists', async () => { - repository.findOne.mockResolvedValue(makeKey({ algorithm: 'aes-256-gcm', status: 'active' })); + repository.findActiveDataEncryptionKeyByAlgorithm.mockResolvedValue( + makeKey({ algorithm: 'aes-256-gcm', status: 'active' }), + ); await Container.get(KeyManagerService).bootstrapGcmKey(); - expect(repository.findOne).toHaveBeenCalledWith({ - where: { type: 'data_encryption', algorithm: 'aes-256-gcm', status: 'active' }, - }); - expect(repository.insertAsActive).not.toHaveBeenCalled(); + expect(repository.findActiveDataEncryptionKeyByAlgorithm).toHaveBeenCalledWith('aes-256-gcm'); + expect(repository.seedActiveDataEncryptionKey).not.toHaveBeenCalled(); }); it('generates a 64-char hex key and inserts as active when no active GCM key exists', async () => { - repository.findOne.mockResolvedValue(null); + repository.findActiveDataEncryptionKeyByAlgorithm.mockResolvedValue(null); cipher.encryptDEKWithInstanceKey.mockReturnValue('encrypted-gcm-key'); await Container.get(KeyManagerService).bootstrapGcmKey(); @@ -378,54 +371,43 @@ describe('KeyManagerService', () => { const [rawKey] = cipher.encryptDEKWithInstanceKey.mock.calls[0]; expect(typeof rawKey).toBe('string'); expect(rawKey).toHaveLength(64); - expect(repository.insertOrIgnore).toHaveBeenCalledWith({ - type: 'data_encryption', - value: 'encrypted-gcm-key', - algorithm: 'aes-256-gcm', - status: 'active', - }); + expect(repository.seedActiveDataEncryptionKey).toHaveBeenCalledWith( + 'encrypted-gcm-key', + 'aes-256-gcm', + ); }); }); describe('addKey()', () => { it('encrypts the value and inserts as inactive when setAsActive is not set', async () => { const saved = makeKey({ id: 'new-key', status: 'inactive' }); - repository.create.mockReturnValue(saved); - repository.save.mockResolvedValue(saved); + repository.insertInactiveDataEncryptionKey.mockResolvedValue(saved); cipher.encryptDEKWithInstanceKey.mockReturnValue('encrypted-base64'); const result = await Container.get(KeyManagerService).addKey('secret', 'aes-256-gcm'); expect(cipher.encryptDEKWithInstanceKey).toHaveBeenCalledWith('secret'); - expect(repository.insertAsActive).not.toHaveBeenCalled(); - expect(repository.create).toHaveBeenCalledWith( - expect.objectContaining({ - status: 'inactive', - type: 'data_encryption', - value: 'encrypted-base64', - }), + expect(repository.insertAndActivateDataEncryptionKey).not.toHaveBeenCalled(); + expect(repository.insertInactiveDataEncryptionKey).toHaveBeenCalledWith( + 'encrypted-base64', + 'aes-256-gcm', ); expect(result).toBe(saved); }); - it('encrypts the value and delegates to insertAsActive when setAsActive=true', async () => { + it('encrypts the value and inserts it as active when setAsActive=true', async () => { const saved = makeKey({ id: 'new-key', status: 'active' }); - repository.create.mockReturnValue(saved); - repository.insertAsActive.mockResolvedValue(saved); + repository.insertAndActivateDataEncryptionKey.mockResolvedValue(saved); cipher.encryptDEKWithInstanceKey.mockReturnValue('encrypted-base64'); const result = await Container.get(KeyManagerService).addKey('secret', 'aes-256-gcm', true); expect(cipher.encryptDEKWithInstanceKey).toHaveBeenCalledWith('secret'); - expect(repository.save).not.toHaveBeenCalled(); - expect(repository.create).toHaveBeenCalledWith( - expect.objectContaining({ - type: 'data_encryption', - value: 'encrypted-base64', - algorithm: 'aes-256-gcm', - }), + expect(repository.insertInactiveDataEncryptionKey).not.toHaveBeenCalled(); + expect(repository.insertAndActivateDataEncryptionKey).toHaveBeenCalledWith( + 'encrypted-base64', + 'aes-256-gcm', ); - expect(repository.insertAsActive).toHaveBeenCalledWith(saved); expect(result).toBe(saved); }); @@ -441,8 +423,7 @@ describe('KeyManagerService', () => { it('switches the very next write to the new key after the insert commits', async () => { const { service, repo, cipherMock } = makeFreshService(); const saved = makeKey({ id: 'new-key', status: 'active' }); - repo.create.mockReturnValue(saved); - repo.insertAsActive.mockResolvedValue(saved); + repo.insertAndActivateDataEncryptionKey.mockResolvedValue(saved); cipherMock.encryptDEKWithInstanceKey.mockReturnValue('encrypted-base64'); await service.addKey('secret', 'aes-256-gcm', true); @@ -450,19 +431,18 @@ describe('KeyManagerService', () => { expect(active.id).toBe('new-key'); // Served from the memo the commit refreshed — no database read. - expect(repo.find).not.toHaveBeenCalled(); + expect(repo.findActiveDataEncryptionKeys).not.toHaveBeenCalled(); }); it('does not update the memo when the insert fails', async () => { const { service, repo, cipherMock } = makeFreshService(); - repo.create.mockReturnValue(makeKey({ id: 'new-key', status: 'active' })); - repo.insertAsActive.mockRejectedValue(new Error('db down')); + repo.insertAndActivateDataEncryptionKey.mockRejectedValue(new Error('db down')); cipherMock.encryptDEKWithInstanceKey.mockReturnValue('encrypted-base64'); await expect(service.addKey('secret', 'aes-256-gcm', true)).rejects.toThrow('db down'); // The next write must re-read the store, not trust a failed rotation. - repo.find.mockResolvedValue([makeKey({ id: 'still-active' })]); + repo.findActiveDataEncryptionKeys.mockResolvedValue([makeKey({ id: 'still-active' })]); const active = await service.getActiveKey(); expect(active.id).toBe('still-active'); }); @@ -470,13 +450,12 @@ describe('KeyManagerService', () => { it('does not touch the memo for an inactive key insert', async () => { const { service, repo, cipherMock } = makeFreshService(); const saved = makeKey({ id: 'spare-key', status: 'inactive' }); - repo.create.mockReturnValue(saved); - repo.save.mockResolvedValue(saved); + repo.insertInactiveDataEncryptionKey.mockResolvedValue(saved); cipherMock.encryptDEKWithInstanceKey.mockReturnValue('encrypted-base64'); await service.addKey('secret', 'aes-256-gcm'); - repo.find.mockResolvedValue([makeKey({ id: 'the-active-one' })]); + repo.findActiveDataEncryptionKeys.mockResolvedValue([makeKey({ id: 'the-active-one' })]); const active = await service.getActiveKey(); expect(active.id).toBe('the-active-one'); }); @@ -486,8 +465,7 @@ describe('KeyManagerService', () => { describe('rotateKey()', () => { it('generates a 64-char hex key and inserts it as active with aes-256-gcm', async () => { const saved = makeKey({ id: 'rotated', status: 'active', algorithm: 'aes-256-gcm' }); - repository.create.mockReturnValue(saved); - repository.insertAsActive.mockResolvedValue(saved); + repository.insertAndActivateDataEncryptionKey.mockResolvedValue(saved); cipher.encryptDEKWithInstanceKey.mockReturnValue('encrypted-base64'); const result = await Container.get(KeyManagerService).rotateKey(); @@ -497,21 +475,16 @@ describe('KeyManagerService', () => { expect(typeof rawKey).toBe('string'); expect(rawKey.length).toBe(64); - expect(repository.create).toHaveBeenCalledWith( - expect.objectContaining({ - type: 'data_encryption', - value: 'encrypted-base64', - algorithm: 'aes-256-gcm', - }), + expect(repository.insertAndActivateDataEncryptionKey).toHaveBeenCalledWith( + 'encrypted-base64', + 'aes-256-gcm', ); - expect(repository.insertAsActive).toHaveBeenCalledWith(saved); expect(result).toBe(saved); }); it('generates a fresh key value on each call', async () => { const saved = makeKey(); - repository.create.mockReturnValue(saved); - repository.insertAsActive.mockResolvedValue(saved); + repository.insertAndActivateDataEncryptionKey.mockResolvedValue(saved); cipher.encryptDEKWithInstanceKey.mockImplementation( (data: string | object) => `enc:${String(data)}`, ); @@ -610,33 +583,33 @@ describe('KeyManagerService', () => { delete process.env.N8N_ENV_FEAT_ENCRYPTION_KEY_ROTATION; }); - it('delegates to promoteToActive and invalidates the memo', async () => { + it('activates the key and invalidates the memo', async () => { const { service, repo } = makeFreshService(); - repo.find.mockResolvedValue([makeKey({ id: 'old-active' })]); + repo.findActiveDataEncryptionKeys.mockResolvedValue([makeKey({ id: 'old-active' })]); await service.getActiveKey(); // primes the memo - repo.promoteToActive.mockResolvedValue(undefined); + repo.activateDataEncryptionKey.mockResolvedValue(undefined); await service.setActiveKey('target'); - expect(repo.promoteToActive).toHaveBeenCalledWith('target', 'data_encryption'); + expect(repo.activateDataEncryptionKey).toHaveBeenCalledWith('target'); // The memo was dropped: the next write re-reads the store. - repo.find.mockResolvedValue([makeKey({ id: 'target' })]); + repo.findActiveDataEncryptionKeys.mockResolvedValue([makeKey({ id: 'target' })]); const active = await service.getActiveKey(); expect(active.id).toBe('target'); - expect(repo.find).toHaveBeenCalledTimes(2); + expect(repo.findActiveDataEncryptionKeys).toHaveBeenCalledTimes(2); }); it('keeps the memo when the promotion fails', async () => { const { service, repo } = makeFreshService(); - repo.find.mockResolvedValue([makeKey({ id: 'old-active' })]); + repo.findActiveDataEncryptionKeys.mockResolvedValue([makeKey({ id: 'old-active' })]); await service.getActiveKey(); - repo.promoteToActive.mockRejectedValue(new Error('not found')); + repo.activateDataEncryptionKey.mockRejectedValue(new Error('not found')); await expect(service.setActiveKey('ghost')).rejects.toThrow('not found'); const active = await service.getActiveKey(); expect(active.id).toBe('old-active'); - expect(repo.find).toHaveBeenCalledTimes(1); + expect(repo.findActiveDataEncryptionKeys).toHaveBeenCalledTimes(1); }); }); @@ -651,41 +624,37 @@ describe('KeyManagerService', () => { it('sets status to inactive and invalidates the memo', async () => { const { service, repo } = makeFreshService(); - repo.find.mockResolvedValue([makeKey({ id: 'old-active' })]); + repo.findActiveDataEncryptionKeys.mockResolvedValue([makeKey({ id: 'old-active' })]); await service.getActiveKey(); // primes the memo await service.markInactive('old-active'); - expect(repo.update).toHaveBeenCalledWith('old-active', { status: 'inactive' }); + expect(repo.deactivateDataEncryptionKey).toHaveBeenCalledWith('old-active'); // The memo was dropped: the next write re-reads the store. - repo.find.mockResolvedValue([]); + repo.findActiveDataEncryptionKeys.mockResolvedValue([]); await expect(service.getActiveKey()).rejects.toThrow(NotFoundError); }); it('keeps the memo when the status update fails', async () => { const { service, repo } = makeFreshService(); - repo.find.mockResolvedValue([makeKey({ id: 'old-active' })]); + repo.findActiveDataEncryptionKeys.mockResolvedValue([makeKey({ id: 'old-active' })]); await service.getActiveKey(); - repo.update.mockRejectedValue(new Error('db down')); + repo.deactivateDataEncryptionKey.mockRejectedValue(new Error('db down')); await expect(service.markInactive('old-active')).rejects.toThrow('db down'); const active = await service.getActiveKey(); expect(active.id).toBe('old-active'); - expect(repo.find).toHaveBeenCalledTimes(1); + expect(repo.findActiveDataEncryptionKeys).toHaveBeenCalledTimes(1); }); it('never deletes the key row', async () => { const { service, repo } = makeFreshService(); - repo.find.mockResolvedValue([makeKey({ id: 'old-active' })]); + repo.findActiveDataEncryptionKeys.mockResolvedValue([makeKey({ id: 'old-active' })]); await service.markInactive('old-active'); - expect(repo.delete).not.toHaveBeenCalled(); - expect(repo.remove).not.toHaveBeenCalled(); - expect(repo.softDelete).not.toHaveBeenCalled(); - expect(repo.softRemove).not.toHaveBeenCalled(); - expect(repo.update).toHaveBeenCalledWith('old-active', { status: 'inactive' }); + expect(repo.deactivateDataEncryptionKey).toHaveBeenCalledWith('old-active'); }); }); diff --git a/packages/cli/src/encryption/key-manager.service.ts b/packages/cli/src/encryption/key-manager.service.ts index 5f5764e1802..f46f51ff3b2 100644 --- a/packages/cli/src/encryption/key-manager.service.ts +++ b/packages/cli/src/encryption/key-manager.service.ts @@ -82,9 +82,7 @@ export class KeyManagerService implements IEncryptionKeyProvider { return this.activeKeyMemo.info; } - const activeKeys = await this.deploymentKeyRepository.find({ - where: { type: 'data_encryption', status: 'active' }, - }); + const activeKeys = await this.deploymentKeyRepository.findActiveDataEncryptionKeys(); if (activeKeys.length === 0) { throw new NotFoundError('No active encryption key found'); } @@ -116,9 +114,7 @@ export class KeyManagerService implements IEncryptionKeyProvider { return cached; } - const key = await this.deploymentKeyRepository.findOne({ - where: { id, type: 'data_encryption' }, - }); + const key = await this.deploymentKeyRepository.findDataEncryptionKeyById(id); if (!key) return null; const keyInfo: KeyInfo = { id: key.id, @@ -217,9 +213,8 @@ export class KeyManagerService implements IEncryptionKeyProvider { if (this.cachedStoredLegacyKey) return this.cachedStoredLegacyKey; try { - const key = await this.deploymentKeyRepository.findOne({ - where: { type: 'data_encryption', algorithm: 'aes-256-cbc' }, - }); + const key = + await this.deploymentKeyRepository.findDataEncryptionKeyByAlgorithm('aes-256-cbc'); if (key) { this.cachedStoredLegacyKey = { id: key.id, @@ -251,9 +246,8 @@ export class KeyManagerService implements IEncryptionKeyProvider { * skip the lock on every startup after the first. */ async bootstrapLegacyCbcKey(instanceEncryptionKey: string): Promise { - const existing = await this.deploymentKeyRepository.findOne({ - where: { type: 'data_encryption', algorithm: 'aes-256-cbc' }, - }); + const existing = + await this.deploymentKeyRepository.findDataEncryptionKeyByAlgorithm('aes-256-cbc'); if (existing) return; const encryptedValue = this.cipher.encryptDEKWithInstanceKey(instanceEncryptionKey); @@ -282,19 +276,13 @@ export class KeyManagerService implements IEncryptionKeyProvider { * (type, status='active') serializes inserts, and losers are silently ignored. */ async bootstrapGcmKey(): Promise { - const existing = await this.deploymentKeyRepository.findOne({ - where: { type: 'data_encryption', algorithm: 'aes-256-gcm', status: 'active' }, - }); + const existing = + await this.deploymentKeyRepository.findActiveDataEncryptionKeyByAlgorithm('aes-256-gcm'); if (existing) return; const rawKey = randomBytes(32).toString('hex'); const encryptedValue = this.cipher.encryptDEKWithInstanceKey(rawKey); - await this.deploymentKeyRepository.insertOrIgnore({ - type: 'data_encryption', - value: encryptedValue, - algorithm: 'aes-256-gcm', - status: 'active', - }); + await this.deploymentKeyRepository.seedActiveDataEncryptionKey(encryptedValue, 'aes-256-gcm'); } /** @@ -347,24 +335,16 @@ export class KeyManagerService implements IEncryptionKeyProvider { const encryptedValue = this.cipher.encryptDEKWithInstanceKey(plaintextValue); if (!setAsActive) { - const entity = this.deploymentKeyRepository.create({ - type: 'data_encryption', - value: encryptedValue, + return await this.deploymentKeyRepository.insertInactiveDataEncryptionKey( + encryptedValue, algorithm, - status: 'inactive', - }); - return await this.deploymentKeyRepository.save(entity); + ); } - const entity = Object.assign( - this.deploymentKeyRepository.create({ - type: 'data_encryption', - value: encryptedValue, - algorithm, - }), - { status: 'active' as const }, + const saved = await this.deploymentKeyRepository.insertAndActivateDataEncryptionKey( + encryptedValue, + algorithm, ); - const saved = await this.deploymentKeyRepository.insertAsActive(entity); // Update the memo only after the commit — otherwise this instance could // encrypt with a key that never landed in the database. Local writes // switch immediately; other instances follow within the memo window. @@ -381,17 +361,16 @@ export class KeyManagerService implements IEncryptionKeyProvider { /** Atomically deactivates the current active key and promotes the given key. */ async setActiveKey(id: string): Promise { - await this.deploymentKeyRepository.promoteToActive(id, 'data_encryption'); + await this.deploymentKeyRepository.activateDataEncryptionKey(id); this.activeKeyMemo = undefined; } /** - * Transitions key to 'inactive'. Never deletes: DeploymentKeyRepository's - * delete/remove/softDelete/softRemove/clear all throw, so a deactivated - * key's value stays intact and readable for any ciphertext still using it. + * Transitions a key to inactive. The repository exposes no delete operation, + * and the database rejects direct deletion. */ async markInactive(id: string): Promise { - await this.deploymentKeyRepository.update(id, { status: 'inactive' }); + await this.deploymentKeyRepository.deactivateDataEncryptionKey(id); // The active key may be gone now: force the next write to re-read the store. this.activeKeyMemo = undefined; } diff --git a/packages/cli/src/modules/oauth-jwe/__tests__/oauth-jwe-key.service.integration.test.ts b/packages/cli/src/modules/oauth-jwe/__tests__/oauth-jwe-key.service.integration.test.ts index 85b1ca154f8..1eef06ede58 100644 --- a/packages/cli/src/modules/oauth-jwe/__tests__/oauth-jwe-key.service.integration.test.ts +++ b/packages/cli/src/modules/oauth-jwe/__tests__/oauth-jwe-key.service.integration.test.ts @@ -1,6 +1,7 @@ import { mockInstance, testDb } from '@n8n/backend-test-utils'; -import { DeploymentKeyRepository } from '@n8n/db'; +import { DeploymentKey } from '@n8n/db'; import { Container } from '@n8n/di'; +import { DataSource, type Repository } from '@n8n/typeorm'; import type { CryptoKey } from 'jose'; import { CompactEncrypt, compactDecrypt, exportJWK, generateKeyPair, importJWK } from 'jose'; import { Cipher, InstanceSettings } from 'n8n-core'; @@ -14,16 +15,19 @@ import { JWE_PRIVATE_KEY_TYPE, } from '../oauth-jwe.constants'; +let keyStore: Repository; + beforeAll(async () => { mockInstance(InstanceSettings, { encryptionKey: 'oauth-jwe-test-encryption-key', n8nFolder: '/tmp/n8n-test', }); await testDb.init(); + keyStore = Container.get(DataSource).getRepository(DeploymentKey); }); beforeEach(async () => { - await testDb.truncate(['DeploymentKey']); + await testDb.resetDeploymentKeys(); await Container.get(CacheService).delete(JWE_KEY_CACHE_KEY); }); @@ -35,7 +39,7 @@ describe('OAuthJweKeyService (integration)', () => { it('persists exactly one active private-key row per algorithm on first initialize', async () => { await Container.get(OAuthJweKeyService).initialize(); - const rows = await Container.get(DeploymentKeyRepository).find({ + const rows = await keyStore.find({ where: { type: JWE_PRIVATE_KEY_TYPE, status: 'active' }, }); @@ -56,7 +60,7 @@ describe('OAuthJweKeyService (integration)', () => { await Container.get(CacheService).delete(JWE_KEY_CACHE_KEY); await service.initialize(); - const rows = await Container.get(DeploymentKeyRepository).find({ + const rows = await keyStore.find({ where: { type: JWE_PRIVATE_KEY_TYPE, status: 'active' }, }); expect(rows).toHaveLength(JWE_KEY_ALGORITHMS.length); @@ -80,7 +84,7 @@ describe('OAuthJweKeyService (integration)', () => { it('stores new private JWKs in the DEK-style wrap', async () => { await Container.get(OAuthJweKeyService).initialize(); - const rows = await Container.get(DeploymentKeyRepository).find({ + const rows = await keyStore.find({ where: { type: JWE_PRIVATE_KEY_TYPE, status: 'active' }, }); const cipher = Container.get(Cipher); @@ -94,7 +98,6 @@ describe('OAuthJweKeyService (integration)', () => { it('reads a row in the earlier wrap format and leaves its stored wrap in place', async () => { const algorithm = JWE_KEY_ALGORITHMS[0]; const cipher = Container.get(Cipher); - const repository = Container.get(DeploymentKeyRepository); // A row as earlier releases persisted it. const id = 'legacy-format-row-1'; @@ -102,7 +105,7 @@ describe('OAuthJweKeyService (integration)', () => { const privateJwk = { ...(await exportJWK(privateKey)), kid: id, alg: algorithm, use: 'enc' }; const legacyValue = cipher.encryptWithInstanceKey(JSON.stringify(privateJwk)); expect(legacyValue.startsWith('U2FsdGVkX1')).toBe(true); - await repository.insert({ + await keyStore.insert({ id, type: JWE_PRIVATE_KEY_TYPE, value: legacyValue, @@ -119,13 +122,13 @@ describe('OAuthJweKeyService (integration)', () => { // The stored value keeps its wrap byte for byte: older instances in a // rolling deployment can only read this format, and the cache is shared. - const row = await repository.findOne({ where: { id } }); + const row = await keyStore.findOne({ where: { id } }); expect(row!.value).toBe(legacyValue); // A cold re-read leaves it untouched too. await Container.get(CacheService).delete(JWE_KEY_CACHE_KEY); await service.initialize(); - const rowAfter = await repository.findOne({ where: { id } }); + const rowAfter = await keyStore.findOne({ where: { id } }); expect(rowAfter!.value).toBe(legacyValue); }); diff --git a/packages/cli/src/modules/oauth-jwe/__tests__/oauth-jwe-key.service.test.ts b/packages/cli/src/modules/oauth-jwe/__tests__/oauth-jwe-key.service.test.ts index 7ebb4fcb554..bff1e26f2d8 100644 --- a/packages/cli/src/modules/oauth-jwe/__tests__/oauth-jwe-key.service.test.ts +++ b/packages/cli/src/modules/oauth-jwe/__tests__/oauth-jwe-key.service.test.ts @@ -64,44 +64,40 @@ describe('OAuthJweKeyService', () => { describe('initialize / loadOrGenerate', () => { it('reuses an existing active row without inserting', async () => { - repository.findOne.mockResolvedValue(makeRow({ value: JSON.stringify(privateJwkFixture) })); + repository.findActiveOAuthJweKey.mockResolvedValue( + makeRow({ value: JSON.stringify(privateJwkFixture) }), + ); cipher.decryptDEKWithInstanceKey.mockReturnValue(JSON.stringify(privateJwkFixture)); await Container.get(OAuthJweKeyService).initialize(); - expect(repository.insert).not.toHaveBeenCalled(); + expect(repository.insertActiveOAuthJweKey).not.toHaveBeenCalled(); }); it('generates and persists a new key pair when none exists', async () => { let row: DeploymentKey | null = null; - repository.findOne.mockImplementation(async () => row); - repository.insert.mockImplementation(async (entity) => { + repository.findActiveOAuthJweKey.mockImplementation(async () => row); + repository.insertActiveOAuthJweKey.mockImplementation(async (id, value, algorithm) => { row = makeRow({ - value: (entity as DeploymentKey).value, - algorithm: (entity as DeploymentKey).algorithm, + id, + value, + algorithm, }); - cipher.decryptDEKWithInstanceKey.mockImplementation((value: string) => - value === row?.value ? JSON.stringify(privateJwkFixture) : '', - ); - return { identifiers: [{ id: 'row-1' }], generatedMaps: [], raw: [] }; }); await Container.get(OAuthJweKeyService).initialize(); - expect(repository.insert).toHaveBeenCalledTimes(1); - const [inserted] = repository.insert.mock.calls[0]; - expect(inserted).toMatchObject({ - type: JWE_PRIVATE_KEY_TYPE, - algorithm: ALGORITHM, - status: 'active', - }); - expect((inserted as DeploymentKey).id).toEqual(expect.any(String)); + expect(repository.insertActiveOAuthJweKey).toHaveBeenCalledTimes(1); + const [id, value, algorithm] = repository.insertActiveOAuthJweKey.mock.calls[0]; + expect(id).toEqual(expect.any(String)); + expect(value).toEqual(expect.any(String)); + expect(algorithm).toBe(ALGORITHM); expect(cipher.encryptDEKWithInstanceKey).toHaveBeenCalledTimes(1); }); it('throws when the persisted private JWK has no kid', async () => { const { kid: _kid, ...kidless } = privateJwkFixture; - repository.findOne.mockResolvedValue(makeRow({ value: 'enc-row' })); + repository.findActiveOAuthJweKey.mockResolvedValue(makeRow({ value: 'enc-row' })); cipher.decryptDEKWithInstanceKey.mockReturnValue(JSON.stringify(kidless)); await expect(Container.get(OAuthJweKeyService).initialize()).rejects.toThrow( @@ -110,7 +106,9 @@ describe('OAuthJweKeyService', () => { }); it('throws when the persisted JWK kid does not match the row id', async () => { - repository.findOne.mockResolvedValue(makeRow({ id: 'different-id', value: 'enc-row' })); + repository.findActiveOAuthJweKey.mockResolvedValue( + makeRow({ id: 'different-id', value: 'enc-row' }), + ); cipher.decryptDEKWithInstanceKey.mockReturnValue(JSON.stringify(privateJwkFixture)); await expect(Container.get(OAuthJweKeyService).initialize()).rejects.toThrow( @@ -119,8 +117,8 @@ describe('OAuthJweKeyService', () => { }); it('throws when the post-generate re-read returns null', async () => { - repository.findOne.mockResolvedValue(null); - repository.insert.mockResolvedValue({ identifiers: [], generatedMaps: [], raw: [] }); + repository.findActiveOAuthJweKey.mockResolvedValue(null); + repository.insertActiveOAuthJweKey.mockResolvedValue(undefined); await expect(Container.get(OAuthJweKeyService).initialize()).rejects.toThrow( `OAuth JWE key for algorithm "${ALGORITHM}" not found after generation`, @@ -131,8 +129,8 @@ describe('OAuthJweKeyService', () => { describe('race handling', () => { it('swallows a postgres unique-constraint violation and re-reads the winner', async () => { let row: DeploymentKey | null = null; - repository.findOne.mockImplementation(async () => row); - repository.insert.mockImplementation(async () => { + repository.findActiveOAuthJweKey.mockImplementation(async () => row); + repository.insertActiveOAuthJweKey.mockImplementation(async () => { row = makeRow({ value: 'winner-row' }); cipher.decryptDEKWithInstanceKey.mockImplementation((value: string) => value === 'winner-row' ? JSON.stringify(privateJwkFixture) : '', @@ -145,8 +143,8 @@ describe('OAuthJweKeyService', () => { it('swallows a sqlite unique-constraint violation', async () => { let row: DeploymentKey | null = null; - repository.findOne.mockImplementation(async () => row); - repository.insert.mockImplementation(async () => { + repository.findActiveOAuthJweKey.mockImplementation(async () => row); + repository.insertActiveOAuthJweKey.mockImplementation(async () => { row = makeRow({ value: 'winner-row' }); cipher.decryptDEKWithInstanceKey.mockImplementation((value: string) => value === 'winner-row' ? JSON.stringify(privateJwkFixture) : '', @@ -158,9 +156,9 @@ describe('OAuthJweKeyService', () => { }); it('re-throws non-unique-constraint errors unchanged', async () => { - repository.findOne.mockResolvedValue(null); + repository.findActiveOAuthJweKey.mockResolvedValue(null); const connectionError = makeUniqueViolation('ECONNREFUSED'); - repository.insert.mockRejectedValue(connectionError); + repository.insertActiveOAuthJweKey.mockRejectedValue(connectionError); await expect(Container.get(OAuthJweKeyService).initialize()).rejects.toBe(connectionError); }); @@ -168,7 +166,7 @@ describe('OAuthJweKeyService', () => { describe('getKeyPair / getPublicJwk', () => { beforeEach(() => { - repository.findOne.mockResolvedValue(makeRow({ value: 'enc-active' })); + repository.findActiveOAuthJweKey.mockResolvedValue(makeRow({ value: 'enc-active' })); cipher.decryptDEKWithInstanceKey.mockReturnValue(JSON.stringify(privateJwkFixture)); }); @@ -221,12 +219,12 @@ describe('OAuthJweKeyService', () => { const legacyValue = 'U2FsdGVkX1-legacy-wrapped'; beforeEach(() => { - repository.findOne.mockResolvedValue(makeRow({ value: legacyValue })); + repository.findActiveOAuthJweKey.mockResolvedValue(makeRow({ value: legacyValue })); cipher.decryptWithInstanceKey.mockReturnValue(JSON.stringify(privateJwkFixture)); }); it('reads a row in the earlier wrap format and leaves the stored value untouched', async () => { - repository.findOne.mockResolvedValue(makeRow({ value: legacyValue })); + repository.findActiveOAuthJweKey.mockResolvedValue(makeRow({ value: legacyValue })); const pair = await Container.get(OAuthJweKeyService).getKeyPair(); @@ -234,15 +232,15 @@ describe('OAuthJweKeyService', () => { expect(cipher.decryptWithInstanceKey).toHaveBeenCalledWith(legacyValue); // Older instances in a rolling deployment can only read this format, // and the cache is shared — the stored row must not be rewritten. - expect(repository.update).not.toHaveBeenCalled(); + expect(repository.insertActiveOAuthJweKey).not.toHaveBeenCalled(); }); it('does not touch a row already in the DEK-style wrap', async () => { - repository.findOne.mockResolvedValue(makeRow({ value: 'enc-active' })); + repository.findActiveOAuthJweKey.mockResolvedValue(makeRow({ value: 'enc-active' })); await Container.get(OAuthJweKeyService).getKeyPair(); - expect(repository.update).not.toHaveBeenCalled(); + expect(repository.insertActiveOAuthJweKey).not.toHaveBeenCalled(); expect(cipher.decryptWithInstanceKey).not.toHaveBeenCalled(); }); }); @@ -260,8 +258,8 @@ describe('OAuthJweKeyService', () => { await Container.get(OAuthJweKeyService).getKeyPair(); - expect(repository.findOne).not.toHaveBeenCalled(); - expect(repository.insert).not.toHaveBeenCalled(); + expect(repository.findActiveOAuthJweKey).not.toHaveBeenCalled(); + expect(repository.insertActiveOAuthJweKey).not.toHaveBeenCalled(); }); }); }); diff --git a/packages/cli/src/modules/oauth-jwe/oauth-jwe-key.service.ts b/packages/cli/src/modules/oauth-jwe/oauth-jwe-key.service.ts index 639da727a18..f55a9948d23 100644 --- a/packages/cli/src/modules/oauth-jwe/oauth-jwe-key.service.ts +++ b/packages/cli/src/modules/oauth-jwe/oauth-jwe-key.service.ts @@ -13,7 +13,6 @@ import { JWE_KEY_ALGORITHMS, JWE_KEY_CACHE_KEY, JWE_KEY_USE, - JWE_PRIVATE_KEY_TYPE, type JweKeyAlgorithm, } from './oauth-jwe.constants'; @@ -181,13 +180,7 @@ export class OAuthJweKeyService { } private async readActiveEntry(algorithm: JweKeyAlgorithm): Promise { - const privateRow = await this.deploymentKeyRepository.findOne({ - where: { - type: JWE_PRIVATE_KEY_TYPE, - algorithm, - status: 'active', - }, - }); + const privateRow = await this.deploymentKeyRepository.findActiveOAuthJweKey(algorithm); if (!privateRow) return null; const decryptedPrivate = this.unwrapPrivateJwk(privateRow.value); @@ -228,13 +221,7 @@ export class OAuthJweKeyService { const encryptedPrivate = this.cipher.encryptDEKWithInstanceKey(JSON.stringify(privateJwk)); try { - await this.deploymentKeyRepository.insert({ - id, - type: JWE_PRIVATE_KEY_TYPE, - value: encryptedPrivate, - algorithm, - status: 'active', - }); + await this.deploymentKeyRepository.insertActiveOAuthJweKey(id, encryptedPrivate, algorithm); this.logger.info('Generated new instance OAuth JWE key pair', { algorithm, kid: id }); } catch (error) { diff --git a/packages/cli/src/modules/oauth-jwe/oauth-jwe.constants.ts b/packages/cli/src/modules/oauth-jwe/oauth-jwe.constants.ts index 0d8e3f15375..c11056aa556 100644 --- a/packages/cli/src/modules/oauth-jwe/oauth-jwe.constants.ts +++ b/packages/cli/src/modules/oauth-jwe/oauth-jwe.constants.ts @@ -1,4 +1,4 @@ -export const JWE_PRIVATE_KEY_TYPE = 'jwe.private-key'; +export { OAUTH_JWE_PRIVATE_KEY_TYPE as JWE_PRIVATE_KEY_TYPE } from '@n8n/db'; /** * RSA `alg` values accepted in the public JWKS for `kty: 'RSA'`. Drives the diff --git a/packages/cli/src/modules/policy-infrastructure/README.md b/packages/cli/src/modules/policy-infrastructure/README.md index 60deb5fdbea..c5d2a236dcd 100644 --- a/packages/cli/src/modules/policy-infrastructure/README.md +++ b/packages/cli/src/modules/policy-infrastructure/README.md @@ -197,11 +197,11 @@ checks that the token exists, was minted for this point, and binds to this subject. Only `PolicyEnforcementService` may import the minter; a lint rule enforces that. -A second lint rule, `no-unsealed-workflow-entity-write`, flags direct -`save`/`insert`/`update`/`upsert` calls on `WorkflowEntity` in runtime code. It is -syntactic. It catches `save({ id, nodes })` and `update(id, { nodes })`, not a -payload built off-site or an aliased receiver. The runtime check is the enforcing -half. +The workflow entity subscriber rejects inserts and node-bearing updates outside +the repository's scoped write context. Policy-cleared repository methods open +that context after they validate the matching token. Standalone node execution +is the documented exception: it opens the context without a save token, then +enforces the policy before the temporary workflow starts. ## Add a check diff --git a/packages/cli/src/services/__tests__/deployment-signing-secrets.integration.test.ts b/packages/cli/src/services/__tests__/deployment-signing-secrets.integration.test.ts index 820ff3256fc..0f65cc4ad47 100644 --- a/packages/cli/src/services/__tests__/deployment-signing-secrets.integration.test.ts +++ b/packages/cli/src/services/__tests__/deployment-signing-secrets.integration.test.ts @@ -1,7 +1,8 @@ import { mockInstance, testDb } from '@n8n/backend-test-utils'; import { GlobalConfig } from '@n8n/config'; -import { DeploymentKeyRepository } from '@n8n/db'; +import { DeploymentKey, DeploymentKeyRepository } from '@n8n/db'; import { Container } from '@n8n/di'; +import { DataSource, type Repository } from '@n8n/typeorm'; import jsonwebtoken from 'jsonwebtoken'; import { Cipher, InstanceSettings } from 'n8n-core'; @@ -27,21 +28,23 @@ afterAll(async () => { // real database and the real cipher. describe('deployment signing secrets (integration)', () => { let repo: DeploymentKeyRepository; + let keyStore: Repository; let cipher: Cipher; beforeAll(() => { repo = Container.get(DeploymentKeyRepository); + keyStore = Container.get(DataSource).getRepository(DeploymentKey); cipher = Container.get(Cipher); }); beforeEach(async () => { - await testDb.truncate(['DeploymentKey']); + await testDb.resetDeploymentKeys(); }); it('seeds a secret in wrapped form and reads it back in usable form', async () => { await repo.seedSigningSecret('signing.hmac', 'the-hmac-secret'); - const row = await repo.findActiveByType('signing.hmac'); + const row = await keyStore.findOne({ where: { type: 'signing.hmac', status: 'active' } }); expect(row).not.toBeNull(); expect(row!.algorithm).toBe('aes-256-gcm'); expect(row!.value).not.toContain('the-hmac-secret'); @@ -51,33 +54,37 @@ describe('deployment signing secrets (integration)', () => { }); it('returns a row in the original stored form as-is without rewriting it', async () => { - await repo.insertOrIgnore({ - type: 'signing.jwt', - value: 'stored-plain-secret', - status: 'active', - algorithm: null, - }); + await keyStore.save( + keyStore.create({ + type: 'signing.jwt', + value: 'stored-plain-secret', + status: 'active', + algorithm: null, + }), + ); await expect(repo.findActiveSigningSecret('signing.jwt')).resolves.toBe('stored-plain-secret'); - const row = await repo.findActiveByType('signing.jwt'); + const row = await keyStore.findOne({ where: { type: 'signing.jwt', status: 'active' } }); expect(row!.algorithm).toBeNull(); expect(row!.value).toBe('stored-plain-secret'); }); it('upgrades a row in the original stored form when rewrapLegacy is set', async () => { - await repo.insertOrIgnore({ - type: 'signing.jwt', - value: 'stored-plain-secret', - status: 'active', - algorithm: null, - }); + await keyStore.save( + keyStore.create({ + type: 'signing.jwt', + value: 'stored-plain-secret', + status: 'active', + algorithm: null, + }), + ); await expect(repo.findActiveSigningSecret('signing.jwt', { rewrapLegacy: true })).resolves.toBe( 'stored-plain-secret', ); - const row = await repo.findActiveByType('signing.jwt'); + const row = await keyStore.findOne({ where: { type: 'signing.jwt', status: 'active' } }); expect(row!.algorithm).toBe('aes-256-gcm'); expect(row!.value).not.toBe('stored-plain-secret'); @@ -88,12 +95,14 @@ describe('deployment signing secrets (integration)', () => { }); it('throws on a storage format this version cannot read', async () => { - await repo.insertOrIgnore({ - type: 'signing.jwt', - value: 'value-in-some-future-format', - status: 'active', - algorithm: 'aes-256-cbc', - }); + await keyStore.save( + keyStore.create({ + type: 'signing.jwt', + value: 'value-in-some-future-format', + status: 'active', + algorithm: 'aes-256-cbc', + }), + ); await expect(repo.findActiveSigningSecret('signing.jwt')).rejects.toThrow( "unsupported storage format 'aes-256-cbc'", @@ -102,9 +111,11 @@ describe('deployment signing secrets (integration)', () => { it('throws a usable error when a wrapped value cannot be read', async () => { await repo.seedSigningSecret('signing.binary_data', 'the-binary-secret'); - const row = await repo.findActiveByType('signing.binary_data'); + const row = await keyStore.findOne({ + where: { type: 'signing.binary_data', status: 'active' }, + }); // Simulate a value written under a different instance encryption key. - await repo.update({ id: row!.id }, { value: `${row!.value.slice(0, -4)}AAAA` }); + await keyStore.update({ id: row!.id }, { value: `${row!.value.slice(0, -4)}AAAA` }); await expect(repo.findActiveSigningSecret('signing.binary_data')).rejects.toThrow( 'cannot be read with this instance encryption key', diff --git a/packages/cli/test/integration/encryption-keys.api.test.ts b/packages/cli/test/integration/encryption-keys.api.test.ts index 5fec4309ba8..ba7fbe9d19c 100644 --- a/packages/cli/test/integration/encryption-keys.api.test.ts +++ b/packages/cli/test/integration/encryption-keys.api.test.ts @@ -1,7 +1,7 @@ import { testDb } from '@n8n/backend-test-utils'; -import type { DeploymentKey, User } from '@n8n/db'; -import { DeploymentKeyRepository } from '@n8n/db'; +import { DeploymentKey, type User } from '@n8n/db'; import { Container } from '@n8n/di'; +import { DataSource, type Repository } from '@n8n/typeorm'; import { createMember, createOwner } from './shared/db/users'; import type { SuperAgentTest } from './shared/types'; @@ -9,18 +9,19 @@ import * as utils from './shared/utils/'; const testServer = utils.setupTestServer({ endpointGroups: ['encryption-keys'] }); -let deploymentKeyRepository: DeploymentKeyRepository; +let keyStore: Repository; let owner: User; let ownerAgent: SuperAgentTest; let member: User; let memberAgent: SuperAgentTest; beforeAll(() => { - deploymentKeyRepository = Container.get(DeploymentKeyRepository); + keyStore = Container.get(DataSource).getRepository(DeploymentKey); }); beforeEach(async () => { - await testDb.truncate(['User', 'DeploymentKey']); + await testDb.truncate(['User']); + await testDb.resetDeploymentKeys(); owner = await createOwner(); ownerAgent = testServer.authAgentFor(owner); member = await createMember(); @@ -38,8 +39,8 @@ const seedKey = async ( }> = {}, ) => { const { createdAt, updatedAt, ...rest } = overrides; - const saved = await deploymentKeyRepository.save( - deploymentKeyRepository.create({ + const saved = await keyStore.save( + keyStore.create({ type: 'data_encryption', value: 'seed-value', algorithm: 'aes-256-cbc', @@ -48,11 +49,11 @@ const seedKey = async ( }), ); if (createdAt || updatedAt) { - await deploymentKeyRepository.update(saved.id, { + await keyStore.update(saved.id, { ...(createdAt ? { createdAt } : {}), ...(updatedAt ? { updatedAt } : {}), }); - return await deploymentKeyRepository.findOneByOrFail({ id: saved.id }); + return await keyStore.findOneByOrFail({ id: saved.id }); } return saved; }; @@ -317,7 +318,7 @@ describe('POST /encryption/keys', () => { }); expect(response.body.data).not.toHaveProperty('value'); - const rows = await deploymentKeyRepository.find({ where: { type: 'data_encryption' } }); + const rows = await keyStore.find({ where: { type: 'data_encryption' } }); expect(rows).toHaveLength(2); const active = rows.filter((r: DeploymentKey) => r.status === 'active'); @@ -327,7 +328,7 @@ describe('POST /encryption/keys', () => { expect(active[0].value.length).toBeGreaterThan(0); expect(JSON.stringify(response.body)).not.toContain(active[0].value); - const reloadedPrevious = await deploymentKeyRepository.findOneByOrFail({ + const reloadedPrevious = await keyStore.findOneByOrFail({ id: previousActive.id, }); expect(reloadedPrevious.status).toBe('inactive'); @@ -344,7 +345,7 @@ describe('POST /encryption/keys', () => { test('returns 403 for a non-owner user', async () => { await memberAgent.post('/encryption/keys').send({ type: 'data_encryption' }).expect(403); - const rows = await deploymentKeyRepository.find(); + const rows = await keyStore.find(); expect(rows).toHaveLength(0); }); }); diff --git a/packages/cli/test/migration/1790054384570-prevent-deployment-key-deletion.test.ts b/packages/cli/test/migration/1790054384570-prevent-deployment-key-deletion.test.ts new file mode 100644 index 00000000000..bb0ed8fc542 --- /dev/null +++ b/packages/cli/test/migration/1790054384570-prevent-deployment-key-deletion.test.ts @@ -0,0 +1,101 @@ +import { + createTestMigrationContext, + initDbUpToMigration, + runSingleMigration, + undoLastSingleMigration, + type TestMigrationContext, +} from '@n8n/backend-test-utils'; +import { DbConnection } from '@n8n/db'; +import { Container } from '@n8n/di'; +import { DataSource } from '@n8n/typeorm'; + +const MIGRATION_NAME = 'PreventDeploymentKeyDeletion1790054384570'; + +describe('PreventDeploymentKeyDeletion migration', () => { + let dataSource: DataSource; + + async function withContext(fn: (context: TestMigrationContext) => Promise): Promise { + const context = createTestMigrationContext(dataSource); + try { + return await fn(context); + } finally { + await context.queryRunner.release(); + } + } + + beforeAll(async () => { + const dbConnection = Container.get(DbConnection); + await dbConnection.init(); + dataSource = Container.get(DataSource); + }); + + beforeEach(async () => { + await withContext(async (context) => await context.queryRunner.clearDatabase()); + await initDbUpToMigration(MIGRATION_NAME); + }); + + afterAll(async () => { + await Container.get(DbConnection).close(); + }); + + async function insertKey(context: TestMigrationContext) { + const table = context.escape.tableName('deployment_key'); + await context.runQuery( + `INSERT INTO ${table} (id, type, value, algorithm, status, "createdAt", "updatedAt") + VALUES (:id, :type, :value, :algorithm, :status, :createdAt, :updatedAt)`, + { + id: 'key-1', + type: 'data_encryption', + value: 'encrypted-value', + algorithm: 'aes-256-gcm', + status: 'active', + createdAt: new Date(), + updatedAt: new Date(), + }, + ); + } + + it('rejects deletion and keeps the key readable', async () => { + await withContext(insertKey); + await runSingleMigration(MIGRATION_NAME); + + await withContext(async (context) => { + const table = context.escape.tableName('deployment_key'); + await expect(context.runQuery(`DELETE FROM ${table}`)).rejects.toThrow( + 'Deployment keys must not be deleted', + ); + await expect( + context.runQuery(`UPDATE ${table} SET status = 'inactive' WHERE id = 'key-1'`), + ).resolves.not.toThrow(); + + const rows = await context.runQuery>( + `SELECT id, status FROM ${table}`, + ); + expect(rows).toEqual([{ id: 'key-1', status: 'inactive' }]); + }); + }); + + it('rejects truncation on Postgres', async () => { + if (dataSource.options.type !== 'postgres') return; + await withContext(insertKey); + await runSingleMigration(MIGRATION_NAME); + + await withContext(async (context) => { + const table = context.escape.tableName('deployment_key'); + await expect(context.runQuery(`TRUNCATE TABLE ${table}`)).rejects.toThrow( + 'Deployment keys must not be deleted', + ); + }); + }); + + it('allows deletion after the migration is reverted', async () => { + await withContext(insertKey); + await runSingleMigration(MIGRATION_NAME); + await undoLastSingleMigration(); + + await withContext(async (context) => { + const table = context.escape.tableName('deployment_key'); + await expect(context.runQuery(`DELETE FROM ${table}`)).resolves.not.toThrow(); + }); + }); +}); diff --git a/packages/core/src/instance-settings/__tests__/instance-settings.test.ts b/packages/core/src/instance-settings/__tests__/instance-settings.test.ts index 11cd8511969..ad0620884c6 100644 --- a/packages/core/src/instance-settings/__tests__/instance-settings.test.ts +++ b/packages/core/src/instance-settings/__tests__/instance-settings.test.ts @@ -224,8 +224,8 @@ describe('InstanceSettings', () => { describe('initialize', () => { const mockRepo = { - findActiveByType: vi.fn(), - insertOrIgnore: vi.fn(), + findActiveIdentifier: vi.fn(), + seedActiveIdentifier: vi.fn(), findActiveSigningSecret: vi.fn(), seedSigningSecret: vi.fn(), }; @@ -240,8 +240,8 @@ describe('InstanceSettings', () => { settings = createInstanceSettings({ encryptionKey: 'test_key' }); // Default: no DB rows, inserts succeed - mockRepo.findActiveByType.mockResolvedValue(null); - mockRepo.insertOrIgnore.mockResolvedValue(undefined); + mockRepo.findActiveIdentifier.mockResolvedValue(null); + mockRepo.seedActiveIdentifier.mockResolvedValue(undefined); mockRepo.findActiveSigningSecret.mockResolvedValue(null); mockRepo.seedSigningSecret.mockResolvedValue(undefined); }); @@ -253,22 +253,24 @@ describe('InstanceSettings', () => { await settings.initialize(mockRepo); expect(settings.instanceId).toEqual('env-pinned-id'); - expect(mockRepo.findActiveByType).not.toHaveBeenCalledWith('instance.id'); - expect(mockRepo.insertOrIgnore).not.toHaveBeenCalledWith( - expect.objectContaining({ type: 'instance.id' }), + expect(mockRepo.findActiveIdentifier).not.toHaveBeenCalledWith('instance.id'); + expect(mockRepo.seedActiveIdentifier).not.toHaveBeenCalledWith( + 'instance.id', + expect.anything(), ); }); it('should use the value from the active DB row when one exists', async () => { - mockRepo.findActiveByType.mockImplementation(async (type: string) => + mockRepo.findActiveIdentifier.mockImplementation(async (type: string) => type === 'instance.id' ? { value: 'db-stored-id' } : null, ); await settings.initialize(mockRepo); expect(settings.instanceId).toEqual('db-stored-id'); - expect(mockRepo.insertOrIgnore).not.toHaveBeenCalledWith( - expect.objectContaining({ type: 'instance.id' }), + expect(mockRepo.seedActiveIdentifier).not.toHaveBeenCalledWith( + 'instance.id', + expect.anything(), ); }); @@ -277,12 +279,7 @@ describe('InstanceSettings', () => { await settings.initialize(mockRepo); - expect(mockRepo.insertOrIgnore).toHaveBeenCalledWith({ - type: 'instance.id', - value: derivedId, - status: 'active', - algorithm: null, - }); + expect(mockRepo.seedActiveIdentifier).toHaveBeenCalledWith('instance.id', derivedId); expect(settings.instanceId).toEqual(derivedId); }); @@ -298,11 +295,10 @@ describe('InstanceSettings', () => { }); it('should use the winner row when a concurrent insert is ignored', async () => { - mockRepo.insertOrIgnore.mockImplementation(async (entity: { type: string }) => { - // Simulate conflict only for instance.id - if (entity.type === 'instance.id') return undefined; + mockRepo.seedActiveIdentifier.mockImplementation(async (type: string) => { + if (type === 'instance.id') return undefined; }); - mockRepo.findActiveByType.mockImplementation(async (type: string) => + mockRepo.findActiveIdentifier.mockImplementation(async (type: string) => type === 'instance.id' ? { value: 'winner-id' } : null, ); @@ -365,7 +361,7 @@ describe('InstanceSettings', () => { await settings.initialize(mockRepo, { canSeed: false }); - expect(mockRepo.insertOrIgnore).not.toHaveBeenCalled(); + expect(mockRepo.seedActiveIdentifier).not.toHaveBeenCalled(); expect(mockRepo.seedSigningSecret).not.toHaveBeenCalled(); expect(settings.instanceId).toEqual(derivedId); expect(settings.hmacSignatureSecret).toEqual(derivedHmac); @@ -402,7 +398,7 @@ describe('InstanceSettings', () => { expect(settings.instanceId).toEqual('env-pinned-id'); expect(settings.hmacSignatureSecret).toEqual('db-stored-hmac'); - expect(mockRepo.insertOrIgnore).not.toHaveBeenCalled(); + expect(mockRepo.seedActiveIdentifier).not.toHaveBeenCalled(); expect(mockRepo.seedSigningSecret).not.toHaveBeenCalled(); }); }); diff --git a/packages/core/src/instance-settings/instance-settings.ts b/packages/core/src/instance-settings/instance-settings.ts index 0d78f5666ba..e77c894957f 100644 --- a/packages/core/src/instance-settings/instance-settings.ts +++ b/packages/core/src/instance-settings/instance-settings.ts @@ -34,13 +34,8 @@ type Settings = ReadOnlySettings & WritableSettings; * package dependency: `@n8n/db` depends on `n8n-core` at runtime. */ export type DeploymentStateRepo = { - findActiveByType(type: string): Promise<{ value: string } | null>; - insertOrIgnore(entity: { - type: string; - value: string; - status: string; - algorithm: null; - }): Promise; + findActiveIdentifier(type: string): Promise<{ value: string } | null>; + seedActiveIdentifier(type: string, value: string): Promise; findActiveSigningSecret(type: string, opts?: { rewrapLegacy?: boolean }): Promise; seedSigningSecret(type: string, secret: string): Promise; }; @@ -165,14 +160,14 @@ export class InstanceSettings { set(envValue); return; } - const existing = await repo.findActiveByType(type); + const existing = await repo.findActiveIdentifier(type); if (existing) { set(existing.value); return; } if (!canSeed) return; - await repo.insertOrIgnore({ type, value: get(), status: 'active', algorithm: null }); - const winner = await repo.findActiveByType(type); + await repo.seedActiveIdentifier(type, get()); + const winner = await repo.findActiveIdentifier(type); if (winner) set(winner.value); } diff --git a/packages/testing/code-health/src/rules/encryption-boundary.rule.test.ts b/packages/testing/code-health/src/rules/encryption-boundary.rule.test.ts index 3ec9ef66415..47559fd7d76 100644 --- a/packages/testing/code-health/src/rules/encryption-boundary.rule.test.ts +++ b/packages/testing/code-health/src/rules/encryption-boundary.rule.test.ts @@ -120,7 +120,7 @@ describe('EncryptionBoundaryRule', () => { const downgraded = `${BOUNDARY_CONFIG.trimEnd()} export const extra = { rules: { - 'n8n-local-rules/no-deployment-key-delete': 'off', + 'n8n-local-rules/no-encryption-guardrail-disable': ['warn'], 'n8n-local-rules/no-uncaught-json-parse': 'off', }, }; @@ -130,8 +130,7 @@ export const extra = { const violations = await analyze(); expect(violations).toHaveLength(1); - expect(violations[0]).toContain('eslint.config.mjs:7'); - expect(violations[0]).toContain('no-deployment-key-delete'); + expect(violations[0]).toContain('no-encryption-guardrail-disable'); }); }); @@ -182,7 +181,7 @@ export const extra = { write( 'packages/a/src/index.ts', [ - '// oxlint-disable-next-line n8n-local-rules/no-deployment-key-delete', + '// oxlint-disable-next-line n8n-local-rules/no-encryption-guardrail-disable', 'export const a = 1; // oxlint-disable-line no-console', '/* oxlint-disable */', '/* oxlint n8n-local-rules/no-encryption-guardrail-disable: "off" */', @@ -193,7 +192,7 @@ export const extra = { const violations = await analyze(); expect(violations).toHaveLength(3); - expect(violations[0]).toContain('no-deployment-key-delete'); + expect(violations[0]).toContain('no-encryption-guardrail-disable'); expect(violations[1]).toContain('`oxlint-disable` directive silences every lint rule'); expect(violations[2]).toContain('inline `oxlint` configuration comment'); }); @@ -215,7 +214,7 @@ export const extra = { write( 'packages/a/src/index.ts', [ - '// eslint-disable-next-line n8n-local-rules/no-deployment-key-delete', + '// eslint-disable-next-line n8n-local-rules/no-encryption-guardrail-disable', 'export const a = 1; // eslint-disable-line no-console', '/* eslint n8n-local-rules/no-encryption-guardrail-disable: "off" */', '', @@ -225,7 +224,7 @@ export const extra = { const violations = await analyze(); expect(violations).toHaveLength(2); - expect(violations[0]).toContain('no-deployment-key-delete'); + expect(violations[0]).toContain('no-encryption-guardrail-disable'); expect(violations[1]).toContain('inline `eslint` configuration comment'); }); @@ -246,7 +245,7 @@ export const extra = { it('does not read the explanation after -- as a rule list', async () => { write( 'packages/a/src/index.ts', - '// eslint-disable-next-line no-console -- unrelated to no-deployment-key-delete\nexport const a = 1;\n', + '// eslint-disable-next-line no-console -- unrelated to encryption guardrails\nexport const a = 1;\n', ); expect(await analyze()).toEqual([]); diff --git a/packages/testing/code-health/src/rules/encryption-boundary.rule.ts b/packages/testing/code-health/src/rules/encryption-boundary.rule.ts index 047b222e299..4398aa4a787 100644 --- a/packages/testing/code-health/src/rules/encryption-boundary.rule.ts +++ b/packages/testing/code-health/src/rules/encryption-boundary.rule.ts @@ -19,7 +19,7 @@ import { const DEFAULT_TRIGGER_DEPENDENCIES = ['n8n-core', '@n8n/db']; /** The rules `encryptionBoundaryConfig` enables, without the plugin prefix. */ -const DEFAULT_GUARDED_RULES = ['no-deployment-key-delete', 'no-encryption-guardrail-disable']; +const DEFAULT_GUARDED_RULES = ['no-encryption-guardrail-disable']; const BOUNDARY_CONFIG_PATH = 'packages/@n8n/eslint-config/src/configs/encryption-boundary.ts'; diff --git a/scripts/lint-parity/OXLINT_MIGRATION_BLOCKERS.md b/scripts/lint-parity/OXLINT_MIGRATION_BLOCKERS.md new file mode 100644 index 00000000000..d5153713d81 --- /dev/null +++ b/scripts/lint-parity/OXLINT_MIGRATION_BLOCKERS.md @@ -0,0 +1,198 @@ +# Oxlint migration blockers + +Last reviewed: 2026-09-18 +Oxlint version: 1.78.0 + +This file is the working backlog for the ESLint to Oxlint migration. It focuses on rules that block an Oxlint-only lint command for many packages. `oxlint-gap.json` is the machine-readable source of truth for shared-layer parity. + +## Completion criteria + +A blocker is complete when all of these conditions are true: + +1. Oxlint enforces equivalent behavior, or we intentionally retire the rule. +2. We remove the rule from `oxlint-gap.json` if it is present there. +3. `node scripts/lint-parity/oxlint-parity.mjs --pkg packages/ --eslint /tmp/eslint-snapshot.json` passes. +4. The affected packages no longer need an ESLint pass for that rule. +5. We test fixer behavior when the rule has a fixer. + +## Majority-package blockers + +These rules come from the shared base or backend layers. Resolve these first because they affect the largest number of packages. + +| Priority | Rule | Blocker | Suggested next action | +| --- | --- | --- | --- | +| P0 | `import-x/no-extraneous-dependencies` | Oxlint reserves the `import-x` plugin name. The JS plugin bridge has not been validated with an alias. | Test `eslint-plugin-import-x` under an alias. If that is unstable, implement a native package-manifest check. | +| P1 | `no-restricted-syntax` | Oxlint has no AST-selector rule. The base layer uses it for the raw-enum restriction. | Add a purpose-built syntax rule for each active selector. Start with the raw-enum rule. | +| P1 | `typescript/consistent-type-imports` | Oxlint does not honor the `emitDecoratorMetadata` exception. Its fixer can remove imports that DI needs at runtime. | Wait for upstream support or add a tested shim that skips decorated files. Do not enable the current fixer. | +| P1 | `typescript/prefer-optional-chain` | The native nursery rule changes guarded undeclared globals incorrectly. | Re-test after the rule leaves nursery or after the upstream fixer issue is resolved. | +| P2 | `import/export` | A native rule exists, but it is still in the nursery category. | Run parity samples and enable it after it becomes stable. | +| P3 | `no-octal` | Oxlint has no rule. The parser already rejects relevant octal syntax in module and strict-mode code. | Confirm parser coverage and retire the explicit rule if no supported source form remains. | + +## Package-level typed blocker + +### `@typescript-eslint/naming-convention` + +Oxlint has no native equivalent. The shared base disables this rule, but multiple package configs enable it at `error`. This blocks those packages from reaching full parity even after the shared blockers are complete. + +Possible approaches: + +1. Retire rules that only enforce style preferences. +2. Replace important selectors with small syntax-only custom rules. +3. Use existing Oxlint rules for specific conventions, such as filename or identifier restrictions. +4. Keep a narrow ESLint pass only in packages whose convention protects an API contract. + +Audit every package configuration before conversion. Do not copy the full generic naming rule into a new custom implementation. + +## Frontend package blockers + +Oxlint can lint the script section of a Vue SFC, but it does not expose the Vue template AST. An Oxlint-only frontend package would lose the following 59 rules: + +```text +vue/block-order +vue/comment-directive +vue/component-name-in-template-casing +vue/jsx-uses-vars +vue/no-child-content +vue/no-deprecated-dollar-listeners-api +vue/no-deprecated-dollar-scopedslots-api +vue/no-deprecated-filter +vue/no-deprecated-functional-template +vue/no-deprecated-html-element-is +vue/no-deprecated-inline-template +vue/no-deprecated-router-link-tag-prop +vue/no-deprecated-scope-attribute +vue/no-deprecated-slot-attribute +vue/no-deprecated-slot-scope-attribute +vue/no-deprecated-v-bind-sync +vue/no-deprecated-v-is +vue/no-deprecated-v-on-native-modifier +vue/no-deprecated-v-on-number-modifiers +vue/no-dupe-v-else-if +vue/no-duplicate-attributes +vue/no-multiple-template-root +vue/no-parsing-error +vue/no-ref-as-operand +vue/no-template-key +vue/no-textarea-mustache +vue/no-undef-components +vue/no-unused-components +vue/no-unused-vars +vue/no-use-computed-property-like-method +vue/no-use-v-if-with-v-for +vue/no-useless-template-attributes +vue/no-v-for-template-key-on-child +vue/no-v-html +vue/require-component-is +vue/require-macro-variable-name +vue/require-toggle-inside-transition +vue/require-v-for-key +vue/require-valid-default-prop +vue/use-v-on-exact +vue/v-slot-style +vue/valid-attribute-name +vue/valid-template-root +vue/valid-v-bind +vue/valid-v-cloak +vue/valid-v-else +vue/valid-v-else-if +vue/valid-v-for +vue/valid-v-html +vue/valid-v-if +vue/valid-v-is +vue/valid-v-memo +vue/valid-v-model +vue/valid-v-on +vue/valid-v-once +vue/valid-v-pre +vue/valid-v-show +vue/valid-v-slot +vue/valid-v-text +``` + +The design-system rule `@n8n/design-system/require-teleported-tooltip-in-dropdown` has the same blocker. It uses `vue-eslint-parser` template services, not TypeScript type information. + +Suggested direction: + +1. Keep ESLint for `.vue` files until Oxlint exposes a template AST or another template-aware linter replaces it. +2. Run Oxlint for `.ts`, `.tsx`, and the SFC script section. +3. Do not rewrite 60 template rules as source-text checks. + +## Node package blockers + +The `n8n-nodes-base` plugin contributes 94 enforced rules: + +- 12 credential rules in `packages/@n8n/eslint-config/src/configs/nodes.ts`. +- 82 node rules in the same config. + +The blocker is not known type awareness. The plugin needs a compatibility test through Oxlint's JavaScript plugin bridge. Its file-scoped rule tables also need an Oxlint translation. + +Use the rule tables in `packages/@n8n/eslint-config/src/configs/nodes.ts` as the canonical list. They change more often than this document and must not be duplicated here. + +Suggested spike: + +1. Load `eslint-plugin-n8n-nodes-base` through `jsPlugins` in an isolated node package. +2. Run all 12 credential rules against representative valid and invalid files. +3. Run the 82 node rules against representative valid and invalid files. +4. Compare diagnostics, locations, options, and fixes with ESLint. +5. Add the working file-scoped tables to `@n8n/oxlint-config/nodes`. + +## Rules that are not blockers + +Oxlint has native implementations for almost all configured type-aware `@typescript-eslint` rules. This includes promise analysis, unsafe-operation checks, type constituent checks, and template-expression checks. + +Do not keep ESLint only because a rule is type-aware. Check whether Oxlint already provides the rule in the `typescript` namespace. + +The main exceptions are: + +- `@typescript-eslint/naming-convention`, which has no native equivalent. +- `@typescript-eslint/consistent-type-imports`, which diverges for decorated files. +- `@typescript-eslint/prefer-optional-chain`, which has a known semantic divergence. + +## Work order + +1. Resolve or isolate `import-x/no-extraneous-dependencies`. +2. Replace active `no-restricted-syntax` selectors with focused rules. +3. Resolve decorator-safe `consistent-type-imports` behavior. +4. Audit and reduce `naming-convention` package overrides. +5. Re-test the two nursery rules. +6. Run a JS-plugin bridge spike for `n8n-nodes-base`. +7. Keep a template-only ESLint pass for Vue until native template support exists. + +## Validation commands + +Create an ESLint snapshot and run the shared parity check for each converted package: + +```sh +node scripts/lint-parity/snapshot.mjs --out /tmp/eslint-snapshot.json --only packages/ +node scripts/lint-parity/oxlint-parity.mjs --pkg packages/ --eslint /tmp/eslint-snapshot.json +``` + +Inspect package rule downgrades and overrides: + +```sh +node scripts/lint-parity/majority.mjs +``` + +Before changing the config, create the baseline snapshot. After the change, create the second snapshot and compare them: + +```sh +pnpm turbo run build --filter=@n8n/eslint-config +node scripts/lint-parity/snapshot.mjs --out /tmp/lint-before.json +# Change the config. +node scripts/lint-parity/snapshot.mjs --out /tmp/lint-after.json +node scripts/lint-parity/diff.mjs /tmp/lint-before.json /tmp/lint-after.json +``` + +Run lint from each changed package. Use both linters when the package still has a compatibility pass. + +## Source references + +- Machine-readable gaps: `scripts/lint-parity/oxlint-gap.json` +- Parity checker: `scripts/lint-parity/oxlint-parity.mjs` +- ESLint base: `packages/@n8n/eslint-config/src/configs/base.ts` +- Oxlint base: `packages/@n8n/oxlint-config/src/configs/base.ts` +- ESLint frontend: `packages/@n8n/eslint-config/src/configs/frontend.ts` +- Oxlint frontend: `packages/@n8n/oxlint-config/src/configs/frontend.ts` +- ESLint nodes: `packages/@n8n/eslint-config/src/configs/nodes.ts` +- Oxlint nodes: `packages/@n8n/oxlint-config/src/configs/nodes.ts` +- Functional guardrails: `packages/@n8n/eslint-config/src/configs/functional-guardrails.ts` diff --git a/scripts/lint-parity/expected-pr1.json b/scripts/lint-parity/expected-pr1.json index 1def3a82a72..ae269de15e4 100644 --- a/scripts/lint-parity/expected-pr1.json +++ b/scripts/lint-parity/expected-pr1.json @@ -120,13 +120,6 @@ "after": "error", "reason": "package moved from base to the backend layer, which carries the network boundary" }, - { - "rule": "n8n-local-rules/no-deployment-key-delete", - "pkg": "packages/", - "before": "off", - "after": "error", - "reason": "package moved from base to the backend layer, which carries the encryption boundary" - }, { "rule": "n8n-local-rules/no-encryption-guardrail-disable", "pkg": "packages/", @@ -149,13 +142,6 @@ "after": "off", "reason": "runtime-agnostic library moved to base: no Node runtime, no HTTP imports" }, - { - "rule": "n8n-local-rules/no-deployment-key-delete", - "pkg": "packages/@n8n/utils", - "before": "error", - "after": "off", - "reason": "runtime-agnostic library moved to base" - }, { "rule": "n8n-local-rules/no-encryption-guardrail-disable", "pkg": "packages/@n8n/utils", @@ -170,13 +156,6 @@ "after": "off", "reason": "pure library moved to base: no Node APIs" }, - { - "rule": "n8n-local-rules/no-deployment-key-delete", - "pkg": "packages/@n8n/json-schema-to-zod", - "before": "error", - "after": "off", - "reason": "pure library moved to base" - }, { "rule": "n8n-local-rules/no-encryption-guardrail-disable", "pkg": "packages/@n8n/json-schema-to-zod", @@ -191,13 +170,6 @@ "after": "off", "reason": "yjs-only library moved to base: no Node APIs" }, - { - "rule": "n8n-local-rules/no-deployment-key-delete", - "pkg": "packages/@n8n/crdt", - "before": "error", - "after": "off", - "reason": "yjs-only library moved to base" - }, { "rule": "n8n-local-rules/no-encryption-guardrail-disable", "pkg": "packages/@n8n/crdt", diff --git a/scripts/lint-parity/oxlint-gap.json b/scripts/lint-parity/oxlint-gap.json index 2e23a4e66c5..8f4faeafbc1 100644 --- a/scripts/lint-parity/oxlint-gap.json +++ b/scripts/lint-parity/oxlint-gap.json @@ -50,20 +50,6 @@ "replacement": "Wait for the upstream emitDecoratorMetadata option, or use an approved shim that suppresses decorated files.", "upstream": "https://github.com/oxc-project/oxc/issues/13609" }, - "n8n-local-rules/no-unsealed-workflow-entity-write": { - "layer": "base", - "native": null, - "reason": "Needs type information. oxlint gives jsPlugin rules a frozen empty `parserServices`, so `ESLintUtils.getParserServices` throws and the rule reports `Error running JS plugin` on any file that calls a checked method. It is in `localRulesPlugin.configs.recommended`, so this is a base-layer gap, not a backend-only one.", - "impact": "The WorkflowEntity node-write seal is unenforced in migrated packages. None of the three spike packages depends on @n8n/db, but this blocks any package that does.", - "replacement": "Move this type-aware functional guardrail to @n8n/code-health in the follow-up PR." - }, - "n8n-local-rules/no-deployment-key-delete": { - "layer": "backend", - "native": null, - "reason": "Needs type information. Oxlint gives jsPlugin rules an empty parserServices object, so the rule cannot identify DeploymentKey repository receivers.", - "impact": "Deployment-key deletion is not guarded by an Oxlint-only package.", - "replacement": "Move this type-aware functional guardrail to @n8n/code-health in the follow-up PR." - }, "frontend/vue-template-rules": { "layer": "frontend", "native": null,