refactor(core): Make browserId check mandatory

n8n-io · Jul 19, 2024 · e12ab2a · e12ab2a
1 parent 0542765
commit e12ab2a
Show file tree

Hide file tree

Showing 5 changed files with 6 additions and 40 deletions.
diff --git a/packages/cli/src/auth/auth.service.ts b/packages/cli/src/auth/auth.service.ts
@@ -21,7 +21,7 @@ interface AuthJwtPayload {
 	/** This hash is derived from email and bcrypt of password */
 	hash: string;
 	/** This is a client generated unique string to prevent session hijacking */
-	browserId?: string;
+	browserId: string;
 }
 
 interface IssuedJWT extends AuthJwtPayload {
@@ -83,7 +83,7 @@ export class AuthService {
 		res.clearCookie(AUTH_COOKIE_NAME);
 	}
 
-	issueCookie(res: Response, user: User, browserId?: string) {
+	issueCookie(res: Response, user: User, browserId: string) {
 		// TODO: move this check to the login endpoint in AuthController
 		// If the instance has exceeded its user quota, prevent non-owners from logging in
 		const isWithinUsersLimit = this.license.isWithinUsersLimit();
@@ -104,11 +104,11 @@ export class AuthService {
 		});
 	}
 
-	issueJWT(user: User, browserId?: string) {
+	issueJWT(user: User, browserId: string) {
 		const payload: AuthJwtPayload = {
 			id: user.id,
 			hash: this.createJWTHash(user),
-			browserId: browserId && this.hash(browserId),
+			browserId: this.hash(browserId),
 		};
 		return this.jwtService.sign(payload, {
 			expiresIn: this.jwtExpiration,
@@ -140,10 +140,7 @@ export class AuthService {
 		const endpoint = req.route ? `${req.baseUrl}${req.route.path}` : req.baseUrl;
 		if (req.method === 'GET' && skipBrowserIdCheckEndpoints.includes(endpoint)) {
 			this.logger.debug(`Skipped browserId check on ${endpoint}`);
-		} else if (
-			jwtPayload.browserId &&
-			(!req.browserId || jwtPayload.browserId !== this.hash(req.browserId))
-		) {
+		} else if (!jwtPayload.browserId || !req.browserId || jwtPayload.browserId !== this.hash(req.browserId)) {
 			this.logger.warn(`browserId check failed on ${endpoint}`);
 			throw new AuthError('Unauthorized');
 		}

diff --git a/packages/cli/src/auth/jwt.ts b/packages/cli/src/auth/jwt.ts
diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts
@@ -68,7 +68,7 @@ export type APIRequest<
 	RequestBody = {},
 	RequestQuery = {},
 > = express.Request<RouteParams, ResponseBody, RequestBody, RequestQuery> & {
-	browserId?: string;
+	browserId: string;
 };
 
 export type AuthlessRequest<

diff --git a/packages/cli/src/services/hooks.service.ts b/packages/cli/src/services/hooks.service.ts
@@ -43,14 +43,6 @@ export class HooksService {
 		return await this.userService.inviteUsers(owner, attributes);
 	}
 
-	/**
-	 * Set the n8n-auth cookie in the response to auto-login
-	 * the user after instance is provisioned
-	 */
-	issueCookie(res: Response, user: AuthUser) {
-		return this.authService.issueCookie(res, user);
-	}
-
 	/**
 	 * Find user in the instance
 	 * 1. To know whether the instance owner is already setup

diff --git a/packages/cli/test/unit/services/hooks.service.test.ts b/packages/cli/test/unit/services/hooks.service.test.ts
@@ -50,17 +50,6 @@ describe('HooksService', () => {
 		expect(userService.inviteUsers).toHaveBeenCalledWith(mockedUser, usersToInvite);
 	});
 
-	it('hooksService.issueCookie should call authService.issueCookie', async () => {
-		// ARRANGE
-		const res = mock<Response>();
-
-		// ACT
-		hooksService.issueCookie(res, mockedUser);
-
-		// ASSERT
-		expect(authService.issueCookie).toHaveBeenCalledWith(res, mockedUser);
-	});
-
 	it('hooksService.findOneUser should call authUserRepository.findOne', async () => {
 		// ARRANGE
 		const filter = { where: { id: '1' } };