diff --git a/.changeset/fix-scaffolder-secret-redaction-bypass.md b/.changeset/fix-scaffolder-secret-redaction-bypass.md new file mode 100644 index 0000000000..9771c6b80e --- /dev/null +++ b/.changeset/fix-scaffolder-secret-redaction-bypass.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': patch +--- + +Fixed a security vulnerability where secrets could bypass log redaction when transformed through Nunjucks filters in scaffolder templates. diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts index 530835e434..745d571f44 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts @@ -896,6 +896,405 @@ describe('NunjucksWorkflowRunner', () => { expectTaskLog('info: *** {"thing":"***"}'); }); + + // eslint-disable-next-line jest/expect-expect + it('should redact secrets that have been transformed with a replace filter', async () => { + actionRegistry.register({ + id: 'log-secret', + description: 'Mock action for testing', + supportsDryRun: true, + handler: async ctx => { + ctx.logger.info(ctx.input.secret); + }, + schema: { + input: { + type: 'object', + required: ['secret'], + properties: { + secret: { + type: 'string', + }, + }, + }, + }, + }); + + const task = createMockTaskWithSpec( + { + steps: [ + { + id: 'test', + name: 'name', + action: 'log-secret', + input: { + secret: "${{ secrets.backstageToken | replace('.', '_DOT_') }}", + }, + }, + ], + }, + { backstageToken: 'header.payload.signature' }, + ); + + await runner.execute(task); + + expectTaskLog('info: ***'); + }); + + // eslint-disable-next-line jest/expect-expect + it('should redact secrets transformed with the upper filter', async () => { + actionRegistry.register({ + id: 'log-secret', + description: 'Mock action for testing', + supportsDryRun: true, + handler: async ctx => { + ctx.logger.info(ctx.input.secret); + }, + schema: { + input: { + type: 'object', + required: ['secret'], + properties: { + secret: { + type: 'string', + }, + }, + }, + }, + }); + + const task = createMockTaskWithSpec( + { + steps: [ + { + id: 'test', + name: 'name', + action: 'log-secret', + input: { + secret: '${{ secrets.mySecret | upper }}', + }, + }, + ], + }, + { mySecret: 'super-secret-token' }, + ); + + await runner.execute(task); + + expectTaskLog('info: ***'); + }); + + // eslint-disable-next-line jest/expect-expect + it('should redact secrets embedded in a larger string with other text', async () => { + actionRegistry.register({ + id: 'log-secret', + description: 'Mock action for testing', + supportsDryRun: true, + handler: async ctx => { + ctx.logger.info(ctx.input.message); + }, + schema: { + input: { + type: 'object', + required: ['message'], + properties: { + message: { + type: 'string', + }, + }, + }, + }, + }); + + const task = createMockTaskWithSpec( + { + steps: [ + { + id: 'test', + name: 'name', + action: 'log-secret', + input: { + message: + "scaffold-init:${{ secrets.backstageToken | replace('.', '_DOT_') }}", + }, + }, + ], + }, + { backstageToken: 'header.payload.signature' }, + ); + + await runner.execute(task); + + expectTaskLog('info: ***'); + }); + + // eslint-disable-next-line jest/expect-expect + it('should redact environment secrets that have been transformed', async () => { + actionRegistry.register({ + id: 'log-secret', + description: 'Mock action for testing', + supportsDryRun: true, + handler: async ctx => { + ctx.logger.info(ctx.input.secret); + }, + schema: { + input: { + type: 'object', + required: ['secret'], + properties: { + secret: { + type: 'string', + }, + }, + }, + }, + }); + + const task = createMockTaskWithSpec( + { + steps: [ + { + id: 'test', + name: 'name', + action: 'log-secret', + input: { + secret: '${{ environment.secrets.AWS_ACCESS_KEY | upper }}', + }, + }, + ], + }, + {}, + ); + + await runner.execute(task); + + expectTaskLog('info: ***'); + }); + + it('should not redact non-secret values in rendered input', async () => { + actionRegistry.register({ + id: 'log-secret', + description: 'Mock action for testing', + supportsDryRun: true, + handler: async ctx => { + ctx.logger.info(ctx.input.name); + }, + schema: { + input: { + type: 'object', + required: ['name'], + properties: { + name: { + type: 'string', + }, + }, + }, + }, + }); + + const task = createMockTaskWithSpec({ + steps: [ + { + id: 'test', + name: 'name', + action: 'log-secret', + input: { + name: '${{ parameters.serviceName }}', + }, + }, + ], + parameters: { serviceName: 'my-service' }, + }); + + await runner.execute(task); + + expectTaskLog('info: my-service'); + }); + + // eslint-disable-next-line jest/expect-expect + it('should redact secrets in deeply nested input objects', async () => { + actionRegistry.register({ + id: 'log-secret', + description: 'Mock action for testing', + supportsDryRun: true, + handler: async ctx => { + ctx.logger.info(ctx.input.nested.deep.secret); + }, + schema: { + input: { + type: 'object', + required: ['nested'], + properties: { + nested: { + type: 'object', + properties: { + deep: { + type: 'object', + properties: { + secret: { type: 'string' }, + }, + }, + }, + }, + }, + }, + }, + }); + + const task = createMockTaskWithSpec( + { + steps: [ + { + id: 'test', + name: 'name', + action: 'log-secret', + input: { + nested: { + deep: { + secret: "${{ secrets.token | replace('.', '-') }}", + }, + }, + }, + }, + ], + }, + { token: 'aaa.bbb.ccc' }, + ); + + await runner.execute(task); + + expectTaskLog('info: ***'); + }); + + // eslint-disable-next-line jest/expect-expect + it('should redact secrets in arrays within input', async () => { + actionRegistry.register({ + id: 'log-secret', + description: 'Mock action for testing', + supportsDryRun: true, + handler: async ctx => { + ctx.logger.info(ctx.input.items[0]); + }, + schema: { + input: { + type: 'object', + required: ['items'], + properties: { + items: { + type: 'array', + items: { type: 'string' }, + }, + }, + }, + }, + }); + + const task = createMockTaskWithSpec( + { + steps: [ + { + id: 'test', + name: 'name', + action: 'log-secret', + input: { + items: ['${{ secrets.token | upper }}', 'not-a-secret'], + }, + }, + ], + }, + { token: 'my-secret' }, + ); + + await runner.execute(task); + + expectTaskLog('info: ***'); + }); + + // eslint-disable-next-line jest/expect-expect + it('should redact multiple different transformed secrets in the same step', async () => { + actionRegistry.register({ + id: 'log-secret', + description: 'Mock action for testing', + supportsDryRun: true, + handler: async ctx => { + ctx.logger.info(`${ctx.input.a} ${ctx.input.b}`); + }, + schema: { + input: { + type: 'object', + required: ['a', 'b'], + properties: { + a: { type: 'string' }, + b: { type: 'string' }, + }, + }, + }, + }); + + const task = createMockTaskWithSpec( + { + steps: [ + { + id: 'test', + name: 'name', + action: 'log-secret', + input: { + a: '${{ secrets.s1 | upper }}', + b: "${{ secrets.s2 | replace('.', '_') }}", + }, + }, + ], + }, + { s1: 'first-secret', s2: 'second.secret' }, + ); + + await runner.execute(task); + + expectTaskLog('info: *** ***'); + }); + + it('should still pass the correct transformed value to the action input', async () => { + actionRegistry.register({ + id: 'log-secret', + description: 'Mock action for testing', + supportsDryRun: true, + handler: fakeActionHandler, + schema: { + input: { + type: 'object', + required: ['secret'], + properties: { + secret: { + type: 'string', + }, + }, + }, + }, + }); + + const task = createMockTaskWithSpec( + { + steps: [ + { + id: 'test', + name: 'name', + action: 'log-secret', + input: { + secret: "${{ secrets.backstageToken | replace('.', '_DOT_') }}", + }, + }, + ], + }, + { backstageToken: 'header.payload.signature' }, + ); + + await runner.execute(task); + + expect(fakeActionHandler).toHaveBeenCalledWith( + expect.objectContaining({ + input: { secret: 'header_DOT_payload_DOT_signature' }, + }), + ); + }); }); describe('each', () => { diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index 0700c58bf8..5c4cbd6730 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -129,6 +129,53 @@ const createStepLogger = ({ return { taskLogger }; }; +/** + * Recursively compares two rendered objects and returns string values from + * `withSecrets` that differ from their counterpart in `withoutSecrets`. + * These are values that were influenced by secret interpolation and should + * be added as log redactions. + */ +function collectSecretRedactions( + withSecrets: unknown, + withoutSecrets: unknown, +): string[] { + if (typeof withSecrets === 'string') { + return withSecrets !== withoutSecrets ? [withSecrets] : []; + } + if (Array.isArray(withSecrets)) { + const other = Array.isArray(withoutSecrets) ? withoutSecrets : []; + return withSecrets.flatMap((val, i) => + collectSecretRedactions(val, other[i]), + ); + } + if (withSecrets && typeof withSecrets === 'object') { + const other = + withoutSecrets && typeof withoutSecrets === 'object' + ? (withoutSecrets as Record) + : {}; + return Object.entries(withSecrets as Record).flatMap( + ([key, val]) => collectSecretRedactions(val, other[key]), + ); + } + return []; +} + +/** + * Extracts all string values from a nested object structure. + * Used as a fallback when the comparison render fails. + */ +function extractStringValues(obj: unknown): string[] { + if (typeof obj === 'string') return [obj]; + if (Array.isArray(obj)) return obj.flatMap(extractStringValues); + if (obj && typeof obj === 'object') { + return Object.entries(obj).flatMap(([key, val]) => [ + key, + ...extractStringValues(val), + ]); + } + return []; +} + const isActionAuthorized = createConditionAuthorizer( Object.values(scaffolderActionRules), ); @@ -445,6 +492,39 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { ); } + // Redact any rendered values that were influenced by secrets. + // Re-render the input without secrets and diff against the real render + // to find values that changed due to secret interpolation. + if (step.input) { + const hasSecrets = + Object.keys(task.secrets ?? {}).length > 0 || + Object.keys(this.environment?.secrets ?? {}).length > 0; + + if (hasSecrets) { + try { + const contextNoSecrets = { + ...preIterationContext, + ...(iteration.each ? { each: iteration.each } : {}), + secrets: {}, + environment: { + ...preIterationContext.environment, + secrets: {}, + }, + }; + const inputWithoutSecrets = this.render( + step.input, + contextNoSecrets, + renderTemplate, + ); + taskLogger.addRedactions( + collectSecretRedactions(iteration.input, inputWithoutSecrets), + ); + } catch { + taskLogger.addRedactions(extractStringValues(iteration.input)); + } + } + } + await action.handler({ input: iteration.input, task: {