From a61d42edc39092914eff356b544d544a6be58975 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Feb 2025 10:51:08 +0100 Subject: [PATCH 01/13] JS: Make inline CG tests report call target if NONE was given Previously it would only report a spurious callee if the target function was named. Now, if specifying 'calls:NONE' if will report any callee as spurious. --- .../CallGraphs/AnnotatedTest/Test.expected | 1 + .../library-tests/CallGraphs/AnnotatedTest/Test.ql | 11 +++++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected index 8182d0174140..983e8164c2c6 100644 --- a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected @@ -1,4 +1,5 @@ spuriousCallee +| accessors.js:54:1:54:7 | new D() | accessors.js:32:9:32:8 | () {} | -1 | calls | missingCallee | constructor-field.ts:40:5:40:14 | f3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 | calls | | constructor-field.ts:71:1:71:11 | bf3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 | calls | diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.ql b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.ql index e92524adc199..4388a2d388d1 100644 --- a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.ql +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.ql @@ -58,20 +58,23 @@ class AnnotatedCall extends DataFlow::Node { string getKind() { result = kind } } -predicate callEdge(AnnotatedCall call, AnnotatedFunction target, int boundArgs) { +predicate callEdge(AnnotatedCall call, Function target, int boundArgs) { FlowSteps::calls(call, target) and boundArgs = -1 or FlowSteps::callsBound(call, target, boundArgs) } -query predicate spuriousCallee( - AnnotatedCall call, AnnotatedFunction target, int boundArgs, string kind -) { +query predicate spuriousCallee(AnnotatedCall call, Function target, int boundArgs, string kind) { callEdge(call, target, boundArgs) and kind = call.getKind() and not ( target = call.getAnExpectedCallee(kind) and boundArgs = call.getBoundArgsOrMinusOne() + ) and + ( + target instanceof AnnotatedFunction + or + call.getCallTargetName() = "NONE" ) } From 404376500820e4c92ce15de88a0101ae90bf8e60 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Feb 2025 10:52:15 +0100 Subject: [PATCH 02/13] JS: Avoid ambiguity in an inline CG annotation --- .../test/library-tests/CallGraphs/AnnotatedTest/Test.expected | 3 +-- .../test/library-tests/CallGraphs/AnnotatedTest/accessors.js | 3 ++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected index 983e8164c2c6..0abd563b4193 100644 --- a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected @@ -1,5 +1,4 @@ spuriousCallee -| accessors.js:54:1:54:7 | new D() | accessors.js:32:9:32:8 | () {} | -1 | calls | missingCallee | constructor-field.ts:40:5:40:14 | f3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 | calls | | constructor-field.ts:71:1:71:11 | bf3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 | calls | @@ -13,4 +12,4 @@ accessorCall | accessors.js:44:1:44:9 | new D().f | accessors.js:37:8:37:13 | (x) {} | | accessors.js:48:1:48:5 | obj.f | accessors.js:5:8:5:12 | () {} | | accessors.js:51:1:51:3 | C.f | accessors.js:19:15:19:19 | () {} | -| accessors.js:54:1:54:9 | new D().f | accessors.js:34:8:34:12 | () {} | +| accessors.js:55:1:55:3 | d.f | accessors.js:34:8:34:12 | () {} | diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/accessors.js b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/accessors.js index f9f4e25d0925..5ec118c460b7 100644 --- a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/accessors.js +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/accessors.js @@ -50,5 +50,6 @@ obj.f(); /** calls:NONE */ C.f(); +const d = new D(); /** calls:NONE */ -new D().f(); +d.f(); From 9321d69034f4306d3124383ba3928e398fb49247 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Feb 2025 10:53:13 +0100 Subject: [PATCH 03/13] JS: Add CG test showing lack of calls down to subclasses --- .../CallGraphs/AnnotatedTest/subclasses.js | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js new file mode 100644 index 000000000000..885b130649fb --- /dev/null +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js @@ -0,0 +1,37 @@ +import 'dummy'; + +class Base { + workInBase() { + /** calls:methodInBase */ + this.methodInBase(); + + /** calls:NONE */ + this.methodInSub(); + } + + /** name:methodInBase */ + methodInBase() { + /** calls:NONE */ + this.methodInSub(); + } +} + +class Subclass1 extends Base { + workInSub() { + /** calls:methodInBase */ + this.methodInBase(); + } + + methodInSub() { + } +} + +class Subclass2 extends Base { + workInSub() { + /** calls:methodInBase */ + this.methodInBase(); + } + + methodInSub() { + } +} From aff458d948bf9616c0b4b6017b97f5dc01e66563 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Feb 2025 11:14:50 +0100 Subject: [PATCH 04/13] JS: Also add tests for upward calls and overriding --- .../CallGraphs/AnnotatedTest/subclasses.js | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js index 885b130649fb..591aa635ff57 100644 --- a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js @@ -7,6 +7,9 @@ class Base { /** calls:NONE */ this.methodInSub(); + + /** calls:overridenInSub0 */ + this.overridenInSub(); } /** name:methodInBase */ @@ -14,24 +17,44 @@ class Base { /** calls:NONE */ this.methodInSub(); } + + /** name:overridenInSub0 */ + overridenInSub() { + } } class Subclass1 extends Base { workInSub() { /** calls:methodInBase */ this.methodInBase(); + + /** calls:overridenInSub1 */ + this.overridenInSub(); } + /** name:methodInSub1 */ methodInSub() { } + + /** name:overridenInSub1 */ + overridenInSub() { + } } class Subclass2 extends Base { workInSub() { /** calls:methodInBase */ this.methodInBase(); + + /** calls:overridenInSub2 */ + this.overridenInSub(); } + /** name:methodInSub2 */ methodInSub() { } + + /** name:overridenInSub2 */ + overridenInSub() { + } } From b8b2b9a470960e33c1c9a6e2406113e75375978c Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Feb 2025 11:15:42 +0100 Subject: [PATCH 05/13] JS: Resolve calls downward in the class hierarchy --- .../lib/semmle/javascript/dataflow/Nodes.qll | 37 ++++++++++++++++--- .../CallGraphs/AnnotatedTest/subclasses.js | 6 +-- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll b/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll index ff68540d346e..6739f4417504 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll @@ -1066,6 +1066,32 @@ class ClassNode extends DataFlow::SourceNode instanceof ClassNode::Range { result = this.getAnInstanceReference(DataFlow::TypeTracker::end()) } + pragma[nomagic] + private DataFlow::PropRead getAnOwnInstanceMemberAccess(string name, DataFlow::TypeTracker t) { + result = this.getAnInstanceReference(t.continue()).getAPropertyRead(name) + } + + pragma[nomagic] + private DataFlow::PropRead getAnInstanceMemberAccessOnSubClass( + string name, DataFlow::TypeTracker t + ) { + exists(DataFlow::ClassNode subclass | + subclass = this.getADirectSubClass() and + not exists(subclass.getInstanceMember(name, _)) + | + result = subclass.getAnOwnInstanceMemberAccess(name, t) + or + result = subclass.getAnInstanceMemberAccessOnSubClass(name, t) + ) + } + + pragma[nomagic] + private DataFlow::PropRead getAnInstanceMemberAccessOnSuperClass(string name) { + result = this.getADirectSuperClass().getAReceiverNode().getAPropertyRead(name) + or + result = this.getADirectSuperClass().getAnInstanceMemberAccessOnSuperClass(name) + } + /** * Gets a property read that accesses the property `name` on an instance of this class. * @@ -1073,13 +1099,12 @@ class ClassNode extends DataFlow::SourceNode instanceof ClassNode::Range { */ pragma[nomagic] DataFlow::PropRead getAnInstanceMemberAccess(string name, DataFlow::TypeTracker t) { - result = this.getAnInstanceReference(t.continue()).getAPropertyRead(name) + result = this.getAnOwnInstanceMemberAccess(name, t) or - exists(DataFlow::ClassNode subclass | - result = subclass.getAnInstanceMemberAccess(name, t) and - not exists(subclass.getInstanceMember(name, _)) and - this = subclass.getADirectSuperClass() - ) + result = this.getAnInstanceMemberAccessOnSubClass(name, t) + or + t.start() and + result = this.getAnInstanceMemberAccessOnSuperClass(name) } /** diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js index 591aa635ff57..c545cd9731de 100644 --- a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js @@ -5,16 +5,16 @@ class Base { /** calls:methodInBase */ this.methodInBase(); - /** calls:NONE */ + /** calls:methodInSub1 calls:methodInSub2 */ this.methodInSub(); - /** calls:overridenInSub0 */ + /** calls:overridenInSub0 calls:overridenInSub1 calls:overridenInSub2 */ this.overridenInSub(); } /** name:methodInBase */ methodInBase() { - /** calls:NONE */ + /** calls:methodInSub1 calls:methodInSub2 */ this.methodInSub(); } From d3c4b5d4939d8503e96c1f71645027d291369d1d Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Feb 2025 12:41:45 +0100 Subject: [PATCH 06/13] JS: Add test with spurious flow due to up-down calls --- .../test/library-tests/TripleDot/subclass.js | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 javascript/ql/test/library-tests/TripleDot/subclass.js diff --git a/javascript/ql/test/library-tests/TripleDot/subclass.js b/javascript/ql/test/library-tests/TripleDot/subclass.js new file mode 100644 index 000000000000..6a7f3f593fa0 --- /dev/null +++ b/javascript/ql/test/library-tests/TripleDot/subclass.js @@ -0,0 +1,34 @@ +import 'dummy'; + +class Base { + baseMethod(x) { + this.subclassMethod(x); + } +} + +class Subclass1 extends Base { + work() { + this.baseMethod(source("sub1")); + } + subclassMethod(x) { + sink(x); // $ hasValueFlow=sub1 SPURIOUS: hasValueFlow=sub2 + } +} + +class Subclass2 extends Base { + work() { + this.baseMethod(source("sub2")); + } + subclassMethod(x) { + sink(x); // $ hasValueFlow=sub2 SPURIOUS: hasValueFlow=sub1 + } +} + +class Subclass3 extends Base { + work() { + this.baseMethod("safe"); + } + subclassMethod(x) { + sink(x); // $ SPURIOUS: hasValueFlow=sub1 hasValueFlow=sub2 + } +} From ff7bc7c25e1322d1e05e0627967068d2b6d2f554 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Feb 2025 12:39:57 +0100 Subject: [PATCH 07/13] JS: Track types of classes in data flow --- .../dataflow/internal/DataFlowPrivate.qll | 38 ++++++++++++++++--- .../test/library-tests/TripleDot/subclass.js | 2 +- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll index 0e95d3511559..6cb34c28dca1 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll @@ -558,15 +558,19 @@ DataFlowCallable nodeGetEnclosingCallable(Node node) { newtype TDataFlowType = TFunctionType(Function f) or + TInstanceType(DataFlow::ClassNode cls) or TAnyType() class DataFlowType extends TDataFlowType { string toDebugString() { - this instanceof TFunctionType and result = "TFunctionType(" + this.asFunction().toString() + ") at line " + this.asFunction().getLocation().getStartLine() or + result = + "TInstanceType(" + this.asInstanceOfClass().toString() + ") at line " + + this.asInstanceOfClass().getLocation().getStartLine() + or this instanceof TAnyType and result = "TAnyType" } @@ -575,13 +579,25 @@ class DataFlowType extends TDataFlowType { } Function asFunction() { this = TFunctionType(result) } + + DataFlow::ClassNode asInstanceOfClass() { this = TInstanceType(result) } +} + +private predicate typeStrongerThan1(DataFlowType t1, DataFlowType t2) { + // 't1' is a subclass of 't2' + t1.asInstanceOfClass() = t2.asInstanceOfClass().getADirectSubClass() } /** * Holds if `t1` is strictly stronger than `t2`. */ predicate typeStrongerThan(DataFlowType t1, DataFlowType t2) { - t1 instanceof TFunctionType and t2 = TAnyType() + typeStrongerThan1(t1, t2) + or + // Ensure all types are transitively stronger than 'any' + not typeStrongerThan1(t1, _) and + not t1 = TAnyType() and + t2 = TAnyType() } private DataFlowType getPreciseType(Node node) { @@ -590,6 +606,9 @@ private DataFlowType getPreciseType(Node node) { result = TFunctionType(f) ) or + result.asInstanceOfClass() = + unique(DataFlow::ClassNode cls | cls.getAnInstanceReference().getALocalUse() = node) + or result = getPreciseType(node.getImmediatePredecessor()) or result = getPreciseType(node.(PostUpdateNode).getPreUpdateNode()) @@ -683,18 +702,27 @@ predicate neverSkipInPathGraph(Node node) { string ppReprType(DataFlowType t) { none() } pragma[inline] -private predicate compatibleTypesNonSymRefl(DataFlowType t1, DataFlowType t2) { +private predicate compatibleTypesWithAny(DataFlowType t1, DataFlowType t2) { t1 != TAnyType() and t2 = TAnyType() } +pragma[nomagic] +private predicate compatibleTypes1(DataFlowType t1, DataFlowType t2) { + t1.asInstanceOfClass().getADirectSubClass+() = t2.asInstanceOfClass() +} + pragma[inline] predicate compatibleTypes(DataFlowType t1, DataFlowType t2) { t1 = t2 or - compatibleTypesNonSymRefl(t1, t2) + compatibleTypesWithAny(t1, t2) + or + compatibleTypesWithAny(t2, t1) + or + compatibleTypes1(t1, t2) or - compatibleTypesNonSymRefl(t2, t1) + compatibleTypes1(t2, t1) } predicate forceHighPrecision(Content c) { none() } diff --git a/javascript/ql/test/library-tests/TripleDot/subclass.js b/javascript/ql/test/library-tests/TripleDot/subclass.js index 6a7f3f593fa0..b5d41cbcc16c 100644 --- a/javascript/ql/test/library-tests/TripleDot/subclass.js +++ b/javascript/ql/test/library-tests/TripleDot/subclass.js @@ -29,6 +29,6 @@ class Subclass3 extends Base { this.baseMethod("safe"); } subclassMethod(x) { - sink(x); // $ SPURIOUS: hasValueFlow=sub1 hasValueFlow=sub2 + sink(x); } } From ab5fc9f4d7d88df42110f63e84f3b710c01f5a4c Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Feb 2025 13:25:19 +0100 Subject: [PATCH 08/13] JS: Implement viableImplInCallContext --- .../dataflow/internal/DataFlowPrivate.qll | 41 ++++++++++++++++++- .../test/library-tests/TripleDot/subclass.js | 4 +- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll index 6cb34c28dca1..ba50946f9311 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll @@ -1089,17 +1089,54 @@ DataFlowCallable viableCallable(DataFlowCall node) { result.asSourceCallableNotExterns() = node.asImpliedLambdaCall() } +private DataFlowCall getACallOnThis(DataFlow::ClassNode cls) { + result.asOrdinaryCall() = cls.getAReceiverNode().getAPropertyRead().getACall() + or + result.asAccessorCall() = cls.getAReceiverNode().getAPropertyRead() + or + result.asPartialCall().getACallbackNode() = cls.getAReceiverNode().getAPropertyRead() +} + +private predicate downwardCall(DataFlowCall call) { + exists(DataFlow::ClassNode cls | + call = getACallOnThis(cls) and + viableCallable(call).asSourceCallable() = + cls.getADirectSubClass+().getAnInstanceMember().getFunction() + ) +} + /** * Holds if the set of viable implementations that can be called by `call` * might be improved by knowing the call context. */ -predicate mayBenefitFromCallContext(DataFlowCall call) { none() } +predicate mayBenefitFromCallContext(DataFlowCall call) { downwardCall(call) } + +/** Gets the type of the receiver of `call`. */ +private DataFlowType getThisArgumentType(DataFlowCall call) { + exists(DataFlow::Node node | + isArgumentNodeImpl(node, call, MkThisParameter()) and + result = getNodeType(node) + ) +} + +/** Gets the type of the 'this' parameter of `call`. */ +private DataFlowType getThisParameterType(DataFlowCallable callable) { + exists(DataFlow::Node node | + isParameterNodeImpl(node, callable, MkThisParameter()) and + result = getNodeType(node) + ) +} /** * Gets a viable dispatch target of `call` in the context `ctx`. This is * restricted to those `call`s for which a context might make a difference. */ -DataFlowCallable viableImplInCallContext(DataFlowCall call, DataFlowCall ctx) { none() } +DataFlowCallable viableImplInCallContext(DataFlowCall call, DataFlowCall ctx) { + mayBenefitFromCallContext(call) and + result = viableCallable(call) and + viableCallable(ctx) = call.getEnclosingCallable() and + compatibleTypes(getThisArgumentType(ctx), getThisParameterType(result)) +} bindingset[node, fun] pragma[inline_late] diff --git a/javascript/ql/test/library-tests/TripleDot/subclass.js b/javascript/ql/test/library-tests/TripleDot/subclass.js index b5d41cbcc16c..35c722d2df46 100644 --- a/javascript/ql/test/library-tests/TripleDot/subclass.js +++ b/javascript/ql/test/library-tests/TripleDot/subclass.js @@ -11,7 +11,7 @@ class Subclass1 extends Base { this.baseMethod(source("sub1")); } subclassMethod(x) { - sink(x); // $ hasValueFlow=sub1 SPURIOUS: hasValueFlow=sub2 + sink(x); // $ hasValueFlow=sub1 } } @@ -20,7 +20,7 @@ class Subclass2 extends Base { this.baseMethod(source("sub2")); } subclassMethod(x) { - sink(x); // $ hasValueFlow=sub2 SPURIOUS: hasValueFlow=sub1 + sink(x); // $ hasValueFlow=sub2 } } From 97eb09fef8dfeb274fdc883687fe759319013514 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Feb 2025 10:19:49 +0100 Subject: [PATCH 09/13] JS: Accept updated test output --- .../ql/test/library-tests/CallGraphs/FullTest/tests.expected | 2 ++ 1 file changed, 2 insertions(+) diff --git a/javascript/ql/test/library-tests/CallGraphs/FullTest/tests.expected b/javascript/ql/test/library-tests/CallGraphs/FullTest/tests.expected index 4d2c78206a46..8185f693b227 100644 --- a/javascript/ql/test/library-tests/CallGraphs/FullTest/tests.expected +++ b/javascript/ql/test/library-tests/CallGraphs/FullTest/tests.expected @@ -56,6 +56,7 @@ test_getAFunctionValue | classes.js:3:10:5:5 | () {\\n ... ;\\n } | classes.js:3:10:5:5 | () {\\n ... ;\\n } | | classes.js:7:6:9:5 | () {\\n ... ;\\n } | classes.js:7:6:9:5 | () {\\n ... ;\\n } | | classes.js:8:7:8:16 | this.hello | classes.js:3:10:5:5 | () {\\n ... ;\\n } | +| classes.js:8:7:8:16 | this.hello | classes.js:13:10:15:5 | () {\\n ... ;\\n } | | classes.js:12:3:16:3 | B | classes.js:12:21:12:20 | (...arg ... rgs); } | | classes.js:12:3:16:3 | class B ... }\\n } | classes.js:12:21:12:20 | (...arg ... rgs); } | | classes.js:12:19:12:19 | A | classes.js:2:11:2:10 | () {} | @@ -447,6 +448,7 @@ test_getACallee | a.js:3:1:3:5 | bar() | b.js:2:8:2:24 | function bar() {} | | a.js:4:1:4:5 | qux() | c.js:2:8:2:24 | function bar() {} | | classes.js:8:7:8:18 | this.hello() | classes.js:3:10:5:5 | () {\\n ... ;\\n } | +| classes.js:8:7:8:18 | this.hello() | classes.js:13:10:15:5 | () {\\n ... ;\\n } | | classes.js:12:21:12:20 | super(...args) | classes.js:2:11:2:10 | () {} | | classes.js:18:3:18:9 | new B() | classes.js:12:21:12:20 | (...arg ... rgs); } | | classes.js:18:3:18:17 | new B().hello() | classes.js:13:10:15:5 | () {\\n ... ;\\n } | From b8f48aa711fcd37f2b899809b639b76fc208961c Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Feb 2025 10:24:39 +0100 Subject: [PATCH 10/13] JS: Change note --- javascript/ql/src/change-notes/2025-02-17-downward-calls.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 javascript/ql/src/change-notes/2025-02-17-downward-calls.md diff --git a/javascript/ql/src/change-notes/2025-02-17-downward-calls.md b/javascript/ql/src/change-notes/2025-02-17-downward-calls.md new file mode 100644 index 000000000000..84bde9dbde83 --- /dev/null +++ b/javascript/ql/src/change-notes/2025-02-17-downward-calls.md @@ -0,0 +1,6 @@ +--- +category: majorAnalysis +--- +* Improved call resolution logic to better handle calls resolving "downwards", targeting + a method declared in a subclass of the enclosing class. Data flow analysis + has also improved to avoid spurious flow between unrelated classes in the class hierarchy. From 24e7aad6ba7f06d6d41764d30b7c08d7f50a9443 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 18 Feb 2025 09:51:13 +0100 Subject: [PATCH 11/13] JS: Overriden -> Overridden --- .../CallGraphs/AnnotatedTest/subclasses.js | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js index c545cd9731de..f60181d1cac7 100644 --- a/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js +++ b/javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/subclasses.js @@ -8,8 +8,8 @@ class Base { /** calls:methodInSub1 calls:methodInSub2 */ this.methodInSub(); - /** calls:overridenInSub0 calls:overridenInSub1 calls:overridenInSub2 */ - this.overridenInSub(); + /** calls:overriddenInSub0 calls:overriddenInSub1 calls:overriddenInSub2 */ + this.overriddenInSub(); } /** name:methodInBase */ @@ -18,8 +18,8 @@ class Base { this.methodInSub(); } - /** name:overridenInSub0 */ - overridenInSub() { + /** name:overriddenInSub0 */ + overriddenInSub() { } } @@ -28,16 +28,16 @@ class Subclass1 extends Base { /** calls:methodInBase */ this.methodInBase(); - /** calls:overridenInSub1 */ - this.overridenInSub(); + /** calls:overriddenInSub1 */ + this.overriddenInSub(); } /** name:methodInSub1 */ methodInSub() { } - /** name:overridenInSub1 */ - overridenInSub() { + /** name:overriddenInSub1 */ + overriddenInSub() { } } @@ -46,15 +46,15 @@ class Subclass2 extends Base { /** calls:methodInBase */ this.methodInBase(); - /** calls:overridenInSub2 */ - this.overridenInSub(); + /** calls:overriddenInSub2 */ + this.overriddenInSub(); } /** name:methodInSub2 */ methodInSub() { } - /** name:overridenInSub2 */ - overridenInSub() { + /** name:overriddenInSub2 */ + overriddenInSub() { } } From e40ee821c2cb2e64039ec93cb681a6a268edc879 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 18 Feb 2025 16:02:47 +0100 Subject: [PATCH 12/13] JS: Update a qldoc comment --- javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll b/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll index 6739f4417504..2e2313835227 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll @@ -1095,7 +1095,8 @@ class ClassNode extends DataFlow::SourceNode instanceof ClassNode::Range { /** * Gets a property read that accesses the property `name` on an instance of this class. * - * Concretely, this holds when the base is an instance of this class or a subclass thereof. + * This includes accesses on subclasses (if the member is not overridden) and accesses in a base class + * (only if accessed on `this`). */ pragma[nomagic] DataFlow::PropRead getAnInstanceMemberAccess(string name, DataFlow::TypeTracker t) { From ad4522c7810d5557fe5082e8b4d9b954acd88a60 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 18 Feb 2025 16:03:00 +0100 Subject: [PATCH 13/13] JS: Make 'typeStrongerThan' transitive --- .../javascript/dataflow/internal/DataFlowPrivate.qll | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll index ba50946f9311..03a422404654 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll @@ -583,19 +583,14 @@ class DataFlowType extends TDataFlowType { DataFlow::ClassNode asInstanceOfClass() { this = TInstanceType(result) } } -private predicate typeStrongerThan1(DataFlowType t1, DataFlowType t2) { - // 't1' is a subclass of 't2' - t1.asInstanceOfClass() = t2.asInstanceOfClass().getADirectSubClass() -} - /** * Holds if `t1` is strictly stronger than `t2`. */ predicate typeStrongerThan(DataFlowType t1, DataFlowType t2) { - typeStrongerThan1(t1, t2) + // 't1' is a subclass of 't2' + t1.asInstanceOfClass() = t2.asInstanceOfClass().getADirectSubClass+() or - // Ensure all types are transitively stronger than 'any' - not typeStrongerThan1(t1, _) and + // Ensure all types are stronger than 'any' not t1 = TAnyType() and t2 = TAnyType() }