Skip to content

Java: Deprecate experimental queries. #18299

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.

Sign up for GitHub

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

Jump to bottom
Merged
michaelnebel merged 6 commits into from
Jan 29, 2025
Merged

Java: Deprecate experimental queries. #18299

michaelnebel merged 6 commits into from
Jan 29, 2025

Conversation

@michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Dec 16, 2024 •
edited
Loading

In this PR we deprecate all the Java experimental queries as they have been moved to the Code QL Community packs repo: https://github.com/GitHubSecurityLab/CodeQL-Community-Packs/

DCA looks good.

@github-actions github-actions bot added the Java label Dec 16, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 16, 2024
View reviewed changes
@michaelnebel michaelnebel force-pushed the java/deprecateexperimental branch 2 times, most recently from fe1a2a1 to 3fa6d32 Compare December 17, 2024 08:16
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 17, 2024
View reviewed changes
java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.ql
sink0 = sink and
message1 =
"Call to dangerous java.lang.Runtime.exec() with command '$@' with arg from untrusted input '$@'" and
message2 = sourceCmd.toString() and

Check warning

Code scanning / CodeQL

Using 'toString' in query logic Warning

Query logic depends on implementation of 'toString'.
java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.ql
"Call to dangerous java.lang.Runtime.exec() with command '$@' with arg from untrusted input '$@'" and
message2 = sourceCmd.toString() and
sourceNode = source.getNode() and
message3 = source.toString()

Check warning

Code scanning / CodeQL

Using 'toString' in query logic Warning

Query logic depends on implementation of 'toString'.
java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecLocal.ql
sink0 = sink and
message1 =
"Call to dangerous java.lang.Runtime.exec() with command '$@' with arg from untrusted input '$@'" and
message2 = sourceCmd.toString() and

Check warning

Code scanning / CodeQL

Using 'toString' in query logic Warning

Query logic depends on implementation of 'toString'.
java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecLocal.ql
"Call to dangerous java.lang.Runtime.exec() with command '$@' with arg from untrusted input '$@'" and
message2 = sourceCmd.toString() and
sourceNode = source.getNode() and
message3 = source.toString()

Check warning

Code scanning / CodeQL

Using 'toString' in query logic Warning

Query logic depends on implementation of 'toString'.
@michaelnebel michaelnebel force-pushed the java/deprecateexperimental branch 2 times, most recently from a861f2f to de27511 Compare December 17, 2024 10:47
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 17, 2024
View reviewed changes
java/ql/src/experimental/Security/CWE/CWE-089/MyBatisMapperXmlSqlInjection.ql
import semmle.code.xml.MyBatisMapperXML
deprecated import MyBatisCommonLib
deprecated import MyBatisMapperXmlSqlInjectionLib
deprecated import semmle.code.xml.MyBatisMapperXML

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
MyBatisCommonLib
Loading
.
Redundant import, the module is already imported inside
MyBatisMapperXmlSqlInjectionLib
Loading
.
@michaelnebel michaelnebel marked this pull request as ready for review December 18, 2024 10:39
@michaelnebel michaelnebel requested a review from a team as a code owner December 18, 2024 10:39
@michaelnebel michaelnebel force-pushed the java/deprecateexperimental branch from 1e92baa to b9ed37d Compare January 21, 2025 12:10
jcogs33
jcogs33 reviewed Jan 25, 2025
View reviewed changes
java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll Outdated
);

/**
* DEPRECATED: Do not use.
Copy link
Contributor

@jcogs33 jcogs33 Jan 24, 2025

Choose a reason for hiding this comment

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

Does experimentalSourceModel need a deprecated annotation?
Or should this QLDoc say INTERNAL: Do not use. instead?

Copy link
Contributor Author

@michaelnebel michaelnebel Jan 27, 2025

Choose a reason for hiding this comment

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

Thank you for looking into this PR!

If we add a deprecated annotation, we will get lots of deprecation warnings during compilation.
I don't think there is a good way to deprecate extensible predicates that we have exposed (even though no one should be using this). Normally when we deprecate a predicate or class we can create a private internal variant and then use an alias and deprecate the original predicate. However, we can't use the same trick for extensible predicates as someone else might have declared tuples targeting the specific predicate we wan't to deprecate.
In any case the entire stack of functionality on top of the extensible experimental predicates have been deprecated and when we remove these extensible predicates, we will do it in conjunction with removing all the queries that uses the extensible predicates.
I will elaborate a bit in the comment and also add INTERNAL: Do not use..

java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll Outdated
);

/**
* DEPRECATED: Do not use.
Copy link
Contributor

@jcogs33 jcogs33 Jan 24, 2025

Choose a reason for hiding this comment

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

Same question for experimentalSinkModel.

java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll Outdated
);

/**
* DEPRECATED: Do not use.
Copy link
Contributor

@jcogs33 jcogs33 Jan 24, 2025

Choose a reason for hiding this comment

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

Same question for experimentalSummaryModel.

@michaelnebel michaelnebel force-pushed the java/deprecateexperimental branch from b9ed37d to 98d6353 Compare January 27, 2025 10:22
jcogs33
jcogs33 approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@jcogs33 jcogs33 left a comment

Choose a reason for hiding this comment

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

Looks reasonable

@michaelnebel michaelnebel merged commit ee5416f into github:main Jan 29, 2025
16 checks passed
@michaelnebel michaelnebel deleted the java/deprecateexperimental branch January 29, 2025 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcogs33 jcogs33 jcogs33 approved these changes

Assignees

No one assigned

Labels

documentation Java

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@michaelnebel @jcogs33