-
Notifications
You must be signed in to change notification settings - Fork 1.7k
C#: Improve precision of cs/uncontrolled-format-string
.
#19271
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
C#: Improve precision of cs/uncontrolled-format-string
.
#19271
Conversation
385674a
to
2118c8e
Compare
2118c8e
to
c35a212
Compare
DCA looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refines the detection of uncontrolled format strings in C# by updating tests to use the inline expectations framework, removing the unnecessary hasInsertions check, and adding support for detecting calls to CompositeFormat.Parse.
- Refactored tests to use inline expectations markers
- Removed the hasInsertions check based on recent improvements
- Added CompositeFormat.Parse as a format-like method call to improve precision
Reviewed Changes
Copilot reviewed 4 out of 9 changed files in this pull request and generated no comments.
File | Description |
---|---|
csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatStringBad.cs | Updated inline expectations for source and alert comments |
csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.cs | Added inline expectations markers and support for CompositeFormat.Parse |
csharp/ql/test/query-tests/Security Features/CWE-134/ConsoleUncontrolledFormatString.cs | Added inline expectations markers for Console-based tests |
csharp/ql/src/change-notes/2025-04-10-uncontrolled-format-string.md | Documented the improvement in format string precision |
Files not reviewed (5)
- csharp/ql/lib/semmle/code/csharp/frameworks/Format.qll: Language not supported
- csharp/ql/src/API Abuse/FormatInvalid.ql: Language not supported
- csharp/ql/src/Security Features/CWE-134/UncontrolledFormatString.ql: Language not supported
- csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.expected: Language not supported
- csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.qlref: Language not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only one suggestion. Also the description mentions removal of a hasInsertions
check, but I don't see that happening?
c35a212
to
382ebb1
Compare
It was removed here: https://github.com/github/codeql/pull/19271/files#diff-e6a27e1fa0d22fa846c7cf2887e3f377ad64a33c2ffc646a0575d7439dc657f3R22 |
382ebb1
to
a7ddfe2
Compare
--- | ||
category: minorAnalysis | ||
--- | ||
* The precision of the query `cs/uncontrolled-format-string` has been improved. Calls to `System.Text.CompositeFormat.Parse` are now considered a format like method call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @coadaflorin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The precision of the query
cs/uncontrolled-format-string
has been improved.
Something to explain the kind of improvements will make it easier to understand which way it goes, less FNs, or less FPs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coadaflorin : The change note has been updated. Could you check and approve the PR in case the update is acceptable? Just for future reference - may I assume that the consumers of change notes, know about the abbreviations FPs, FNs and TPs or is better to spell them out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the change! For simplicity let's spell them out. If someone doesn't know what a false positive is, at least they have a chance of finding something if they google it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This is a follow up of #19148
In this PR we
hasInsertions
check. This can be removed as methods likeConsole.WriteLine(string)
are no longer considered potential format calls (this was fixed in C#: Improvecs/invalid-string-formatting
and add to the Code Quality suite. #19148).CompositeFormat.Parse
as this can cause runtime crashes when provided with an invalid format string.DCA doesn't report any changes to performance or alerts.