feat: Hide credentials password values (#4868)

* feat: redact password field values in credentials

* feat: disable expanding password fields

* fix: redacting credentials without a valid type

This only seems to be a thing in testing?

packages/cli/src/constants.ts

@@ -50,3 +50,5 @@ export const SETTINGS_LICENSE_CERT_KEY = 'license.cert';
 export enum LICENSE_FEATURES {
 	SHARING = 'feat:sharing',
 }
+
+export const CREDENTIAL_BLANKING_VALUE = '__n8n_BLANK_VALUE_e5362baf-c777-4d57-a609-6eaf1f9e87f6';

packages/cli/src/credentials/credentials.controller.ee.ts

@@ -1,5 +1,5 @@
 import express from 'express';
-import { INodeCredentialTestResult, LoggerProxy } from 'n8n-workflow';
+import { deepCopy, INodeCredentialTestResult, LoggerProxy } from 'n8n-workflow';
 import * as Db from '@/Db';
 import { InternalHooksManager } from '@/InternalHooksManager';
 import * as ResponseHelper from '@/ResponseHelper';
@@ -91,7 +91,10 @@ EECredentialsController.get(
 		const { id, data: _, ...rest } = credential;
 
 		const key = await EECredentials.getEncryptionKey();
-		const decryptedData = await EECredentials.decrypt(key, credential);
+		const decryptedData = EECredentials.redact(
+			await EECredentials.decrypt(key, credential),
+			credential,
+		);
 
 		// @TODO_TECH_DEBT: Stringify `id` with entity field transformer
 		return { id: id.toString(), data: decryptedData, ...rest };
@@ -112,8 +115,8 @@ EECredentialsController.post(
 
 		const { ownsCredential } = await EECredentials.isOwned(req.user, credentials.id.toString());
 
+		const sharing = await EECredentials.getSharing(req.user, credentials.id);
 		if (!ownsCredential) {
-			const sharing = await EECredentials.getSharing(req.user, credentials.id);
 			if (!sharing) {
 				throw new ResponseHelper.UnauthorizedError(`Forbidden`);
 			}
@@ -122,7 +125,13 @@ EECredentialsController.post(
 			Object.assign(credentials, { data: decryptedData });
 		}
 
-		return EECredentials.test(req.user, encryptionKey, credentials);
+		const mergedCredentials = deepCopy(credentials);
+		if (mergedCredentials.data && sharing?.credentials) {
+			const decryptedData = await EECredentials.decrypt(encryptionKey, sharing.credentials);
+			mergedCredentials.data = EECredentials.unredact(mergedCredentials.data, decryptedData);
+		}
+
+		return EECredentials.test(req.user, encryptionKey, mergedCredentials);
 	}),
 );
 

packages/cli/src/credentials/credentials.controller.ts

@@ -1,7 +1,14 @@
 /* eslint-disable no-param-reassign */
 /* eslint-disable @typescript-eslint/no-unused-vars */
 import express from 'express';
-import { INodeCredentialTestResult, LoggerProxy } from 'n8n-workflow';
+import {
+	deepCopy,
+	ICredentialType,
+	INodeCredentialTestResult,
+	LoggerProxy,
+	NodeHelpers,
+} from 'n8n-workflow';
+import { Credentials } from 'n8n-core';
 
 import * as GenericHelpers from '@/GenericHelpers';
 import { InternalHooksManager } from '@/InternalHooksManager';
@@ -10,6 +17,7 @@ import config from '@/config';
 import { getLogger } from '@/Logger';
 import { EECredentialsController } from './credentials.controller.ee';
 import { CredentialsService } from './credentials.service';
+import { CredentialTypes } from '@/CredentialTypes';
 
 import type { ICredentialsResponse } from '@/Interfaces';
 import type { CredentialRequest } from '@/requests';
@@ -88,17 +96,18 @@ credentialsController.get(
 
 		const { credentials: credential } = sharing;
 
-		if (!includeDecryptedData) {
-			const { id, data: _, ...rest } = credential;
+		const { id, data: _, ...rest } = credential;
 
+		if (!includeDecryptedData) {
 			// @TODO_TECH_DEBT: Stringify `id` with entity field transformer
 			return { id: id.toString(), ...rest };
 		}
 
-		const { id, data: _, ...rest } = credential;
-
 		const key = await CredentialsService.getEncryptionKey();
-		const decryptedData = await CredentialsService.decrypt(key, credential);
+		const decryptedData = CredentialsService.redact(
+			await CredentialsService.decrypt(key, credential),
+			credential,
+		);
 
 		// @TODO_TECH_DEBT: Stringify `id` with entity field transformer
 		return { id: id.toString(), data: decryptedData, ...rest };
@@ -116,7 +125,15 @@ credentialsController.post(
 		const { credentials } = req.body;
 
 		const encryptionKey = await CredentialsService.getEncryptionKey();
-		return CredentialsService.test(req.user, encryptionKey, credentials);
+		const sharing = await CredentialsService.getSharing(req.user, credentials.id);
+
+		const mergedCredentials = deepCopy(credentials);
+		if (mergedCredentials.data && sharing?.credentials) {
+			const decryptedData = await CredentialsService.decrypt(encryptionKey, sharing.credentials);
+			mergedCredentials.data = CredentialsService.unredact(mergedCredentials.data, decryptedData);
+		}
+
+		return CredentialsService.test(req.user, encryptionKey, mergedCredentials);
 	}),
 );
 

packages/cli/src/credentials/credentials.service.ts

@@ -1,10 +1,14 @@
 /* eslint-disable no-restricted-syntax */
 import { Credentials, UserSettings } from 'n8n-core';
 import {
+	deepCopy,
 	ICredentialDataDecryptedObject,
 	ICredentialsDecrypted,
+	ICredentialType,
 	INodeCredentialTestResult,
+	INodeProperties,
 	LoggerProxy,
+	NodeHelpers,
 } from 'n8n-workflow';
 import { FindManyOptions, FindOneOptions, In } from 'typeorm';
 
@@ -12,7 +16,7 @@ import * as Db from '@/Db';
 import * as ResponseHelper from '@/ResponseHelper';
 import { ICredentialsDb } from '@/Interfaces';
 import { CredentialsHelper, createCredentialsFromCredentialsEntity } from '@/CredentialsHelper';
-import { RESPONSE_ERROR_MESSAGES } from '@/constants';
+import { CREDENTIAL_BLANKING_VALUE, RESPONSE_ERROR_MESSAGES } from '@/constants';
 import { CredentialsEntity } from '@db/entities/CredentialsEntity';
 import { SharedCredentials } from '@db/entities/SharedCredentials';
 import { validateEntity } from '@/GenericHelpers';
@@ -20,6 +24,7 @@ import { externalHooks } from '../Server';
 
 import type { User } from '@db/entities/User';
 import type { CredentialRequest } from '@/requests';
+import { CredentialTypes } from '@/CredentialTypes';
 
 export class CredentialsService {
 	static async get(
@@ -157,10 +162,15 @@ export class CredentialsService {
 		data: CredentialRequest.CredentialProperties,
 		decryptedData: ICredentialDataDecryptedObject,
 	): Promise<CredentialsEntity> {
+		const mergedData = deepCopy(data);
+		if (mergedData.data) {
+			mergedData.data = this.unredact(mergedData.data, decryptedData);
+		}
+
 		// This saves us a merge but requires some type casting. These
 		// types are compatiable for this case.
 		const updateData = Db.collections.Credentials.create(
-			data as ICredentialsDb,
+			mergedData as ICredentialsDb,
 		) as CredentialsEntity;
 
 		await validateEntity(updateData);
@@ -215,7 +225,9 @@ export class CredentialsService {
 		credential: CredentialsEntity,
 	): Promise<ICredentialDataDecryptedObject> {
 		const coreCredential = createCredentialsFromCredentialsEntity(credential);
-		return coreCredential.getData(encryptionKey);
+		const data = coreCredential.getData(encryptionKey);
+
+		return data;
 	}
 
 	static async update(
@@ -287,4 +299,90 @@ export class CredentialsService {
 
 		return helper.testCredentials(user, credentials.type, credentials);
 	}
+
+	// Take data and replace all sensitive values with a sentinel value.
+	// This will replace password fields and oauth data.
+	static redact(
+		data: ICredentialDataDecryptedObject,
+		credential: CredentialsEntity,
+	): ICredentialDataDecryptedObject {
+		const copiedData = deepCopy(data);
+
+		const credTypes = CredentialTypes();
+		let credType: ICredentialType;
+		try {
+			credType = credTypes.getByName(credential.type);
+		} catch {
+			// This _should_ only happen when testing. If it does happen in
+			// production it means it's either a mangled credential or a
+			// credential for a removed community node. Either way, there's
+			// no way to know what to redact.
+			return data;
+		}
+
+		const getExtendedProps = (type: ICredentialType) => {
+			const props: INodeProperties[] = [];
+			for (const e of type.extends ?? []) {
+				const extendsType = credTypes.getByName(e);
+				const extendedProps = getExtendedProps(extendsType);
+				NodeHelpers.mergeNodeProperties(props, extendedProps);
+			}
+			NodeHelpers.mergeNodeProperties(props, type.properties);
+			return props;
+		};
+		const properties = getExtendedProps(credType);
+
+		for (const dataKey of Object.keys(copiedData)) {
+			// The frontend only cares that this value isn't falsy.
+			if (dataKey === 'oauthTokenData') {
+				copiedData[dataKey] = CREDENTIAL_BLANKING_VALUE;
+				continue;
+			}
+			const prop = properties.find((v) => v.name === dataKey);
+			if (!prop) {
+				continue;
+			}
+			if (prop.typeOptions?.password) {
+				// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access
+				copiedData[dataKey] = CREDENTIAL_BLANKING_VALUE;
+			}
+		}
+
+		return copiedData;
+	}
+
+	// eslint-disable-next-line @typescript-eslint/no-explicit-any
+	private static unredactRestoreValues(unmerged: any, replacement: any) {
+		// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
+		for (const [key, value] of Object.entries(unmerged)) {
+			// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+			if (value === CREDENTIAL_BLANKING_VALUE) {
+				// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access
+				unmerged[key] = replacement[key];
+			} else if (
+				typeof value === 'object' &&
+				value !== null &&
+				key in replacement &&
+				// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+				typeof replacement[key] === 'object' &&
+				// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+				replacement[key] !== null
+			) {
+				// eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-unsafe-member-access
+				this.unredactRestoreValues(value, replacement[key]);
+			}
+		}
+	}
+
+	// Take unredacted data (probably from the DB) and merge it with
+	// redacted data to create an unredacted version.
+	static unredact(
+		redactedData: ICredentialDataDecryptedObject,
+		savedData: ICredentialDataDecryptedObject,
+	): ICredentialDataDecryptedObject {
+		// Replace any blank sentinel values with their saved version
+		const mergedData = deepCopy(redactedData);
+		this.unredactRestoreValues(mergedData, savedData);
+		return mergedData;
+	}
 }

packages/editor-ui/src/components/ParameterInput.vue

@@ -113,7 +113,7 @@
 				>
 					<template #suffix>
 						<n8n-icon
-							v-if="!isReadOnly"
+							v-if="!isReadOnly && !isSecretParameter"
 							icon="external-link-alt"
 							size="xsmall"
 							class="edit-window-button textarea-modal-opener"
@@ -770,6 +770,9 @@ export default mixins(
 		isResourceLocatorParameter(): boolean {
 			return this.parameter.type === 'resourceLocator';
 		},
+		isSecretParameter(): boolean {
+			return this.getArgument('password') === true;
+		},
 	},
 	methods: {
 		isRemoteParameterOption(option: INodePropertyOptions) {