From 3aeaf2555fb79fd64c207c790d277d2d7bea1788 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 12 Aug 2025 17:04:19 +0200 Subject: [PATCH] scaffolder-backend: refactor parameter truncation Signed-off-by: Patrik Oldsberg --- .../src/scaffolder/tasks/TaskWorker.test.ts | 111 +++++++++++++----- .../src/scaffolder/tasks/TaskWorker.ts | 107 ++++++++++------- 2 files changed, 143 insertions(+), 75 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.test.ts index c66b5dbae5..4b28eb8729 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.test.ts @@ -16,10 +16,14 @@ import os from 'os'; import { DatabaseManager } from '@backstage/backend-defaults/database'; -import { Config, ConfigReader } from '@backstage/config'; +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...', + }, + }, + }); + expect(auditEvent.success).toHaveBeenCalled(); + }); }); describe('Concurrent TaskWorker', () => { @@ -344,33 +408,11 @@ describe('TaskWorker internals', () => { }); }); -describe('TaskWorker.truncateParameters', () => { - let worker: TaskWorker; - - beforeEach(async () => { - jest.resetAllMocks(); - - const logger = { debug: jest.fn() } as any; - - const config = { - getOptionalNumber: jest.fn().mockReturnValue(5), - } as unknown as Config; - - worker = await TaskWorker.create({ - logger, - workingDirectory: '/tmp', - integrations: {} as ScmIntegrations, - taskBroker: {} as TaskBroker, - actionRegistry: {} as TemplateActionRegistry, - config, - }); - }); - +describe('createParameterTruncator', () => { it('successfully does nothing', async () => { const testParams = {}; - // @ts-expect-error (truncateParameters is private, but for test we can access) - const result = worker.truncateParameters(testParams); + const result = createParameterTruncator()(testParams); expect(result).toEqual({}); }); @@ -381,19 +423,28 @@ describe('TaskWorker.truncateParameters', () => { test2: 'thisisaverylongstring', nested: { test3: 'anotherlongstringhere', - test4: ['ok', 'toolongstring'], + test4: ['ok', 'toolongstring', { prop: 'thisisaverylongstring' }], }, }; - // @ts-expect-error (truncateParameters is private, but for test we can access) - const result = worker.truncateParameters(params); + const result = createParameterTruncator( + mockServices.rootConfig({ + data: { + scaffolder: { + auditor: { + taskParameterMaxLength: 5, + }, + }, + }, + }), + )(params); expect(result).toEqual({ test: 'short', test2: 'thisi...', nested: { test3: 'anoth...', - test4: ['ok', 'toolo...'], + test4: ['ok', 'toolo...', { prop: 'thisi...' }], }, }); }); diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts index 9cdab0d58a..9abe5f3ba6 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts +++ b/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 { @@ -32,6 +32,8 @@ import { setTimeout } from 'timers/promises'; import { JsonObject } from '@backstage/types'; import { Config } from '@backstage/config'; +const DEFAULT_TASK_PARAMETER_MAX_LENGTH = 256; + /** * TaskWorkerOptions * @deprecated this type is deprecated, and there will be a new way to create Workers in the next major version. @@ -91,17 +93,21 @@ export class TaskWorker { private taskQueue: PQueue; private logger: LoggerService | undefined; private auditor: AuditorService | undefined; - private config: Config | 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.config = options.config; this.taskQueue = new PQueue({ concurrency: options.concurrentTasksLimit, }); + this.parameterAuditTransform = options.parameterAuditTransform; } static async create(options: CreateWorkerOptions): Promise { @@ -139,6 +145,7 @@ export class TaskWorker { auditor, config, gracefulShutdown, + parameterAuditTransform: createParameterTruncator(config), }); } @@ -190,46 +197,6 @@ export class TaskWorker { }); } - private truncateParameters(parameters: JsonObject) { - const taskParameterMaxLength = - this.config?.getOptionalNumber( - 'scaffolder.auditor.taskParameterMaxLength', - ) ?? 256; - - if (taskParameterMaxLength === -1) { - this.logger?.debug( - `scaffolder.auditor.taskParameterMaxLength manually disabled via configuration, no task parameter length limit set.`, - ); - return parameters; - } - - function truncate(value: unknown): unknown { - if (typeof value === 'string') { - if (value.length > taskParameterMaxLength) { - return value - .slice(0, taskParameterMaxLength) - .concat('...'); - } - return value; - } - if (Array.isArray(value)) { - return value.map(truncate); - } - if (value && typeof value === 'object') { - const result: Record = {}; - 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; - } - async runOneTask(task: TaskContext) { const auditorEvent = await this.auditor?.createEvent({ eventId: 'task', @@ -238,7 +205,7 @@ export class TaskWorker { actionType: 'execution', createdBy: task.createdBy, taskId: task.taskId, - taskParameters: this.truncateParameters(task.spec.parameters), + taskParameters: this.parameterAuditTransform(task.spec.parameters), templateRef: task.spec.templateInfo?.entityRef, }, }); @@ -267,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('...'); + } + return value; + } + if (Array.isArray(value)) { + return value.map(truncate); + } + if (value && typeof value === 'object') { + const result: Record = {}; + 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; + }; +}