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] 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('