Conversation
bhazen
force-pushed
the
beh/sanitized-logging
branch
from
c325f2c to
3cc4bb2
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
identity-server/src/IdentityServer/Stores/Default/DefaultGrantStore.cs
Dismissed
Show dismissed
Hide dismissed
| _logger.LogError("Invalid content type {type} from jwt url {url}", | ||
| response.Content.Headers.ContentType.MediaType, url); | ||
| _sanitizedLogger.LogError("Invalid content type {type} from jwt url {url}", | ||
| response.Content.Headers.ContentType.MediaType, url.ReplaceLineEndings(string.Empty)); |
There was a problem hiding this comment.
The call to ReplaceLineEndings is unnecessary here, since all log arguments are sanitized using ILoggerExtensions.SanitizeLogParameter, which already calls ReplaceLineEndings() internally.
| Factory.CreateRequestObjectValidator(), | ||
| new LicenseUsageTracker(new LicenseAccessor(new IdentityServerOptions(), NullLogger<LicenseAccessor>.Instance)), | ||
| TestLogger.Create<AuthorizeRequestValidator>()); | ||
| new SanitizedLogger<AuthorizeRequestValidator>(TestLogger.Create<AuthorizeRequestValidator>())); |
There was a problem hiding this comment.
Not sure what your code conventions are, but I would much prefer to avoid the duplicate generic type usage, and create a static method SanitizedLogger.Wrap<T>(ILogger<T> logger) which could then be used like this (due to the compiler being able to infer the generic type from the argument):
| new SanitizedLogger<AuthorizeRequestValidator>(TestLogger.Create<AuthorizeRequestValidator>())); | |
| SanitizedLogger.Wrap(TestLogger.Create<AuthorizeRequestValidator>())); |
Of course, the method could also be named CreateFrom() or just From, but you get the point.
|
Question: Did this PR solve your CodeQL alerts around unsanitized input for good? Because for us, using a cleansing extension method on the arguments doesn't stop CodeQL from flagging our logging calls. |
|
That has been our experience as well-we still get some codeql noise even after this change. |
4 participants
What issue does this PR address?
Adds a new decorator for
ILoggerto use in IS internals to address log statements where CodeQL was flagging log input which in some cases came from user input. This does not included any changes to the existing extension methods we have onILoggeras we decided to address that separately.Important: Any code or remarks in your Pull Request are under the following terms:
If You provide us with any comments, bug reports, feedback, enhancements, or modifications proposed or suggested by You for the Software, such Feedback is provided on a non-confidential basis (notwithstanding any notice to the contrary You may include in any accompanying communication), and Licensor shall have the right to use such Feedback at its discretion, including, but not limited to the incorporation of such suggested changes into the Software. You hereby grant Licensor a perpetual, irrevocable, transferable, sublicensable, nonexclusive license under all rights necessary to incorporate and use your Feedback for any purpose, including to make and sell any products and services.
(see our license, section 7)