From e3c38284bfa723ea2956995e1cf07f02f4cf3665 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 24 Mar 2023 20:41:40 +0100 Subject: [PATCH 1/4] chore: reworking how the secure templater works a little bit Signed-off-by: blam --- .../lib/templating/SecureTemplater.test.ts | 24 +++++---- .../src/lib/templating/SecureTemplater.ts | 52 +++++-------------- .../src/lib/templating/filters.ts | 48 +++++++++++++++++ .../tasks/NunjucksWorkflowRunner.test.ts | 52 +++++++++++++++++++ .../tasks/NunjucksWorkflowRunner.ts | 28 +++++----- 5 files changed, 141 insertions(+), 63 deletions(-) create mode 100644 plugins/scaffolder-backend/src/lib/templating/filters.ts diff --git a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts index 76643131f5..051096db57 100644 --- a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts +++ b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.test.ts @@ -69,7 +69,12 @@ describe('SecureTemplater', () => { owner: 'my-owner', host: 'my-host.com', })); - const renderWith = await SecureTemplater.loadRenderer({ parseRepoUrl }); + + const projectSlug = jest.fn(() => 'my-owner/my-repo'); + + const renderWith = await SecureTemplater.loadRenderer({ + templateFilters: { parseRepoUrl, projectSlug }, + }); const renderWithout = await SecureTemplater.loadRenderer(); const ctx = { @@ -95,7 +100,6 @@ describe('SecureTemplater', () => { expect(parseRepoUrl.mock.calls).toEqual([ ['https://my-host.com/my-owner/my-repo'], - ['https://my-host.com/my-owner/my-repo'], ]); }); @@ -104,7 +108,7 @@ describe('SecureTemplater', () => { const mockFilter2 = jest.fn((var1, var2) => `${var1} ${var2}`); const mockFilter3 = jest.fn((var1, var2) => ({ var1, var2 })); const renderWith = await SecureTemplater.loadRenderer({ - additionalTemplateFilters: { mockFilter1, mockFilter2, mockFilter3 }, + templateFilters: { mockFilter1, mockFilter2, mockFilter3 }, }); const renderWithout = await SecureTemplater.loadRenderer(); @@ -149,7 +153,7 @@ describe('SecureTemplater', () => { const mockGlobal2 = 'foo'; const mockGlobal3 = 123456; const renderWith = await SecureTemplater.loadRenderer({ - additionalTemplateGlobals: { mockGlobal1, mockGlobal2, mockGlobal3 }, + templateGlobals: { mockGlobal1, mockGlobal2, mockGlobal3 }, }); const renderWithout = await SecureTemplater.loadRenderer(); @@ -168,11 +172,13 @@ describe('SecureTemplater', () => { it('should not allow helpers to be rewritten', async () => { const render = await SecureTemplater.loadRenderer({ - parseRepoUrl: () => ({ - repo: 'my-repo', - owner: 'my-owner', - host: 'my-host.com', - }), + templateFilters: { + parseRepoUrl: () => ({ + repo: 'my-repo', + owner: 'my-owner', + host: 'my-host.com', + }), + }, }); const ctx = { diff --git a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts index 7dc42658be..f0ccb6f192 100644 --- a/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts +++ b/plugins/scaffolder-backend/src/lib/templating/SecureTemplater.ts @@ -18,7 +18,6 @@ import { VM } from 'vm2'; import { resolvePackagePath } from '@backstage/backend-common'; import fs from 'fs-extra'; import { JsonValue } from '@backstage/types'; -import { RepoSpec } from '../../scaffolder/actions/builtin/publish/util'; // language=JavaScript const mkScript = (nunjucksSource: string) => ` @@ -46,26 +45,14 @@ const { render, renderCompat } = (() => { }); compatEnv.addFilter('jsonify', compatEnv.getFilter('dump')); - if (typeof parseRepoUrl !== 'undefined') { - const safeHelperRef = parseRepoUrl; - - env.addFilter('parseRepoUrl', repoUrl => { - return JSON.parse(safeHelperRef(repoUrl)) - }); - env.addFilter('projectSlug', repoUrl => { - const { owner, repo } = JSON.parse(safeHelperRef(repoUrl)); - return owner + '/' + repo; - }); - } - - if (typeof additionalTemplateFilters !== 'undefined') { - for (const [filterName, filterFn] of Object.entries(additionalTemplateFilters)) { + if (typeof templateFilters !== 'undefined') { + for (const [filterName, filterFn] of Object.entries(templateFilters)) { env.addFilter(filterName, (...args) => JSON.parse(filterFn(...args))); } } - if (typeof additionalTemplateGlobals !== 'undefined') { - for (const [globalName, global] of Object.entries(additionalTemplateGlobals)) { + if (typeof templateGlobals !== 'undefined') { + for (const [globalName, global] of Object.entries(templateGlobals)) { if (typeof global === 'function') { env.addGlobal(globalName, (...args) => JSON.parse(global(...args))); } else { @@ -114,16 +101,12 @@ export type TemplateGlobal = | JsonValue; export interface SecureTemplaterOptions { - /* Optional implementation of the parseRepoUrl filter */ - parseRepoUrl?(repoUrl: string): RepoSpec; - /* Enables jinja compatibility and the "jsonify" filter */ cookiecutterCompat?: boolean; - /* Extra user-provided nunjucks filters */ - additionalTemplateFilters?: Record; + templateFilters?: Record; /* Extra user-provided nunjucks globals */ - additionalTemplateGlobals?: Record; + templateGlobals?: Record; } export type SecureTemplateRenderer = ( @@ -133,21 +116,12 @@ export type SecureTemplateRenderer = ( export class SecureTemplater { static async loadRenderer(options: SecureTemplaterOptions = {}) { - const { - parseRepoUrl, - cookiecutterCompat, - additionalTemplateFilters, - additionalTemplateGlobals, - } = options; + const { cookiecutterCompat, templateFilters, templateGlobals } = options; const sandbox: Record = {}; - if (parseRepoUrl) { - sandbox.parseRepoUrl = (url: string) => JSON.stringify(parseRepoUrl(url)); - } - - if (additionalTemplateFilters) { - sandbox.additionalTemplateFilters = Object.fromEntries( - Object.entries(additionalTemplateFilters) + if (templateFilters) { + sandbox.templateFilters = Object.fromEntries( + Object.entries(templateFilters) .filter(([_, filterFunction]) => !!filterFunction) .map(([filterName, filterFunction]) => [ filterName, @@ -155,9 +129,9 @@ export class SecureTemplater { ]), ); } - if (additionalTemplateGlobals) { - sandbox.additionalTemplateGlobals = Object.fromEntries( - Object.entries(additionalTemplateGlobals) + if (templateGlobals) { + sandbox.templateGlobals = Object.fromEntries( + Object.entries(templateGlobals) .filter(([_, global]) => !!global) .map(([globalName, global]) => { if (typeof global === 'function') { diff --git a/plugins/scaffolder-backend/src/lib/templating/filters.ts b/plugins/scaffolder-backend/src/lib/templating/filters.ts new file mode 100644 index 0000000000..a8d7956c3c --- /dev/null +++ b/plugins/scaffolder-backend/src/lib/templating/filters.ts @@ -0,0 +1,48 @@ +/* + * Copyright 2023 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { parseEntityRef } from '@backstage/catalog-model'; +import { ScmIntegrations } from '@backstage/integration'; +import { JsonValue } from '@backstage/types'; +import { TemplateFilter } from '..'; +import { parseRepoUrl } from '../../scaffolder/actions/builtin/publish/util'; + +export const createDefaultFilters = ({ + integrations, +}: { + integrations: ScmIntegrations; +}): Record => { + return { + parseRepoUrl: url => parseRepoUrl(url as string, integrations), + parseEntityRef: ref => parseEntityRef(ref as string), + pick: (obj: JsonValue, key: JsonValue) => { + if ( + typeof obj === 'object' && + !Array.isArray(obj) && + typeof key === 'string' + ) { + return obj?.[key]; + } + + throw new Error( + `Invalid arguments to pick filter, expected object and string, got ${typeof obj} and ${typeof key}`, + ); + }, + projectSlug: repoUrl => { + const { owner, repo } = parseRepoUrl(repoUrl as string, integrations); + return `${owner}/${repo}`; + }, + }; +}; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts index 3b6ec54249..fa012ea04f 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts @@ -698,6 +698,58 @@ describe('DefaultWorkflowRunner', () => { repo: 'repo', }); }); + + it('provides the parseEntityRef filter', async () => { + const task = createMockTaskWithSpec({ + apiVersion: 'scaffolder.backstage.io/v1beta3', + steps: [ + { + id: 'test', + name: 'name', + action: 'output-action', + input: {}, + }, + ], + output: { + foo: '${{ parameters.entity | parseEntityRef }}', + }, + parameters: { + entity: 'component:default/ben', + }, + }); + + const { output } = await runner.execute(task); + + expect(output.foo).toEqual({ + kind: 'component', + namespace: 'default', + name: 'ben', + }); + }); + + it('provides the pick filter', async () => { + const task = createMockTaskWithSpec({ + apiVersion: 'scaffolder.backstage.io/v1beta3', + steps: [ + { + id: 'test', + name: 'name', + action: 'output-action', + input: {}, + }, + ], + output: { + foo: '${{ parameters.entity | parseEntityRef | pick("kind") }}', + }, + parameters: { + entity: 'component:default/ben', + }, + }); + + const { output } = await runner.execute(task); + + expect(output.foo).toEqual('component'); + }); }); describe('dry run', () => { diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index 629c730d48..9358484642 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -30,7 +30,6 @@ import { InputError } from '@backstage/errors'; import { PassThrough } from 'stream'; import { generateExampleOutput, isTruthy } from './helper'; import { validate as validateJsonSchema } from 'jsonschema'; -import { parseRepoUrl } from '../actions/builtin/publish/util'; import { TemplateActionRegistry } from '../actions'; import { TemplateFilter, @@ -46,6 +45,7 @@ import { import { TemplateAction } from '@backstage/plugin-scaffolder-node'; import { UserEntity } from '@backstage/catalog-model'; import { createCounterMetric, createHistogramMetric } from '../../util/metrics'; +import { createDefaultFilters } from '../../lib/templating/filters'; type NunjucksWorkflowRunnerOptions = { workingDirectory: string; @@ -103,7 +103,12 @@ const createStepLogger = ({ }; export class NunjucksWorkflowRunner implements WorkflowRunner { - constructor(private readonly options: NunjucksWorkflowRunnerOptions) {} + private readonly defaultTemplateFilters: Record; + constructor(private readonly options: NunjucksWorkflowRunnerOptions) { + this.defaultTemplateFilters = createDefaultFilters({ + integrations: this.options.integrations, + }); + } private readonly tracker = scaffoldingTracker(); @@ -329,22 +334,15 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { await task.getWorkspaceName(), ); - const { - additionalTemplateFilters, - additionalTemplateGlobals, - integrations, - } = this.options; + const { additionalTemplateFilters, additionalTemplateGlobals } = + 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); + templateFilters: { + ...this.defaultTemplateFilters, + ...additionalTemplateFilters, }, - additionalTemplateFilters, - additionalTemplateGlobals, + templateGlobals: additionalTemplateGlobals, }); try { From 3d6982b7499ae90c15f9299ea0dff38a7ac37f08 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 24 Mar 2023 20:43:37 +0100 Subject: [PATCH 2/4] chore: fixing old bug of filter not found for parseRepoUrl in fetch:template Signed-off-by: blam --- .../src/scaffolder/actions/builtin/fetch/template.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) 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 68c205ce59..5e1e77e0e2 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts @@ -28,6 +28,7 @@ import { SecureTemplater, TemplateGlobal, } from '../../../../lib/templating/SecureTemplater'; +import { createDefaultFilters } from '../../../../lib/templating/filters'; /** * Downloads a skeleton, templates variables into file and directory names and content. @@ -49,6 +50,11 @@ export function createFetchTemplateAction(options: { additionalTemplateGlobals, } = options; + const templateFilters = { + ...createDefaultFilters({ integrations }), + ...additionalTemplateFilters, + }; + return createTemplateAction<{ url: string; targetPath?: string; @@ -231,8 +237,8 @@ export function createFetchTemplateAction(options: { const renderTemplate = await SecureTemplater.loadRenderer({ cookiecutterCompat: ctx.input.cookiecutterCompat, - additionalTemplateFilters, - additionalTemplateGlobals, + templateFilters, + templateGlobals: additionalTemplateGlobals, }); for (const location of allEntriesInTemplate) { From 95ea9f69b6fb2467a69fb5ce450cdb47ca665eb2 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 24 Mar 2023 20:52:00 +0100 Subject: [PATCH 3/4] chore: added a changeset Signed-off-by: blam --- .changeset/tough-cameras-beam.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 .changeset/tough-cameras-beam.md diff --git a/.changeset/tough-cameras-beam.md b/.changeset/tough-cameras-beam.md new file mode 100644 index 0000000000..c599f168b0 --- /dev/null +++ b/.changeset/tough-cameras-beam.md @@ -0,0 +1,10 @@ +--- +'@backstage/plugin-scaffolder-backend': minor +--- + +Provide some more default filters out of the box and refactoring how the filters are applied to the `SecureTemplater`. + +- `parseEntityRef` will take an string entity triplet and return a parsed object. +- `pick` will allow you to reference a specific property in the piped object. + +So you can now combine things like this: `${{ parameters.entity | parseEntityRef | pick('name') }}` to get the name of a specific entity, or `${{ parameters.repoUrl | parseRepoUrl | pick('owner) }}` to get the owner of a repo. From d425754cc177e736c6c3e7c92187dbbe43a1a0b2 Mon Sep 17 00:00:00 2001 From: blam Date: Tue, 28 Mar 2023 13:19:15 +0200 Subject: [PATCH 4/4] chore: simplifying the code a bit by just using lodash under the hood for the pick method Signed-off-by: blam --- .changeset/tough-cameras-beam.md | 2 +- .../src/lib/templating/filters.ts | 15 ++-------- .../actions/builtin/fetch/template.ts | 10 +++---- .../tasks/NunjucksWorkflowRunner.test.ts | 30 +++++++++++++++++++ 4 files changed, 38 insertions(+), 19 deletions(-) diff --git a/.changeset/tough-cameras-beam.md b/.changeset/tough-cameras-beam.md index c599f168b0..e1bcc87dc4 100644 --- a/.changeset/tough-cameras-beam.md +++ b/.changeset/tough-cameras-beam.md @@ -7,4 +7,4 @@ Provide some more default filters out of the box and refactoring how the filters - `parseEntityRef` will take an string entity triplet and return a parsed object. - `pick` will allow you to reference a specific property in the piped object. -So you can now combine things like this: `${{ parameters.entity | parseEntityRef | pick('name') }}` to get the name of a specific entity, or `${{ parameters.repoUrl | parseRepoUrl | pick('owner) }}` to get the owner of a repo. +So you can now combine things like this: `${{ parameters.entity | parseEntityRef | pick('name') }}` to get the name of a specific entity, or `${{ parameters.repoUrl | parseRepoUrl | pick('owner') }}` to get the owner of a repo. diff --git a/plugins/scaffolder-backend/src/lib/templating/filters.ts b/plugins/scaffolder-backend/src/lib/templating/filters.ts index a8d7956c3c..204d0015d8 100644 --- a/plugins/scaffolder-backend/src/lib/templating/filters.ts +++ b/plugins/scaffolder-backend/src/lib/templating/filters.ts @@ -18,6 +18,7 @@ import { ScmIntegrations } from '@backstage/integration'; import { JsonValue } from '@backstage/types'; import { TemplateFilter } from '..'; import { parseRepoUrl } from '../../scaffolder/actions/builtin/publish/util'; +import get from 'lodash/get'; export const createDefaultFilters = ({ integrations, @@ -27,19 +28,7 @@ export const createDefaultFilters = ({ return { parseRepoUrl: url => parseRepoUrl(url as string, integrations), parseEntityRef: ref => parseEntityRef(ref as string), - pick: (obj: JsonValue, key: JsonValue) => { - if ( - typeof obj === 'object' && - !Array.isArray(obj) && - typeof key === 'string' - ) { - return obj?.[key]; - } - - throw new Error( - `Invalid arguments to pick filter, expected object and string, got ${typeof obj} and ${typeof key}`, - ); - }, + pick: (obj: JsonValue, key: JsonValue) => get(obj, key as string), projectSlug: repoUrl => { const { owner, repo } = parseRepoUrl(repoUrl as string, integrations); return `${owner}/${repo}`; 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 5e1e77e0e2..7659cff240 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts @@ -50,10 +50,7 @@ export function createFetchTemplateAction(options: { additionalTemplateGlobals, } = options; - const templateFilters = { - ...createDefaultFilters({ integrations }), - ...additionalTemplateFilters, - }; + const defaultTemplateFilters = createDefaultFilters({ integrations }); return createTemplateAction<{ url: string; @@ -237,7 +234,10 @@ export function createFetchTemplateAction(options: { const renderTemplate = await SecureTemplater.loadRenderer({ cookiecutterCompat: ctx.input.cookiecutterCompat, - templateFilters, + templateFilters: { + ...defaultTemplateFilters, + ...additionalTemplateFilters, + }, templateGlobals: additionalTemplateGlobals, }); diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts index fa012ea04f..e8698b62f5 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts @@ -750,6 +750,36 @@ describe('DefaultWorkflowRunner', () => { expect(output.foo).toEqual('component'); }); + + it('should allow deep nesting of picked objects', async () => { + const task = createMockTaskWithSpec({ + apiVersion: 'scaffolder.backstage.io/v1beta3', + steps: [ + { + id: 'test', + name: 'name', + action: 'output-action', + input: {}, + }, + ], + output: { + foo: '${{ parameters.entity | pick("something.deeply.nested") }}', + }, + parameters: { + entity: { + something: { + deeply: { + nested: 'component', + }, + }, + }, + }, + }); + + const { output } = await runner.execute(task); + + expect(output.foo).toEqual('component'); + }); }); describe('dry run', () => {