Harden default logging filters for PAR and Authorize endpoints (7.2)

identity-server/src/IdentityServer/Configuration/DependencyInjection/Options/LoggingOptions.cs

@@ -52,18 +52,25 @@ public class LoggingOptions
     public ICollection<string> AuthorizeRequestSensitiveValuesFilter { get; set; } = 
         new HashSet<string>
         {
+            // Secrets and assertions may be passed to the authorize endpoint via PAR
+            OidcConstants.TokenRequest.ClientSecret,
+            OidcConstants.TokenRequest.ClientAssertion,
             OidcConstants.AuthorizeRequest.IdTokenHint
         };
 
     /// <summary>
     /// Gets or sets the collection of keys that will be used to redact sensitive values from a pushed authorization request log.
     /// </summary>
     /// <remarks>Please be aware that initializing this property could expose sensitive information in your logs.</remarks>
+    /// <remarks>Note that pushed authorization parameters are eventually handled by the authorize request pipeline.
+    /// In most cases, changes to this collection should also be made to <see cref="AuthorizeRequestSensitiveValuesFilter"/>
+    /// </remarks>
     public ICollection<string> PushedAuthorizationSensitiveValuesFilter { get; set; } =
         new HashSet<string>
         {
             OidcConstants.TokenRequest.ClientSecret,
-            OidcConstants.TokenRequest.ClientAssertion
+            OidcConstants.TokenRequest.ClientAssertion,
+            OidcConstants.AuthorizeRequest.IdTokenHint
         };
 
     /// <summary>

identity-server/test/IdentityServer.IntegrationTests/Common/IdentityServerPipeline.cs

@@ -392,7 +392,7 @@ public string CreateAuthorizeUrl(
     }
     public async Task<(JsonDocument, HttpStatusCode)> PushAuthorizationRequestAsync(
         Dictionary<string, string> parameters)
-    { 
+    {
         var httpResponse = await BackChannelClient.PostAsync(ParEndpoint,
             new FormUrlEncodedContent(parameters));
         var statusCode = httpResponse.StatusCode;
@@ -423,9 +423,9 @@ public string CreateAuthorizeUrl(
                 { "state", state }
             };
 
-        if(extra != null)
+        if (extra != null)
         {
-            foreach(var (key, value) in extra)
+            foreach (var (key, value) in extra)
             {
                 parameters[key] = value;
             }
@@ -501,15 +501,15 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
     }
 }
 
-public class MockExternalAuthenticationHandler : 
+public class MockExternalAuthenticationHandler :
     IAuthenticationHandler,
     IAuthenticationSignInHandler,
     IAuthenticationRequestHandler
 {
     private readonly IHttpContextAccessor _httpContextAccessor;
     private HttpContext HttpContext => _httpContextAccessor.HttpContext;
 
-    public Func<HttpContext, Task<bool>> OnFederatedSignout = 
+    public Func<HttpContext, Task<bool>> OnFederatedSignout =
         async context =>
         {
             await context.SignOutAsync();

identity-server/test/IdentityServer.IntegrationTests/Common/MockLogger.cs

@@ -16,11 +16,11 @@ public MockLogger(LoggerExternalScopeProvider scopeProvider)
     }
 
     public readonly List<string> LogMessages = new();
-    
-    
+
+
     private readonly LoggerExternalScopeProvider _scopeProvider;
-    
-    
+
+
     public IDisposable BeginScope<TState>(TState state) where TState : notnull => _scopeProvider.Push(state);
 
     public bool IsEnabled(LogLevel logLevel) => true;

identity-server/test/IdentityServer.IntegrationTests/Endpoints/Authorize/PushedAuthorizationTests.cs

@@ -22,14 +22,15 @@ public class PushedAuthorizationTests
 {
     private readonly IdentityServerPipeline _mockPipeline = new();
     private Client _client;
-
+    private string clientSecret = Guid.NewGuid().ToString();
+
     public PushedAuthorizationTests()
     {
         ConfigureClients();
         ConfigureUsers();
         ConfigureScopesAndResources();
 
-        _mockPipeline.Initialize();
+        _mockPipeline.Initialize(enableLogging: true);
 
         _mockPipeline.Options.Endpoints.EnablePushedAuthorizationEndpoint = true;
     }
@@ -65,12 +66,32 @@ public async Task happy_path()
         authorization.State.Should().Be(expectedState);
     }
 
+    [Fact]
+    public async Task sensitive_values_should_not_be_logged_on_bad_request_to_par_endpoint()
+    {
+        // Login
+        await _mockPipeline.LoginAsync("bob");
+        _mockPipeline.BrowserClient.AllowAutoRedirect = false;
+
+        // Push Authorization
+        var expectedCallback = _client.RedirectUris.First();
+        var expectedState = "123_state";
+        var (parJson, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync(
+            clientSecret: clientSecret,
+            redirectUri: "bogus", // <-- Intentionally wrong, to provoke logging an error with raw request
+            state: expectedState
+        );
+
+        _mockPipeline.MockLogger.LogMessages.Should().ContainMatch("*\"client_secret\": \"***REDACTED***\"*");
+        _mockPipeline.MockLogger.LogMessages.Should().NotContainMatch(clientSecret);
+    }
+
     [Fact]
     public async Task using_pushed_authorization_when_it_is_globally_disabled_fails()
     {
         _mockPipeline.Options.Endpoints.EnablePushedAuthorizationEndpoint = false;
 
-        var (_, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync();
+        var (_, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync(clientSecret: clientSecret);
         statusCode.Should().Be(HttpStatusCode.NotFound);
     }
 
@@ -120,7 +141,7 @@ public async Task not_using_pushed_authorization_when_it_is_required_for_client_
     public async Task existing_pushed_authorization_request_uris_become_invalid_when_par_is_disabled()
     {
         // PAR is enabled when we push authorization...
-        var (parJson, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync();
+        var (parJson, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync(clientSecret: clientSecret);
         statusCode.Should().Be(HttpStatusCode.Created);
         parJson.Should().NotBeNull();
 
@@ -148,7 +169,7 @@ public async Task reusing_pushed_authorization_request_uris_fails()
         // Login
         await _mockPipeline.LoginAsync("bob");
 
-        var (parJson, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync();
+        var (parJson, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync(clientSecret: clientSecret);;
         statusCode.Should().Be(HttpStatusCode.Created);
         parJson.Should().NotBeNull();
 
@@ -286,7 +307,7 @@ private void ConfigureClients()
                 ClientId = "client1",
                 ClientSecrets = new []
                 {
-                     new Secret("secret".Sha256())
+                     new Secret(clientSecret.Sha256())
                 },
                 AllowedGrantTypes = GrantTypes.Implicit,
                 RequireConsent = false,