Merge pull request #25711 from gdlcf88/fix/openiddict-clientid-leak-into-cookie

Prevent OpenIddict client_id from leaking into the interactive auth cookie

modules/openiddict/Volo.Abp.OpenIddict.slnx

@@ -16,6 +16,7 @@
     <Project Path="src/Volo.Abp.PermissionManagement.Domain.OpenIddict/Volo.Abp.PermissionManagement.Domain.OpenIddict.csproj" />
   </Folder>
   <Folder Name="/test/">
+    <Project Path="test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo.Abp.OpenIddict.AspNetCore.Tests.csproj" />
     <Project Path="test/Volo.Abp.OpenIddict.Domain.Tests/Volo.Abp.OpenIddict.Domain.Tests.csproj" />
     <Project Path="test/Volo.Abp.OpenIddict.EntityFrameworkCore.Tests/Volo.Abp.OpenIddict.EntityFrameworkCore.Tests.csproj" />
     <Project Path="test/Volo.Abp.OpenIddict.MongoDB.Tests/Volo.Abp.OpenIddict.MongoDB.Tests.csproj" />

modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs

@@ -1,4 +1,5 @@
-using Microsoft.AspNetCore.Mvc.Razor;
+using Microsoft.AspNetCore.Identity;
+using Microsoft.AspNetCore.Mvc.Razor;
 using Microsoft.Extensions.DependencyInjection;
 using OpenIddict.Abstractions;
 using OpenIddict.Server;
@@ -39,6 +40,11 @@ public class AbpOpenIddictAspNetCoreModule : AbpModule
         {
             options.ViewLocationFormats.Add("/Volo/Abp/OpenIddict/Views/{1}/{0}.cshtml");
         });
+
+        Configure<SecurityStampValidatorOptions>(options =>
+        {
+            options.RemoveClientIdClaim();
+        });
     }
 
     private void AddOpenIddictServer(IServiceCollection services)

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);
+            }
+        }
+    }
+}

modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo.Abp.OpenIddict.AspNetCore.Tests.csproj

@@ -0,0 +1,21 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <Import Project="..\..\..\..\common.props" />
+
+  <PropertyGroup>
+    <TargetFramework>net10.0</TargetFramework>
+    <RootNamespace />
+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="Microsoft.NET.Test.Sdk" />
+    <PackageReference Include="xunit" />
+    <PackageReference Include="xunit.runner.visualstudio" />
+    <PackageReference Include="Shouldly" />
+  </ItemGroup>
+
+  <ItemGroup>
+    <ProjectReference Include="..\..\src\Volo.Abp.OpenIddict.AspNetCore\Volo.Abp.OpenIddict.AspNetCore.csproj" />
+  </ItemGroup>
+
+</Project>

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<SecurityStampRefreshingPrincipalContext, Task> 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));
+    }
+}