Skip to content

Commit 961ecfb

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #187 from esben-semmle/js/additional-whitelisting-form-unbound-event-handlers
Approved by asger-semmle
2 parents 6266d8b + 52013f3 commit 961ecfb

File tree

4 files changed

+100
-27
lines changed

4 files changed

+100
-27
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
| **Query** | **Expected impact** | **Change** |
1414
|--------------------------------|----------------------------|----------------------------------------------|
1515
| Regular expression injection | Fewer false-positive results | This rule now identifies calls to `String.prototype.search` with more precision. |
16+
| Unbound event handler receiver | Fewer false-positive results | This rule now recognizes additional ways class methods can be bound. |
1617

1718

1819
## Changes to QL libraries

‎javascript/ql/src/Expressions/UnboundEventHandlerReceiver.ql‎

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,20 +10,48 @@
1010
import javascript
1111

1212
/**
13-
* Holds if the receiver of `method` is bound in a method of its class.
13+
* Holds if the receiver of `method` is bound.
1414
*/
1515
private predicate isBoundInMethod(MethodDeclaration method) {
1616
exists (DataFlow::ThisNode thiz, MethodDeclaration bindingMethod |
1717
bindingMethod.getDeclaringClass() = method.getDeclaringClass() and
1818
not bindingMethod.isStatic() and
19-
thiz.getBinder().getAstNode() = bindingMethod.getBody() and
20-
exists (DataFlow::Node rhs, DataFlow::MethodCallNode bind |
21-
// this.<methodName> = <expr>.bind(...)
22-
thiz.hasPropertyWrite(method.getName(), rhs) and
23-
bind.flowsTo(rhs) and
24-
bind.getMethodName() = "bind"
19+
thiz.getBinder().getAstNode() = bindingMethod.getBody() |
20+
// require("auto-bind")(this)
21+
thiz.flowsTo(DataFlow::moduleImport("auto-bind").getACall().getArgument(0))
22+
or
23+
exists (string name |
24+
name = method.getName() |
25+
exists (DataFlow::Node rhs, DataFlow::MethodCallNode bind |
26+
// this.<methodName> = <expr>.bind(...)
27+
thiz.hasPropertyWrite(name, rhs) and
28+
bind.flowsTo(rhs) and
29+
bind.getMethodName() = "bind"
30+
)
31+
or
32+
exists (DataFlow::MethodCallNode bindAll |
33+
bindAll.getMethodName() = "bindAll" and
34+
thiz.flowsTo(bindAll.getArgument(0)) |
35+
// _.bindAll(this, <name1>)
36+
bindAll.getArgument(1).mayHaveStringValue(name)
37+
or
38+
// _.bindAll(this, [<name1>, <name2>])
39+
exists (DataFlow::ArrayLiteralNode names |
40+
names.flowsTo(bindAll.getArgument(1)) and
41+
names.getAnElement().mayHaveStringValue(name)
42+
)
43+
)
2544
)
2645
)
46+
or
47+
exists (Expr decoration, string name |
48+
decoration = method.getADecorator().getExpression() and
49+
name.regexpMatch("(?i).*(bind|bound).*") |
50+
// @autobind
51+
decoration.(Identifier).getName() = name or
52+
// @action.bound
53+
decoration.(PropAccess).getPropertyName() = name
54+
)
2755
}
2856

2957
/**

‎javascript/ql/test/query-tests/Expressions/UnboundEventHandlerReceiver/UnboundEventHandlerReceiver.expected‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
1-
| tst.js:56:18:56:40 | onClick ... bound1} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:14:9:14:12 | this | this | tst.js:13:5:15:5 | unbound ... ;\\n } | unbound1 |
2-
| tst.js:57:18:57:40 | onClick ... bound2} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:18:15:18:18 | this | this | tst.js:17:5:19:5 | unbound ... ;\\n } | unbound2 |
3-
| tst.js:58:18:58:35 | onClick={unbound3} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:22:15:22:18 | this | this | tst.js:21:5:23:5 | unbound ... ;\\n } | unbound3 |
1+
| tst.js:27:18:27:40 | onClick ... bound1} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:56:9:56:12 | this | this | tst.js:55:5:57:5 | unbound ... ;\\n } | unbound1 |
2+
| tst.js:28:18:28:40 | onClick ... bound2} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:60:15:60:18 | this | this | tst.js:59:5:61:5 | unbound ... ;\\n } | unbound2 |
3+
| tst.js:29:18:29:35 | onClick={unbound3} | The receiver of this event handler call is unbound, `$@` will be `undefined` in the call to $@ | tst.js:64:15:64:18 | this | this | tst.js:63:5:65:5 | unbound ... ;\\n } | unbound3 |

‎javascript/ql/test/query-tests/Expressions/UnboundEventHandlerReceiver/tst.js‎

Lines changed: 61 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,55 @@
11
import React from 'react';
2+
import autoBind from 'auto-bind';
3+
4+
class Component0 extends React.Component {
5+
6+
render() {
7+
return <div>
8+
<div onClick={this.bound_throughAutoBind}/> // OK
9+
</div>
10+
}
11+
12+
constructor(props) {
13+
super(props);
14+
autoBind(this);
15+
}
16+
17+
bound_throughAutoBind() {
18+
this.setState({ });
19+
}
20+
}
21+
22+
class Component1 extends React.Component {
23+
24+
render() {
25+
var unbound3 = this.unbound3;
26+
return <div>
27+
<div onClick={this.unbound1}/> // NOT OK
28+
<div onClick={this.unbound2}/> // NOT OK
29+
<div onClick={unbound3}/> // NOT OK
30+
<div onClick={this.bound_throughBindInConstructor}/> // OK
31+
<div onClick={this.bound_throughDeclaration}/> // OK
32+
<div onClick={this.unbound_butNoThis}/> // OK
33+
<div onClick={this.unbound_butNoThis2}/> // OK
34+
<div onClick={(e) => this.unbound_butInvokedSafely(e)}/> // OK
35+
<div onClick={this.bound_throughBindInMethod}/> // OK
36+
<div onClick={this.bound_throughNonSyntacticBindInConstructor}/> // OK
37+
<div onClick={this.bound_throughBindAllInConstructor1}/> // OK
38+
<div onClick={this.bound_throughBindAllInConstructor2}/> // OK
39+
<div onClick={this.bound_throughDecorator_autobind}/> // OK
40+
<div onClick={this.bound_throughDecorator_actionBound}/> // OK
41+
</div>
42+
}
243

3-
class Component extends React.Component {
444
constructor(props) {
545
super(props);
646
this.bound_throughBindInConstructor = this.bound_throughBindInConstructor.bind(this);
747
this.bound_throughBizarreBind = foo.bar.bind(baz);
848
var cmp = this;
949
var bound = (cmp.bound_throughNonSyntacticBindInConstructor.bind(this));
1050
(cmp).bound_throughNonSyntacticBindInConstructor = bound;
51+
_.bindAll(this, 'bound_throughBindAllInConstructor1');
52+
_.bindAll(this, ['bound_throughBindAllInConstructor2']);
1153
}
1254

1355
unbound1() {
@@ -50,22 +92,6 @@ class Component extends React.Component {
5092
this.setState({ });
5193
}
5294

53-
render() {
54-
var unbound3 = this.unbound3;
55-
return <div>
56-
<div onClick={this.unbound1}/> // NOT OK
57-
<div onClick={this.unbound2}/> // NOT OK
58-
<div onClick={unbound3}/> // NOT OK
59-
<div onClick={this.bound_throughBindInConstructor}/> // OK
60-
<div onClick={this.bound_throughDeclaration}/> // OK
61-
<div onClick={this.unbound_butNoThis}/> // OK
62-
<div onClick={this.unbound_butNoThis2}/> // OK
63-
<div onClick={(e) => this.unbound_butInvokedSafely(e)}/> // OK
64-
<div onClick={this.bound_throughBindInMethod}/> // OK
65-
<div onClick={this.bound_throughNonSyntacticBindInConstructor}/> // OK
66-
</div>
67-
}
68-
6995
componentWillMount() {
7096
this.bound_throughBindInMethod = this.bound_throughBindInMethod.bind(this);
7197
}
@@ -74,6 +100,24 @@ class Component extends React.Component {
74100
this.setState({ });
75101
}
76102

103+
bound_throughBindAllInConstructor1() {
104+
this.setState({ });
105+
}
106+
107+
bound_throughBindAllInConstructor2() {
108+
this.setState({ });
109+
}
110+
111+
@autobind
112+
bound_throughDecorator_autobind() {
113+
this.setState({ });
114+
}
115+
116+
@action.bound
117+
bound_throughDecorator_actionBound() {
118+
this.setState({ });
119+
}
120+
77121
}
78122

79123
// semmle-extractor-options: --experimental

0 commit comments

Comments
 (0)