From e7cc6bf28f94947e4e64acf0ad57856dd1a1283c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 24 Nov 2021 20:59:52 +0100 Subject: [PATCH] scaffolder-backend: refactor SecureTemplater to use separate sandboxes Signed-off-by: Patrik Oldsberg --- .../lib/templating/SecureTemplater.test.ts | 88 ++++++++++--------- .../src/lib/templating/SecureTemplater.ts | 87 ++++++++---------- .../actions/builtin/fetch/template.ts | 16 ++-- .../tasks/NunjucksWorkflowRunner.ts | 56 +++++++----- 4 files changed, 122 insertions(+), 125 deletions(-) diff --git a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts index b83c9eea4f..1ec3f050d1 100644 --- a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts +++ b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts @@ -18,24 +18,21 @@ import { SecureTemplater } from './SecureTemplater'; describe('SecureTemplater', () => { it('should render some templates', async () => { - const templater = new SecureTemplater(); - await templater.initializeIfNeeded(); - expect(templater.render('${{ test }}', { test: 'my-value' })).toBe( - 'my-value', - ); + const render = await SecureTemplater.loadRenderer(); + expect(render('${{ test }}', { test: 'my-value' })).toBe('my-value'); - expect(templater.render('${{ test | dump }}', { test: 'my-value' })).toBe( + expect(render('${{ test | dump }}', { test: 'my-value' })).toBe( '"my-value"', ); expect( - templater.render('${{ test | replace("my-", "our-") }}', { + render('${{ test | replace("my-", "our-") }}', { test: 'my-value', }), ).toBe('our-value'); expect(() => - templater.render('${{ invalid...syntax }}', { + render('${{ invalid...syntax }}', { test: 'my-value', }), ).toThrow( @@ -44,34 +41,28 @@ describe('SecureTemplater', () => { }); it('should make cookiecutter compatibility available when requested', async () => { - const templater = new SecureTemplater(); - await templater.initializeIfNeeded(); + const renderWith = await SecureTemplater.loadRenderer({ + cookiecutterCompat: true, + }); + const renderWithout = await SecureTemplater.loadRenderer(); // Same two tests repeated to make sure switching back and forth works - expect( - templater.render('{{ 1 | jsonify }}', {}, { cookiecutterCompat: true }), - ).toBe('1'); - expect( - templater.render('{{ 1 | jsonify }}', {}, { cookiecutterCompat: true }), - ).toBe('1'); - expect(() => templater.render('${{ 1 | jsonify }}', {})).toThrow( + expect(renderWith('{{ 1 | jsonify }}', {})).toBe('1'); + expect(renderWith('{{ 1 | jsonify }}', {})).toBe('1'); + expect(() => renderWithout('${{ 1 | jsonify }}', {})).toThrow( '(unknown path)\n Error: filter not found: jsonify', ); - expect( - templater.render('{{ 1 | jsonify }}', {}, { cookiecutterCompat: true }), - ).toBe('1'); - expect(() => templater.render('${{ 1 | jsonify }}', {})).toThrow( + expect(renderWith('{{ 1 | jsonify }}', {})).toBe('1'); + expect(() => renderWithout('${{ 1 | jsonify }}', {})).toThrow( '(unknown path)\n Error: filter not found: jsonify', ); - expect(() => templater.render('${{ 1 | jsonify }}', {})).toThrow( + expect(() => renderWithout('${{ 1 | jsonify }}', {})).toThrow( '(unknown path)\n Error: filter not found: jsonify', ); - expect(() => templater.render('${{ 1 | jsonify }}', {})).toThrow( + expect(() => renderWithout('${{ 1 | jsonify }}', {})).toThrow( '(unknown path)\n Error: filter not found: jsonify', ); - expect( - templater.render('{{ 1 | jsonify }}', {}, { cookiecutterCompat: true }), - ).toBe('1'); + expect(renderWith('{{ 1 | jsonify }}', {})).toBe('1'); }); it('should make parseRepoUrl available when requested', async () => { @@ -80,33 +71,29 @@ describe('SecureTemplater', () => { owner: 'my-owner', host: 'my-host.com', })); - const templaterWith = new SecureTemplater({ parseRepoUrl }); - await templaterWith.initializeIfNeeded(); - const templaterWithout = new SecureTemplater(); - await templaterWithout.initializeIfNeeded(); + const renderWith = await SecureTemplater.loadRenderer({ parseRepoUrl }); + const renderWithout = await SecureTemplater.loadRenderer(); const ctx = { repoUrl: 'https://my-host.com/my-owner/my-repo', }; - expect( - templaterWith.render('${{ repoUrl | parseRepoUrl | dump }}', ctx), - ).toBe( + expect(renderWith('${{ repoUrl | parseRepoUrl | dump }}', ctx)).toBe( JSON.stringify({ repo: 'my-repo', owner: 'my-owner', host: 'my-host.com', }), ); - expect(templaterWith.render('${{ repoUrl | projectSlug }}', ctx)).toBe( + expect(renderWith('${{ repoUrl | projectSlug }}', ctx)).toBe( 'my-owner/my-repo', ); expect(() => - templaterWithout.render('${{ repoUrl | parseRepoUrl | dump }}', ctx), + renderWithout('${{ repoUrl | parseRepoUrl | dump }}', ctx), ).toThrow('(unknown path)\n Error: filter not found: parseRepoUrl'); - expect(() => - templaterWithout.render('${{ repoUrl | projectSlug }}', ctx), - ).toThrow('(unknown path)\n Error: filter not found: projectSlug'); + expect(() => renderWithout('${{ repoUrl | projectSlug }}', ctx)).toThrow( + '(unknown path)\n Error: filter not found: projectSlug', + ); expect(parseRepoUrl.mock.calls).toEqual([ ['https://my-host.com/my-owner/my-repo'], @@ -115,26 +102,25 @@ describe('SecureTemplater', () => { }); it('should not allow helpers to be rewritten', async () => { - const templater = new SecureTemplater({ + const render = await SecureTemplater.loadRenderer({ parseRepoUrl: () => ({ repo: 'my-repo', owner: 'my-owner', host: 'my-host.com', }), }); - await templater.initializeIfNeeded(); const ctx = { repoUrl: 'https://my-host.com/my-owner/my-repo', }; expect( - templater.render( + render( '${{ ({}).constructor.constructor("parseRepoUrl = () => JSON.stringify(`inject`)")() }}', ctx, ), ).toBe(''); - expect(templater.render('${{ repoUrl | parseRepoUrl | dump }}', ctx)).toBe( + expect(render('${{ repoUrl | parseRepoUrl | dump }}', ctx)).toBe( JSON.stringify({ repo: 'my-repo', owner: 'my-owner', @@ -142,4 +128,22 @@ describe('SecureTemplater', () => { }), ); }); + + it('allows pollution during a single template execution', async () => { + const render = await SecureTemplater.loadRenderer(); + + const ctx = { + x: 'foo', + }; + expect(render('${{ x }}', ctx)).toBe('foo'); + expect( + render( + '${{ ({}).constructor.constructor("Array.prototype.forEach = () => {}")() }}', + ctx, + ), + ).toBe(''); + expect(() => render('${{ x }}', ctx)).toThrow( + `TypeError: Cannot read property 'length' of undefined`, + ); + }); }); diff --git a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts index eba1c3838d..bee1f310c9 100644 --- a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts +++ b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts @@ -87,68 +87,55 @@ const { render, renderCompat } = (() => { })(); `; -export interface SecureTemplaterRenderOptions { +export interface SecureTemplaterOptions { + /* Optional implementation of the parseRepoUrl filter */ + parseRepoUrl?(repoUrl: string): RepoSpec; + /* Enables jinja compatibility and the "jsonify" filter */ cookiecutterCompat?: boolean; } -export interface SecureTemplaterOptions { - parseRepoUrl?(repoUrl: string): RepoSpec; -} +export type SecureTemplateRenderer = ( + template: string, + values: unknown, +) => string; export class SecureTemplater { - #vm?: VM; + static async loadRenderer(options: SecureTemplaterOptions = {}) { + const { parseRepoUrl, cookiecutterCompat } = options; + let sandbox = undefined; - #parseRepoUrl?: (repoUrl: string) => RepoSpec; - - constructor(options?: SecureTemplaterOptions) { - this.#parseRepoUrl = options?.parseRepoUrl; - } - - render( - template: string, - values: unknown, - options?: SecureTemplaterRenderOptions, - ): string { - if (!this.#vm) { - throw new Error('SecureTemplater has not been initialized'); - } - this.#vm.setGlobal('templateStr', template); - this.#vm.setGlobal('templateValues', JSON.stringify(values)); - - if (options?.cookiecutterCompat) { - return this.#vm.run(`renderCompat(templateStr, templateValues)`); + if (parseRepoUrl) { + sandbox = { + parseRepoUrl: (url: string) => JSON.stringify(parseRepoUrl(url)), + }; } - return this.#vm.run(`render(templateStr, templateValues)`); - } + const vm = new VM({ timeout: 1000, sandbox }); - async initializeIfNeeded() { - if (!this.#vm) { - let sandbox = undefined; + const nunjucksSource = await fs.readFile( + resolvePackagePath( + '@backstage/plugin-scaffolder-backend', + 'assets/nunjucks.js.txt', + ), + 'utf-8', + ); - if (this.#parseRepoUrl) { - const parseRepoUrl = this.#parseRepoUrl; - sandbox = { - parseRepoUrl: (url: string) => JSON.stringify(parseRepoUrl(url)), - }; + vm.run(mkScript(nunjucksSource)); + + const render: SecureTemplateRenderer = (template, values) => { + if (!vm) { + throw new Error('SecureTemplater has not been initialized'); + } + vm.setGlobal('templateStr', template); + vm.setGlobal('templateValues', JSON.stringify(values)); + + if (cookiecutterCompat) { + return vm.run(`renderCompat(templateStr, templateValues)`); } - // Note that helpers in the sandbox can be rewritten with a script injection. - // In order to mitigate that each helper must be assigned to a constant variable - // inside the closure that houses the nunjucks implementation. - this.#vm = new VM({ timeout: 1000, sandbox }); - - const nunjucksSource = await fs.readFile( - resolvePackagePath( - '@backstage/plugin-scaffolder-backend', - 'assets/nunjucks.js.txt', - ), - 'utf-8', - ); - - this.#vm.run(mkScript(nunjucksSource)); - } - return this.#vm; + return vm.run(`render(templateStr, templateValues)`); + }; + return render; } } 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 9e1a3f29db..8dae7b7418 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts @@ -47,8 +47,6 @@ export function createFetchTemplateAction(options: { }) { const { reader, integrations } = options; - const templater = new SecureTemplater(); - return createTemplateAction({ id: 'fetch:template', description: @@ -102,8 +100,6 @@ export function createFetchTemplateAction(options: { async handler(ctx) { ctx.logger.info('Fetching template content from remote URL'); - await templater.initializeIfNeeded(); - const workDir = await ctx.createTemporaryDirectory(); const templateDir = resolvePath(workDir, 'template'); @@ -184,6 +180,10 @@ export function createFetchTemplateAction(options: { ctx.input.values, ); + const renderTemplate = await SecureTemplater.loadRenderer({ + cookiecutterCompat: ctx.input.cookiecutterCompat, + }); + for (const location of allEntriesInTemplate) { let renderFilename: boolean; let renderContents: boolean; @@ -199,9 +199,7 @@ export function createFetchTemplateAction(options: { renderFilename = renderContents = !nonTemplatedEntries.has(location); } if (renderFilename) { - localOutputPath = templater.render(localOutputPath, context, { - cookiecutterCompat: ctx.input.cookiecutterCompat, - }); + localOutputPath = renderTemplate(localOutputPath, context); } const outputPath = resolvePath(outputDir, localOutputPath); // variables have been expanded to make an empty file name @@ -238,9 +236,7 @@ export function createFetchTemplateAction(options: { await fs.outputFile( outputPath, renderContents - ? templater.render(inputFileContents, context, { - cookiecutterCompat: ctx.input.cookiecutterCompat, - }) + ? renderTemplate(inputFileContents, context) : inputFileContents, { mode: statsObj.mode }, ); diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index 37d302e8b2..a3e30c65fe 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -34,7 +34,10 @@ import { isTruthy } from './helper'; import { validate as validateJsonSchema } from 'jsonschema'; import { parseRepoUrl } from '../actions/builtin/publish/util'; import { TemplateActionRegistry } from '../actions'; -import { SecureTemplater } from '../../lib/templating/SecureTemplater'; +import { + SecureTemplater, + SecureTemplateRenderer, +} from '../../lib/templating/SecureTemplater'; type NunjucksWorkflowRunnerOptions = { workingDirectory: string; @@ -86,21 +89,7 @@ const createStepLogger = ({ }; export class NunjucksWorkflowRunner implements WorkflowRunner { - private readonly templater: SecureTemplater; - - constructor(private readonly options: NunjucksWorkflowRunnerOptions) { - const { integrations } = options; - - this.templater = new SecureTemplater({ - // TODO(blam): let's work out how we can deprecate this. - // We shouldn't really need to be exposing these now we can deal with - // objects in the params block. - // Maybe we can expose a new RepoUrlPicker with secrets for V3 that provides an object already. - parseRepoUrl(url: string) { - return parseRepoUrl(url, integrations); - }, - }); - } + constructor(private readonly options: NunjucksWorkflowRunnerOptions) {} private isSingleTemplateString(input: string) { const { parser, nodes } = nunjucks as unknown as { @@ -132,7 +121,11 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { ); } - private render(input: T, context: TemplateContext): T { + private render( + input: T, + context: TemplateContext, + renderTemplate: SecureTemplateRenderer, + ): T { return JSON.parse(JSON.stringify(input), (_key, value) => { try { if (typeof value === 'string') { @@ -145,7 +138,7 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { ); // Run the templating - const templated = this.templater.render(wrappedDumped, context); + const templated = renderTemplate(wrappedDumped, context); // If there's an empty string returned, then it's undefined if (templated === '') { @@ -162,7 +155,7 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { } // Fallback to default behaviour - const templated = this.templater.render(value, context); + const templated = renderTemplate(value, context); if (templated === '') { return undefined; @@ -187,12 +180,23 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { this.options.workingDirectory, await task.getWorkspaceName(), ); + + const { integrations } = this.options; + const renderTemplate = await SecureTemplater.loadRenderer({ + // TODO(blam): let's work out how we can deprecate this. + // We shouldn't really need to be exposing these now we can deal with + // objects in the params block. + // Maybe we can expose a new RepoUrlPicker with secrets for V3 that provides an object already. + parseRepoUrl(url: string) { + return parseRepoUrl(url, integrations); + }, + }); + try { await fs.ensureDir(workspacePath); await task.emitLog( `Starting up task with ${task.spec.steps.length} steps`, ); - await this.templater.initializeIfNeeded(); const context: TemplateContext = { parameters: task.spec.parameters, @@ -202,7 +206,11 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { for (const step of task.spec.steps) { try { if (step.if) { - const ifResult = await this.render(step.if, context); + const ifResult = await this.render( + step.if, + context, + renderTemplate, + ); if (!isTruthy(ifResult)) { await task.emitLog( `Skipping step ${step.id} because it's if condition was false`, @@ -220,7 +228,9 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { const action = this.options.actionRegistry.get(step.action); const { taskLogger, streamLogger } = createStepLogger({ task, step }); - const input = (step.input && this.render(step.input, context)) ?? {}; + const input = + (step.input && this.render(step.input, context, renderTemplate)) ?? + {}; if (action.schema?.input) { const validateResult = validateJsonSchema( @@ -283,7 +293,7 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { } } - const output = this.render(task.spec.output, context); + const output = this.render(task.spec.output, context, renderTemplate); return { output }; } finally {