Skip to content

Rust: Make current MaD predicates deprecated #19502

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 1 commit into
base: main
Choose a base branch
from

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented May 15, 2025

In preparation for a transition period where we will allow for MaD definitions using extractor-provided canonical paths as well as the future QL-provided canonical paths.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label May 15, 2025
In preparation for a transition period where we will allow for MaD definitions
using extractor-provided canonical paths as well as the future QL-provided
canonical paths.
@hvitved hvitved force-pushed the rust/deprecate-mad-predicates branch from d5e2958 to 7c986d8 Compare May 16, 2025 07:23
@hvitved hvitved marked this pull request as ready for review May 16, 2025 08:02
@Copilot Copilot AI review requested due to automatic review settings May 16, 2025 08:02
@hvitved hvitved requested a review from a team as a code owner May 16, 2025 08:02
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 marks existing MaD (Model as Data) predicates as deprecated by renaming all sourceModel, sinkModel, and summaryModel extensionals to *Deprecated, and updates the QLL logic to consume only the deprecated predicates.

  • Rename all YAML extensible entries to <modelType>Deprecated
  • Update ModelsAsData.qll to reference the new deprecated predicate names

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
rust/ql/test/utils-tests/modelgenerator/CaptureSinkModels.ext.yml Updated sinkModel to sinkModelDeprecated
rust/ql/test/library-tests/dataflow/models/models.ext.yml Renamed sourceModel, sinkModel, summaryModel entries to deprecated variants
rust/ql/lib/codeql/rust/frameworks/**/*.model.yml Renamed all framework models (sourceModel, sinkModel, summaryModel) to deprecated variants
rust/ql/lib/codeql/rust/dataflow/internal/empty.model.yml Added deprecated suffix to all internal extensionals
rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll Updated predicate definitions and consumer logic to use the deprecated predicate names
Comments suppressed due to low confidence (1)

rust/ql/test/utils-tests/modelgenerator/CaptureSinkModels.ext.yml:4

  • Add or update a test to ensure that sinkModelDeprecated entries are correctly interpreted by the parser, mirroring existing coverage for the non-deprecated sinkModel.
      extensible: sinkModelDeprecated

@@ -58,7 +58,7 @@ private import codeql.rust.dataflow.FlowSink
* For more information on the `kind` parameter, see
* https://github.com/github/codeql/blob/main/docs/codeql/reusables/threat-model-description.rst.
*/
extensible predicate sourceModel(
extensible predicate sourceModelDeprecated(
Copy link
Preview

Copilot AI May 16, 2025

Choose a reason for hiding this comment

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

To preserve backward compatibility during the transition, consider leaving the original sourceModel (and its sink/summary counterparts) as wrapper predicates that forward to sourceModelDeprecated, so existing models won’t break immediately.

Copilot uses AI. Check for mistakes.

@@ -2,6 +2,6 @@
extensions:
- addsTo:
pack: codeql/rust-all
Copy link
Preview

Copilot AI May 16, 2025

Choose a reason for hiding this comment

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

[nitpick] Add a comment above this line (e.g. # Deprecated in favor of extractor-provided canonical paths) to clearly document that this model entry is deprecated and will be replaced in a future release.

Suggested change
pack: codeql/rust-all
pack: codeql/rust-all
# Deprecated in favor of extractor-provided canonical paths

Copilot uses AI. Check for mistakes.

@paldepind
Copy link
Contributor

Unless I'm missing something this will break the generated models the next time we run the model generator as it still outputs rows for predicates with the names {source,sink,summary}Model.

The predicate names are specified in a shared script so it's not trivial to just rename them.

Perhaps the absolutely easiest thing we could do is just to keep the current predicates as aliases for those with the Deprecated suffix, but only use them in the generated models. When the format breaks updating the generated models should be easy anyway, so while it doesn't seem very clean, pragmatically it seems fine?

@@ -74,7 +74,7 @@ extensible predicate sourceModel(
*
* - `sql-injection`: a flow sink for SQL injection.
*/
extensible predicate sinkModel(
extensible predicate sinkModelDeprecated(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably explain what's going on in the qldoc here, i.e. that a new format is coming soon but is not available yet (usually we would link to replacing predicate, but I understand these don't yet exist).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants