C#: Improve precision of cs/uncontrolled-format-string.#19271
C#: Improve precision of cs/uncontrolled-format-string.#19271michaelnebel merged 7 commits intogithub:mainfrom
cs/uncontrolled-format-string.#19271Conversation
michaelnebel
force-pushed
the
csharp/uncontrolled-format-string
branch
from
385674a to
2118c8e
Compare
michaelnebel
force-pushed
the
csharp/uncontrolled-format-string
branch
from
2118c8e to
c35a212
Compare
|
DCA looks good. |
There was a problem hiding this comment.
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
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
LGTM, only one suggestion. Also the description mentions removal of a hasInsertions check, but I don't see that happening?
michaelnebel
force-pushed
the
csharp/uncontrolled-format-string
branch
from
c35a212 to
382ebb1
Compare
It was removed here: https://github.com/github/codeql/pull/19271/files#diff-e6a27e1fa0d22fa846c7cf2887e3f377ad64a33c2ffc646a0575d7439dc657f3R22 |
michaelnebel
force-pushed
the
csharp/uncontrolled-format-string
branch
from
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.
The precision of the query
cs/uncontrolled-format-stringhas 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.
@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.
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.
3 participants
This is a follow up of #19148
In this PR we
hasInsertionscheck. This can be removed as methods likeConsole.WriteLine(string)are no longer considered potential format calls (this was fixed in C#: Improvecs/invalid-string-formattingand add to the Code Quality suite. #19148).CompositeFormat.Parseas this can cause runtime crashes when provided with an invalid format string.DCA doesn't report any changes to performance or alerts.