From d57e93d5c63b803386f46965ba11a643cd76904b Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Mon, 1 Oct 2018 08:04:45 +0100 Subject: [PATCH 01/18] JavaScript: Fix typo in query help. (cherry picked from commit 1ab943c16b81784a22c31a3ca7277bddc1952174) --- javascript/ql/src/Expressions/ImplicitOperandConversion.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Expressions/ImplicitOperandConversion.qhelp b/javascript/ql/src/Expressions/ImplicitOperandConversion.qhelp index 7313c503e78d..667db45513e1 100644 --- a/javascript/ql/src/Expressions/ImplicitOperandConversion.qhelp +++ b/javascript/ql/src/Expressions/ImplicitOperandConversion.qhelp @@ -39,7 +39,7 @@ property of the name stored in variable member:

However, this test is ineffective as written: the operator ! binds more -tighly than in, so it is applied first. Applying ! to a +tightly than in, so it is applied first. Applying ! to a non-empty string yields false, so the in operator actually ends up checking whether obj contains a property called "false".

From 6a75ebbae26163efb0ca7b4d1311369f69f8c31a Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Mon, 1 Oct 2018 08:50:53 +0100 Subject: [PATCH 02/18] JavaScript: Update model of `DOMException`. cf. https://developer.mozilla.org/en-US/docs/Web/API/DOMException/DOMException (cherry picked from commit 8cc7f5c24259d7a96efc6d1a99f3d25f60f3e7be) --- javascript/externs/web/w3c_dom1.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/javascript/externs/web/w3c_dom1.js b/javascript/externs/web/w3c_dom1.js index 6f43c00a3efc..bf85793fec6b 100644 --- a/javascript/externs/web/w3c_dom1.js +++ b/javascript/externs/web/w3c_dom1.js @@ -25,9 +25,11 @@ /** * @constructor + * @param {string=} message + * @param {string=} message * @see http://www.w3.org/TR/1998/REC-DOM-Level-1-19981001/level-one-core.html#ID-17189187 */ -function DOMException() {} +function DOMException(message, name) {} /** * @type {number} From de108a843dbdce7700b78ea82aba2b186af2f0fc Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 27 Sep 2018 10:56:32 +0100 Subject: [PATCH 03/18] JavaScript: Patch CFG to improve support for non-top level import declarations. --- javascript/ql/src/semmle/javascript/CFG.qll | 46 ++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/CFG.qll b/javascript/ql/src/semmle/javascript/CFG.qll index faec8deca6d8..c70b0dd15116 100644 --- a/javascript/ql/src/semmle/javascript/CFG.qll +++ b/javascript/ql/src/semmle/javascript/CFG.qll @@ -281,7 +281,7 @@ import javascript */ class ControlFlowNode extends @cfg_node, Locatable { /** Gets a node succeeding this node in the CFG. */ - ControlFlowNode getASuccessor() { + cached ControlFlowNode getASuccessor() { successor(this, result) } @@ -457,3 +457,47 @@ class ConcreteControlFlowNode extends ControlFlowNode { not this instanceof SyntheticControlFlowNode } } + +/** + * A CFG node corresponding to a nested (that is, non-toplevel) import declaration. + * + * This is a non-standard language feature that is not currently supported very well + * by the extractor, in particular such imports do not appear in the control flow graph + * generated by the extractor. We patch them in by overriding `getASuccessor`; once an + * extractor fix becomes available, this class can be removed. + */ +private class NestedImportDeclaration extends ImportDeclaration { + NestedImportDeclaration() { + exists (ASTNode p | p = getParent() | + not p instanceof TopLevel + ) and + // if there are no specifiers, the default control flow graph is fine + exists(getASpecifier()) + } + + override ControlFlowNode getASuccessor() { + result = getSpecifier(0).getFirstControlFlowNode() + } +} + +/** + * A CFG node corresponding to an import specifier in a nested import declaration. + * + * As for `NestedImportDeclaration` above, this is a temporary workaround that will be + * removed once extractor support for this non-standard language feature becomes available. + */ +private class NestedImportSpecifier extends ImportSpecifier { + NestedImportDeclaration decl; + int i; + + NestedImportSpecifier() { + this = decl.getSpecifier(i) + } + + override ControlFlowNode getASuccessor() { + result = decl.getSpecifier(i+1).getFirstControlFlowNode() + or + not exists(decl.getSpecifier(i+1)) and + successor(decl, result) + } +} From e683b516119b3a2673a3706c3f830f185b1da606 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 27 Sep 2018 10:57:45 +0100 Subject: [PATCH 04/18] JavaScript: Generalise code that assumes imports only appear at the toplevel. (cherry picked from commit db32dc2bdf5b7609f932169a71d3f00d09c7f861) --- javascript/ql/src/semmle/javascript/ES2015Modules.qll | 6 +++--- .../dataflow/internal/InterModuleTypeInference.qll | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/ES2015Modules.qll b/javascript/ql/src/semmle/javascript/ES2015Modules.qll index f5cf676f3d50..4cfa9dcb04d8 100644 --- a/javascript/ql/src/semmle/javascript/ES2015Modules.qll +++ b/javascript/ql/src/semmle/javascript/ES2015Modules.qll @@ -27,7 +27,7 @@ class ES2015Module extends Module { /** Gets an export declaration in this module. */ ExportDeclaration getAnExport() { - result.getContainer() = this + result.getTopLevel() = this } override Module getAnImportedModule() { @@ -55,7 +55,7 @@ class ES2015Module extends Module { /** An import declaration. */ class ImportDeclaration extends Stmt, Import, @importdeclaration { override ES2015Module getEnclosingModule() { - result = getContainer() + result = getTopLevel() } override PathExprInModule getImportedPath() { @@ -254,7 +254,7 @@ class BulkReExportDeclaration extends ReExportDeclaration, @exportalldeclaration * but we ignore this subtlety. */ private predicate isShadowedFromBulkExport(BulkReExportDeclaration reExport, string name) { - exists (ExportNamedDeclaration other | other.getContainer() = reExport.getEnclosingModule() | + exists (ExportNamedDeclaration other | other.getTopLevel() = reExport.getEnclosingModule() | other.getAnExportedDecl().getName() = name or other.getASpecifier().getExportedName() = name) diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll index 6efa6f9b0cf9..77359efb216d 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll @@ -301,7 +301,7 @@ private class AnalyzedExportAssign extends AnalyzedPropertyWrite, DataFlow::Valu } override predicate writes(AbstractValue baseVal, string propName, DataFlow::AnalyzedNode source) { - baseVal = TAbstractModuleObject(exportAssign.getContainer()) and + baseVal = TAbstractModuleObject(exportAssign.getTopLevel()) and propName = "exports" and source = this } From 5e75a62f5c39ed98d7c931dc91ce46f657b11016 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 27 Sep 2018 10:58:26 +0100 Subject: [PATCH 05/18] JavaScript: Add test case for type inference in the presence of non-toplevel imports. (cherry picked from commit 8b7bb8ceccf229985608f6e5274171e5ee864d39) --- .../test/library-tests/Flow/AbstractValues.expected | 4 ++++ .../ql/test/library-tests/Flow/abseval.expected | 5 +++++ .../ql/test/library-tests/Flow/nestedImport.js | 12 ++++++++++++ javascript/ql/test/library-tests/Flow/types.expected | 3 +++ 4 files changed, 24 insertions(+) create mode 100644 javascript/ql/test/library-tests/Flow/nestedImport.js diff --git a/javascript/ql/test/library-tests/Flow/AbstractValues.expected b/javascript/ql/test/library-tests/Flow/AbstractValues.expected index 0f3244a73cbd..77911a902153 100644 --- a/javascript/ql/test/library-tests/Flow/AbstractValues.expected +++ b/javascript/ql/test/library-tests/Flow/AbstractValues.expected @@ -180,6 +180,10 @@ | n.js:3:16:3:23 | object literal | | namespace-reexport.js:1:1:4:0 | exports object of module namespace-reexport | | namespace-reexport.js:1:1:4:0 | module object of module namespace-reexport | +| nestedImport.js:1:1:13:0 | exports object of module nestedImport | +| nestedImport.js:1:1:13:0 | module object of module nestedImport | +| nestedImport.js:9:1:12:1 | function tst | +| nestedImport.js:9:1:12:1 | instance of function tst | | nodeJsClient.js:1:1:6:0 | exports object of module nodeJsClient | | nodeJsClient.js:1:1:6:0 | module object of module nodeJsClient | | nodeJsLib.js:1:1:4:0 | exports object of module nodeJsLib | diff --git a/javascript/ql/test/library-tests/Flow/abseval.expected b/javascript/ql/test/library-tests/Flow/abseval.expected index dbc1a06ebfe4..86e8334adc79 100644 --- a/javascript/ql/test/library-tests/Flow/abseval.expected +++ b/javascript/ql/test/library-tests/Flow/abseval.expected @@ -54,6 +54,7 @@ | b.js:42:5:42:7 | z11 | b.js:42:11:42:18 | toString | file://:0:0:0:0 | indefinite value (import) | | b.js:45:5:45:7 | z12 | b.js:45:11:45:12 | f2 | ts2.ts:1:1:6:0 | exports object of module ts2 | | b.js:45:5:45:7 | z12 | b.js:45:11:45:12 | f2 | ts2.ts:1:10:1:22 | anonymous function | +| b.js:45:5:45:7 | z12 | b.js:45:11:45:12 | f2 | ts2.ts:4:12:4:13 | object literal | | b.js:48:5:48:7 | z13 | b.js:48:11:48:11 | w | file://:0:0:0:0 | non-empty, non-numeric string | | b.js:51:5:51:7 | z14 | b.js:51:11:51:24 | foo_reexported | file://:0:0:0:0 | indefinite value (import) | | b.js:54:5:54:7 | z15 | b.js:54:11:54:19 | something | file://:0:0:0:0 | indefinite value (import) | @@ -171,6 +172,10 @@ | mixed.js:9:5:9:6 | fn | mixed.js:9:10:9:19 | __filename | file://:0:0:0:0 | numeric string | | mixed.js:10:5:10:6 | dn | mixed.js:10:10:10:18 | __dirname | file://:0:0:0:0 | non-empty, non-numeric string | | mixed.js:10:5:10:6 | dn | mixed.js:10:10:10:18 | __dirname | file://:0:0:0:0 | numeric string | +| nestedImport.js:2:5:2:6 | x1 | nestedImport.js:2:10:2:12 | foo | esLib.js:3:8:3:24 | function foo | +| nestedImport.js:6:7:6:8 | x2 | nestedImport.js:6:12:6:14 | foo | file://:0:0:0:0 | indefinite value (import) | +| nestedImport.js:6:7:6:8 | x2 | nestedImport.js:6:12:6:14 | foo | nodeJsLib.js:3:15:3:37 | function nodeJsFoo | +| nestedImport.js:11:7:11:8 | x3 | nestedImport.js:11:12:11:14 | foo | esLib.js:3:8:3:24 | function foo | | nodeJsClient.js:1:5:1:6 | nj | nodeJsClient.js:1:10:1:31 | require ... JsLib') | file://:0:0:0:0 | indefinite value (call) | | nodeJsClient.js:1:5:1:6 | nj | nodeJsClient.js:1:10:1:31 | require ... JsLib') | nodeJsLib.js:1:1:4:0 | exports object of module nodeJsLib | | nodeJsClient.js:1:5:1:6 | nj | nodeJsClient.js:1:10:1:31 | require ... JsLib') | nodeJsLib.js:1:18:1:43 | function nodeJsModule | diff --git a/javascript/ql/test/library-tests/Flow/nestedImport.js b/javascript/ql/test/library-tests/Flow/nestedImport.js new file mode 100644 index 000000000000..c52953db2cfb --- /dev/null +++ b/javascript/ql/test/library-tests/Flow/nestedImport.js @@ -0,0 +1,12 @@ +import { foo } from './esLib'; +let x1 = foo; + +if (!foo) { + import { foo } from './nodeJsLib'; + let x2 = foo; +} + +function tst() { + import { foo } from './esLib'; + let x3 = foo; +} diff --git a/javascript/ql/test/library-tests/Flow/types.expected b/javascript/ql/test/library-tests/Flow/types.expected index 786fef8fcb96..f0aef176a107 100644 --- a/javascript/ql/test/library-tests/Flow/types.expected +++ b/javascript/ql/test/library-tests/Flow/types.expected @@ -98,6 +98,9 @@ | mixed.js:8:5:8:7 | exp | mixed.js:8:11:8:17 | exports | object | | mixed.js:9:5:9:6 | fn | mixed.js:9:10:9:19 | __filename | string | | mixed.js:10:5:10:6 | dn | mixed.js:10:10:10:18 | __dirname | string | +| nestedImport.js:2:5:2:6 | x1 | nestedImport.js:2:10:2:12 | foo | function | +| nestedImport.js:6:7:6:8 | x2 | nestedImport.js:6:12:6:14 | foo | boolean, class, date, function, null, number, object, regular expression,string or undefined | +| nestedImport.js:11:7:11:8 | x3 | nestedImport.js:11:12:11:14 | foo | function | | nodeJsClient.js:1:5:1:6 | nj | nodeJsClient.js:1:10:1:31 | require ... JsLib') | boolean, class, date, function, null, number, object, regular expression,string or undefined | | nodeJsClient.js:2:5:2:6 | es | nodeJsClient.js:2:10:2:27 | require('./esLib') | boolean, class, date, function, null, number, object, regular expression,string or undefined | | nodeJsClient.js:4:5:4:6 | x1 | nodeJsClient.js:4:10:4:15 | nj.foo | boolean, class, date, function, null, number, object, regular expression,string or undefined | From b2824447405fdccd8bede80dfd97b4f27462a276 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Mon, 8 Oct 2018 16:20:40 +0200 Subject: [PATCH 06/18] Revert "JavaScript: Patch CFG to improve support for non-top level import declarations." This reverts commit f05e777e640d6c890609b6863472a371b86a6df1. --- javascript/ql/src/semmle/javascript/CFG.qll | 46 +-------------------- 1 file changed, 1 insertion(+), 45 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/CFG.qll b/javascript/ql/src/semmle/javascript/CFG.qll index c70b0dd15116..faec8deca6d8 100644 --- a/javascript/ql/src/semmle/javascript/CFG.qll +++ b/javascript/ql/src/semmle/javascript/CFG.qll @@ -281,7 +281,7 @@ import javascript */ class ControlFlowNode extends @cfg_node, Locatable { /** Gets a node succeeding this node in the CFG. */ - cached ControlFlowNode getASuccessor() { + ControlFlowNode getASuccessor() { successor(this, result) } @@ -457,47 +457,3 @@ class ConcreteControlFlowNode extends ControlFlowNode { not this instanceof SyntheticControlFlowNode } } - -/** - * A CFG node corresponding to a nested (that is, non-toplevel) import declaration. - * - * This is a non-standard language feature that is not currently supported very well - * by the extractor, in particular such imports do not appear in the control flow graph - * generated by the extractor. We patch them in by overriding `getASuccessor`; once an - * extractor fix becomes available, this class can be removed. - */ -private class NestedImportDeclaration extends ImportDeclaration { - NestedImportDeclaration() { - exists (ASTNode p | p = getParent() | - not p instanceof TopLevel - ) and - // if there are no specifiers, the default control flow graph is fine - exists(getASpecifier()) - } - - override ControlFlowNode getASuccessor() { - result = getSpecifier(0).getFirstControlFlowNode() - } -} - -/** - * A CFG node corresponding to an import specifier in a nested import declaration. - * - * As for `NestedImportDeclaration` above, this is a temporary workaround that will be - * removed once extractor support for this non-standard language feature becomes available. - */ -private class NestedImportSpecifier extends ImportSpecifier { - NestedImportDeclaration decl; - int i; - - NestedImportSpecifier() { - this = decl.getSpecifier(i) - } - - override ControlFlowNode getASuccessor() { - result = decl.getSpecifier(i+1).getFirstControlFlowNode() - or - not exists(decl.getSpecifier(i+1)) and - successor(decl, result) - } -} From 2b7d69aaf44eee1c10bbf33f28f4ce8731d8c98a Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 10 Oct 2018 15:04:16 +0100 Subject: [PATCH 07/18] JavaScript: Add support for Google Cloud Spanner. (cherry picked from commit cd284b2f97f392fef4898437245c617946323a54) --- .../src/semmle/javascript/frameworks/SQL.qll | 102 ++++++++++++++++++ .../frameworks/SQL/SqlString.expected | 20 ++++ .../library-tests/frameworks/SQL/spanner.js | 20 ++++ .../library-tests/frameworks/SQL/spanner2.js | 7 ++ 4 files changed, 149 insertions(+) create mode 100644 javascript/ql/test/library-tests/frameworks/SQL/spanner.js create mode 100644 javascript/ql/test/library-tests/frameworks/SQL/spanner2.js diff --git a/javascript/ql/src/semmle/javascript/frameworks/SQL.qll b/javascript/ql/src/semmle/javascript/frameworks/SQL.qll index d31df8973f10..305f8f8adcdf 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/SQL.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/SQL.qll @@ -387,3 +387,105 @@ private module Sequelize { } } } + +/** + * Provides classes modelling the Google Cloud Spanner library. + */ +private module Spanner { + /** + * Gets a node that refers to the `Spanner` class + */ + DataFlow::SourceNode spanner() { + // older versions + result = DataFlow::moduleImport("@google-cloud/spanner") + or + // newer versions + result = DataFlow::moduleMember("@google-cloud/spanner", "Spanner") + } + + /** + * Gets a node that refers to an instance of the `Database` class. + */ + DataFlow::SourceNode database() { + result = spanner().getAnInvocation().getAMethodCall("instance").getAMethodCall("database") + } + + /** + * Gets a node that refers to an instance of the `v1.SpannerClient` class. + */ + DataFlow::SourceNode v1SpannerClient() { + result = spanner().getAPropertyRead("v1").getAPropertyRead("SpannerClient").getAnInstantiation() + } + + /** + * Gets a node that refers to a transaction object. + */ + DataFlow::SourceNode transaction() { + result = database().getAMethodCall("runTransaction").getCallback(0).getParameter(1) + } + + /** + * A call to a Spanner method that executes a SQL query. + */ + abstract class SqlExecution extends DatabaseAccess, DataFlow::InvokeNode { + /** + * Gets the position of the query argument; default is zero, which can be overridden + * by subclasses. + */ + int getQueryArgumentPosition() { + result = 0 + } + + override DataFlow::Node getAQueryArgument() { + result = getArgument(getQueryArgumentPosition()) or + result = getOptionArgument(getQueryArgumentPosition(), "sql") + } + } + + /** + * A call to `Database.run` or `Database.runStream`. + */ + class DatabaseRunCall extends SqlExecution { + DatabaseRunCall() { + exists (string run | run = "run" or run = "runPartitionedUpdate" or run = "runStream" | + this = database().getAMethodCall(run) + ) + } + } + + /** + * A call to `Transaction.run` or `Database.runStream`. + */ + class TransactionRunCall extends SqlExecution { + TransactionRunCall() { + exists (string run | run = "run" or run = "runStream" or run = "runUpdate" | + this = transaction().getAMethodCall(run) + ) + } + } + + /** + * A call to `v1.SpannerClient.executeSql` or `v1.SpannerClient.executeStreamingSql`. + */ + class ExecuteSqlCall extends SqlExecution { + ExecuteSqlCall() { + exists (string exec | exec = "executeSql" or exec = "executeStreamingSql" | + this = v1SpannerClient().getAMethodCall(exec) + ) + } + + override DataFlow::Node getAQueryArgument() { + // `executeSql` and `executeStreamingSql` do not accept query strings directly + result = getOptionArgument(0, "sql") + } + } + + /** + * An expression that is interpreted as a SQL string. + */ + class QueryString extends SQL::SqlString { + QueryString() { + this = any(SqlExecution se).getAQueryArgument().asExpr() + } + } +} diff --git a/javascript/ql/test/library-tests/frameworks/SQL/SqlString.expected b/javascript/ql/test/library-tests/frameworks/SQL/SqlString.expected index d4866f842e94..ace4b4fa5488 100644 --- a/javascript/ql/test/library-tests/frameworks/SQL/SqlString.expected +++ b/javascript/ql/test/library-tests/frameworks/SQL/SqlString.expected @@ -14,4 +14,24 @@ | postgres3.js:15:16:15:40 | 'SELECT ... s name' | | sequelize2.js:10:17:10:118 | 'SELECT ... Y name' | | sequelize.js:8:17:8:118 | 'SELECT ... Y name' | +| spanner2.js:5:26:5:35 | "SQL code" | +| spanner2.js:7:35:7:44 | "SQL code" | +| spanner.js:6:8:6:17 | "SQL code" | +| spanner.js:7:8:7:26 | { sql: "SQL code" } | +| spanner.js:7:15:7:24 | "SQL code" | +| spanner.js:8:25:8:34 | "SQL code" | +| spanner.js:9:25:9:43 | { sql: "SQL code" } | +| spanner.js:9:32:9:41 | "SQL code" | +| spanner.js:10:14:10:23 | "SQL code" | +| spanner.js:11:14:11:31 | { sql: "SQL code"} | +| spanner.js:11:21:11:30 | "SQL code" | +| spanner.js:14:10:14:19 | "SQL code" | +| spanner.js:15:10:15:28 | { sql: "SQL code" } | +| spanner.js:15:17:15:26 | "SQL code" | +| spanner.js:16:16:16:25 | "SQL code" | +| spanner.js:17:16:17:34 | { sql: "SQL code" } | +| spanner.js:17:23:17:32 | "SQL code" | +| spanner.js:18:16:18:25 | "SQL code" | +| spanner.js:19:16:19:34 | { sql: "SQL code" } | +| spanner.js:19:23:19:32 | "SQL code" | | sqlite.js:7:8:7:45 | "UPDATE ... id = ?" | diff --git a/javascript/ql/test/library-tests/frameworks/SQL/spanner.js b/javascript/ql/test/library-tests/frameworks/SQL/spanner.js new file mode 100644 index 000000000000..d42762230062 --- /dev/null +++ b/javascript/ql/test/library-tests/frameworks/SQL/spanner.js @@ -0,0 +1,20 @@ +const { Spanner } = require("@google-cloud/spanner"); +const spanner = new Spanner(); +const instance = spanner.instance('inst'); +const db = instance.database('db'); + +db.run("SQL code", (err, rows) => {}); +db.run({ sql: "SQL code" }, (err, rows) => {}); +db.runPartitionedUpdate("SQL code", (err, rows) => {}); +db.runPartitionedUpdate({ sql: "SQL code" }, (err, rows) => {}); +db.runStream("SQL code"); +db.runStream({ sql: "SQL code"}); + +db.runTransaction((err, tx) => { + tx.run("SQL code"); + tx.run({ sql: "SQL code" }); + tx.runStream("SQL code"); + tx.runStream({ sql: "SQL code" }); + tx.runUpdate("SQL code"); + tx.runUpdate({ sql: "SQL code" }); +}); \ No newline at end of file diff --git a/javascript/ql/test/library-tests/frameworks/SQL/spanner2.js b/javascript/ql/test/library-tests/frameworks/SQL/spanner2.js new file mode 100644 index 000000000000..9795d0f3a39e --- /dev/null +++ b/javascript/ql/test/library-tests/frameworks/SQL/spanner2.js @@ -0,0 +1,7 @@ +const spanner = require("@google-cloud/spanner"); +const client = new spanner.v1.SpannerClient({}); + +client.executeSql("not SQL code", (err, rows) => {}); +client.executeSql({ sql: "SQL code" }, (err, rows) => {}); +client.executeStreamingSql("not SQL code", (err, rows) => {}); +client.executeStreamingSql({ sql: "SQL code" }, (err, rows) => {}); From 898ba94837039674926218d68fe33397b195cb96 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Mon, 15 Oct 2018 20:14:40 +0100 Subject: [PATCH 08/18] JavaScript: Address review comments. (cherry picked from commit 6835815673bffd8e0bff57c29338a76b0be2a74c) --- javascript/ql/src/semmle/javascript/frameworks/SQL.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/SQL.qll b/javascript/ql/src/semmle/javascript/frameworks/SQL.qll index 305f8f8adcdf..f933b25b4776 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/SQL.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/SQL.qll @@ -443,7 +443,7 @@ private module Spanner { } /** - * A call to `Database.run` or `Database.runStream`. + * A call to `Database.run`, `Database.runPartitionedUpdate` or `Database.runStream`. */ class DatabaseRunCall extends SqlExecution { DatabaseRunCall() { @@ -454,7 +454,7 @@ private module Spanner { } /** - * A call to `Transaction.run` or `Database.runStream`. + * A call to `Transaction.run`, `Transaction.runStream` or `Transaction.runUpdate`. */ class TransactionRunCall extends SqlExecution { TransactionRunCall() { From 5167d43fbc2e169ce5835ccad88848e4758a6323 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Tue, 9 Oct 2018 12:55:38 +0100 Subject: [PATCH 09/18] JavaScript: Refactor `AnalyzedPropertyWrite::writes` to enable correct modelling of variable exports. (cherry picked from commit 080f974663798b42b31aeb4f7c5952198797ba20) --- .../javascript/dataflow/AbstractProperties.qll | 5 ++--- .../internal/InterModuleTypeInference.qll | 4 ++-- .../internal/PropertyTypeInference.qll | 18 +++++++++++++++++- .../library-tests/Flow/AbstractValues.expected | 10 ++++++---- javascript/ql/test/library-tests/Flow/a.js | 5 +++++ .../test/library-tests/Flow/abseval.expected | 3 +++ javascript/ql/test/library-tests/Flow/b.js | 3 +++ .../ql/test/library-tests/Flow/types.expected | 2 ++ 8 files changed, 40 insertions(+), 10 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/AbstractProperties.qll b/javascript/ql/src/semmle/javascript/dataflow/AbstractProperties.qll index ed362d313d20..9d5ee2065328 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/AbstractProperties.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/AbstractProperties.qll @@ -96,8 +96,7 @@ class AbstractProtoProperty extends AbstractProperty { * which in turn introduces a materialization. */ private AbstractValue getAnAssignedValue(AbstractValue b, string p) { - exists (AnalyzedPropertyWrite apw, DataFlow::AnalyzedNode afn | - apw.writes(b, p, afn) and - result = afn.getALocalValue() + exists (AnalyzedPropertyWrite apw | + apw.writesValue(b, p, result) ) } diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll index 77359efb216d..599ebb4ae7e3 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll @@ -263,10 +263,10 @@ private class AnalyzedVariableExport extends AnalyzedPropertyWrite, DataFlow::Va astNode = varDef.getTarget() } - override predicate writes(AbstractValue baseVal, string propName, DataFlow::AnalyzedNode source) { + override predicate writesValue(AbstractValue baseVal, string propName, AbstractValue val) { baseVal = TAbstractExportsObject(export.getEnclosingModule()) and propName = name and - source = varDef.getSource().analyze() + val = varDef.getAnAssignedValue() } } diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/PropertyTypeInference.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/PropertyTypeInference.qll index 8eafaa532224..c9160e4a4a71 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/PropertyTypeInference.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/PropertyTypeInference.qll @@ -96,8 +96,24 @@ abstract class AnalyzedPropertyWrite extends DataFlow::Node { /** * Holds if this property write assigns `source` to property `propName` of one of the * concrete objects represented by `baseVal`. + * + * Note that not all property writes have an explicit `source` node; use predicate + * `writesValue` below to cover these cases. */ - abstract predicate writes(AbstractValue baseVal, string propName, DataFlow::AnalyzedNode source); + predicate writes(AbstractValue baseVal, string propName, DataFlow::AnalyzedNode source) { + none() + } + + /** + * Holds if this property write assigns `val` to property `propName` of one of the + * concrete objects represented by `baseVal`. + */ + predicate writesValue(AbstractValue baseVal, string propName, AbstractValue val) { + exists (AnalyzedNode source | + writes(baseVal, propName, source) and + val = source.getALocalValue() + ) + } /** * Holds if the flow information for the base node of this property write is incomplete diff --git a/javascript/ql/test/library-tests/Flow/AbstractValues.expected b/javascript/ql/test/library-tests/Flow/AbstractValues.expected index 77911a902153..e7379efebeca 100644 --- a/javascript/ql/test/library-tests/Flow/AbstractValues.expected +++ b/javascript/ql/test/library-tests/Flow/AbstractValues.expected @@ -4,10 +4,12 @@ | ChatListScreen.js:3:1:5:1 | instance of function foo | | a2.js:1:1:2:0 | exports object of module a2 | | a2.js:1:1:2:0 | module object of module a2 | -| a.js:1:1:13:0 | exports object of module a | -| a.js:1:1:13:0 | module object of module a | +| a.js:1:1:18:0 | exports object of module a | +| a.js:1:1:18:0 | module object of module a | | a.js:3:8:5:1 | function setX | | a.js:3:8:5:1 | instance of function setX | +| a.js:15:1:17:1 | function bump | +| a.js:15:1:17:1 | instance of function bump | | amd2.js:1:1:4:0 | exports object of module amd2 | | amd2.js:1:1:4:0 | module object of module amd2 | | amd2.js:1:8:3:1 | anonymous function | @@ -36,8 +38,8 @@ | arguments.js:30:2:33:1 | anonymous function | | arguments.js:30:2:33:1 | arguments object of anonymous function | | arguments.js:30:2:33:1 | instance of anonymous function | -| b.js:1:1:55:0 | exports object of module b | -| b.js:1:1:55:0 | module object of module b | +| b.js:1:1:58:0 | exports object of module b | +| b.js:1:1:58:0 | module object of module b | | backend.js:1:1:3:0 | exports object of module backend | | backend.js:1:1:3:0 | module object of module backend | | backend.js:1:17:1:18 | object literal | diff --git a/javascript/ql/test/library-tests/Flow/a.js b/javascript/ql/test/library-tests/Flow/a.js index 5319d461a5ba..e733d3361276 100644 --- a/javascript/ql/test/library-tests/Flow/a.js +++ b/javascript/ql/test/library-tests/Flow/a.js @@ -10,3 +10,8 @@ let z = someGlobal; export let w; w = "w"; + +export let notAlwaysZero = 0; +function bump() { + ++notAlwaysZero; +} diff --git a/javascript/ql/test/library-tests/Flow/abseval.expected b/javascript/ql/test/library-tests/Flow/abseval.expected index 86e8334adc79..11deddd1ab92 100644 --- a/javascript/ql/test/library-tests/Flow/abseval.expected +++ b/javascript/ql/test/library-tests/Flow/abseval.expected @@ -6,6 +6,7 @@ | a.js:9:5:9:5 | z | a.js:9:9:9:18 | someGlobal | file://:0:0:0:0 | indefinite value (global) | | a.js:9:5:9:5 | z | a.js:9:9:9:18 | someGlobal | file://:0:0:0:0 | non-zero value | | a.js:9:5:9:5 | z | a.js:9:9:9:18 | someGlobal | file://:0:0:0:0 | true | +| a.js:14:12:14:24 | notAlwaysZero | a.js:14:28:14:28 | 0 | file://:0:0:0:0 | 0 | | amd.js:2:7:2:7 | m | amd.js:2:11:2:13 | mod | amd.js:1:1:7:0 | module object of module amd | | amd.js:2:7:2:7 | m | amd.js:2:11:2:13 | mod | file://:0:0:0:0 | indefinite value (call) | | amd.js:3:7:3:7 | e | amd.js:3:11:3:13 | exp | amd.js:1:1:7:0 | exports object of module amd | @@ -58,6 +59,8 @@ | b.js:48:5:48:7 | z13 | b.js:48:11:48:11 | w | file://:0:0:0:0 | non-empty, non-numeric string | | b.js:51:5:51:7 | z14 | b.js:51:11:51:24 | foo_reexported | file://:0:0:0:0 | indefinite value (import) | | b.js:54:5:54:7 | z15 | b.js:54:11:54:19 | something | file://:0:0:0:0 | indefinite value (import) | +| b.js:57:5:57:7 | z16 | b.js:57:11:57:23 | notAlwaysZero | file://:0:0:0:0 | 0 | +| b.js:57:5:57:7 | z16 | b.js:57:11:57:23 | notAlwaysZero | file://:0:0:0:0 | non-zero value | | backend.js:1:7:1:13 | Backend | backend.js:1:17:1:18 | {} | backend.js:1:17:1:18 | object literal | | classAccessors.js:10:9:10:11 | myX | classAccessors.js:10:15:10:20 | this.x | file://:0:0:0:0 | indefinite value (call) | | classAccessors.js:10:9:10:11 | myX | classAccessors.js:10:15:10:20 | this.x | file://:0:0:0:0 | indefinite value (heap) | diff --git a/javascript/ql/test/library-tests/Flow/b.js b/javascript/ql/test/library-tests/Flow/b.js index 6c8632f53483..f1d9c7c66067 100644 --- a/javascript/ql/test/library-tests/Flow/b.js +++ b/javascript/ql/test/library-tests/Flow/b.js @@ -52,3 +52,6 @@ let z14 = foo_reexported; import { something } from './reexport-unknown'; let z15 = something; + +import { notAlwaysZero } from './a'; +let z16 = notAlwaysZero; diff --git a/javascript/ql/test/library-tests/Flow/types.expected b/javascript/ql/test/library-tests/Flow/types.expected index f0aef176a107..bdbff344ac5f 100644 --- a/javascript/ql/test/library-tests/Flow/types.expected +++ b/javascript/ql/test/library-tests/Flow/types.expected @@ -2,6 +2,7 @@ | a.js:1:12:1:12 | x | a.js:1:16:1:16 | 0 | number | | a.js:1:19:1:19 | y | a.js:1:23:1:23 | 0 | number | | a.js:9:5:9:5 | z | a.js:9:9:9:18 | someGlobal | boolean, class, date, function, null, number, object, regular expression,string or undefined | +| a.js:14:12:14:24 | notAlwaysZero | a.js:14:28:14:28 | 0 | number | | amd.js:2:7:2:7 | m | amd.js:2:11:2:13 | mod | boolean, class, date, function, null, number, object, regular expression,string or undefined | | amd.js:3:7:3:7 | e | amd.js:3:11:3:13 | exp | boolean, class, date, function, null, number, object, regular expression,string or undefined | | arguments.js:2:7:2:7 | y | arguments.js:2:11:2:11 | x | number | @@ -32,6 +33,7 @@ | b.js:48:5:48:7 | z13 | b.js:48:11:48:11 | w | string | | b.js:51:5:51:7 | z14 | b.js:51:11:51:24 | foo_reexported | boolean, class, date, function, null, number, object, regular expression,string or undefined | | b.js:54:5:54:7 | z15 | b.js:54:11:54:19 | something | boolean, class, date, function, null, number, object, regular expression,string or undefined | +| b.js:57:5:57:7 | z16 | b.js:57:11:57:23 | notAlwaysZero | number | | backend.js:1:7:1:13 | Backend | backend.js:1:17:1:18 | {} | object | | classAccessors.js:10:9:10:11 | myX | classAccessors.js:10:15:10:20 | this.x | boolean, class, date, function, null, number, object, regular expression,string or undefined | | classAccessors.js:11:9:11:11 | myY | classAccessors.js:11:15:11:20 | this.y | boolean, class, date, function, null, number, object, regular expression,string or undefined | From b0425a298cf651585c9eb9dce8f70ed1220d4306 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 10 Oct 2018 11:41:52 +0100 Subject: [PATCH 10/18] JavaScript: Eliminate slow antijoin predicate. (cherry picked from commit 0cfd04dfa2cc521f0df56c61390ee3c2f23d562f) --- .../internal/InterProceduralTypeInference.qll | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/InterProceduralTypeInference.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/InterProceduralTypeInference.qll index e71561b4139c..49c60bc3eba8 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/InterProceduralTypeInference.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/InterProceduralTypeInference.qll @@ -160,18 +160,26 @@ private class IIFEWithAnalyzedReturnFlow extends CallWithAnalyzedReturnFlow { } +/** + * Gets the only access to `v`, which is the variable declared by `fn`. + * + * This predicate is not defined for global functions `fn`, or for + * local variables `v` that do not have exactly one access. + */ +private VarAccess getOnlyAccess(FunctionDeclStmt fn, LocalVariable v) { + v = fn.getVariable() and + result = v.getAnAccess() and + strictcount(v.getAnAccess()) = 1 +} + /** A function that only is used locally, making it amenable to type inference. */ class LocalFunction extends Function { DataFlow::Impl::ExplicitInvokeNode invk; LocalFunction() { - this instanceof FunctionDeclStmt and - exists (LocalVariable v, Expr callee | - callee = invk.getCalleeNode().asExpr() and - v = getVariable() and - v.getAnAccess() = callee and - forall(VarAccess o | o = v.getAnAccess() | o = callee) and + exists (LocalVariable v | + getOnlyAccess(this, v) = invk.getCalleeNode().asExpr() and not exists(v.getAnAssignedExpr()) and not exists(ExportDeclaration export | export.exportsAs(v, _)) ) and From 374fd597d7482899e7601deebdd4f5589325ae61 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Tue, 16 Oct 2018 07:29:41 +0100 Subject: [PATCH 11/18] JavaScript: Reinstate override. (cherry picked from commit df5a8651c361c1406d24c4e320b1408a6a98b809) --- .../dataflow/internal/InterModuleTypeInference.qll | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll index 599ebb4ae7e3..117e6be397be 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll @@ -263,6 +263,12 @@ private class AnalyzedVariableExport extends AnalyzedPropertyWrite, DataFlow::Va astNode = varDef.getTarget() } + override predicate writes(AbstractValue baseVal, string propName, DataFlow::AnalyzedNode source) { + baseVal = TAbstractExportsObject(export.getEnclosingModule()) and + propName = name and + source = varDef.getSource().analyze() + } + override predicate writesValue(AbstractValue baseVal, string propName, AbstractValue val) { baseVal = TAbstractExportsObject(export.getEnclosingModule()) and propName = name and From 4d7e76262995117abd977935eed9c48f0b63dc3a Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 2 Oct 2018 15:06:52 +0100 Subject: [PATCH 12/18] TS: test case for type expansion through type parameter bound (cherry picked from commit 8bc92bd53439982e004fa9b79838aebde2d0cbc9) --- .../TypeScript/ExpansiveTypes/ExpansiveTypes.expected | 1 + .../TypeScript/ExpansiveTypes/NonExpansiveTypes.expected | 1 + .../TypeScript/ExpansiveTypes/expansive_signature.ts | 4 ++++ 3 files changed, 6 insertions(+) diff --git a/javascript/ql/test/library-tests/TypeScript/ExpansiveTypes/ExpansiveTypes.expected b/javascript/ql/test/library-tests/TypeScript/ExpansiveTypes/ExpansiveTypes.expected index 152299ee5878..8ced7c1fb7d0 100644 --- a/javascript/ql/test/library-tests/TypeScript/ExpansiveTypes/ExpansiveTypes.expected +++ b/javascript/ql/test/library-tests/TypeScript/ExpansiveTypes/ExpansiveTypes.expected @@ -18,4 +18,5 @@ | ExpansiveMethod in expansive_signature.ts | has no properties | | ExpansiveParameter in expansive_signature.ts | has no properties | | ExpansiveSignature in expansive_signature.ts | has no properties | +| ExpansiveSignatureTypeBound in expansive_signature.ts | has no properties | | ExpansiveX in used_from_expansion.ts | has no properties | diff --git a/javascript/ql/test/library-tests/TypeScript/ExpansiveTypes/NonExpansiveTypes.expected b/javascript/ql/test/library-tests/TypeScript/ExpansiveTypes/NonExpansiveTypes.expected index edf58f7f0e15..a150036fa210 100644 --- a/javascript/ql/test/library-tests/TypeScript/ExpansiveTypes/NonExpansiveTypes.expected +++ b/javascript/ql/test/library-tests/TypeScript/ExpansiveTypes/NonExpansiveTypes.expected @@ -25,6 +25,7 @@ | ExpansiveMethod in expansive_signature.ts | has properties | | ExpansiveParameter in expansive_signature.ts | has properties | | ExpansiveSignature in expansive_signature.ts | has properties | +| ExpansiveSignatureTypeBound in expansive_signature.ts | has properties | | ExpansiveX in used_from_expansion.ts | has properties | | Function in global scope | has properties | | Intl.CollatorOptions in global scope | has properties | diff --git a/javascript/ql/test/library-tests/TypeScript/ExpansiveTypes/expansive_signature.ts b/javascript/ql/test/library-tests/TypeScript/ExpansiveTypes/expansive_signature.ts index d0ce77d194c1..6bdfd8e1f2aa 100644 --- a/javascript/ql/test/library-tests/TypeScript/ExpansiveTypes/expansive_signature.ts +++ b/javascript/ql/test/library-tests/TypeScript/ExpansiveTypes/expansive_signature.ts @@ -19,3 +19,7 @@ interface ExpansiveMethod { interface ExpansiveFunctionType { x: () => ExpansiveFunctionType; } + +interface ExpansiveSignatureTypeBound { + foo : { >(x: G): G }; +} From cbf06ae74d41b7cac3e8725de119f8b636b72fc1 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 1 Oct 2018 14:36:19 +0100 Subject: [PATCH 13/18] TypeScript: test case for tokenization of template literals (cherry picked from commit 9146cc26bdb85977021ab99d8527ca9c2ba75ca0) --- .../TypeScript/Tokens/PrettyPrintTokens.expected | 4 ++++ .../TypeScript/Tokens/PrettyPrintTokens.ql | 15 +++++++++++++++ .../TypeScript/Tokens/templateLiteralsJS.js | 4 ++++ .../TypeScript/Tokens/templateLiteralsTS.ts | 4 ++++ 4 files changed, 27 insertions(+) create mode 100644 javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected create mode 100644 javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.ql create mode 100644 javascript/ql/test/library-tests/TypeScript/Tokens/templateLiteralsJS.js create mode 100644 javascript/ql/test/library-tests/TypeScript/Tokens/templateLiteralsTS.ts diff --git a/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected b/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected new file mode 100644 index 000000000000..f82a9638acc8 --- /dev/null +++ b/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected @@ -0,0 +1,4 @@ +| templateLiteralsJS.js:2 | console . log ( ` template~without~placeholders ` ) ; | +| templateLiteralsJS.js:3 | console . log ( ` template~with~placeholder~ ${ x } . ` ) ; | +| templateLiteralsTS.ts:2 | console . log ( `template~without~placeholders` ) ; | +| templateLiteralsTS.ts:3 | console . log ( `template~with~placeholder~${ x }.` ) ; | diff --git a/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.ql b/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.ql new file mode 100644 index 000000000000..e3344a5075cb --- /dev/null +++ b/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.ql @@ -0,0 +1,15 @@ +import javascript + +Token getATokenAtLine(File file, int line) { + result.getFile() = file and + result.getLocation().getStartLine() = line +} + +bindingset[line] +string getTokenStringAtLine(File file, int line) { + result = concat(Token tok | tok = getATokenAtLine(file, line) | tok.toString().replaceAll(" ", "~") + " " order by tok.getLocation().getStartColumn()) +} + +from File file, int line +where exists(CallExpr call | call.getFile() = file and call.getLocation().getStartLine() = line) +select file.getBaseName() + ":" + line, getTokenStringAtLine(file, line) diff --git a/javascript/ql/test/library-tests/TypeScript/Tokens/templateLiteralsJS.js b/javascript/ql/test/library-tests/TypeScript/Tokens/templateLiteralsJS.js new file mode 100644 index 000000000000..689e596474a1 --- /dev/null +++ b/javascript/ql/test/library-tests/TypeScript/Tokens/templateLiteralsJS.js @@ -0,0 +1,4 @@ +function f(x) { + console.log(`template without placeholders`); + console.log(`template with placeholder ${x}.`); +} diff --git a/javascript/ql/test/library-tests/TypeScript/Tokens/templateLiteralsTS.ts b/javascript/ql/test/library-tests/TypeScript/Tokens/templateLiteralsTS.ts new file mode 100644 index 000000000000..689e596474a1 --- /dev/null +++ b/javascript/ql/test/library-tests/TypeScript/Tokens/templateLiteralsTS.ts @@ -0,0 +1,4 @@ +function f(x) { + console.log(`template without placeholders`); + console.log(`template with placeholder ${x}.`); +} From 2abe34b2f9fcfca825e91962408a7d08e0d71865 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 1 Oct 2018 15:19:43 +0100 Subject: [PATCH 14/18] TypeScript: test case for whitespace before a rescanned token (cherry picked from commit a199035a05e7fc5b2fe246706a2cbfdcee00ce03) --- .../TypeScript/Tokens/PrettyPrintTokens.expected | 5 +++++ .../ql/test/library-tests/TypeScript/Tokens/regexpTS.ts | 4 ++++ .../library-tests/TypeScript/Tokens/templateLiteralsTS.ts | 3 +++ 3 files changed, 12 insertions(+) create mode 100644 javascript/ql/test/library-tests/TypeScript/Tokens/regexpTS.ts diff --git a/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected b/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected index f82a9638acc8..a12760caf5ee 100644 --- a/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected +++ b/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected @@ -1,4 +1,9 @@ +| regexpTS.ts:2 | console . log ( /foo/g ) ; | +| regexpTS.ts:3 | console . log ( /foo/g ) ; | | templateLiteralsJS.js:2 | console . log ( ` template~without~placeholders ` ) ; | | templateLiteralsJS.js:3 | console . log ( ` template~with~placeholder~ ${ x } . ` ) ; | | templateLiteralsTS.ts:2 | console . log ( `template~without~placeholders` ) ; | | templateLiteralsTS.ts:3 | console . log ( `template~with~placeholder~${ x }.` ) ; | +| templateLiteralsTS.ts:4 | console . log ( `template~with~placeholder~${ x }.` ) ; | +| templateLiteralsTS.ts:5 | console . log ( `template~with~placeholder~${ x }.` ) ; | +| templateLiteralsTS.ts:6 | console . log ( `template~with~placeholder~${ x }.` ) ; | diff --git a/javascript/ql/test/library-tests/TypeScript/Tokens/regexpTS.ts b/javascript/ql/test/library-tests/TypeScript/Tokens/regexpTS.ts new file mode 100644 index 000000000000..ca269c3db89a --- /dev/null +++ b/javascript/ql/test/library-tests/TypeScript/Tokens/regexpTS.ts @@ -0,0 +1,4 @@ +function f() { + console.log(/foo/g); + console.log( /foo/g); +} diff --git a/javascript/ql/test/library-tests/TypeScript/Tokens/templateLiteralsTS.ts b/javascript/ql/test/library-tests/TypeScript/Tokens/templateLiteralsTS.ts index 689e596474a1..803685637b28 100644 --- a/javascript/ql/test/library-tests/TypeScript/Tokens/templateLiteralsTS.ts +++ b/javascript/ql/test/library-tests/TypeScript/Tokens/templateLiteralsTS.ts @@ -1,4 +1,7 @@ function f(x) { console.log(`template without placeholders`); console.log(`template with placeholder ${x}.`); + console.log(`template with placeholder ${x }.`); + console.log(`template with placeholder ${ x}.`); + console.log(`template with placeholder ${ x }.`); } From 39c788f4f1c231cef5a81fbc13e55a2c8031bebc Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 1 Oct 2018 15:22:02 +0100 Subject: [PATCH 15/18] TypeScript: test case for tokens starting with ">" (cherry picked from commit d3a1df644cea84b159a21fe7606c4ea9752f4e01) --- .../TypeScript/Tokens/PrettyPrintTokens.expected | 8 ++++++++ .../library-tests/TypeScript/Tokens/greaterThanTS.ts | 11 +++++++++++ 2 files changed, 19 insertions(+) create mode 100644 javascript/ql/test/library-tests/TypeScript/Tokens/greaterThanTS.ts diff --git a/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected b/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected index a12760caf5ee..f9de49e83b9c 100644 --- a/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected +++ b/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected @@ -1,3 +1,11 @@ +| greaterThanTS.ts:2 | console . log ( x >= 1 ) ; | +| greaterThanTS.ts:3 | console . log ( x >>= 1 ) ; | +| greaterThanTS.ts:4 | console . log ( x >>>= 1 ) ; | +| greaterThanTS.ts:5 | console . log ( x >> 1 ) ; | +| greaterThanTS.ts:6 | console . log ( x >>> 1 ) ; | +| greaterThanTS.ts:8 | console . log ( x >= 1 ) ; | +| greaterThanTS.ts:9 | console . log ( x >= 1 ) ; | +| greaterThanTS.ts:10 | console . log ( x >= 1 ) ; | | regexpTS.ts:2 | console . log ( /foo/g ) ; | | regexpTS.ts:3 | console . log ( /foo/g ) ; | | templateLiteralsJS.js:2 | console . log ( ` template~without~placeholders ` ) ; | diff --git a/javascript/ql/test/library-tests/TypeScript/Tokens/greaterThanTS.ts b/javascript/ql/test/library-tests/TypeScript/Tokens/greaterThanTS.ts new file mode 100644 index 000000000000..391971677f51 --- /dev/null +++ b/javascript/ql/test/library-tests/TypeScript/Tokens/greaterThanTS.ts @@ -0,0 +1,11 @@ +function f(x) { + console.log(x >= 1); + console.log(x >>= 1); + console.log(x >>>= 1); + console.log(x >> 1); + console.log(x >>> 1); + + console.log(x>=1); + console.log(x>= 1); + console.log(x >=1); +} From f9634040b067a1e735ce67a6ab7f4842ec2200f8 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 2 Oct 2018 10:42:33 +0100 Subject: [PATCH 16/18] TypeScript: add test case with mixed rescanned tokens (cherry picked from commit 057af7c8657c9484a759a59fd30d42c477f97b35) --- .../TypeScript/Tokens/PrettyPrintTokens.expected | 4 ++++ javascript/ql/test/library-tests/TypeScript/Tokens/mixed.ts | 6 ++++++ 2 files changed, 10 insertions(+) create mode 100644 javascript/ql/test/library-tests/TypeScript/Tokens/mixed.ts diff --git a/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected b/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected index f9de49e83b9c..81e03e435923 100644 --- a/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected +++ b/javascript/ql/test/library-tests/TypeScript/Tokens/PrettyPrintTokens.expected @@ -6,6 +6,10 @@ | greaterThanTS.ts:8 | console . log ( x >= 1 ) ; | | greaterThanTS.ts:9 | console . log ( x >= 1 ) ; | | greaterThanTS.ts:10 | console . log ( x >= 1 ) ; | +| mixed.ts:2 | console . log ( x >= 1 ) ; | +| mixed.ts:3 | console . log ( `${ /r/g >= 1 }` ) ; | +| mixed.ts:4 | console . log ( /r/g ) ; | +| mixed.ts:5 | console . log ( `${ 1 }${ 1 }` ) ; | | regexpTS.ts:2 | console . log ( /foo/g ) ; | | regexpTS.ts:3 | console . log ( /foo/g ) ; | | templateLiteralsJS.js:2 | console . log ( ` template~without~placeholders ` ) ; | diff --git a/javascript/ql/test/library-tests/TypeScript/Tokens/mixed.ts b/javascript/ql/test/library-tests/TypeScript/Tokens/mixed.ts new file mode 100644 index 000000000000..7f01ada79ad9 --- /dev/null +++ b/javascript/ql/test/library-tests/TypeScript/Tokens/mixed.ts @@ -0,0 +1,6 @@ +function f(x) { + console.log(x >= 1); + console.log(`${/r/g >= 1}`); + console.log( /r/g); + console.log( `${1}${1}`); +} From 2e49cd117a9ba1a2d08d7293e065bbc8e5a56ff5 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Fri, 12 Oct 2018 09:37:30 +0200 Subject: [PATCH 17/18] JS: avoid flagging early returns in js/user-controlled-bypass (cherry picked from commit ffbbb807f4dc3ea7b36f3d42fbca070e9c382673) --- .../src/Security/CWE-807/ConditionalBypass.ql | 28 +++++++++++++++-- .../CWE-807/ConditionalBypass.expected | 2 ++ .../test/query-tests/Security/CWE-807/tst.js | 31 +++++++++++++++++++ 3 files changed, 59 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-807/ConditionalBypass.ql b/javascript/ql/src/Security/CWE-807/ConditionalBypass.ql index ca7188ce5eb2..e939bb61f8a1 100644 --- a/javascript/ql/src/Security/CWE-807/ConditionalBypass.ql +++ b/javascript/ql/src/Security/CWE-807/ConditionalBypass.ql @@ -3,7 +3,7 @@ * @description Conditions that the user controls are not suited for making security-related decisions. * @kind problem * @problem.severity error - * @precision high + * @precision medium * @id js/user-controlled-bypass * @tags security * external/cwe/cwe-807 @@ -83,8 +83,32 @@ predicate isTaintedGuardForSensitiveAction(Sink sink, DataFlow::Node source, Sen ) } +/** + * Holds if `e` effectively guards access to `action` by returning or throwing early. + * + * Example: `if (e) return; action(x)`. + */ +predicate isEarlyAbortGuard(Sink e, SensitiveAction action) { + exists (IfStmt guard | + // `e` is in the condition of an if-statement ... + e.asExpr().getParentExpr*() = guard.getCondition() and + // ... where the then-branch always throws or returns + exists (Stmt abort | + abort instanceof ThrowStmt or + abort instanceof ReturnStmt | + abort.nestedIn(guard) and + abort.getBasicBlock().(ReachableBasicBlock).postDominates(guard.getThen().getBasicBlock() ) + ) and + // ... and the else-branch does not exist + not exists (guard.getElse()) | + // ... and `action` is outside the if-statement + not action.asExpr().getEnclosingStmt().nestedIn(guard) + ) +} + from DataFlow::Node source, DataFlow::Node sink, SensitiveAction action -where isTaintedGuardForSensitiveAction(sink, source, action) +where isTaintedGuardForSensitiveAction(sink, source, action) and + not isEarlyAbortGuard(sink, action) select sink, "This condition guards a sensitive $@, but $@ controls it.", action, "action", source, "a user-provided value" diff --git a/javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected b/javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected index 36c6f163cf5f..0ee7194b924b 100644 --- a/javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected +++ b/javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected @@ -10,3 +10,5 @@ | tst.js:76:9:76:10 | v1 | This condition guards a sensitive $@, but $@ controls it. | tst.js:78:9:78:22 | process.exit() | action | tst.js:75:14:75:24 | req.cookies | a user-provided value | | tst.js:76:9:76:10 | v1 | This condition guards a sensitive $@, but $@ controls it. | tst.js:78:9:78:22 | process.exit() | action | tst.js:75:39:75:58 | req.params.requestId | a user-provided value | | tst.js:90:9:90:41 | req.coo ... secret" | This condition guards a sensitive $@, but $@ controls it. | tst.js:92:9:92:22 | process.exit() | action | tst.js:90:9:90:19 | req.cookies | a user-provided value | +| tst.js:111:13:111:32 | req.query.vulnerable | This condition guards a sensitive $@, but $@ controls it. | tst.js:114:9:114:16 | verify() | action | tst.js:111:13:111:32 | req.query.vulnerable | a user-provided value | +| tst.js:118:13:118:32 | req.query.vulnerable | This condition guards a sensitive $@, but $@ controls it. | tst.js:121:13:121:20 | verify() | action | tst.js:118:13:118:32 | req.query.vulnerable | a user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-807/tst.js b/javascript/ql/test/query-tests/Security/CWE-807/tst.js index 84235fa0d3f1..aa00c47f1ebd 100644 --- a/javascript/ql/test/query-tests/Security/CWE-807/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-807/tst.js @@ -99,3 +99,34 @@ app.get('/user/:id', function(req, res) { console.log(commit.author().toString()); } }); + +app.get('/user/:id', function(req, res) { + if (!req.body || !username || !password || riskAssessnment == null) { // OK: early return below + res.status(400).send({ error: '...', id: '...' }); + return + } + customerLogin.customerLogin(username, password, riskAssessment, clientIpAddress); + + while (!verified) { + if (req.query.vulnerable) { // NOT OK + break; + } + verify(); + } + + while (!verified) { + if (req.query.vulnerable) { // NOT OK + break; + } else { + verify(); + } + } + + while (!verified) { + if (req.query.vulnerable) { // OK: early return + return; + } + verify(); + } + +}); From 25224cc4a056731c66a03aba86775ba1525b1745 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 19 Oct 2018 10:27:13 +0100 Subject: [PATCH 18/18] Revert "TypeScript: disable queries that rely on token information" This reverts commit 003b600e2478fb0aaf4db19b61c782d30cae5156. --- javascript/ql/src/LanguageFeatures/EmptyArrayInit.ql | 1 - .../ql/src/LanguageFeatures/SemicolonInsertion.ql | 3 +-- .../MisleadingIndentationAfterControlStmt.ql | 3 +-- .../SemicolonInsertion/template_literal.ts | 12 ------------ 4 files changed, 2 insertions(+), 17 deletions(-) delete mode 100644 javascript/ql/test/query-tests/LanguageFeatures/SemicolonInsertion/template_literal.ts diff --git a/javascript/ql/src/LanguageFeatures/EmptyArrayInit.ql b/javascript/ql/src/LanguageFeatures/EmptyArrayInit.ql index 70c89715bf85..48ae48112446 100644 --- a/javascript/ql/src/LanguageFeatures/EmptyArrayInit.ql +++ b/javascript/ql/src/LanguageFeatures/EmptyArrayInit.ql @@ -45,5 +45,4 @@ class OmittedArrayElement extends ArrayExpr { } from OmittedArrayElement ae -where not ae.getFile().getFileType().isTypeScript() // ignore quirks in TypeScript tokenizer select ae, "Avoid omitted array elements." \ No newline at end of file diff --git a/javascript/ql/src/LanguageFeatures/SemicolonInsertion.ql b/javascript/ql/src/LanguageFeatures/SemicolonInsertion.ql index 2eef68fe5f4f..fc52539be8db 100644 --- a/javascript/ql/src/LanguageFeatures/SemicolonInsertion.ql +++ b/javascript/ql/src/LanguageFeatures/SemicolonInsertion.ql @@ -36,8 +36,7 @@ where s.hasSemicolonInserted() and asi = strictcount(Stmt ss | asi(sc, ss, true)) and nstmt = strictcount(Stmt ss | asi(sc, ss, _)) and perc = ((1-asi/nstmt)*100).floor() and - perc >= 90 and - not s.getFile().getFileType().isTypeScript() // ignore some quirks in the TypeScript tokenizer + perc >= 90 select (LastLineOf)s, "Avoid automated semicolon insertion " + "(" + perc + "% of all statements in $@ have an explicit semicolon).", sc, "the enclosing " + sctype \ No newline at end of file diff --git a/javascript/ql/src/Statements/MisleadingIndentationAfterControlStmt.ql b/javascript/ql/src/Statements/MisleadingIndentationAfterControlStmt.ql index 84e11138f7bc..0629224ed503 100644 --- a/javascript/ql/src/Statements/MisleadingIndentationAfterControlStmt.ql +++ b/javascript/ql/src/Statements/MisleadingIndentationAfterControlStmt.ql @@ -39,7 +39,6 @@ where misleadingIndentationCandidate(ctrl, s1, s2) and f.hasIndentation(ctrlStartLine, indent, _) and f.hasIndentation(startLine1, indent, _) and f.hasIndentation(startLine2, indent, _) and - not s2 instanceof EmptyStmt and - not f.getFileType().isTypeScript() // ignore quirks in TypeScript tokenizer + not s2 instanceof EmptyStmt select (FirstLineOf)s2, "The indentation of this statement suggests that it is controlled by $@, while in fact it is not.", (FirstLineOf)ctrl, "this statement" \ No newline at end of file diff --git a/javascript/ql/test/query-tests/LanguageFeatures/SemicolonInsertion/template_literal.ts b/javascript/ql/test/query-tests/LanguageFeatures/SemicolonInsertion/template_literal.ts deleted file mode 100644 index 01ce984f8e30..000000000000 --- a/javascript/ql/test/query-tests/LanguageFeatures/SemicolonInsertion/template_literal.ts +++ /dev/null @@ -1,12 +0,0 @@ -function foo(arg) { - console.log(arg); - console.log(arg); - console.log(arg); - console.log(arg); - console.log(arg); - console.log(arg); - console.log(arg); - console.log(arg); - console.log(arg); - console.log(`Unknown option '${arg}'.`); -}