From 293d5afd643f2a6df6195073beed314679399290 Mon Sep 17 00:00:00 2001 From: "n8n-assistant[bot]" <100856346+n8n-assistant[bot]@users.noreply.github.com> Date: Wed, 13 May 2026 08:23:48 +0300 Subject: [PATCH 01/21] chore: Bundle/2.x (#30359) Co-authored-by: Matsu Co-authored-by: RomanDavydchuk Co-authored-by: Dawid Myslak Co-authored-by: Claude Opus 4.7 Co-authored-by: Bernhard Wittmann Co-authored-by: Daria Co-authored-by: Andreas Fitzek --- packages/@n8n/api-types/src/index.ts | 1 + .../src/schemas/data-table.schema.ts | 5 +- .../oauth1-credential.controller.test.ts | 4 +- .../oauth2-credential.controller.test.ts | 2 +- .../oauth/oauth1-credential.controller.ts | 2 +- .../oauth/oauth2-credential.controller.ts | 2 +- .../src/modules/data-table/utils/sql-utils.ts | 5 + .../source-control-import.service.ee.test.ts | 106 ++++++++-- .../source-control-import.service.ee.ts | 15 ++ .../src/oauth/__tests__/oauth.service.test.ts | 51 +++-- packages/cli/src/oauth/oauth.service.ts | 40 ++-- .../__tests__/task-runner-process-py.test.ts | 82 ++++++++ .../__tests__/task-runner-process.test.ts | 27 +++ .../task-runners/task-runner-process-js.ts | 4 +- .../task-runners/task-runner-process-py.ts | 4 +- .../controllers/oauth/oauth1.api.test.ts | 187 ++++++++++++++++++ .../controllers/oauth/oauth2.api.test.ts | 143 +++++++++++++- packages/cli/test/integration/shared/types.ts | 1 + .../integration/shared/utils/test-server.ts | 4 + packages/nodes-base/nodes/Git/Git.node.ts | 11 +- .../nodes/Git/test/Git.node.test.ts | 26 +++ .../HttpRequest/V3/HttpRequestV3.node.ts | 30 ++- .../test/node/HttpRequestV3.test.ts | 152 ++++++++++++++ packages/nodes-base/nodes/Xml/Xml.node.ts | 28 +-- .../nodes/Xml/test/node/Xml.test.ts | 100 ++++++++++ 25 files changed, 936 insertions(+), 96 deletions(-) create mode 100644 packages/cli/src/task-runners/__tests__/task-runner-process-py.test.ts create mode 100644 packages/cli/test/integration/controllers/oauth/oauth1.api.test.ts diff --git a/packages/@n8n/api-types/src/index.ts b/packages/@n8n/api-types/src/index.ts index 1f7aed31203..248f95a5647 100644 --- a/packages/@n8n/api-types/src/index.ts +++ b/packages/@n8n/api-types/src/index.ts @@ -221,6 +221,7 @@ export { type DataTableListSortBy, dateTimeSchema, dataTableColumnNameSchema, + dataTableIdSchema, } from './schemas/data-table.schema'; export type { diff --git a/packages/@n8n/api-types/src/schemas/data-table.schema.ts b/packages/@n8n/api-types/src/schemas/data-table.schema.ts index 00e281e21bc..c8c10c9d451 100644 --- a/packages/@n8n/api-types/src/schemas/data-table.schema.ts +++ b/packages/@n8n/api-types/src/schemas/data-table.schema.ts @@ -5,7 +5,10 @@ import type { ListDataTableQueryDto } from '../dto'; export const insertRowReturnType = z.union([z.literal('all'), z.literal('count'), z.literal('id')]); export const dataTableNameSchema = z.string().trim().min(1).max(128); -export const dataTableIdSchema = z.string().max(36); +export const dataTableIdSchema = z + .string() + .max(36) + .regex(/^[a-zA-Z0-9]+$/); // Postgres does not allow leading numbers or - export const DATA_TABLE_COLUMN_REGEX = /^[a-zA-Z][a-zA-Z0-9_]*$/; diff --git a/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts b/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts index 7c0da5fe770..b3addb5df8e 100644 --- a/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts +++ b/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts @@ -31,7 +31,7 @@ describe('OAuth1CredentialController', () => { describe('getAuthUri', () => { it('should return a valid auth URI', async () => { const mockResolvedCredential = mock({ id: '1' }); - oauthService.getCredential.mockResolvedValueOnce(mockResolvedCredential); + oauthService.getCredentialForUpdate.mockResolvedValueOnce(mockResolvedCredential); oauthService.generateAOauth1AuthUri.mockResolvedValueOnce( 'https://example.domain/oauth/authorize?oauth_token=random-token', ); @@ -90,7 +90,7 @@ describe('OAuth1CredentialController', () => { createdAt: timestamp, data: 'encrypted-data', }; - oauthService.getCredential.mockResolvedValueOnce(mockResolvedCredential); + oauthService.getCredentialForUpdate.mockResolvedValueOnce(mockResolvedCredential); // @ts-ignore oauthService.getDecryptedData.mockResolvedValue({ csrfSecret: 'invalid' }); oauthService.getOAuthCredentials.mockResolvedValueOnce({ diff --git a/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts b/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts index 52ea9ae567b..a35c16424ee 100644 --- a/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts +++ b/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts @@ -49,7 +49,7 @@ describe('OAuth2CredentialController', () => { ); const mockResolvedCredential = mock({ id: '1' }); - oauthService.getCredential.mockResolvedValueOnce(mockResolvedCredential); + oauthService.getCredentialForUpdate.mockResolvedValueOnce(mockResolvedCredential); oauthService.getOAuthCredentials.mockResolvedValueOnce({ clientId: 'client_id', clientSecret: 'client_secret', diff --git a/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts b/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts index 0ed7c4ee2f5..c7ddab7459e 100644 --- a/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts +++ b/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts @@ -22,7 +22,7 @@ export class OAuth1CredentialController { /** Get Authorization url */ @Get('/auth') async getAuthUri(req: OAuthRequest.OAuth1Credential.Auth): Promise { - const credential = await this.oauthService.getCredential(req); + const credential = await this.oauthService.getCredentialForUpdate(req); const uri = await this.oauthService.generateAOauth1AuthUri(credential, { cid: credential.id, diff --git a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts index e82a9f89c55..d4bb34bd4fd 100644 --- a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts +++ b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts @@ -26,7 +26,7 @@ export class OAuth2CredentialController { /** Get Authorization url */ @Get('/auth') async getAuthUri(req: OAuthRequest.OAuth2Credential.Auth): Promise { - const credential = await this.oauthService.getCredential(req); + const credential = await this.oauthService.getCredentialForUpdate(req); const uri = await this.oauthService.generateAOauth2AuthUri(credential, { cid: credential.id, diff --git a/packages/cli/src/modules/data-table/utils/sql-utils.ts b/packages/cli/src/modules/data-table/utils/sql-utils.ts index 64f65e1288e..1802b5403e5 100644 --- a/packages/cli/src/modules/data-table/utils/sql-utils.ts +++ b/packages/cli/src/modules/data-table/utils/sql-utils.ts @@ -1,5 +1,6 @@ import { dataTableColumnNameSchema, + dataTableIdSchema, DATA_TABLE_COLUMN_ERROR_MESSAGE, type DataTableCreateColumnSchema, } from '@n8n/api-types'; @@ -79,6 +80,10 @@ export function isValidColumnName(name: string) { return dataTableColumnNameSchema.safeParse(name).success; } +export function isValidDataTableId(id: string) { + return dataTableIdSchema.safeParse(id).success; +} + export function addColumnQuery( tableName: DataTableUserTableName, column: DataTableCreateColumnSchema, diff --git a/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts b/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts index de39fbd0788..31526404a35 100644 --- a/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts +++ b/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts @@ -2746,7 +2746,7 @@ describe('SourceControlImportService', () => { id: 'dt1', name: 'Test Table 1', projectId: 'project1', - columns: [{ id: 'col1', name: 'Column 1', type: 'string', index: 0 }], + columns: [{ id: 'col1', name: 'Column1', type: 'string', index: 0 }], createdAt: '2024-01-01T00:00:00.000Z', updatedAt: '2024-01-02T00:00:00.000Z', }; @@ -2754,7 +2754,7 @@ describe('SourceControlImportService', () => { id: 'dt2', name: 'Test Table 2', projectId: 'project2', - columns: [{ id: 'col2', name: 'Column 2', type: 'number', index: 0 }], + columns: [{ id: 'col2', name: 'Column2', type: 'number', index: 0 }], createdAt: '2024-01-03T00:00:00.000Z', updatedAt: '2024-01-04T00:00:00.000Z', }; @@ -2824,7 +2824,7 @@ describe('SourceControlImportService', () => { id: 'dt1', name: 'Test Table', projectId: 'project1', - columns: [{ id: 'col1', name: 'Column 1', type: 'string', index: 0 }], + columns: [{ id: 'col1', name: 'Column1', type: 'string', index: 0 }], createdAt: new Date('2024-01-01'), updatedAt: new Date('2024-01-02'), project: { @@ -2851,7 +2851,7 @@ describe('SourceControlImportService', () => { projectId: 'project1', projectName: 'Team Project 1', }, - columns: [{ id: 'col1', name: 'Column 1', type: 'string', index: 0 }], + columns: [{ id: 'col1', name: 'Column1', type: 'string', index: 0 }], filename: expect.stringContaining('dt1.json'), createdAt: '2024-01-01T00:00:00.000Z', updatedAt: '2024-01-02T00:00:00.000Z', @@ -2960,7 +2960,7 @@ describe('SourceControlImportService', () => { teamId: 'project1', teamName: 'Team Project 1', }, - columns: [{ id: 'col1', name: 'Column 1', type: 'string', index: 0 }], + columns: [{ id: 'col1', name: 'Column1', type: 'string', index: 0 }], createdAt: '2024-01-01T00:00:00.000Z', updatedAt: '2024-01-02T00:00:00.000Z', }; @@ -3003,7 +3003,7 @@ describe('SourceControlImportService', () => { projectName: 'User Name', personalEmail: 'user@example.com', }, - columns: [{ id: 'col1', name: 'Column 1', type: 'string', index: 0 }], + columns: [{ id: 'col1', name: 'Column1', type: 'string', index: 0 }], createdAt: '2024-01-01T00:00:00.000Z', updatedAt: '2024-01-02T00:00:00.000Z', }; @@ -3041,8 +3041,8 @@ describe('SourceControlImportService', () => { teamName: 'Team Project 1', }, columns: [ - { id: 'col1', name: 'Column 1', type: 'string', index: 0 }, - { id: 'col2', name: 'Column 2', type: 'number', index: 1 }, + { id: 'col1', name: 'Column1', type: 'string', index: 0 }, + { id: 'col2', name: 'Column2', type: 'number', index: 1 }, ], createdAt: '2024-01-01T00:00:00.000Z', updatedAt: '2024-01-02T00:00:00.000Z', @@ -3052,12 +3052,12 @@ describe('SourceControlImportService', () => { id: 'dt1', name: 'Old Name', projectId: 'project1', - columns: [{ id: 'col1', name: 'Column 1' }], + columns: [{ id: 'col1', name: 'Column1' }], }; fsReadFile.mockResolvedValue(JSON.stringify(mockDataTable) as any); dataTableRepository.findOne.mockResolvedValue(existingTable as any); - dataTableColumnRepository.find.mockResolvedValue([{ id: 'col1', name: 'Column 1' }] as any); + dataTableColumnRepository.find.mockResolvedValue([{ id: 'col1', name: 'Column1' }] as any); dataTableColumnRepository.save.mockImplementation(async (col: any) => col); projectRepository.findOne.mockResolvedValue({ id: 'project1', type: 'team' } as any); @@ -3126,7 +3126,7 @@ describe('SourceControlImportService', () => { teamId: 'project1', teamName: 'Team Project 1', }, - columns: [{ id: 'col1', name: 'Column 1', type: 'string', index: 0 }], + columns: [{ id: 'col1', name: 'Column1', type: 'string', index: 0 }], createdAt: '2024-01-01T00:00:00.000Z', updatedAt: '2024-01-02T00:00:00.000Z', }; @@ -3136,16 +3136,16 @@ describe('SourceControlImportService', () => { name: 'Test Table', projectId: 'project1', columns: [ - { id: 'col1', name: 'Column 1' }, - { id: 'col2', name: 'Column 2' }, + { id: 'col1', name: 'Column1' }, + { id: 'col2', name: 'Column2' }, ], }; fsReadFile.mockResolvedValue(JSON.stringify(mockDataTable) as any); dataTableRepository.findOne.mockResolvedValue(existingTable as any); dataTableColumnRepository.find.mockResolvedValue([ - { id: 'col1', name: 'Column 1' }, - { id: 'col2', name: 'Column 2' }, + { id: 'col1', name: 'Column1' }, + { id: 'col2', name: 'Column2' }, ] as any); dataTableColumnRepository.save.mockResolvedValue({ id: 'col1' } as any); projectRepository.findOne.mockResolvedValue({ id: 'project1', type: 'team' } as any); @@ -3156,7 +3156,7 @@ describe('SourceControlImportService', () => { // Assert expect(dataTableDDLService.dropColumnFromTable).toHaveBeenCalledWith( 'dt1', - 'Column 2', + 'Column2', 'sqlite', expect.anything(), ); @@ -3183,7 +3183,7 @@ describe('SourceControlImportService', () => { teamId: 'project1', teamName: 'Team Project 1', }, - columns: [{ id: 'col1', name: 'Column 1', type: 'string', index: 0 }], + columns: [{ id: 'col1', name: 'Column1', type: 'string', index: 0 }], createdAt: '2024-01-01T00:00:00.000Z', updatedAt: '2024-01-02T00:00:00.000Z', }; @@ -3204,13 +3204,81 @@ describe('SourceControlImportService', () => { expect(dataTableRepository.upsert).not.toHaveBeenCalled(); }); + it('should skip columns with invalid names', async () => { + // Arrange + const mockDataTable = { + id: 'dt1', + name: 'Test Table', + ownedBy: { + type: 'team', + teamId: 'project1', + teamName: 'Team Project 1', + }, + columns: [ + { id: 'col1', name: 'validName', type: 'string', index: 0 }, + { + id: 'col2', + name: 'invalid" text); create INVALID table; --', + type: 'string', + index: 1, + }, + ], + createdAt: '2024-01-01T00:00:00.000Z', + updatedAt: '2024-01-02T00:00:00.000Z', + }; + + fsReadFile.mockResolvedValue(JSON.stringify(mockDataTable) as any); + dataTableRepository.findOne.mockResolvedValue(null); + dataTableColumnRepository.find.mockResolvedValue([]); + dataTableColumnRepository.save.mockImplementation(async (col: any) => col); + projectRepository.findOne.mockResolvedValue({ id: 'project1', type: 'team' } as any); + + // Act + await service.importDataTablesFromWorkFolder([mockCandidate], mockUser.id); + + // Assert + expect(dataTableDDLService.createTableWithColumns).toHaveBeenCalledWith( + 'dt1', + expect.arrayContaining([expect.objectContaining({ id: 'col1', name: 'validName' })]), + expect.anything(), + ); + const columns = (dataTableDDLService.createTableWithColumns as jest.Mock).mock.calls[0][1]; + expect(columns).not.toEqual( + expect.arrayContaining([expect.objectContaining({ id: 'col2' })]), + ); + }); + + it('should skip data tables with invalid IDs', async () => { + // Arrange + const mockDataTable = { + id: 'invalid"; create INVALID table;--', + name: 'Test Table', + ownedBy: { + type: 'team', + teamId: 'project1', + teamName: 'Team Project 1', + }, + columns: [{ id: 'col1', name: 'validName', type: 'string', index: 0 }], + createdAt: '2024-01-01T00:00:00.000Z', + updatedAt: '2024-01-02T00:00:00.000Z', + }; + + fsReadFile.mockResolvedValue(JSON.stringify(mockDataTable) as any); + + // Act + await service.importDataTablesFromWorkFolder([mockCandidate], mockUser.id); + + // Assert + expect(dataTableRepository.upsert).not.toHaveBeenCalled(); + }); + it('should not partially import when a name collision exists among multiple tables', async () => { // Arrange — two tables: dt1 is valid, dt2 has a name collision const validTable = { id: 'dt1', name: 'Valid Table', ownedBy: { type: 'team', teamId: 'project1', teamName: 'Team Project 1' }, - columns: [{ id: 'col1', name: 'Column 1', type: 'string', index: 0 }], + columns: [{ id: 'col1', name: 'Column1', type: 'string', index: 0 }], createdAt: '2024-01-01T00:00:00.000Z', updatedAt: '2024-01-02T00:00:00.000Z', }; @@ -3218,7 +3286,7 @@ describe('SourceControlImportService', () => { id: 'dt2', name: 'Colliding Table', ownedBy: { type: 'team', teamId: 'project1', teamName: 'Team Project 1' }, - columns: [{ id: 'col2', name: 'Column 2', type: 'string', index: 0 }], + columns: [{ id: 'col2', name: 'Column2', type: 'string', index: 0 }], createdAt: '2024-01-01T00:00:00.000Z', updatedAt: '2024-01-02T00:00:00.000Z', }; diff --git a/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts b/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts index 1aedd9785cd..6e14f4ce1a5 100644 --- a/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts +++ b/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts @@ -46,6 +46,7 @@ import { DataTableColumn } from '@/modules/data-table/data-table-column.entity'; import { DataTableColumnRepository } from '@/modules/data-table/data-table-column.repository'; import { DataTableDDLService } from '@/modules/data-table/data-table-ddl.service'; import { DataTableRepository } from '@/modules/data-table/data-table.repository'; +import { isValidColumnName, isValidDataTableId } from '@/modules/data-table/utils/sql-utils'; import { RedactionEnforcementService } from '@/modules/redaction/redaction-enforcement.service'; import { isUniqueConstraintError } from '@/response-helper'; import { TagService } from '@/services/tag.service'; @@ -1239,6 +1240,13 @@ export class SourceControlImportService { continue; } + if (!isValidDataTableId(dataTable.id)) { + this.logger.warn( + `Invalid data table ID "${dataTable.id}" in file ${candidate.file}. Skipping.`, + ); + continue; + } + let targetProject: Project | null = null; if (dataTable.ownedBy) { @@ -1354,6 +1362,13 @@ export class SourceControlImportService { // Upsert columns const columnEntities = []; for (const column of dataTable.columns) { + if (!isValidColumnName(column.name)) { + this.logger.warn( + `Invalid column name "${column.name}" in data table ${dataTable.name}. Skipping column.`, + ); + continue; + } + if (!isValidDataTableColumnType(column.type)) { this.logger.warn( `Invalid column type "${column.type}" in data table ${dataTable.name}, column ${column.name}. Skipping column.`, diff --git a/packages/cli/src/oauth/__tests__/oauth.service.test.ts b/packages/cli/src/oauth/__tests__/oauth.service.test.ts index 9fa6413d69f..fc86a206478 100644 --- a/packages/cli/src/oauth/__tests__/oauth.service.test.ts +++ b/packages/cli/src/oauth/__tests__/oauth.service.test.ts @@ -136,7 +136,7 @@ describe('OauthService', () => { enumerable: true, }); - const promise = service.getCredential(req); + const promise = service.getCredentialForUpdate(req); await expect(promise).rejects.toThrow(BadRequestError); await expect(promise).rejects.toThrow('Required credential ID is missing'); }); @@ -149,10 +149,10 @@ describe('OauthService', () => { credentialsFinderService.findCredentialForUser.mockResolvedValue(null); - await expect(service.getCredential(req)).rejects.toThrow(NotFoundError); + await expect(service.getCredentialForUpdate(req)).rejects.toThrow(NotFoundError); expect(logger.error).toHaveBeenCalledWith( 'OAuth credential authorization failed because the current user does not have the correct permissions', - { userId: '123' }, + { userId: '123', credentialId: 'credential-id' }, ); }); @@ -165,13 +165,13 @@ describe('OauthService', () => { credentialsFinderService.findCredentialForUser.mockResolvedValue(mockCredential); - const result = await service.getCredential(req); + const result = await service.getCredentialForUpdate(req); expect(result).toBe(mockCredential); expect(credentialsFinderService.findCredentialForUser).toHaveBeenCalledWith( 'credential-id', req.user, - ['credential:read'], + ['credential:update'], ); }); }); @@ -404,20 +404,28 @@ describe('OauthService', () => { }; const encodedState = Buffer.from(JSON.stringify(state)).toString('base64'); cipher.decryptV2.mockResolvedValue(JSON.stringify(csrfData)); + const mockCredential = mock({ id: 'credential-id' }); + credentialsFinderService.findCredentialForUser.mockResolvedValue(mockCredential); const req = mock({ user: mock({ id: 'user-id' }), }); - const result = await (service as any).decodeCsrfState(encodedState, req); + const [decodedState, credential] = await (service as any).decodeCsrfState(encodedState, req); expect(cipher.decryptV2).toHaveBeenCalledWith('encrypted-data'); - expect(result).toMatchObject({ + expect(decodedState).toMatchObject({ token: 'token', createdAt: timestamp, cid: 'credential-id', userId: 'user-id', origin: 'static-credential', }); + expect(credential).toBe(mockCredential); + expect(credentialsFinderService.findCredentialForUser).toHaveBeenCalledWith( + 'credential-id', + req.user, + ['credential:update'], + ); }); it('should throw error when state format is invalid', async () => { @@ -529,20 +537,24 @@ describe('OauthService', () => { }; const encodedState = Buffer.from(JSON.stringify(state)).toString('base64'); cipher.decryptV2.mockResolvedValue(JSON.stringify(csrfData)); + const mockCredential = mock({ id: 'credential-id' }); + credentialsRepository.findOneBy.mockResolvedValue(mockCredential as any); const req = mock({ user: mock({ id: 'user-id' }), }); - const result = await (service as any).decodeCsrfState(encodedState, req); + const [decodedState, credential] = await (service as any).decodeCsrfState(encodedState, req); - expect(result).toMatchObject({ + expect(decodedState).toMatchObject({ token: 'token', createdAt: timestamp, cid: 'credential-id', userId: 'different-user-id', origin: 'dynamic-credential', }); + expect(credential).toBe(mockCredential); expect(cipher.decryptV2).toHaveBeenCalledWith('encrypted-data'); + expect(credentialsFinderService.findCredentialForUser).not.toHaveBeenCalled(); }); it('should bypass user validation for dynamic-credential origin even when req.user is undefined', async () => { @@ -558,19 +570,22 @@ describe('OauthService', () => { }; const encodedState = Buffer.from(JSON.stringify(state)).toString('base64'); cipher.decryptV2.mockResolvedValue(JSON.stringify(csrfData)); + const mockCredential = mock({ id: 'credential-id' }); + credentialsRepository.findOneBy.mockResolvedValue(mockCredential as any); const req = mock({ user: undefined, }); - const result = await (service as any).decodeCsrfState(encodedState, req); + const [decodedState, credential] = await (service as any).decodeCsrfState(encodedState, req); - expect(result).toMatchObject({ + expect(decodedState).toMatchObject({ token: 'token', createdAt: timestamp, cid: 'credential-id', userId: 'user-id', origin: 'dynamic-credential', }); + expect(credential).toBe(mockCredential); expect(cipher.decryptV2).toHaveBeenCalledWith('encrypted-data'); }); @@ -760,7 +775,7 @@ describe('OauthService', () => { }); cipher.decryptV2.mockResolvedValue(JSON.stringify(csrfData)); - credentialsRepository.findOneBy.mockResolvedValue(mockCredential); + credentialsFinderService.findCredentialForUser.mockResolvedValue(mockCredential); jest.mocked(WorkflowExecuteAdditionalData.getBase).mockResolvedValue(mockAdditionalData); credentialsHelper.getDecrypted.mockResolvedValue(mockDecryptedData); credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue(mockOAuthCredentials); @@ -781,7 +796,7 @@ describe('OauthService', () => { }); }); - it('should throw UnexpectedError when credential is not found', async () => { + it('should throw NotFoundError when credential is not found', async () => { const csrfData = { cid: 'credential-id', userId: 'user-id', @@ -800,12 +815,10 @@ describe('OauthService', () => { user: mock({ id: 'user-id' }), }); - credentialsRepository.findOneBy.mockResolvedValue(null); + credentialsFinderService.findCredentialForUser.mockResolvedValue(null); - await expect(service.resolveCredential(req)).rejects.toThrow(UnexpectedError); - await expect(service.resolveCredential(req)).rejects.toThrow( - 'OAuth callback failed because of insufficient permissions', - ); + await expect(service.resolveCredential(req)).rejects.toThrow(NotFoundError); + await expect(service.resolveCredential(req)).rejects.toThrow('Credential not found'); }); it('should throw UnexpectedError when CSRF state is invalid', async () => { @@ -832,7 +845,7 @@ describe('OauthService', () => { user: mock({ id: 'user-id' }), }); - credentialsRepository.findOneBy.mockResolvedValue(mockCredential); + credentialsFinderService.findCredentialForUser.mockResolvedValue(mockCredential); jest.mocked(WorkflowExecuteAdditionalData.getBase).mockResolvedValue(mockAdditionalData); credentialsHelper.getDecrypted.mockResolvedValue(mockDecryptedData); credentialsHelper.applyDefaultsAndOverwrites.mockResolvedValue(mockOAuthCredentials); diff --git a/packages/cli/src/oauth/oauth.service.ts b/packages/cli/src/oauth/oauth.service.ts index aea2562ed13..3e22f86fcb3 100644 --- a/packages/cli/src/oauth/oauth.service.ts +++ b/packages/cli/src/oauth/oauth.service.ts @@ -90,7 +90,7 @@ export class OauthService { return `${restUrl}/oauth${oauthVersion}-credential`; } - async getCredential( + async getCredentialForUpdate( req: OAuthRequest.OAuth1Credential.Auth | OAuthRequest.OAuth2Credential.Auth, ): Promise { const { id: credentialId } = req.query; @@ -102,13 +102,13 @@ export class OauthService { const credential = await this.credentialsFinderService.findCredentialForUser( credentialId, req.user, - ['credential:read'], + ['credential:update'], ); if (!credential) { this.logger.error( 'OAuth credential authorization failed because the current user does not have the correct permissions', - { userId: req.user.id }, + { userId: req.user.id, credentialId }, ); throw new NotFoundError(RESPONSE_ERROR_MESSAGES.NO_CREDENTIAL); } @@ -250,7 +250,7 @@ export class OauthService { protected async decodeCsrfState( encodedState: string, req: AuthenticatedRequest, - ): Promise { + ): Promise<[CsrfState & CreateCsrfStateData, CredentialsEntity | null]> { const errorMessage = 'Invalid state format'; const decodedState = Buffer.from(encodedState, 'base64').toString(); const decoded = jsonParse(decodedState, { @@ -270,28 +270,31 @@ export class OauthService { // Dynamic credentials: skip user validation (e.g. embed/iframe flows) as they do not contain an n8n user if (decryptedState.origin === 'dynamic-credential') { - return { - ...decoded, - ...decryptedState, - }; + return [ + { ...decoded, ...decryptedState }, + await this.getCredentialWithoutUser(decryptedState.cid), + ]; } // Static credentials: skip user validation only when N8N_SKIP_AUTH_ON_OAUTH_CALLBACK is true (e.g. embed/iframe) if (skipAuthOnOAuthCallback) { - return { - ...decoded, - ...decryptedState, - }; + return [ + { ...decoded, ...decryptedState }, + await this.getCredentialWithoutUser(decryptedState.cid), + ]; } if (req.user?.id === undefined || decryptedState.userId !== req.user.id) { throw new AuthError('Unauthorized'); } - return { - ...decoded, - ...decryptedState, - }; + const credential = await this.credentialsFinderService.findCredentialForUser( + decryptedState.cid, + req.user, + ['credential:update'], + ); + + return [{ ...decoded, ...decryptedState }, credential]; } protected verifyCsrfState( @@ -313,10 +316,9 @@ export class OauthService { [CredentialsEntity, ICredentialDataDecryptedObject, T, CsrfState & CreateCsrfStateData] > { const { state: encodedState } = req.query; - const state = await this.decodeCsrfState(encodedState, req); - const credential = await this.getCredentialWithoutUser(state.cid); + const [state, credential] = await this.decodeCsrfState(encodedState, req); if (!credential) { - throw new UnexpectedError('OAuth callback failed because of insufficient permissions'); + throw new NotFoundError(RESPONSE_ERROR_MESSAGES.NO_CREDENTIAL); } const additionalData = await this.getAdditionalData(); diff --git a/packages/cli/src/task-runners/__tests__/task-runner-process-py.test.ts b/packages/cli/src/task-runners/__tests__/task-runner-process-py.test.ts new file mode 100644 index 00000000000..a15bf677be6 --- /dev/null +++ b/packages/cli/src/task-runners/__tests__/task-runner-process-py.test.ts @@ -0,0 +1,82 @@ +import { Logger } from '@n8n/backend-common'; +import { mockInstance } from '@n8n/backend-test-utils'; +import { TaskRunnersConfig } from '@n8n/config'; +import { mock } from 'jest-mock-extended'; +import type { ChildProcess, SpawnOptions } from 'node:child_process'; + +import type { TaskBrokerAuthService } from '@/task-runners/task-broker/auth/task-broker-auth.service'; +import { PyTaskRunnerProcess } from '@/task-runners/task-runner-process-py'; + +const spawnMock = jest.fn(() => + mock({ + stdout: { + pipe: jest.fn(), + }, + stderr: { + pipe: jest.fn(), + }, + }), +); +require('child_process').spawn = spawnMock; + +describe('PyTaskRunnerProcess', () => { + const logger = mockInstance(Logger); + const runnerConfig = mockInstance(TaskRunnersConfig); + runnerConfig.mode = 'internal'; + const authService = mock(); + let taskRunnerProcess = new PyTaskRunnerProcess(logger, runnerConfig, authService, mock()); + + afterEach(() => { + spawnMock.mockClear(); + }); + + describe('start', () => { + beforeEach(() => { + taskRunnerProcess = new PyTaskRunnerProcess(logger, runnerConfig, authService, mock()); + jest.spyOn(authService, 'createGrantToken').mockResolvedValue('grantToken'); + }); + + test.each([ + 'PATH', + 'N8N_RUNNERS_STDLIB_ALLOW', + 'N8N_RUNNERS_EXTERNAL_ALLOW', + 'N8N_RUNNERS_BUILTINS_DENY', + 'N8N_BLOCK_RUNNER_ENV_ACCESS', + ])('should propagate %s from env as is', async (envVar) => { + process.env[envVar] = 'custom value'; + + await taskRunnerProcess.start(); + + // @ts-expect-error The type is not correct + const options = spawnMock.mock.calls[0][2] as SpawnOptions; + expect(options.env).toEqual( + expect.objectContaining({ + [envVar]: 'custom value', + }), + ); + }); + + it('should build env with a null prototype', async () => { + await taskRunnerProcess.start(); + + // @ts-expect-error The type is not correct + const options = spawnMock.mock.calls[0][2] as SpawnOptions; + expect(Object.getPrototypeOf(options.env)).toBeNull(); + }); + + it('should not inherit env keys from Object.prototype', async () => { + const proto = Object.prototype as Record; + proto.NODE_OPTIONS = '--inherited-value'; + + try { + await taskRunnerProcess.start(); + + // @ts-expect-error The type is not correct + const options = spawnMock.mock.calls[0][2] as SpawnOptions; + expect(options.env?.NODE_OPTIONS).toBeUndefined(); + } finally { + delete proto.NODE_OPTIONS; + } + }); + }); +}); diff --git a/packages/cli/src/task-runners/__tests__/task-runner-process.test.ts b/packages/cli/src/task-runners/__tests__/task-runner-process.test.ts index c6011198b55..c409b10f9c0 100644 --- a/packages/cli/src/task-runners/__tests__/task-runner-process.test.ts +++ b/packages/cli/src/task-runners/__tests__/task-runner-process.test.ts @@ -120,6 +120,33 @@ describe('TaskRunnerProcess', () => { expect(options.env).not.toHaveProperty('NODE_OPTIONS'); }); + it('should build env with a null prototype', async () => { + jest.spyOn(authService, 'createGrantToken').mockResolvedValue('grantToken'); + + await taskRunnerProcess.start(); + + // @ts-expect-error The type is not correct + const options = spawnMock.mock.calls[0][2] as SpawnOptions; + expect(Object.getPrototypeOf(options.env)).toBeNull(); + }); + + it('should not inherit env keys from Object.prototype', async () => { + jest.spyOn(authService, 'createGrantToken').mockResolvedValue('grantToken'); + runnerConfig.maxOldSpaceSize = ''; + const proto = Object.prototype as Record; + proto.NODE_OPTIONS = '--inherited-value'; + + try { + await taskRunnerProcess.start(); + + // @ts-expect-error The type is not correct + const options = spawnMock.mock.calls[0][2] as SpawnOptions; + expect(options.env?.NODE_OPTIONS).toBeUndefined(); + } finally { + delete proto.NODE_OPTIONS; + } + }); + it('should pass N8N_RUNNERS_TASK_TIMEOUT if set', async () => { jest.spyOn(authService, 'createGrantToken').mockResolvedValue('grantToken'); runnerConfig.taskTimeout = 123; diff --git a/packages/cli/src/task-runners/task-runner-process-js.ts b/packages/cli/src/task-runners/task-runner-process-js.ts index 54b7181074a..69e1e145654 100644 --- a/packages/cli/src/task-runners/task-runner-process-js.ts +++ b/packages/cli/src/task-runners/task-runner-process-js.ts @@ -53,7 +53,7 @@ export class JsTaskRunnerProcess extends TaskRunnerProcessBase { } private getProcessEnvVars(grantToken: string, taskBrokerUri: string) { - const envVars: Record = { + const envVars: Record = Object.assign(Object.create(null), { // system environment PATH: process.env.PATH, HOME: process.env.HOME ?? process.env.USERPROFILE, @@ -78,7 +78,7 @@ export class JsTaskRunnerProcess extends TaskRunnerProcessBase { N8N_RUNNERS_TASK_TIMEOUT: this.runnerConfig.taskTimeout.toString(), N8N_RUNNERS_HEARTBEAT_INTERVAL: this.runnerConfig.heartbeatInterval.toString(), N8N_RUNNERS_INSECURE_MODE: process.env.N8N_RUNNERS_INSECURE_MODE, - }; + }); if (this.runnerConfig.maxOldSpaceSize) { envVars.NODE_OPTIONS = `--max-old-space-size=${this.runnerConfig.maxOldSpaceSize}`; diff --git a/packages/cli/src/task-runners/task-runner-process-py.ts b/packages/cli/src/task-runners/task-runner-process-py.ts index a60c636d02e..ab02c9edbc7 100644 --- a/packages/cli/src/task-runners/task-runner-process-py.ts +++ b/packages/cli/src/task-runners/task-runner-process-py.ts @@ -37,7 +37,7 @@ export class PyTaskRunnerProcess extends TaskRunnerProcessBase { return spawn(venvPath, ['-m', 'src.main'], { cwd: pythonDir, - env: { + env: Object.assign(Object.create(null), { // system environment PATH: process.env.PATH, HOME: process.env.HOME ?? process.env.USERPROFILE, @@ -55,7 +55,7 @@ export class PyTaskRunnerProcess extends TaskRunnerProcessBase { N8N_RUNNERS_EXTERNAL_ALLOW: process.env.N8N_RUNNERS_EXTERNAL_ALLOW, N8N_RUNNERS_BUILTINS_DENY: process.env.N8N_RUNNERS_BUILTINS_DENY, N8N_BLOCK_RUNNER_ENV_ACCESS: process.env.N8N_BLOCK_RUNNER_ENV_ACCESS, - }, + }), }); } diff --git a/packages/cli/test/integration/controllers/oauth/oauth1.api.test.ts b/packages/cli/test/integration/controllers/oauth/oauth1.api.test.ts new file mode 100644 index 00000000000..2366acfbafc --- /dev/null +++ b/packages/cli/test/integration/controllers/oauth/oauth1.api.test.ts @@ -0,0 +1,187 @@ +import { createTeamProject, linkUserToProject, testDb } from '@n8n/backend-test-utils'; +import type { CredentialsEntity, User } from '@n8n/db'; +import { Container } from '@n8n/di'; +import { response as Response } from 'express'; +import nock from 'nock'; + +import { CredentialsHelper } from '@/credentials-helper'; +import { OauthService } from '@/oauth/oauth.service'; +import { + decryptCredentialData, + getCredentialById, + saveCredential, + shareCredentialWithUsers, +} from '@test-integration/db/credentials'; +import { createMember, createOwner } from '@test-integration/db/users'; +import { setupTestServer } from '@test-integration/utils'; + +describe('OAuth1 API', () => { + const testServer = setupTestServer({ endpointGroups: ['oauth1'] }); + + let owner: User; + let credential: CredentialsEntity; + const credentialData = { + consumerKey: 'consumer_key', + consumerSecret: 'consumer_secret', + authUrl: 'https://test.domain/oauth1/auth', + requestTokenUrl: 'https://test.domain/oauth1/request_token', + accessTokenUrl: 'https://test.domain/oauth1/access_token', + signatureMethod: 'HMAC-SHA1' as const, + }; + + const sharedCredentialPayload = { + name: 'Shared OAuth1 credential', + type: 'testOAuth1Api', + data: credentialData, + }; + + CredentialsHelper.prototype.applyDefaultsAndOverwrites = async (_, decryptedDataOriginal) => + decryptedDataOriginal; + + beforeAll(async () => { + owner = await createOwner(); + }); + + beforeEach(async () => { + await testDb.truncate(['SharedCredentials', 'CredentialsEntity']); + credential = await saveCredential(sharedCredentialPayload, { + user: owner, + role: 'credential:owner', + }); + }); + + afterEach(() => { + jest.restoreAllMocks(); + nock.cleanAll(); + }); + + describe('OAuth reconnect authorization', () => { + const expectNoCsrfStateOnCredential = async (credentialId: string) => { + const stored = await getCredentialById(credentialId); + expect(stored).not.toBeNull(); + const decrypted = (await decryptCredentialData(stored!)) as Record; + expect(decrypted).not.toHaveProperty('csrfSecret'); + }; + + const mockRequestTokenEndpoint = () => { + nock('https://test.domain') + .post('/oauth1/request_token') + .reply(200, 'oauth_token=request_token&oauth_token_secret=request_secret'); + }; + + it('should reject auth start for a sharee with credential:user role', async () => { + const sharee = await createMember(); + await shareCredentialWithUsers(credential, [sharee]); + + const response = await testServer + .authAgentFor(sharee) + .get('/oauth1-credential/auth') + .query({ id: credential.id }); + + expect(response.statusCode).toBe(404); + await expectNoCsrfStateOnCredential(credential.id); + }); + + it('should reject auth start for a project viewer on a project-shared credential', async () => { + const projectViewer = await createMember(); + const teamProject = await createTeamProject(undefined, owner); + await linkUserToProject(projectViewer, teamProject, 'project:viewer'); + + const projectCredential = await saveCredential(sharedCredentialPayload, { + project: teamProject, + role: 'credential:owner', + }); + + const response = await testServer + .authAgentFor(projectViewer) + .get('/oauth1-credential/auth') + .query({ id: projectCredential.id }); + + expect(response.statusCode).toBe(404); + await expectNoCsrfStateOnCredential(projectCredential.id); + }); + + it('should allow auth start for a project editor on a project-shared credential', async () => { + const projectEditor = await createMember(); + const teamProject = await createTeamProject(undefined, owner); + await linkUserToProject(projectEditor, teamProject, 'project:editor'); + + const projectCredential = await saveCredential(sharedCredentialPayload, { + project: teamProject, + role: 'credential:owner', + }); + + mockRequestTokenEndpoint(); + + const response = await testServer + .authAgentFor(projectEditor) + .get('/oauth1-credential/auth') + .query({ id: projectCredential.id }); + + expect(response.statusCode).toBe(200); + expect(response.body.data).toContain('https://test.domain/oauth1/auth'); + expect(response.body.data).toContain('oauth_token=request_token'); + }); + + it('should reject callback when requester lacks credential:update on the target credential', async () => { + const sharee = await createMember(); + await shareCredentialWithUsers(credential, [sharee]); + + const oauthService = Container.get(OauthService); + const renderSpy = jest.spyOn(Response, 'render').mockImplementation(function (this: any) { + this.end(); + return this; + }); + + // Build a callback state whose decrypted userId equals the requesting member, + // so the userId equality check inside decodeCsrfState passes and the credential + // scope check is the only remaining gate. The owner-initiated /auth call below + // produces a valid encrypted state; we then re-encrypt its contents with the + // member's userId before driving the callback as the member. + const csrfSpy = jest.spyOn(oauthService, 'createCsrfState').mockClear(); + mockRequestTokenEndpoint(); + + await testServer + .authAgentFor(owner) + .get('/oauth1-credential/auth') + .query({ id: credential.id }) + .expect(200); + + const [, ownerState] = await csrfSpy.mock.results[0].value; + + const decoded = JSON.parse(Buffer.from(ownerState, 'base64').toString()); + const decryptedData = JSON.parse(oauthService['cipher'].decrypt(decoded.data)) as Record< + string, + unknown + >; + decryptedData.userId = sharee.id; + decoded.data = oauthService['cipher'].encrypt(JSON.stringify(decryptedData)); + const reencodedState = Buffer.from(JSON.stringify(decoded)).toString('base64'); + + nock('https://test.domain') + .post('/oauth1/access_token') + .reply(200, 'oauth_token=member_token&oauth_token_secret=member_secret'); + + await testServer + .authAgentFor(sharee) + .get('/oauth1-credential/callback') + .query({ + oauth_token: 'request_token', + oauth_verifier: 'verifier', + state: reencodedState, + }) + .expect(200); + + expect(renderSpy).toHaveBeenCalledWith('oauth-error-callback', { + error: { message: 'Credential not found' }, + }); + + const stored = await Container.get(CredentialsHelper).getCredentials( + credential, + credential.type, + ); + const credentials = await stored.getData(); + expect(credentials.oauthTokenData).toBeUndefined(); + }); + }); +}); diff --git a/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts b/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts index ef4642e17dc..083b07dd9ee 100644 --- a/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts +++ b/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts @@ -1,4 +1,4 @@ -import { testDb } from '@n8n/backend-test-utils'; +import { createTeamProject, linkUserToProject, testDb } from '@n8n/backend-test-utils'; import type { CredentialsEntity, User } from '@n8n/db'; import { Container } from '@n8n/di'; import { response as Response } from 'express'; @@ -8,7 +8,12 @@ import { parse as parseQs } from 'querystring'; import { CredentialsHelper } from '@/credentials-helper'; import { ExternalHooks } from '@/external-hooks'; import { OauthService } from '@/oauth/oauth.service'; -import { saveCredential } from '@test-integration/db/credentials'; +import { + decryptCredentialData, + getCredentialById, + saveCredential, + shareCredentialWithUsers, +} from '@test-integration/db/credentials'; import { createMember, createOwner } from '@test-integration/db/users'; import type { SuperAgentTest } from '@test-integration/types'; import { setupTestServer } from '@test-integration/utils'; @@ -52,6 +57,10 @@ describe('OAuth2 API', () => { ); }); + afterEach(() => { + jest.restoreAllMocks(); + }); + it('should return a valid auth URL when the auth flow is initiated', async () => { const response = await ownerAgent .get('/oauth2-credential/auth') @@ -131,9 +140,10 @@ describe('OAuth2 API', () => { it('should fail on auth when callback is called as another user', async () => { const oauthService = Container.get(OauthService); const csrfSpy = jest.spyOn(oauthService, 'createCsrfState').mockClear(); - const renderSpy = (Response.render = jest.fn(function () { + const renderSpy = jest.spyOn(Response, 'render').mockImplementation(function (this: any) { this.end(); - })); + return this; + }); await ownerAgent.get('/oauth2-credential/auth').query({ id: credential.id }).expect(200); @@ -153,9 +163,10 @@ describe('OAuth2 API', () => { it('should handle a valid callback without auth', async () => { const oauthService = Container.get(OauthService); const csrfSpy = jest.spyOn(oauthService, 'createCsrfState').mockClear(); - const renderSpy = (Response.render = jest.fn(function () { + const renderSpy = jest.spyOn(Response, 'render').mockImplementation(function (this: any) { this.end(); - })); + return this; + }); await ownerAgent.get('/oauth2-credential/auth').query({ id: credential.id }).expect(200); @@ -179,4 +190,124 @@ describe('OAuth2 API', () => { oauthTokenData: { access_token: 'updated_token' }, }); }); + + describe('OAuth reconnect authorization', () => { + const sharedCredentialPayload = { + name: 'Shared OAuth2 credential', + type: 'testOAuth2Api', + data: credentialData, + }; + + const expectNoCsrfStateOnCredential = async (credentialId: string) => { + const stored = await getCredentialById(credentialId); + expect(stored).not.toBeNull(); + const decrypted = (await decryptCredentialData(stored!)) as Record; + expect(decrypted).not.toHaveProperty('csrfSecret'); + expect(decrypted).not.toHaveProperty('codeVerifier'); + }; + + it('should reject auth start for a sharee with credential:user role', async () => { + const sharee = await createMember(); + await shareCredentialWithUsers(credential, [sharee]); + + const response = await testServer + .authAgentFor(sharee) + .get('/oauth2-credential/auth') + .query({ id: credential.id }); + + expect(response.statusCode).toBe(404); + await expectNoCsrfStateOnCredential(credential.id); + }); + + it('should reject auth start for a project viewer on a project-shared credential', async () => { + const projectViewer = await createMember(); + const teamProject = await createTeamProject(undefined, owner); + await linkUserToProject(projectViewer, teamProject, 'project:viewer'); + + const projectCredential = await saveCredential(sharedCredentialPayload, { + project: teamProject, + role: 'credential:owner', + }); + + const response = await testServer + .authAgentFor(projectViewer) + .get('/oauth2-credential/auth') + .query({ id: projectCredential.id }); + + expect(response.statusCode).toBe(404); + await expectNoCsrfStateOnCredential(projectCredential.id); + }); + + it('should allow auth start for a project editor on a project-shared credential', async () => { + const projectEditor = await createMember(); + const teamProject = await createTeamProject(undefined, owner); + await linkUserToProject(projectEditor, teamProject, 'project:editor'); + + const projectCredential = await saveCredential(sharedCredentialPayload, { + project: teamProject, + role: 'credential:owner', + }); + + const response = await testServer + .authAgentFor(projectEditor) + .get('/oauth2-credential/auth') + .query({ id: projectCredential.id }); + + expect(response.statusCode).toBe(200); + expect(response.body.data).toContain('https://test.domain/oauth2/auth'); + }); + + it('should reject callback when requester lacks credential:update on the target credential', async () => { + const sharee = await createMember(); + await shareCredentialWithUsers(credential, [sharee]); + + const oauthService = Container.get(OauthService); + const renderSpy = (Response.render = jest.fn(function () { + this.end(); + })); + + // Build a callback state whose decrypted userId equals the requesting member, + // so the userId equality check inside decodeCsrfState passes and the credential + // scope check is the only remaining gate. The owner-initiated /auth call below + // produces a valid encrypted state; we then re-encrypt its contents with the + // member's userId before driving the callback as the member. + const ownerAgentForSetup = testServer.authAgentFor(owner); + const csrfSpy = jest.spyOn(oauthService, 'createCsrfState').mockClear(); + await ownerAgentForSetup + .get('/oauth2-credential/auth') + .query({ id: credential.id }) + .expect(200); + const [, ownerState] = await csrfSpy.mock.results[0].value; + + const decoded = JSON.parse(Buffer.from(ownerState, 'base64').toString()); + const decryptedData = JSON.parse(oauthService['cipher'].decrypt(decoded.data)) as Record< + string, + unknown + >; + decryptedData.userId = sharee.id; + decoded.data = oauthService['cipher'].encrypt(JSON.stringify(decryptedData)); + const reencodedState = Buffer.from(JSON.stringify(decoded)).toString('base64'); + + nock('https://test.domain') + .post('/oauth2/token') + .reply(200, { access_token: 'member_token' }); + + await testServer + .authAgentFor(sharee) + .get('/oauth2-credential/callback') + .query({ code: 'auth_code', state: reencodedState }) + .expect(200); + + expect(renderSpy).toHaveBeenCalledWith('oauth-error-callback', { + error: { message: 'Credential not found' }, + }); + + const updatedCredential = await Container.get(CredentialsHelper).getCredentials( + credential, + credential.type, + ); + const credentials = await updatedCredential.getData(); + expect(credentials.oauthTokenData).toBeUndefined(); + }); + }); }); diff --git a/packages/cli/test/integration/shared/types.ts b/packages/cli/test/integration/shared/types.ts index 6976ac7efe2..0250784d115 100644 --- a/packages/cli/test/integration/shared/types.ts +++ b/packages/cli/test/integration/shared/types.ts @@ -12,6 +12,7 @@ type EndpointGroup = | 'me' | 'users' | 'auth' + | 'oauth1' | 'oauth2' | 'owner' | 'passwordReset' diff --git a/packages/cli/test/integration/shared/utils/test-server.ts b/packages/cli/test/integration/shared/utils/test-server.ts index 54083cf9f17..e026f752457 100644 --- a/packages/cli/test/integration/shared/utils/test-server.ts +++ b/packages/cli/test/integration/shared/utils/test-server.ts @@ -223,6 +223,10 @@ export const setupTestServer = ({ await import('@/controllers/auth.controller'); break; + case 'oauth1': + await import('@/controllers/oauth/oauth1-credential.controller'); + break; + case 'oauth2': await import('@/controllers/oauth/oauth2-credential.controller'); break; diff --git a/packages/nodes-base/nodes/Git/Git.node.ts b/packages/nodes-base/nodes/Git/Git.node.ts index 7978cdc49ce..e2bd7d72ca9 100644 --- a/packages/nodes-base/nodes/Git/Git.node.ts +++ b/packages/nodes-base/nodes/Git/Git.node.ts @@ -343,11 +343,12 @@ export class Git implements INodeType { ...(!enableHooks && { unsafe: { allowUnsafeHooksPath: true } }), }; - const git: SimpleGit = simpleGit(gitOptions) - // Tell git not to ask for any information via the terminal like for - // example the username. As nobody will be able to answer it would - // n8n keep on waiting forever. - .env('GIT_TERMINAL_PROMPT', '0'); + const cleanEnv = Object.create(null) as Record; + // Tell git not to ask for any information via the terminal like for + // example the username. As nobody will be able to answer it would + // n8n keep on waiting forever. + cleanEnv['GIT_TERMINAL_PROMPT'] = '0'; + const git: SimpleGit = simpleGit(gitOptions).env(cleanEnv); if (operation === 'add') { // ---------------------------------- diff --git a/packages/nodes-base/nodes/Git/test/Git.node.test.ts b/packages/nodes-base/nodes/Git/test/Git.node.test.ts index 87c81c6b637..5a4ec7c413f 100644 --- a/packages/nodes-base/nodes/Git/test/Git.node.test.ts +++ b/packages/nodes-base/nodes/Git/test/Git.node.test.ts @@ -61,6 +61,32 @@ describe('Git Node', () => { jest.clearAllMocks(); }); + describe('Environment validation', () => { + it('should not include invalid inherited environment keys in simple-git env', async () => { + const inheritedEnvKey = 'N8N_TEST_INVALID_ENV_KEY'; + (Object.prototype as Record)[inheritedEnvKey] = 'ignored'; + + mockExecuteFunctions.getNodeParameter + .mockReturnValueOnce('log') + .mockReturnValueOnce('/repo') + .mockReturnValueOnce({}); + + mockGit.log.mockResolvedValueOnce({ all: [] } as any); + + try { + await gitNode.execute.call(mockExecuteFunctions); + } finally { + delete (Object.prototype as Record)[inheritedEnvKey]; + } + + expect(mockGit.env).toHaveBeenCalledTimes(1); + const envArg = mockGit.env.mock.calls[0][0] as Record; + expect(Object.prototype.hasOwnProperty.call(envArg, inheritedEnvKey)).toBe(false); + expect(envArg[inheritedEnvKey]).toBeUndefined(); + expect(envArg.GIT_TERMINAL_PROMPT).toBe('0'); + }); + }); + describe('Branch switching', () => { const mockNodeParameters = (params: Record) => { mockExecuteFunctions.getNodeParameter.mockImplementation( diff --git a/packages/nodes-base/nodes/HttpRequest/V3/HttpRequestV3.node.ts b/packages/nodes-base/nodes/HttpRequest/V3/HttpRequestV3.node.ts index 50bb4ded2dc..67ccfa831c7 100644 --- a/packages/nodes-base/nodes/HttpRequest/V3/HttpRequestV3.node.ts +++ b/packages/nodes-base/nodes/HttpRequest/V3/HttpRequestV3.node.ts @@ -24,6 +24,7 @@ import { removeCircularRefs, sleep, ensureError, + setSafeObjectProperty, } from 'n8n-workflow'; import type { Readable } from 'stream'; @@ -60,6 +61,10 @@ function isEmptyResponseBody(body: unknown): body is string { return typeof body === 'string' && body.trim().length === 0; } +function isPaginationRequestType(value: string): value is 'body' | 'headers' | 'qs' { + return value === 'body' || value === 'headers' || value === 'qs'; +} + export class HttpRequestV3 implements INodeType { description: INodeTypeDescription; @@ -639,13 +644,12 @@ export class HttpRequestV3 implements INodeType { const paginationData: PaginationOptions = { continue: continueExpression, - request: {}, + request: Object.create(null) as Record, requestInterval: pagination.requestInterval, }; if (pagination.paginationMode === 'updateAParameterInEachRequest') { // Iterate over all parameters and add them to the request - paginationData.request = {}; const { parameters } = pagination.parameters; if ( parameters.length === 1 && @@ -658,9 +662,6 @@ export class HttpRequestV3 implements INodeType { ); } pagination.parameters.parameters.forEach((parameter, index) => { - if (!paginationData.request[parameter.type]) { - paginationData.request[parameter.type] = {}; - } const parameterName = parameter.name; if (parameterName === '') { throw new NodeOperationError( @@ -668,6 +669,18 @@ export class HttpRequestV3 implements INodeType { `Parameter name must be set for parameter [${index + 1}] in pagination settings`, ); } + + if (!isPaginationRequestType(parameter.type)) { + throw new NodeOperationError( + this.getNode(), + `Parameter type must be one of: body, headers, qs for parameter [${ + index + 1 + }] in pagination settings`, + ); + } + + paginationData.request[parameter.type] ??= Object.create(null); + const parameterValue = parameter.value; if (parameterValue === '') { throw new NodeOperationError( @@ -677,7 +690,12 @@ export class HttpRequestV3 implements INodeType { }] in pagination settings, omitting it will result in an infinite loop`, ); } - paginationData.request[parameter.type]![parameterName] = parameterValue; + + setSafeObjectProperty( + paginationData.request[parameter.type]!, + parameterName, + parameterValue, + ); }); } else if (pagination.paginationMode === 'responseContainsNextURL') { paginationData.request.url = pagination.nextURL; diff --git a/packages/nodes-base/nodes/HttpRequest/test/node/HttpRequestV3.test.ts b/packages/nodes-base/nodes/HttpRequest/test/node/HttpRequestV3.test.ts index 582b63e585e..115332e4b2e 100644 --- a/packages/nodes-base/nodes/HttpRequest/test/node/HttpRequestV3.test.ts +++ b/packages/nodes-base/nodes/HttpRequest/test/node/HttpRequestV3.test.ts @@ -607,4 +607,156 @@ describe('HttpRequestV3', () => { ); }); }); + + describe('Pagination parameter validation', () => { + it('should keep valid pagination parameters and ignore invalid parameter names', async () => { + const paginationTestOptions = { + ...options, + response: { + response: { + neverError: false, + responseFormat: 'json', + fullResponse: false, + outputPropertyName: 'data', + }, + }, + }; + (executeFunctions.getInputData as jest.Mock).mockReturnValue([{ json: {} }]); + (executeFunctions.getNodeParameter as jest.Mock).mockImplementation( + (paramName: string, _itemIndex: number, defaultValue: unknown) => { + switch (paramName) { + case 'method': + return 'GET'; + case 'url': + return baseUrl; + case 'authentication': + return 'none'; + case 'options': + return paginationTestOptions; + case 'options.pagination.pagination': + return { + paginationMode: 'updateAParameterInEachRequest', + parameters: { + parameters: [ + { + type: 'qs', + // eslint-disable-next-line n8n-nodes-base/node-param-display-name-miscased + name: 'page', + value: '1', + }, + { + type: 'qs', + name: 'constructor', + value: 'ignored', + }, + ], + }, + paginationCompleteWhen: 'responseIsEmpty', + statusCodesWhenComplete: '', + completeExpression: '', + limitPagesFetched: false, + maxRequests: 10, + requestInterval: 0, + }; + case 'options.response.response.responseFormat': + return 'json'; + default: + return defaultValue; + } + }, + ); + (executeFunctions.helpers.requestWithAuthenticationPaginated as jest.Mock).mockResolvedValue([ + { + headers: { 'content-type': 'application/json' }, + body: { success: true }, + statusCode: 200, + }, + ]); + + const result = await node.execute.call(executeFunctions); + + expect(result).toEqual([[{ json: { success: true }, pairedItem: { item: 0 } }]]); + expect(executeFunctions.helpers.requestWithAuthenticationPaginated).toHaveBeenCalledTimes(1); + const paginationData = ( + executeFunctions.helpers.requestWithAuthenticationPaginated as jest.Mock + ).mock.calls[0][2] as { + request: { + qs: Record; + }; + }; + expect(paginationData.request.qs).toEqual({ page: '1' }); + expect(Object.prototype.hasOwnProperty.call(paginationData.request.qs, 'constructor')).toBe( + false, + ); + }); + + it('should reject invalid pagination parameter type values', async () => { + const paginationTestOptions = { + ...options, + response: { + response: { + neverError: false, + responseFormat: 'json', + fullResponse: false, + outputPropertyName: 'data', + }, + }, + }; + + (executeFunctions.getInputData as jest.Mock).mockReturnValue([{ json: {} }]); + (executeFunctions.helpers.requestWithAuthenticationPaginated as jest.Mock).mockResolvedValue([ + { + headers: { 'content-type': 'application/json' }, + body: { success: true }, + statusCode: 200, + }, + ]); + (executeFunctions.helpers.request as jest.Mock).mockResolvedValue({ + headers: { 'content-type': 'application/json' }, + body: Buffer.from(JSON.stringify({ success: true })), + }); + + (executeFunctions.getNodeParameter as jest.Mock).mockImplementation( + (paramName: string, _itemIndex: number, defaultValue: unknown) => { + switch (paramName) { + case 'method': + return 'GET'; + case 'url': + return baseUrl; + case 'authentication': + return 'none'; + case 'options': + return paginationTestOptions; + case 'options.pagination.pagination': + return { + paginationMode: 'updateAParameterInEachRequest', + parameters: { + parameters: [ + { + type: '__proto__' as unknown as 'qs', + // eslint-disable-next-line n8n-nodes-base/node-param-display-name-miscased + name: 'page', + value: '1', + }, + ], + }, + paginationCompleteWhen: 'responseIsEmpty', + statusCodesWhenComplete: '', + completeExpression: '', + limitPagesFetched: false, + maxRequests: 10, + requestInterval: 0, + }; + default: + return defaultValue; + } + }, + ); + + await expect(node.execute.call(executeFunctions)).rejects.toThrow( + 'Parameter type must be one of: body, headers, qs for parameter [1] in pagination settings', + ); + expect(executeFunctions.helpers.requestWithAuthenticationPaginated).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/nodes-base/nodes/Xml/Xml.node.ts b/packages/nodes-base/nodes/Xml/Xml.node.ts index 76322b46bec..b098dbc6c9f 100644 --- a/packages/nodes-base/nodes/Xml/Xml.node.ts +++ b/packages/nodes-base/nodes/Xml/Xml.node.ts @@ -236,19 +236,23 @@ export class Xml implements INodeType { const options = this.getNodeParameter('options', 0, {}); const forbiddenKeys = ['__proto__', 'constructor', 'prototype']; - const attrkey = String(options.attrkey ?? ''); - const charkey = String(options.charkey ?? ''); - if (forbiddenKeys.includes(attrkey)) { - throw new NodeOperationError( - this.getNode(), - `The "Attribute Key" option value "${attrkey}" is not allowed`, - ); + if (options.attrkey !== undefined) { + options.attrkey = String(options.attrkey); + if (forbiddenKeys.includes(options.attrkey)) { + throw new NodeOperationError( + this.getNode(), + `The "Attribute Key" option value "${options.attrkey}" is not allowed`, + ); + } } - if (forbiddenKeys.includes(charkey)) { - throw new NodeOperationError( - this.getNode(), - `The "Character Key" option value "${charkey}" is not allowed`, - ); + if (options.charkey !== undefined) { + options.charkey = String(options.charkey); + if (forbiddenKeys.includes(options.charkey)) { + throw new NodeOperationError( + this.getNode(), + `The "Character Key" option value "${options.charkey}" is not allowed`, + ); + } } let item: INodeExecutionData; diff --git a/packages/nodes-base/nodes/Xml/test/node/Xml.test.ts b/packages/nodes-base/nodes/Xml/test/node/Xml.test.ts index 0f8cc231d03..5102ed28d02 100644 --- a/packages/nodes-base/nodes/Xml/test/node/Xml.test.ts +++ b/packages/nodes-base/nodes/Xml/test/node/Xml.test.ts @@ -130,4 +130,104 @@ describe('Xml Node - options validation', () => { }, ); }); + + describe('default option behaviour', () => { + test('should use the parser default attribute key when attrkey option is not set', async () => { + setupGetNodeParameter('xmlToJson', { mergeAttrs: false, explicitArray: false }); + mockExecuteFunctions.getInputData.mockReturnValue([ + { json: { data: 'x' } }, + ]); + + const result = await xmlNode.execute.call(mockExecuteFunctions); + + const parsed = result[0][0].json as { root: Record }; + expect(parsed.root).toHaveProperty('$', { foo: 'bar' }); + }); + + test('should use the parser default character key when charkey option is not set', async () => { + setupGetNodeParameter('xmlToJson', { mergeAttrs: false, explicitArray: false }); + mockExecuteFunctions.getInputData.mockReturnValue([ + { json: { data: 'text' } }, + ]); + + const result = await xmlNode.execute.call(mockExecuteFunctions); + + const parsed = result[0][0].json as { root: Record }; + expect(parsed.root).toHaveProperty('_', 'text'); + }); + }); + + describe('attrkey and charkey are coerced to a stable string', () => { + test('should forward the coerced attrkey string to the parser in xmlToJson mode', async () => { + const statefulAttrkey = (() => { + let calls = 0; + return { + toString: () => (++calls === 1 ? 'safe' : 'changed'), + }; + })(); + + setupGetNodeParameter('xmlToJson', { + attrkey: statefulAttrkey, + mergeAttrs: false, + explicitArray: false, + }); + mockExecuteFunctions.getInputData.mockReturnValue([ + { json: { data: 'x' } }, + ]); + + const result = await xmlNode.execute.call(mockExecuteFunctions); + + const parsed = result[0][0].json as { root: Record }; + expect(parsed.root).toHaveProperty('safe', { foo: 'bar' }); + expect(parsed.root).not.toHaveProperty('changed'); + }); + + test('should forward the coerced charkey string to the parser in xmlToJson mode', async () => { + const statefulCharkey = (() => { + let calls = 0; + return { + toString: () => (++calls === 1 ? 'safe' : 'changed'), + }; + })(); + + setupGetNodeParameter('xmlToJson', { + charkey: statefulCharkey, + mergeAttrs: false, + explicitArray: false, + }); + mockExecuteFunctions.getInputData.mockReturnValue([ + { json: { data: 'text' } }, + ]); + + const result = await xmlNode.execute.call(mockExecuteFunctions); + + const parsed = result[0][0].json as { root: Record }; + expect(parsed.root).toHaveProperty('safe', 'text'); + expect(parsed.root).not.toHaveProperty('changed'); + }); + + test('should forward the coerced attrkey string to the builder in jsonToxml mode', async () => { + const statefulAttrkey = (() => { + let calls = 0; + return { + toString: () => (++calls === 1 ? 'safe' : 'changed'), + }; + })(); + + setupGetNodeParameter('jsonToxml', { + attrkey: statefulAttrkey, + headless: true, + }); + mockExecuteFunctions.getInputData.mockReturnValue([ + { json: { root: { safe: { foo: 'bar' } } } }, + ]); + + const result = await xmlNode.execute.call(mockExecuteFunctions); + + const xml = (result[0][0].json as { data: string }).data; + expect(xml).toContain('foo="bar"'); + expect(xml).not.toContain(' Date: Wed, 13 May 2026 08:09:39 +0200 Subject: [PATCH 02/21] feat(core): Export and import data table user-row contents (#29455) Co-authored-by: Claude Opus 4.7 (1M context) --- .../export/__tests__/entities.test.ts | 25 +++ packages/cli/src/commands/export/entities.ts | 11 +- .../src/modules/data-table/utils/sql-utils.ts | 45 ++++- .../services/__tests__/export.service.test.ts | 139 +++++++++++++- .../services/__tests__/import.service.test.ts | 145 +++++++++++++++ packages/cli/src/services/export.service.ts | 156 ++++++++++++++++ packages/cli/src/services/import.service.ts | 170 +++++++++++++----- 7 files changed, 632 insertions(+), 59 deletions(-) diff --git a/packages/cli/src/commands/export/__tests__/entities.test.ts b/packages/cli/src/commands/export/__tests__/entities.test.ts index 3b747d0ed8a..1f731c08663 100644 --- a/packages/cli/src/commands/export/__tests__/entities.test.ts +++ b/packages/cli/src/commands/export/__tests__/entities.test.ts @@ -32,6 +32,7 @@ describe('ExportEntitiesCommand', () => { 'execution_metadata', ]), undefined, + { includeDataTableRows: undefined }, ); }); @@ -53,6 +54,7 @@ describe('ExportEntitiesCommand', () => { './exports', new Set(), undefined, + { includeDataTableRows: undefined }, ); }); @@ -80,6 +82,29 @@ describe('ExportEntitiesCommand', () => { 'execution_metadata', ]), 'key.txt', + { includeDataTableRows: undefined }, + ); + }); + + it('should pass includeDataTableRows=false through to the export service', async () => { + const command = new ExportEntitiesCommand(); + // @ts-expect-error Protected property + command.flags = { + outputDir: './exports', + includeDataTableRows: false, + }; + // @ts-expect-error Protected property + command.logger = { + info: jest.fn(), + error: jest.fn(), + }; + await command.run(); + + expect(mockExportService.exportEntities).toHaveBeenCalledWith( + './exports', + expect.any(Set), + undefined, + { includeDataTableRows: false }, ); }); }); diff --git a/packages/cli/src/commands/export/entities.ts b/packages/cli/src/commands/export/entities.ts index e770ab3595c..4eee58f3e04 100644 --- a/packages/cli/src/commands/export/entities.ts +++ b/packages/cli/src/commands/export/entities.ts @@ -17,6 +17,12 @@ const flagsSchema = z.object({ 'Include execution history data tables, these are excluded by default as they can be very large', ) .default(false), + includeDataTableRows: z.coerce + .boolean() + .describe( + 'Include user-data table row contents. When false, only the data-table schemas (registry rows) are exported.', + ) + .default(true), keyFile: z .string() .describe('Optional path to a file containing a custom encryption key') @@ -31,6 +37,7 @@ const flagsSchema = z.object({ '--outputDir=./exports', '--outputDir=/path/to/backup', '--includeExecutionHistoryDataTables=true', + '--includeDataTableRows=false', '--keyFile=/path/to/key.txt', '--outputDir=./exports --keyFile=/path/to/key.txt', ], @@ -50,7 +57,9 @@ export class ExportEntitiesCommand extends BaseCommand(value: T): T | boolean { + if (typeof value === 'boolean') return value; + if (value === 1 || value === '1') return true; + if (value === 0 || value === '0') return false; + return value; +} + // Convert date objects or strings to dates in UTC -function normalizeDate(value: DataTableColumnJsType): Date | null { +export function normalizeDate(value: unknown): Date | null { if (value instanceof Date) return value; if (typeof value === 'string') { @@ -234,14 +242,7 @@ export function normalizeRows( const type = typeMap[key]; if (type === 'boolean') { - // Convert boolean values to true/false - if (typeof value === 'boolean') { - normalized[key] = value; - } else if (value === 1 || value === '1') { - normalized[key] = true; - } else if (value === 0 || value === '0') { - normalized[key] = false; - } + normalized[key] = normalizeBoolean(value); } if (type === 'date' && value !== null && value !== undefined) { @@ -301,6 +302,32 @@ export function normalizeValueForDatabase( return value; } +/** + * Normalize a raw value coming from a cross-database data-table export so it can + * be inserted into the destination database. Differs from `normalizeValueForDatabase` + * by also handling booleans (SQLite stores them as 0/1 INTEGER, Postgres as BOOLEAN) + * and SQLite-style date strings without timezone info (treated as UTC). + */ +export function normalizeUserRowValueForDatabase( + value: unknown, + columnType: string | undefined, + dbType?: DataSourceOptions['type'], +): unknown { + if (value === null || value === undefined) return null; + + if (columnType === 'boolean') { + return normalizeBoolean(value); + } + + if (columnType === 'date') { + const date = normalizeDate(value); + if (date === null) return value; + return normalizeValueForDatabase(date, columnType, dbType); + } + + return value; +} + /** * Convert a LIKE-style pattern (only % is wildcard) into a SQLite GLOB pattern. */ diff --git a/packages/cli/src/services/__tests__/export.service.test.ts b/packages/cli/src/services/__tests__/export.service.test.ts index 8fc34be8c67..eb72c54a42f 100644 --- a/packages/cli/src/services/__tests__/export.service.test.ts +++ b/packages/cli/src/services/__tests__/export.service.test.ts @@ -3,6 +3,7 @@ import { ExportService } from '../export.service'; import { type DataSource } from '@n8n/typeorm'; import { mkdir, rm, readdir, appendFile, readFile } from 'fs/promises'; import { mock } from 'jest-mock-extended'; +import type * as JestMockExtended from 'jest-mock-extended'; import type { Cipher } from 'n8n-core'; // Mock fs/promises with proper implementations @@ -25,9 +26,16 @@ jest.mock('@/utils/validate-database-type', () => ({ })); // Mock @n8n/db -jest.mock('@n8n/db', () => ({ - DataSource: mock(), -})); +jest.mock('@n8n/db', () => { + // `mock` from jest-mock-extended is not yet initialised when this hoisted + // factory runs, so resolve it lazily via require. + const jestMockExtended = require('jest-mock-extended') as typeof JestMockExtended; + return { + DataSource: jestMockExtended.mock(), + // `sql-utils` imports `DslColumn`; provide a no-op so the import resolves. + DslColumn: class {}, + }; +}); describe('ExportService', () => { let exportService: ExportService; @@ -182,7 +190,7 @@ describe('ExportService', () => { await exportService.exportEntities(outputDir); - expect(mockDataSource.query).toHaveBeenCalledTimes(5); // 1 migrations + 3 entity queries + expect(mockDataSource.query).toHaveBeenCalledTimes(6); // 2 migrations + 1 data_table + 3 entity queries expect(appendFile).toHaveBeenCalled(); }); @@ -210,10 +218,34 @@ describe('ExportService', () => { await exportService.exportEntities(outputDir, new Set(['execution_data'])); - expect(mockDataSource.query).toHaveBeenCalledTimes(4); // 1 migrations + 3 entity queries + expect(mockDataSource.query).toHaveBeenCalledTimes(5); // 2 migrations + 1 data_table + 2 entity queries expect(appendFile).toHaveBeenCalled(); }); + it('should skip data-table row export when includeDataTableRows is false', async () => { + const outputDir = '/test/output'; + + jest.mocked(mockDataSource.query).mockImplementation(async () => []); + jest.mocked(readdir).mockResolvedValue([]); + + await exportService.exportEntities(outputDir, undefined, undefined, { + includeDataTableRows: false, + }); + + // Without the data_table SELECT id query, query count drops by 1 vs the + // includeDataTableRows=true path. Verify by ensuring no data_table query was + // issued and the schema-only log message was emitted. + const dataTableQueries = jest + .mocked(mockDataSource.query) + .mock.calls.filter( + ([sql]) => typeof sql === 'string' && /SELECT id FROM "data_table"/.test(sql), + ); + expect(dataTableQueries).toHaveLength(0); + expect(mockLogger.info).toHaveBeenCalledWith( + expect.stringContaining('Skipping data-table row export'), + ); + }); + it('should clear existing files before export', async () => { const outputDir = '/test/output'; const existingFiles = ['user.jsonl', 'user.2.jsonl', 'other.txt']; @@ -338,6 +370,103 @@ describe('ExportService', () => { }); }); + describe('exportDataTableUserTables', () => { + it('should silently skip when the data_table registry is missing', async () => { + jest.mocked(mockDataSource.query).mockImplementation(async (query: string) => { + if (query.includes('data_table') && !query.includes('data_table_column')) { + throw new Error('relation does not exist'); + } + return []; + }); + + // @ts-expect-error accessing private method for testing + const result = await exportService.exportDataTableUserTables('/test/output'); + + expect(result).toEqual({ totalTables: 0, totalRows: 0 }); + }); + + it('should skip when there are no data tables', async () => { + jest.mocked(mockDataSource.query).mockResolvedValueOnce([]); // SELECT id FROM data_table -> empty + + // @ts-expect-error accessing private method for testing + const result = await exportService.exportDataTableUserTables('/test/output'); + + expect(result).toEqual({ totalTables: 0, totalRows: 0 }); + expect(appendFile).not.toHaveBeenCalled(); + }); + + it('should keyset-paginate user rows by id and write JSONL files', async () => { + const dataTableId = 'abc'; + + const firstPage = Array.from({ length: 500 }, (_, i) => ({ + id: i + 1, + createdAt: '2024-01-01 00:00:00', + updatedAt: '2024-01-01 00:00:00', + val: i, + })); + const secondPage = [ + { + id: 501, + createdAt: '2024-01-01 00:00:00', + updatedAt: '2024-01-01 00:00:00', + val: 500, + }, + ]; + + let queryCount = 0; + jest.mocked(mockDataSource.query).mockImplementation(async (query: string) => { + queryCount++; + if (query.includes('FROM') && query.includes('data_table_column')) { + return [{ dataTableId, name: 'val', type: 'number', index: 0 }]; + } + if (query.startsWith('SELECT id FROM') && query.includes('data_table')) { + return [{ id: dataTableId }]; + } + if (query.includes('data_table_user_')) { + if (query.includes('WHERE "id" > 0')) return firstPage; + if (query.includes('WHERE "id" > 500')) return secondPage; + return []; + } + return []; + }); + + jest.mocked(readdir).mockResolvedValue([]); + + // @ts-expect-error accessing private method for testing + const result = await exportService.exportDataTableUserTables('/test/output'); + + expect(result.totalTables).toBe(1); + expect(result.totalRows).toBe(501); + // First file appended once for first page; second page rolls into a new file + expect(appendFile).toHaveBeenCalledTimes(2); + // 1 list-tables + 2 page queries — second page has < pageSize + // rows, so the loop short-circuits without a terminator query. + expect(queryCount).toBe(3); + }); + + it('should handle a missing dynamic table gracefully without aborting', async () => { + jest.mocked(mockDataSource.query).mockImplementation(async (query: string) => { + if (query.startsWith('SELECT id FROM') && query.includes('data_table')) { + return [{ id: 'broken' }]; + } + if (query.includes('data_table_column')) return []; + if (query.includes('data_table_user_')) { + throw new Error('no such table'); + } + return []; + }); + + // @ts-expect-error accessing private method for testing + const result = await exportService.exportDataTableUserTables('/test/output'); + + expect(result).toEqual({ totalTables: 1, totalRows: 0 }); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('Could not read rows'), + expect.objectContaining({ error: expect.any(Error) }), + ); + }); + }); + describe('exportMigrationsTable', () => { it('should export migrations table when it has data', async () => { const outputDir = '/test/output'; diff --git a/packages/cli/src/services/__tests__/import.service.test.ts b/packages/cli/src/services/__tests__/import.service.test.ts index 852758bb9fc..5c6a19f8acc 100644 --- a/packages/cli/src/services/__tests__/import.service.test.ts +++ b/packages/cli/src/services/__tests__/import.service.test.ts @@ -281,6 +281,7 @@ describe('ImportService', () => { workflowentity: ['/test/input/workflowentity.jsonl'], }, tableNames: ['user', 'workflow_entity'], + dataTableFiles: {}, }); }); @@ -301,6 +302,7 @@ describe('ImportService', () => { user: ['/test/input/user.jsonl', '/test/input/user.2.jsonl', '/test/input/user.3.jsonl'], }, tableNames: ['user'], + dataTableFiles: {}, }); }); @@ -314,6 +316,7 @@ describe('ImportService', () => { expect(result).toEqual({ entityFiles: {}, tableNames: [], + dataTableFiles: {}, }); }); @@ -325,6 +328,7 @@ describe('ImportService', () => { expect(result).toEqual({ entityFiles: {}, tableNames: [], + dataTableFiles: {}, }); }); @@ -338,6 +342,7 @@ describe('ImportService', () => { expect(result).toEqual({ entityFiles: {}, tableNames: [], + dataTableFiles: {}, }); }); @@ -355,6 +360,37 @@ describe('ImportService', () => { user: ['/test/input/user.jsonl'], }, tableNames: ['user'], + dataTableFiles: {}, + }); + }); + + it('should route data-table user-row files into dataTableFiles', async () => { + const mockFiles = [ + 'user.jsonl', + 'data_table_user_abc.jsonl', + 'data_table_user_abc.2.jsonl', + 'data_table_user_xyz.jsonl', + ]; + + jest.mocked(readdir).mockResolvedValue(mockFiles as any); + jest + .mocked(safeJoinPath) + .mockReturnValueOnce('/test/input/user.jsonl') + .mockReturnValueOnce('/test/input/data_table_user_abc.jsonl') + .mockReturnValueOnce('/test/input/data_table_user_abc.2.jsonl') + .mockReturnValueOnce('/test/input/data_table_user_xyz.jsonl'); + + const result = await importService.getImportMetadata('/test/input'); + + expect(result).toEqual({ + entityFiles: { + user: ['/test/input/user.jsonl'], + }, + tableNames: ['user'], + dataTableFiles: { + abc: ['/test/input/data_table_user_abc.jsonl', '/test/input/data_table_user_abc.2.jsonl'], + xyz: ['/test/input/data_table_user_xyz.jsonl'], + }, }); }); }); @@ -1105,6 +1141,115 @@ describe('ImportService', () => { expect(mockDataTableDDLService.createTableWithColumns).not.toHaveBeenCalled(); expect(mockDataTableDDLService.dropTable).not.toHaveBeenCalled(); }); + + describe('row import', () => { + beforeEach(() => { + mockDataSource.driver.escapeQueryWithParameters = jest + .fn() + .mockImplementation((sql, params) => [sql, params]); + }); + + it('should insert rows into the recreated backing table when files are provided', async () => { + mockEntityManager.query = jest + .fn() + .mockResolvedValueOnce([{ id: 'abc' }]) + .mockResolvedValueOnce([ + { id: 'col-1', dataTableId: 'abc', name: 'flag', type: 'boolean', index: 0 }, + ]) + .mockResolvedValue(undefined); // subsequent INSERTs + + const row = { + id: 1, + createdAt: '2024-01-01 12:00:00', + updatedAt: '2024-01-01 12:00:00', + flag: 0, + }; + jest.mocked(readFile).mockResolvedValue(JSON.stringify(row)); + + await importService.recreateDataTableUserTablesFromRegistry(mockEntityManager, { + abc: ['/test/input/data_table_user_abc.jsonl'], + }); + + const insertCalls = (mockEntityManager.query as jest.Mock).mock.calls.filter( + ([sql]) => typeof sql === 'string' && sql.startsWith('INSERT INTO'), + ); + expect(insertCalls).toHaveLength(1); + const [, params] = insertCalls[0]; + // Boolean must have been normalised from 0 -> false (matters for cross-DB inserts) + expect((params as Record).flag).toBe(false); + // id is preserved + expect((params as Record).id).toBe(1); + }); + + it('should warn when archive has row files but registry is empty', async () => { + mockEntityManager.query = jest.fn().mockResolvedValueOnce([]); + + await importService.recreateDataTableUserTablesFromRegistry(mockEntityManager, { + orphan: ['/test/input/data_table_user_orphan.jsonl'], + }); + + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('but no entries in the data_table registry'), + ); + }); + + it('should reset the id sequence after inserts on Postgres', async () => { + // @ts-expect-error overriding for the test + mockDataSource.options = { type: 'postgres' }; + + // Mixed-case id matters: pg_get_serial_sequence folds unquoted + // identifiers to lowercase, so the param must be the quoted form. + const mixedCaseId = 'AbC123'; + + mockEntityManager.query = jest + .fn() + .mockResolvedValueOnce([{ id: mixedCaseId }]) + .mockResolvedValueOnce([ + { id: 'col-1', dataTableId: mixedCaseId, name: 'foo', type: 'string', index: 0 }, + ]) + .mockResolvedValueOnce(undefined) // INSERT row + .mockResolvedValueOnce([{ column_name: 'id' }]) // information_schema lookup + .mockResolvedValue(undefined); + + jest.mocked(readFile).mockResolvedValue( + JSON.stringify({ + id: 5, + createdAt: '2024-01-01T00:00:00.000Z', + updatedAt: '2024-01-01T00:00:00.000Z', + foo: 'x', + }), + ); + + await importService.recreateDataTableUserTablesFromRegistry(mockEntityManager, { + [mixedCaseId]: [`/test/input/data_table_user_${mixedCaseId}.jsonl`], + }); + + const setvalCalls = (mockEntityManager.query as jest.Mock).mock.calls.filter( + ([sql]) => typeof sql === 'string' && sql.includes('setval('), + ); + expect(setvalCalls).toHaveLength(1); + const [, params] = setvalCalls[0]; + expect(params).toEqual([`"data_table_user_${mixedCaseId}"`, 'id']); + }); + + it('should NOT call setval on Postgres when no rows were inserted', async () => { + // @ts-expect-error overriding for the test + mockDataSource.options = { type: 'postgres' }; + + mockEntityManager.query = jest + .fn() + .mockResolvedValueOnce([{ id: 'abc' }]) + .mockResolvedValueOnce([]) + .mockResolvedValue(undefined); + + await importService.recreateDataTableUserTablesFromRegistry(mockEntityManager, {}); + + const setvalCalls = (mockEntityManager.query as jest.Mock).mock.calls.filter( + ([sql]) => typeof sql === 'string' && sql.includes('setval('), + ); + expect(setvalCalls).toHaveLength(0); + }); + }); }); describe('advanceIdentitySequences', () => { diff --git a/packages/cli/src/services/export.service.ts b/packages/cli/src/services/export.service.ts index 99439add468..d9453835573 100644 --- a/packages/cli/src/services/export.service.ts +++ b/packages/cli/src/services/export.service.ts @@ -8,6 +8,9 @@ import { DataSource } from '@n8n/typeorm'; import { validateDbTypeForExportEntities } from '@/utils/validate-database-type'; import { Cipher } from 'n8n-core'; import { compressFolder } from '@/utils/compression.util'; +import { quoteIdentifier, toTableName } from '@/modules/data-table/utils/sql-utils'; + +const DATA_TABLE_ROWS_FILE_PREFIX = 'data_table_user_'; @Service() export class ExportService { @@ -90,11 +93,151 @@ export class ExportService { return systemTablesExported; } + private async loadDataTableIds(): Promise { + const tablePrefix = this.dataSource.options.entityPrefix || ''; + const dataTableTableName = `${tablePrefix}data_table`; + + let dataTables: Array<{ id: string }>; + try { + dataTables = await this.dataSource.query( + `SELECT id FROM ${this.dataSource.driver.escape(dataTableTableName)}`, + ); + } catch (error) { + this.logger.info( + ` ⚠️ ${dataTableTableName} registry not found, skipping data-table row export...`, + { error }, + ); + return []; + } + + if (dataTables.length === 0) { + this.logger.info(' ℹ️ No data tables found, nothing to export.'); + return []; + } + + return dataTables.map((t) => t.id); + } + + private async exportSingleDataTable( + dataTableId: string, + outputDir: string, + customEncryptionKey: string | undefined, + ): Promise { + const dbType = this.dataSource.options.type; + const userTableName = toTableName(dataTableId); + const fileBaseName = `${DATA_TABLE_ROWS_FILE_PREFIX}${dataTableId}`; + + this.logger.info(`\n📊 Processing data table: ${userTableName}`); + + await this.clearExistingEntityFiles(outputDir, fileBaseName); + + const idCol = quoteIdentifier('id', dbType); + const escapedUserTable = quoteIdentifier(userTableName, dbType); + const pageSize = 500; + const entitiesPerFile = 500; + + let lastId = 0; + let fileIndex = 1; + let currentFileEntityCount = 0; + let totalEntityCount = 0; + let hasNextPage = true; + + do { + let pageRows: Array>; + try { + pageRows = await this.dataSource.query( + `SELECT * FROM ${escapedUserTable} WHERE ${idCol} > ${lastId} ORDER BY "id" LIMIT ${pageSize}`, + ); + } catch (error) { + this.logger.warn( + ` ⚠️ Could not read rows from ${userTableName}; skipping. The dynamic table may be missing on the source instance.`, + { error }, + ); + break; + } + + if (pageRows.length === 0) break; + + const targetFileIndex = Math.floor(totalEntityCount / entitiesPerFile) + 1; + const fileName = + targetFileIndex === 1 + ? `${fileBaseName}.jsonl` + : `${fileBaseName}.${targetFileIndex}.jsonl`; + const filePath = safeJoinPath(outputDir, fileName); + + if (targetFileIndex > fileIndex) { + this.logger.info(` ✅ Completed file ${fileIndex}: ${currentFileEntityCount} rows`); + fileIndex = targetFileIndex; + currentFileEntityCount = 0; + } + + const rowsJsonl = pageRows.map((row) => JSON.stringify(row)).join('\n'); + await appendFile( + filePath, + (await this.cipher.encryptV2(rowsJsonl, customEncryptionKey)) + '\n', + 'utf8', + ); + + const lastRowId = Number((pageRows[pageRows.length - 1] as { id: unknown }).id); + if (!Number.isFinite(lastRowId)) { + throw new Error( + `Unexpected non-numeric id in ${userTableName}; cannot continue keyset pagination`, + ); + } + lastId = lastRowId; + + totalEntityCount += pageRows.length; + currentFileEntityCount += pageRows.length; + + this.logger.info( + ` Fetched ${pageRows.length} rows (last id: ${lastId}, total: ${totalEntityCount})`, + ); + + hasNextPage = pageRows.length >= pageSize; + } while (hasNextPage); + + if (currentFileEntityCount > 0) { + this.logger.info(` ✅ Completed file ${fileIndex}: ${currentFileEntityCount} rows`); + } + + this.logger.info( + ` ✅ Completed export for ${userTableName}: ${totalEntityCount} rows in ${fileIndex} file(s)`, + ); + + return totalEntityCount; + } + + private async exportDataTableUserTables( + outputDir: string, + customEncryptionKey?: string, + ): Promise<{ totalTables: number; totalRows: number }> { + this.logger.info('\n📚 Exporting data table user rows:'); + this.logger.info('=================================='); + + const dataTableIds = await this.loadDataTableIds(); + if (dataTableIds.length === 0) { + return { totalTables: 0, totalRows: 0 }; + } + + let totalRows = 0; + for (const dataTableId of dataTableIds) { + totalRows += await this.exportSingleDataTable(dataTableId, outputDir, customEncryptionKey); + } + + this.logger.info( + `\n📊 Data table row export summary: ${dataTableIds.length} table(s), ${totalRows} row(s)`, + ); + + return { totalTables: dataTableIds.length, totalRows }; + } + async exportEntities( outputDir: string, excludedTables: Set = new Set(), keyFilePath?: string, + options: { includeDataTableRows?: boolean } = {}, ) { + const { includeDataTableRows = true } = options; this.logger.info('\n⚠️⚠️ This feature is currently under development. ⚠️⚠️'); validateDbTypeForExportEntities(this.dataSource.options.type); @@ -133,6 +276,19 @@ export class ExportService { await this.exportMigrationsTable(outputDir, customEncryptionKey); + if (includeDataTableRows) { + const dataTableRowsExported = await this.exportDataTableUserTables( + outputDir, + customEncryptionKey, + ); + totalEntitiesExported += dataTableRowsExported.totalRows; + totalTablesProcessed += dataTableRowsExported.totalTables; + } else { + this.logger.info( + '\nℹ️ Skipping data-table row export (only schemas will be in the archive).', + ); + } + for (const metadata of entityMetadatas) { // Get table name and entity name const tableName = metadata.tableName; diff --git a/packages/cli/src/services/import.service.ts b/packages/cli/src/services/import.service.ts index 379f60ca65b..83f79c73b59 100644 --- a/packages/cli/src/services/import.service.ts +++ b/packages/cli/src/services/import.service.ts @@ -33,7 +33,14 @@ import { ActiveWorkflowManager } from '@/active-workflow-manager'; import type { IWorkflowWithVersionMetadata } from '@/interfaces'; import { WorkflowIndexService } from '@/modules/workflow-index/workflow-index.service'; import { DataTableDDLService } from '@/modules/data-table/data-table-ddl.service'; -import { DataTableColumn } from '@/modules/data-table/data-table-column.entity'; +import type { DataTableColumn } from '@/modules/data-table/data-table-column.entity'; +import { + normalizeUserRowValueForDatabase, + quoteIdentifier, + toTableName, +} from '@/modules/data-table/utils/sql-utils'; + +const DATA_TABLE_ROWS_FILE_PREFIX = 'data_table_user_'; @Service() export class ImportService { @@ -328,49 +335,64 @@ export class ImportService { /** * Get import metadata including table names and entity files * @param inputDir - Directory containing exported entity files - * @returns Object containing table names and entity files grouped by entity name + * @returns Object containing table names, entity files grouped by entity name, + * and data-table row files grouped by data table id. */ async getImportMetadata(inputDir: string): Promise<{ tableNames: string[]; entityFiles: Record; + dataTableFiles: Record; }> { const files = await readdir(inputDir); const entityTableNamesMap: Record = {}; const entityFiles: Record = {}; + const dataTableFiles: Record = {}; for (const file of files) { - if (file.endsWith('.jsonl')) { - const entityName = file.replace(/\.\d+\.jsonl$/, '.jsonl').replace('.jsonl', ''); - // Exclude migrations from regular entity import - if (entityName === 'migrations') { + if (!file.endsWith('.jsonl')) continue; + + const baseName = file.replace(/\.\d+\.jsonl$/, '.jsonl').replace('.jsonl', ''); + + // Exclude migrations from regular entity import + if (baseName === 'migrations') { + continue; + } + + // Route data-table user-row files into their own bucket keyed by data table id. + if (baseName.startsWith(DATA_TABLE_ROWS_FILE_PREFIX)) { + const dataTableId = baseName.slice(DATA_TABLE_ROWS_FILE_PREFIX.length); + if (!dataTableFiles[dataTableId]) dataTableFiles[dataTableId] = []; + dataTableFiles[dataTableId].push(safeJoinPath(inputDir, file)); + continue; + } + + const entityName = baseName; + + // Build table names map (only need to do this once per entity) + if (!entityTableNamesMap[entityName]) { + const entityMetadata = this.dataSource.entityMetadatas.find( + (meta) => meta.name.toLowerCase() === entityName, + ); + + if (!entityMetadata) { + this.logger.warn(`⚠️ No entity metadata found for ${entityName}, skipping...`); continue; } - // Build table names map (only need to do this once per entity) - if (!entityTableNamesMap[entityName]) { - const entityMetadata = this.dataSource.entityMetadatas.find( - (meta) => meta.name.toLowerCase() === entityName, - ); - - if (!entityMetadata) { - this.logger.warn(`⚠️ No entity metadata found for ${entityName}, skipping...`); - continue; - } - - entityTableNamesMap[entityName] = entityMetadata.tableName; - } - - // Build entity files map (only for entities with valid metadata) - if (!entityFiles[entityName]) { - entityFiles[entityName] = []; - } - entityFiles[entityName].push(safeJoinPath(inputDir, file)); + entityTableNamesMap[entityName] = entityMetadata.tableName; } + + // Build entity files map (only for entities with valid metadata) + if (!entityFiles[entityName]) { + entityFiles[entityName] = []; + } + entityFiles[entityName].push(safeJoinPath(inputDir, file)); } return { tableNames: Object.values(entityTableNamesMap), entityFiles, + dataTableFiles, }; } @@ -463,7 +485,7 @@ export class ImportService { // Get import metadata after migration validation const importMetadata = await this.getImportMetadata(inputDir); - const { tableNames, entityFiles } = importMetadata; + const { tableNames, entityFiles, dataTableFiles } = importMetadata; const entityNames = Object.keys(entityFiles); if (truncateTables) { @@ -506,8 +528,12 @@ export class ImportService { await this.advanceIdentitySequences(transactionManager, tableNames); // After the data_table / data_table_column registry rows are imported, - // recreate the dynamic backing tables (empty) so the imported tables work. - await this.recreateDataTableUserTablesFromRegistry(transactionManager); + // recreate the dynamic backing tables and load any user rows from the archive. + await this.recreateDataTableUserTablesFromRegistry( + transactionManager, + dataTableFiles, + customEncryptionKey, + ); if (!skipTogglingForeignKeyConstraints) { await this.enableForeignKeyConstraints(transactionManager); @@ -710,15 +736,20 @@ export class ImportService { /** * Recreate dynamic data-table backing tables for every entry in the imported - * `data_table` registry. The export bug (ADO-5143) means archives produced - * before this fix only contain registry rows; without recreating the backing - * tables the imported data tables would reference tables that don't exist on - * the destination. + * `data_table` registry, and insert user rows from any matching JSONL files. * * Tables are dropped before recreation so the operation is idempotent and - * handles stale tables left over from a partially-failed prior import. + * handles stale tables left over from a partially-failed prior import. When + * no row file exists for a table (e.g. an archive produced with + * `--includeDataTableRows=false` or before this feature) the backing table + * is recreated empty so the system stays functional. */ - async recreateDataTableUserTablesFromRegistry(transactionManager: EntityManager): Promise { + async recreateDataTableUserTablesFromRegistry( + transactionManager: EntityManager, + dataTableFiles: Record = {}, + customEncryptionKey?: string, + ): Promise { + const dbType = this.dataSource.options.type; const tablePrefix = this.dataSource.options.entityPrefix || ''; const dataTableTableName = `${tablePrefix}data_table`; const dataTableColumnTableName = `${tablePrefix}data_table_column`; @@ -736,41 +767,92 @@ export class ImportService { return; } - if (dataTables.length === 0) return; + if (dataTables.length === 0) { + if (Object.keys(dataTableFiles).length > 0) { + this.logger.warn( + ` ⚠️ Found ${Object.keys(dataTableFiles).length} data-table row file(s) in the archive but no entries in the data_table registry; skipping.`, + ); + } + return; + } const escapedColumnTable = this.dataSource.driver.escape(dataTableColumnTableName); const escapedDataTableId = this.dataSource.driver.escape('dataTableId'); const escapedIndex = this.dataSource.driver.escape('index'); - const columnRows: Array<{ - id: string; - dataTableId: string; - name: string; - type: 'string' | 'number' | 'boolean' | 'date'; - index: number; - }> = await transactionManager.query( + const columnRows: DataTableColumn[] = await transactionManager.query( `SELECT id, ${escapedDataTableId}, name, type, ${escapedIndex} FROM ${escapedColumnTable}`, ); const columnsByDataTableId = new Map(); for (const row of columnRows) { const list = columnsByDataTableId.get(row.dataTableId) ?? []; - list.push(transactionManager.create(DataTableColumn, row)); + list.push(row); columnsByDataTableId.set(row.dataTableId, list); } this.logger.info(`\n📚 Recreating ${dataTables.length} data-table backing table(s)...`); + let totalRowsImported = 0; for (const { id: dataTableId } of dataTables) { + const userTable = toTableName(dataTableId); const cols = (columnsByDataTableId.get(dataTableId) ?? []) .slice() .sort((a, b) => a.index - b.index); await this.dataTableDDLService.dropTable(dataTableId, transactionManager); await this.dataTableDDLService.createTableWithColumns(dataTableId, cols, transactionManager); + + const files = dataTableFiles[dataTableId]; + if (!files || files.length === 0) continue; + + const columnTypeMap = new Map(cols.map((c) => [c.name, c.type])); + columnTypeMap.set('createdAt', 'date'); + columnTypeMap.set('updatedAt', 'date'); + + const escapedTable = quoteIdentifier(userTable, dbType); + let rowsForTable = 0; + + for (const filePath of files) { + const rows = await this.readEntityFile(filePath, customEncryptionKey); + for (const row of rows) { + const normalizedRow: Record = {}; + for (const [key, value] of Object.entries(row)) { + normalizedRow[key] = normalizeUserRowValueForDatabase( + value, + columnTypeMap.get(key), + dbType, + ); + } + + const columnNames = Object.keys(normalizedRow); + const escapedCols = columnNames.map((c) => quoteIdentifier(c, dbType)).join(', '); + const placeholders = columnNames.map((c) => `:${c}`).join(', '); + const [query, params] = this.dataSource.driver.escapeQueryWithParameters( + `INSERT INTO ${escapedTable} (${escapedCols}) VALUES (${placeholders})`, + normalizedRow, + {}, + ); + await transactionManager.query(query, params); + rowsForTable++; + } + } + + // Postgres IDENTITY columns don't auto-advance when explicit ids are + // inserted, so the next implicit insert would collide. Reset to MAX(id). + if (rowsForTable > 0 && dbType === 'postgres') { + await this.advanceIdentitySequences(transactionManager, [userTable]); + } + + if (rowsForTable > 0) { + this.logger.info(` ✅ ${userTable}: ${rowsForTable} row(s) imported`); + } + totalRowsImported += rowsForTable; } - this.logger.info(`✅ Recreated ${dataTables.length} data-table backing table(s)`); + this.logger.info( + `✅ Recreated ${dataTables.length} data-table backing table(s); imported ${totalRowsImported} row(s)`, + ); } /** From 108fb026521ec2143802053024a0a6923b9f3fae Mon Sep 17 00:00:00 2001 From: Charlie Kolb Date: Wed, 13 May 2026 08:34:34 +0200 Subject: [PATCH 03/21] fix(editor): Auto-derive data table name from CSV filename (#30066) Co-authored-by: Claude Opus 4.7 --- .../components/AddDataTableModal.vue | 36 ++++++++++++---- .../core/dataTable/dataTable.store.ts | 42 +++++++++++++++++++ 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/packages/frontend/editor-ui/src/features/core/dataTable/components/AddDataTableModal.vue b/packages/frontend/editor-ui/src/features/core/dataTable/components/AddDataTableModal.vue index 786f636d3a1..52908e99a6e 100644 --- a/packages/frontend/editor-ui/src/features/core/dataTable/components/AddDataTableModal.vue +++ b/packages/frontend/editor-ui/src/features/core/dataTable/components/AddDataTableModal.vue @@ -199,9 +199,34 @@ const reset = (clearTableName = false) => { creationMode.value = 'select'; }; -const handleFileChange = (uploadFile: UploadFile) => { - if (uploadFile.raw) { - selectedFile.value = uploadFile.raw; +const deriveNameFromFileName = (fileName: string): string => { + return fileName + .replace(/\.csv$/i, '') + .replace(/[^a-zA-Z0-9]+/g, ' ') + .trim() + .slice(0, 128) + .trim(); +}; + +const handleFileChange = async (uploadFile: UploadFile) => { + if (!uploadFile.raw) return; + const file = uploadFile.raw; + selectedFile.value = file; + if (dataTableName.value) return; + + const baseName = deriveNameFromFileName(file.name); + if (!baseName) return; + + const projectId = route.params.projectId as string; + try { + const suggested = await dataTableStore.findAvailableDataTableName(baseName, projectId); + if (selectedFile.value === file && !dataTableName.value) { + dataTableName.value = suggested; + } + } catch { + if (selectedFile.value === file && !dataTableName.value) { + dataTableName.value = baseName; + } } }; @@ -231,11 +256,6 @@ const uploadFile = async () => { csvColumnName: col.name, }; }); - - if (!dataTableName.value) { - const fileName = selectedFile.value.name.replace(/\.csv$/i, ''); - dataTableName.value = fileName; - } } catch (error) { toast.showError(error, i18n.baseText('dataTable.upload.error')); reset(); diff --git a/packages/frontend/editor-ui/src/features/core/dataTable/dataTable.store.ts b/packages/frontend/editor-ui/src/features/core/dataTable/dataTable.store.ts index c146fb72e92..82037c28eb5 100644 --- a/packages/frontend/editor-ui/src/features/core/dataTable/dataTable.store.ts +++ b/packages/frontend/editor-ui/src/features/core/dataTable/dataTable.store.ts @@ -128,6 +128,47 @@ export const useDataTableStore = defineStore(DATA_TABLE_STORE, () => { return await uploadCsvFileApi(rootStore.restApiContext, file, hasHeaders); }; + const findAvailableDataTableName = async ( + baseName: string, + projectId: string, + ): Promise => { + const MAX_NAME_LENGTH = 128; + const PAGE_SIZE = 250; + const trimmed = baseName.trim().slice(0, MAX_NAME_LENGTH).trim(); + if (!trimmed || !projectId) return trimmed; + + const existingNames = new Set(); + let skip = 0; + while (true) { + const response = await fetchDataTablesApi( + rootStore.restApiContext, + projectId, + { skip, take: PAGE_SIZE }, + { name: trimmed }, + ); + for (const t of response.data) { + existingNames.add(t.name.toLowerCase()); + } + skip += response.data.length; + if (response.data.length < PAGE_SIZE || skip >= response.count) break; + } + + if (!existingNames.has(trimmed.toLowerCase())) return trimmed; + + const buildCandidate = (n: number): string => { + const suffix = ` ${n}`; + const maxBaseLen = MAX_NAME_LENGTH - suffix.length; + const base = trimmed.length > maxBaseLen ? trimmed.slice(0, maxBaseLen).trim() : trimmed; + return `${base}${suffix}`; + }; + + let n = 2; + while (existingNames.has(buildCandidate(n).toLowerCase())) { + n++; + } + return buildCandidate(n); + }; + const importCsvToDataTable = async (dataTableId: string, projectId: string, fileId: string) => { return await importCsvToDataTableApi(rootStore.restApiContext, dataTableId, projectId, fileId); }; @@ -385,6 +426,7 @@ export const useDataTableStore = defineStore(DATA_TABLE_STORE, () => { maxSizeMB, createDataTable, uploadCsvFile, + findAvailableDataTableName, importCsvToDataTable, deleteDataTable, updateDataTable, From f244fc564b2869a05daed6c2761fd2e2446fb4f6 Mon Sep 17 00:00:00 2001 From: Declan Carroll Date: Wed, 13 May 2026 07:38:47 +0100 Subject: [PATCH 04/21] ci: Add workflow pull_request_target safety rule to code-health (no-changelog) (#30327) Co-authored-by: Claude Opus 4.7 --- .github/workflows/ci-pr-quality.yml | 1 + packages/testing/code-health/src/index.ts | 8 + .../workflow-pr-target-safety.rule.test.ts | 267 ++++++++++++++++++ .../rules/workflow-pr-target-safety.rule.ts | 150 ++++++++++ .../code-health/src/utils/workflow-scanner.ts | 105 +++++++ 5 files changed, 531 insertions(+) create mode 100644 packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.test.ts create mode 100644 packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.ts create mode 100644 packages/testing/code-health/src/utils/workflow-scanner.ts diff --git a/.github/workflows/ci-pr-quality.yml b/.github/workflows/ci-pr-quality.yml index 33bad80fcd9..f13c783c43a 100644 --- a/.github/workflows/ci-pr-quality.yml +++ b/.github/workflows/ci-pr-quality.yml @@ -134,6 +134,7 @@ jobs: pnpm-workspace.yaml .code-health-baseline.json packages/testing/code-health/** + .github/workflows/** check-static-analysis: name: Static Analysis diff --git a/packages/testing/code-health/src/index.ts b/packages/testing/code-health/src/index.ts index fc89d367b1e..89fea1bd148 100644 --- a/packages/testing/code-health/src/index.ts +++ b/packages/testing/code-health/src/index.ts @@ -3,9 +3,11 @@ import type { RuleSettingsMap } from '@n8n/rules-engine'; import type { CodeHealthContext } from './context.js'; import { CatalogViolationsRule } from './rules/catalog-violations.rule.js'; +import { WorkflowPrTargetSafetyRule } from './rules/workflow-pr-target-safety.rule.js'; export type { CodeHealthContext } from './context.js'; export { CatalogViolationsRule } from './rules/catalog-violations.rule.js'; +export { WorkflowPrTargetSafetyRule } from './rules/workflow-pr-target-safety.rule.js'; const defaultRuleSettings: RuleSettingsMap = { 'catalog-violations': { @@ -13,6 +15,11 @@ const defaultRuleSettings: RuleSettingsMap = { severity: 'error', options: { workspaceFile: 'pnpm-workspace.yaml' }, }, + 'workflow-pr-target-safety': { + enabled: true, + severity: 'error', + options: { allowedWorkflows: ['ci-cla-check.yml'] }, + }, }; function mergeSettings(defaults: RuleSettingsMap, overrides?: RuleSettingsMap): RuleSettingsMap { @@ -31,6 +38,7 @@ function mergeSettings(defaults: RuleSettingsMap, overrides?: RuleSettingsMap): export function createDefaultRunner(settings?: RuleSettingsMap): RuleRunner { const runner = new RuleRunner(); runner.registerRule(new CatalogViolationsRule()); + runner.registerRule(new WorkflowPrTargetSafetyRule()); runner.applySettings(mergeSettings(defaultRuleSettings, settings)); return runner; } diff --git a/packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.test.ts b/packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.test.ts new file mode 100644 index 00000000000..18b080a7676 --- /dev/null +++ b/packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.test.ts @@ -0,0 +1,267 @@ +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import type { CodeHealthContext } from '../context.js'; +import { WorkflowPrTargetSafetyRule } from './workflow-pr-target-safety.rule.js'; + +function createTempDir(): string { + return fs.mkdtempSync(path.join(os.tmpdir(), 'code-health-workflow-test-')); +} + +function writeWorkflow(dir: string, name: string, content: string): void { + const fullPath = path.join(dir, '.github', 'workflows', name); + fs.mkdirSync(path.dirname(fullPath), { recursive: true }); + fs.writeFileSync(fullPath, content); +} + +describe('WorkflowPrTargetSafetyRule', () => { + let tmpDir: string; + let rule: WorkflowPrTargetSafetyRule; + + beforeEach(() => { + tmpDir = createTempDir(); + rule = new WorkflowPrTargetSafetyRule(); + rule.configure({ options: { allowedWorkflows: ['ci-cla-check.yml'] } }); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + function context(): CodeHealthContext { + return { rootDir: tmpDir }; + } + + it('ignores workflows that only use pull_request', async () => { + writeWorkflow( + tmpDir, + 'safe.yml', + ` +name: Safe +on: + pull_request: + types: [opened] +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + ref: \${{ github.event.pull_request.head.sha }} +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(0); + }); + + it('flags any non-allowlisted workflow that uses pull_request_target', async () => { + writeWorkflow( + tmpDir, + 'risky.yml', + ` +name: Risky +on: + pull_request_target: + types: [opened] +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(1); + expect(violations[0].rule).toBe('workflow-pr-target-safety'); + expect(violations[0].message).toContain('pull_request_target'); + expect(violations[0].message).toContain('Prefer pull_request'); + }); + + it('flags pull_request_target even when on: is a list', async () => { + writeWorkflow( + tmpDir, + 'list-trigger.yml', + ` +name: List +on: [pull_request_target] +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(1); + }); + + it('allows pull_request_target in an allowlisted workflow with no checkout override', async () => { + writeWorkflow( + tmpDir, + 'ci-cla-check.yml', + ` +name: CLA Check +on: + pull_request_target: + types: [opened] +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + sparse-checkout: .github/scripts/cla +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(0); + }); + + it('flags allowlisted workflow that checks out PR head sha', async () => { + writeWorkflow( + tmpDir, + 'ci-cla-check.yml', + ` +name: CLA Check +on: + pull_request_target: + types: [opened] +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + ref: \${{ github.event.pull_request.head.sha }} +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(1); + expect(violations[0].message).toContain('PR-author-controlled ref'); + expect(violations[0].message).toContain('their code, our keys'); + }); + + it('flags allowlisted workflow that checks out github.head_ref', async () => { + writeWorkflow( + tmpDir, + 'ci-cla-check.yml', + ` +name: CLA Check +on: + pull_request_target: +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + ref: \${{ github.head_ref }} +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(1); + expect(violations[0].message).toContain('PR-author-controlled ref'); + }); + + it('flags allowlisted workflow that checks out PR fork repository', async () => { + writeWorkflow( + tmpDir, + 'ci-cla-check.yml', + ` +name: CLA Check +on: + pull_request_target: +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + repository: \${{ github.event.pull_request.head.repo.full_name }} +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(1); + expect(violations[0].message).toContain('PR-author-controlled repository'); + }); + + it('flags shell git checkout of PR head ref', async () => { + writeWorkflow( + tmpDir, + 'ci-cla-check.yml', + ` +name: CLA Check +on: + pull_request_target: +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Fetch PR + run: | + git fetch origin \${{ github.event.pull_request.head.sha }} + git checkout FETCH_HEAD +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(1); + expect(violations[0].message).toContain('shell git command'); + }); + + it('does not flag a checkout with no ref override', async () => { + writeWorkflow( + tmpDir, + 'ci-cla-check.yml', + ` +name: CLA Check +on: + pull_request_target: +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + path: vendor +`, + ); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(0); + }); + + it('handles workflows with no triggers without throwing', async () => { + writeWorkflow(tmpDir, 'broken.yml', 'name: Broken\njobs: {}\n'); + + const violations = await rule.analyze(context()); + + expect(violations).toHaveLength(0); + }); + + it('skips files that are not valid YAML', async () => { + writeWorkflow(tmpDir, 'invalid.yml', '::: not yaml :::\n\t- broken'); + + await expect(rule.analyze(context())).resolves.toBeDefined(); + }); +}); diff --git a/packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.ts b/packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.ts new file mode 100644 index 00000000000..1ebaaf7be94 --- /dev/null +++ b/packages/testing/code-health/src/rules/workflow-pr-target-safety.rule.ts @@ -0,0 +1,150 @@ +import { BaseRule } from '@n8n/rules-engine'; +import type { Violation } from '@n8n/rules-engine'; +import * as path from 'node:path'; + +import type { CodeHealthContext } from '../context.js'; +import { + findLineContaining, + findWorkflowFiles, + parseWorkflow, + type WorkflowFile, + type WorkflowJobStep, +} from '../utils/workflow-scanner.js'; + +const RISKY_TRIGGER = 'pull_request_target'; + +const UNTRUSTED_REF_EXPRESSIONS = [ + 'github.event.pull_request.head.sha', + 'github.event.pull_request.head.ref', + 'github.event.pull_request.merge_commit_sha', + 'github.head_ref', +]; + +const UNTRUSTED_REPOSITORY_EXPRESSIONS = [ + 'github.event.pull_request.head.repo.full_name', + 'github.event.pull_request.head.repo.clone_url', +]; + +export class WorkflowPrTargetSafetyRule extends BaseRule { + readonly id = 'workflow-pr-target-safety'; + readonly name = 'Workflow pull_request_target Safety'; + readonly description = + 'Disallow pull_request_target triggers. Allowlisted workflows may use it only if they do not check out PR-author-controlled code.'; + readonly severity = 'error' as const; + + async analyze(context: CodeHealthContext): Promise { + const { rootDir } = context; + const options = this.getOptions(); + const allowedWorkflows = Array.isArray(options.allowedWorkflows) + ? (options.allowedWorkflows as string[]) + : []; + + const files = await findWorkflowFiles(rootDir); + const violations: Violation[] = []; + + for (const filePath of files) { + const workflow = parseWorkflow(filePath, rootDir); + if (!workflow) continue; + if (!workflow.triggers.includes(RISKY_TRIGGER)) continue; + + const fileName = path.basename(filePath); + if (!allowedWorkflows.includes(fileName)) { + violations.push(this.flagTrigger(workflow)); + continue; + } + + violations.push(...this.flagUnsafeCheckouts(workflow)); + } + + return violations; + } + + private flagTrigger(workflow: WorkflowFile): Violation { + return this.createViolation( + workflow.filePath, + findLineContaining(workflow.lines, RISKY_TRIGGER), + 1, + `${workflow.relativePath} uses pull_request_target. Prefer pull_request — pull_request_target runs in the base-repo context with secrets exposed, which is unsafe when combined with PR-author-controlled code.`, + 'Switch to pull_request, or if pull_request_target is required, request a security review and add this workflow to the rule allowlist.', + ); + } + + private flagUnsafeCheckouts(workflow: WorkflowFile): Violation[] { + const violations: Violation[] = []; + + for (const job of workflow.jobs) { + for (const step of job.steps) { + violations.push(...this.flagCheckoutAction(workflow, job.id, step)); + violations.push(...this.flagShellCheckout(workflow, job.id, step)); + } + } + + return violations; + } + + private flagCheckoutAction( + workflow: WorkflowFile, + jobId: string, + step: WorkflowJobStep, + ): Violation[] { + if (!step.uses?.startsWith('actions/checkout@')) return []; + const stepWith = step.with; + if (!stepWith) return []; + + const violations: Violation[] = []; + const stepLine = findLineContaining(workflow.lines, step.uses); + + const ref = typeof stepWith.ref === 'string' ? stepWith.ref : undefined; + if (ref && containsExpression(ref, UNTRUSTED_REF_EXPRESSIONS)) { + violations.push( + this.createViolation( + workflow.filePath, + findLineContaining(workflow.lines, 'ref:', stepLine), + 1, + `${workflow.relativePath} job "${jobId}" checks out PR-author-controlled ref under pull_request_target. This pattern ("their code, our keys") is the canonical pull_request_target exploit.`, + 'Remove the ref override (defaults to the base branch), or move this step to a workflow triggered by pull_request.', + ), + ); + } + + const repository = typeof stepWith.repository === 'string' ? stepWith.repository : undefined; + if (repository && containsExpression(repository, UNTRUSTED_REPOSITORY_EXPRESSIONS)) { + violations.push( + this.createViolation( + workflow.filePath, + findLineContaining(workflow.lines, 'repository:', stepLine), + 1, + `${workflow.relativePath} job "${jobId}" checks out a PR-author-controlled repository under pull_request_target.`, + 'Remove the repository override so the checkout stays on github.repository.', + ), + ); + } + + return violations; + } + + private flagShellCheckout( + workflow: WorkflowFile, + jobId: string, + step: WorkflowJobStep, + ): Violation[] { + if (!step.run) return []; + const usesGitCheckout = /\bgit\s+(checkout|fetch|pull)\b/.test(step.run); + if (!usesGitCheckout) return []; + if (!containsExpression(step.run, UNTRUSTED_REF_EXPRESSIONS)) return []; + + return [ + this.createViolation( + workflow.filePath, + findLineContaining(workflow.lines, 'run:'), + 1, + `${workflow.relativePath} job "${jobId}" runs a shell git command that fetches PR-author-controlled refs under pull_request_target.`, + 'Avoid checking out PR head from a pull_request_target workflow; do the work in a pull_request-triggered workflow instead.', + ), + ]; + } +} + +function containsExpression(value: string, needles: string[]): boolean { + return needles.some((needle) => value.includes(needle)); +} diff --git a/packages/testing/code-health/src/utils/workflow-scanner.ts b/packages/testing/code-health/src/utils/workflow-scanner.ts new file mode 100644 index 00000000000..3cb9e9ece37 --- /dev/null +++ b/packages/testing/code-health/src/utils/workflow-scanner.ts @@ -0,0 +1,105 @@ +import fg from 'fast-glob'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { parse as parseYaml } from 'yaml'; + +export interface WorkflowJobStep { + name?: string; + uses?: string; + run?: string; + with?: Record; +} + +export interface WorkflowJob { + id: string; + steps: WorkflowJobStep[]; +} + +export interface WorkflowFile { + filePath: string; + relativePath: string; + lines: string[]; + triggers: string[]; + jobs: WorkflowJob[]; +} + +export async function findWorkflowFiles(rootDir: string): Promise { + return await fg('.github/workflows/*.{yml,yaml}', { + cwd: rootDir, + absolute: true, + dot: true, + }); +} + +export function parseWorkflow(filePath: string, rootDir: string): WorkflowFile | null { + const content = fs.readFileSync(filePath, 'utf-8'); + const lines = content.split('\n'); + + let doc: unknown; + try { + doc = parseYaml(content); + } catch { + return null; + } + + if (!doc || typeof doc !== 'object') return null; + + const workflow = doc as Record; + // YAML's `on:` keyword gets parsed as boolean `true` by the yaml lib + // unless quoted, so check both keys. + const onValue = workflow.on ?? workflow.true; + + return { + filePath, + relativePath: path.relative(rootDir, filePath), + lines, + triggers: extractTriggers(onValue), + jobs: extractJobs(workflow.jobs), + }; +} + +function extractTriggers(onValue: unknown): string[] { + if (typeof onValue === 'string') return [onValue]; + if (Array.isArray(onValue)) return onValue.filter((t): t is string => typeof t === 'string'); + if (onValue && typeof onValue === 'object') return Object.keys(onValue); + return []; +} + +function extractJobs(jobsValue: unknown): WorkflowJob[] { + if (!jobsValue || typeof jobsValue !== 'object') return []; + + const jobs: WorkflowJob[] = []; + for (const [id, raw] of Object.entries(jobsValue as Record)) { + if (!raw || typeof raw !== 'object') continue; + const job = raw as Record; + const stepsValue = job.steps; + if (!Array.isArray(stepsValue)) { + jobs.push({ id, steps: [] }); + continue; + } + + const steps: WorkflowJobStep[] = []; + for (const stepRaw of stepsValue) { + if (!stepRaw || typeof stepRaw !== 'object') continue; + const step = stepRaw as Record; + steps.push({ + name: typeof step.name === 'string' ? step.name : undefined, + uses: typeof step.uses === 'string' ? step.uses : undefined, + run: typeof step.run === 'string' ? step.run : undefined, + with: + step.with && typeof step.with === 'object' + ? (step.with as Record) + : undefined, + }); + } + jobs.push({ id, steps }); + } + return jobs; +} + +export function findLineContaining(lines: string[], needle: string, startLine = 0): number { + for (let i = startLine; i < lines.length; i++) { + if (lines[i].includes(needle)) return i + 1; + } + return 1; +} From 2fd54d8230001334543325a2b7b968bd25711015 Mon Sep 17 00:00:00 2001 From: Mutasem Aldmour <4711238+mutdmour@users.noreply.github.com> Date: Wed, 13 May 2026 08:45:39 +0200 Subject: [PATCH 05/21] feat(core): Curate workflow examples for the builder sandbox (#30025) Co-authored-by: Claude Opus 4.7 (1M context) --- .../evaluations/cli/compare-pairwise.ts | 274 +- .../src/runtime/resumable-stream-executor.ts | 13 + .../src/stream/consume-with-hitl.ts | 7 + .../tools/__tests__/credentials.tool.test.ts | 37 + .../instance-ai/src/tools/credentials.tool.ts | 8 + .../build-workflow-agent.prompt.ts | 13 + .../build-workflow-agent.tool.ts | 35 + .../instance-ai/src/utils/scrub-secrets.ts | 34 + .../__tests__/builder-sandbox-factory.test.ts | 21 + .../workspace/__tests__/sandbox-setup.test.ts | 24 + .../src/workspace/builder-sandbox-factory.ts | 16 +- .../instance-ai/src/workspace/sandbox-fs.ts | 32 +- .../src/workspace/sandbox-setup.ts | 32 + .../src/workspace/template-telemetry.test.ts | 417 ++ .../src/workspace/template-telemetry.ts | 272 ++ packages/@n8n/workflow-sdk/.gitignore | 8 + .../workflow-sdk/docs/template-criteria.md | 95 + .../workflow-sdk/examples/_calibration.json | 245 ++ .../examples/_coverage-report.json | 809 ++++ .../@n8n/workflow-sdk/examples/manifest.json | 3491 +++++++++++++++++ .../@n8n/workflow-sdk/examples/templates.zip | Bin 0 -> 533128 bytes packages/@n8n/workflow-sdk/jest.config.js | 5 +- packages/@n8n/workflow-sdk/package.json | 19 +- .../@n8n/workflow-sdk/scripts/calibration.ts | 179 + .../workflow-sdk/scripts/coverage.test.ts | 64 + .../@n8n/workflow-sdk/scripts/coverage.ts | 324 ++ .../scripts/create-examples-zip.ts | 40 + .../workflow-sdk/scripts/criteria.test.ts | 452 +++ .../@n8n/workflow-sdk/scripts/criteria.ts | 398 ++ .../workflow-sdk/scripts/extract-workflows.ts | 55 +- .../workflow-sdk/scripts/fetch-templates.ts | 230 ++ .../workflow-sdk/scripts/jest-global-setup.ts | 10 + .../scripts/regenerate-examples.ts | 551 +++ .../src/__tests__/examples-roundtrip.test.ts | 81 + .../src/codegen/emit-instance-ai.test.ts | 363 ++ .../src/codegen/emit-instance-ai.ts | 203 + .../@n8n/workflow-sdk/src/codegen/index.ts | 1 + .../workflow-sdk/src/examples-loader.test.ts | 50 + .../@n8n/workflow-sdk/src/examples-loader.ts | 144 + .../@n8n/workflow-sdk/src/examples-zip.ts | 73 + packages/@n8n/workflow-sdk/src/index.ts | 1 + 41 files changed, 9070 insertions(+), 56 deletions(-) create mode 100644 packages/@n8n/instance-ai/src/utils/scrub-secrets.ts create mode 100644 packages/@n8n/instance-ai/src/workspace/template-telemetry.test.ts create mode 100644 packages/@n8n/instance-ai/src/workspace/template-telemetry.ts create mode 100644 packages/@n8n/workflow-sdk/docs/template-criteria.md create mode 100644 packages/@n8n/workflow-sdk/examples/_calibration.json create mode 100644 packages/@n8n/workflow-sdk/examples/_coverage-report.json create mode 100644 packages/@n8n/workflow-sdk/examples/manifest.json create mode 100644 packages/@n8n/workflow-sdk/examples/templates.zip create mode 100644 packages/@n8n/workflow-sdk/scripts/calibration.ts create mode 100644 packages/@n8n/workflow-sdk/scripts/coverage.test.ts create mode 100644 packages/@n8n/workflow-sdk/scripts/coverage.ts create mode 100644 packages/@n8n/workflow-sdk/scripts/create-examples-zip.ts create mode 100644 packages/@n8n/workflow-sdk/scripts/criteria.test.ts create mode 100644 packages/@n8n/workflow-sdk/scripts/criteria.ts create mode 100644 packages/@n8n/workflow-sdk/scripts/fetch-templates.ts create mode 100644 packages/@n8n/workflow-sdk/scripts/jest-global-setup.ts create mode 100644 packages/@n8n/workflow-sdk/scripts/regenerate-examples.ts create mode 100644 packages/@n8n/workflow-sdk/src/__tests__/examples-roundtrip.test.ts create mode 100644 packages/@n8n/workflow-sdk/src/codegen/emit-instance-ai.test.ts create mode 100644 packages/@n8n/workflow-sdk/src/codegen/emit-instance-ai.ts create mode 100644 packages/@n8n/workflow-sdk/src/examples-loader.test.ts create mode 100644 packages/@n8n/workflow-sdk/src/examples-loader.ts create mode 100644 packages/@n8n/workflow-sdk/src/examples-zip.ts diff --git a/packages/@n8n/instance-ai/evaluations/cli/compare-pairwise.ts b/packages/@n8n/instance-ai/evaluations/cli/compare-pairwise.ts index fafbeb99db6..040e76d3cf9 100644 --- a/packages/@n8n/instance-ai/evaluations/cli/compare-pairwise.ts +++ b/packages/@n8n/instance-ai/evaluations/cli/compare-pairwise.ts @@ -17,6 +17,9 @@ import { jsonParse } from 'n8n-workflow'; import { promises as fs } from 'node:fs'; import path from 'node:path'; +import { scrubSecretsInText } from '../../src/utils/scrub-secrets'; +import { redactSecrets } from '../harness/redact'; + // --------------------------------------------------------------------------- // Shared shape after normalization // --------------------------------------------------------------------------- @@ -50,6 +53,8 @@ export interface BuilderRecord { toolCallErrors?: number; /** Total tool calls observed, used as the error-rate denominator. */ toolCallsTotal?: number; + /** Raw tool-call trace. IA-only — populated by `loadInstanceAiRun`. */ + toolCalls?: IAToolCallTrace[]; } interface BuilderSummary { @@ -190,6 +195,7 @@ async function loadInstanceAiRun(dir: string): Promise { submitCalls: tcs.filter((tc) => tc.toolName === 'submit-workflow').length, toolCallErrors: tcs.filter(isErroredIAToolCall).length, toolCallsTotal: tcs.length, + toolCalls: tcs, }; }); @@ -634,7 +640,7 @@ function renderBuilderColumn(label: string, record: BuilderRecord | undefined): } const errorBlock = record.errorMessage - ? `
${escapeHtml(record.errorMessage)}
` + ? `
${escapeHtml(scrubSecretsInText(record.errorMessage))}
` : ''; const idLine = record.exampleId @@ -654,12 +660,110 @@ function renderBuilderColumn(label: string, record: BuilderRecord | undefined): `; } -function renderRow(row: ComparisonRow, index: number): string { +interface SideLabels { + ee: string; + ia: string; +} + +function summarizeToolCallArgs(toolName: string, args: unknown): string { + if (!args || typeof args !== 'object') return ''; + // Redact secret-shaped keys (password / token / api_key / …) and scrub the + // extracted free-form strings for known credential patterns. The same + // summarized text is used as a data-attribute on the row, so any leaked + // secret would persist in the HTML and be searchable via filter input. + const a = redactSecrets(args) as Record; + const str = (v: unknown): string => (typeof v === 'string' ? scrubSecretsInText(v) : ''); + const trunc = (s: string, n = 160): string => (s.length > n ? s.slice(0, n) + '…' : s); + switch (toolName) { + case 'mastra_workspace_execute_command': + return trunc(str(a.command)); + case 'mastra_workspace_edit_file': + return trunc(str(a.path)); + case 'mastra_workspace_write_file': { + const p = str(a.path); + const len = typeof a.content === 'string' ? a.content.length : 0; + return `${p}${len ? ` (${len} chars)` : ''}`; + } + case 'mastra_workspace_read_file': { + const p = str(a.path); + const off = typeof a.offset === 'number' ? a.offset : undefined; + const lim = typeof a.limit === 'number' ? a.limit : undefined; + const range = off !== undefined || lim !== undefined ? ` @${off ?? 0}+${lim ?? '∞'}` : ''; + return trunc(`${p}${range}`); + } + case 'mastra_workspace_grep': + return trunc(`${str(a.pattern)}${a.path ? ` in ${str(a.path)}` : ''}`); + case 'mastra_workspace_lsp_inspect': { + const line = typeof a.line === 'number' ? String(a.line) : '?'; + return trunc(`${str(a.path)}:${line} ${str(a.match)}`); + } + case 'mastra_workspace_mkdir': + return trunc(str(a.path)); + case 'submit-workflow': + return trunc(`${str(a.name)} ${str(a.filePath)}`); + case 'verify-built-workflow': + return trunc(str(a.workflowId) || str(a.workItemId)); + case 'credentials': + case 'data-tables': + case 'nodes': + case 'workflows': + return trunc(str(a.action)); + default: + return trunc(scrubSecretsInText(JSON.stringify(a)), 120); + } +} + +function renderToolCallRows(traces: IAToolCallTrace[]): string { + return traces + .map((tc) => { + const errored = isErroredIAToolCall(tc); + const elapsed = tc.elapsedMs !== undefined ? formatDuration(tc.elapsedMs) : '—'; + const cls = errored ? 'tc-err' : ''; + const argSummary = summarizeToolCallArgs(tc.toolName, tc.args); + const argCell = argSummary ? `${escapeHtml(argSummary)}` : ''; + const detail = errored && tc.error ? escapeHtml(scrubSecretsInText(tc.error)) : ''; + return `${tc.step}${escapeHtml(tc.toolName)}${argCell}${elapsed}${detail}`; + }) + .join(''); +} + +function renderToolCallsColumn(label: string, record: BuilderRecord | undefined): string { + if (!record) { + return `
${escapeHtml(label)}
No record.
`; + } + const traces = record.toolCalls ?? []; + if (traces.length === 0) { + return `
${escapeHtml(label)} (0 calls)
No tool calls captured.
`; + } + const errCount = traces.filter(isErroredIAToolCall).length; + const errBadge = errCount > 0 ? ` · ${errCount} err` : ''; + return `
+
${escapeHtml(label)} (${traces.length} call${traces.length === 1 ? '' : 's'}${errBadge})
+ + + ${renderToolCallRows(traces)} +
#ToolArgsTimeError
+
`; +} + +function renderToolCallsSection(row: ComparisonRow, labels: SideLabels): string { + const anyTraces = (row.ee?.toolCalls?.length ?? 0) > 0 || (row.ia?.toolCalls?.length ?? 0) > 0; + if (!anyTraces) return ''; + return `
+ Tool calls +
+ ${renderToolCallsColumn(labels.ee, row.ee)} + ${renderToolCallsColumn(labels.ia, row.ia)} +
+
`; +} + +function renderRow(row: ComparisonRow, index: number, labels: SideLabels): string { const verdictLabel: Record = { 'both-pass': 'BOTH PASS', 'both-fail': 'BOTH FAIL', - 'ee-only': 'CODE ONLY', - 'ia-only': 'IA ONLY', + 'ee-only': `${labels.ee.toUpperCase()} ONLY`, + 'ia-only': `${labels.ia.toUpperCase()} ONLY`, neither: '—', }; const verdictCls: Record = { @@ -690,14 +794,33 @@ function renderRow(row: ComparisonRow, index: number): string { // Heavy content (workflow previews + judge tables) is wrapped in a