From ac84ea14452cbcec95f14073e8e70427169e6a7f Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Tue, 28 Jan 2025 18:34:06 +0200 Subject: [PATCH] fix(core): Fix possible corruption of OAuth2 credential (#12880) --- packages/@n8n/client-oauth2/.eslintrc.js | 1 + .../@n8n/client-oauth2/src/ClientOAuth2.ts | 65 ++++--- .../client-oauth2/src/ClientOAuth2Token.ts | 13 +- packages/@n8n/client-oauth2/src/CodeFlow.ts | 4 +- .../@n8n/client-oauth2/src/CredentialsFlow.ts | 4 +- packages/@n8n/client-oauth2/src/types.ts | 11 ++ .../client-oauth2/test/ClientOAuth2.test.ts | 168 ++++++++++++++++++ 7 files changed, 228 insertions(+), 38 deletions(-) create mode 100644 packages/@n8n/client-oauth2/test/ClientOAuth2.test.ts diff --git a/packages/@n8n/client-oauth2/.eslintrc.js b/packages/@n8n/client-oauth2/.eslintrc.js index c3fe283453..be8ebd21d1 100644 --- a/packages/@n8n/client-oauth2/.eslintrc.js +++ b/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', }, }; diff --git a/packages/@n8n/client-oauth2/src/ClientOAuth2.ts b/packages/@n8n/client-oauth2/src/ClientOAuth2.ts index 676249254a..62e0241e6f 100644 --- a/packages/@n8n/client-oauth2/src/ClientOAuth2.ts +++ b/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(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(options: ClientOAuth2RequestObject): Promise { + async accessTokenRequest(options: ClientOAuth2RequestObject): Promise { 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(response.data); + if (response.status >= 400) { + const body = this.parseResponseBody(response); + const authErr = getAuthError(body); - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const authErr = getAuthError(body); - if (authErr) throw authErr; + 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(response); + } + + /** + * Attempt to parse response body based on the content type. + */ + private parseResponseBody(response: AxiosResponse): 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}`); } } diff --git a/packages/@n8n/client-oauth2/src/ClientOAuth2Token.ts b/packages/@n8n/client-oauth2/src/ClientOAuth2Token.ts index 2bcacdf112..d7a80a1699 100644 --- a/packages/@n8n/client-oauth2/src/ClientOAuth2Token.ts +++ b/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 { 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 = { refresh_token: this.refreshToken, @@ -99,7 +100,7 @@ export class ClientOAuth2Token { options, ); - const responseData = await this.client.request(requestOptions); + const responseData = await this.client.accessTokenRequest(requestOptions); return this.client.createToken({ ...this.data, ...responseData }); } diff --git a/packages/@n8n/client-oauth2/src/CodeFlow.ts b/packages/@n8n/client-oauth2/src/CodeFlow.ts index 6d0fff235e..6db98929f9 100644 --- a/packages/@n8n/client-oauth2/src/CodeFlow.ts +++ b/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(requestOptions); + const responseData = await this.client.accessTokenRequest(requestOptions); return this.client.createToken(responseData); } } diff --git a/packages/@n8n/client-oauth2/src/CredentialsFlow.ts b/packages/@n8n/client-oauth2/src/CredentialsFlow.ts index f1ccc256e7..eeb5550cf3 100644 --- a/packages/@n8n/client-oauth2/src/CredentialsFlow.ts +++ b/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(requestOptions); + const responseData = await this.client.accessTokenRequest(requestOptions); return this.client.createToken(responseData); } } diff --git a/packages/@n8n/client-oauth2/src/types.ts b/packages/@n8n/client-oauth2/src/types.ts index 69c225d827..26a90bd441 100644 --- a/packages/@n8n/client-oauth2/src/types.ts +++ b/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 { + error: string; + error_description?: string; + error_uri?: string; +} diff --git a/packages/@n8n/client-oauth2/test/ClientOAuth2.test.ts b/packages/@n8n/client-oauth2/test/ClientOAuth2.test.ts new file mode 100644 index 0000000000..7e6fa788be --- /dev/null +++ b/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; + }) => + 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: 'Hello, world!', + }, + { + contentType: 'application/xml', + body: 'Hello, world!', + }, + { + 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'); + }); + }); +});