diff --git a/api/src/unraid-api/auth/authorization.guard.graphql.spec.ts b/api/src/unraid-api/auth/authorization.guard.graphql.spec.ts index 24be5e0b50..6bf83bb730 100644 --- a/api/src/unraid-api/auth/authorization.guard.graphql.spec.ts +++ b/api/src/unraid-api/auth/authorization.guard.graphql.spec.ts @@ -261,4 +261,13 @@ describe('GraphQL authorization boundary', () => { }); expect(JSON.stringify(result)).not.toContain(provider.clientSecret); }); + it('omits persisted OIDC client secrets from every admin read path', async () => { + const result = await execute( + 'query { oidcProviders { clientSecret } oidcProvider(id: "test-provider") { clientSecret } oidcConfiguration { providers { clientSecret } } settings { sso { oidcProviders { clientSecret } } unified { values } } }', + Role.ADMIN + ); + expect(result.errors).toBeUndefined(); + expect(JSON.stringify(result)).not.toContain(provider.clientSecret); + expect(result.data?.oidcProviders).toEqual([{ clientSecret: null }]); + }); }); diff --git a/api/src/unraid-api/graph/resolvers/settings/settings.resolver.ts b/api/src/unraid-api/graph/resolvers/settings/settings.resolver.ts index f2146cc3f9..d0c42b31b5 100644 --- a/api/src/unraid-api/graph/resolvers/settings/settings.resolver.ts +++ b/api/src/unraid-api/graph/resolvers/settings/settings.resolver.ts @@ -17,7 +17,40 @@ import { import { ApiSettings } from '@app/unraid-api/graph/resolvers/settings/settings.service.js'; import { SsoSettings } from '@app/unraid-api/graph/resolvers/settings/sso-settings.model.js'; import { OidcConfigPersistence } from '@app/unraid-api/graph/resolvers/sso/core/oidc-config.service.js'; -import { OidcProvider } from '@app/unraid-api/graph/resolvers/sso/models/oidc-provider.model.js'; +import { + OidcProvider, + redactOidcClientSecret, +} from '@app/unraid-api/graph/resolvers/sso/models/oidc-provider.model.js'; + +const redactOidcClientSecrets = (values: Record): Record => { + const sso = values.sso; + if (!sso || typeof sso !== 'object' || Array.isArray(sso)) { + return values; + } + + const providers = (sso as Record).providers; + if (!Array.isArray(providers)) { + return values; + } + + return { + ...values, + sso: { + ...(sso as Record), + providers: providers.map((provider) => { + if (!provider || typeof provider !== 'object' || Array.isArray(provider)) { + return provider; + } + + const { clientSecret: _clientSecret, ...safeProvider } = provider as Record< + string, + unknown + >; + return safeProvider; + }), + }, + }; +}; @Resolver(() => Settings) export class SettingsResolver { @@ -120,8 +153,7 @@ export class UnifiedSettingsResolver { }) @ResolveField(() => GraphQLJSON) async values() { - // Unified settings include persisted OIDC client secrets. - return this.userSettings.getAllValues(); + return redactOidcClientSecrets(await this.userSettings.getAllValues()); } @Mutation(() => UpdateSettingsResponse) @@ -132,14 +164,14 @@ export class UnifiedSettingsResolver { async updateSettings( @Args('input', { type: () => GraphQLJSON }) input: Record ): Promise { - this.logger.verbose('Updating Settings %O', input); + this.logger.verbose('Updating Settings %O', redactOidcClientSecrets(input)); const { restartRequired, values } = await this.userSettings.updateNamespacedValues(input); - this.logger.verbose('Updated Setting Values %O', values); + this.logger.verbose('Updated Setting Values %O', redactOidcClientSecrets(values)); if (restartRequired) { // hack: allow time for pending writes to flush this.lifecycleService.restartApi({ delayMs: 300 }); } - return { restartRequired, values }; + return { restartRequired, values: redactOidcClientSecrets(values) }; } } @@ -153,6 +185,7 @@ export class SsoSettingsResolver { }) @ResolveField(() => [OidcProvider], { description: 'List of configured OIDC providers' }) async oidcProviders(): Promise { - return this.oidcConfig.getProviders(); + const providers = await this.oidcConfig.getProviders(); + return providers.map(redactOidcClientSecret); } } diff --git a/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.ts b/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.ts index deee4335af..35c0cbad1a 100644 --- a/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.ts +++ b/api/src/unraid-api/graph/resolvers/sso/core/oidc-config.service.ts @@ -282,11 +282,21 @@ export class OidcConfigPersistence extends ConfigFilePersister { } // Clean up the provider object - remove UI-only fields + const existingProvider = + providers.find((currentProvider) => currentProvider.id === provider.id) || + providers.find( + (currentProvider) => + currentProvider.clientId === provider.clientId && + (currentProvider.issuer ?? '') === (provider.issuer ?? '') && + (currentProvider.authorizationEndpoint ?? '') === + (provider.authorizationEndpoint ?? '') && + (currentProvider.tokenEndpoint ?? '') === (provider.tokenEndpoint ?? '') + ); const cleanedProvider: OidcProvider = { id: provider.id, name: provider.name, clientId: provider.clientId, - clientSecret: provider.clientSecret, + clientSecret: provider.clientSecret ?? existingProvider?.clientSecret, usePkce: provider.usePkce, issuer: provider.issuer, authorizationEndpoint: provider.authorizationEndpoint, @@ -300,7 +310,7 @@ export class OidcConfigPersistence extends ConfigFilePersister { buttonStyle: provider.buttonStyle, }; - const existingIndex = providers.findIndex((p) => p.id === provider.id); + const existingIndex = existingProvider ? providers.indexOf(existingProvider) : -1; if (existingIndex >= 0) { providers[existingIndex] = cleanedProvider; } else { @@ -416,11 +426,32 @@ export class OidcConfigPersistence extends ConfigFilePersister { >; } ) => { + const currentConfig = + this.configService.get(this.configKey()) || this.defaultConfig(); + // Process each provider to handle simple mode conversion const processedConfig: OidcConfig = { ...config, providers: config.providers.map((provider) => { - const extendedProvider = provider as OidcProvider & { + const existingProvider = + currentConfig.providers.find( + (currentProvider) => currentProvider.id === provider.id + ) || + currentConfig.providers.find( + (currentProvider) => + currentProvider.clientId === provider.clientId && + (currentProvider.issuer ?? '') === (provider.issuer ?? '') && + (currentProvider.authorizationEndpoint ?? '') === + (provider.authorizationEndpoint ?? '') && + (currentProvider.tokenEndpoint ?? '') === + (provider.tokenEndpoint ?? '') + ); + const providerWithSecret = + provider.clientSecret === undefined && + existingProvider?.clientSecret !== undefined + ? { ...provider, clientSecret: existingProvider.clientSecret } + : provider; + const extendedProvider = providerWithSecret as OidcProvider & { authorizationMode?: string; simpleAuthorization?: unknown; }; diff --git a/api/src/unraid-api/graph/resolvers/sso/models/oidc-provider.model.ts b/api/src/unraid-api/graph/resolvers/sso/models/oidc-provider.model.ts index 5f56493b14..469bd3da26 100644 --- a/api/src/unraid-api/graph/resolvers/sso/models/oidc-provider.model.ts +++ b/api/src/unraid-api/graph/resolvers/sso/models/oidc-provider.model.ts @@ -174,3 +174,8 @@ export class OidcProvider { @IsOptional() buttonStyle?: string; } + +export const redactOidcClientSecret = (provider: OidcProvider): Omit => { + const { clientSecret: _clientSecret, ...safeProvider } = provider; + return safeProvider; +}; diff --git a/api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts b/api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts index e269ad8d00..eac9ead99c 100644 --- a/api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts +++ b/api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts @@ -8,7 +8,10 @@ import { UsePermissions } from '@unraid/shared/use-permissions.directive.js'; import { Public } from '@app/unraid-api/auth/public.decorator.js'; import { OidcConfigPersistence } from '@app/unraid-api/graph/resolvers/sso/core/oidc-config.service.js'; import { OidcConfiguration } from '@app/unraid-api/graph/resolvers/sso/models/oidc-configuration.model.js'; -import { OidcProvider } from '@app/unraid-api/graph/resolvers/sso/models/oidc-provider.model.js'; +import { + OidcProvider, + redactOidcClientSecret, +} from '@app/unraid-api/graph/resolvers/sso/models/oidc-provider.model.js'; import { OidcSessionValidation } from '@app/unraid-api/graph/resolvers/sso/models/oidc-session-validation.model.js'; import { PublicOidcProvider } from '@app/unraid-api/graph/resolvers/sso/models/public-oidc-provider.model.js'; import { OidcSessionService } from '@app/unraid-api/graph/resolvers/sso/session/oidc-session.service.js'; @@ -76,7 +79,8 @@ export class SsoResolver { resource: Resource.CONFIG, }) public async oidcProviders(): Promise { - return this.oidcConfig.getProviders(); + const providers = await this.oidcConfig.getProviders(); + return providers.map(redactOidcClientSecret); } @Query(() => OidcProvider, { nullable: true, description: 'Get a specific OIDC provider by ID' }) @@ -87,7 +91,8 @@ export class SsoResolver { public async oidcProvider( @Args('id', { type: () => PrefixedID }) id: string ): Promise { - return this.oidcConfig.getProvider(id); + const provider = await this.oidcConfig.getProvider(id); + return provider ? redactOidcClientSecret(provider) : null; } @Query(() => OidcConfiguration, { description: 'Get the full OIDC configuration (admin only)' }) @@ -98,7 +103,7 @@ export class SsoResolver { public async oidcConfiguration(): Promise { const config = await this.oidcConfig.getConfig(); return { - providers: config?.providers || [], + providers: (config?.providers || []).map(redactOidcClientSecret), defaultAllowedOrigins: config?.defaultAllowedOrigins || [], }; }