Merge pull request #25154 from backstage/blam/scaffolder-redact

scaffolder: Fixing issues with redaction formatting

.changeset/neat-chairs-complain.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-scaffolder-backend': patch
+---
+
+Fixing issues with log redaction in the scaffolder logs

plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts

@@ -644,6 +644,100 @@ describe('NunjucksWorkflowRunner', () => {
     });
   });
 
+  describe('redactions', () => {
+    // eslint-disable-next-line jest/expect-expect
+    it('should redact secrets that are passed with the task', 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(
+        {
+          apiVersion: 'scaffolder.backstage.io/v1beta3',
+          parameters: {},
+          output: {},
+          steps: [
+            {
+              id: 'test',
+              name: 'name',
+              action: 'log-secret',
+              input: {
+                secret: '${{ secrets.secret }}',
+              },
+            },
+          ],
+        },
+        { secret: 'my-secret-value' },
+      );
+
+      await runner.execute(task);
+
+      expectTaskLog('info: ***');
+    });
+
+    // eslint-disable-next-line jest/expect-expect
+    it('should redact meta fields properly', async () => {
+      actionRegistry.register({
+        id: 'log-secret',
+        description: 'Mock action for testing',
+        supportsDryRun: true,
+        handler: async ctx => {
+          ctx.logger.child({ thing: ctx.input.secret }).info(ctx.input.secret);
+        },
+        schema: {
+          input: {
+            type: 'object',
+            required: ['secret'],
+            properties: {
+              secret: {
+                type: 'string',
+              },
+            },
+          },
+        },
+      });
+
+      const task = createMockTaskWithSpec(
+        {
+          apiVersion: 'scaffolder.backstage.io/v1beta3',
+          parameters: {},
+          output: {},
+          steps: [
+            {
+              id: 'test',
+              name: 'name',
+              action: 'log-secret',
+              input: {
+                secret: '${{ secrets.secret }}',
+              },
+            },
+          ],
+        },
+        { secret: 'my-secret-value' },
+      );
+
+      await runner.execute(task);
+
+      expectTaskLog('info: *** {"thing":"***"}');
+    });
+  });
+
   describe('each', () => {
     it('should run a step repeatedly - flat values', async () => {
       const colors = ['blue', 'green', 'red'];

plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts

@@ -104,24 +104,13 @@ const createStepLogger = ({
   step: TaskStep;
   rootLogger: winston.Logger;
 }) => {
-  const stepLogStream = new PassThrough();
-  stepLogStream.on('data', async data => {
-    const message = data.toString().trim();
-    if (message?.length > 1) {
-      await task.emitLog(message, { stepId: step.id });
-    }
-  });
-
   const taskLogger = WinstonLogger.create({
     level: process.env.LOG_LEVEL || 'info',
     format: winston.format.combine(
       winston.format.colorize(),
       winston.format.simple(),
     ),
-    transports: [
-      new winston.transports.Stream({ stream: stepLogStream }),
-      new BackstageLoggerTransport(rootLogger),
-    ],
+    transports: [new BackstageLoggerTransport(rootLogger, task, step.id)],
   });
 
   taskLogger.addRedactions(Object.values(task.secrets ?? {}));

plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts

@@ -21,7 +21,9 @@ import { JsonObject } from '@backstage/types';
 import { Format, TransformableInfo } from 'logform';
 import Transport, { TransportStreamOptions } from 'winston-transport';
 import { Logger, format, createLogger, transports } from 'winston';
-import { MESSAGE } from 'triple-beam';
+import { LEVEL, MESSAGE, SPLAT } from 'triple-beam';
+import { TaskContext } from '@backstage/plugin-scaffolder-node';
+import _ from 'lodash';
 
 /**
  * Escapes a given string to be used inside a RegExp.
@@ -44,33 +46,41 @@ interface WinstonLoggerOptions {
 export class BackstageLoggerTransport extends Transport {
   constructor(
     private readonly backstageLogger: LoggerService,
+    private readonly taskContext: TaskContext,
+    private readonly stepId: string,
     opts?: TransportStreamOptions,
   ) {
     super(opts);
   }
 
-  log(info: unknown, callback: VoidFunction) {
+  log(info: TransformableInfo, callback: VoidFunction) {
     if (typeof info !== 'object' || info === null) {
       callback();
       return;
     }
-    const { level, message, ...meta } = info as JsonObject;
+
+    const message = info[MESSAGE];
+    const level = info[LEVEL];
+    const splat = info[SPLAT];
+
     switch (level) {
       case 'error':
-        this.backstageLogger.error(String(message), meta);
+        this.backstageLogger.error(String(message), ...splat);
         break;
       case 'warn':
-        this.backstageLogger.warn(String(message), meta);
+        this.backstageLogger.warn(String(message), ...splat);
         break;
       case 'info':
-        this.backstageLogger.info(String(message), meta);
+        this.backstageLogger.info(String(message), ...splat);
         break;
       case 'debug':
-        this.backstageLogger.debug(String(message), meta);
+        this.backstageLogger.debug(String(message), ...splat);
         break;
       default:
-        this.backstageLogger.info(String(message), meta);
+        this.backstageLogger.info(String(message), ...splat);
     }
+
+    this.taskContext.emitLog(message, { stepId: this.stepId });
     callback();
   }
 }
@@ -87,9 +97,10 @@ export class WinstonLogger implements RootLoggerService {
 
     let logger = createLogger({
       level: options.level,
-      format: format.combine(redacter.format, options.format),
+      format: format.combine(options.format, redacter.format),
       transports: options.transports ?? new transports.Console(),
     });
+
     if (options.meta) {
       logger = logger.child(options.meta);
     }
@@ -163,7 +174,10 @@ export class WinstonLogger implements RootLoggerService {
         },
       }),
       format.printf((info: TransformableInfo) => {
-        const { timestamp, level, message, plugin, service, ...fields } = info;
+        const { timestamp, plugin, service } = info;
+        const message = info[MESSAGE];
+        const level = info[LEVEL];
+        const fields = info[SPLAT];
         const prefix = plugin || service;
         const timestampColor = colorizer.colorize('timestamp', timestamp);
         const prefixColor = colorizer.colorize('prefix', prefix);