From af6b692cb3ca685d97ef16b40b6e9dc99913e3d4 Mon Sep 17 00:00:00 2001 From: David Zemon Date: Fri, 21 Jan 2022 09:37:05 -0600 Subject: [PATCH] feat: [8920] Define explicit type for filter; Add SecureTemplater tests Signed-off-by: David Zemon --- .../lib/templating/SecureTemplater.test.ts | 46 +++++++++++++++++++ .../src/lib/templating/SecureTemplater.ts | 37 +++++++++++---- .../actions/builtin/createBuiltinActions.ts | 3 +- .../actions/builtin/fetch/template.ts | 7 ++- .../tasks/NunjucksWorkflowRunner.ts | 3 +- .../src/scaffolder/tasks/TaskWorker.ts | 3 +- .../scaffolder-backend/src/service/router.ts | 3 +- 7 files changed, 87 insertions(+), 15 deletions(-) diff --git a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts index 134b877efe..ce524384c7 100644 --- a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts +++ b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts @@ -99,6 +99,52 @@ describe('SecureTemplater', () => { ]); }); + it('should make additional filters available when requested', async () => { + const mockFilter1 = jest.fn(() => 'filtered text'); + const mockFilter2 = jest.fn((var1, var2) => `${var1} ${var2}`); + const mockFilter3 = jest.fn((var1, var2) => ({ var1, var2 })); + const renderWith = await SecureTemplater.loadRenderer({ + additionalFilters: { mockFilter1, mockFilter2, mockFilter3 }, + }); + const renderWithout = await SecureTemplater.loadRenderer(); + + const ctx = { inputValue: 'the input value' }; + + expect(renderWith('${{ inputValue | mockFilter1 }}', ctx)).toBe( + 'filtered text', + ); + expect( + renderWith('${{ inputValue | mockFilter2("extra arg") }}', ctx), + ).toBe('the input value extra arg'); + expect( + renderWith( + '${{ inputValue | mockFilter3("another extra arg") | dump }}', + ctx, + ), + ).toBe( + JSON.stringify({ + var1: 'the input value', + var2: 'another extra arg', + }), + ); + + expect(() => renderWithout('${{ inputValue | mockFilter1 }}', ctx)).toThrow( + /Error: filter not found: mockFilter1/, + ); + expect(() => + renderWithout('${{ inputValue | mockFilter2("extra arg") }}', ctx), + ).toThrow(/Error: filter not found: mockFilter2/); + expect(() => + renderWithout('${{ inputValue | mockFilter3("extra arg") }}', ctx), + ).toThrow(/Error: filter not found: mockFilter3/); + + expect(mockFilter1.mock.calls).toEqual([['the input value']]); + expect(mockFilter2.mock.calls).toEqual([['the input value', 'extra arg']]); + expect(mockFilter3.mock.calls).toEqual([ + ['the input value', 'another extra arg'], + ]); + }); + it('should not allow helpers to be rewritten', async () => { const render = await SecureTemplater.loadRenderer({ parseRepoUrl: () => ({ diff --git a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts index 416bad3295..b3e99cf1dd 100644 --- a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts +++ b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts @@ -17,8 +17,10 @@ import { VM } from 'vm2'; import { resolvePackagePath } from '@backstage/backend-common'; import fs from 'fs-extra'; +import { JsonValue } from '@backstage/types'; import { RepoSpec } from '../../scaffolder/actions/builtin/publish/util'; +// language=JavaScript const mkScript = (nunjucksSource: string) => ` const { render, renderCompat } = (() => { const module = {}; @@ -56,6 +58,16 @@ const { render, renderCompat } = (() => { }); } + if (typeof additionalFilters !== "undefined") { + Object.entries(additionalFilters) + .forEach(([filterName, filterFunction]) => { + env.addFilter( + filterName, + (...args) => JSON.parse(filterFunction.apply(null, args)) + ); + }); + } + let uninstallCompat = undefined; function render(str, values) { @@ -87,6 +99,8 @@ const { render, renderCompat } = (() => { })(); `; +export type NunjucksFilter = (...args: JsonValue[]) => JsonValue | undefined; + export interface SecureTemplaterOptions { /* Optional implementation of the parseRepoUrl filter */ parseRepoUrl?(repoUrl: string): RepoSpec; @@ -95,7 +109,7 @@ export interface SecureTemplaterOptions { cookiecutterCompat?: boolean; /* Extra user-provided nunjucks filters */ - additionalFilters?: Record any>; + additionalFilters?: Record; } export type SecureTemplateRenderer = ( @@ -106,17 +120,22 @@ export type SecureTemplateRenderer = ( export class SecureTemplater { static async loadRenderer(options: SecureTemplaterOptions = {}) { const { parseRepoUrl, cookiecutterCompat, additionalFilters } = options; - let sandbox = undefined; + const sandbox: Record = {}; if (parseRepoUrl) { - sandbox = { - parseRepoUrl: (url: string) => JSON.stringify(parseRepoUrl(url)), - }; + sandbox.parseRepoUrl = (url: string) => JSON.stringify(parseRepoUrl(url)); + } + + if (additionalFilters) { + sandbox.additionalFilters = Object.entries(additionalFilters) + .filter(([_, filterFunction]) => !!filterFunction) + .reduce((safeFilters, [filterName, filterFunction]) => { + const newSafeFilters = { ...safeFilters }; + newSafeFilters[filterName] = (...args) => + JSON.stringify(filterFunction.apply(null, args)); + return newSafeFilters; + }, {} as Record); } - sandbox = { - ...sandbox, - ...additionalFilters, - }; const vm = new VM({ sandbox }); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/createBuiltinActions.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/createBuiltinActions.ts index 492e1881b8..9cc60a6b1e 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/createBuiltinActions.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/createBuiltinActions.ts @@ -46,6 +46,7 @@ import { createGithubActionsDispatchAction, createGithubWebhookAction, } from './github'; +import { NunjucksFilter } from '../../../lib/templating/SecureTemplater'; export const createBuiltinActions = (options: { reader: UrlReader; @@ -53,7 +54,7 @@ export const createBuiltinActions = (options: { catalogClient: CatalogApi; containerRunner?: ContainerRunner; config: Config; - nunjucksFilters?: Record any>; + nunjucksFilters?: Record; }) => { const { reader, diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts index 7d68c4a129..c4c666abc1 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts @@ -23,7 +23,10 @@ import { createTemplateAction } from '../../createTemplateAction'; import globby from 'globby'; import fs from 'fs-extra'; import { isBinaryFile } from 'isbinaryfile'; -import { SecureTemplater } from '../../../../lib/templating/SecureTemplater'; +import { + NunjucksFilter, + SecureTemplater, +} from '../../../../lib/templating/SecureTemplater'; type CookieCompatInput = { copyWithoutRender?: string[]; @@ -44,7 +47,7 @@ export type FetchTemplateInput = { export function createFetchTemplateAction(options: { reader: UrlReader; integrations: ScmIntegrations; - nunjucksFilters?: Record any>; + nunjucksFilters?: Record; }) { const { reader, integrations, nunjucksFilters } = options; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index 1a0b9c9d16..5bcf83a231 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -35,6 +35,7 @@ import { validate as validateJsonSchema } from 'jsonschema'; import { parseRepoUrl } from '../actions/builtin/publish/util'; import { TemplateActionRegistry } from '../actions'; import { + NunjucksFilter, SecureTemplater, SecureTemplateRenderer, } from '../../lib/templating/SecureTemplater'; @@ -44,7 +45,7 @@ type NunjucksWorkflowRunnerOptions = { actionRegistry: TemplateActionRegistry; integrations: ScmIntegrations; logger: winston.Logger; - additionalFilters?: Record any>; + additionalFilters?: Record; }; type TemplateContext = { diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts index adbcd75773..8bf10b051a 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts @@ -21,6 +21,7 @@ import { Logger } from 'winston'; import { TemplateActionRegistry } from '../actions'; import { ScmIntegrations } from '@backstage/integration'; import { assertError } from '@backstage/errors'; +import { NunjucksFilter } from '../../lib/templating/SecureTemplater'; /** * TaskWorkerOptions @@ -46,7 +47,7 @@ export type CreateWorkerOptions = { integrations: ScmIntegrations; workingDirectory: string; logger: Logger; - nunjucksFilters?: Record any>; + nunjucksFilters?: Record; }; /** diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index b12dace5fa..3770f525a4 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -41,6 +41,7 @@ import { } from '../scaffolder'; import { StorageTaskBroker } from '../scaffolder/tasks/StorageTaskBroker'; import { getEntityBaseUrl, getWorkingDirectory } from './helpers'; +import { NunjucksFilter } from '../lib/templating/SecureTemplater'; /** * RouterOptions @@ -57,7 +58,7 @@ export interface RouterOptions { taskWorkers?: number; containerRunner?: ContainerRunner; taskBroker?: TaskBroker; - nunjucksFilters?: Record any>; + nunjucksFilters?: Record; } function isSupportedTemplate(