From 6a73fff696b013b18be8570c56944148bb624591 Mon Sep 17 00:00:00 2001 From: Justin Bryant Date: Fri, 17 Apr 2026 17:25:50 -0400 Subject: [PATCH] fix(SecureTemplater): implement PR suggestions Signed-off-by: Justin Bryant --- .changeset/bitter-files-flash.md | 2 +- .../src/lib/templating/SecureTemplater.ts | 12 +- .../builtin/fetch/templateActionHandler.ts | 122 +++++++++--------- 3 files changed, 73 insertions(+), 63 deletions(-) diff --git a/.changeset/bitter-files-flash.md b/.changeset/bitter-files-flash.md index 1e0985c39c..9b7ab014a5 100644 --- a/.changeset/bitter-files-flash.md +++ b/.changeset/bitter-files-flash.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-scaffolder-backend': minor +'@backstage/plugin-scaffolder-backend': major --- Add explicit memory management to SecureTemplater usage diff --git a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts index 71a7a14e60..e230dba184 100644 --- a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts +++ b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts @@ -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)); @@ -219,8 +221,10 @@ export class SecureTemplater { return { render, dispose: () => { - context.release(); - isolate.dispose(); + if (context && !isolate.isDisposed) { + 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 9ee580e324..a8a4dbafd7 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateActionHandler.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/templateActionHandler.ts @@ -117,77 +117,83 @@ export async function createTemplateActionHandler< 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(); } - dispose(); ctx.logger.info(`Template result written to ${outputDir}`); }