Merge pull request #30396 from veenarm/fix-auditor-length

fix: Scaffolder Auditor length issue with file uploads or very long task parameters

.changeset/three-mammals-move.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-scaffolder-backend': minor
+---
+
+Implement max length for scaffolder auditor audit logging with default of 256

app-config.yaml

@@ -199,6 +199,8 @@ catalog:
         - allow: [Template]
 
 scaffolder:
+  auditor:
+    taskParameterMaxLength: 256
   # Use to customize default commit author info used when new components are created
   defaultAuthor:
     name: Scaffolder

plugins/scaffolder-backend/config.d.ts

@@ -93,5 +93,20 @@ export interface Config {
      * Default value is 24 hours.
      */
     taskTimeout?: HumanDuration | string;
+
+    /**
+     * Sets the maximum length for task parameters recorded by the auditor.
+     *
+     * If set to -1, the limit is disabled and parameters are not truncated.
+     * Defaults to 256 character length.
+     *
+     * @example
+     * scaffolder:
+     *   auditor:
+     *     taskParameterMaxLength: 512
+     */
+    auditor?: {
+      taskParameterMaxLength?: number;
+    };
   };
 }

plugins/scaffolder-backend/report.api.md

@@ -275,6 +275,7 @@ export type CreateWorkerOptions = {
   workingDirectory: string;
   logger: LoggerService;
   auditor?: AuditorService;
+  config?: Config;
   additionalTemplateFilters?: Record<string, TemplateFilter>;
   concurrentTasksLimit?: number;
   additionalTemplateGlobals?: Record<string, TemplateGlobal>;

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

@@ -19,7 +19,11 @@ import { DatabaseManager } from '@backstage/backend-defaults/database';
 import { ConfigReader } from '@backstage/config';
 import { DatabaseTaskStore } from './DatabaseTaskStore';
 import { StorageTaskBroker } from './StorageTaskBroker';
-import { TaskWorker, TaskWorkerOptions } from './TaskWorker';
+import {
+  createParameterTruncator,
+  TaskWorker,
+  TaskWorkerOptions,
+} from './TaskWorker';
 import { ScmIntegrations } from '@backstage/integration';
 import { TemplateActionRegistry } from '../actions';
 import { NunjucksWorkflowRunner } from './NunjucksWorkflowRunner';
@@ -140,6 +144,66 @@ describe('TaskWorker', () => {
     const event = events.find(e => e.type === 'completion');
     expect(event?.body.output).toEqual({ testOutput: 'testmockoutput' });
   });
+
+  it('should log an audit event with task parameters when running a task', async () => {
+    (workflowRunner.execute as jest.Mock).mockResolvedValue({
+      output: {},
+    });
+
+    const auditor = mockServices.auditor.mock();
+    const auditEvent = {
+      success: jest.fn(),
+      fail: jest.fn(),
+    };
+    auditor.createEvent.mockResolvedValue(auditEvent);
+
+    const broker = new StorageTaskBroker(storage, logger);
+    const taskWorker = await TaskWorker.create({
+      logger,
+      workingDirectory,
+      integrations,
+      taskBroker: broker,
+      actionRegistry,
+      auditor,
+      config: mockServices.rootConfig({
+        data: {
+          scaffolder: {
+            auditor: {
+              taskParameterMaxLength: 5,
+            },
+          },
+        },
+      }),
+    });
+
+    await taskWorker.runOneTask({
+      spec: {
+        apiVersion: 'scaffolder.backstage.io/v1beta3',
+        parameters: {
+          test: 'thisisaverylongstring',
+        },
+        steps: [],
+        output: {},
+      },
+      complete: jest.fn(),
+      createdBy: 'test-creator',
+      taskId: 'test-id',
+    } as unknown as TaskContext);
+
+    expect(auditor.createEvent).toHaveBeenCalledWith({
+      eventId: 'task',
+      severityLevel: 'medium',
+      meta: {
+        actionType: 'execution',
+        createdBy: 'test-creator',
+        taskId: 'test-id',
+        taskParameters: {
+          test: 'thisi...<truncated>',
+        },
+      },
+    });
+    expect(auditEvent.success).toHaveBeenCalled();
+  });
 });
 
 describe('Concurrent TaskWorker', () => {
@@ -343,3 +407,111 @@ describe('TaskWorker internals', () => {
     expect(inflightTasks.length).toBe(2);
   });
 });
+
+describe('createParameterTruncator', () => {
+  it('successfully does nothing', async () => {
+    const testParams = {};
+
+    const result = createParameterTruncator()(testParams);
+
+    expect(result).toEqual({});
+  });
+
+  it('truncates long strings in nested objects and arrays', async () => {
+    const params = {
+      test: 'short',
+      test2: 'thisisaverylongstring',
+      nested: {
+        test3: 'anotherlongstringhere',
+        test4: ['ok', 'toolongstring', { prop: 'thisisaverylongstring' }],
+      },
+    };
+
+    const result = createParameterTruncator(
+      mockServices.rootConfig({
+        data: {
+          scaffolder: {
+            auditor: {
+              taskParameterMaxLength: 5,
+            },
+          },
+        },
+      }),
+    )(params);
+
+    expect(result).toEqual({
+      test: 'short',
+      test2: 'thisi...<truncated>',
+      nested: {
+        test3: 'anoth...<truncated>',
+        test4: ['ok', 'toolo...<truncated>', { prop: 'thisi...<truncated>' }],
+      },
+    });
+  });
+
+  it('should not truncate if max length is -1', async () => {
+    const params = {
+      test: 'short',
+      test2: 'thisisaverylongstring',
+      nested: {
+        test3: 'anotherlongstringhere',
+        test4: ['ok', 'toolongstring', { prop: 'thisisaverylongstring' }],
+      },
+    };
+
+    const result = createParameterTruncator(
+      mockServices.rootConfig({
+        data: {
+          scaffolder: {
+            auditor: {
+              taskParameterMaxLength: -1,
+            },
+          },
+        },
+      }),
+    )(params);
+
+    expect(result).toEqual({
+      test: 'short',
+      test2: 'thisisaverylongstring',
+      nested: {
+        test3: 'anotherlongstringhere',
+        test4: ['ok', 'toolongstring', { prop: 'thisisaverylongstring' }],
+      },
+    });
+  });
+
+  it('should throw on invalid max length', async () => {
+    expect(() =>
+      createParameterTruncator(
+        mockServices.rootConfig({
+          data: {
+            scaffolder: {
+              auditor: {
+                taskParameterMaxLength: -2,
+              },
+            },
+          },
+        }),
+      ),
+    ).toThrowErrorMatchingInlineSnapshot(
+      `"Invalid configuration for 'scaffolder.auditor.taskParameterMaxLength', got -2. Must be a positive integer or -1 to disable truncation."`,
+    );
+
+    expect(() =>
+      createParameterTruncator(
+        mockServices.rootConfig({
+          data: {
+            scaffolder: {
+              auditor: {
+                taskParameterMaxLength: 1.5,
+              },
+            },
+          },
+        }),
+      ),
+    ).toThrowErrorMatchingInlineSnapshot(
+      `"Invalid configuration for 'scaffolder.auditor.taskParameterMaxLength', got 1.5. Must be a positive integer or -1 to disable truncation."`,
+    );
+  });
+});

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

@@ -15,7 +15,7 @@
  */
 
 import { AuditorService, LoggerService } from '@backstage/backend-plugin-api';
-import { assertError, stringifyError } from '@backstage/errors';
+import { assertError, InputError, stringifyError } from '@backstage/errors';
 import { ScmIntegrations } from '@backstage/integration';
 import { PermissionEvaluator } from '@backstage/plugin-permission-common';
 import {
@@ -29,6 +29,10 @@ import { TemplateActionRegistry } from '../actions';
 import { NunjucksWorkflowRunner } from './NunjucksWorkflowRunner';
 import { WorkflowRunner } from './types';
 import { setTimeout } from 'timers/promises';
+import { JsonObject } from '@backstage/types';
+import { Config } from '@backstage/config';
+
+const DEFAULT_TASK_PARAMETER_MAX_LENGTH = 256;
 
 /**
  * TaskWorkerOptions
@@ -44,6 +48,7 @@ export type TaskWorkerOptions = {
   permissions?: PermissionEvaluator;
   logger?: LoggerService;
   auditor?: AuditorService;
+  config?: Config;
   gracefulShutdown?: boolean;
 };
 
@@ -59,6 +64,7 @@ export type CreateWorkerOptions = {
   workingDirectory: string;
   logger: LoggerService;
   auditor?: AuditorService;
+  config?: Config;
   additionalTemplateFilters?: Record<string, TemplateFilter>;
   /**
    * The number of tasks that can be executed at the same time by the worker
@@ -87,15 +93,21 @@ export class TaskWorker {
   private taskQueue: PQueue;
   private logger: LoggerService | undefined;
   private auditor: AuditorService | undefined;
+  private parameterAuditTransform: ParameterAuditTransform;
   private stopWorkers: boolean;
 
-  private constructor(private readonly options: TaskWorkerOptions) {
+  private constructor(
+    private readonly options: TaskWorkerOptions & {
+      parameterAuditTransform: ParameterAuditTransform;
+    },
+  ) {
     this.stopWorkers = false;
     this.logger = options.logger;
     this.auditor = options.auditor;
     this.taskQueue = new PQueue({
       concurrency: options.concurrentTasksLimit,
     });
+    this.parameterAuditTransform = options.parameterAuditTransform;
   }
 
   static async create(options: CreateWorkerOptions): Promise<TaskWorker> {
@@ -103,6 +115,7 @@ export class TaskWorker {
       taskBroker,
       logger,
       auditor,
+      config,
       actionRegistry,
       integrations,
       workingDirectory,
@@ -130,7 +143,9 @@ export class TaskWorker {
       concurrentTasksLimit,
       permissions,
       auditor,
+      config,
       gracefulShutdown,
+      parameterAuditTransform: createParameterTruncator(config),
     });
   }
 
@@ -188,9 +203,9 @@ export class TaskWorker {
       severityLevel: 'medium',
       meta: {
         actionType: 'execution',
-        taskId: task.taskId,
         createdBy: task.createdBy,
-        taskParameters: task.spec.parameters,
+        taskId: task.taskId,
+        taskParameters: this.parameterAuditTransform(task.spec.parameters),
         templateRef: task.spec.templateInfo?.entityRef,
       },
     });
@@ -219,3 +234,53 @@ export class TaskWorker {
     }
   }
 }
+
+type ParameterAuditTransform = (parameters: JsonObject) => JsonObject;
+
+/**
+ * Truncates task parameters for audit logging using the configured max length.
+ * @internal
+ */
+export function createParameterTruncator(
+  config?: Config,
+): ParameterAuditTransform {
+  const maxLength =
+    config?.getOptionalNumber('scaffolder.auditor.taskParameterMaxLength') ??
+    DEFAULT_TASK_PARAMETER_MAX_LENGTH;
+
+  if (!Number.isSafeInteger(maxLength) || maxLength < -1) {
+    throw new InputError(
+      `Invalid configuration for 'scaffolder.auditor.taskParameterMaxLength', got ${maxLength}. Must be a positive integer or -1 to disable truncation.`,
+    );
+  }
+
+  if (maxLength === -1) {
+    return (parameters: JsonObject) => parameters;
+  }
+
+  return (parameters: JsonObject) => {
+    function truncate(value: unknown): unknown {
+      if (typeof value === 'string') {
+        if (value.length > maxLength) {
+          return value.slice(0, maxLength).concat('...<truncated>');
+        }
+        return value;
+      }
+      if (Array.isArray(value)) {
+        return value.map(truncate);
+      }
+      if (value && typeof value === 'object') {
+        const result: Record<string, unknown> = {};
+        for (const k in value as object) {
+          if (Object.hasOwn(value, k)) {
+            result[k] = truncate((value as any)[k]);
+          }
+        }
+        return result;
+      }
+      return value;
+    }
+
+    return truncate(parameters) as JsonObject;
+  };
+}

plugins/scaffolder-backend/src/service/router.ts

@@ -287,6 +287,7 @@ export async function createRouter(
       integrations,
       logger,
       auditor,
+      config,
       workingDirectory,
       concurrentTasksLimit,
       permissions,