fix(core): Fix possible corruption of OAuth2 credential (#12880)

packages/@n8n/client-oauth2/.eslintrc.js

@@ -11,5 +11,6 @@ module.exports = {
 	rules: {
 		'@typescript-eslint/consistent-type-imports': 'error',
 		'n8n-local-rules/no-plain-errors': 'off',
+		'n8n-local-rules/no-uncaught-json-parse': 'off',
 	},
 };

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

@@ -1,8 +1,6 @@
-/* eslint-disable @typescript-eslint/no-unsafe-return */
-/* eslint-disable @typescript-eslint/no-unsafe-argument */
 /* eslint-disable @typescript-eslint/no-explicit-any */
 import axios from 'axios';
-import type { AxiosRequestConfig } from 'axios';
+import type { AxiosRequestConfig, AxiosResponse } from 'axios';
 import { Agent } from 'https';
 import * as qs from 'querystring';
 
@@ -10,7 +8,7 @@ import type { ClientOAuth2TokenData } from './ClientOAuth2Token';
 import { ClientOAuth2Token } from './ClientOAuth2Token';
 import { CodeFlow } from './CodeFlow';
 import { CredentialsFlow } from './CredentialsFlow';
-import type { Headers } from './types';
+import type { Headers, OAuth2AccessTokenErrorResponse } from './types';
 import { getAuthError } from './utils';
 
 export interface ClientOAuth2RequestObject {
@@ -38,10 +36,10 @@ export interface ClientOAuth2Options {
 	ignoreSSLIssues?: boolean;
 }
 
-class ResponseError extends Error {
+export class ResponseError extends Error {
 	constructor(
 		readonly status: number,
-		readonly body: object,
+		readonly body: unknown,
 		readonly code = 'ESTATUS',
 	) {
 		super(`HTTP status ${status}`);
@@ -74,21 +72,12 @@ export class ClientOAuth2 {
 	}
 
 	/**
-	 * Attempt to parse response body as JSON, fall back to parsing as a query string.
+	 * Request an access token from the OAuth2 server.
+	 *
+	 * @throws {ResponseError} If the response is an unexpected status code.
+	 * @throws {AuthError} If the response is an authentication error.
 	 */
-	private parseResponseBody<T extends object>(body: string): T {
-		try {
-			return JSON.parse(body);
-		} catch (e) {
-			return qs.parse(body) as T;
-		}
-	}
-
-	/**
-	 * Using the built-in request method, we'll automatically attempt to parse
-	 * the response.
-	 */
-	async request<T extends object>(options: ClientOAuth2RequestObject): Promise<T> {
+	async accessTokenRequest(options: ClientOAuth2RequestObject): Promise<ClientOAuth2TokenData> {
 		let url = options.url;
 		const query = qs.stringify(options.query);
 
@@ -101,7 +90,7 @@ export class ClientOAuth2 {
 			method: options.method,
 			data: qs.stringify(options.body),
 			headers: options.headers,
-			transformResponse: (res) => res,
+			transformResponse: (res: unknown) => res,
 			// Axios rejects the promise by default for all status codes 4xx.
 			// We override this to reject promises only on 5xxs
 			validateStatus: (status) => status < 500,
@@ -113,16 +102,36 @@ export class ClientOAuth2 {
 
 		const response = await axios.request(requestConfig);
 
-		const body = this.parseResponseBody<T>(response.data);
-
-		// eslint-disable-next-line @typescript-eslint/ban-ts-comment
-		// @ts-ignore
+		if (response.status >= 400) {
+			const body = this.parseResponseBody<OAuth2AccessTokenErrorResponse>(response);
 			const authErr = getAuthError(body);
+
 			if (authErr) throw authErr;
+			else throw new ResponseError(response.status, response.data);
+		}
 
-		if (response.status < 200 || response.status >= 399)
+		if (response.status >= 300) {
 			throw new ResponseError(response.status, response.data);
+		}
 
-		return body;
+		return this.parseResponseBody<ClientOAuth2TokenData>(response);
+	}
+
+	/**
+	 * Attempt to parse response body based on the content type.
+	 */
+	private parseResponseBody<T extends object>(response: AxiosResponse<unknown>): T {
+		const contentType = (response.headers['content-type'] as string) ?? '';
+		const body = response.data as string;
+
+		if (contentType.startsWith('application/json')) {
+			return JSON.parse(body) as T;
+		}
+
+		if (contentType.startsWith('application/x-www-form-urlencoded')) {
+			return qs.parse(body) as T;
+		}
+
+		throw new Error(`Unsupported content type: ${contentType}`);
 	}
 }

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

@@ -1,3 +1,5 @@
+import * as a from 'node:assert';
+
 import type { ClientOAuth2, ClientOAuth2Options, ClientOAuth2RequestObject } from './ClientOAuth2';
 import { DEFAULT_HEADERS } from './constants';
 import { auth, expects, getRequestOptions } from './utils';
@@ -65,17 +67,16 @@ export class ClientOAuth2Token {
 	}
 
 	/**
-	 * Refresh a user access token with the supplied token.
+	 * Refresh a user access token with the refresh token.
+	 * As in RFC 6749 Section 6: https://www.rfc-editor.org/rfc/rfc6749.html#section-6
 	 */
 	async refresh(opts?: ClientOAuth2Options): Promise<ClientOAuth2Token> {
 		const options = { ...this.client.options, ...opts };
 
 		expects(options, 'clientSecret');
+		a.ok(this.refreshToken, 'refreshToken is required');
 
-		if (!this.refreshToken) throw new Error('No refresh token');
-
-		const clientId = options.clientId;
-		const clientSecret = options.clientSecret;
+		const { clientId, clientSecret } = options;
 		const headers = { ...DEFAULT_HEADERS };
 		const body: Record<string, string> = {
 			refresh_token: this.refreshToken,
@@ -99,7 +100,7 @@ export class ClientOAuth2Token {
 			options,
 		);
 
-		const responseData = await this.client.request<ClientOAuth2TokenData>(requestOptions);
+		const responseData = await this.client.accessTokenRequest(requestOptions);
 		return this.client.createToken({ ...this.data, ...responseData });
 	}
 

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

@@ -1,7 +1,7 @@
 import * as qs from 'querystring';
 
 import type { ClientOAuth2, ClientOAuth2Options } from './ClientOAuth2';
-import type { ClientOAuth2Token, ClientOAuth2TokenData } from './ClientOAuth2Token';
+import type { ClientOAuth2Token } from './ClientOAuth2Token';
 import { DEFAULT_HEADERS, DEFAULT_URL_BASE } from './constants';
 import { auth, expects, getAuthError, getRequestOptions } from './utils';
 
@@ -117,7 +117,7 @@ export class CodeFlow {
 			options,
 		);
 
-		const responseData = await this.client.request<ClientOAuth2TokenData>(requestOptions);
+		const responseData = await this.client.accessTokenRequest(requestOptions);
 		return this.client.createToken(responseData);
 	}
 }

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

@@ -1,5 +1,5 @@
 import type { ClientOAuth2 } from './ClientOAuth2';
-import type { ClientOAuth2Token, ClientOAuth2TokenData } from './ClientOAuth2Token';
+import type { ClientOAuth2Token } from './ClientOAuth2Token';
 import { DEFAULT_HEADERS } from './constants';
 import type { Headers } from './types';
 import { auth, expects, getRequestOptions } from './utils';
@@ -55,7 +55,7 @@ export class CredentialsFlow {
 			options,
 		);
 
-		const responseData = await this.client.request<ClientOAuth2TokenData>(requestOptions);
+		const responseData = await this.client.accessTokenRequest(requestOptions);
 		return this.client.createToken(responseData);
 	}
 }

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

@@ -17,3 +17,14 @@ export interface OAuth2CredentialData {
 		refresh_token?: string;
 	};
 }
+
+/**
+ * The response from the OAuth2 server when the access token is not successfully
+ * retrieved. As specified in RFC 6749 Section 5.2:
+ * https://www.rfc-editor.org/rfc/rfc6749.html#section-5.2
+ */
+export interface OAuth2AccessTokenErrorResponse extends Record<string, unknown> {
+	error: string;
+	error_description?: string;
+	error_uri?: string;
+}

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

@@ -0,0 +1,168 @@
+import axios from 'axios';
+import nock from 'nock';
+
+import { ClientOAuth2, ResponseError } from '@/ClientOAuth2';
+import { ERROR_RESPONSES } from '@/constants';
+import { auth, AuthError } from '@/utils';
+
+import * as config from './config';
+
+describe('ClientOAuth2', () => {
+	const client = new ClientOAuth2({
+		clientId: config.clientId,
+		clientSecret: config.clientSecret,
+		accessTokenUri: config.accessTokenUri,
+		authentication: 'header',
+	});
+
+	beforeAll(async () => {
+		nock.disableNetConnect();
+	});
+
+	afterAll(() => {
+		nock.restore();
+	});
+
+	describe('accessTokenRequest', () => {
+		const authHeader = auth(config.clientId, config.clientSecret);
+
+		const makeTokenCall = async () =>
+			await client.accessTokenRequest({
+				url: config.accessTokenUri,
+				method: 'POST',
+				headers: {
+					Authorization: authHeader,
+					Accept: 'application/json',
+					'Content-Type': 'application/x-www-form-urlencoded',
+				},
+				body: {
+					refresh_token: 'test',
+					grant_type: 'refresh_token',
+				},
+			});
+
+		const mockTokenResponse = ({
+			status = 200,
+			headers,
+			body,
+		}: {
+			status: number;
+			body: string;
+			headers: Record<string, string>;
+		}) =>
+			nock(config.baseUrl).post('/login/oauth/access_token').once().reply(status, body, headers);
+
+		it('should send the correct request based on given options', async () => {
+			mockTokenResponse({
+				status: 200,
+				headers: { 'Content-Type': 'application/json' },
+				body: JSON.stringify({
+					access_token: config.accessToken,
+					refresh_token: config.refreshToken,
+				}),
+			});
+
+			const axiosSpy = jest.spyOn(axios, 'request');
+
+			await makeTokenCall();
+
+			expect(axiosSpy).toHaveBeenCalledWith(
+				expect.objectContaining({
+					url: config.accessTokenUri,
+					method: 'POST',
+					data: 'refresh_token=test&grant_type=refresh_token',
+					headers: {
+						Authorization: authHeader,
+						Accept: 'application/json',
+						'Content-Type': 'application/x-www-form-urlencoded',
+					},
+				}),
+			);
+		});
+
+		test.each([
+			{
+				contentType: 'application/json',
+				body: JSON.stringify({
+					access_token: config.accessToken,
+					refresh_token: config.refreshToken,
+				}),
+			},
+			{
+				contentType: 'application/json; charset=utf-8',
+				body: JSON.stringify({
+					access_token: config.accessToken,
+					refresh_token: config.refreshToken,
+				}),
+			},
+			{
+				contentType: 'application/x-www-form-urlencoded',
+				body: `access_token=${config.accessToken}&refresh_token=${config.refreshToken}`,
+			},
+		])('should parse response with content type $contentType', async ({ contentType, body }) => {
+			mockTokenResponse({
+				status: 200,
+				headers: { 'Content-Type': contentType },
+				body,
+			});
+
+			const response = await makeTokenCall();
+
+			expect(response).toEqual({
+				access_token: config.accessToken,
+				refresh_token: config.refreshToken,
+			});
+		});
+
+		test.each([
+			{
+				contentType: 'text/html',
+				body: '<html><body>Hello, world!</body></html>',
+			},
+			{
+				contentType: 'application/xml',
+				body: '<xml><body>Hello, world!</body></xml>',
+			},
+			{
+				contentType: 'text/plain',
+				body: 'Hello, world!',
+			},
+		])('should reject content type $contentType', async ({ contentType, body }) => {
+			mockTokenResponse({
+				status: 200,
+				headers: { 'Content-Type': contentType },
+				body,
+			});
+
+			const result = await makeTokenCall().catch((err) => err);
+			expect(result).toBeInstanceOf(Error);
+			expect(result.message).toEqual(`Unsupported content type: ${contentType}`);
+		});
+
+		it('should reject 4xx responses with auth errors', async () => {
+			mockTokenResponse({
+				status: 401,
+				headers: { 'Content-Type': 'application/json' },
+				body: JSON.stringify({ error: 'access_denied' }),
+			});
+
+			const result = await makeTokenCall().catch((err) => err);
+			expect(result).toBeInstanceOf(AuthError);
+			expect(result.message).toEqual(ERROR_RESPONSES.access_denied);
+			expect(result.body).toEqual({ error: 'access_denied' });
+		});
+
+		it('should reject 3xx responses with response errors', async () => {
+			mockTokenResponse({
+				status: 302,
+				headers: {},
+				body: 'Redirected',
+			});
+
+			const result = await makeTokenCall().catch((err) => err);
+			expect(result).toBeInstanceOf(ResponseError);
+			expect(result.message).toEqual('HTTP status 302');
+			expect(result.body).toEqual('Redirected');
+		});
+	});
+});