fix(core): User update endpoint should only allow updating email, fir…

…stName, and lastName

packages/cli/package.json

@@ -129,6 +129,7 @@
     "callsites": "^3.1.0",
     "change-case": "^4.1.1",
     "class-validator": "^0.14.0",
+    "class-transformer": "^0.5.1",
     "client-oauth2": "^4.2.5",
     "compression": "^1.7.4",
     "connect-history-api-fallback": "^1.6.0",

packages/cli/src/GenericHelpers.ts

@@ -22,6 +22,7 @@ import type { WorkflowEntity } from '@db/entities/WorkflowEntity';
 import type { CredentialsEntity } from '@db/entities/CredentialsEntity';
 import type { TagEntity } from '@db/entities/TagEntity';
 import type { User } from '@db/entities/User';
+import type { UserUpdatePayload } from '@/requests';
 
 /**
  * Returns the base URL n8n is reachable from
@@ -99,7 +100,7 @@ export async function generateUniqueName(
 }
 
 export async function validateEntity(
-	entity: WorkflowEntity | CredentialsEntity | TagEntity | User,
+	entity: WorkflowEntity | CredentialsEntity | TagEntity | User | UserUpdatePayload,
 ): Promise<void> {
 	const errors = await validate(entity);
 

packages/cli/src/controllers/me.controller.ts

@@ -1,4 +1,5 @@
 import validator from 'validator';
+import { plainToInstance } from 'class-transformer';
 import { Delete, Get, Patch, Post, RestController } from '@/decorators';
 import {
 	compareHash,
@@ -13,14 +14,15 @@ import { issueCookie } from '@/auth/jwt';
 import { Response } from 'express';
 import type { Repository } from 'typeorm';
 import type { ILogger } from 'n8n-workflow';
-import { AuthenticatedRequest, MeRequest } from '@/requests';
+import { AuthenticatedRequest, MeRequest, UserUpdatePayload } from '@/requests';
 import type {
 	PublicUser,
 	IDatabaseCollections,
 	IExternalHooksClass,
 	IInternalHooksClass,
 } from '@/Interfaces';
 import { randomBytes } from 'crypto';
+import pick from 'lodash.pick';
 
 @RestController('/me')
 export class MeController {
@@ -53,38 +55,44 @@ export class MeController {
 	 * Update the logged-in user's settings, except password.
 	 */
 	@Patch('/')
-	async updateCurrentUser(req: MeRequest.Settings, res: Response): Promise<PublicUser> {
-		const { email } = req.body;
+	async updateCurrentUser(req: MeRequest.UserUpdate, res: Response): Promise<PublicUser> {
+		const { id: userId, email: currentEmail } = req.user;
+
+		const payload = plainToInstance(
+			UserUpdatePayload,
+			pick(req.body, 'email', 'firstName', 'lastName'),
+		);
+
+		const { email } = payload;
 		if (!email) {
 			this.logger.debug('Request to update user email failed because of missing email in payload', {
-				userId: req.user.id,
-				payload: req.body,
+				userId,
+				payload,
 			});
 			throw new BadRequestError('Email is mandatory');
 		}
 
 		if (!validator.isEmail(email)) {
 			this.logger.debug('Request to update user email failed because of invalid email in payload', {
-				userId: req.user.id,
+				userId,
 				invalidEmail: email,
 			});
 			throw new BadRequestError('Invalid email address');
 		}
 
-		const { email: currentEmail } = req.user;
-		const newUser = new User();
-
-		Object.assign(newUser, req.user, req.body);
+		await validateEntity(payload);
 
-		await validateEntity(newUser);
-
-		const user = await this.userRepository.save(newUser);
+		await this.userRepository.update(userId, payload);
+		const user = await this.userRepository.findOneOrFail({
+			where: { id: userId },
+			relations: { globalRole: true },
+		});
 
-		this.logger.info('User updated successfully', { userId: user.id });
+		this.logger.info('User updated successfully', { userId });
 
 		await issueCookie(res, user);
 
-		const updatedKeys = Object.keys(req.body);
+		const updatedKeys = Object.keys(payload);
 		void this.internalHooks.onUserUpdate({
 			user,
 			fields_changed: updatedKeys,

packages/cli/src/databases/entities/User.ts

@@ -111,6 +111,9 @@ export class User extends AbstractEntity implements IUser {
 	@AfterLoad()
 	@AfterUpdate()
 	computeIsPending(): void {
-		this.isPending = this.password === null;
+		this.isPending =
+			this.globalRole?.name === 'owner' && this.globalRole.scope === 'global'
+				? false
+				: this.password === null;
 	}
 }

packages/cli/src/requests.ts

@@ -10,11 +10,28 @@ import type {
 	IWorkflowSettings,
 } from 'n8n-workflow';
 
+import { IsEmail, IsString, Length } from 'class-validator';
+import { NoXss } from '@db/utils/customValidators';
 import type { PublicUser, IExecutionDeleteFilter, IWorkflowDb } from '@/Interfaces';
 import type { Role } from '@db/entities/Role';
 import type { User } from '@db/entities/User';
 import type * as UserManagementMailer from '@/UserManagement/email/UserManagementMailer';
 
+export class UserUpdatePayload implements Pick<User, 'email' | 'firstName' | 'lastName'> {
+	@IsEmail()
+	email: string;
+
+	@NoXss()
+	@IsString({ message: 'First name must be of type string.' })
+	@Length(1, 32, { message: 'First name must be $constraint1 to $constraint2 characters long.' })
+	firstName: string;
+
+	@NoXss()
+	@IsString({ message: 'Last name must be of type string.' })
+	@Length(1, 32, { message: 'Last name must be $constraint1 to $constraint2 characters long.' })
+	lastName: string;
+}
+
 export type AuthlessRequest<
 	RouteParams = {},
 	ResponseBody = {},
@@ -144,11 +161,7 @@ export declare namespace ExecutionRequest {
 // ----------------------------------
 
 export declare namespace MeRequest {
-	export type Settings = AuthenticatedRequest<
-		{},
-		{},
-		Pick<PublicUser, 'email' | 'firstName' | 'lastName'>
-	>;
+	export type UserUpdate = AuthenticatedRequest<{}, {}, UserUpdatePayload>;
 	export type Password = AuthenticatedRequest<
 		{},
 		{},

packages/cli/test/unit/controllers/me.controller.test.ts

@@ -25,40 +25,74 @@ describe('MeController', () => {
 
 	describe('updateCurrentUser', () => {
 		it('should throw BadRequestError if email is missing in the payload', async () => {
-			const req = mock<MeRequest.Settings>({});
+			const req = mock<MeRequest.UserUpdate>({});
 			expect(controller.updateCurrentUser(req, mock())).rejects.toThrowError(
 				new BadRequestError('Email is mandatory'),
 			);
 		});
 
 		it('should throw BadRequestError if email is invalid', async () => {
-			const req = mock<MeRequest.Settings>({ body: { email: 'invalid-email' } });
+			const req = mock<MeRequest.UserUpdate>({ body: { email: 'invalid-email' } });
 			expect(controller.updateCurrentUser(req, mock())).rejects.toThrowError(
 				new BadRequestError('Invalid email address'),
 			);
 		});
 
 		it('should update the user in the DB, and issue a new cookie', async () => {
-			const req = mock<MeRequest.Settings>({
-				user: mock({ id: '123', password: 'password', authIdentities: [] }),
-				body: { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' },
+			const user = mock<User>({
+				id: '123',
+				password: 'password',
+				authIdentities: [],
+				globalRoleId: '1',
 			});
+			const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' };
+			const req = mock<MeRequest.UserUpdate>({ user, body: reqBody });
 			const res = mock<Response>();
-			userRepository.save.calledWith(anyObject()).mockResolvedValue(req.user);
+			userRepository.findOneByOrFail.mockResolvedValue(user);
 			jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token');
 
 			await controller.updateCurrentUser(req, res);
 
+			expect(userRepository.update).toHaveBeenCalled();
+
 			const cookieOptions = captor<CookieOptions>();
 			expect(res.cookie).toHaveBeenCalledWith(AUTH_COOKIE_NAME, 'signed-token', cookieOptions);
 			expect(cookieOptions.value.httpOnly).toBe(true);
 			expect(cookieOptions.value.sameSite).toBe('lax');
 
 			expect(externalHooks.run).toHaveBeenCalledWith('user.profile.update', [
-				req.user.email,
+				user.email,
 				anyObject(),
 			]);
 		});
+
+		it('should not allow updating any other fields on a user besides email and name', async () => {
+			const user = mock<User>({
+				id: '123',
+				password: 'password',
+				authIdentities: [],
+				globalRoleId: '1',
+			});
+			const reqBody = { email: 'valid@email.com', firstName: 'John', lastName: 'Potato' };
+			const req = mock<MeRequest.UserUpdate>({ user, body: reqBody });
+			const res = mock<Response>();
+			userRepository.findOneByOrFail.mockResolvedValue(user);
+			jest.spyOn(jwt, 'sign').mockImplementation(() => 'signed-token');
+
+			// Add invalid data to the request payload
+			Object.assign(reqBody, { id: '0', globalRoleId: '42' });
+
+			await controller.updateCurrentUser(req, res);
+
+			expect(userRepository.update).toHaveBeenCalled();
+
+			const updatedUser = userRepository.update.mock.calls[0][1];
+			expect(updatedUser.email).toBe(reqBody.email);
+			expect(updatedUser.firstName).toBe(reqBody.firstName);
+			expect(updatedUser.lastName).toBe(reqBody.lastName);
+			expect(updatedUser.id).not.toBe('0');
+			expect(updatedUser.globalRoleId).not.toBe('42');
+		});
 	});
 
 	describe('updatePassword', () => {