Merge pull request #28000 from backstage/rugvip/stack

More safeguards against stack traces in error responses

.changeset/dull-bulldogs-learn.md

@@ -0,0 +1,5 @@
+---
+'@backstage/backend-defaults': patch
+---
+
+The user and plugin token verification in the default `AuthService` implementation will no longer forward verification errors to the caller, and instead log them as warnings.

.changeset/forty-ravens-fry.md

@@ -0,0 +1,5 @@
+---
+'@backstage/backend-defaults': patch
+---
+
+The default `authServiceFactory` now correctly depends on the plugin scoped `Logger` services rather than the root scoped one.

.changeset/thin-hotels-explode.md

@@ -0,0 +1,5 @@
+---
+'@backstage/errors': patch
+---
+
+Trim `error.cause.stack` in addition to `error.stack` when trimming stack traces from serialized errors.

packages/backend-defaults/src/entrypoints/auth/authServiceFactory.test.ts

@@ -109,8 +109,9 @@ describe('authServiceFactory', () => {
   });
 
   it('should authenticate issued tokens with new auth', async () => {
+    const logger = mockServices.logger.mock();
     const tester = ServiceFactoryTester.from(authServiceFactory, {
-      dependencies: mockDeps,
+      dependencies: [...mockDeps, logger.factory],
     });
 
     const searchAuth = await tester.getSubject('search');
@@ -134,8 +135,13 @@ describe('authServiceFactory', () => {
       targetPluginId: 'catalog',
     });
 
+    expect(logger.warn).not.toHaveBeenCalled();
     await expect(searchAuth.authenticate(searchToken)).rejects.toThrow(
-      'Invalid plugin token',
+      'Failed plugin token verification',
+    );
+    expect(logger.warn).toHaveBeenCalledWith(
+      'Failed to verify incoming plugin token',
+      expect.any(Error),
     );
     await expect(catalogAuth.authenticate(searchToken)).resolves.toEqual(
       expect.objectContaining({

packages/backend-defaults/src/entrypoints/auth/authServiceFactory.ts

@@ -59,7 +59,7 @@ export const authServiceFactory = createServiceFactory({
   service: coreServices.auth,
   deps: {
     config: coreServices.rootConfig,
-    logger: coreServices.rootLogger,
+    logger: coreServices.logger,
     discovery: coreServices.discovery,
     plugin: coreServices.pluginMetadata,
     database: coreServices.database,
@@ -89,6 +89,7 @@ export const authServiceFactory = createServiceFactory({
 
     const userTokens = UserTokenHandler.create({
       discovery,
+      logger,
     });
 
     const pluginTokens = pluginTokenHandlerDecorator(

packages/backend-defaults/src/entrypoints/auth/plugin/PluginTokenHandler.ts

@@ -120,7 +120,8 @@ export class DefaultPluginTokenHandler implements PluginTokenHandler {
         requiredClaims: ['iat', 'exp', 'sub', 'aud'],
       },
     ).catch(e => {
-      throw new AuthenticationError('Invalid plugin token', e);
+      this.logger.warn('Failed to verify incoming plugin token', e);
+      throw new AuthenticationError('Failed plugin token verification');
     });
 
     return { subject: `plugin:${payload.sub}`, limitedUserToken: payload.obo };

packages/backend-defaults/src/entrypoints/auth/user/UserTokenHandler.test.ts

@@ -64,13 +64,17 @@ async function createToken(options: {
 describe('UserTokenHandler', () => {
   let userTokenHandler: UserTokenHandler;
 
+  const logger = mockServices.logger.mock();
+
   registerMswTestHooks(server);
 
   beforeEach(() => {
     jest.useRealTimers();
+    jest.resetAllMocks();
 
     userTokenHandler = UserTokenHandler.create({
       discovery: mockServices.discovery(),
+      logger,
     });
 
     server.use(
@@ -120,7 +124,7 @@ describe('UserTokenHandler', () => {
             signature: 'sig',
           }),
         ),
-      ).rejects.toThrow('signature verification failed');
+      ).rejects.toThrow('Failed user token verification');
 
       await expect(
         userTokenHandler.verifyToken(
@@ -130,7 +134,7 @@ describe('UserTokenHandler', () => {
             signature: 'sig',
           }),
         ),
-      ).rejects.toThrow('signature verification failed');
+      ).rejects.toThrow('Failed user token verification');
     });
 
     it('should fail to verify tokens that have a bad alg', async () => {
@@ -156,8 +160,13 @@ describe('UserTokenHandler', () => {
       });
       const token = `${header}.${payload}.`;
 
+      expect(logger.warn).not.toHaveBeenCalled();
       await expect(userTokenHandler.verifyToken(token)).rejects.toThrow(
-        /Unsupported "alg" value/,
+        'Failed user token verification',
+      );
+      expect(logger.warn).toHaveBeenCalledWith(
+        'Failed to verify incoming user token',
+        expect.any(Error),
       );
     });
 

packages/backend-defaults/src/entrypoints/auth/user/UserTokenHandler.ts

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { DiscoveryService } from '@backstage/backend-plugin-api';
+import { DiscoveryService, LoggerService } from '@backstage/backend-plugin-api';
 import { AuthenticationError } from '@backstage/errors';
 import { tokenTypes } from '@backstage/plugin-auth-node';
 import {
@@ -33,15 +33,21 @@ import { JwksClient } from '../JwksClient';
  * @internal
  */
 export class UserTokenHandler {
-  static create(options: { discovery: DiscoveryService }): UserTokenHandler {
+  static create(options: {
+    discovery: DiscoveryService;
+    logger: LoggerService;
+  }): UserTokenHandler {
     const jwksClient = new JwksClient(async () => {
       const url = await options.discovery.getBaseUrl('auth');
       return new URL(`${url}/.well-known/jwks.json`);
     });
-    return new UserTokenHandler(jwksClient);
+    return new UserTokenHandler(jwksClient, options.logger);
   }
 
-  constructor(private readonly jwksClient: JwksClient) {}
+  constructor(
+    private readonly jwksClient: JwksClient,
+    private readonly logger: LoggerService,
+  ) {}
 
   async verifyToken(token: string) {
     const verifyOpts = this.#getTokenVerificationOptions(token);
@@ -57,7 +63,8 @@ export class UserTokenHandler {
       this.jwksClient.getKey,
       verifyOpts,
     ).catch(e => {
-      throw new AuthenticationError('Invalid token', e);
+      this.logger.warn('Failed to verify incoming user token', e);
+      throw new AuthenticationError('Failed user token verification');
     });
 
     const userEntityRef = payload.sub;

packages/errors/src/serialization/error.test.ts

@@ -63,6 +63,20 @@ describe('serialization', () => {
     expect(withoutStack2.stack).not.toBeDefined();
   });
 
+  it('serializes stack traces only when allowed with error cause', () => {
+    const before = new CustomError('m');
+    before.cause = new Error('cause');
+    const withStack: any = serializeError(before, { includeStack: true });
+    const withoutStack1: any = serializeError(before, { includeStack: false });
+    const withoutStack2: any = serializeError(before);
+    expect(withStack.stack).toEqual(before.stack);
+    expect(withStack.cause.stack).toEqual(withStack.cause.stack);
+    expect(withoutStack1.stack).not.toBeDefined();
+    expect(withoutStack2.stack).not.toBeDefined();
+    expect(withoutStack1.cause.stack).not.toBeDefined();
+    expect(withoutStack2.cause.stack).not.toBeDefined();
+  });
+
   it('stringifies all supported forms', () => {
     expect(stringifyError({})).toEqual("unknown error '[object Object]'");
     expect(

packages/errors/src/serialization/error.ts

@@ -60,6 +60,14 @@ export function serializeError(
 
   if (!options?.includeStack) {
     delete result.stack;
+
+    if (
+      result.cause &&
+      typeof result.cause === 'object' &&
+      'stack' in result.cause
+    ) {
+      delete result.cause.stack;
+    }
   }
 
   return result;

plugins/catalog-backend/src/tests/integration.test.ts

@@ -442,7 +442,6 @@ describe('Catalog Backend Integration', () => {
                 cause: {
                   name: 'Error',
                   message: 'NOPE',
-                  stack: expect.stringMatching(/^Error: NOPE/),
                 },
               },
             },