diff --git a/change-notes/1.25/analysis-javascript.md b/change-notes/1.25/analysis-javascript.md index 2aada0cbd86a..2e4d2b280c5b 100644 --- a/change-notes/1.25/analysis-javascript.md +++ b/change-notes/1.25/analysis-javascript.md @@ -46,6 +46,7 @@ | Hard-coded credentials (`js/hardcoded-credentials`) | More results | This query now recognizes hard-coded credentials sent via HTTP authorization headers. | | Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes additional url scheme checks. | | Misspelled variable name (`js/misspelled-variable-name`) | Message changed | The message for this query now correctly identifies the misspelled variable in additional cases. | +| Non-linear pattern (`js/non-linear-pattern`) | Fewer duplicates and message changed | This query now generates fewer duplicate alerts and has a clearer explanation in case of type annotations used in a pattern. | | Prototype pollution in utility function (`js/prototype-pollution-utility`) | More results | This query now recognizes additional utility functions as vulnerable to prototype polution. | | Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. | | Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. | diff --git a/javascript/ql/src/LanguageFeatures/NonLinearPattern.qhelp b/javascript/ql/src/LanguageFeatures/NonLinearPattern.qhelp index 3de37f49759e..7690d9df1cee 100644 --- a/javascript/ql/src/LanguageFeatures/NonLinearPattern.qhelp +++ b/javascript/ql/src/LanguageFeatures/NonLinearPattern.qhelp @@ -8,6 +8,11 @@ If the same pattern variable is bound multiple times in the same object or array binding overwrites all of the earlier ones. This is most likely unintended and should be avoided.

+

+In TypeScript, a common mistake is to try to write type annotations inside a pattern. This is not +possible, and the type annotation should come after the pattern. +

+ @@ -34,6 +39,21 @@ From context, it appears that the second binding should have been for variable < +

+This can sometimes happen in TypeScript, due to the apparant similarity between property patterns +and type annotations. In the following example, the function uses a pattern parameter with properties x +and y. These appear to have type number, but are in fact untyped properties both stored in a variable named number. +

+ + + +

+It is not possible to specify type annotations inside a pattern. The correct way is to specify the type +after the parameter: +

+ + +
  • Mozilla Developer Network: Destructuring assignment.
  • diff --git a/javascript/ql/src/LanguageFeatures/NonLinearPattern.ql b/javascript/ql/src/LanguageFeatures/NonLinearPattern.ql index faa5392806e1..5a8bf72dbb22 100644 --- a/javascript/ql/src/LanguageFeatures/NonLinearPattern.ql +++ b/javascript/ql/src/LanguageFeatures/NonLinearPattern.ql @@ -14,12 +14,55 @@ import javascript -from BindingPattern p, string n, VarDecl v, VarDecl w +class RootDestructuringPattern extends BindingPattern { + RootDestructuringPattern() { + this instanceof DestructuringPattern and + not this = any(PropertyPattern p).getValuePattern() and + not this = any(ArrayPattern p).getAnElement() + } + + /** Holds if this pattern has multiple bindings for `name`. */ + predicate hasConflictingBindings(string name) { + exists(VarRef v, VarRef w | + v = getABindingVarRef() and + w = getABindingVarRef() and + name = v.getName() and + name = w.getName() and + v != w + ) + } + + /** Gets the first occurrence of the conflicting binding `name`. */ + VarDecl getFirstClobberedVarDecl(string name) { + hasConflictingBindings(name) and + result = + min(VarDecl decl | + decl = getABindingVarRef() and decl.getName() = name + | + decl order by decl.getLocation().getStartLine(), decl.getLocation().getStartColumn() + ) + } + + /** Holds if variables in this pattern may resemble type annotations. */ + predicate resemblesTypeAnnotation() { + hasConflictingBindings(_) and // Restrict size of predicate. + this instanceof Parameter and + this instanceof ObjectPattern and + not exists(getTypeAnnotation()) and + getFile().getFileType().isTypeScript() + } +} + +from RootDestructuringPattern p, string n, VarDecl v, VarDecl w, string message where - v = p.getABindingVarRef() and + v = p.getFirstClobberedVarDecl(n) and w = p.getABindingVarRef() and - v.getName() = n and w.getName() = n and v != w and - v.getLocation().startsBefore(w.getLocation()) -select w, "Repeated binding of pattern variable '" + n + "' previously bound $@.", v, "here" + if p.resemblesTypeAnnotation() + then + message = + "The pattern variable '" + n + + "' appears to be a type, but is a variable previously bound $@." + else message = "Repeated binding of pattern variable '" + n + "' previously bound $@." +select w, message, v, "here" diff --git a/javascript/ql/src/LanguageFeatures/examples/NonLinearPatternTS.ts b/javascript/ql/src/LanguageFeatures/examples/NonLinearPatternTS.ts new file mode 100644 index 000000000000..d19e8bb327fc --- /dev/null +++ b/javascript/ql/src/LanguageFeatures/examples/NonLinearPatternTS.ts @@ -0,0 +1,3 @@ +function distance({x: number, y: number}) { + return Math.sqrt(x*x + y*y); +} diff --git a/javascript/ql/src/LanguageFeatures/examples/NonLinearPatternTSGood.ts b/javascript/ql/src/LanguageFeatures/examples/NonLinearPatternTSGood.ts new file mode 100644 index 000000000000..ee0f4305507b --- /dev/null +++ b/javascript/ql/src/LanguageFeatures/examples/NonLinearPatternTSGood.ts @@ -0,0 +1,3 @@ +function distance({x, y}: {x: number, y: number}) { + return Math.sqrt(x*x + y*y); +} diff --git a/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPattern.expected b/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPattern.expected index 17c9b7031d9b..f4e0790d55fa 100644 --- a/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPattern.expected +++ b/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPattern.expected @@ -1,6 +1,11 @@ +| NonLinearPatternTS.ts:1:34:1:39 | number | The pattern variable 'number' appears to be a type, but is a variable previously bound $@. | NonLinearPatternTS.ts:1:23:1:28 | number | here | | ts-test.ts:3:13:3:13 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:3:10:3:10 | x | here | | ts-test.ts:8:16:8:16 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:8:10:8:10 | x | here | | ts-test.ts:11:10:11:10 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:11:7:11:7 | x | here | +| ts-test.ts:21:8:21:13 | string | The pattern variable 'string' appears to be a type, but is a variable previously bound $@. | ts-test.ts:20:8:20:13 | string | here | +| ts-test.ts:32:16:32:16 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:30:12:30:12 | x | here | +| ts-test.ts:34:20:34:20 | x | Repeated binding of pattern variable 'x' previously bound $@. | ts-test.ts:30:12:30:12 | x | here | +| ts-test.ts:40:27:40:32 | string | Repeated binding of pattern variable 'string' previously bound $@. | ts-test.ts:40:16:40:21 | string | here | | tst.js:3:13:3:13 | x | Repeated binding of pattern variable 'x' previously bound $@. | tst.js:3:10:3:10 | x | here | | tst.js:8:16:8:16 | x | Repeated binding of pattern variable 'x' previously bound $@. | tst.js:8:10:8:10 | x | here | | tst.js:11:10:11:10 | x | Repeated binding of pattern variable 'x' previously bound $@. | tst.js:11:7:11:7 | x | here | diff --git a/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPatternTS.ts b/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPatternTS.ts new file mode 100644 index 000000000000..d19e8bb327fc --- /dev/null +++ b/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPatternTS.ts @@ -0,0 +1,3 @@ +function distance({x: number, y: number}) { + return Math.sqrt(x*x + y*y); +} diff --git a/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPatternTSGood.ts b/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPatternTSGood.ts new file mode 100644 index 000000000000..ee0f4305507b --- /dev/null +++ b/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/NonLinearPatternTSGood.ts @@ -0,0 +1,3 @@ +function distance({x, y}: {x: number, y: number}) { + return Math.sqrt(x*x + y*y); +} diff --git a/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/ts-test.ts b/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/ts-test.ts index 0e6b79f4675f..1198d2c5ff74 100644 --- a/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/ts-test.ts +++ b/javascript/ql/test/query-tests/LanguageFeatures/NonLinearPattern/ts-test.ts @@ -15,3 +15,27 @@ var { x: x, x: y } = o; // OK var { p = x, q = x } = o; + +function f({ + x: string, + y: string // NOT OK +}) { +} + +function g({x, y}: {x: string, y: string}) { // OK +} + +function blah(arg) { + var { + x: x, + y: { + x: x, // NOT OK + y: { + x: x // NOT OK + } + } + } = arg; +} + +function h({x: string, y: string}: any) { // NOT OK +}