Skip to content

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

Merged

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Apr 10, 2025

This is a follow up of #19148

In this PR we

  • Re-factor tests to use the inline expectations framework.
  • Remove the hasInsertions check. This can be removed as methods like Console.WriteLine(string) are no longer considered potential format calls (this was fixed in C#: Improve cs/invalid-string-formatting and add to the Code Quality suite. #19148).
  • Add support for 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.

@michaelnebel michaelnebel force-pushed the csharp/uncontrolled-format-string branch from 385674a to 2118c8e Compare April 10, 2025 12:37
@michaelnebel michaelnebel force-pushed the csharp/uncontrolled-format-string branch from 2118c8e to c35a212 Compare April 24, 2025 08:42
@michaelnebel
Copy link
Contributor Author

DCA looks good.

@michaelnebel michaelnebel marked this pull request as ready for review April 24, 2025 12:39
@Copilot Copilot AI review requested due to automatic review settings April 24, 2025 12:39
@michaelnebel michaelnebel requested a review from a team as a code owner April 24, 2025 12:39
Copy link
Contributor

@Copilot Copilot AI left a 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

hvitved
hvitved previously approved these changes Apr 29, 2025
Copy link
Contributor

@hvitved hvitved left a 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?

@michaelnebel
Copy link
Contributor Author

LGTM, only one suggestion. Also the description mentions removal of a hasInsertions check, but I don't see that happening?

It was removed here: https://github.com/github/codeql/pull/19271/files#diff-e6a27e1fa0d22fa846c7cf2887e3f377ad64a33c2ffc646a0575d7439dc657f3R22

@michaelnebel michaelnebel force-pushed the csharp/uncontrolled-format-string branch from 382ebb1 to a7ddfe2 Compare May 12, 2025 14:06
---
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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

@coadaflorin coadaflorin May 14, 2025

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.

@michaelnebel michaelnebel requested a review from coadaflorin May 13, 2025 09:45
hvitved
hvitved previously approved these changes May 13, 2025
Copy link
Contributor

@coadaflorin coadaflorin left a comment

Choose a reason for hiding this comment

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

LGTM!

@michaelnebel michaelnebel merged commit 7531a95 into github:main May 14, 2025
23 checks passed
@michaelnebel michaelnebel deleted the csharp/uncontrolled-format-string branch May 14, 2025 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants