fix(editor): sending data as query on DELETE method (#3972)

* 🐛 fix sending data as query on DELETE method

* 👕 add type to imports

* 💪 enhance test

packages/cli/src/UserManagement/routes/users.ts

@@ -1,13 +1,17 @@
 /* eslint-disable no-restricted-syntax */
 /* eslint-disable import/no-cycle */
 import { Response } from 'express';
+import { LoggerProxy as Logger } from 'n8n-workflow';
 import { In } from 'typeorm';
 import validator from 'validator';
-import { LoggerProxy as Logger } from 'n8n-workflow';
 
 import { Db, InternalHooksManager, ITelemetryUserDeletionData, ResponseHelper } from '../..';
-import { N8nApp, PublicUser } from '../Interfaces';
+import { SharedCredentials } from '../../databases/entities/SharedCredentials';
+import { SharedWorkflow } from '../../databases/entities/SharedWorkflow';
+import { User } from '../../databases/entities/User';
 import { UserRequest } from '../../requests';
+import * as UserManagementMailer from '../email/UserManagementMailer';
+import { N8nApp, PublicUser } from '../Interfaces';
 import {
 	getInstanceBaseUrl,
 	hashPassword,
@@ -16,10 +20,6 @@ import {
 	sanitizeUser,
 	validatePassword,
 } from '../UserManagementHelper';
-import { User } from '../../databases/entities/User';
-import { SharedWorkflow } from '../../databases/entities/SharedWorkflow';
-import { SharedCredentials } from '../../databases/entities/SharedCredentials';
-import * as UserManagementMailer from '../email/UserManagementMailer';
 
 import * as config from '../../../config';
 import { issueCookie } from '../auth/jwt';

packages/cli/src/api/nodes.api.ts

@@ -2,36 +2,36 @@
 import express from 'express';
 import { PublicInstalledPackage } from 'n8n-workflow';
 
+import { InternalHooksManager, LoadNodesAndCredentials, Push, ResponseHelper } from '..';
 import config from '../../config';
-import { ResponseHelper, LoadNodesAndCredentials, Push, InternalHooksManager } from '..';
 
 import {
-	RESPONSE_ERROR_MESSAGES,
-	UNKNOWN_FAILURE_REASON,
-	STARTER_TEMPLATE_NAME,
-} from '../constants';
-import {
-	matchMissingPackages,
-	matchPackagesWithUpdates,
-	executeCommand,
 	checkNpmPackageStatus,
+	executeCommand,
 	hasPackageLoaded,
-	removePackageFromMissingList,
-	parseNpmPackageName,
 	isClientError,
-	sanitizeNpmPackageName,
 	isNpmError,
+	matchMissingPackages,
+	matchPackagesWithUpdates,
+	parseNpmPackageName,
+	removePackageFromMissingList,
+	sanitizeNpmPackageName,
 } from '../CommunityNodes/helpers';
 import {
-	getAllInstalledPackages,
 	findInstalledPackage,
+	getAllInstalledPackages,
 	isPackageInstalled,
 } from '../CommunityNodes/packageModel';
+import {
+	RESPONSE_ERROR_MESSAGES,
+	STARTER_TEMPLATE_NAME,
+	UNKNOWN_FAILURE_REASON,
+} from '../constants';
 import { isAuthenticatedRequest } from '../UserManagement/UserManagementHelper';
 
-import type { NodeRequest } from '../requests';
-import type { CommunityPackages } from '../Interfaces';
 import { InstalledPackages } from '../databases/entities/InstalledPackages';
+import type { CommunityPackages } from '../Interfaces';
+import type { NodeRequest } from '../requests';
 
 const { PACKAGE_NOT_INSTALLED, PACKAGE_NAME_NOT_PROVIDED } = RESPONSE_ERROR_MESSAGES;
 
@@ -223,7 +223,7 @@ nodesController.get(
 nodesController.delete(
 	'/',
 	ResponseHelper.send(async (req: NodeRequest.Delete) => {
-		const { name } = req.body;
+		const { name } = req.query;
 
 		if (!name) {
 			throw new ResponseHelper.ResponseError(PACKAGE_NAME_NOT_PROVIDED, undefined, 400);

packages/cli/src/requests.d.ts

@@ -11,11 +11,11 @@ import {
 	IWorkflowSettings,
 } from 'n8n-workflow';
 
-import { User } from './databases/entities/User';
-import { Role } from './databases/entities/Role';
 import type { IExecutionDeleteFilter, IWorkflowDb } from '.';
-import type { PublicUser } from './UserManagement/Interfaces';
+import type { Role } from './databases/entities/Role';
+import type { User } from './databases/entities/User';
 import * as UserManagementMailer from './UserManagement/email/UserManagementMailer';
+import type { PublicUser } from './UserManagement/Interfaces';
 
 export type AuthlessRequest<
 	RouteParams = {},
@@ -302,7 +302,7 @@ export declare namespace NodeRequest {
 
 	type Post = AuthenticatedRequest<{}, {}, { name?: string }>;
 
-	type Delete = Post;
+	type Delete = AuthenticatedRequest<{}, {}, {}, { name: string }>;
 
 	type Update = Post;
 }

packages/cli/test/integration/credentials.api.test.ts

@@ -1,7 +1,7 @@
 import express from 'express';
 import { UserSettings } from 'n8n-core';
 import { Db } from '../../src';
-import { randomName, randomString } from './shared/random';
+import { randomCredentialPayload, randomName, randomString } from './shared/random';
 import * as utils from './shared/utils';
 import type { CredentialPayload, SaveCredentialFunction } from './shared/types';
 import type { Role } from '../../src/databases/entities/Role';
@@ -49,7 +49,7 @@ test('POST /credentials should create cred', async () => {
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
-	const payload = credentialPayload();
+	const payload = randomCredentialPayload();
 
 	const response = await authOwnerAgent.post('/credentials').send(payload);
 
@@ -97,7 +97,7 @@ test('POST /credentials should fail with missing encryption key', async () => {
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
 
-	const response = await authOwnerAgent.post('/credentials').send(credentialPayload());
+	const response = await authOwnerAgent.post('/credentials').send(randomCredentialPayload());
 
 	expect(response.statusCode).toBe(500);
 
@@ -110,21 +110,21 @@ test('POST /credentials should ignore ID in payload', async () => {
 
 	const firstResponse = await authOwnerAgent
 		.post('/credentials')
-		.send({ id: '8', ...credentialPayload() });
+		.send({ id: '8', ...randomCredentialPayload() });
 
 	expect(firstResponse.body.data.id).not.toBe('8');
 
 	const secondResponse = await authOwnerAgent
 		.post('/credentials')
-		.send({ id: 8, ...credentialPayload() });
+		.send({ id: 8, ...randomCredentialPayload() });
 
 	expect(secondResponse.body.data.id).not.toBe(8);
 });
 
 test('DELETE /credentials/:id should delete owned cred for owner', async () => {
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
-	const savedCredential = await saveCredential(credentialPayload(), { user: ownerShell });
+	const savedCredential = await saveCredential(randomCredentialPayload(), { user: ownerShell });
 
 	const response = await authOwnerAgent.delete(`/credentials/${savedCredential.id}`);
 
@@ -144,7 +144,7 @@ test('DELETE /credentials/:id should delete non-owned cred for owner', async ()
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
 	const member = await testDb.createUser({ globalRole: globalMemberRole });
-	const savedCredential = await saveCredential(credentialPayload(), { user: member });
+	const savedCredential = await saveCredential(randomCredentialPayload(), { user: member });
 
 	const response = await authOwnerAgent.delete(`/credentials/${savedCredential.id}`);
 
@@ -163,7 +163,7 @@ test('DELETE /credentials/:id should delete non-owned cred for owner', async ()
 test('DELETE /credentials/:id should delete owned cred for member', async () => {
 	const member = await testDb.createUser({ globalRole: globalMemberRole });
 	const authMemberAgent = utils.createAgent(app, { auth: true, user: member });
-	const savedCredential = await saveCredential(credentialPayload(), { user: member });
+	const savedCredential = await saveCredential(randomCredentialPayload(), { user: member });
 
 	const response = await authMemberAgent.delete(`/credentials/${savedCredential.id}`);
 
@@ -183,7 +183,7 @@ test('DELETE /credentials/:id should not delete non-owned cred for member', asyn
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 	const member = await testDb.createUser({ globalRole: globalMemberRole });
 	const authMemberAgent = utils.createAgent(app, { auth: true, user: member });
-	const savedCredential = await saveCredential(credentialPayload(), { user: ownerShell });
+	const savedCredential = await saveCredential(randomCredentialPayload(), { user: ownerShell });
 
 	const response = await authMemberAgent.delete(`/credentials/${savedCredential.id}`);
 
@@ -210,8 +210,8 @@ test('DELETE /credentials/:id should fail if cred not found', async () => {
 test('PATCH /credentials/:id should update owned cred for owner', async () => {
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
-	const savedCredential = await saveCredential(credentialPayload(), { user: ownerShell });
-	const patchPayload = credentialPayload();
+	const savedCredential = await saveCredential(randomCredentialPayload(), { user: ownerShell });
+	const patchPayload = randomCredentialPayload();
 
 	const response = await authOwnerAgent
 		.patch(`/credentials/${savedCredential.id}`)
@@ -245,8 +245,8 @@ test('PATCH /credentials/:id should update non-owned cred for owner', async () =
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
 	const member = await testDb.createUser({ globalRole: globalMemberRole });
-	const savedCredential = await saveCredential(credentialPayload(), { user: member });
-	const patchPayload = credentialPayload();
+	const savedCredential = await saveCredential(randomCredentialPayload(), { user: member });
+	const patchPayload = randomCredentialPayload();
 
 	const response = await authOwnerAgent
 		.patch(`/credentials/${savedCredential.id}`)
@@ -279,8 +279,8 @@ test('PATCH /credentials/:id should update non-owned cred for owner', async () =
 test('PATCH /credentials/:id should update owned cred for member', async () => {
 	const member = await testDb.createUser({ globalRole: globalMemberRole });
 	const authMemberAgent = utils.createAgent(app, { auth: true, user: member });
-	const savedCredential = await saveCredential(credentialPayload(), { user: member });
-	const patchPayload = credentialPayload();
+	const savedCredential = await saveCredential(randomCredentialPayload(), { user: member });
+	const patchPayload = randomCredentialPayload();
 
 	const response = await authMemberAgent
 		.patch(`/credentials/${savedCredential.id}`)
@@ -314,8 +314,8 @@ test('PATCH /credentials/:id should not update non-owned cred for member', async
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 	const member = await testDb.createUser({ globalRole: globalMemberRole });
 	const authMemberAgent = utils.createAgent(app, { auth: true, user: member });
-	const savedCredential = await saveCredential(credentialPayload(), { user: ownerShell });
-	const patchPayload = credentialPayload();
+	const savedCredential = await saveCredential(randomCredentialPayload(), { user: ownerShell });
+	const patchPayload = randomCredentialPayload();
 
 	const response = await authMemberAgent
 		.patch(`/credentials/${savedCredential.id}`)
@@ -331,7 +331,7 @@ test('PATCH /credentials/:id should not update non-owned cred for member', async
 test('PATCH /credentials/:id should fail with invalid inputs', async () => {
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
-	const savedCredential = await saveCredential(credentialPayload(), { user: ownerShell });
+	const savedCredential = await saveCredential(randomCredentialPayload(), { user: ownerShell });
 
 	await Promise.all(
 		INVALID_PAYLOADS.map(async (invalidPayload) => {
@@ -348,7 +348,7 @@ test('PATCH /credentials/:id should fail if cred not found', async () => {
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
 
-	const response = await authOwnerAgent.patch('/credentials/123').send(credentialPayload());
+	const response = await authOwnerAgent.patch('/credentials/123').send(randomCredentialPayload());
 
 	expect(response.statusCode).toBe(404);
 });
@@ -357,11 +357,10 @@ test('PATCH /credentials/:id should fail with missing encryption key', async ()
 	const mock = jest.spyOn(UserSettings, 'getEncryptionKey');
 	mock.mockRejectedValue(new Error(RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY));
 
-
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
 
-	const response = await authOwnerAgent.post('/credentials').send(credentialPayload());
+	const response = await authOwnerAgent.post('/credentials').send(randomCredentialPayload());
 
 	expect(response.statusCode).toBe(500);
 
@@ -373,12 +372,12 @@ test('GET /credentials should retrieve all creds for owner', async () => {
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
 
 	for (let i = 0; i < 3; i++) {
-		await saveCredential(credentialPayload(), { user: ownerShell });
+		await saveCredential(randomCredentialPayload(), { user: ownerShell });
 	}
 
 	const member = await testDb.createUser({ globalRole: globalMemberRole });
 
-	await saveCredential(credentialPayload(), { user: member });
+	await saveCredential(randomCredentialPayload(), { user: member });
 
 	const response = await authOwnerAgent.get('/credentials');
 
@@ -402,7 +401,7 @@ test('GET /credentials should retrieve owned creds for member', async () => {
 	const authMemberAgent = utils.createAgent(app, { auth: true, user: member });
 
 	for (let i = 0; i < 3; i++) {
-		await saveCredential(credentialPayload(), { user: member });
+		await saveCredential(randomCredentialPayload(), { user: member });
 	}
 
 	const response = await authMemberAgent.get('/credentials');
@@ -428,7 +427,7 @@ test('GET /credentials should not retrieve non-owned creds for member', async ()
 	const authMemberAgent = utils.createAgent(app, { auth: true, user: member });
 
 	for (let i = 0; i < 3; i++) {
-		await saveCredential(credentialPayload(), { user: ownerShell });
+		await saveCredential(randomCredentialPayload(), { user: ownerShell });
 	}
 
 	const response = await authMemberAgent.get('/credentials');
@@ -440,7 +439,7 @@ test('GET /credentials should not retrieve non-owned creds for member', async ()
 test('GET /credentials/:id should retrieve owned cred for owner', async () => {
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
-	const savedCredential = await saveCredential(credentialPayload(), { user: ownerShell });
+	const savedCredential = await saveCredential(randomCredentialPayload(), { user: ownerShell });
 
 	const firstResponse = await authOwnerAgent.get(`/credentials/${savedCredential.id}`);
 
@@ -465,7 +464,7 @@ test('GET /credentials/:id should retrieve owned cred for owner', async () => {
 test('GET /credentials/:id should retrieve owned cred for member', async () => {
 	const member = await testDb.createUser({ globalRole: globalMemberRole });
 	const authMemberAgent = utils.createAgent(app, { auth: true, user: member });
-	const savedCredential = await saveCredential(credentialPayload(), { user: member });
+	const savedCredential = await saveCredential(randomCredentialPayload(), { user: member });
 
 	const firstResponse = await authMemberAgent.get(`/credentials/${savedCredential.id}`);
 
@@ -492,7 +491,7 @@ test('GET /credentials/:id should not retrieve non-owned cred for member', async
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 	const member = await testDb.createUser({ globalRole: globalMemberRole });
 	const authMemberAgent = utils.createAgent(app, { auth: true, user: member });
-	const savedCredential = await saveCredential(credentialPayload(), { user: ownerShell });
+	const savedCredential = await saveCredential(randomCredentialPayload(), { user: ownerShell });
 
 	const response = await authMemberAgent.get(`/credentials/${savedCredential.id}`);
 
@@ -503,12 +502,11 @@ test('GET /credentials/:id should not retrieve non-owned cred for member', async
 test('GET /credentials/:id should fail with missing encryption key', async () => {
 	const ownerShell = await testDb.createUserShell(globalOwnerRole);
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: ownerShell });
-	const savedCredential = await saveCredential(credentialPayload(), { user: ownerShell });
+	const savedCredential = await saveCredential(randomCredentialPayload(), { user: ownerShell });
 
 	const mock = jest.spyOn(UserSettings, 'getEncryptionKey');
 	mock.mockRejectedValue(new Error(RESPONSE_ERROR_MESSAGES.NO_ENCRYPTION_KEY));
 
-
 	const response = await authOwnerAgent
 		.get(`/credentials/${savedCredential.id}`)
 		.query({ includeData: true });
@@ -527,13 +525,6 @@ test('GET /credentials/:id should return 404 if cred not found', async () => {
 	expect(response.statusCode).toBe(404);
 });
 
-const credentialPayload = () => ({
-	name: randomName(),
-	type: randomName(),
-	nodesAccess: [{ nodeType: randomName() }],
-	data: { accessToken: randomString(6, 16) },
-});
-
 const INVALID_PAYLOADS = [
 	{
 		type: randomName(),

packages/cli/test/integration/nodes.api.test.ts

@@ -265,7 +265,7 @@ test('DELETE /nodes should reject if package is not installed', async () => {
 	const {
 		statusCode,
 		body: { message },
-	} = await authAgent(ownerShell).delete('/nodes').send({
+	} = await authAgent(ownerShell).delete('/nodes').query({
 		name: utils.installedPackagePayload().packageName,
 	});
 
@@ -282,7 +282,7 @@ test('DELETE /nodes should uninstall package', async () => {
 
 	mocked(findInstalledPackage).mockImplementationOnce(mockedEmptyPackage);
 
-	const { statusCode } = await authAgent(ownerShell).delete('/nodes').send({
+	const { statusCode } = await authAgent(ownerShell).delete('/nodes').query({
 		name: utils.installedPackagePayload().packageName,
 	});
 

packages/cli/test/integration/shared/random.ts

@@ -45,3 +45,12 @@ const POPULAR_TOP_LEVEL_DOMAINS = ['com', 'org', 'net', 'io', 'edu'];
 const randomTopLevelDomain = () => chooseRandomly(POPULAR_TOP_LEVEL_DOMAINS);
 
 export const randomName = () => randomString(4, 8);
+
+export function randomCredentialPayload() {
+	return {
+		name: randomName(),
+		type: randomName(),
+		nodesAccess: [{ nodeType: randomName() }],
+		data: { accessToken: randomString(6, 16) },
+	};
+}