Skip to content

Update precision java concatenated command line #19723

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

apsscolari
Copy link
Contributor

Updated precision to medium because this query is producing false positives when hard coded strings are used in the concatenated string of the command line.

This query is generating False positives with hard coded strings declared within the function - issue reported by customer. We had a discussion on code_scanning channel on 6/5/25 and the team agreed upon reducing its precision to Medium.
@Copilot Copilot AI review requested due to automatic review settings June 11, 2025 03:35
@apsscolari apsscolari requested a review from a team as a code owner June 11, 2025 03:35
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 lowers the precision of the java/concatenated-command-line query from high to medium to reduce false positives with hard-coded strings.

  • Updated change note documenting the precision adjustment.
  • Modified the @precision tag in the QL rule file.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
java/ql/src/change-notes/2025-06-10-reduce-precision-for-building-cmdline-with-string-concatenation.md Added a note about lowering precision due to hard-coded string false positives.
java/ql/src/Security/CWE/CWE-078/ExecUnescaped.ql Changed @precision from high to medium in metadata.

Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Looks good except the typo that copilot spotted. Will approve when that is fixed.

@michaelnebel
Copy link
Contributor

michaelnebel commented Jun 11, 2025

If the precision is changed the query will be removed from the code-scanning query suite. Perhaps we should open an issue for addressing the problem with the query, but keep the precision at high?
(for some reason the integration tests didn't run - they would have failed as the code-scanning suite changes due to the change in precision).

@owen-mc
Copy link
Contributor

owen-mc commented Jun 11, 2025

@michaelnebel Since we don't have the time to work on this now, I suggest we accept the lower precision and make an issue to improve the precision in future.

apsscolari and others added 2 commits June 11, 2025 08:55
…ing-cmdline-with-string-concatenation.md

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ing-cmdline-with-string-concatenation.md

Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>
@owen-mc
Copy link
Contributor

owen-mc commented Jun 11, 2025

We recently added integration tests to make it harder to accidentally change what is included in the codeql query suites without realising it. You need to edit java/ql/integration-tests/java/query-suite/java-code-scanning.qls.expected to remove this query from the list (line 15). Then the integration test will pass.

@apsscolari
Copy link
Contributor Author

hey @owen-mc all checks have passed now, do I have your approval now?
thank you!

@michaelnebel
Copy link
Contributor

If the precision is changed the query will be removed from the code-scanning query suite. Perhaps we should open an issue for addressing the problem with the query, but keep the precision at high? (for some reason the integration tests didn't run - they would have failed as the code-scanning suite changes due to the change in precision).

I realise that this was already discussed in Slack and the decision is to lower the precision for now and then perhaps improve the query in the future 😄

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

LGTM!

@owen-mc owen-mc merged commit 23cbc6a into github:main Jun 12, 2025
18 checks passed
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