C#: Improve precision of cs/uncontrolled-format-string.
#19271
Conversation
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
|
|
||
| final class FormatStringParseCall = FormatStringParseCallImpl; | ||
|
|
||
| private class OrdinaryFormatCall extends FormatStringParseCallImpl instanceof FormatCall { |
There was a problem hiding this comment.
Perhaps just let FormatCall extend FormatStringParseCallImpl directly?
There was a problem hiding this comment.
Yes, that makes sense as well, I think. The last two commits are the changes since you reviewed last time.
There was a problem hiding this comment.
Ah, I misread your comment - and added the parse call as a FormatCall instead. Let me know, whether you think we should keep it like this (it does simplify the code).
There was a problem hiding this comment.
It looks like your rewrite changed the expected test output, is that good/expected?
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 |
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.