-
Notifications
You must be signed in to change notification settings - Fork 345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BackchannelAuthenticationRequestValidator inserts an unwanted string format item when calling LogError #1345
Comments
Thanks for this bug report. I haven't been able to reproduce exactly the same behavior, but I suspect that the differences I'm seeing come from implementation details of the logging provider. I'm using Serilog, and what I see in my testing is that the bad log messages are simply discarded rather than getting an error written to the log. I have a fix in progress that looks like it resolves the issue. Can you give us a sense of how often these bad log messages are showing up, and how much of an impact this is having for you? Also, which log provider are you using? |
This fixes #1345, which happened because the passed in log message contained a placeholder, and LogWithRequestDetails also added a placeholder for the values, resulting in too many placeholders. The fix is to remove the extra placeholder from LogWithRequestDetails and review all calls to it to ensure the passed log message always contains placeholders for the values.
Hi Joseph, Thank you for taking a look at this issue. I'll address each of the questions you had for us below: 1. Can you give us a sense of how often these bad log messages are showing up? 2. How much of an impact is this having for you? 3. Which log provider are you using? Let me know if there's anything else you need from us :) |
@Hoddikr please have a look at the PR to see if you feel that will address your issue. Thanks |
@brockallen @josephdecock Looks like this should do it, thanks. |
@Hoddikr Are you requesting that we patch this on 6.2.x, or are you ok waiting for 7.0? |
@brockallen 7.0 would be OK. |
Which version of Duende IdentityServer are you using?
6.2.3
Which version of .NET are you using?
.NET 6
Describe the bug
BackchannelAuthenticationRequestValidator calls LogError with a string format item which results in an exception when LogWithRequestDetails is called.
A permalink directly to the line that causes the error:
https://github.com/DuendeSoftware/IdentityServer/blob/d933a168074d7248226241acd1b96d1910baf8bd/src/IdentityServer/Validation/Default/BackchannelAuthenticationRequestValidator.cs#LL410C9-L410C9
To Reproduce
Implement IBackchannelAuthenticationUserValidator and make a ciba request that results in any Error value that is not one of the below:
Actual behavior
LogError calls LogWithRequestDetails with the string format item "{error}" within the message which will result in the following error being logged:
Expected behavior
LogError logs the error with the Error value from BackchannelAuthenticationUserValidationResult.Error :
Additional context
This arises when we have an implementation of IBackchannelAuthenticationUserValidator where ValidateRequestAsync populates the field BackchannelAuthenticationUserValidationResult.Error with a value that is not one of the below:
This does not cause any actual errors or unexpected behavior in the ciba flow itself, but the error logs do not log the expected errors because of the additional string format item.
The text was updated successfully, but these errors were encountered: