diff --git a/.changeset/three-mammals-move.md b/.changeset/three-mammals-move.md new file mode 100644 index 0000000000..646e60f84c --- /dev/null +++ b/.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 diff --git a/app-config.yaml b/app-config.yaml index 9c0cb8ef16..60842ee61a 100644 --- a/app-config.yaml +++ b/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 diff --git a/plugins/scaffolder-backend/config.d.ts b/plugins/scaffolder-backend/config.d.ts index ae91c905b3..b4fb532901 100644 --- a/plugins/scaffolder-backend/config.d.ts +++ b/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; + }; }; } diff --git a/plugins/scaffolder-backend/report.api.md b/plugins/scaffolder-backend/report.api.md index ada936c6e9..918718e495 100644 --- a/plugins/scaffolder-backend/report.api.md +++ b/plugins/scaffolder-backend/report.api.md @@ -275,6 +275,7 @@ export type CreateWorkerOptions = { workingDirectory: string; logger: LoggerService; auditor?: AuditorService; + config?: Config; additionalTemplateFilters?: Record; concurrentTasksLimit?: number; additionalTemplateGlobals?: Record; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.test.ts index 6fedc4df40..7fe0618af2 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.test.ts +++ b/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...', + }, + }, + }); + 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...', + nested: { + test3: 'anoth...', + test4: ['ok', 'toolo...', { prop: 'thisi...' }], + }, + }); + }); + + 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."`, + ); + }); +}); diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts index 24871ca452..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 { @@ -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; /** * 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 { @@ -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('...'); + } + 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; + }; +} diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index e8b9f04797..5bdaf5ef86 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -287,6 +287,7 @@ export async function createRouter( integrations, logger, auditor, + config, workingDirectory, concurrentTasksLimit, permissions,