- Notifications
You must be signed in to change notification settings - Fork1.7k
C#: Diff-informed queries: phase 3 (non-trivial locations)#20074
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 enables diff-informed mode on C# queries that select locations other than dataflow source or sink by adding theobserveDiffInformedIncrementalMode() predicate and custom location overrides where needed. This is the final step in mass-enabling diff-informed queries across all languages.
- Adds
observeDiffInformedIncrementalMode()predicate to enable diff-informed mode on data flow configurations - Implements custom location selection logic via
getASelectedSinkLocation()for queries that report locations different from sink locations - Distinguishes between primary configurations (enabled with
any()) and secondary configurations (disabled withnone())
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| HardcodedConnectionString.ql | Enables diff-informed mode and adds location override to select call locations instead of sink locations |
| ThreadUnsafeICryptoTransformLambda.ql | Enables diff-informed mode for parallel crypto transform usage detection |
| UnsafeDeserializationQuery.qll | Enables diff-informed mode for primary configs, disables for secondary configs used in unsafe deserialization queries |
| ExternalAPIsQuery.qll | Enables diff-informed mode for external API data flow tracking |
| ConditionalBypassQuery.qll | Enables diff-informed mode and adds location override to select both sink and sensitive method call locations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM under the conditions that
(1) DCA looks good
(2) The discussion startedhere is resolved.
| @@ -78,6 +78,8 @@ private module RemoteSourceToExternalApiConfig implements DataFlow::ConfigSig { | |||
| predicateisSource(DataFlow::Nodesource){sourceinstanceofActiveThreatModelSource} | |||
| predicateisSink(DataFlow::Nodesink){sinkinstanceofExternalApiDataNode} | |||
| predicateobserveDiffInformedIncrementalMode(){any()} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This will also affect the output ofcs/count-untrusted-data-external-api (if run in diff informed mode). However, this query is not included in any query suite, so this should be ok.
This PR enables diff-informed mode on queries that select a location other than dataflow source or sink. This entails adding a non-trivial location override that returns the locations that are actually selected.
Prior work includes PRs like#19663,#19759, and#19817. This PR uses the same patch script as those PRs to find candidate queries to convert to diff-enabled. This is the final step in mass-enabling diff-informed queries on all the languages.
Commit-by-commit reviewing is recommended.