Merge pull request #33385 from drodil/auth_mem_leaks

fix(auth): memory leaks

plugins/auth-backend/migrations/20260317120000_drop_offline_sessions_fk.js

@@ -0,0 +1,49 @@
+/*
+ * Copyright 2026 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// @ts-check
+
+/**
+ * Drop the foreign key constraint on offline_sessions.oidc_client_id
+ * to allow CIMD (Client ID Metadata Document) clients which are not stored
+ * in the oidc_clients table.
+ *
+ * @param {import('knex').Knex} knex
+ */
+exports.up = async function up(knex) {
+  await knex.schema.alterTable('offline_sessions', table => {
+    table.dropForeign(['oidc_client_id']);
+  });
+};
+
+/**
+ * @param {import('knex').Knex} knex
+ */
+exports.down = async function down(knex) {
+  // Delete sessions with CIMD client_ids (not in oidc_clients) before re-adding FK
+  await knex('offline_sessions')
+    .whereNotNull('oidc_client_id')
+    .whereNotIn('oidc_client_id', knex('oidc_clients').select('client_id'))
+    .delete();
+
+  await knex.schema.alterTable('offline_sessions', table => {
+    table
+      .foreign('oidc_client_id')
+      .references('client_id')
+      .inTable('oidc_clients')
+      .onDelete('CASCADE');
+  });
+};

plugins/auth-backend/src/service/OidcRouter.test.ts

@@ -109,6 +109,7 @@ describe('OidcRouter', () => {
       userInfo: userInfoDatabase,
       oidc: oidcDatabase,
       config: mockConfig,
+      logger: mockServices.logger.mock(),
     });
 
     const oidcRouter = OidcRouter.create({
@@ -194,6 +195,7 @@ describe('OidcRouter', () => {
       userInfo: userInfoDatabase,
       oidc: oidcDatabase,
       config: mockConfig,
+      logger: mockServices.logger.mock(),
       offlineAccess,
     });
 

plugins/auth-backend/src/service/OidcService.test.ts

@@ -20,6 +20,7 @@ import {
 } from '@backstage/backend-test-utils';
 import { JsonObject } from '@backstage/types';
 import { OidcService } from './OidcService';
+import { OfflineAccessService } from './OfflineAccessService';
 import {
   BackstageCredentials,
   BackstageServicePrincipal,
@@ -53,10 +54,11 @@ describe('OidcService', () => {
   interface CreateOidcServiceOptions {
     databaseId: TestDatabaseId;
     config?: JsonObject;
+    offlineAccess?: OfflineAccessService;
   }
 
   async function createOidcService(options: CreateOidcServiceOptions) {
-    const { databaseId, config: configData = {} } = options;
+    const { databaseId, config: configData = {}, offlineAccess } = options;
 
     const knex = await databases.init(databaseId);
 
@@ -94,6 +96,8 @@ describe('OidcService', () => {
         userInfo: mockUserInfo,
         oidc: oidcDatabase,
         config,
+        logger: mockServices.logger.mock(),
+        offlineAccess,
       }),
       mocks: {
         auth: mockAuth,
@@ -836,6 +840,50 @@ describe('OidcService', () => {
           }),
         ).rejects.toThrow('Invalid code verifier');
       });
+
+      it('should still return access token when refresh token issuance fails', async () => {
+        const mockOfflineAccess = {
+          issueRefreshToken: jest
+            .fn()
+            .mockRejectedValue(new Error('DB constraint violation')),
+        } as unknown as OfflineAccessService;
+
+        const { service, mocks } = await createOidcService({
+          databaseId,
+          offlineAccess: mockOfflineAccess,
+        });
+        const mockToken = 'mock-jwt-token';
+        mocks.tokenIssuer.issueToken.mockResolvedValue({ token: mockToken });
+
+        const client = await service.registerClient({
+          clientName: 'Test Client',
+          redirectUris: ['https://example.com/callback'],
+        });
+
+        const authSession = await service.createAuthorizationSession({
+          clientId: client.clientId,
+          redirectUri: 'https://example.com/callback',
+          responseType: 'code',
+          scope: 'openid offline_access',
+        });
+
+        const authResult = await service.approveAuthorizationSession({
+          sessionId: authSession.id,
+          userEntityRef: 'user:default/test',
+        });
+
+        const code = new URL(authResult.redirectUrl).searchParams.get('code')!;
+
+        const tokenResult = await service.exchangeCodeForToken({
+          code,
+          redirectUri: 'https://example.com/callback',
+          grantType: 'authorization_code',
+        });
+
+        expect(tokenResult.accessToken).toBe(mockToken);
+        expect(tokenResult.refreshToken).toBeUndefined();
+        expect(mockOfflineAccess.issueRefreshToken).toHaveBeenCalledTimes(1);
+      });
     });
 
     describe('CIMD (Client ID Metadata Document) support', () => {

plugins/auth-backend/src/service/OidcService.ts

@@ -13,7 +13,11 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { AuthService, RootConfigService } from '@backstage/backend-plugin-api';
+import {
+  AuthService,
+  LoggerService,
+  RootConfigService,
+} from '@backstage/backend-plugin-api';
 import { TokenIssuer } from '../identity/types';
 import { UserInfoDatabase } from '../database/UserInfoDatabase';
 import {
@@ -48,6 +52,7 @@ export class OidcService {
   private readonly userInfo: UserInfoDatabase;
   private readonly oidc: OidcDatabase;
   private readonly config: RootConfigService;
+  private readonly logger: LoggerService;
   private readonly offlineAccess?: OfflineAccessService;
 
   private constructor(
@@ -57,6 +62,7 @@ export class OidcService {
     userInfo: UserInfoDatabase,
     oidc: OidcDatabase,
     config: RootConfigService,
+    logger: LoggerService,
     offlineAccess?: OfflineAccessService,
   ) {
     this.auth = auth;
@@ -65,6 +71,7 @@ export class OidcService {
     this.userInfo = userInfo;
     this.oidc = oidc;
     this.config = config;
+    this.logger = logger;
     this.offlineAccess = offlineAccess;
   }
 
@@ -75,6 +82,7 @@ export class OidcService {
     userInfo: UserInfoDatabase;
     oidc: OidcDatabase;
     config: RootConfigService;
+    logger: LoggerService;
     offlineAccess?: OfflineAccessService;
   }) {
     return new OidcService(
@@ -84,6 +92,7 @@ export class OidcService {
       options.userInfo,
       options.oidc,
       options.config,
+      options.logger,
       options.offlineAccess,
     );
   }
@@ -509,10 +518,18 @@ export class OidcService {
     let refreshToken: string | undefined;
     const scopes = session.scope?.split(' ') ?? [];
     if (scopes.includes('offline_access') && this.offlineAccess) {
-      refreshToken = await this.offlineAccess.issueRefreshToken({
-        userEntityRef: session.userEntityRef,
-        oidcClientId: session.clientId,
-      });
+      try {
+        refreshToken = await this.offlineAccess.issueRefreshToken({
+          userEntityRef: session.userEntityRef,
+          oidcClientId: session.clientId,
+        });
+      } catch (err) {
+        // Don't fail the entire token exchange if refresh token issuance fails.
+        // The access token is still valid and should be returned.
+        this.logger.warn(
+          `Failed to issue refresh token for user ${session.userEntityRef}, offline_access will not be available: ${err}`,
+        );
+      }
     }
 
     return {