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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

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

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.

1 participant