From 4fb8469d37993f66794a7dea11e812e51a2a83ff Mon Sep 17 00:00:00 2001 From: Justin Bryant Date: Fri, 17 Apr 2026 17:00:37 -0400 Subject: [PATCH] fix(SecureTemplater): return dispose function to clean up secure templater isolate vm & context Signed-off-by: Justin Bryant --- .../lib/templating/SecureTemplater.test.ts | 57 ++++++++++++------- .../src/lib/templating/SecureTemplater.ts | 13 ++++- .../builtin/fetch/templateActionHandler.ts | 26 +++++---- .../fetch/templateFileActionHandler.ts | 26 +++++---- .../tasks/NunjucksWorkflowRunner.ts | 26 +++++---- 5 files changed, 90 insertions(+), 58 deletions(-) diff --git a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts index 53bf67fd87..2957848279 100644 --- a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts +++ b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts @@ -18,7 +18,7 @@ import { SecureTemplater } from './SecureTemplater'; describe('SecureTemplater', () => { it('should render some templates', async () => { - const render = await SecureTemplater.loadRenderer(); + const { render, dispose } = await SecureTemplater.loadRenderer(); expect(render('${{ test }}', { test: 'my-value' })).toBe('my-value'); expect(render('${{ test | dump }}', { test: 'my-value' })).toBe( @@ -36,13 +36,16 @@ describe('SecureTemplater', () => { test: 'my-value', }), ).toThrow(/expected name as lookup value, got ./); + dispose(); }); it('should make cookiecutter compatibility available when requested', async () => { - const renderWith = await SecureTemplater.loadRenderer({ - cookiecutterCompat: true, - }); - const renderWithout = await SecureTemplater.loadRenderer(); + const { render: renderWith, dispose: disposeWith } = + await SecureTemplater.loadRenderer({ + cookiecutterCompat: true, + }); + const { render: renderWithout, dispose: disposeWithout } = + await SecureTemplater.loadRenderer(); // Same two tests repeated to make sure switching back and forth works expect(renderWith('{{ 1 | jsonify }}', {})).toBe('1'); @@ -61,6 +64,8 @@ describe('SecureTemplater', () => { /Error: filter not found: jsonify/, ); expect(renderWith('{{ 1 | jsonify }}', {})).toBe('1'); + disposeWith(); + disposeWithout(); }); it('should make parseRepoUrl available when requested', async () => { @@ -72,10 +77,12 @@ describe('SecureTemplater', () => { const projectSlug = jest.fn(() => 'my-owner/my-repo'); - const renderWith = await SecureTemplater.loadRenderer({ - templateFilters: { parseRepoUrl, projectSlug }, - }); - const renderWithout = await SecureTemplater.loadRenderer(); + const { render: renderWith, dispose: disposeWith } = + await SecureTemplater.loadRenderer({ + templateFilters: { parseRepoUrl, projectSlug }, + }); + const { render: renderWithout, dispose: disposeWithout } = + await SecureTemplater.loadRenderer(); const ctx = { repoUrl: 'https://my-host.com/my-owner/my-repo', @@ -101,6 +108,8 @@ describe('SecureTemplater', () => { expect(parseRepoUrl.mock.calls).toEqual([ ['https://my-host.com/my-owner/my-repo'], ]); + disposeWith(); + disposeWithout(); }); it('should make additional filters available when requested', async () => { @@ -108,10 +117,12 @@ describe('SecureTemplater', () => { const mockFilter2 = jest.fn((var1, var2) => `${var1} ${var2}`); const mockFilter3 = jest.fn((var1, var2) => ({ var1, var2 })); const mockFilter4 = jest.fn(() => undefined); - const renderWith = await SecureTemplater.loadRenderer({ - templateFilters: { mockFilter1, mockFilter2, mockFilter3, mockFilter4 }, - }); - const renderWithout = await SecureTemplater.loadRenderer(); + const { render: renderWith, dispose: disposeWith } = + await SecureTemplater.loadRenderer({ + templateFilters: { mockFilter1, mockFilter2, mockFilter3, mockFilter4 }, + }); + const { render: renderWithout, dispose: disposeWithout } = + await SecureTemplater.loadRenderer(); const ctx = { inputValue: 'the input value' }; @@ -149,16 +160,20 @@ describe('SecureTemplater', () => { expect(mockFilter3.mock.calls).toEqual([ ['the input value', 'another extra arg'], ]); + disposeWith(); + disposeWithout(); }); it('should make additional globals available when requested', async () => { const mockGlobal1 = jest.fn(() => 'awesome global function'); const mockGlobal2 = 'foo'; const mockGlobal3 = 123456; const mockGlobal4 = jest.fn(() => undefined); - const renderWith = await SecureTemplater.loadRenderer({ - templateGlobals: { mockGlobal1, mockGlobal2, mockGlobal3, mockGlobal4 }, - }); - const renderWithout = await SecureTemplater.loadRenderer(); + const { render: renderWith, dispose: disposeWith } = + await SecureTemplater.loadRenderer({ + templateGlobals: { mockGlobal1, mockGlobal2, mockGlobal3, mockGlobal4 }, + }); + const { render: renderWithout, dispose: disposeWithout } = + await SecureTemplater.loadRenderer(); const ctx = {}; @@ -172,10 +187,12 @@ describe('SecureTemplater', () => { expect(() => renderWithout('${{ mockGlobal1() }}', ctx)).toThrow( /Error: Unable to call `mockGlobal1`/, ); + disposeWith(); + disposeWithout(); }); it('should not allow helpers to be rewritten', async () => { - const render = await SecureTemplater.loadRenderer({ + const { render, dispose } = await SecureTemplater.loadRenderer({ templateFilters: { parseRepoUrl: () => ({ repo: 'my-repo', @@ -202,10 +219,11 @@ describe('SecureTemplater', () => { host: 'my-host.com', }), ); + dispose(); }); it('allows pollution during a single template execution', async () => { - const render = await SecureTemplater.loadRenderer(); + const { render, dispose } = await SecureTemplater.loadRenderer(); const ctx = { x: 'foo', @@ -218,5 +236,6 @@ describe('SecureTemplater', () => { ), ).toBe(''); expect(() => render('${{ x }}', ctx)).toThrow(); + dispose(); }); }); diff --git a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts index 709cb1308d..71a7a14e60 100644 --- a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts +++ b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts @@ -14,14 +14,14 @@ * limitations under the License. */ -import { Isolate } from 'isolated-vm'; import { resolvePackagePath } from '@backstage/backend-plugin-api'; import { TemplateFilter, TemplateGlobal, } from '@backstage/plugin-scaffolder-node'; -import fs from 'fs-extra'; import { JsonValue } from '@backstage/types'; +import fs from 'fs-extra'; +import { Isolate } from 'isolated-vm'; import { getMajorNodeVersion, isNoNodeSnapshotOptionProvided } from './helpers'; // language=JavaScript @@ -215,6 +215,13 @@ export class SecureTemplater { return context.evalSync(`render(templateStr, templateValues)`); }; - return render; + + return { + render, + dispose: () => { + context.release(); + isolate.dispose(); + }, + }; } } diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateActionHandler.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateActionHandler.ts index e436616651..9ee580e324 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateActionHandler.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateActionHandler.ts @@ -27,10 +27,10 @@ import { import fs from 'fs-extra'; import globby from 'globby'; import { isBinaryFile } from 'isbinaryfile'; -import { createDefaultFilters } from '../../../../lib/templating/filters/createDefaultFilters'; -import { convertFiltersToRecord } from '../../../../util/templating'; -import { SecureTemplater } from '../../../../lib/templating/SecureTemplater'; import { extname } from 'path'; +import { createDefaultFilters } from '../../../../lib/templating/filters/createDefaultFilters'; +import { SecureTemplater } from '../../../../lib/templating/SecureTemplater'; +import { convertFiltersToRecord } from '../../../../util/templating'; export type TemplateActionInput = { targetPath?: string; @@ -107,15 +107,16 @@ export async function createTemplateActionHandler< ctx.input.values, ); - const renderTemplate = await SecureTemplater.loadRenderer({ - cookiecutterCompat: ctx.input.cookiecutterCompat, - templateFilters, - templateGlobals, - nunjucksConfigs: { - trimBlocks: ctx.input.trimBlocks, - lstripBlocks: ctx.input.lstripBlocks, - }, - }); + const { render: renderTemplate, dispose } = + await SecureTemplater.loadRenderer({ + cookiecutterCompat: ctx.input.cookiecutterCompat, + templateFilters, + templateGlobals, + nunjucksConfigs: { + trimBlocks: ctx.input.trimBlocks, + lstripBlocks: ctx.input.lstripBlocks, + }, + }); for (const location of allEntriesInTemplate) { let renderContents: boolean; @@ -186,6 +187,7 @@ export async function createTemplateActionHandler< } } } + dispose(); ctx.logger.info(`Template result written to ${outputDir}`); } diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateFileActionHandler.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateFileActionHandler.ts index d82a1dd5a2..8e5e8d14e9 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateFileActionHandler.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateFileActionHandler.ts @@ -13,6 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +import { resolveSafeChildPath } from '@backstage/backend-plugin-api'; import { ScmIntegrations } from '@backstage/integration'; import { ActionContext, @@ -20,11 +21,10 @@ import { TemplateGlobal, } from '@backstage/plugin-scaffolder-node'; import fs from 'fs-extra'; -import { createDefaultFilters } from '../../../../lib/templating/filters/createDefaultFilters'; -import { convertFiltersToRecord } from '../../../../util/templating'; -import { resolveSafeChildPath } from '@backstage/backend-plugin-api'; import path from 'path'; +import { createDefaultFilters } from '../../../../lib/templating/filters/createDefaultFilters'; import { SecureTemplater } from '../../../../lib/templating/SecureTemplater'; +import { convertFiltersToRecord } from '../../../../util/templating'; export type TemplateFileActionInput = { targetPath: string; @@ -80,20 +80,22 @@ export async function createTemplateFileActionHandler< ctx.input.values, ); - const renderTemplate = await SecureTemplater.loadRenderer({ - cookiecutterCompat, - templateFilters, - templateGlobals, - nunjucksConfigs: { - trimBlocks: ctx.input.trimBlocks, - lstripBlocks: ctx.input.lstripBlocks, - }, - }); + const { render: renderTemplate, dispose } = + await SecureTemplater.loadRenderer({ + cookiecutterCompat, + templateFilters, + templateGlobals, + nunjucksConfigs: { + trimBlocks: ctx.input.trimBlocks, + lstripBlocks: ctx.input.lstripBlocks, + }, + }); const contents = await fs.readFile(filePath, 'utf-8'); const result = renderTemplate(contents, context); await fs.ensureDir(path.dirname(outputPath)); await fs.outputFile(outputPath, result); + dispose(); ctx.logger.info(`Template file has been written to ${outputPath}`); } diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index e2e36438a1..a9a3a31121 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -55,15 +55,15 @@ import { TemplateFilter, TemplateGlobal, } from '@backstage/plugin-scaffolder-node'; -import { createDefaultFilters } from '../../lib/templating/filters/createDefaultFilters'; -import { scaffolderActionRules } from '../../service/rules'; -import { createCounterMetric, createHistogramMetric } from '../../util/metrics'; -import { BackstageLoggerTransport, WinstonLogger } from './logger'; -import { convertFiltersToRecord } from '../../util/templating'; import { CheckpointContext, CheckpointState, } from '@backstage/plugin-scaffolder-node/alpha'; +import { createDefaultFilters } from '../../lib/templating/filters/createDefaultFilters'; +import { scaffolderActionRules } from '../../service/rules'; +import { createCounterMetric, createHistogramMetric } from '../../util/metrics'; +import { convertFiltersToRecord } from '../../util/templating'; +import { BackstageLoggerTransport, WinstonLogger } from './logger'; type NunjucksWorkflowRunnerOptions = { workingDirectory: string; @@ -485,13 +485,14 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { const { additionalTemplateFilters, additionalTemplateGlobals } = this.options; - const renderTemplate = await SecureTemplater.loadRenderer({ - templateFilters: { - ...this.defaultTemplateFilters, - ...additionalTemplateFilters, - }, - templateGlobals: additionalTemplateGlobals, - }); + const { render: renderTemplate, dispose } = + await SecureTemplater.loadRenderer({ + templateFilters: { + ...this.defaultTemplateFilters, + ...additionalTemplateFilters, + }, + templateGlobals: additionalTemplateGlobals, + }); try { await task.rehydrateWorkspace?.({ taskId, targetPath: workspacePath }); @@ -533,6 +534,7 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { const output = this.render(task.spec.output, context, renderTemplate); await taskTrack.markSuccessful(); await task.cleanWorkspace?.(); + dispose(); return { output }; } finally {