Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
4 open issues
isTypeRef's second disjunct (trackTypeSelector(_) = getNodeFromRef(access)) can classify… · NewCallExprto aModelTypeconstructor call (purely via… · Newname = any(Selector s).getPropertyName()introduces a global join over all selectors, but… · NewModels::isSource(node, [\"local\", \"remote\"])restriction is now duplicated across multiple… · New
What changed in this PR
Updates unified Swift MaD/type-inference plumbing so property-based sources/sinks and model-only types are recognized, and adjusts security/library tests accordingly.
Changes:
- Add model-driven property access selectors and a
ModelTypeto support types referenced only in models-as-data (no declaration in DB). - Improve implicit-receiver inference and base-type inference for member accesses (including unqualified accesses).
- Update multiple Swift query tests/expected outputs to reflect newly detected property sources/sinks.
| File | Description |
|---|---|
| unified/ql/test/query-tests/security/CWE-078/CommandInjection/CommandInjection.swift | Updates inline expectations where new sinks are now detected. |
| unified/ql/test/query-tests/security/CWE-078/CommandInjection/CommandInjection.expected | Adds expected results/edges/nodes for newly detected executableURL/arguments flows. |
| unified/ql/test/query-tests/security/CWE-022/PathInjection/testPathInjection.swift | Updates Realm configuration property expectations to alerts. |
| unified/ql/test/query-tests/security/CWE-022/PathInjection/PathInjectionTest.expected | Adds expected results/edges/nodes for new Realm fileURL/seedFilePath sinks. |
| unified/ql/test/library-tests/mad/test.swift | Expands MaD library test coverage for property sources/sinks, nested types, and implicit self. |
| unified/ql/test/library-tests/mad/test.expected | Updates expected MaD outputs to include new property sinks/sources. |
| unified/ql/test/library-tests/mad-builtins/test.swift | Adds a builtins-focused MaD test case without local stubs. |
| unified/ql/test/library-tests/mad-builtins/test.qlref | Wires the new builtins test to the existing MaD test query + postprocess. |
| unified/ql/test/library-tests/mad-builtins/test.expected | Adds expected outputs for the new builtins-based MaD test. |
| unified/ql/src/queries/security/CWE-078/CommandInjection.ql | Narrows sources used by the command injection config. |
| unified/ql/src/queries/security/CWE-022/PathInjection.ql | Narrows sources used by the path injection config. |
| unified/ql/lib/codeql/unified/internal/typeinference/TypeMention.qll | Allows type mentions to be resolved via ModelType selectors. |
| unified/ql/lib/codeql/unified/internal/typeinference/TypeInference.qll | Adds variable/base-type inference helpers and seeds inference from model-only constructor calls. |
| unified/ql/lib/codeql/unified/internal/typeinference/Type.qll | Introduces ModelType backed by MaD TypeSelectors. |
| unified/ql/lib/codeql/unified/internal/mad/LegacyMaD.qll | Adds property selectors, type selectors, and property source/sink extraction for MaD. |
| unified/ql/lib/codeql/unified/internal/StaticNameBinding.qll | Extends name binding graph to support resolving MaD TypeSelectors; adds isTypeRef. |
| unified/ql/lib/codeql/unified/internal/ExprPositions.qll | Switches type-context detection from getTypeFromRef existence to isTypeRef. |
| unified/ql/lib/codeql/unified/internal/AnalysisQuality.qll | Narrows sources used by taint reach quality analysis config. |
Review effort: Lite
Contributor
Author
Rerun has been triggered: 5 restarted 🚀 |
asgerf
marked this pull request as ready for review
November 9, 2026 10:08
hvitved
reviewed
Nov 9, 2026
| private predicate propertyAccessSelector(MemberAccess access, ClassLikeDeclaration cls, string name) { | ||
| name = access.getMemberName() and | ||
| name = any(Selector s).getPropertyName() and | ||
| not access = any(CallExpr call).getCallee() and |
Contributor
There was a problem hiding this comment.
Could a property not hold a closure value?
Contributor
Author
There was a problem hiding this comment.
Yes. I suppose this line is an optimization that's only valid because we know we can't derive meaningful sources or sinks when a property access appears in callee position.
| } | ||
|
|
||
| /** Gets the declaration of this type, if any. */ | ||
| ClassLikeDeclaration getDeclaration() { result.getQualifiedName() = this } |
Contributor
There was a problem hiding this comment.
Should we avoid generating synthetic namebinding nodes and types in this case?
Contributor
Author
There was a problem hiding this comment.
A later commit restricts types to avoid duplicate representatives for the type.
For static name binding it would lead to negative recursion as getQualifiedName() depends on extension-resolution (which is beneficial, albeit only in rare situations).
We could use the syntactic qualified name if we really wanted to cut that dependency, but I suspect we'll want to be able to depend on static name binding here even more in the future, e.g. when we want types to be qualified by their module (e.g Foundation.URL instead of URL).
Avoid inflating the numebr with string-length sources and similar.
This enables type inference to track these types because they now have a synthetic definition.
ClassLikeDeclaration is now a static member
Some more tests in mad-builtins pass because they now work without stub declarations
This is needed to make TypeMention include references to a type from a model
Avoid creating ModelType when a ClassDeclarationType exists
If an extension resolving to a TypeSelector existed, we'd fail to create a ModelType for it.
This addresses a very niche concern from Copilot that, if a value resolves to both a value (e.g. field) and a model type, then it should not be seen as being in type context. It's very difficult to see this matter in practice but why not.
asgerf
force-pushed
the
unified/property-models-rebased
branch
from
November 12, 2026 11:23
64e164e to
a41f207
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extends legacy MaD to support sources and sinks derived from property accesses, such as
Process.arguments. Previously only calls and callables were matched.Matching properties on name alone is too imprecise even at this stage, so this also makes a start on matching the base object against the inferred type. Types mentioned in a model are now tracked by static name binding and type inference. For example, when a function annotated with
Processas the return type we can now track that, whereas previously we lacked a representative for it.I'd like to take this a bit further and introduce a
ClassLikeElementnewtype that's the canonical representative for a class (think: union of non-extension class declaration andTypeSelector) and make APIs likeClassLikeDeclaration.resolve()andgetAMemberIncludingExtensionsbased on that class instead. But to avoid a mega-PR and too many conflicts, I'm deferred that to a later PR.The last commit documents a test failure that will pass once the
ClassLikeElementrefactoring is in.The PR also limits taint reach to use
localandremotesources, in particular to avoid inflating the number with things like string-length sources. Queries are also restricted tolocalandremotefor this reason.DCA run shows an increase in taint reach from 778->1058