From ff15f3032970aa35015ce245f68ba00f03fc3283 Mon Sep 17 00:00:00 2001 From: benjdlambert Date: Mon, 8 Sep 2025 17:48:33 +0200 Subject: [PATCH] feat: implementing fixes for wildcard matching for callback URLs Signed-off-by: benjdlambert --- .../src/service/OidcRouter.test.ts | 14 ++++++- .../auth-backend/src/service/OidcRouter.ts | 15 ++++--- .../src/service/OidcService.test.ts | 42 +++++++++++++++++++ .../auth-backend/src/service/OidcService.ts | 22 ++++++++-- plugins/auth-backend/src/service/router.ts | 5 +-- 5 files changed, 84 insertions(+), 14 deletions(-) diff --git a/plugins/auth-backend/src/service/OidcRouter.test.ts b/plugins/auth-backend/src/service/OidcRouter.test.ts index bffe9ee509..81430ef05e 100644 --- a/plugins/auth-backend/src/service/OidcRouter.test.ts +++ b/plugins/auth-backend/src/service/OidcRouter.test.ts @@ -70,6 +70,15 @@ describe('OidcRouter', () => { const mockAuth = mockServices.auth.mock(); const mockHttpAuth = mockServices.httpAuth.mock(); + const mockConfig = mockServices.rootConfig({ + data: { + auth: { + experimentalDynamicClientRegistration: { + enabled: true, + }, + }, + }, + }); const oidcService = OidcService.create({ auth: mockAuth, @@ -77,6 +86,7 @@ describe('OidcRouter', () => { baseUrl: 'http://localhost:7000', userInfo: userInfoDatabase, oidc: oidcDatabase, + config: mockConfig, }); const oidcRouter = OidcRouter.create({ @@ -88,7 +98,7 @@ describe('OidcRouter', () => { userInfo: userInfoDatabase, oidc: oidcDatabase, httpAuth: mockHttpAuth, - enableDynamicClientRegistration: true, + config: mockConfig, }); return { @@ -303,7 +313,7 @@ describe('OidcRouter', () => { .expect(302); expect(response.header.location).toMatch( - /^http:\/\/localhost:3000\/auth\/sessions\/[a-f0-9-]+$/, + /^http:\/\/localhost:3000\/oauth2\/authorize\/[a-f0-9-]+$/, ); }); diff --git a/plugins/auth-backend/src/service/OidcRouter.ts b/plugins/auth-backend/src/service/OidcRouter.ts index 6c39115528..7090fadf0b 100644 --- a/plugins/auth-backend/src/service/OidcRouter.ts +++ b/plugins/auth-backend/src/service/OidcRouter.ts @@ -20,6 +20,7 @@ import { AuthService, HttpAuthService, LoggerService, + RootConfigService, } from '@backstage/backend-plugin-api'; import { TokenIssuer } from '../identity/types'; import { UserInfoDatabase } from '../database/UserInfoDatabase'; @@ -33,7 +34,7 @@ export class OidcRouter { private readonly auth: AuthService, private readonly appUrl: string, private readonly httpAuth: HttpAuthService, - private readonly enableDynamicClientRegistration: boolean, + private readonly config: RootConfigService, ) {} static create(options: { @@ -45,7 +46,7 @@ export class OidcRouter { userInfo: UserInfoDatabase; oidc: OidcDatabase; httpAuth: HttpAuthService; - enableDynamicClientRegistration: boolean; + config: RootConfigService; }) { return new OidcRouter( OidcService.create(options), @@ -53,7 +54,7 @@ export class OidcRouter { options.auth, options.appUrl, options.httpAuth, - options.enableDynamicClientRegistration, + options.config, ); } @@ -97,7 +98,11 @@ export class OidcRouter { res.json(userInfo); }); - if (this.enableDynamicClientRegistration) { + if ( + this.config.getOptionalBoolean( + 'auth.experimentalDynamicClientRegistration.enabled', + ) + ) { // Authorization endpoint // https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest // Handles the initial authorization request from the client, validates parameters, @@ -140,7 +145,7 @@ export class OidcRouter { // the plugin is mounted somewhere else? // support slashes in baseUrl? const authSessionRedirectUrl = new URL( - `/auth/sessions/${result.id}`, + `/oauth2/authorize/${result.id}`, this.appUrl, ); diff --git a/plugins/auth-backend/src/service/OidcService.test.ts b/plugins/auth-backend/src/service/OidcService.test.ts index b1f03c68ed..7829be3841 100644 --- a/plugins/auth-backend/src/service/OidcService.test.ts +++ b/plugins/auth-backend/src/service/OidcService.test.ts @@ -63,6 +63,8 @@ describe('OidcService', () => { getUserInfo: jest.fn(), } as unknown as jest.Mocked; + const mockConfig = mockServices.rootConfig.mock(); + return { service: OidcService.create({ auth: mockAuth, @@ -70,11 +72,13 @@ describe('OidcService', () => { baseUrl: 'http://mock-base-url', userInfo: mockUserInfo, oidc: oidcDatabase, + config: mockConfig, }), mocks: { auth: mockAuth, tokenIssuer: mockTokenIssuer, userInfo: mockUserInfo, + config: mockConfig, }, }; } @@ -216,6 +220,44 @@ describe('OidcService', () => { expect(client.clientSecret).toBeDefined(); }); + it('should throw an error for invalid redirect URI', async () => { + const { + service, + mocks: { config }, + } = await createOidcService(databaseId); + + config.getOptionalStringArray.mockReturnValue([ + 'https://example.com/*', + ]); + + await expect( + service.registerClient({ + clientName: 'Test Client', + redirectUris: ['https://invalid.com/callback'], + }), + ).rejects.toThrow('Invalid redirect_uri'); + }); + + it('should create a new client with valid redirect URI', async () => { + const { + service, + mocks: { config }, + } = await createOidcService(databaseId); + + config.getOptionalStringArray.mockReturnValue(['cursor://*']); + + const client = await service.registerClient({ + clientName: 'Test Client', + redirectUris: ['cursor://callback'], + }); + + expect(client).toEqual( + expect.objectContaining({ + redirectUris: ['cursor://callback'], + }), + ); + }); + it('should create a client with default values', async () => { const { service } = await createOidcService(databaseId); diff --git a/plugins/auth-backend/src/service/OidcService.ts b/plugins/auth-backend/src/service/OidcService.ts index 2b7eb40bc9..3139ccc73d 100644 --- a/plugins/auth-backend/src/service/OidcService.ts +++ b/plugins/auth-backend/src/service/OidcService.ts @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { AuthService } from '@backstage/backend-plugin-api'; +import { AuthService, RootConfigService } from '@backstage/backend-plugin-api'; import { TokenIssuer } from '../identity/types'; import { UserInfoDatabase } from '../database/UserInfoDatabase'; import { @@ -33,6 +33,7 @@ export class OidcService { private readonly baseUrl: string, private readonly userInfo: UserInfoDatabase, private readonly oidc: OidcDatabase, + private readonly config: RootConfigService, ) {} static create(options: { @@ -41,6 +42,7 @@ export class OidcService { baseUrl: string; userInfo: UserInfoDatabase; oidc: OidcDatabase; + config: RootConfigService; }) { return new OidcService( options.auth, @@ -48,6 +50,7 @@ export class OidcService { options.baseUrl, options.userInfo, options.oidc, + options.config, ); } @@ -116,8 +119,21 @@ export class OidcService { const generatedClientId = crypto.randomUUID(); const generatedClientSecret = crypto.randomUUID(); - // todo(blam): add validation for redirectUris here. - // should be a list of urls and / or allowed schemes or something. + const allowedRedirectUriPatterns = this.config.getOptionalStringArray( + 'auth.experimentalDynamicClientRegistration.allowedRedirectUriPatterns', + ); + + if (allowedRedirectUriPatterns) { + for (const redirectUri of opts.redirectUris ?? []) { + if ( + !allowedRedirectUriPatterns.some(pattern => + new RegExp(pattern).test(redirectUri), + ) + ) { + throw new InputError('Invalid redirect_uri'); + } + } + } return await this.oidc.createClient({ clientId: generatedClientId, diff --git a/plugins/auth-backend/src/service/router.ts b/plugins/auth-backend/src/service/router.ts index 0d5b26078d..0f0d5b2830 100644 --- a/plugins/auth-backend/src/service/router.ts +++ b/plugins/auth-backend/src/service/router.ts @@ -162,10 +162,7 @@ export async function createRouter( oidc, logger, httpAuth, - enableDynamicClientRegistration: - config.getOptionalBoolean( - 'auth.experimentalDynamicClientRegistration.enabled', - ) ?? false, + config, }); router.use(oidcRouter.getRouter());