[CodeQL] Suppress Random Dictionary Key Warning in SqlDependencyUtils#4276
[CodeQL] Suppress Random Dictionary Key Warning in SqlDependencyUtils#4276benrr101 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to address a CodeQL alert in SqlDependencyUtils by suppressing a warning related to “randomly generated” identifiers used during query notification correlation, rather than changing the identifier generation logic.
Changes:
- Normalized whitespace in a couple of comments.
- Added a CodeQL suppression justification comment near the
notificationIdmapping logic inAddCommandEntry.
| _commandHashToNotificationId.Add(commandHash, notificationId); // CodeQL [SM04207] This value is an opaque query-notification correlation identifier, not a secret or security token. It is used only for uniqueness and exact dictionary lookup after SQL Server round-trips the user data. Guid.NewGuid provides sufficient collision resistance for the expected in-process notification cardinality, and changing the generator would not materially improve security. | ||
| _notificationIdToDependenciesHash.Add(notificationId, dependencyList); |
There was a problem hiding this comment.
Don't ask me. This is the line CodeQL is complaining about 🤷♂️
There was a problem hiding this comment.
@benrr101 What you've added on line 219 looks correct to me.
|
|
||
| // map command hash to notification we just created to reuse it for the next client | ||
| _commandHashToNotificationId.Add(commandHash, notificationId); | ||
| _commandHashToNotificationId.Add(commandHash, notificationId); // CodeQL [SM04207] This value is an opaque query-notification correlation identifier, not a secret or security token. It is used only for uniqueness and exact dictionary lookup after SQL Server round-trips the user data. Guid.NewGuid provides sufficient collision resistance for the expected in-process notification cardinality, and changing the generator would not materially improve security. |
There was a problem hiding this comment.
The suppression comment formats accepted by CodeQL are all ugly. It's silly that we can't use multi-line comments to avoid these hugely long lines.
I worked around it it here:
You can try that format if you want.
There was a problem hiding this comment.
Yeah, I agree it is ugly. But I really don't want to kick off a full rebuild and re-review just to tweak the formatting a tiny bit.
Description
This PR addresses a CodeQL issue that was raised recently. The issue is that we are using "randomly generated" dictionary keys. Technically this is true, as the keys we're using in this case is partially generated by a
Guid.NewGuid()call. The "fix" for this is to replace theGuid.NewGuid()calls with cryptographically secure random number generation. However, there's little reason to go to this trouble - even with hundreds of thousands of IDs being generated, the risk of a collision is exceedingly small. Therefore, rather than rewrite the key generation (and likely all the other instances where IDs are generated by GUIDs), I've opted to suppress this specific line of code.Issues
CodeQL issue.
🤖
Suppression message generated by 🤖