From 4d88472b626ef8096ebd18f8be4f9ebb8cb95795 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 20 Feb 2023 17:07:54 +0100 Subject: [PATCH 1/3] fix(core): Do not allow arbitrary path traversal in the credential-translation endpoint --- packages/cli/src/Server.ts | 18 +++++++++++++----- packages/cli/src/TranslationHelpers.ts | 16 ---------------- 2 files changed, 13 insertions(+), 21 deletions(-) diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index 87e9b7463ea06..60ea0a82650ad 100644 --- a/packages/cli/src/Server.ts +++ b/packages/cli/src/Server.ts @@ -57,7 +57,6 @@ import history from 'connect-history-api-fallback'; import config from '@/config'; import * as Queue from '@/Queue'; import { InternalHooksManager } from '@/InternalHooksManager'; -import { getCredentialTranslationPath } from '@/TranslationHelpers'; import { getSharedWorkflowIds } from '@/WorkflowHelpers'; import { nodesController } from '@/api/nodes.api'; @@ -589,6 +588,7 @@ class Server extends AbstractServer { ), ); + const credsPath = pathJoin(NODES_BASE_DIR, 'dist', 'credentials'); this.app.get( `/${this.restEndpoint}/credential-translation`, ResponseHelper.send( @@ -596,10 +596,18 @@ class Server extends AbstractServer { req: express.Request & { query: { credentialType: string } }, res: express.Response, ): Promise => { - const translationPath = getCredentialTranslationPath({ - locale: this.frontendSettings.defaultLocale, - credentialType: req.query.credentialType, - }); + const { credentialType } = req.query; + + if (!this.credentialTypes.recognizes(credentialType)) + throw new ResponseHelper.BadRequestError(`Invalid Credential type ${credentialType}`); + + const { defaultLocale } = this.frontendSettings; + const translationPath = pathJoin( + credsPath, + 'translations', + defaultLocale, + `${credentialType}.json`, + ); try { return require(translationPath); diff --git a/packages/cli/src/TranslationHelpers.ts b/packages/cli/src/TranslationHelpers.ts index cc4319b04f283..dd829534a6d78 100644 --- a/packages/cli/src/TranslationHelpers.ts +++ b/packages/cli/src/TranslationHelpers.ts @@ -1,7 +1,6 @@ import { join, dirname } from 'path'; import { readdir } from 'fs/promises'; import type { Dirent } from 'fs'; -import { NODES_BASE_DIR } from '@/constants'; const ALLOWED_VERSIONED_DIRNAME_LENGTH = [2, 3]; // e.g. v1, v10 @@ -47,18 +46,3 @@ export async function getNodeTranslationPath({ ? join(nodeDir, `v${maxVersion}`, 'translations', locale, `${nodeType}.json`) : join(nodeDir, 'translations', locale, `${nodeType}.json`); } - -/** - * Get the full path to a credential translation file in `/dist`. - */ -export function getCredentialTranslationPath({ - locale, - credentialType, -}: { - locale: string; - credentialType: string; -}): string { - const credsPath = join(NODES_BASE_DIR, 'dist', 'credentials'); - - return join(credsPath, 'translations', locale, `${credentialType}.json`); -} From 49ede90976c22f81a975cc503c7106f4220ad86e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 20 Feb 2023 17:25:19 +0100 Subject: [PATCH 2/3] move translation endpoints to a new controller --- packages/cli/src/Server.ts | 53 +----------------- packages/cli/src/controllers/index.ts | 1 + .../src/controllers/translation.controller.ts | 54 +++++++++++++++++++ 3 files changed, 57 insertions(+), 51 deletions(-) create mode 100644 packages/cli/src/controllers/translation.controller.ts diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index 60ea0a82650ad..e0f47f127e969 100644 --- a/packages/cli/src/Server.ts +++ b/packages/cli/src/Server.ts @@ -87,6 +87,7 @@ import { MeController, OwnerController, PasswordResetController, + TranslationController, UsersController, } from '@/controllers'; @@ -350,6 +351,7 @@ class Server extends AbstractServer { new OwnerController({ config, internalHooks, repositories, logger }), new MeController({ externalHooks, internalHooks, repositories, logger }), new PasswordResetController({ config, externalHooks, internalHooks, repositories, logger }), + new TranslationController(config, this.credentialTypes), new UsersController({ config, mailer, @@ -588,57 +590,6 @@ class Server extends AbstractServer { ), ); - const credsPath = pathJoin(NODES_BASE_DIR, 'dist', 'credentials'); - this.app.get( - `/${this.restEndpoint}/credential-translation`, - ResponseHelper.send( - async ( - req: express.Request & { query: { credentialType: string } }, - res: express.Response, - ): Promise => { - const { credentialType } = req.query; - - if (!this.credentialTypes.recognizes(credentialType)) - throw new ResponseHelper.BadRequestError(`Invalid Credential type ${credentialType}`); - - const { defaultLocale } = this.frontendSettings; - const translationPath = pathJoin( - credsPath, - 'translations', - defaultLocale, - `${credentialType}.json`, - ); - - try { - return require(translationPath); - } catch (error) { - return null; - } - }, - ), - ); - - // Returns node information based on node names and versions - const headersPath = pathJoin(NODES_BASE_DIR, 'dist', 'nodes', 'headers'); - this.app.get( - `/${this.restEndpoint}/node-translation-headers`, - ResponseHelper.send( - async (req: express.Request, res: express.Response): Promise => { - try { - await fsAccess(`${headersPath}.js`); - } catch (_) { - return; // no headers available - } - - try { - return require(headersPath); - } catch (error) { - res.status(500).send('Failed to load headers file'); - } - }, - ), - ); - // ---------------------------------------- // Node-Types // ---------------------------------------- diff --git a/packages/cli/src/controllers/index.ts b/packages/cli/src/controllers/index.ts index 2ee7c7fd064ec..37ce548a540dc 100644 --- a/packages/cli/src/controllers/index.ts +++ b/packages/cli/src/controllers/index.ts @@ -2,4 +2,5 @@ export { AuthController } from './auth.controller'; export { MeController } from './me.controller'; export { OwnerController } from './owner.controller'; export { PasswordResetController } from './passwordReset.controller'; +export { TranslationController } from './translation.controller'; export { UsersController } from './users.controller'; diff --git a/packages/cli/src/controllers/translation.controller.ts b/packages/cli/src/controllers/translation.controller.ts new file mode 100644 index 0000000000000..35c076f23b9c2 --- /dev/null +++ b/packages/cli/src/controllers/translation.controller.ts @@ -0,0 +1,54 @@ +import type { Request } from 'express'; +import { ICredentialTypes } from 'n8n-workflow'; +import { join } from 'path'; +import { access } from 'fs/promises'; +import { Get, RestController } from '@/decorators'; +import { BadRequestError, InternalServerError } from '@/ResponseHelper'; +import { Config } from '@/config'; +import { NODES_BASE_DIR } from '@/constants'; + +const credentialTranslationsDir = join(NODES_BASE_DIR, 'dist/credentials/translations'); +const nodeHeadersPath = join(NODES_BASE_DIR, 'dist/nodes/headers'); + +@RestController('/') +export class TranslationController { + constructor(private config: Config, private credentialTypes: ICredentialTypes) {} + + @Get('/credential-translation') + async getCredentialTranslation(req: Request & { query: { credentialType: string } }) { + const { credentialType } = req.query; + + if (!this.credentialTypes.recognizes(credentialType)) + throw new BadRequestError(`Invalid Credential type ${credentialType}`); + + const defaultLocale = this.config.getEnv('defaultLocale'); + const translationPath = join( + credentialTranslationsDir, + defaultLocale, + `${credentialType}.json`, + ); + + try { + // eslint-disable-next-line @typescript-eslint/no-unsafe-return + return await import(translationPath); + } catch (error) { + return null; + } + } + + @Get('/node-translation-headers') + async getNodeTranslationHeaders() { + try { + await access(`${nodeHeadersPath}.js`); + } catch (_) { + return; // no headers available + } + + try { + // eslint-disable-next-line @typescript-eslint/no-unsafe-return + return await import(nodeHeadersPath); + } catch (error) { + throw new InternalServerError('Failed to load headers file'); + } + } +} From eb9cbb4e6f86e49990f047f0bd381470cf1e7099 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 20 Feb 2023 17:59:33 +0100 Subject: [PATCH 3/3] add unit tests for TranslationController --- .../src/controllers/translation.controller.ts | 20 ++++++---- packages/cli/test/setup-mocks.ts | 2 + .../translation.controller.test.ts | 40 +++++++++++++++++++ 3 files changed, 54 insertions(+), 8 deletions(-) create mode 100644 packages/cli/test/unit/controllers/translation.controller.test.ts diff --git a/packages/cli/src/controllers/translation.controller.ts b/packages/cli/src/controllers/translation.controller.ts index 35c076f23b9c2..8d24f9e11365e 100644 --- a/packages/cli/src/controllers/translation.controller.ts +++ b/packages/cli/src/controllers/translation.controller.ts @@ -7,30 +7,34 @@ import { BadRequestError, InternalServerError } from '@/ResponseHelper'; import { Config } from '@/config'; import { NODES_BASE_DIR } from '@/constants'; -const credentialTranslationsDir = join(NODES_BASE_DIR, 'dist/credentials/translations'); -const nodeHeadersPath = join(NODES_BASE_DIR, 'dist/nodes/headers'); +export const CREDENTIAL_TRANSLATIONS_DIR = 'n8n-nodes-base/dist/credentials/translations'; +export const NODE_HEADERS_PATH = join(NODES_BASE_DIR, 'dist/nodes/headers'); + +export declare namespace TranslationRequest { + export type Credential = Request<{}, {}, {}, { credentialType: string }>; +} @RestController('/') export class TranslationController { constructor(private config: Config, private credentialTypes: ICredentialTypes) {} @Get('/credential-translation') - async getCredentialTranslation(req: Request & { query: { credentialType: string } }) { + async getCredentialTranslation(req: TranslationRequest.Credential) { const { credentialType } = req.query; if (!this.credentialTypes.recognizes(credentialType)) - throw new BadRequestError(`Invalid Credential type ${credentialType}`); + throw new BadRequestError(`Invalid Credential type: "${credentialType}"`); const defaultLocale = this.config.getEnv('defaultLocale'); const translationPath = join( - credentialTranslationsDir, + CREDENTIAL_TRANSLATIONS_DIR, defaultLocale, `${credentialType}.json`, ); try { // eslint-disable-next-line @typescript-eslint/no-unsafe-return - return await import(translationPath); + return require(translationPath); } catch (error) { return null; } @@ -39,14 +43,14 @@ export class TranslationController { @Get('/node-translation-headers') async getNodeTranslationHeaders() { try { - await access(`${nodeHeadersPath}.js`); + await access(`${NODE_HEADERS_PATH}.js`); } catch (_) { return; // no headers available } try { // eslint-disable-next-line @typescript-eslint/no-unsafe-return - return await import(nodeHeadersPath); + return require(NODE_HEADERS_PATH); } catch (error) { throw new InternalServerError('Failed to load headers file'); } diff --git a/packages/cli/test/setup-mocks.ts b/packages/cli/test/setup-mocks.ts index 1a9bb4e9c0a15..c6db2d147cdc8 100644 --- a/packages/cli/test/setup-mocks.ts +++ b/packages/cli/test/setup-mocks.ts @@ -1,3 +1,5 @@ +import 'reflect-metadata'; + jest.mock('@sentry/node'); jest.mock('@n8n_io/license-sdk'); jest.mock('@/telemetry'); diff --git a/packages/cli/test/unit/controllers/translation.controller.test.ts b/packages/cli/test/unit/controllers/translation.controller.test.ts new file mode 100644 index 0000000000000..a24d462f81818 --- /dev/null +++ b/packages/cli/test/unit/controllers/translation.controller.test.ts @@ -0,0 +1,40 @@ +import { mock } from 'jest-mock-extended'; +import type { ICredentialTypes } from 'n8n-workflow'; +import type { Config } from '@/config'; +import { + TranslationController, + TranslationRequest, + CREDENTIAL_TRANSLATIONS_DIR, +} from '@/controllers/translation.controller'; +import { BadRequestError } from '@/ResponseHelper'; + +describe('TranslationController', () => { + const config = mock(); + const credentialTypes = mock(); + const controller = new TranslationController(config, credentialTypes); + + describe('getCredentialTranslation', () => { + it('should throw 400 on invalid credential types', async () => { + const credentialType = 'not-a-valid-credential-type'; + const req = mock({ query: { credentialType } }); + credentialTypes.recognizes.calledWith(credentialType).mockReturnValue(false); + + expect(controller.getCredentialTranslation(req)).rejects.toThrowError( + new BadRequestError(`Invalid Credential type: "${credentialType}"`), + ); + }); + + it('should return translation json on valid credential types', async () => { + const credentialType = 'credential-type'; + const req = mock({ query: { credentialType } }); + config.getEnv.calledWith('defaultLocale').mockReturnValue('de'); + credentialTypes.recognizes.calledWith(credentialType).mockReturnValue(true); + const response = { translation: 'string' }; + jest.mock(`${CREDENTIAL_TRANSLATIONS_DIR}/de/credential-type.json`, () => response, { + virtual: true, + }); + + expect(await controller.getCredentialTranslation(req)).toEqual(response); + }); + }); +});