Fix credscan bugs - #40454
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
| { | ||
| // Apple requires all private keys to be exported encrypted, but since we're trying to export | ||
| // as parsed structures we will need to decrypt it for the user. | ||
| // [SuppressMessage("Microsoft.Security", "CS002:SecretInNextLine", Justification="Unit test password.")] |
There was a problem hiding this comment.
This code, and a few other places, are not part of a unit test. Perhaps the comment above can be incorporated as the justification?
There was a problem hiding this comment.
Unfortunately it is not possible to use multiline justification. I added short justification with reference to the source.
dff8816 to
0199c3b
Compare
0199c3b to
9e1eb44
Compare
| "src/libraries/System.Security.Cryptography.Xml/tests/TestHelpers.cs" | ||
| "src/libraries/System.Security.Cryptography.Xml/tests/TestHelpers.cs", | ||
| "src/libraries/System.Security.Cryptography.X509Certificates/tests/PfxTests.cs", | ||
| "src/libraries/System.Management/src/System/Management/ManagementScope.cs", |
There was a problem hiding this comment.
This file, according to the path, doesn't look like a test code. Is there a reason we cannot do inline suppression for it instead?
ManickaP
left a comment
ManickaP
left a comment
There was a problem hiding this comment.
Small comments, otherwise LGTM. Thanks!
|
|
||
| internal static ECParameters ExportPublicParametersFromPrivateKey(SafeSecKeyRefHandle handle) | ||
| { | ||
| // [SuppressMessage("Microsoft.Security", "CS002:SecretInNextLine", Justification="Password for temporary operation. See code comments for more details.")] |
There was a problem hiding this comment.
See code comments for more details.
There're no comments though.
|
@aik-jahoda is this PR still important? Be sure to rerun tests if you want to merge it. |
|
ping @aik-jahoda 😄 Do we still need this PR or can we go ahead and close? |
|
Closing as we have new credscan effort and we need reevaluate the approach |
ghost
locked as resolved and limited conversation to collaborators
8 participants
follow up of #38026