diff --git a/packages/@n8n/permissions/src/__tests__/__snapshots__/scope-information.test.ts.snap b/packages/@n8n/permissions/src/__tests__/__snapshots__/scope-information.test.ts.snap index 921bdcfe598..258d327f325 100644 --- a/packages/@n8n/permissions/src/__tests__/__snapshots__/scope-information.test.ts.snap +++ b/packages/@n8n/permissions/src/__tests__/__snapshots__/scope-information.test.ts.snap @@ -37,7 +37,6 @@ exports[`Scope Information ensure scopes are defined correctly 1`] = ` "externalSecretsProvider:list", "externalSecretsProvider:*", "externalSecret:list", - "externalSecret:use", "externalSecret:*", "eventBusDestination:test", "eventBusDestination:create", diff --git a/packages/@n8n/permissions/src/constants.ee.ts b/packages/@n8n/permissions/src/constants.ee.ts index 1ef6aa52e48..c51ca1b41bd 100644 --- a/packages/@n8n/permissions/src/constants.ee.ts +++ b/packages/@n8n/permissions/src/constants.ee.ts @@ -8,7 +8,7 @@ export const RESOURCES = { communityPackage: ['install', 'uninstall', 'update', 'list', 'manage'] as const, credential: ['share', 'shareGlobally', 'move', ...DEFAULT_OPERATIONS] as const, externalSecretsProvider: ['sync', ...DEFAULT_OPERATIONS] as const, - externalSecret: ['list', 'use'] as const, + externalSecret: ['list'] as const, eventBusDestination: ['test', ...DEFAULT_OPERATIONS] as const, ldap: ['sync', 'manage'] as const, license: ['manage'] as const, diff --git a/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts b/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts index b47c44aab70..ef3bee9c06a 100644 --- a/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts +++ b/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts @@ -34,7 +34,6 @@ export const GLOBAL_OWNER_SCOPES: Scope[] = [ 'externalSecretsProvider:list', 'externalSecretsProvider:sync', 'externalSecret:list', - 'externalSecret:use', 'ldap:manage', 'ldap:sync', 'license:manage', diff --git a/packages/cli/src/credentials/__tests__/credentials.controller.test.ts b/packages/cli/src/credentials/__tests__/credentials.controller.test.ts index c327d23affc..6befa6109ef 100644 --- a/packages/cli/src/credentials/__tests__/credentials.controller.test.ts +++ b/packages/cli/src/credentials/__tests__/credentials.controller.test.ts @@ -1,22 +1,57 @@ -import type { AuthenticatedRequest, SharedCredentialsRepository, CredentialsEntity } from '@n8n/db'; -import { GLOBAL_OWNER_ROLE, GLOBAL_MEMBER_ROLE } from '@n8n/db'; +import type { + AuthenticatedRequest, + CredentialsEntity, + CredentialsRepository, + SharedCredentialsRepository, +} from '@n8n/db'; +import { GLOBAL_MEMBER_ROLE, GLOBAL_OWNER_ROLE } from '@n8n/db'; import { mock } from 'jest-mock-extended'; import { createRawProjectData } from '@/__tests__/project.test-data'; import type { EventService } from '@/events/event.service'; -import { createdCredentialsWithScopes, createNewCredentialsPayload } from './credentials.test-data'; -import { CredentialsController } from '../credentials.controller'; -import type { CredentialsService } from '../credentials.service'; -import type { CredentialsFinderService } from '../credentials-finder.service'; import type { CredentialRequest } from '@/requests'; +import type { CredentialsFinderService } from '../credentials-finder.service'; +import { CredentialsController } from '../credentials.controller'; +import { CredentialsService } from '../credentials.service'; +import { createdCredentialsWithScopes, createNewCredentialsPayload } from './credentials.test-data'; describe('CredentialsController', () => { const eventService = mock(); - const credentialsService = mock(); const sharedCredentialsRepository = mock(); const credentialsFinderService = mock(); + // Mock the credentialsRepository with a working create method + const credentialsRepository = mock(); + + // real CredentialsService instance with mocked dependencies + const credentialsService = new CredentialsService( + credentialsRepository, + mock(), // sharedCredentialsRepository + mock(), // ownershipService + mock(), // logger + mock(), // errorReporter + mock(), // credentialsTester + mock(), // externalHooks + mock(), // credentialTypes + mock(), // projectRepository + mock(), // projectService + mock(), // roleService + mock(), // userRepository + mock(), // credentialsFinderService + mock(), // credentialsHelper + ); + + // Spy on methods that need to be mocked in tests + // This allows us to mock specific behavior while keeping real implementations + // for isChangingExternalSecretExpression and validateExternalSecretsPermissions + jest.spyOn(credentialsService, 'decrypt'); + jest.spyOn(credentialsService, 'prepareUpdateData'); + jest.spyOn(credentialsService, 'createEncryptedData'); + jest.spyOn(credentialsService, 'getCredentialScopes'); + jest.spyOn(credentialsService, 'update'); + jest.spyOn(credentialsService, 'createUnmanagedCredential'); + const credentialsController = new CredentialsController( mock(), credentialsService, @@ -39,12 +74,12 @@ describe('CredentialsController', () => { beforeEach(() => { jest.resetAllMocks(); + // Set up credentialsRepository.create to return the input data + credentialsRepository.create.mockImplementation((data) => data as CredentialsEntity); }); describe('createCredentials', () => { it('should create new credentials and emit "credentials-created"', async () => { - // Arrange - const newCredentialsPayload = createNewCredentialsPayload(); req.body = newCredentialsPayload; @@ -57,8 +92,9 @@ describe('CredentialsController', () => { id: newCredentialsPayload.projectId, }); - // @ts-ignore - credentialsService.createUnmanagedCredential.mockResolvedValue(createdCredentials); + jest + .mocked(credentialsService.createUnmanagedCredential) + .mockResolvedValue(createdCredentials); sharedCredentialsRepository.findCredentialOwningProject.mockResolvedValue( projectOwningCredentialData, @@ -106,13 +142,8 @@ describe('CredentialsController', () => { }); beforeEach(() => { - credentialsService.decrypt.mockReturnValue({ apiKey: 'test-key' }); - credentialsService.prepareUpdateData.mockResolvedValue({ - name: 'Updated Credential', - type: 'apiKey', - data: { apiKey: 'updated-key' }, - } as any); - credentialsService.createEncryptedData.mockReturnValue({ + jest.mocked(credentialsService.decrypt).mockReturnValue({ apiKey: 'test-key' }); + jest.mocked(credentialsService.createEncryptedData).mockReturnValue({ name: 'Updated Credential', type: 'apiKey', data: 'encrypted-data', @@ -120,10 +151,9 @@ describe('CredentialsController', () => { createdAt: new Date(), updatedAt: new Date(), } as any); - credentialsService.getCredentialScopes.mockResolvedValue([ - 'credential:read', - 'credential:update', - ] as any); + jest + .mocked(credentialsService.getCredentialScopes) + .mockResolvedValue(['credential:read', 'credential:update'] as any); }); it('should allow owner to set isGlobal to true', async () => { @@ -140,7 +170,7 @@ describe('CredentialsController', () => { } as unknown as CredentialRequest.Update; credentialsFinderService.findCredentialForUser.mockResolvedValue(existingCredential); - credentialsService.update.mockResolvedValue({ + jest.mocked(credentialsService.update).mockResolvedValue({ ...existingCredential, name: 'Updated Credential', isGlobal: true, @@ -181,7 +211,7 @@ describe('CredentialsController', () => { } as unknown as CredentialRequest.Update; credentialsFinderService.findCredentialForUser.mockResolvedValue(globalCredential); - credentialsService.update.mockResolvedValue({ + jest.mocked(credentialsService.update).mockResolvedValue({ ...globalCredential, isGlobal: false, }); @@ -263,7 +293,7 @@ describe('CredentialsController', () => { } as unknown as CredentialRequest.Update; credentialsFinderService.findCredentialForUser.mockResolvedValue(existingCredential); - credentialsService.update.mockResolvedValue({ + jest.mocked(credentialsService.update).mockResolvedValue({ ...existingCredential, name: 'Updated Credential', }); diff --git a/packages/cli/src/credentials/__tests__/credentials.service.test.ts b/packages/cli/src/credentials/__tests__/credentials.service.test.ts index d92b161683f..ca607235ca0 100644 --- a/packages/cli/src/credentials/__tests__/credentials.service.test.ts +++ b/packages/cli/src/credentials/__tests__/credentials.service.test.ts @@ -16,6 +16,7 @@ import { CREDENTIAL_BLANKING_VALUE } from '@/constants'; import type { CredentialTypes } from '@/credential-types'; import type { CredentialsFinderService } from '@/credentials/credentials-finder.service'; import { CredentialsService } from '@/credentials/credentials.service'; +import * as validation from '@/credentials/validation'; import type { CredentialsHelper } from '@/credentials-helper'; import type { ExternalHooks } from '@/external-hooks'; import type { CredentialsTester } from '@/services/credentials-tester.service'; @@ -1548,6 +1549,23 @@ describe('CredentialsService', () => { 'The field "apiKey" is mandatory for credentials of type "apiKey"', ); }); + + it('should prevent use of external secret expression when required permission is missing', async () => { + credentialsHelper.getCredentialsProperties.mockReturnValue([]); + const payload = { + name: 'Test Credential', + type: 'apiKey', + data: { + apiKey: '$secrets.myApiKey', + url: 'https://api.example.com', + }, + projectId: 'project-1', + }; + + await expect(service.createUnmanagedCredential(payload, memberUser)).rejects.toThrow( + 'Lacking permissions to reference external secrets in credentials', + ); + }); }); describe('createManagedCredential', () => { @@ -1641,4 +1659,136 @@ describe('CredentialsService', () => { expect(result).toHaveProperty('name', 'Managed Credential'); }); }); + + describe('checkCredentialData', () => { + const ownerUser = mock({ id: 'owner-id', role: GLOBAL_OWNER_ROLE }); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should pass when all required fields are provided', () => { + credentialsHelper.getCredentialsProperties.mockReturnValue([ + { + displayName: 'API Key', + name: 'apiKey', + type: 'string', + required: true, + default: '', + }, + { + displayName: 'Domain', + name: 'domain', + type: 'string', + required: true, + default: '', + }, + ]); + + const data = { + apiKey: 'test-key', + domain: 'example.com', + }; + + expect(() => service.checkCredentialData('apiCredential', data, ownerUser)).not.toThrow(); + }); + + it('should pass when required field with valid default is not provided', () => { + credentialsHelper.getCredentialsProperties.mockReturnValue([ + { + displayName: 'Host', + name: 'host', + type: 'string', + required: true, + default: 'https://api.example.com', + }, + ]); + + const data = {}; + + expect(() => service.checkCredentialData('apiCredential', data, ownerUser)).not.toThrow(); + }); + + it('should pass when credential has no required fields', () => { + credentialsHelper.getCredentialsProperties.mockReturnValue([ + { + displayName: 'Optional Field', + name: 'optionalField', + type: 'string', + required: false, + default: '', + }, + ]); + + const data = {}; + + expect(() => service.checkCredentialData('apiCredential', data, ownerUser)).not.toThrow(); + }); + + it('should call validateExternalSecretsPermissions', () => { + credentialsHelper.getCredentialsProperties.mockReturnValue([]); + const validateSpy = jest.spyOn(validation, 'validateExternalSecretsPermissions'); + + const data = { apiKey: 'test-key' }; + + service.checkCredentialData('apiCredential', data, ownerUser); + + expect(validateSpy).toHaveBeenCalledWith(ownerUser, data); + }); + + it('should throw BadRequestError when required field is missing and has no default', () => { + credentialsHelper.getCredentialsProperties.mockReturnValue([ + { + displayName: 'API Key', + name: 'apiKey', + type: 'string', + required: true, + default: undefined, + }, + ]); + + const data = {}; // apiKey is missing + + expect(() => service.checkCredentialData('apiCredential', data, ownerUser)).toThrow( + 'The field "apiKey" is mandatory for credentials of type "apiCredential"', + ); + }); + + it('should throw when required field value is undefined', () => { + credentialsHelper.getCredentialsProperties.mockReturnValue([ + { + displayName: 'API Key', + name: 'apiKey', + type: 'string', + required: true, + default: '', + }, + ]); + + const data = { apiKey: undefined }; + + // @ts-expect-error - Testing edge case with undefined value + expect(() => service.checkCredentialData('apiCredential', data, ownerUser)).toThrow( + 'The field "apiKey" is mandatory for credentials of type "apiCredential"', + ); + }); + + it('should throw when required field value is empty string', () => { + credentialsHelper.getCredentialsProperties.mockReturnValue([ + { + displayName: 'API Key', + name: 'apiKey', + type: 'string', + required: true, + default: '', + }, + ]); + + const data = { apiKey: '' }; + + expect(() => service.checkCredentialData('apiCredential', data, ownerUser)).toThrow( + 'The field "apiKey" is mandatory for credentials of type "apiCredential"', + ); + }); + }); }); diff --git a/packages/cli/src/credentials/__tests__/credentials.test-data.ts b/packages/cli/src/credentials/__tests__/credentials.test-data.ts index fa16a3b1980..1b659ab1e7d 100644 --- a/packages/cli/src/credentials/__tests__/credentials.test-data.ts +++ b/packages/cli/src/credentials/__tests__/credentials.test-data.ts @@ -1,18 +1,10 @@ import type { CreateCredentialDto } from '@n8n/api-types'; +import type { CredentialsEntity } from '@n8n/db'; import type { Scope } from '@n8n/permissions'; import { nanoId, date } from 'minifaker'; import { randomString } from 'n8n-workflow'; -type NewCredentialWithSCopes = { - scopes: Scope[]; - name: string; - data: string; - type: string; - isManaged: boolean; - id: string; - createdAt: Date; - updatedAt: Date; -}; +export type NewCredentialWithScopes = CredentialsEntity & { scopes: Scope[] }; const name = 'new Credential'; const type = 'openAiApi'; @@ -43,17 +35,17 @@ export const createNewCredentialsPayload = (payload?: Partial, -): NewCredentialWithSCopes => { + payload?: Partial, +): NewCredentialWithScopes => { return { - name, - type, - data: randomString(20), - id: nanoId.nanoid(), - createdAt: date(), - updatedAt: date(), - isManaged: false, - scopes: credentialScopes, - ...payload, - }; + name: payload?.name ?? name, + type: payload?.type ?? type, + data: payload?.data ?? randomString(20), + id: payload?.id ?? nanoId.nanoid(), + createdAt: payload?.createdAt ?? date(), + updatedAt: payload?.updatedAt ?? date(), + isManaged: payload?.isManaged ?? false, + isGlobal: payload?.isGlobal ?? false, + scopes: payload?.scopes ?? credentialScopes, + } as NewCredentialWithScopes; }; diff --git a/packages/cli/src/credentials/__tests__/validation.test.ts b/packages/cli/src/credentials/__tests__/validation.test.ts new file mode 100644 index 00000000000..61a895985f3 --- /dev/null +++ b/packages/cli/src/credentials/__tests__/validation.test.ts @@ -0,0 +1,203 @@ +import { GLOBAL_OWNER_ROLE, GLOBAL_MEMBER_ROLE } from '@n8n/db'; +import type { User } from '@n8n/db'; +import { mock } from 'jest-mock-extended'; + +import { + validateExternalSecretsPermissions, + isChangingExternalSecretExpression, +} from '../validation'; + +describe('Credentials Validation', () => { + const ownerUser = mock({ id: 'owner-id', role: GLOBAL_OWNER_ROLE }); + const memberUser = mock({ id: 'member-id', role: GLOBAL_MEMBER_ROLE }); + const errorMessage = 'Lacking permissions to reference external secrets in credentials'; + + describe('validateExternalSecretsPermissions', () => { + it('should pass when credential data contains no external secrets', () => { + const data = { + apiKey: 'regular-key', + domain: 'example.com', + }; + + expect(() => validateExternalSecretsPermissions(memberUser, data)).not.toThrow(); + }); + + it('should pass when credential data contains external secrets and user has permission', () => { + const data = { + apiKey: '={{ $secrets.myApiKey }}', + domain: 'example.com', + }; + + expect(() => validateExternalSecretsPermissions(ownerUser, data)).not.toThrow(); + }); + + it('should throw BadRequestError when user lacks externalSecret:list permission', () => { + const data = { + apiKey: '={{ $secrets.myApiKey }}', + domain: 'example.com', + }; + + expect(() => validateExternalSecretsPermissions(memberUser, data)).toThrow(errorMessage); + }); + + it('should throw when user lacks permission and uses nested external secrets', () => { + const data = { + apiKey: 'regular-key', + advanced: { + token: '={{ $secrets.myToken }}', + }, + }; + + expect(() => validateExternalSecretsPermissions(memberUser, data)).toThrow(errorMessage); + }); + + it('should pass when user has permission and uses nested external secrets', () => { + const data = { + apiKey: 'regular-key', + advanced: { + token: '={{ $secrets.myToken }}', + }, + }; + + expect(() => validateExternalSecretsPermissions(ownerUser, data)).not.toThrow(); + }); + + it('should throw when updating nested credential with external secret without permission', () => { + const existingData = { + apiKey: 'key', + config: { token: 'old-token' }, + }; + const newData = { + apiKey: 'key', + config: { token: '={{ $secrets.newToken }}' }, + }; + + expect(() => validateExternalSecretsPermissions(memberUser, newData, existingData)).toThrow( + errorMessage, + ); + }); + }); + + describe('isChangingExternalSecretExpression', () => { + it('should return true when adding a new external secret expression', () => { + const existingData = { apiKey: 'plain-value' }; + const newData = { apiKey: '={{ $secrets.myKey }}' }; + + expect(isChangingExternalSecretExpression(newData, existingData)).toBe(true); + }); + + it('should return true when modifying an existing external secret expression', () => { + const existingData = { apiKey: '={{ $secrets.oldKey }}' }; + const newData = { apiKey: '={{ $secrets.newKey }}' }; + + expect(isChangingExternalSecretExpression(newData, existingData)).toBe(true); + }); + + it('should return false when keeping external secret unchanged while modifying other fields', () => { + const existingData = { apiKey: '={{ $secrets.myKey }}', domain: 'old.com' }; + const newData = { apiKey: '={{ $secrets.myKey }}', domain: 'new.com' }; + + expect(isChangingExternalSecretExpression(newData, existingData)).toBe(false); + }); + + it('should return false when removing an external secret expression', () => { + const existingData = { apiKey: '={{ $secrets.myKey }}' }; + const newData = { apiKey: 'plain-value' }; + + expect(isChangingExternalSecretExpression(newData, existingData)).toBe(false); + }); + + it('should return false when neither version contains external secrets', () => { + const existingData = { apiKey: 'value1' }; + const newData = { apiKey: 'value2' }; + + expect(isChangingExternalSecretExpression(newData, existingData)).toBe(false); + }); + + it('should return true when one external secret changed among multiple fields', () => { + const existingData = { key1: '={{ $secrets.a }}', key2: '={{ $secrets.b }}' }; + const newData = { key1: '={{ $secrets.a }}', key2: '={{ $secrets.c }}' }; + + expect(isChangingExternalSecretExpression(newData, existingData)).toBe(true); + }); + + it('should return true when adding external secret in nested object', () => { + const existingData = { apiKey: 'plain', config: { token: 'old-token' } }; + const newData = { apiKey: 'plain', config: { token: '={{ $secrets.myToken }}' } }; + + expect(isChangingExternalSecretExpression(newData, existingData)).toBe(true); + }); + + it('should return true when modifying external secret in nested object', () => { + const existingData = { apiKey: 'plain', config: { token: '$secrets.oldToken' } }; + const newData = { apiKey: 'plain', config: { token: '$secrets.newToken' } }; + + expect(isChangingExternalSecretExpression(newData, existingData)).toBe(true); + }); + + it('should return false when keeping nested external secret unchanged', () => { + const existingData = { apiKey: 'plain', config: { token: '={{ $secrets.myToken }}' } }; + const newData = { apiKey: 'plain', config: { token: '={{ $secrets.myToken }}' } }; + + expect(isChangingExternalSecretExpression(newData, existingData)).toBe(false); + }); + + it('should return false when removing nested external secret', () => { + const existingData = { config: { token: '={{ $secrets.myToken }}' } }; + const newData = { config: { token: 'plain-value' } }; + + expect(isChangingExternalSecretExpression(newData, existingData)).toBe(false); + }); + }); + + describe('bracket notation support', () => { + describe('validateExternalSecretsPermissions with bracket notation', () => { + it('should throw when user lacks permission and uses bracket notation', () => { + const data = { + apiKey: "={{ $secrets['vault']['key'] }}", + }; + + expect(() => validateExternalSecretsPermissions(memberUser, data)).toThrow(errorMessage); + }); + + it('should pass when user has permission and uses bracket notation', () => { + const data = { + apiKey: "={{ $secrets['vault']['key'] }}", + }; + + expect(() => validateExternalSecretsPermissions(ownerUser, data)).not.toThrow(); + }); + + it('should throw when user lacks permission and uses mixed notation', () => { + const data = { + apiKey: "={{ $secrets.vault['nested'] }}", + }; + + expect(() => validateExternalSecretsPermissions(memberUser, data)).toThrow(errorMessage); + }); + }); + + describe('isChangingExternalSecretExpression with bracket notation', () => { + it('should return true when adding bracket notation external secret', () => { + const existingData = { apiKey: 'plain-value' }; + const newData = { apiKey: "={{ $secrets['vault']['key'] }}" }; + + expect(isChangingExternalSecretExpression(newData, existingData)).toBe(true); + }); + + it('should return true when changing from dot to bracket notation', () => { + const existingData = { apiKey: '={{ $secrets.vault.key }}' }; + const newData = { apiKey: "={{ $secrets['vault']['key'] }}" }; + + expect(isChangingExternalSecretExpression(newData, existingData)).toBe(true); + }); + + it('should return false when keeping bracket notation unchanged', () => { + const existingData = { apiKey: "={{ $secrets['vault']['key'] }}" }; + const newData = { apiKey: "={{ $secrets['vault']['key'] }}" }; + + expect(isChangingExternalSecretExpression(newData, existingData)).toBe(false); + }); + }); + }); +}); diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index 965966262d9..3ddcc5c8bc2 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -232,7 +232,9 @@ export class CredentialsController { const decryptedData = this.credentialsService.decrypt(credential, true); // We never want to allow users to change the oauthTokenData delete body.data?.oauthTokenData; + const preparedCredentialData = await this.credentialsService.prepareUpdateData( + req.user, req.body, decryptedData, ); diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 899cd8a232f..417444db5ec 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -51,7 +51,8 @@ import { CredentialsTester } from '@/services/credentials-tester.service'; import { OwnershipService } from '@/services/ownership.service'; import { ProjectService } from '@/services/project.service.ee'; import { RoleService } from '@/services/role.service'; -import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; + +import { validateExternalSecretsPermissions } from './validation'; export type CredentialsGetSharedOptions = | { allowGlobalScope: true; globalScope: Scope } @@ -445,9 +446,12 @@ export class CredentialsService { } async prepareUpdateData( + user: User, data: CredentialRequest.CredentialProperties, decryptedData: ICredentialDataDecryptedObject, ): Promise { + validateExternalSecretsPermissions(user, data.data, decryptedData); + const mergedData = deepCopy(data); if (mergedData.data) { mergedData.data = this.unredact(mergedData.data, decryptedData); @@ -865,7 +869,12 @@ export class CredentialsService { return await this.createCredential({ ...dto, isManaged: false }, user); } - private checkCredentialData(type: string, data: ICredentialDataDecryptedObject) { + /** + * Used to check credential data for creating a new credential. + * TODO: consider refactoring enable using this for both creating and updating, right now only used for creation + * (likely only affects the validateExternalSecretsPermissions call) + */ + checkCredentialData(type: string, data: ICredentialDataDecryptedObject, user: User) { // check mandatory fields are present const credentialProperties = this.credentialsHelper.getCredentialsProperties(type); for (const property of credentialProperties) { @@ -882,6 +891,7 @@ export class CredentialsService { } } + validateExternalSecretsPermissions(user, data); // TODO: add further validation if needed } @@ -894,7 +904,7 @@ export class CredentialsService { } private async createCredential(opts: CreateCredentialOptions, user: User) { - this.checkCredentialData(opts.type, opts.data as ICredentialDataDecryptedObject); + this.checkCredentialData(opts.type, opts.data as ICredentialDataDecryptedObject, user); const encryptedCredential = this.createEncryptedData({ id: null, name: opts.name, diff --git a/packages/cli/src/credentials/validation.ts b/packages/cli/src/credentials/validation.ts new file mode 100644 index 00000000000..6037521b12b --- /dev/null +++ b/packages/cli/src/credentials/validation.ts @@ -0,0 +1,76 @@ +import type { User } from '@n8n/db'; +import { hasGlobalScope } from '@n8n/permissions'; +import get from 'lodash/get'; +import type { ICredentialDataDecryptedObject } from 'n8n-workflow'; + +import { BadRequestError } from '@/errors/response-errors/bad-request.error'; +import { getAllKeyPaths } from '@/utils'; + +// #region External Secrets + +/** + * Checks if a string value contains an external secret expression. + * Detects both dot notation ($secrets.vault.key) and bracket notation ($secrets['vault']['key']). + */ +function containsExternalSecretExpression(value: string): boolean { + return value.includes('$secrets.') || value.includes('$secrets['); +} + +/** + * Checks if credential data contains any external secret expressions ($secrets) + */ +function containsExternalSecrets(data: ICredentialDataDecryptedObject): boolean { + const secretPaths = getAllKeyPaths(data, '', [], containsExternalSecretExpression); + return secretPaths.length > 0; +} + +/** + * Checks if any changed field in a credential contains an external secret expression + */ +export function isChangingExternalSecretExpression( + newData: ICredentialDataDecryptedObject, + existingData: ICredentialDataDecryptedObject, +): boolean { + // Find all paths in newData that contain external secret expressions + const newSecretPaths = getAllKeyPaths(newData, '', [], containsExternalSecretExpression); + + // Check if any of these paths represent a change from existingData + for (const path of newSecretPaths) { + const newValue = get(newData, path); + const existingValue = get(existingData, path); + + if (newValue !== existingValue) { + return true; // External secret expression is being added or modified + } + } + + return false; +} + +/** + * Validates if a user has permission to use external secrets in credentials + * + * @param dataToSave - only optional in case it's not provided in the payload of the request + * @param decryptedExistingData - Optional existing credential data (optional as it can only be provided when updating an existing credential) + * @throws {BadRequestError} If user lacks permission when attempting to use external secrets + */ +export function validateExternalSecretsPermissions( + user: User, + dataToSave?: ICredentialDataDecryptedObject, + decryptedExistingData?: ICredentialDataDecryptedObject, +): void { + if (!dataToSave) { + return; + } + const isUpdatingExistingCredential = !!decryptedExistingData; + const needsCheck = isUpdatingExistingCredential + ? isChangingExternalSecretExpression(dataToSave, decryptedExistingData) + : containsExternalSecrets(dataToSave); + if (needsCheck) { + if (!hasGlobalScope(user, 'externalSecret:list')) { + throw new BadRequestError('Lacking permissions to reference external secrets in credentials'); + } + } +} + +// #endregion diff --git a/packages/cli/src/public-api/v1/handlers/credentials/__tests__/credentials.service.test.ts b/packages/cli/src/public-api/v1/handlers/credentials/__tests__/credentials.service.test.ts index 77489c14e3d..60c437c7622 100644 --- a/packages/cli/src/public-api/v1/handlers/credentials/__tests__/credentials.service.test.ts +++ b/packages/cli/src/public-api/v1/handlers/credentials/__tests__/credentials.service.test.ts @@ -1,9 +1,17 @@ +import { Container } from '@n8n/di'; +import { mock } from 'jest-mock-extended'; +import type { InstanceSettings } from 'n8n-core'; +import { Cipher } from 'n8n-core'; import type { GenericValue, IDataObject, INodeProperties } from 'n8n-workflow'; import type { IDependency } from '@/public-api/types'; import { toJsonSchema } from '../credentials.service'; +// Set up real Cipher with mocked InstanceSettings for encryption +const cipher = new Cipher(mock({ encryptionKey: 'test-encryption-key' })); +Container.set(Cipher, cipher); + describe('CredentialsService', () => { describe('toJsonSchema', () => { it('should add "false" displayOptions.show dependant value as allof condition', () => { diff --git a/packages/cli/src/public-api/v1/handlers/credentials/credentials.handler.ts b/packages/cli/src/public-api/v1/handlers/credentials/credentials.handler.ts index d9335310248..9f1603b1bcc 100644 --- a/packages/cli/src/public-api/v1/handlers/credentials/credentials.handler.ts +++ b/packages/cli/src/public-api/v1/handlers/credentials/credentials.handler.ts @@ -10,8 +10,6 @@ import { CredentialsHelper } from '@/credentials-helper'; import { validCredentialsProperties, validCredentialType } from './credentials.middleware'; import { - createCredential, - encryptCredential, getCredentials, getSharedCredentials, removeCredential, @@ -32,13 +30,7 @@ export = { res: express.Response, ): Promise>> => { try { - const newCredential = await createCredential(req.body); - - const encryptedData = await encryptCredential(newCredential); - - Object.assign(newCredential, encryptedData); - - const savedCredential = await saveCredential(newCredential, req.user, encryptedData); + const savedCredential = await saveCredential(req.body, req.user); return res.json(sanitizeCredentials(savedCredential)); } catch ({ message, httpStatusCode }) { diff --git a/packages/cli/src/public-api/v1/handlers/credentials/credentials.service.ts b/packages/cli/src/public-api/v1/handlers/credentials/credentials.service.ts index a9050406557..9879de63d99 100644 --- a/packages/cli/src/public-api/v1/handlers/credentials/credentials.service.ts +++ b/packages/cli/src/public-api/v1/handlers/credentials/credentials.service.ts @@ -10,11 +10,13 @@ import { Container } from '@n8n/di'; import { Credentials } from 'n8n-core'; import type { DisplayCondition, + ICredentialDataDecryptedObject, IDataObject, INodeProperties, INodePropertyOptions, } from 'n8n-workflow'; +import { validateExternalSecretsPermissions } from '@/credentials/validation'; import { EventService } from '@/events/event.service'; import { ExternalHooks } from '@/external-hooks'; import type { CredentialRequest } from '@/requests'; @@ -49,10 +51,16 @@ export async function createCredential( } export async function saveCredential( - credential: CredentialsEntity, + payload: { type: string; name: string; data: ICredentialDataDecryptedObject }, user: User, - encryptedData: ICredentialsDb, ): Promise { + const credential = await createCredential(payload); + + validateExternalSecretsPermissions(user, payload.data); + + const encryptedData = await encryptCredential(credential); + Object.assign(credential, encryptedData); + const projectRepository = Container.get(ProjectRepository); const { manager: dbManager } = projectRepository; const result = await dbManager.transaction(async (transactionManager) => { diff --git a/packages/cli/src/public-api/v1/handlers/credentials/spec/paths/credentials.yml b/packages/cli/src/public-api/v1/handlers/credentials/spec/paths/credentials.yml index 13a1be6de43..50e40da9bb0 100644 --- a/packages/cli/src/public-api/v1/handlers/credentials/spec/paths/credentials.yml +++ b/packages/cli/src/public-api/v1/handlers/credentials/spec/paths/credentials.yml @@ -19,6 +19,8 @@ post: application/json: schema: $ref: '../schemas/create-credential-response.yml' + '400': + description: Bad request - invalid credential type or data. '401': $ref: '../../../../shared/spec/responses/unauthorized.yml' '415':