Skip to content

Commit 475d216

Browse files
codeql-cicodeql-ci
authored
Merge pull request #5087 from erik-krogh/immutable
Approved by asgerf
2 parents bed10ad + 504db87 commit 475d216

File tree

8 files changed

+295
-1
lines changed

8 files changed

+295
-1
lines changed

javascript/change-notes/202020-12-22-execa.md renamed to javascript/change-notes/2020-12-22-execa.md

File renamed without changes.

‎javascript/change-notes/2021-02-08-immutable.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
lgtm,codescanning
2+
* The dataflow libraries now model dataflow in the Immutable.js library.
3+
Affected packages are
4+
[Immutable.js](https://npmjs.com/package/immutable)

‎javascript/ql/src/javascript.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ import semmle.javascript.frameworks.Electron
8585
import semmle.javascript.frameworks.EventEmitter
8686
import semmle.javascript.frameworks.Files
8787
import semmle.javascript.frameworks.Firebase
88+
import semmle.javascript.frameworks.Immutable
8889
import semmle.javascript.frameworks.jQuery
8990
import semmle.javascript.frameworks.JWT
9091
import semmle.javascript.frameworks.Handlebars

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -659,9 +659,15 @@ module PseudoProperties {
659659
*/
660660
pragma[inline]
661661
string mapValueKnownKey(DataFlow::Node key) {
662-
result = pseudoProperty("mapValue", any(string s | key.mayHaveStringValue(s)))
662+
result = mapValueKey(any(string s | key.mayHaveStringValue(s)))
663663
}
664664

665+
/**
666+
* Gets a pseudo-property for the location of a map value where the key is `key`.
667+
*/
668+
bindingset[key]
669+
string mapValueKey(string key) { result = pseudoProperty("mapValue", key) }
670+
665671
/**
666672
* Gets a pseudo-property for the location of a map value where the key is `key`.
667673
*/

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

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
/**
2+
* Provides classes and predicates for reasoning about [immutable](https://www.npmjs.com/package/immutable).
3+
*/
4+
5+
import javascript
6+
7+
/**
8+
* Provides classes implementing data-flow for Immutable.
9+
*
10+
* The implemention rely on the flowsteps implemented in `Collections.qll`.
11+
*/
12+
private module Immutable {
13+
/**
14+
* An API entrypoint for the global `Immutable` variable.
15+
*/
16+
private class ImmutableGlobalEntry extends API::EntryPoint {
17+
ImmutableGlobalEntry() { this = "ImmutableGlobalEntry" }
18+
19+
override DataFlow::SourceNode getAUse() { result = DataFlow::globalVarRef("Immutable") }
20+
21+
override DataFlow::Node getARhs() { none() }
22+
}
23+
24+
/**
25+
* An import of the `Immutable` library.
26+
*/
27+
API::Node immutableImport() {
28+
result = API::moduleImport("immutable")
29+
or
30+
result = API::root().getASuccessor(any(ImmutableGlobalEntry i))
31+
}
32+
33+
/**
34+
* An instance of any immutable collection.
35+
*
36+
* This predicate keeps track of which values in the program are Immutable collections.
37+
*/
38+
API::Node immutableCollection() {
39+
// keep this predicate in sync with the constructors defined in `storeStep`/`step`.
40+
result =
41+
immutableImport()
42+
.getMember(["Map", "OrderedMap", "List", "Stack", "Set", "OrderedSet", "fromJS", "merge"])
43+
.getReturn()
44+
or
45+
result = immutableImport().getMember("Record").getReturn().getReturn()
46+
or
47+
result =
48+
immutableImport()
49+
.getMember(["List", "Set", "OrderedSet", "Stack"])
50+
.getMember("of")
51+
.getReturn()
52+
or
53+
result = immutableCollection().getMember(["set", "map", "filter", "push", "merge"]).getReturn()
54+
}
55+
56+
/**
57+
* Gets the immutable collection where `pred` has been stored using the name `prop`.
58+
*/
59+
DataFlow::SourceNode storeStep(DataFlow::Node pred, string prop) {
60+
// Immutable.Map() and Immutable.fromJS().
61+
exists(DataFlow::CallNode call |
62+
call = immutableImport().getMember(["Map", "OrderedMap", "fromJS"]).getACall()
63+
|
64+
pred = call.getOptionArgument(0, prop) and
65+
result = call
66+
)
67+
or
68+
// Immutable.List()
69+
exists(DataFlow::CallNode call, DataFlow::ArrayCreationNode arr |
70+
call = immutableImport().getMember(["List", "Stack", "Set", "OrderedSet"]).getACall()
71+
|
72+
arr = call.getArgument(0).getALocalSource() and
73+
exists(int i |
74+
prop = DataFlow::PseudoProperties::arrayElement(i) and
75+
pred = arr.getElement(i) and
76+
result = call
77+
)
78+
)
79+
or
80+
// collection.set(key, value)
81+
exists(DataFlow::CallNode call | call = immutableCollection().getMember("set").getACall() |
82+
call.getArgument(0).mayHaveStringValue(prop) and
83+
pred = call.getArgument(1) and
84+
result = call
85+
)
86+
or
87+
// list.push(x)
88+
exists(DataFlow::CallNode call | call = immutableCollection().getMember("push").getACall() |
89+
pred = call.getArgument(0) and
90+
result = call and
91+
prop = DataFlow::PseudoProperties::arrayElement()
92+
)
93+
or
94+
// Immutable.Record({defaults})({values}).
95+
exists(API::CallNode factoryCall, API::CallNode recordCall |
96+
factoryCall = immutableImport().getMember("Record").getACall() and
97+
recordCall = factoryCall.getReturn().getACall()
98+
|
99+
pred = [factoryCall, recordCall].getOptionArgument(0, prop) and
100+
result = recordCall
101+
)
102+
or
103+
// List/Set/Stack.of(values)
104+
exists(API::CallNode call |
105+
call =
106+
immutableImport()
107+
.getMember(["List", "Set", "OrderedSet", "Stack"])
108+
.getMember("of")
109+
.getACall()
110+
|
111+
pred = call.getAnArgument() and
112+
result = call and
113+
prop = DataFlow::PseudoProperties::arrayElement()
114+
)
115+
}
116+
117+
/**
118+
* Gets the value that was stored in the immutable collection `pred` under the name `prop`.
119+
*/
120+
DataFlow::Node loadStep(DataFlow::Node pred, string prop) {
121+
// map.get()
122+
exists(DataFlow::MethodCallNode call |
123+
call = immutableCollection().getMember("get").getACall()
124+
|
125+
call.getArgument(0).mayHaveStringValue(prop) and
126+
pred = call.getReceiver() and
127+
result = call
128+
)
129+
}
130+
131+
/**
132+
* Gets an immutable collection that contains all the elements from `pred`.
133+
*/
134+
DataFlow::SourceNode step(DataFlow::Node pred) {
135+
// map.set() / list.push() copies all existing values
136+
exists(DataFlow::CallNode call |
137+
call = immutableCollection().getMember(["set", "push"]).getACall()
138+
|
139+
pred = call.getReceiver() and
140+
result = call
141+
)
142+
or
143+
// toJS()/toList() on any immutable collection converts it to a plain JavaScript object/array (and vice versa for `fromJS`).
144+
exists(DataFlow::CallNode call |
145+
call = immutableCollection().getMember(["toJS", "toList"]).getACall()
146+
|
147+
pred = call.getReceiver() and
148+
result = call
149+
)
150+
or
151+
// Immutable.merge(x, y)
152+
exists(DataFlow::CallNode call | call = immutableImport().getMember("merge").getACall() |
153+
pred = call.getAnArgument() and
154+
result = call
155+
)
156+
or
157+
// collection.merge(other)
158+
exists(DataFlow::CallNode call | call = immutableCollection().getMember("merge").getACall() |
159+
pred = [call.getAnArgument(), call.getReceiver()] and
160+
result = call
161+
)
162+
}
163+
164+
/**
165+
* A dataflow step for an immutable collection.
166+
*/
167+
class ImmutableConstructionStep extends DataFlow::AdditionalFlowStep {
168+
ImmutableConstructionStep() { this = [loadStep(_, _), storeStep(_, _), step(_)] }
169+
170+
override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
171+
this = loadStep(pred, prop) and
172+
succ = this
173+
}
174+
175+
override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
176+
this = storeStep(pred, prop) and
177+
succ = this
178+
}
179+
180+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
181+
this = step(pred) and
182+
succ = this
183+
}
184+
}
185+
}

‎javascript/ql/test/library-tests/frameworks/Immutable/immutable.js‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
var obj = { a: source("a"), b: source("b1") };
2+
sink(obj["a"]); // NOT OK
3+
4+
const { Map, fromJS, List, OrderedMap, Record, merge, Stack, Set, OrderedSet } = require('immutable');
5+
6+
const map1 = Map(obj);
7+
8+
sink(map1.get("b")); // NOT OK
9+
10+
const map2 = map1.set('c', "safe");
11+
sink(map1.get("a")); // NOT OK
12+
sink(map2.get("a")); // NOT OK
13+
sink(map2.get("b")); // OK - but still flagged [INCONSISTENCY]
14+
15+
const map3 = map2.set("d", source("d"));
16+
sink(map1.get("d")); // OK
17+
sink(map3.get("d")); // NOT OK
18+
19+
20+
sink(map3.toJS()["a"]); // NOT OK
21+
22+
sink(fromJS({"e": source("e")}).get("e")); // NOT OK
23+
24+
const l1 = List([source(), "foobar"]);
25+
l1.forEach(x => sink(x)); // NOT OK
26+
27+
l1.map(x => "safe").forEach(x => sink(x)); // OK
28+
29+
List(["safe"]).map(x => source()).forEach(x => sink(x)); // NOT OK
30+
31+
List([source()]).map(x => x).filter(x => true).toList().forEach(x => sink(x)); // NOT OK
32+
33+
List(["safe"]).push(source()).forEach(x => sink(x)); // NOT OK
34+
35+
36+
const map4 = OrderedMap({}).set("f", source());
37+
sink(map4.get("f")); // NOT OK
38+
39+
const map5 = Record({a: source(), b: null, c: null})({b: source()});
40+
sink(map5.get("a")); // NOT OK
41+
sink(map5.get("b")); // NOT OK
42+
sink(map5.get("c")); // OK
43+
44+
const map6 = merge(Map({}), Record({a: source()})());
45+
sink(map6.get("a")); // NOT OK
46+
47+
const map7 = map6.merge(Map({b: source()}));
48+
sink(map7.get("b")); // NOT OK
49+
50+
Stack.of(source(), "foobar").forEach(x => sink(x)); // NOT OK
51+
52+
List.of(source()).filter(x => true).toList().forEach(x => sink(x)); // NOT OK
53+
54+
Set.of(source()).filter(x => true).toList().forEach(x => sink(x)); // NOT OK
55+
56+
Set([source()]).filter(x => true).toList().forEach(x => sink(x)); // NOT OK
57+
58+
OrderedSet([source()]).filter(x => true).toList().forEach(x => sink(x)); // NOT OK

‎javascript/ql/test/library-tests/frameworks/Immutable/tests.expected‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
| immutable.js:1:16:1:26 | source("a") | immutable.js:2:6:2:13 | obj["a"] |
2+
| immutable.js:1:16:1:26 | source("a") | immutable.js:11:6:11:18 | map1.get("a") |
3+
| immutable.js:1:16:1:26 | source("a") | immutable.js:12:6:12:18 | map2.get("a") |
4+
| immutable.js:1:16:1:26 | source("a") | immutable.js:20:6:20:21 | map3.toJS()["a"] |
5+
| immutable.js:1:32:1:43 | source("b1") | immutable.js:8:6:8:18 | map1.get("b") |
6+
| immutable.js:1:32:1:43 | source("b1") | immutable.js:13:6:13:18 | map2.get("b") |
7+
| immutable.js:15:28:15:38 | source("d") | immutable.js:17:6:17:18 | map3.get("d") |
8+
| immutable.js:22:19:22:29 | source("e") | immutable.js:22:6:22:40 | fromJS( ... et("e") |
9+
| immutable.js:24:18:24:25 | source() | immutable.js:25:22:25:22 | x |
10+
| immutable.js:29:25:29:32 | source() | immutable.js:29:53:29:53 | x |
11+
| immutable.js:31:7:31:14 | source() | immutable.js:31:75:31:75 | x |
12+
| immutable.js:33:21:33:28 | source() | immutable.js:33:49:33:49 | x |
13+
| immutable.js:36:38:36:45 | source() | immutable.js:37:6:37:18 | map4.get("f") |
14+
| immutable.js:39:25:39:32 | source() | immutable.js:40:6:40:18 | map5.get("a") |
15+
| immutable.js:39:58:39:65 | source() | immutable.js:41:6:41:18 | map5.get("b") |
16+
| immutable.js:44:40:44:47 | source() | immutable.js:45:6:45:18 | map6.get("a") |
17+
| immutable.js:47:33:47:40 | source() | immutable.js:48:6:48:18 | map7.get("b") |
18+
| immutable.js:50:10:50:17 | source() | immutable.js:50:48:50:48 | x |
19+
| immutable.js:52:9:52:16 | source() | immutable.js:52:64:52:64 | x |
20+
| immutable.js:54:8:54:15 | source() | immutable.js:54:63:54:63 | x |
21+
| immutable.js:56:6:56:13 | source() | immutable.js:56:62:56:62 | x |
22+
| immutable.js:58:13:58:20 | source() | immutable.js:58:69:58:69 | x |

‎javascript/ql/test/library-tests/frameworks/Immutable/tests.ql‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import javascript
2+
private import semmle.javascript.dataflow.internal.StepSummary
3+
4+
class Config extends DataFlow::Configuration {
5+
Config() { this = "Config" }
6+
7+
override predicate isSource(DataFlow::Node source) {
8+
source.(DataFlow::CallNode).getCalleeName() = "source"
9+
}
10+
11+
override predicate isSink(DataFlow::Node sink) {
12+
exists(DataFlow::CallNode call | call.getCalleeName() = "sink" | call.getAnArgument() = sink)
13+
}
14+
}
15+
16+
query predicate dataFlow(DataFlow::Node pred, DataFlow::Node succ) {
17+
any(Config c).hasFlow(pred, succ)
18+
}

0 commit comments

Comments
 (0)