fix(core): Do not save credential overwrites data into the database (#13170)

packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts

@@ -1,9 +1,8 @@
 import { Container } from '@n8n/di';
 import Csrf from 'csrf';
 import type { Response } from 'express';
-import { mock } from 'jest-mock-extended';
-import { Cipher } from 'n8n-core';
-import { Logger } from 'n8n-core';
+import { captor, mock } from 'jest-mock-extended';
+import { Cipher, type InstanceSettings, Logger } from 'n8n-core';
 import type { IWorkflowExecuteAdditionalData } from 'n8n-workflow';
 import nock from 'nock';
 
@@ -35,7 +34,8 @@ describe('OAuth1CredentialController', () => {
 	const additionalData = mock<IWorkflowExecuteAdditionalData>();
 	(WorkflowExecuteAdditionalData.getBase as jest.Mock).mockReturnValue(additionalData);
 
-	const cipher = mockInstance(Cipher);
+	const cipher = new Cipher(mock<InstanceSettings>({ encryptionKey: 'password' }));
+	Container.set(Cipher, cipher);
 	const credentialsHelper = mockInstance(CredentialsHelper);
 	const credentialsRepository = mockInstance(CredentialsRepository);
 	const sharedCredentialsRepository = mockInstance(SharedCredentialsRepository);
@@ -51,6 +51,7 @@ describe('OAuth1CredentialController', () => {
 		id: '1',
 		name: 'Test Credential',
 		type: 'oAuth1Api',
+		data: cipher.encrypt({}),
 	});
 
 	const controller = Container.get(OAuth1CredentialController);
@@ -98,21 +99,23 @@ describe('OAuth1CredentialController', () => {
 				})
 				.once()
 				.reply(200, { oauth_token: 'random-token' });
-			cipher.encrypt.mockReturnValue('encrypted');
 
 			const req = mock<OAuthRequest.OAuth1Credential.Auth>({ user, query: { id: '1' } });
 			const authUri = await controller.getAuthUri(req);
 			expect(authUri).toEqual('https://example.domain/oauth/authorize?oauth_token=random-token');
+			const dataCaptor = captor();
 			expect(credentialsRepository.update).toHaveBeenCalledWith(
 				'1',
 				expect.objectContaining({
-					data: 'encrypted',
+					data: dataCaptor,
 					id: '1',
 					name: 'Test Credential',
 					type: 'oAuth1Api',
 				}),
 			);
-			expect(cipher.encrypt).toHaveBeenCalledWith({ csrfSecret });
+			expect(cipher.decrypt(dataCaptor.value)).toEqual(
+				JSON.stringify({ csrfSecret: 'csrf-secret' }),
+			);
 			expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith(
 				additionalData,
 				credential,
@@ -233,22 +236,22 @@ describe('OAuth1CredentialController', () => {
 				})
 				.once()
 				.reply(200, 'access_token=new_token');
-			cipher.encrypt.mockReturnValue('encrypted');
 
 			await controller.handleCallback(req, res);
 
-			expect(cipher.encrypt).toHaveBeenCalledWith({
-				oauthTokenData: { access_token: 'new_token' },
-			});
+			const dataCaptor = captor();
 			expect(credentialsRepository.update).toHaveBeenCalledWith(
 				'1',
 				expect.objectContaining({
-					data: 'encrypted',
+					data: dataCaptor,
 					id: '1',
 					name: 'Test Credential',
 					type: 'oAuth1Api',
 				}),
 			);
+			expect(cipher.decrypt(dataCaptor.value)).toEqual(
+				JSON.stringify({ oauthTokenData: { access_token: 'new_token' } }),
+			);
 			expect(res.render).toHaveBeenCalledWith('oauth-callback');
 			expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith(
 				additionalData,

packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts

@@ -1,9 +1,8 @@
 import { Container } from '@n8n/di';
 import Csrf from 'csrf';
 import { type Response } from 'express';
-import { mock } from 'jest-mock-extended';
-import { Cipher } from 'n8n-core';
-import { Logger } from 'n8n-core';
+import { captor, mock } from 'jest-mock-extended';
+import { Cipher, type InstanceSettings, Logger } from 'n8n-core';
 import type { IWorkflowExecuteAdditionalData } from 'n8n-workflow';
 import nock from 'nock';
 
@@ -34,7 +33,9 @@ describe('OAuth2CredentialController', () => {
 	const additionalData = mock<IWorkflowExecuteAdditionalData>();
 	(WorkflowExecuteAdditionalData.getBase as jest.Mock).mockReturnValue(additionalData);
 
-	const cipher = mockInstance(Cipher);
+	const cipher = new Cipher(mock<InstanceSettings>({ encryptionKey: 'password' }));
+	Container.set(Cipher, cipher);
+
 	const externalHooks = mockInstance(ExternalHooks);
 	const credentialsHelper = mockInstance(CredentialsHelper);
 	const credentialsRepository = mockInstance(CredentialsRepository);
@@ -51,6 +52,7 @@ describe('OAuth2CredentialController', () => {
 		id: '1',
 		name: 'Test Credential',
 		type: 'oAuth2Api',
+		data: cipher.encrypt({}),
 	});
 
 	const controller = Container.get(OAuth2CredentialController);
@@ -92,7 +94,6 @@ describe('OAuth2CredentialController', () => {
 			jest.spyOn(Csrf.prototype, 'create').mockReturnValueOnce('token');
 			sharedCredentialsRepository.findCredentialForUser.mockResolvedValueOnce(credential);
 			credentialsHelper.getDecrypted.mockResolvedValueOnce({});
-			cipher.encrypt.mockReturnValue('encrypted');
 
 			const req = mock<OAuthRequest.OAuth2Credential.Auth>({ user, query: { id: '1' } });
 			const authUri = await controller.getAuthUri(req);
@@ -106,15 +107,19 @@ describe('OAuth2CredentialController', () => {
 				createdAt: timestamp,
 				userId: '123',
 			});
+			const dataCaptor = captor();
 			expect(credentialsRepository.update).toHaveBeenCalledWith(
 				'1',
 				expect.objectContaining({
-					data: 'encrypted',
+					data: dataCaptor,
 					id: '1',
 					name: 'Test Credential',
 					type: 'oAuth2Api',
 				}),
 			);
+			expect(cipher.decrypt(dataCaptor.value)).toEqual(
+				JSON.stringify({ csrfSecret: 'csrf-secret' }),
+			);
 			expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith(
 				additionalData,
 				credential,
@@ -248,7 +253,6 @@ describe('OAuth2CredentialController', () => {
 					'code=code&grant_type=authorization_code&redirect_uri=http%3A%2F%2Flocalhost%3A5678%2Frest%2Foauth2-credential%2Fcallback',
 				)
 				.reply(200, { access_token: 'access-token', refresh_token: 'refresh-token' });
-			cipher.encrypt.mockReturnValue('encrypted');
 
 			await controller.handleCallback(req, res);
 
@@ -258,18 +262,21 @@ describe('OAuth2CredentialController', () => {
 					redirectUri: 'http://localhost:5678/rest/oauth2-credential/callback',
 				}),
 			]);
-			expect(cipher.encrypt).toHaveBeenCalledWith({
-				oauthTokenData: { access_token: 'access-token', refresh_token: 'refresh-token' },
-			});
+			const dataCaptor = captor();
 			expect(credentialsRepository.update).toHaveBeenCalledWith(
 				'1',
 				expect.objectContaining({
-					data: 'encrypted',
+					data: dataCaptor,
 					id: '1',
 					name: 'Test Credential',
 					type: 'oAuth2Api',
 				}),
 			);
+			expect(cipher.decrypt(dataCaptor.value)).toEqual(
+				JSON.stringify({
+					oauthTokenData: { access_token: 'access-token', refresh_token: 'refresh-token' },
+				}),
+			);
 			expect(res.render).toHaveBeenCalledWith('oauth-callback');
 			expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith(
 				additionalData,
@@ -294,7 +301,6 @@ describe('OAuth2CredentialController', () => {
 					'code=code&grant_type=authorization_code&redirect_uri=http%3A%2F%2Flocalhost%3A5678%2Frest%2Foauth2-credential%2Fcallback',
 				)
 				.reply(200, { access_token: 'access-token', refresh_token: 'refresh-token' });
-			cipher.encrypt.mockReturnValue('encrypted');
 
 			await controller.handleCallback(req, res);
 
@@ -304,22 +310,25 @@ describe('OAuth2CredentialController', () => {
 					redirectUri: 'http://localhost:5678/rest/oauth2-credential/callback',
 				}),
 			]);
-			expect(cipher.encrypt).toHaveBeenCalledWith({
-				oauthTokenData: {
-					token: true,
-					access_token: 'access-token',
-					refresh_token: 'refresh-token',
-				},
-			});
+			const dataCaptor = captor();
 			expect(credentialsRepository.update).toHaveBeenCalledWith(
 				'1',
 				expect.objectContaining({
-					data: 'encrypted',
+					data: dataCaptor,
 					id: '1',
 					name: 'Test Credential',
 					type: 'oAuth2Api',
 				}),
 			);
+			expect(cipher.decrypt(dataCaptor.value)).toEqual(
+				JSON.stringify({
+					oauthTokenData: {
+						token: true,
+						access_token: 'access-token',
+						refresh_token: 'refresh-token',
+					},
+				}),
+			);
 			expect(res.render).toHaveBeenCalledWith('oauth-callback');
 		});
 
@@ -336,7 +345,6 @@ describe('OAuth2CredentialController', () => {
 					'code=code&grant_type=authorization_code&redirect_uri=http%3A%2F%2Flocalhost%3A5678%2Frest%2Foauth2-credential%2Fcallback',
 				)
 				.reply(200, { access_token: 'access-token', refresh_token: 'refresh-token' });
-			cipher.encrypt.mockReturnValue('encrypted');
 
 			await controller.handleCallback(req, res);
 
@@ -346,18 +354,21 @@ describe('OAuth2CredentialController', () => {
 					redirectUri: 'http://localhost:5678/rest/oauth2-credential/callback',
 				}),
 			]);
-			expect(cipher.encrypt).toHaveBeenCalledWith({
-				oauthTokenData: { access_token: 'access-token', refresh_token: 'refresh-token' },
-			});
+			const dataCaptor = captor();
 			expect(credentialsRepository.update).toHaveBeenCalledWith(
 				'1',
 				expect.objectContaining({
-					data: 'encrypted',
+					data: dataCaptor,
 					id: '1',
 					name: 'Test Credential',
 					type: 'oAuth2Api',
 				}),
 			);
+			expect(cipher.decrypt(dataCaptor.value)).toEqual(
+				JSON.stringify({
+					oauthTokenData: { access_token: 'access-token', refresh_token: 'refresh-token' },
+				}),
+			);
 			expect(res.render).toHaveBeenCalledWith('oauth-callback');
 		});
 	});

packages/cli/src/controllers/oauth/abstract-oauth.controller.ts

@@ -136,10 +136,11 @@ export abstract class AbstractOAuthController {
 
 	protected async encryptAndSaveData(
 		credential: ICredentialsDb,
-		decryptedData: ICredentialDataDecryptedObject,
+		toUpdate: ICredentialDataDecryptedObject,
+		toDelete: string[] = [],
 	) {
-		const credentials = new Credentials(credential, credential.type);
-		credentials.setData(decryptedData);
+		const credentials = new Credentials(credential, credential.type, credential.data);
+		credentials.updateData(toUpdate, toDelete);
 		await this.credentialsRepository.update(credential.id, {
 			...credentials.getDataToSave(),
 			updatedAt: new Date(),

packages/cli/src/controllers/oauth/oauth1-credential.controller.ts

@@ -91,8 +91,7 @@ export class OAuth1CredentialController extends AbstractOAuthController {
 
 		const returnUri = `${oauthCredentials.authUrl}?oauth_token=${responseJson.oauth_token}`;
 
-		decryptedDataOriginal.csrfSecret = csrfSecret;
-		await this.encryptAndSaveData(credential, decryptedDataOriginal);
+		await this.encryptAndSaveData(credential, { csrfSecret });
 
 		this.logger.debug('OAuth1 authorization successful for new credential', {
 			userId: req.user.id,
@@ -116,7 +115,7 @@ export class OAuth1CredentialController extends AbstractOAuthController {
 				);
 			}
 
-			const [credential, decryptedDataOriginal, oauthCredentials] =
+			const [credential, _, oauthCredentials] =
 				await this.resolveCredential<OAuth1CredentialData>(req);
 
 			const oauthToken = await axios.post<string>(oauthCredentials.accessTokenUrl, {
@@ -128,12 +127,9 @@ export class OAuth1CredentialController extends AbstractOAuthController {
 
 			const paramParser = new URLSearchParams(oauthToken.data);
 
-			const oauthTokenJson = Object.fromEntries(paramParser.entries());
+			const oauthTokenData = Object.fromEntries(paramParser.entries());
 
-			delete decryptedDataOriginal.csrfSecret;
-			decryptedDataOriginal.oauthTokenData = oauthTokenJson;
-
-			await this.encryptAndSaveData(credential, decryptedDataOriginal);
+			await this.encryptAndSaveData(credential, { oauthTokenData }, ['csrfSecret']);
 
 			this.logger.debug('OAuth1 callback successful for new credential', {
 				credentialId: credential.id,

packages/cli/src/controllers/oauth/oauth2-credential.controller.ts

@@ -4,7 +4,7 @@ import { Response } from 'express';
 import omit from 'lodash/omit';
 import set from 'lodash/set';
 import split from 'lodash/split';
-import { jsonStringify } from 'n8n-workflow';
+import { type ICredentialDataDecryptedObject, jsonStringify } from 'n8n-workflow';
 import pkceChallenge from 'pkce-challenge';
 import * as qs from 'querystring';
 
@@ -60,7 +60,7 @@ export class OAuth2CredentialController extends AbstractOAuthController {
 
 		await this.externalHooks.run('oauth2.authenticate', [oAuthOptions]);
 
-		decryptedDataOriginal.csrfSecret = csrfSecret;
+		const toUpdate: ICredentialDataDecryptedObject = { csrfSecret };
 		if (oauthCredentials.grantType === 'pkce') {
 			const { code_verifier, code_challenge } = pkceChallenge();
 			oAuthOptions.query = {
@@ -68,10 +68,10 @@ export class OAuth2CredentialController extends AbstractOAuthController {
 				code_challenge,
 				code_challenge_method: 'S256',
 			};
-			decryptedDataOriginal.codeVerifier = code_verifier;
+			toUpdate.codeVerifier = code_verifier;
 		}
 
-		await this.encryptAndSaveData(credential, decryptedDataOriginal);
+		await this.encryptAndSaveData(credential, toUpdate);
 
 		const oAuthObj = new ClientOAuth2(oAuthOptions);
 		const returnUri = oAuthObj.code.getUri();
@@ -133,17 +133,15 @@ export class OAuth2CredentialController extends AbstractOAuthController {
 				set(oauthToken.data, 'callbackQueryString', omit(req.query, 'state', 'code'));
 			}
 
-			if (typeof decryptedDataOriginal.oauthTokenData === 'object') {
-				// Only overwrite supplied data as some providers do for example just return the
-				// refresh_token on the very first request and not on subsequent ones.
-				Object.assign(decryptedDataOriginal.oauthTokenData, oauthToken.data);
-			} else {
-				// No data exists so simply set
-				decryptedDataOriginal.oauthTokenData = oauthToken.data;
-			}
+			// Only overwrite supplied data as some providers do for example just return the
+			// refresh_token on the very first request and not on subsequent ones.
+			let { oauthTokenData } = decryptedDataOriginal;
+			oauthTokenData = {
+				...(typeof oauthTokenData === 'object' ? oauthTokenData : {}),
+				...oauthToken.data,
+			};
 
-			delete decryptedDataOriginal.csrfSecret;
-			await this.encryptAndSaveData(credential, decryptedDataOriginal);
+			await this.encryptAndSaveData(credential, { oauthTokenData }, ['csrfSecret']);
 
 			this.logger.debug('OAuth2 callback successful for credential', {
 				credentialId: credential.id,

packages/core/src/__tests__/credentials.test.ts

@@ -118,4 +118,74 @@ describe('Credentials', () => {
 			},
 		);
 	});
+
+	describe('updateData', () => {
+		const nodeCredentials = { id: '123', name: 'Test Credential' };
+		const credentialType = 'testApi';
+
+		test('should update existing data', () => {
+			const credentials = new Credentials(
+				nodeCredentials,
+				credentialType,
+				cipher.encrypt({
+					username: 'olduser',
+					password: 'oldpass',
+					apiKey: 'oldkey',
+				}),
+			);
+
+			credentials.updateData({ username: 'newuser', password: 'newpass' });
+
+			expect(credentials.getData()).toEqual({
+				username: 'newuser',
+				password: 'newpass',
+				apiKey: 'oldkey',
+			});
+		});
+
+		test('should delete specified keys', () => {
+			const credentials = new Credentials(
+				nodeCredentials,
+				credentialType,
+				cipher.encrypt({
+					username: 'testuser',
+					password: 'testpass',
+					apiKey: 'testkey',
+				}),
+			);
+
+			credentials.updateData({}, ['username', 'apiKey']);
+
+			expect(credentials.getData()).toEqual({
+				password: 'testpass',
+			});
+		});
+
+		test('should update and delete keys in same operation', () => {
+			const credentials = new Credentials(
+				nodeCredentials,
+				credentialType,
+				cipher.encrypt({
+					username: 'olduser',
+					password: 'oldpass',
+					apiKey: 'oldkey',
+				}),
+			);
+
+			credentials.updateData({ username: 'newuser' }, ['apiKey']);
+
+			expect(credentials.getData()).toEqual({
+				username: 'newuser',
+				password: 'oldpass',
+			});
+		});
+
+		test('should throw an error if no data was previously set', () => {
+			const credentials = new Credentials(nodeCredentials, credentialType);
+
+			expect(() => {
+				credentials.updateData({ username: 'newuser' });
+			}).toThrow(CREDENTIAL_ERRORS.NO_DATA);
+		});
+	});
 });

packages/core/src/credentials.ts

@@ -30,6 +30,18 @@ export class Credentials<
 		this.data = this.cipher.encrypt(data);
 	}
 
+	/**
+	 * Update parts of the credential data.
+	 * This decrypts the data, modifies it, and then re-encrypts the updated data back to a string.
+	 */
+	updateData(toUpdate: Partial<T>, toDelete: Array<keyof T> = []) {
+		const updatedData: T = { ...this.getData(), ...toUpdate };
+		for (const key of toDelete) {
+			delete updatedData[key];
+		}
+		this.setData(updatedData);
+	}
+
 	/**
 	 * Returns the decrypted credential object
 	 */