Skip to content

[CodeQL] Suppress Random Dictionary Key Warning in SqlDependencyUtils #4276

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.

Sign up for GitHub

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

Open
benrr101 wants to merge 1 commit into
base: main
Choose a base branch
from
+3 −3
Open

[CodeQL] Suppress Random Dictionary Key Warning in SqlDependencyUtils #4276

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDependencyUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ private SqlDependencyPerAppDomainDispatcher()
#if NETFRAMEWORK
_timeoutTimer = new Timer(new TimerCallback(TimeoutTimerCallback), null, Timeout.Infinite, Timeout.Infinite);

// If rude abort - we'll leak. This is acceptable for now.
// If rude abort - we'll leak. This is acceptable for now.
AppDomain.CurrentDomain.DomainUnload += new EventHandler(UnloadEventHandler);
#else
_timeoutTimer = ADP.UnsafeCreateTimer(
Expand Down Expand Up @@ -131,7 +131,7 @@ private void UnloadEventHandler(object sender, EventArgs e)
}
}

// When remoted across appdomains, MarshalByRefObject links by default time out if there is no activity
// When remoted across appdomains, MarshalByRefObject links by default time out if there is no activity
// within a few minutes. Add this override to prevent marshaled links from timing out.
#if NET
[Obsolete("InitializeLifetimeService() is not supported after .Net5.0 and throws PlatformNotSupportedException.")]
Expand Down Expand Up @@ -216,7 +216,7 @@ internal string AddCommandEntry(string commandHash, SqlDependency dep)
dependencyList.Add(dep);

// 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.
Copy link
Copy Markdown
Contributor

@paulmedynski paulmedynski May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

SqlClient/src/Microsoft.Data.SqlClient.Extensions/Azure/src/ActiveDirectoryAuthenticationProvider.cs

Line 756 in dc19dcd

// SqlClient is a library and provides support to acquire access

You can try that format if you want.

Copy link
Copy Markdown
Contributor Author

@benrr101 benrr101 May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_notificationIdToDependenciesHash.Add(notificationId, dependencyList);
Comment on lines +219 to 220
Copy link
Copy Markdown
Contributor Author

@benrr101 benrr101 May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't ask me. This is the line CodeQL is complaining about 🤷‍♂️

Copy link
Copy Markdown
Member

@cheenamalhotra cheenamalhotra May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benrr101 What you've added on line 219 looks correct to me.

}

Expand Down
Loading