paulononato/backstage
1
0
Fork 0
Code Issues Pull Requests Actions 44 Packages Projects Releases Wiki Activity

Merge pull request #33991 from jtbry/master

Browse Source 
fix(SecureTemplater): return dispose function to clean up secure temp…
This commit is contained in:
Fredrik Adelöw
2026-05-07 17:15:23 +03:00
committed by GitHub
parent 90d9e05133 964bc8694f
commit 213c6807d9
6 changed files with 199 additions and 129 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.changeset/bitter-files-flash.md
+5
View File
@@ -0,0 +1,5 @@
---
'@backstage/plugin-scaffolder-backend': major
---
Add explicit memory management to SecureTemplater usage
plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts
+50 -19
View File
@@ -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();
});
});
plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts
+20 -5
View File
@@ -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
}
}
},
};
}
}
plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateActionHandler.ts
+74 -66
View File
@@ -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}`);
}
plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateFileActionHandler.ts
+22 -17
View File
@@ -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();
}
}
plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts
+28 -22
View File
@@ -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);
}