fix(aap+3scale+ocm): don't log sensitive data from errors (#945)

Signed-off-by: Christoph Jerolimov <[email protected]>

packages/cli/templates/default-backend-plugin/src/run.ts.hbs

@@ -7,7 +7,7 @@ const enableCors = yn(process.env.PLUGIN_CORS, { default: false });
 const logger = getRootLogger();
 
 startStandaloneServer({ port, enableCors, logger }).catch(err => {
-  logger.error(err);
+  logger.error('Standalone server failed:', err);
   process.exit(1);
 });
 

packages/cli/templates/default-backend-plugin/src/service/standaloneServer.ts.hbs

@@ -26,7 +26,7 @@ export async function startStandaloneServer(
   }
 
   return await service.start().catch(err => {
-    logger.error(err);
+    logger.error('Dev server failed:', err);
     process.exit(1);
   });
 }

plugins/3scale-backend/src/providers/ThreeScaleApiEntityProvider.ts

@@ -104,8 +104,19 @@ export class ThreeScaleApiEntityProvider implements EntityProvider {
         fn: async () => {
           try {
             await this.run();
-          } catch (error) {
-            this.logger.error(error);
+          } catch (error: any) {
+            // Ensure that we don't log any sensitive internal data:
+            this.logger.error(
+              `Error while syncing 3scale API from ${this.baseUrl}`,
+              {
+                // Default Error properties:
+                name: error.name,
+                message: error.message,
+                stack: error.stack,
+                // Additional status code if available:
+                status: error.response?.status,
+              },
+            );
           }
         },
       });

plugins/aap-backend/src/providers/AapResourceEntityProvider.test.ts

@@ -1,5 +1,5 @@
 import { getVoidLogger } from '@backstage/backend-common';
-import { TaskRunner } from '@backstage/backend-tasks';
+import { TaskInvocationDefinition, TaskRunner } from '@backstage/backend-tasks';
 import { ConfigReader } from '@backstage/config';
 import { EntityProviderConnection } from '@backstage/plugin-catalog-node';
 
@@ -9,6 +9,8 @@ import {
 } from '../clients/AapResourceConnector';
 import { AapResourceEntityProvider } from './AapResourceEntityProvider';
 
+const AUTH_HEADER = 'Bearer xxxx'; // NOSONAR
+
 const BASIC_VALID_CONFIG = {
   catalog: {
     providers: {
@@ -27,7 +29,7 @@ const BASIC_VALID_CONFIG_2 = {
       aap: {
         dev: {
           baseUrl: 'http://localhost:8080',
-          authorization: 'Bearer xxxx',
+          authorization: AUTH_HEADER,
         },
       },
     },
@@ -45,17 +47,71 @@ jest.mock('../clients/AapResourceConnector', () => ({
   listWorkflowJobTemplates: jest.fn().mockReturnValue({}),
 }));
 
+class FakeAbortSignal implements AbortSignal {
+  readonly aborted = false;
+  readonly reason = undefined;
+  onabort() {
+    return null;
+  }
+  throwIfAborted() {
+    return null;
+  }
+  addEventListener() {
+    return null;
+  }
+  removeEventListener() {
+    return null;
+  }
+  dispatchEvent() {
+    return true;
+  }
+}
+
+class ManualTaskRunner implements TaskRunner {
+  private tasks: TaskInvocationDefinition[] = [];
+  async run(task: TaskInvocationDefinition) {
+    this.tasks.push(task);
+  }
+  async runAll() {
+    const abortSignal = new FakeAbortSignal();
+    for await (const task of this.tasks) {
+      await task.fn(abortSignal);
+    }
+  }
+  clear() {
+    this.tasks = [];
+  }
+}
+
 describe('AapResourceEntityProvider', () => {
+  const logMock = jest.fn();
+
+  const logger = getVoidLogger();
+  logger.child = () => logger;
+  ['log', ...Object.keys(logger.levels)].forEach(logFunctionName => {
+    (logger as any)[logFunctionName] = function LogMock() {
+      logMock(logFunctionName, ...arguments);
+    };
+  });
+
+  const manualTaskRunner = new ManualTaskRunner();
+
   beforeEach(() => {
-    (listJobTemplates as jest.Mock).mockClear();
-    (listWorkflowJobTemplates as jest.Mock).mockClear();
+    jest.clearAllMocks();
+    manualTaskRunner.clear();
+  });
+
+  afterEach(() => {
+    const logs = JSON.stringify(logMock.mock.calls);
+    // eslint-disable-next-line jest/no-standalone-expect
+    expect(logs).not.toContain(AUTH_HEADER);
   });
 
   it('should return an empty array if no providers are configured', () => {
     const config = new ConfigReader({});
 
     const result = AapResourceEntityProvider.fromConfig(config, {
-      logger: getVoidLogger(),
+      logger,
     });
 
     expect(result).toEqual([]);
@@ -66,7 +122,7 @@ describe('AapResourceEntityProvider', () => {
 
     expect(() =>
       AapResourceEntityProvider.fromConfig(config, {
-        logger: getVoidLogger(),
+        logger,
       }),
     ).toThrow(
       "Missing required config value at 'catalog.providers.aap.dev.authorization",
@@ -78,7 +134,7 @@ describe('AapResourceEntityProvider', () => {
 
     expect(() =>
       AapResourceEntityProvider.fromConfig(config, {
-        logger: getVoidLogger(),
+        logger,
       }),
     ).toThrow(
       'No schedule provided neither via code nor config for AapResourceEntityProvider:dev.',
@@ -88,8 +144,8 @@ describe('AapResourceEntityProvider', () => {
   it('should return a single provider if one is configured', () => {
     const config = new ConfigReader(BASIC_VALID_CONFIG_2);
     const aap = AapResourceEntityProvider.fromConfig(config, {
-      logger: getVoidLogger(),
-      schedule: {} as TaskRunner,
+      logger,
+      schedule: manualTaskRunner,
     });
 
     expect(aap).toHaveLength(1);
@@ -99,8 +155,8 @@ describe('AapResourceEntityProvider', () => {
     const config = new ConfigReader(BASIC_VALID_CONFIG_2);
 
     const aap = AapResourceEntityProvider.fromConfig(config, {
-      logger: getVoidLogger(),
-      schedule: {} as TaskRunner,
+      logger,
+      schedule: manualTaskRunner,
     });
 
     expect(aap.map(k => k.getProviderName())).toEqual([
@@ -112,8 +168,8 @@ describe('AapResourceEntityProvider', () => {
     const config = new ConfigReader(BASIC_VALID_CONFIG_2);
 
     const aap = AapResourceEntityProvider.fromConfig(config, {
-      logger: getVoidLogger(),
-      schedule: { run: jest.fn() } as TaskRunner,
+      logger,
+      schedule: manualTaskRunner,
     });
 
     const result = await Promise.all(
@@ -146,20 +202,32 @@ describe('AapResourceEntityProvider', () => {
     const config = new ConfigReader(BASIC_VALID_CONFIG_2);
 
     const aap = AapResourceEntityProvider.fromConfig(config, {
-      logger: getVoidLogger(),
-      schedule: { run: jest.fn() } as TaskRunner,
+      logger,
+      schedule: manualTaskRunner,
     });
 
     for await (const k of aap) {
       await k.connect(connection);
-      await expect(k.run()).resolves.toBeUndefined();
+      await manualTaskRunner.runAll();
     }
+
+    expect(connection.applyMutation).toHaveBeenCalledTimes(1);
+    expect(
+      (connection.applyMutation as jest.Mock).mock.calls,
+    ).toMatchSnapshot();
   });
 
   it('should connect and run should resolves even if one api call fails', async () => {
-    (listJobTemplates as jest.Mock).mockReturnValue(
-      Promise.reject(new Error('404')),
+    const error: Error & { config?: any; status?: number } = new Error(
+      'Request failed with status code 401',
     );
+    error.config = {
+      header: {
+        authorization: 'Bearer xxxx', // NOSONAR
+      },
+    };
+    error.status = 401;
+    (listJobTemplates as jest.Mock).mockRejectedValue(error);
     (listWorkflowJobTemplates as jest.Mock).mockReturnValue(
       Promise.resolve([
         {
@@ -173,13 +241,36 @@ describe('AapResourceEntityProvider', () => {
     const config = new ConfigReader(BASIC_VALID_CONFIG_2);
 
     const aap = AapResourceEntityProvider.fromConfig(config, {
-      logger: getVoidLogger(),
-      schedule: { run: jest.fn() } as TaskRunner,
+      logger,
+      schedule: manualTaskRunner,
     });
 
     for await (const k of aap) {
       await k.connect(connection);
-      await expect(k.run()).resolves.toBeUndefined();
+      await manualTaskRunner.runAll();
     }
+
+    expect(connection.applyMutation).toHaveBeenCalledTimes(1);
+    expect(
+      (connection.applyMutation as jest.Mock).mock.calls,
+    ).toMatchSnapshot();
+
+    expect(logMock).toHaveBeenCalledWith(
+      'info',
+      'Discovering ResourceEntities from AAP http://localhost:8080',
+    );
+    expect(logMock).toHaveBeenCalledWith(
+      'error',
+      'Failed to fetch AAP job templates',
+      {
+        name: 'Error',
+        message: 'Request failed with status code 401',
+        stack: expect.any(String),
+      },
+    );
+    expect(logMock).toHaveBeenCalledWith(
+      'debug',
+      'Discovered ResourceEntity "demoWorkflowJobTemplate"',
+    );
   });
 });

plugins/aap-backend/src/providers/AapResourceEntityProvider.ts

@@ -80,16 +80,27 @@ export class AapResourceEntityProvider implements EntityProvider {
     this.scheduleFn = this.createScheduleFn(taskRunner);
   }
 
-  private createScheduleFn(taskRunner: TaskRunner): () => Promise<void> {
+  createScheduleFn(taskRunner: TaskRunner): () => Promise<void> {
     return async () => {
       const taskId = `${this.getProviderName()}:run`;
       return taskRunner.run({
         id: taskId,
         fn: async () => {
           try {
             await this.run();
-          } catch (error) {
-            this.logger.error(error);
+          } catch (error: any) {
+            // Ensure that we don't log any sensitive internal data:
+            this.logger.error(
+              `Error while syncing resources from AAP ${this.baseUrl}`,
+              {
+                // Default Error properties:
+                name: error.name,
+                message: error.message,
+                stack: error.stack,
+                // Additional status code if available:
+                status: error.response?.status,
+              },
+            );
           }
         },
       });
@@ -125,15 +136,24 @@ export class AapResourceEntityProvider implements EntityProvider {
       if (results.status === 'fulfilled') {
         templates.push(...results.value);
       } else if (results.status === 'rejected') {
-        console.error('Failed to fetch AAP job templates', results.reason);
+        // Ensure that we don't log any sensitive internal data:
+        const error = results.reason || {};
+        this.logger.error('Failed to fetch AAP job templates', {
+          // Default Error properties:
+          name: error.name,
+          message: error.message,
+          stack: error.stack,
+          // Additional status code if available:
+          status: error.response?.status,
+        });
       }
     });
 
     for (const template of templates) {
       const resourceEntity: ResourceEntity =
         this.buildApiEntityFromJobTemplate(template);
       entities.push(resourceEntity);
-      this.logger.debug(`Discovered ResourceEntity ${template}`);
+      this.logger.debug(`Discovered ResourceEntity "${template.name}"`);
     }
 
     this.logger.info(`Applying the mutation with ${entities.length} entities`);