diff --git a/.changeset/bitter-files-flash.md b/.changeset/bitter-files-flash.md new file mode 100644 index 0000000000..9b7ab014a5 --- /dev/null +++ b/.changeset/bitter-files-flash.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': major +--- + +Add explicit memory management to SecureTemplater usage diff --git a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts index 53bf67fd87..cd9eedcd96 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,28 @@ describe('SecureTemplater', () => { test: 'my-value', }), ).toThrow(/expected name as lookup value, got ./); + + dispose(); + + expect(() => render('${{ test }}', { test: 'my-value' })).toThrow( + /disposed/i, + ); }); + it('should allow dispose to be called more than once', async () => { + const { dispose } = await SecureTemplater.loadRenderer(); + + dispose(); + + expect(() => dispose()).not.toThrow(); + }); 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 +76,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 +89,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 +120,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 +129,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 +172,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 +199,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 +231,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 +248,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..72815fbd89 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 @@ -202,8 +202,10 @@ export class SecureTemplater { await nunjucksScript.run(context); const render: SecureTemplateRenderer = (template, values) => { - if (!context) { - throw new Error('SecureTemplater has not been initialized'); + if (!context || isolate.isDisposed) { + throw new Error( + 'SecureTemplater has not been initialized or has been disposed', + ); } contextGlobal.setSync('templateStr', String(template)); @@ -215,6 +217,19 @@ export class SecureTemplater { return context.evalSync(`render(templateStr, templateValues)`); }; - return render; + + return { + render, + dispose: () => { + if (context && !isolate.isDisposed) { + try { + context.release(); + isolate.dispose(); + } catch (error) { + // Ignore errors during dispose, as there's not much we can do about it + } + } + }, + }; } } 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 2fa64c6e19..bfd4d24b8c 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateActionHandler.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateActionHandler.ts @@ -107,84 +107,92 @@ 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, + }, + }); + try { + for (const location of allEntriesInTemplate) { + let renderContents: boolean; - for (const location of allEntriesInTemplate) { - let renderContents: boolean; - - let localOutputPath = location; - if (extension) { - renderContents = extname(localOutputPath) === extension; - if (renderContents) { - localOutputPath = localOutputPath.slice(0, -extension.length); - } - // extension is mutual exclusive with copyWithoutRender/copyWithoutTemplating, - // therefore the output path is always rendered. - localOutputPath = renderTemplate(localOutputPath, context); - } else { - renderContents = !nonTemplatedEntries.has(location); - // The logic here is a bit tangled because it depends on two variables. - // If renderFilename is true, which means copyWithoutTemplating is used, - // then the path is always rendered. - // If renderFilename is false, which means copyWithoutRender is used, - // then matched file/directory won't be processed, same as before. - if (renderFilename) { + let localOutputPath = location; + if (extension) { + renderContents = extname(localOutputPath) === extension; + if (renderContents) { + localOutputPath = localOutputPath.slice(0, -extension.length); + } + // extension is mutual exclusive with copyWithoutRender/copyWithoutTemplating, + // therefore the output path is always rendered. localOutputPath = renderTemplate(localOutputPath, context); } else { - localOutputPath = renderContents - ? renderTemplate(localOutputPath, context) - : localOutputPath; + renderContents = !nonTemplatedEntries.has(location); + // The logic here is a bit tangled because it depends on two variables. + // If renderFilename is true, which means copyWithoutTemplating is used, + // then the path is always rendered. + // If renderFilename is false, which means copyWithoutRender is used, + // then matched file/directory won't be processed, same as before. + if (renderFilename) { + localOutputPath = renderTemplate(localOutputPath, context); + } else { + localOutputPath = renderContents + ? renderTemplate(localOutputPath, context) + : localOutputPath; + } } - } - if (containsSkippedContent(localOutputPath)) { - continue; - } + if (containsSkippedContent(localOutputPath)) { + continue; + } - const outputPath = resolveSafeChildPath(outputDir, localOutputPath); - if (fs.existsSync(outputPath) && !ctx.input.replace) { - continue; - } + const outputPath = resolveSafeChildPath(outputDir, localOutputPath); + if (fs.existsSync(outputPath) && !ctx.input.replace) { + continue; + } - if (!renderContents && !extension) { - ctx.logger.info(`Copying file/directory ${location} without processing.`); - } - - if (location.endsWith('/')) { - ctx.logger.info(`Writing directory ${location} to template output path.`); - await fs.ensureDir(outputPath); - } else { - const inputFilePath = resolveSafeChildPath(templateDir, location); - const stats = await fs.promises.lstat(inputFilePath); - - if (stats.isSymbolicLink() || (await isBinaryFile(inputFilePath))) { + if (!renderContents && !extension) { ctx.logger.info( - `Copying file binary or symbolic link at ${location}, to template output path.`, + `Copying file/directory ${location} without processing.`, ); - await fs.copy(inputFilePath, outputPath); + } + + if (location.endsWith('/')) { + ctx.logger.info( + `Writing directory ${location} to template output path.`, + ); + await fs.ensureDir(outputPath); } else { - const statsObj = await fs.stat(inputFilePath); - ctx.logger.info( - `Writing file ${location} to template output path with mode ${statsObj.mode}.`, - ); - const inputFileContents = await fs.readFile(inputFilePath, 'utf-8'); - await fs.outputFile( - outputPath, - renderContents - ? renderTemplate(inputFileContents, context) - : inputFileContents, - { mode: statsObj.mode }, - ); + const inputFilePath = resolveSafeChildPath(templateDir, location); + const stats = await fs.promises.lstat(inputFilePath); + + if (stats.isSymbolicLink() || (await isBinaryFile(inputFilePath))) { + ctx.logger.info( + `Copying file binary or symbolic link at ${location}, to template output path.`, + ); + await fs.copy(inputFilePath, outputPath); + } else { + const statsObj = await fs.stat(inputFilePath); + ctx.logger.info( + `Writing file ${location} to template output path with mode ${statsObj.mode}.`, + ); + const inputFileContents = await fs.readFile(inputFilePath, 'utf-8'); + await fs.outputFile( + outputPath, + renderContents + ? renderTemplate(inputFileContents, context) + : inputFileContents, + { mode: statsObj.mode }, + ); + } } } + } finally { + 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 8bc283d479..ab85070fa3 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 'node: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,25 @@ 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); + try { + const contents = await fs.readFile(filePath, 'utf-8'); + const result = renderTemplate(contents, context); + await fs.ensureDir(path.dirname(outputPath)); + await fs.outputFile(outputPath, result); - ctx.logger.info(`Template file has been written to ${outputPath}`); + ctx.logger.info(`Template file has been written to ${outputPath}`); + } finally { + dispose(); + } } diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index 656427bf09..eb52d77cf8 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -25,8 +25,8 @@ import { import { JsonArray, JsonObject, JsonValue } from '@backstage/types'; import fs from 'fs-extra'; import { validate as validateJsonSchema } from 'jsonschema'; -import nunjucks from 'nunjucks'; import path from 'node:path'; +import nunjucks from 'nunjucks'; import * as winston from 'winston'; import { SecureTemplater, @@ -47,6 +47,7 @@ import type { } from '@backstage/backend-plugin-api'; import type { MetricsService } from '@backstage/backend-plugin-api/alpha'; import { UserEntity } from '@backstage/catalog-model'; +import { Config } from '@backstage/config'; import { AuthorizeResult, PolicyDecision, @@ -60,17 +61,16 @@ 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 { Config } from '@backstage/config'; import { resolveDefaultEnvironment } from '../../lib/defaultEnvironment'; +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; @@ -650,23 +650,24 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { // Track whether a status check global (always/failure) was invoked during rendering const statusCheckInvoked = { value: false }; - const renderTemplate = await SecureTemplater.loadRenderer({ - templateFilters: { - ...this.defaultTemplateFilters, - ...additionalTemplateFilters, - }, - templateGlobals: { - ...additionalTemplateGlobals, - always: () => { - statusCheckInvoked.value = true; - return true; + const { render: renderTemplate, dispose } = + await SecureTemplater.loadRenderer({ + templateFilters: { + ...this.defaultTemplateFilters, + ...additionalTemplateFilters, }, - failure: () => { - statusCheckInvoked.value = true; - return taskState.failed; + templateGlobals: { + ...additionalTemplateGlobals, + always: () => { + statusCheckInvoked.value = true; + return true; + }, + failure: () => { + statusCheckInvoked.value = true; + return taskState.failed; + }, }, - }, - }); + }); try { await task.rehydrateWorkspace?.({ taskId, targetPath: workspacePath }); @@ -785,6 +786,11 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { return { output }; } finally { + try { + dispose(); + } catch { + // Ignore disposal errors so they don't mask the original failure. + } if (workspacePath) { await fs.remove(workspacePath); }