From f0f8d59fee223c6bc9f8459890ed4a31ef8cb0af 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: Tue, 21 Feb 2023 11:21:04 +0100 Subject: [PATCH] fix(core): Do not allow arbitrary path traversal in the credential-translation endpoint (#5522) --- packages/cli/src/Server.ts | 45 +------------- packages/cli/src/TranslationHelpers.ts | 16 ----- packages/cli/src/controllers/index.ts | 1 + .../src/controllers/translation.controller.ts | 58 +++++++++++++++++++ packages/cli/test/setup-mocks.ts | 2 + .../translation.controller.test.ts | 40 +++++++++++++ 6 files changed, 103 insertions(+), 59 deletions(-) create mode 100644 packages/cli/src/controllers/translation.controller.ts create mode 100644 packages/cli/test/unit/controllers/translation.controller.test.ts diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index 8d2b740a20..a5832346bd 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'; @@ -88,6 +87,7 @@ import { MeController, OwnerController, PasswordResetController, + TranslationController, UsersController, } from '@/controllers'; @@ -366,6 +366,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, @@ -606,48 +607,6 @@ class Server extends AbstractServer { ), ); - this.app.get( - `/${this.restEndpoint}/credential-translation`, - ResponseHelper.send( - async ( - req: express.Request & { query: { credentialType: string } }, - res: express.Response, - ): Promise => { - const translationPath = getCredentialTranslationPath({ - locale: this.frontendSettings.defaultLocale, - credentialType: req.query.credentialType, - }); - - 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/TranslationHelpers.ts b/packages/cli/src/TranslationHelpers.ts index cc4319b04f..dd829534a6 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`); -} diff --git a/packages/cli/src/controllers/index.ts b/packages/cli/src/controllers/index.ts index 2ee7c7fd06..37ce548a54 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 0000000000..8d24f9e113 --- /dev/null +++ b/packages/cli/src/controllers/translation.controller.ts @@ -0,0 +1,58 @@ +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'; + +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: TranslationRequest.Credential) { + 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( + CREDENTIAL_TRANSLATIONS_DIR, + defaultLocale, + `${credentialType}.json`, + ); + + try { + // eslint-disable-next-line @typescript-eslint/no-unsafe-return + return require(translationPath); + } catch (error) { + return null; + } + } + + @Get('/node-translation-headers') + async getNodeTranslationHeaders() { + try { + await access(`${NODE_HEADERS_PATH}.js`); + } catch (_) { + return; // no headers available + } + + try { + // eslint-disable-next-line @typescript-eslint/no-unsafe-return + 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 1a9bb4e9c0..c6db2d147c 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 0000000000..a24d462f81 --- /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); + }); + }); +});