Skip to content

Commit ef1d08d

Browse files
author
Max Schaefer
committed
JavaScript: Restructure implementation of DataFlow::SourceNode.
It now uses a facade pattern similar to `InvokeNode`: the range of the class is defined by an abstract class `DataFlow::SourceNode::Range`, while the actual behaviour is defined by the (no longer abstract) `SourceNode` class itself. Clients that want to add new source nodes need to extend `DataFlow::SourceNode::Range`, those that want to refine the behaviour of existing source nodes should extend `DataFlow::SourceNode` itself. While this is technically a breaking API change, I think separating the two aspects in this way is cleaner and makes it easier to use, and improves performance as well.
1 parent aa6b89d commit ef1d08d

10 files changed

Lines changed: 106 additions & 60 deletions

File tree

‎change-notes/1.20/analysis-javascript.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,3 +33,6 @@
3333
| Uncontrolled data used in path expression | Fewer false-positive results | This rule now recognizes the Express `root` option, which prevents path traversal. |
3434

3535
## Changes to QL libraries
36+
37+
* `DataFlow::SourceNode` is no longer an abstract class; to add new source nodes, extend `DataFlow::SourceNode::Range` instead.
38+
* Subclasses of `DataFlow::PropRead` are no longer automatically made source nodes; you now need to additionally define a corresponding subclass of `DataFlow::SourceNode::Range` to achieve this.

‎javascript/ql/src/semmle/javascript/EmailClients.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import javascript
33
/**
44
* An operation that sends an email.
55
*/
6-
abstract class EmailSender extends DataFlow::DefaultSourceNode {
6+
abstract class EmailSender extends DataFlow::SourceNode {
77
/**
88
* Gets a data flow node holding the plaintext version of the email body.
99
*/

‎javascript/ql/src/semmle/javascript/StandardLibrary.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ private class AnalyzedThisInArrayIterationFunction extends AnalyzedNode, DataFlo
9090
/**
9191
* A definition of a `Promise` object.
9292
*/
93-
abstract class PromiseDefinition extends DataFlow::DefaultSourceNode {
93+
abstract class PromiseDefinition extends DataFlow::SourceNode {
9494
/** Gets the executor function of this promise object. */
9595
abstract DataFlow::FunctionNode getExecutor();
9696

‎javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,6 +620,20 @@ module DataFlow {
620620
abstract class PropRead extends PropRef, SourceNode {
621621
}
622622

623+
/**
624+
* A property read, considered as a source node.
625+
*
626+
* Note that we cannot simplify the characteristic predicate to `this instanceof PropRead`,
627+
* since `PropRead` is itself a subclass of `SourceNode`.
628+
*/
629+
private class PropReadAsSourceNode extends SourceNode::Range {
630+
PropReadAsSourceNode() {
631+
this = TPropNode(any(PropertyPattern p)) or
632+
this instanceof RestPatternNode or
633+
this instanceof ElementPatternNode
634+
}
635+
}
636+
623637
/**
624638
* A property access in rvalue position.
625639
*/

‎javascript/ql/src/semmle/javascript/dataflow/Nodes.qll‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
import javascript
88

99
/** A data flow node corresponding to a parameter. */
10-
class ParameterNode extends DataFlow::DefaultSourceNode {
10+
class ParameterNode extends DataFlow::SourceNode {
1111
Parameter p;
1212

1313
ParameterNode() { DataFlow::parameterNode(this, p) }
@@ -20,7 +20,7 @@ class ParameterNode extends DataFlow::DefaultSourceNode {
2020
}
2121

2222
/** A data flow node corresponding to a function invocation (with or without `new`). */
23-
class InvokeNode extends DataFlow::DefaultSourceNode {
23+
class InvokeNode extends DataFlow::SourceNode {
2424
DataFlow::Impl::InvokeNodeDef impl;
2525

2626
InvokeNode() { this = impl }
@@ -199,7 +199,7 @@ class NewNode extends InvokeNode {
199199
}
200200

201201
/** A data flow node corresponding to the `this` parameter in a function or `this` at the top-level. */
202-
class ThisNode extends DataFlow::Node, DataFlow::DefaultSourceNode {
202+
class ThisNode extends DataFlow::Node, DataFlow::SourceNode {
203203
ThisNode() {
204204
DataFlow::thisNode(this, _)
205205
}
@@ -232,7 +232,7 @@ class ThisNode extends DataFlow::Node, DataFlow::DefaultSourceNode {
232232
}
233233

234234
/** A data flow node corresponding to a global variable access. */
235-
class GlobalVarRefNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
235+
class GlobalVarRefNode extends DataFlow::ValueNode, DataFlow::SourceNode {
236236
override GlobalVarAccess astNode;
237237

238238
/** Gets the name of the global variable. */
@@ -246,8 +246,9 @@ class GlobalVarRefNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode
246246
*/
247247
DataFlow::SourceNode globalObjectRef() {
248248
// top-level `this`
249-
exists (ThisNode globalThis | result = globalThis |
250-
not exists(globalThis.getBinder())
249+
exists (StmtContainer sc |
250+
sc = result.(ThisNode).getBindingContainer() and
251+
not sc instanceof Function
251252
)
252253
or
253254
// DOM
@@ -274,7 +275,7 @@ DataFlow::SourceNode globalVarRef(string name) {
274275
}
275276

276277
/** A data flow node corresponding to a function definition. */
277-
class FunctionNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
278+
class FunctionNode extends DataFlow::ValueNode, DataFlow::SourceNode {
278279
override Function astNode;
279280

280281
/** Gets the `i`th parameter of this function. */
@@ -333,12 +334,12 @@ class FunctionNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
333334
}
334335

335336
/** A data flow node corresponding to an object literal expression. */
336-
class ObjectLiteralNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
337+
class ObjectLiteralNode extends DataFlow::ValueNode, DataFlow::SourceNode {
337338
override ObjectExpr astNode;
338339
}
339340

340341
/** A data flow node corresponding to an array literal expression. */
341-
class ArrayLiteralNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
342+
class ArrayLiteralNode extends DataFlow::ValueNode, DataFlow::SourceNode {
342343
override ArrayExpr astNode;
343344

344345
/** Gets the `i`th element of this array literal. */
@@ -376,7 +377,7 @@ class ArrayConstructorInvokeNode extends DataFlow::InvokeNode {
376377
* A data flow node corresponding to the creation or a new array, either through an array literal
377378
* or an invocation of the `Array` constructor.
378379
*/
379-
class ArrayCreationNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
380+
class ArrayCreationNode extends DataFlow::ValueNode, DataFlow::SourceNode {
380381
ArrayCreationNode() {
381382
this instanceof ArrayLiteralNode or
382383
this instanceof ArrayConstructorInvokeNode
@@ -401,7 +402,7 @@ class ArrayCreationNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode
401402
* For compatibility with old transpilers, we treat `import * from '...'`
402403
* as a default import as well.
403404
*/
404-
class ModuleImportNode extends DataFlow::DefaultSourceNode {
405+
class ModuleImportNode extends DataFlow::SourceNode {
405406
string path;
406407

407408
ModuleImportNode() {

‎javascript/ql/src/semmle/javascript/dataflow/Sources.qll‎

Lines changed: 66 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,17 @@
99
import javascript
1010

1111
/**
12-
* A source node for local data flow, that is, a node for which local
13-
* data flow cannot provide any information about its inputs.
12+
* A source node for local data flow, that is, a node from which local data flow is tracked.
1413
*
15-
* By default, functions, object and array expressions and JSX nodes
16-
* are considered sources, as well as expressions that have non-local
17-
* flow (such as calls and property accesses). Additional sources
18-
* can be modelled by extending this class with additional subclasses.
14+
* Examples include function parameters, imports and property accesses; see
15+
* `DataFlow::SourceNode::DefaultRange` for details. You can introduce new kinds of
16+
* source nodes by defining new subclasses of `DataFlow::SourceNode::Range`.
1917
*/
20-
abstract class SourceNode extends DataFlow::Node {
18+
class SourceNode extends DataFlow::Node {
19+
SourceNode() {
20+
this instanceof SourceNode::Range
21+
}
22+
2123
/**
2224
* Holds if this node flows into `sink` in zero or more local (that is,
2325
* intra-procedural) steps.
@@ -187,44 +189,63 @@ abstract class SourceNode extends DataFlow::Node {
187189
}
188190
}
189191

190-
/**
191-
* A data flow node that is considered a source node by default.
192-
*
193-
* Currently, the following nodes are source nodes:
194-
* - import specifiers
195-
* - non-destructuring function parameters
196-
* - property accesses
197-
* - function invocations
198-
* - `this` expressions
199-
* - global variable accesses
200-
* - function definitions
201-
* - class definitions
202-
* - object expressions
203-
* - array expressions
204-
* - JSX literals.
205-
*/
206-
class DefaultSourceNode extends SourceNode {
192+
module SourceNode {
193+
/**
194+
* A data flow node that should be considered a source node.
195+
*
196+
* Subclass this class to introduce new kinds of source nodes. If you want to refine
197+
* the definition of existing source nodes, subclass `DataFlow::SourceNode` instead.
198+
*/
199+
abstract cached class Range extends DataFlow::Node {
200+
}
201+
202+
/**
203+
* A data flow node that is considered a source node by default.
204+
*
205+
* Currently, the following nodes are source nodes:
206+
* - import specifiers
207+
* - function parameters
208+
* - `this` nodes
209+
* - property accesses
210+
* - function invocations
211+
* - global variable accesses
212+
* - function definitions
213+
* - class definitions
214+
* - object expressions
215+
* - array expressions
216+
* - JSX literals
217+
*
218+
* This class is for internal use only and should not normally be used directly.
219+
*/
220+
class DefaultRange extends Range {
221+
DefaultRange() {
222+
exists (ASTNode astNode | this = DataFlow::valueNode(astNode) |
223+
astNode instanceof PropAccess or
224+
astNode instanceof Function or
225+
astNode instanceof ClassDefinition or
226+
astNode instanceof ObjectExpr or
227+
astNode instanceof ArrayExpr or
228+
astNode instanceof JSXNode or
229+
astNode instanceof GlobalVarAccess or
230+
astNode instanceof ExternalModuleReference
231+
)
232+
or
233+
exists (SsaExplicitDefinition ssa, VarDef def |
234+
this = DataFlow::ssaDefinitionNode(ssa) and def = ssa.getDef() |
235+
def instanceof ImportSpecifier
236+
)
237+
or
238+
DataFlow::parameterNode(this, _)
239+
or
240+
this instanceof DataFlow::Impl::InvokeNodeDef
241+
or
242+
DataFlow::thisNode(this, _)
243+
}
244+
}
245+
}
246+
247+
deprecated class DefaultSourceNode extends SourceNode {
207248
DefaultSourceNode() {
208-
exists (ASTNode astNode | this = DataFlow::valueNode(astNode) |
209-
astNode instanceof PropAccess or
210-
astNode instanceof Function or
211-
astNode instanceof ClassDefinition or
212-
astNode instanceof ObjectExpr or
213-
astNode instanceof ArrayExpr or
214-
astNode instanceof JSXNode or
215-
astNode instanceof GlobalVarAccess or
216-
astNode instanceof ExternalModuleReference
217-
)
218-
or
219-
exists (SsaExplicitDefinition ssa, VarDef def |
220-
this = DataFlow::ssaDefinitionNode(ssa) and def = ssa.getDef() |
221-
def instanceof ImportSpecifier
222-
)
223-
or
224-
DataFlow::parameterNode(this, _)
225-
or
226-
this instanceof DataFlow::Impl::InvokeNodeDef
227-
or
228-
DataFlow::thisNode(this, _)
249+
this instanceof SourceNode::DefaultRange
229250
}
230251
}

‎javascript/ql/src/semmle/javascript/frameworks/AngularJS/AngularJSCore.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ private predicate isAngularString(Expr s) {
4747
* String literals in Angular code are often used as identifiers or references, so we
4848
* want to track them.
4949
*/
50-
private class TrackStringsInAngularCode extends DataFlow::SourceNode, DataFlow::ValueNode {
50+
private class TrackStringsInAngularCode extends DataFlow::SourceNode::Range, DataFlow::ValueNode {
5151
TrackStringsInAngularCode() {
5252
isAngularString(astNode)
5353
}

‎javascript/ql/src/semmle/javascript/frameworks/HTTP.qll‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,7 +469,10 @@ module HTTP {
469469
* Gets a secret key used for signed cookies.
470470
*/
471471
abstract DataFlow::Node getASecretKey();
472+
}
472473

474+
private class CookieMiddlewareInstanceAsSourceNode extends DataFlow::SourceNode::Range {
475+
CookieMiddlewareInstanceAsSourceNode() { this instanceof CookieMiddlewareInstance }
473476
}
474477

475478
/**

‎javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@ module LodashUnderscore {
1313
abstract string getName();
1414
}
1515

16+
private class MemberAsSourceNode extends DataFlow::SourceNode::Range {
17+
MemberAsSourceNode() { this instanceof Member }
18+
}
19+
1620
/**
1721
* An import of `lodash` or `underscore` accessing a given member of that package.
1822
*/

‎javascript/ql/src/semmle/javascript/frameworks/ReactNative.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import javascript
66

77
module ReactNative {
88
/** A `WebView` JSX element. */
9-
class WebViewElement extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
9+
class WebViewElement extends DataFlow::ValueNode, DataFlow::SourceNode {
1010
override JSXElement astNode;
1111

1212
WebViewElement() {

0 commit comments

Comments
 (0)