From 30119d7d90c5c5d06e757f75ff66b19a74e13b10 Mon Sep 17 00:00:00 2001 From: maliming Date: Tue, 30 Jun 2026 10:27:57 +0800 Subject: [PATCH] Refactor client_id cookie cleanup into an options extension - move OnRefreshingPrincipal logic to AbpOpenIddictSecurityStampValidatorOptionsExtensions - add unit tests for removal and callback composition order --- .../Properties/AssemblyInfo.cs | 3 - .../AbpOpenIddictAspNetCoreModule.cs | 58 +------ ...SecurityStampValidatorOptionsExtensions.cs | 48 ++++++ ...tyStampValidatorOptionsExtensions_Tests.cs | 154 ++++++++++++++++++ .../OpenIddictCookieClientIdLeak_Tests.cs | 120 -------------- 5 files changed, 205 insertions(+), 178 deletions(-) delete mode 100644 modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Properties/AssemblyInfo.cs create mode 100644 modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions.cs create mode 100644 modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions_Tests.cs delete mode 100644 modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/OpenIddictCookieClientIdLeak_Tests.cs diff --git a/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Properties/AssemblyInfo.cs b/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Properties/AssemblyInfo.cs deleted file mode 100644 index 956f3cb990..0000000000 --- a/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Properties/AssemblyInfo.cs +++ /dev/null @@ -1,3 +0,0 @@ -using System.Runtime.CompilerServices; - -[assembly: InternalsVisibleTo("Volo.Abp.OpenIddict.AspNetCore.Tests")] diff --git a/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs b/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs index 66ef1b5373..26729669e7 100644 --- a/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs +++ b/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs @@ -1,5 +1,4 @@ -using System.Linq; -using Microsoft.AspNetCore.Identity; +using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc.Razor; using Microsoft.Extensions.DependencyInjection; using OpenIddict.Abstractions; @@ -42,63 +41,12 @@ public class AbpOpenIddictAspNetCoreModule : AbpModule options.ViewLocationFormats.Add("/Volo/Abp/OpenIddict/Views/{1}/{0}.cshtml"); }); - ConfigureSecurityStampValidator(context.Services); - } - - /// - /// The adds the ambient authorization - /// request's client_id claim to every principal built by - /// SignInManager.CreateUserPrincipalAsync. That is correct for principals that OpenIddict - /// signs into tokens, but the same method is also used by the cookie security-stamp validator to - /// rebuild and re-issue the interactive authentication cookie. When the cookie happens to be - /// refreshed during a /connect/authorize request, the client_id of the OAuth client - /// being authorized leaks into the cookie and corrupts ICurrentClient.Id (and therefore - /// audit-log client attribution) for every later cookie-authenticated request in that browser. - /// - /// The contributor cannot tell whether the principal it contributes to is destined for a token or - /// for the cookie, so the claim is stripped here, at the only point where the cookie is actually - /// re-written: the security-stamp OnRefreshingPrincipal callback. This never runs for token - /// issuance (which signs into the OpenIddict scheme, not the cookie), so the token path is left - /// untouched. The removal is chained after any previously registered callback (e.g. ABP Identity's - /// SecurityStampValidatorCallback.UpdatePrincipal), so it also self-heals cookies that were - /// already corrupted before this fix. - /// - internal static void ConfigureSecurityStampValidator(IServiceCollection services) - { - services.Configure(options => + Configure(options => { - var previousOnRefreshingPrincipal = options.OnRefreshingPrincipal; - options.OnRefreshingPrincipal = async context => - { - if (previousOnRefreshingPrincipal != null) - { - await previousOnRefreshingPrincipal(context); - } - - // Runs after any previously registered callback (e.g. ABP Identity's - // SecurityStampValidatorCallback.UpdatePrincipal), so an already-corrupted - // cookie that re-introduces client_id during the refresh still gets cleaned. - RemoveClientIdClaimsFromRefreshedPrincipal(context); - }; + options.RemoveClientIdClaim(); }); } - internal static void RemoveClientIdClaimsFromRefreshedPrincipal(SecurityStampRefreshingPrincipalContext context) - { - if (context.NewPrincipal == null) - { - return; - } - - foreach (var identity in context.NewPrincipal.Identities) - { - foreach (var clientIdClaim in identity.FindAll(AbpClaimTypes.ClientId).ToArray()) - { - identity.RemoveClaim(clientIdClaim); - } - } - } - private void AddOpenIddictServer(IServiceCollection services) { var builderOptions = services.ExecutePreConfiguredActions(); diff --git a/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions.cs b/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions.cs new file mode 100644 index 0000000000..fa4fde8a71 --- /dev/null +++ b/modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions.cs @@ -0,0 +1,48 @@ +using System.Linq; +using Microsoft.AspNetCore.Identity; +using Volo.Abp.Security.Claims; + +namespace Volo.Abp.OpenIddict; + +public static class AbpOpenIddictSecurityStampValidatorOptionsExtensions +{ + public static SecurityStampValidatorOptions RemoveClientIdClaim(this SecurityStampValidatorOptions options) + { + // OpenIddictClaimsPrincipalContributor stamps the ambient /connect/authorize request's client_id + // onto every principal built by CreateUserPrincipalAsync. That is meant for the access_token, but the + // cookie security-stamp validator rebuilds the interactive cookie through the same method, so a refresh + // that lands on /connect/authorize leaks client_id into the cookie and corrupts ICurrentClient. + // OnRefreshingPrincipal is the only place the cookie is re-written and never runs for token issuance. + var previousOnRefreshingPrincipal = options.OnRefreshingPrincipal; + options.OnRefreshingPrincipal = async context => + { + // Run the previous callback first: ABP Identity's UpdatePrincipal copies claims that are on the + // current cookie but not on the new principal forward, re-introducing client_id from an + // already-corrupted cookie. Removing it afterwards lets such a cookie self-heal on its next refresh. + if (previousOnRefreshingPrincipal != null) + { + await previousOnRefreshingPrincipal.Invoke(context); + } + + RemoveClientIdClaimsFromPrincipal(context); + }; + + return options; + } + + private static void RemoveClientIdClaimsFromPrincipal(SecurityStampRefreshingPrincipalContext context) + { + if (context.NewPrincipal == null) + { + return; + } + + foreach (var identity in context.NewPrincipal.Identities) + { + foreach (var claim in identity.FindAll(AbpClaimTypes.ClientId).ToArray()) + { + identity.RemoveClaim(claim); + } + } + } +} diff --git a/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions_Tests.cs b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions_Tests.cs new file mode 100644 index 0000000000..4231e23b33 --- /dev/null +++ b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions_Tests.cs @@ -0,0 +1,154 @@ +using System; +using System.Linq; +using System.Security.Claims; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Identity; +using Shouldly; +using Volo.Abp.Security.Claims; +using Xunit; + +namespace Volo.Abp.OpenIddict; + +public class AbpOpenIddictSecurityStampValidatorOptionsExtensions_Tests +{ + [Fact] + public async Task Should_Remove_ClientId_From_Refreshed_Principal() + { + var context = new SecurityStampRefreshingPrincipalContext + { + NewPrincipal = CreateCookiePrincipal( + new Claim(AbpClaimTypes.UserId, "user-1"), + new Claim(AbpClaimTypes.ClientId, "MyClient")) + }; + + await RefreshPrincipalAsync(context); + + context.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty(); + context.NewPrincipal.FindFirst(AbpClaimTypes.UserId).Value.ShouldBe("user-1"); + } + + [Fact] + public async Task Should_Not_Touch_A_Principal_Without_ClientId() + { + var context = new SecurityStampRefreshingPrincipalContext + { + NewPrincipal = CreateCookiePrincipal(new Claim(AbpClaimTypes.UserId, "user-1")) + }; + + await RefreshPrincipalAsync(context); + + context.NewPrincipal.Claims.Count().ShouldBe(1); + context.NewPrincipal.FindFirst(AbpClaimTypes.UserId).Value.ShouldBe("user-1"); + } + + [Fact] + public async Task Should_Remove_Every_ClientId_Claim_From_Every_Identity() + { + var principal = new ClaimsPrincipal(); + principal.AddIdentity(new ClaimsIdentity( + new[] { new Claim(AbpClaimTypes.UserId, "user-1"), new Claim(AbpClaimTypes.ClientId, "Client-A") }, + IdentityConstants.ApplicationScheme)); + principal.AddIdentity(new ClaimsIdentity( + new[] { new Claim(AbpClaimTypes.ClientId, "Client-B"), new Claim(AbpClaimTypes.ClientId, "Client-C") }, + IdentityConstants.ApplicationScheme)); + + var context = new SecurityStampRefreshingPrincipalContext { NewPrincipal = principal }; + + await RefreshPrincipalAsync(context); + + context.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty(); + context.NewPrincipal.FindFirst(AbpClaimTypes.UserId).Value.ShouldBe("user-1"); + } + + [Fact] + public async Task Should_Not_Throw_When_New_Principal_Is_Null() + { + var context = new SecurityStampRefreshingPrincipalContext { NewPrincipal = null }; + + await Should.NotThrowAsync(() => RefreshPrincipalAsync(context)); + } + + [Fact] + public async Task Should_Remove_ClientId_After_Running_The_Previously_Registered_Callback() + { + // The real module order: ABP Identity's callback is registered first, the removal after it. Identity's + // SecurityStampValidatorCallback.UpdatePrincipal copies client_id from an already-corrupted cookie onto + // the refreshed principal; the removal still strips it, so the cookie self-heals on its next refresh. + var previousCallbackRan = false; + Task PreviousCallback(SecurityStampRefreshingPrincipalContext context) + { + previousCallbackRan = true; + CopyClientIdForward(context); + return Task.CompletedTask; + } + + var refreshingContext = CreateCorruptedCookieRefreshContext(); + + await RefreshPrincipalAsync(refreshingContext, PreviousCallback); + + previousCallbackRan.ShouldBeTrue(); + refreshingContext.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty(); + } + + [Fact] + public async Task Should_Remove_ClientId_When_A_Callback_Is_Registered_After_It() + { + // The reverse order: the removal is registered first and an Identity-style callback (which runs its own + // copy-forward before invoking the previous callback) is registered after it. Because the two wrappers + // chain in opposite directions, the removal still runs last, so the order the modules load does not matter. + var options = new SecurityStampValidatorOptions(); + options.RemoveClientIdClaim(); + + var previousOnRefreshingPrincipal = options.OnRefreshingPrincipal; + options.OnRefreshingPrincipal = async context => + { + CopyClientIdForward(context); + if (previousOnRefreshingPrincipal != null) + { + await previousOnRefreshingPrincipal.Invoke(context); + } + }; + + var refreshingContext = CreateCorruptedCookieRefreshContext(); + + await options.OnRefreshingPrincipal(refreshingContext); + + refreshingContext.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty(); + } + + private static async Task RefreshPrincipalAsync( + SecurityStampRefreshingPrincipalContext context, + Func previousCallback = null) + { + var options = new SecurityStampValidatorOptions { OnRefreshingPrincipal = previousCallback }; + + options.RemoveClientIdClaim().ShouldBeSameAs(options); + + await options.OnRefreshingPrincipal(context); + } + + private static void CopyClientIdForward(SecurityStampRefreshingPrincipalContext context) + { + var clientId = context.CurrentPrincipal.FindFirst(AbpClaimTypes.ClientId); + if (clientId != null) + { + context.NewPrincipal.Identities.First().AddClaim(clientId); + } + } + + private static SecurityStampRefreshingPrincipalContext CreateCorruptedCookieRefreshContext() + { + return new SecurityStampRefreshingPrincipalContext + { + CurrentPrincipal = CreateCookiePrincipal( + new Claim(AbpClaimTypes.UserId, "user-1"), + new Claim(AbpClaimTypes.ClientId, "MyClient")), + NewPrincipal = CreateCookiePrincipal(new Claim(AbpClaimTypes.UserId, "user-1")) + }; + } + + private static ClaimsPrincipal CreateCookiePrincipal(params Claim[] claims) + { + return new ClaimsPrincipal(new ClaimsIdentity(claims, IdentityConstants.ApplicationScheme)); + } +} diff --git a/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/OpenIddictCookieClientIdLeak_Tests.cs b/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/OpenIddictCookieClientIdLeak_Tests.cs deleted file mode 100644 index 711009a28c..0000000000 --- a/modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/OpenIddictCookieClientIdLeak_Tests.cs +++ /dev/null @@ -1,120 +0,0 @@ -using System.Linq; -using System.Security.Claims; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Identity; -using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Options; -using Shouldly; -using Volo.Abp.Security.Claims; -using Xunit; - -namespace Volo.Abp.OpenIddict; - -/// -/// Tests for the fix that stops the OAuth client_id of a /connect/authorize request -/// from leaking into the interactive authentication cookie. -/// -/// Background: when the cookie's security stamp happens to be refreshed during a -/// /connect/authorize request, OpenIddictClaimsPrincipalContributor stamps the ambient -/// request's client_id onto the principal that is written back to the cookie. From then on -/// ICurrentClient.Id resolves to that client for every later cookie-authenticated request -/// (corrupting audit-log client attribution). The fix strips client_id from the principal at -/// the security-stamp OnRefreshingPrincipal callback, which only runs when the cookie is -/// re-issued and never for OpenIddict token issuance. -/// -public class OpenIddictCookieClientIdLeak_Tests -{ - [Fact] - public void Should_Remove_ClientId_From_Refreshed_Cookie_Principal() - { - // A cookie principal that was wrongly stamped with client_id while being rebuilt - // by the security-stamp validator during /connect/authorize. - var context = new SecurityStampRefreshingPrincipalContext - { - CurrentPrincipal = CreateCookiePrincipal(new Claim(AbpClaimTypes.UserId, "user-1")), - NewPrincipal = CreateCookiePrincipal( - new Claim(AbpClaimTypes.UserId, "user-1"), - new Claim(AbpClaimTypes.ClientId, "MyClient")) - }; - - AbpOpenIddictAspNetCoreModule.RemoveClientIdClaimsFromRefreshedPrincipal(context); - - context.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty(); - // unrelated claims are preserved - context.NewPrincipal.FindFirst(AbpClaimTypes.UserId)!.Value.ShouldBe("user-1"); - } - - [Fact] - public void Should_Not_Touch_A_Principal_That_Has_No_ClientId() - { - var context = new SecurityStampRefreshingPrincipalContext - { - NewPrincipal = CreateCookiePrincipal(new Claim(AbpClaimTypes.UserId, "user-1")) - }; - - AbpOpenIddictAspNetCoreModule.RemoveClientIdClaimsFromRefreshedPrincipal(context); - - context.NewPrincipal.Claims.Count().ShouldBe(1); - context.NewPrincipal.FindFirst(AbpClaimTypes.UserId)!.Value.ShouldBe("user-1"); - } - - [Fact] - public void Should_Not_Throw_When_New_Principal_Is_Null() - { - var context = new SecurityStampRefreshingPrincipalContext { NewPrincipal = null }; - - Should.NotThrow(() => AbpOpenIddictAspNetCoreModule.RemoveClientIdClaimsFromRefreshedPrincipal(context)); - } - - [Fact] - public async Task Registered_Callback_Should_Strip_ClientId_After_Running_The_Previous_Callback() - { - // Reproduces the real composition order. A previously registered callback - e.g. ABP - // Identity's SecurityStampValidatorCallback.UpdatePrincipal - re-introduces client_id from - // an already-corrupted cookie onto the refreshed principal. The fix is chained AFTER it, so - // the claim is still removed and the cookie self-heals on its next refresh. - var services = new ServiceCollection(); - services.AddOptions(); - - var previousCallbackRan = false; - services.Configure(options => - { - options.OnRefreshingPrincipal = context => - { - previousCallbackRan = true; - var currentClientId = context.CurrentPrincipal!.FindFirst(AbpClaimTypes.ClientId); - if (currentClientId != null) - { - context.NewPrincipal!.Identities.First().AddClaim(currentClientId); - } - - return Task.CompletedTask; - }; - }); - - AbpOpenIddictAspNetCoreModule.ConfigureSecurityStampValidator(services); - - var options = services - .BuildServiceProvider() - .GetRequiredService>() - .Value; - - var context = new SecurityStampRefreshingPrincipalContext - { - CurrentPrincipal = CreateCookiePrincipal( - new Claim(AbpClaimTypes.UserId, "user-1"), - new Claim(AbpClaimTypes.ClientId, "MyClient")), - NewPrincipal = CreateCookiePrincipal(new Claim(AbpClaimTypes.UserId, "user-1")) - }; - - await options.OnRefreshingPrincipal!(context); - - previousCallbackRan.ShouldBeTrue(); - context.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty(); - } - - private static ClaimsPrincipal CreateCookiePrincipal(params Claim[] claims) - { - return new ClaimsPrincipal(new ClaimsIdentity(claims, IdentityConstants.ApplicationScheme)); - } -}