Skip to content

Commit 4e62a51

Browse files
committed
JS: Only apply exception propagator when no other summary applies
Previously a few Promise-related methods were special-cased, which is no longer needed.
1 parent 84820ad commit 4e62a51

File tree

3 files changed

+29
-6
lines changed

3 files changed

+29
-6
lines changed

javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,19 @@ abstract class LibraryCallable extends string {
438438
DataFlow::InvokeNode getACallSimple() { none() }
439439
}
440440

441+
/** Internal subclass of `LibraryCallable`, whose member predicates should not be visible on `SummarizedCallable`. */
442+
abstract class LibraryCallableInternal extends LibraryCallable {
443+
bindingset[this]
444+
LibraryCallableInternal() { any() }
445+
446+
/**
447+
* Gets a call to this library callable.
448+
*
449+
* Same as `getACall()` but is evaluated later and may depend negatively on `getACall()`.
450+
*/
451+
DataFlow::InvokeNode getACallStage2() { none() }
452+
}
453+
441454
private predicate isParameterNodeImpl(Node p, DataFlowCallable c, ParameterPosition pos) {
442455
exists(Parameter parameter |
443456
parameter = c.asSourceCallable().(Function).getParameter(pos.asPositional()) and
@@ -1014,7 +1027,11 @@ DataFlowCallable viableCallable(DataFlowCall node) {
10141027
or
10151028
exists(LibraryCallable callable |
10161029
result = MkLibraryCallable(callable) and
1017-
node.asOrdinaryCall() = [callable.getACall(), callable.getACallSimple()]
1030+
node.asOrdinaryCall() =
1031+
[
1032+
callable.getACall(), callable.getACallSimple(),
1033+
callable.(LibraryCallableInternal).getACallStage2()
1034+
]
10181035
)
10191036
or
10201037
result.asSourceCallableNotExterns() = node.asImpliedLambdaCall()

javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSummaryPrivate.qll

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ private import sharedlib.DataFlowImplCommon
99
private import sharedlib.FlowSummaryImpl::Private as Private
1010
private import sharedlib.FlowSummaryImpl::Public
1111
private import codeql.dataflow.internal.AccessPathSyntax as AccessPathSyntax
12+
private import semmle.javascript.internal.flow_summaries.ExceptionFlow
1213

1314
/**
1415
* A class of callables that are candidates for flow summary modeling.
@@ -131,7 +132,11 @@ ReturnKind getStandardReturnValueKind() { result = MkNormalReturnKind() and Stag
131132
private module FlowSummaryStepInput implements Private::StepsInputSig {
132133
DataFlowCall getACall(SummarizedCallable sc) {
133134
exists(LibraryCallable callable | callable = sc |
134-
result.asOrdinaryCall() = [callable.getACall(), callable.getACallSimple()]
135+
result.asOrdinaryCall() =
136+
[
137+
callable.getACall(), callable.getACallSimple(),
138+
callable.(LibraryCallableInternal).getACallStage2()
139+
]
135140
)
136141
}
137142
}

javascript/ql/lib/semmle/javascript/internal/flow_summaries/ExceptionFlow.qll

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
private import javascript
66
private import FlowSummaryUtil
77
private import semmle.javascript.dataflow.internal.AdditionalFlowInternal
8+
private import semmle.javascript.dataflow.internal.DataFlowPrivate
89
private import semmle.javascript.dataflow.FlowSummary
910
private import semmle.javascript.internal.flow_summaries.Promises
1011

@@ -22,14 +23,14 @@ private predicate isCallback(DataFlow::SourceNode node) {
2223
/**
2324
* Summary that propagates exceptions out of callbacks back to the caller.
2425
*/
25-
private class ExceptionFlowSummary extends SummarizedCallable {
26+
private class ExceptionFlowSummary extends SummarizedCallable, LibraryCallableInternal {
2627
ExceptionFlowSummary() { this = "Exception propagator" }
2728

28-
override DataFlow::CallNode getACall() {
29+
override DataFlow::CallNode getACallStage2() {
2930
not exists(result.getACallee()) and
31+
not exists(SummarizedCallable c | result = [c.getACall(), c.getACallSimple()]) and
3032
// Avoid a few common cases where the exception should not propagate back
31-
not result.getCalleeName() =
32-
["then", "catch", "finally", "addEventListener", EventEmitter::on()] and
33+
not result.getCalleeName() = ["addEventListener", EventEmitter::on()] and
3334
not result = promiseConstructorRef().getAnInvocation() and
3435
// Restrict to cases where a callback is known to flow in, as lambda flow in DataFlowImplCommon blows up otherwise
3536
isCallback(result.getAnArgument().getALocalSource())

0 commit comments

Comments
 (0)