Swift: Diff-informed queries: phase 3 (non-trivial locations) #20082
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enables diff-informed mode on Swift security queries that select non-trivial locations (other than dataflow source or sink). The changes add location override predicates to handle cases where queries select locations that need special handling for diff-informed analysis.
- Adds
observeDiffInformedIncrementalMode()predicate to four security query modules - Implements
getASelectedSinkLocation()predicate for queries that can support diff-informed mode - Disables diff-informed mode for queries with location selection complexities
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| UnsafeWebViewFetchQuery.qll | Disables diff-informed mode due to secondary location use in select |
| InsecureTLSQuery.qll | Disables diff-informed mode due to Swift nodes with problematic locations |
| CleartextStoragePreferencesQuery.qll | Enables diff-informed mode with custom sink location handling |
| CleartextStorageDatabaseQuery.qll | Enables diff-informed mode with custom sink location handling |
|
|
||
| predicate observeDiffInformedIncrementalMode() { any() } | ||
|
|
||
| Location getASelectedSinkLocation(DataFlow::Node sink) { |
There was a problem hiding this comment.
The logic in getASelectedSinkLocation is duplicated across CleartextStoragePreferencesQuery.qll and CleartextStorageDatabaseQuery.qll. Consider extracting this common pattern into a shared utility predicate to improve maintainability.
Copilot uses AI. Check for mistakes.
|
|
||
| predicate observeDiffInformedIncrementalMode() { | ||
| none() // query selects some Swift nodes (e.g. "[post] self") that have location file://:0:0:0:0, which always fall outside the diff range. | ||
| } |
There was a problem hiding this comment.
Do you recall if there's any reason we don't give post-update nodes a Location (equal to that of the pre-update node)?
I may do a quick experiment with this (probably after your PR, to avoid complicating things).
|
|
||
| predicate observeDiffInformedIncrementalMode() { | ||
| none() // can't override location accurately because of secondary use in select. | ||
| } |
There was a problem hiding this comment.
What's the issue here? The only unusual thing I can see in that query is selecting from three possible alert messages.
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.