fix(core): Use correct scopes-separator when generating authorization urls (#6502)

packages/@n8n/client-oauth2/src/ClientOAuth2.ts

@@ -10,28 +10,28 @@ import type { ClientOAuth2TokenData } from './ClientOAuth2Token';
 import { ClientOAuth2Token } from './ClientOAuth2Token';
 import { CodeFlow } from './CodeFlow';
 import { CredentialsFlow } from './CredentialsFlow';
-import type { Headers, Query } from './types';
+import type { Headers } from './types';
 
 export interface ClientOAuth2RequestObject {
 	url: string;
 	method: 'DELETE' | 'GET' | 'HEAD' | 'PATCH' | 'POST' | 'PUT';
 	body?: Record<string, any>;
-	query?: Query;
+	query?: qs.ParsedUrlQuery;
 	headers?: Headers;
 }
 
 export interface ClientOAuth2Options {
 	clientId: string;
-	clientSecret: string;
+	clientSecret?: string;
 	accessTokenUri: string;
 	authorizationUri?: string;
 	redirectUri?: string;
 	scopes?: string[];
+	scopesSeparator?: ',' | ' ';
 	authorizationGrants?: string[];
 	state?: string;
 	body?: Record<string, any>;
-	query?: Query;
-	headers?: Headers;
+	query?: qs.ParsedUrlQuery;
 }
 
 class ResponseError extends Error {

packages/@n8n/client-oauth2/src/ClientOAuth2Token.ts

@@ -2,7 +2,7 @@
 /* eslint-disable @typescript-eslint/no-unsafe-assignment */
 /* eslint-disable @typescript-eslint/naming-convention */
 import type { ClientOAuth2, ClientOAuth2Options, ClientOAuth2RequestObject } from './ClientOAuth2';
-import { auth, getRequestOptions } from './utils';
+import { auth, expects, getRequestOptions } from './utils';
 import { DEFAULT_HEADERS } from './constants';
 
 export interface ClientOAuth2TokenData extends Record<string, string | undefined> {
@@ -69,6 +69,8 @@ export class ClientOAuth2Token {
 	async refresh(opts?: ClientOAuth2Options): Promise<ClientOAuth2Token> {
 		const options = { ...this.client.options, ...opts };
 
+		expects(options, 'clientSecret');
+
 		if (!this.refreshToken) throw new Error('No refresh token');
 
 		const requestOptions = getRequestOptions(

packages/@n8n/client-oauth2/src/CodeFlow.ts

@@ -2,7 +2,7 @@ import * as qs from 'querystring';
 import type { ClientOAuth2, ClientOAuth2Options } from './ClientOAuth2';
 import type { ClientOAuth2Token, ClientOAuth2TokenData } from './ClientOAuth2Token';
 import { DEFAULT_HEADERS, DEFAULT_URL_BASE } from './constants';
-import { auth, expects, getAuthError, getRequestOptions, sanitizeScope } from './utils';
+import { auth, expects, getAuthError, getRequestOptions } from './utils';
 
 interface CodeFlowBody {
 	code: string | string[];
@@ -22,27 +22,30 @@ export class CodeFlow {
 	/**
 	 * Generate the uri for doing the first redirect.
 	 */
-	getUri(opts?: ClientOAuth2Options): string {
-		const options = { ...this.client.options, ...opts };
+	getUri(opts?: Partial<ClientOAuth2Options>): string {
+		const options: ClientOAuth2Options = { ...this.client.options, ...opts };
 
 		// Check the required parameters are set.
 		expects(options, 'clientId', 'authorizationUri');
 
-		const query: Record<string, string | undefined> = {
+		const url = new URL(options.authorizationUri);
+
+		const queryParams = {
+			...options.query,
 			client_id: options.clientId,
 			redirect_uri: options.redirectUri,
 			response_type: 'code',
 			state: options.state,
+			...(options.scopes ? { scope: options.scopes.join(options.scopesSeparator ?? ' ') } : {}),
 		};
-		if (options.scopes !== undefined) {
-			query.scope = sanitizeScope(options.scopes);
+
+		for (const [key, value] of Object.entries(queryParams)) {
+			if (value !== null && value !== undefined) {
+				url.searchParams.append(key, value);
+			}
 		}
 
-		if (options.authorizationUri) {
-			const sep = options.authorizationUri.includes('?') ? '&' : '?';
-			return options.authorizationUri + sep + qs.stringify({ ...query, ...options.query });
-		}
-		throw new TypeError('Missing authorization uri, unable to get redirect uri');
+		return url.toString();
 	}
 
 	/**

packages/@n8n/client-oauth2/src/CredentialsFlow.ts

@@ -1,7 +1,7 @@
 import type { ClientOAuth2, ClientOAuth2Options } from './ClientOAuth2';
 import type { ClientOAuth2Token, ClientOAuth2TokenData } from './ClientOAuth2Token';
 import { DEFAULT_HEADERS } from './constants';
-import { auth, expects, getRequestOptions, sanitizeScope } from './utils';
+import { auth, expects, getRequestOptions } from './utils';
 
 interface CredentialsFlowBody {
 	grant_type: 'client_credentials';
@@ -29,7 +29,7 @@ export class CredentialsFlow {
 		};
 
 		if (options.scopes !== undefined) {
-			body.scope = sanitizeScope(options.scopes);
+			body.scope = options.scopes.join(options.scopesSeparator ?? ' ');
 		}
 
 		const requestOptions = getRequestOptions(

packages/@n8n/client-oauth2/src/types.ts

@@ -1,2 +1 @@
 export type Headers = Record<string, string | string[]>;
-export type Query = Record<string, string | string[]>;

packages/@n8n/client-oauth2/src/utils.ts

@@ -2,17 +2,21 @@
 /* eslint-disable @typescript-eslint/restrict-plus-operands */
 /* eslint-disable @typescript-eslint/no-unsafe-assignment */
 /* eslint-disable @typescript-eslint/no-explicit-any */
-import type { ClientOAuth2RequestObject } from './ClientOAuth2';
+import type { ClientOAuth2Options, ClientOAuth2RequestObject } from './ClientOAuth2';
 import { ERROR_RESPONSES } from './constants';
 
 /**
  * Check if properties exist on an object and throw when they aren't.
  */
-export function expects(obj: any, ...args: any[]) {
-	for (let i = 1; i < args.length; i++) {
-		const prop = args[i];
-		if (obj[prop] === null) {
-			throw new TypeError('Expected "' + prop + '" to exist');
+export function expects<Keys extends keyof ClientOAuth2Options>(
+	obj: ClientOAuth2Options,
+	...keys: Keys[]
+): asserts obj is ClientOAuth2Options & {
+	[K in Keys]: NonNullable<ClientOAuth2Options[K]>;
+} {
+	for (const key of keys) {
+		if (obj[key] === null || obj[key] === undefined) {
+			throw new TypeError('Expected "' + key + '" to exist');
 		}
 	}
 }
@@ -47,13 +51,6 @@ function toString(str: string | null | undefined) {
 	return str === null ? '' : String(str);
 }
 
-/**
- * Sanitize the scopes option to be a string.
- */
-export function sanitizeScope(scopes: string[] | string): string {
-	return Array.isArray(scopes) ? scopes.join(' ') : toString(scopes);
-}
-
 /**
  * Create basic auth header.
  */

packages/@n8n/client-oauth2/test/CodeFlow.test.ts

@@ -29,7 +29,7 @@ describe('CodeFlow', () => {
 			expect(githubAuth.code.getUri()).toEqual(
 				`${config.authorizationUri}?client_id=abc&` +
 					`redirect_uri=${encodeURIComponent(config.redirectUri)}&` +
-					'response_type=code&state=&scope=notifications',
+					'response_type=code&scope=notifications',
 			);
 		});
 
@@ -46,7 +46,7 @@ describe('CodeFlow', () => {
 				expect(authWithoutScopes.code.getUri()).toEqual(
 					`${config.authorizationUri}?client_id=abc&` +
 						`redirect_uri=${encodeURIComponent(config.redirectUri)}&` +
-						'response_type=code&state=',
+						'response_type=code',
 				);
 			});
 		});
@@ -64,7 +64,7 @@ describe('CodeFlow', () => {
 			expect(authWithEmptyScopes.code.getUri()).toEqual(
 				`${config.authorizationUri}?client_id=abc&` +
 					`redirect_uri=${encodeURIComponent(config.redirectUri)}&` +
-					'response_type=code&state=&scope=',
+					'response_type=code&scope=',
 			);
 		});
 
@@ -81,7 +81,7 @@ describe('CodeFlow', () => {
 			expect(authWithEmptyScopes.code.getUri()).toEqual(
 				`${config.authorizationUri}?client_id=abc&` +
 					`redirect_uri=${encodeURIComponent(config.redirectUri)}&` +
-					'response_type=code&state=&scope=',
+					'response_type=code&scope=',
 			);
 		});
 
@@ -99,7 +99,7 @@ describe('CodeFlow', () => {
 				expect(authWithParams.code.getUri()).toEqual(
 					`${config.authorizationUri}?bar=qux&client_id=abc&` +
 						`redirect_uri=${encodeURIComponent(config.redirectUri)}&` +
-						'response_type=code&state=&scope=notifications',
+						'response_type=code&scope=notifications',
 				);
 			});
 		});

packages/cli/src/credentials/oauth2Credential.api.ts

@@ -3,6 +3,7 @@ import { ClientOAuth2 } from '@n8n/client-oauth2';
 import Csrf from 'csrf';
 import express from 'express';
 import pkceChallenge from 'pkce-challenge';
+import * as qs from 'querystring';
 import get from 'lodash/get';
 import omit from 'lodash/omit';
 import set from 'lodash/set';
@@ -121,15 +122,21 @@ oauth2CredentialController.get(
 		};
 		const stateEncodedStr = Buffer.from(JSON.stringify(state)).toString('base64');
 
+		const scopes = get(oauthCredentials, 'scope', 'openid') as string;
 		const oAuthOptions: ClientOAuth2Options = {
 			clientId: get(oauthCredentials, 'clientId') as string,
 			clientSecret: get(oauthCredentials, 'clientSecret', '') as string,
 			accessTokenUri: get(oauthCredentials, 'accessTokenUrl', '') as string,
 			authorizationUri: get(oauthCredentials, 'authUrl', '') as string,
 			redirectUri: `${getInstanceBaseUrl()}/${restEndpoint}/oauth2-credential/callback`,
-			scopes: split(get(oauthCredentials, 'scope', 'openid,') as string, ','),
+			scopes: split(scopes, ','),
+			scopesSeparator: scopes.includes(',') ? ',' : ' ',
 			state: stateEncodedStr,
 		};
+		const authQueryParameters = get(oauthCredentials, 'authQueryParameters', '') as string;
+		if (authQueryParameters) {
+			oAuthOptions.query = qs.parse(authQueryParameters);
+		}
 
 		await Container.get(ExternalHooks).run('oauth2.authenticate', [oAuthOptions]);
 
@@ -162,26 +169,12 @@ oauth2CredentialController.get(
 		// Update the credentials in DB
 		await Db.collections.Credentials.update(req.query.id, newCredentialsData);
 
-		const authQueryParameters = get(oauthCredentials, 'authQueryParameters', '') as string;
-		let returnUri = oAuthObj.code.getUri();
-
-		// if scope uses comma, change it as the library always return then with spaces
-		if ((get(oauthCredentials, 'scope') as string).includes(',')) {
-			const data = returnUri.split('?')[1];
-			const scope = get(oauthCredentials, 'scope') as string;
-			const percentEncoded = [data, `scope=${encodeURIComponent(scope)}`].join('&');
-			returnUri = `${get(oauthCredentials, 'authUrl', '') as string}?${percentEncoded}`;
-		}
-
-		if (authQueryParameters) {
-			returnUri += `&${authQueryParameters}`;
-		}
-
-		LoggerProxy.verbose('OAuth2 authentication successful for new credential', {
+		LoggerProxy.verbose('OAuth2 authorization url created for credential', {
 			userId: req.user.id,
 			credentialId,
 		});
-		return returnUri;
+
+		return oAuthObj.code.getUri();
 	}),
 );
 
@@ -263,13 +256,15 @@ oauth2CredentialController.get(
 
 			let options: Partial<ClientOAuth2Options> = {};
 
+			const scopes = get(oauthCredentials, 'scope', 'openid') as string;
 			const oAuth2Parameters: ClientOAuth2Options = {
 				clientId: get(oauthCredentials, 'clientId') as string,
 				clientSecret: get(oauthCredentials, 'clientSecret', '') as string,
 				accessTokenUri: get(oauthCredentials, 'accessTokenUrl', '') as string,
 				authorizationUri: get(oauthCredentials, 'authUrl', '') as string,
 				redirectUri: `${getInstanceBaseUrl()}/${restEndpoint}/oauth2-credential/callback`,
-				scopes: split(get(oauthCredentials, 'scope', 'openid,') as string, ','),
+				scopes: split(scopes, ','),
+				scopesSeparator: scopes.includes(',') ? ',' : ' ',
 			};
 
 			if (oauthCredentials.grantType === 'pkce') {