From b414192dccfa7960289af3e13fd3cf0d9490833a Mon Sep 17 00:00:00 2001 From: Asger F Date: Sat, 20 Aug 2022 07:57:51 +0200 Subject: [PATCH 1/9] Bugfix pruning in package-pruning API graph models --- .../javascript/frameworks/data/internal/ApiGraphModels.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll index 69563a3eab4..6de2126e20d 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll @@ -253,7 +253,7 @@ private predicate isRelevantPackage(string package) { sourceModel(package, _, _, _) or sinkModel(package, _, _, _) or summaryModel(package, _, _, _, _, _) or - typeModel(package, _, _, _, _) + typeModel(_, _, package, _, _) ) and ( Specific::isPackageUsed(package) From ef627b487291f257e65b01a0d89d54973ac0442b Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 22 Aug 2022 15:55:12 +0200 Subject: [PATCH 2/9] Add support for TypeVar[x] and typeVariable rows --- .../data/internal/ApiGraphModels.qll | 94 ++++++++++++++++++- .../frameworks/data/test.expected | 9 ++ .../library-tests/frameworks/data/test.js | 18 ++++ .../library-tests/frameworks/data/test.ql | 14 +++ 4 files changed, 132 insertions(+), 3 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll index 6de2126e20d..810aeb9d371 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll @@ -155,6 +155,22 @@ module ModelInput { */ abstract predicate row(string row); } + + /** + * A unit class for adding additional type variable model rows. + */ + class TypeVariableModelCsv extends Unit { + /** + * Holds if `row` specifies a path through a type variable. + * + * A row of form, + * ``` + * name;path + * ``` + * means `path` can be substituted for a token `TypeVar[name]`. + */ + abstract predicate row(string row); + } } private import ModelInput @@ -182,6 +198,8 @@ private predicate summaryModel(string row) { any(SummaryModelCsv s).row(inverseP private predicate typeModel(string row) { any(TypeModelCsv s).row(inversePad(row)) } +private predicate typeVariableModel(string row) { any(TypeVariableModelCsv s).row(inversePad(row)) } + /** Holds if a source model exists for the given parameters. */ predicate sourceModel(string package, string type, string path, string kind) { exists(string row | @@ -219,7 +237,7 @@ private predicate summaryModel( ) } -/** Holds if an type model exists for the given parameters. */ +/** Holds if a type model exists for the given parameters. */ private predicate typeModel( string package1, string type1, string package2, string type2, string path ) { @@ -233,6 +251,15 @@ private predicate typeModel( ) } +/** Holds if a type variable model exists for the given parameters. */ +private predicate typeVariableModel(string name, string path) { + exists(string row | + typeVariableModel(row) and + row.splitAt(";", 0) = name and + row.splitAt(";", 1) = path + ) +} + /** * Gets a package that should be seen as an alias for the given other `package`, * or the `package` itself. @@ -290,6 +317,8 @@ private class AccessPathRange extends AccessPath::Range { summaryModel(package, _, _, this, _, _) or summaryModel(package, _, _, _, this, _) ) + or + typeVariableModel(_, this) } } @@ -361,6 +390,60 @@ private API::Node getNodeFromPath(string package, string type, AccessPath path, // Similar to the other recursive case, but where the path may have stepped through one or more call-site filters result = getSuccessorFromInvoke(getInvocationFromPath(package, type, path, n - 1), path.getToken(n - 1)) + or + // Apply a subpath + result = + getNodeFromSubPath(getNodeFromPath(package, type, path, n - 1), getSubPathAt(path, n - 1)) +} + +/** + * Gets a subpath for the `TypeVar` token found at the `n`th token of `path`. + */ +pragma[nomagic] +private AccessPath getSubPathAt(AccessPath path, int n) { + exists(string typeVarName | + path.getToken(n).getAnArgument("TypeVar") = typeVarName and + typeVariableModel(typeVarName, result) + ) +} + +/** + * Gets a node that is found by evaluating the first `n` tokens of `subPath` starting at `base`. + */ +pragma[nomagic] +private API::Node getNodeFromSubPath(API::Node base, AccessPath subPath, int n) { + exists(AccessPath path, int k | + base = [getNodeFromPath(_, _, path, k), getNodeFromSubPath(_, path, k)] and + subPath = getSubPathAt(path, k) and + result = base and + n = 0 + ) + or + result = getSuccessorFromNode(getNodeFromSubPath(base, subPath, n - 1), subPath.getToken(n - 1)) + or + result = + getSuccessorFromInvoke(getInvocationFromSubPath(base, subPath, n - 1), subPath.getToken(n - 1)) + or + result = + getNodeFromSubPath(getNodeFromSubPath(base, subPath, n - 1), getSubPathAt(subPath, n - 1)) +} + +/** + * Gets a call site that is found by evaluating the first `n` tokens of `subPath` starting at `base`. + */ +private Specific::InvokeNode getInvocationFromSubPath(API::Node base, AccessPath subPath, int n) { + result = Specific::getAnInvocationOf(getNodeFromSubPath(base, subPath, n)) + or + result = getInvocationFromSubPath(base, subPath, n - 1) and + invocationMatchesCallSiteFilter(result, subPath.getToken(n - 1)) +} + +/** + * Gets a node that is found by evaluating `subPath` starting at `base`. + */ +pragma[nomagic] +private API::Node getNodeFromSubPath(API::Node base, AccessPath subPath) { + result = getNodeFromSubPath(base, subPath, subPath.getNumToken()) } /** Gets the node identified by the given `(package, type, path)` tuple. */ @@ -390,7 +473,7 @@ Specific::InvokeNode getInvocationFromPath(string package, string type, AccessPa */ bindingset[name] predicate isValidTokenNameInIdentifyingAccessPath(string name) { - name = ["Argument", "Parameter", "ReturnValue", "WithArity"] + name = ["Argument", "Parameter", "ReturnValue", "WithArity", "TypeVar"] or Specific::isExtraValidTokenNameInIdentifyingAccessPath(name) } @@ -418,6 +501,9 @@ predicate isValidTokenArgumentInIdentifyingAccessPath(string name, string argume name = "WithArity" and argument.regexpMatch("\\d+(\\.\\.(\\d+)?)?") or + name = "TypeVar" and + exists(argument) + or Specific::isExtraValidTokenArgumentInIdentifyingAccessPath(name, argument) } @@ -489,6 +575,8 @@ module ModelOutput { any(SummaryModelCsv csv).row(row) and kind = "summary" and expectedArity = 6 or any(TypeModelCsv csv).row(row) and kind = "type" and expectedArity = 5 + or + any(TypeVariableModelCsv csv).row(row) and kind = "type-variable" and expectedArity = 2 | actualArity = count(row.indexOf(";")) + 1 and actualArity != expectedArity and @@ -499,7 +587,7 @@ module ModelOutput { or // Check names and arguments of access path tokens exists(AccessPath path, AccessPathToken token | - isRelevantFullPath(_, _, path) and + (isRelevantFullPath(_, _, path) or typeVariableModel(_, path)) and token = path.getToken(_) | not isValidTokenNameInIdentifyingAccessPath(token.getName()) and diff --git a/javascript/ql/test/library-tests/frameworks/data/test.expected b/javascript/ql/test/library-tests/frameworks/data/test.expected index 31fa854e157..eba68431510 100644 --- a/javascript/ql/test/library-tests/frameworks/data/test.expected +++ b/javascript/ql/test/library-tests/frameworks/data/test.expected @@ -58,6 +58,10 @@ taintFlow | test.js:207:24:207:31 | source() | test.js:207:24:207:31 | source() | | test.js:208:24:208:31 | source() | test.js:208:24:208:31 | source() | | test.js:211:34:211:41 | source() | test.js:211:34:211:41 | source() | +| test.js:214:34:214:41 | source() | test.js:214:34:214:41 | source() | +| test.js:223:45:223:52 | source() | test.js:223:45:223:52 | source() | +| test.js:225:39:225:46 | source() | test.js:225:39:225:46 | source() | +| test.js:226:50:226:57 | source() | test.js:226:50:226:57 | source() | isSink | test.js:54:18:54:25 | source() | test-sink | | test.js:55:22:55:29 | source() | test-sink | @@ -119,6 +123,11 @@ isSink | test.js:207:24:207:31 | source() | test-sink | | test.js:208:24:208:31 | source() | test-sink | | test.js:211:34:211:41 | source() | test-sink | +| test.js:214:34:214:41 | source() | test-sink | +| test.js:222:52:222:52 | 0 | test-sink | +| test.js:223:45:223:52 | source() | test-sink | +| test.js:225:39:225:46 | source() | test-sink | +| test.js:226:50:226:57 | source() | test-sink | syntaxErrors | Member[foo | | Member[foo] .Member[bar] | diff --git a/javascript/ql/test/library-tests/frameworks/data/test.js b/javascript/ql/test/library-tests/frameworks/data/test.js index 222d876acbb..ba06cb75569 100644 --- a/javascript/ql/test/library-tests/frameworks/data/test.js +++ b/javascript/ql/test/library-tests/frameworks/data/test.js @@ -209,3 +209,21 @@ testlib.bar.memberSink(source()); // NOT OK testlib.memberSink(source()); // OK testlib.overloadedSink('safe', source()); // OK testlib.overloadedSink('danger', source()); // NOT OK + +function typeVars() { + testlib.typevar.a.b().c.mySink(source()); // NOT OK + + testlib.typevar.mySink(source()); // OK - does not match sub path + testlib.typevar.a.mySink(source()); // OK - does not match sub path + testlib.typevar.a.b.mySink(source()); // OK - does not match sub path + testlib.typevar.a.b.c.mySink(source()); // OK - does not match sub path + testlib.typevar.a.b(1).c.mySink(source()); // OK - does not match sub path + + testlib.typevar.a.b().c.a.b().c.mySink(source(), 0); // OK + testlib.typevar.a.b().c.a.b().c.mySink(0, source()); // NOT OK + + testlib.typevar.left.x.right.mySink(source()); // NOT OK + testlib.typevar.left.left.x.right.right.mySink(source()); // NOT OK + testlib.typevar.left.x.right.right.mySink(source()); // OK - mismatched left and right + testlib.typevar.left.left.x.right.mySink(source()); // OK - mismatched left and right +} diff --git a/javascript/ql/test/library-tests/frameworks/data/test.ql b/javascript/ql/test/library-tests/frameworks/data/test.ql index e41d962d985..608f82e6644 100644 --- a/javascript/ql/test/library-tests/frameworks/data/test.ql +++ b/javascript/ql/test/library-tests/frameworks/data/test.ql @@ -40,6 +40,20 @@ class Sinks extends ModelInput::SinkModelCsv { "testlib;;Member[ParamDecoratorSink].DecoratedParameter;test-sink", "testlib;;AnyMember.Member[memberSink].Argument[0];test-sink", "testlib;;Member[overloadedSink].WithStringArgument[0=danger].Argument[1];test-sink", + "testlib;;Member[typevar].TypeVar[ABC].Member[mySink].Argument[0];test-sink", + "testlib;;Member[typevar].TypeVar[ABC].TypeVar[ABC].Member[mySink].Argument[1];test-sink", + "testlib;;Member[typevar].TypeVar[LeftRight].Member[mySink].Argument[0];test-sink", + ] + } +} + +class TypeVars extends ModelInput::TypeVariableModelCsv { + override predicate row(string row) { + row = + [ + "ABC;Member[a].Member[b].WithArity[0].ReturnValue.Member[c]", // + "LeftRight;Member[left].TypeVar[LeftRight].Member[right]", // + "LeftRight;Member[x]", ] } } From 654c4eea0f2866b5ee1a5ccd62df27a4eb3ba230 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 29 Aug 2022 10:13:58 +0200 Subject: [PATCH 3/9] Add support for type steps --- .../data/internal/ApiGraphModels.qll | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll index 810aeb9d371..20621942b8a 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll @@ -394,6 +394,9 @@ private API::Node getNodeFromPath(string package, string type, AccessPath path, // Apply a subpath result = getNodeFromSubPath(getNodeFromPath(package, type, path, n - 1), getSubPathAt(path, n - 1)) + or + // Apply a receiver step + typeStep(getNodeFromPath(package, type, path, n), result) } /** @@ -419,6 +422,13 @@ private API::Node getNodeFromSubPath(API::Node base, AccessPath subPath, int n) n = 0 ) or + exists(string package, string type, AccessPath basePath | + typeStepModel(package, type, basePath, subPath) and + base = getNodeFromPath(package, type, basePath) and + result = base and + n = 0 + ) + or result = getSuccessorFromNode(getNodeFromSubPath(base, subPath, n - 1), subPath.getToken(n - 1)) or result = @@ -451,6 +461,20 @@ API::Node getNodeFromPath(string package, string type, AccessPath path) { result = getNodeFromPath(package, type, path, path.getNumToken()) } +pragma[nomagic] +private predicate typeStepModel(string package, string type, AccessPath basePath, AccessPath output) { + summaryModel(package, type, basePath, "", output, "type") +} + +pragma[nomagic] +private predicate typeStep(API::Node pred, API::Node succ) { + exists(string package, string type, AccessPath basePath, AccessPath output | + typeStepModel(package, type, basePath, output) and + pred = getNodeFromPath(package, type, basePath) and + succ = getNodeFromSubPath(pred, output) + ) +} + /** * Gets an invocation identified by the given `(package, type, path)` tuple. * From d5d1365104111a4f2bdf6e07a6d7a1b972913194 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 29 Aug 2022 11:13:04 +0200 Subject: [PATCH 4/9] Synchronize ApiGraphModels.qll --- .../data/internal/ApiGraphModels.qll | 120 +++++++++++++++++- .../data/internal/ApiGraphModels.qll | 120 +++++++++++++++++- 2 files changed, 232 insertions(+), 8 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll b/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll index 69563a3eab4..20621942b8a 100644 --- a/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll +++ b/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll @@ -155,6 +155,22 @@ module ModelInput { */ abstract predicate row(string row); } + + /** + * A unit class for adding additional type variable model rows. + */ + class TypeVariableModelCsv extends Unit { + /** + * Holds if `row` specifies a path through a type variable. + * + * A row of form, + * ``` + * name;path + * ``` + * means `path` can be substituted for a token `TypeVar[name]`. + */ + abstract predicate row(string row); + } } private import ModelInput @@ -182,6 +198,8 @@ private predicate summaryModel(string row) { any(SummaryModelCsv s).row(inverseP private predicate typeModel(string row) { any(TypeModelCsv s).row(inversePad(row)) } +private predicate typeVariableModel(string row) { any(TypeVariableModelCsv s).row(inversePad(row)) } + /** Holds if a source model exists for the given parameters. */ predicate sourceModel(string package, string type, string path, string kind) { exists(string row | @@ -219,7 +237,7 @@ private predicate summaryModel( ) } -/** Holds if an type model exists for the given parameters. */ +/** Holds if a type model exists for the given parameters. */ private predicate typeModel( string package1, string type1, string package2, string type2, string path ) { @@ -233,6 +251,15 @@ private predicate typeModel( ) } +/** Holds if a type variable model exists for the given parameters. */ +private predicate typeVariableModel(string name, string path) { + exists(string row | + typeVariableModel(row) and + row.splitAt(";", 0) = name and + row.splitAt(";", 1) = path + ) +} + /** * Gets a package that should be seen as an alias for the given other `package`, * or the `package` itself. @@ -253,7 +280,7 @@ private predicate isRelevantPackage(string package) { sourceModel(package, _, _, _) or sinkModel(package, _, _, _) or summaryModel(package, _, _, _, _, _) or - typeModel(package, _, _, _, _) + typeModel(_, _, package, _, _) ) and ( Specific::isPackageUsed(package) @@ -290,6 +317,8 @@ private class AccessPathRange extends AccessPath::Range { summaryModel(package, _, _, this, _, _) or summaryModel(package, _, _, _, this, _) ) + or + typeVariableModel(_, this) } } @@ -361,6 +390,70 @@ private API::Node getNodeFromPath(string package, string type, AccessPath path, // Similar to the other recursive case, but where the path may have stepped through one or more call-site filters result = getSuccessorFromInvoke(getInvocationFromPath(package, type, path, n - 1), path.getToken(n - 1)) + or + // Apply a subpath + result = + getNodeFromSubPath(getNodeFromPath(package, type, path, n - 1), getSubPathAt(path, n - 1)) + or + // Apply a receiver step + typeStep(getNodeFromPath(package, type, path, n), result) +} + +/** + * Gets a subpath for the `TypeVar` token found at the `n`th token of `path`. + */ +pragma[nomagic] +private AccessPath getSubPathAt(AccessPath path, int n) { + exists(string typeVarName | + path.getToken(n).getAnArgument("TypeVar") = typeVarName and + typeVariableModel(typeVarName, result) + ) +} + +/** + * Gets a node that is found by evaluating the first `n` tokens of `subPath` starting at `base`. + */ +pragma[nomagic] +private API::Node getNodeFromSubPath(API::Node base, AccessPath subPath, int n) { + exists(AccessPath path, int k | + base = [getNodeFromPath(_, _, path, k), getNodeFromSubPath(_, path, k)] and + subPath = getSubPathAt(path, k) and + result = base and + n = 0 + ) + or + exists(string package, string type, AccessPath basePath | + typeStepModel(package, type, basePath, subPath) and + base = getNodeFromPath(package, type, basePath) and + result = base and + n = 0 + ) + or + result = getSuccessorFromNode(getNodeFromSubPath(base, subPath, n - 1), subPath.getToken(n - 1)) + or + result = + getSuccessorFromInvoke(getInvocationFromSubPath(base, subPath, n - 1), subPath.getToken(n - 1)) + or + result = + getNodeFromSubPath(getNodeFromSubPath(base, subPath, n - 1), getSubPathAt(subPath, n - 1)) +} + +/** + * Gets a call site that is found by evaluating the first `n` tokens of `subPath` starting at `base`. + */ +private Specific::InvokeNode getInvocationFromSubPath(API::Node base, AccessPath subPath, int n) { + result = Specific::getAnInvocationOf(getNodeFromSubPath(base, subPath, n)) + or + result = getInvocationFromSubPath(base, subPath, n - 1) and + invocationMatchesCallSiteFilter(result, subPath.getToken(n - 1)) +} + +/** + * Gets a node that is found by evaluating `subPath` starting at `base`. + */ +pragma[nomagic] +private API::Node getNodeFromSubPath(API::Node base, AccessPath subPath) { + result = getNodeFromSubPath(base, subPath, subPath.getNumToken()) } /** Gets the node identified by the given `(package, type, path)` tuple. */ @@ -368,6 +461,20 @@ API::Node getNodeFromPath(string package, string type, AccessPath path) { result = getNodeFromPath(package, type, path, path.getNumToken()) } +pragma[nomagic] +private predicate typeStepModel(string package, string type, AccessPath basePath, AccessPath output) { + summaryModel(package, type, basePath, "", output, "type") +} + +pragma[nomagic] +private predicate typeStep(API::Node pred, API::Node succ) { + exists(string package, string type, AccessPath basePath, AccessPath output | + typeStepModel(package, type, basePath, output) and + pred = getNodeFromPath(package, type, basePath) and + succ = getNodeFromSubPath(pred, output) + ) +} + /** * Gets an invocation identified by the given `(package, type, path)` tuple. * @@ -390,7 +497,7 @@ Specific::InvokeNode getInvocationFromPath(string package, string type, AccessPa */ bindingset[name] predicate isValidTokenNameInIdentifyingAccessPath(string name) { - name = ["Argument", "Parameter", "ReturnValue", "WithArity"] + name = ["Argument", "Parameter", "ReturnValue", "WithArity", "TypeVar"] or Specific::isExtraValidTokenNameInIdentifyingAccessPath(name) } @@ -418,6 +525,9 @@ predicate isValidTokenArgumentInIdentifyingAccessPath(string name, string argume name = "WithArity" and argument.regexpMatch("\\d+(\\.\\.(\\d+)?)?") or + name = "TypeVar" and + exists(argument) + or Specific::isExtraValidTokenArgumentInIdentifyingAccessPath(name, argument) } @@ -489,6 +599,8 @@ module ModelOutput { any(SummaryModelCsv csv).row(row) and kind = "summary" and expectedArity = 6 or any(TypeModelCsv csv).row(row) and kind = "type" and expectedArity = 5 + or + any(TypeVariableModelCsv csv).row(row) and kind = "type-variable" and expectedArity = 2 | actualArity = count(row.indexOf(";")) + 1 and actualArity != expectedArity and @@ -499,7 +611,7 @@ module ModelOutput { or // Check names and arguments of access path tokens exists(AccessPath path, AccessPathToken token | - isRelevantFullPath(_, _, path) and + (isRelevantFullPath(_, _, path) or typeVariableModel(_, path)) and token = path.getToken(_) | not isValidTokenNameInIdentifyingAccessPath(token.getName()) and diff --git a/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll b/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll index 69563a3eab4..20621942b8a 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll @@ -155,6 +155,22 @@ module ModelInput { */ abstract predicate row(string row); } + + /** + * A unit class for adding additional type variable model rows. + */ + class TypeVariableModelCsv extends Unit { + /** + * Holds if `row` specifies a path through a type variable. + * + * A row of form, + * ``` + * name;path + * ``` + * means `path` can be substituted for a token `TypeVar[name]`. + */ + abstract predicate row(string row); + } } private import ModelInput @@ -182,6 +198,8 @@ private predicate summaryModel(string row) { any(SummaryModelCsv s).row(inverseP private predicate typeModel(string row) { any(TypeModelCsv s).row(inversePad(row)) } +private predicate typeVariableModel(string row) { any(TypeVariableModelCsv s).row(inversePad(row)) } + /** Holds if a source model exists for the given parameters. */ predicate sourceModel(string package, string type, string path, string kind) { exists(string row | @@ -219,7 +237,7 @@ private predicate summaryModel( ) } -/** Holds if an type model exists for the given parameters. */ +/** Holds if a type model exists for the given parameters. */ private predicate typeModel( string package1, string type1, string package2, string type2, string path ) { @@ -233,6 +251,15 @@ private predicate typeModel( ) } +/** Holds if a type variable model exists for the given parameters. */ +private predicate typeVariableModel(string name, string path) { + exists(string row | + typeVariableModel(row) and + row.splitAt(";", 0) = name and + row.splitAt(";", 1) = path + ) +} + /** * Gets a package that should be seen as an alias for the given other `package`, * or the `package` itself. @@ -253,7 +280,7 @@ private predicate isRelevantPackage(string package) { sourceModel(package, _, _, _) or sinkModel(package, _, _, _) or summaryModel(package, _, _, _, _, _) or - typeModel(package, _, _, _, _) + typeModel(_, _, package, _, _) ) and ( Specific::isPackageUsed(package) @@ -290,6 +317,8 @@ private class AccessPathRange extends AccessPath::Range { summaryModel(package, _, _, this, _, _) or summaryModel(package, _, _, _, this, _) ) + or + typeVariableModel(_, this) } } @@ -361,6 +390,70 @@ private API::Node getNodeFromPath(string package, string type, AccessPath path, // Similar to the other recursive case, but where the path may have stepped through one or more call-site filters result = getSuccessorFromInvoke(getInvocationFromPath(package, type, path, n - 1), path.getToken(n - 1)) + or + // Apply a subpath + result = + getNodeFromSubPath(getNodeFromPath(package, type, path, n - 1), getSubPathAt(path, n - 1)) + or + // Apply a receiver step + typeStep(getNodeFromPath(package, type, path, n), result) +} + +/** + * Gets a subpath for the `TypeVar` token found at the `n`th token of `path`. + */ +pragma[nomagic] +private AccessPath getSubPathAt(AccessPath path, int n) { + exists(string typeVarName | + path.getToken(n).getAnArgument("TypeVar") = typeVarName and + typeVariableModel(typeVarName, result) + ) +} + +/** + * Gets a node that is found by evaluating the first `n` tokens of `subPath` starting at `base`. + */ +pragma[nomagic] +private API::Node getNodeFromSubPath(API::Node base, AccessPath subPath, int n) { + exists(AccessPath path, int k | + base = [getNodeFromPath(_, _, path, k), getNodeFromSubPath(_, path, k)] and + subPath = getSubPathAt(path, k) and + result = base and + n = 0 + ) + or + exists(string package, string type, AccessPath basePath | + typeStepModel(package, type, basePath, subPath) and + base = getNodeFromPath(package, type, basePath) and + result = base and + n = 0 + ) + or + result = getSuccessorFromNode(getNodeFromSubPath(base, subPath, n - 1), subPath.getToken(n - 1)) + or + result = + getSuccessorFromInvoke(getInvocationFromSubPath(base, subPath, n - 1), subPath.getToken(n - 1)) + or + result = + getNodeFromSubPath(getNodeFromSubPath(base, subPath, n - 1), getSubPathAt(subPath, n - 1)) +} + +/** + * Gets a call site that is found by evaluating the first `n` tokens of `subPath` starting at `base`. + */ +private Specific::InvokeNode getInvocationFromSubPath(API::Node base, AccessPath subPath, int n) { + result = Specific::getAnInvocationOf(getNodeFromSubPath(base, subPath, n)) + or + result = getInvocationFromSubPath(base, subPath, n - 1) and + invocationMatchesCallSiteFilter(result, subPath.getToken(n - 1)) +} + +/** + * Gets a node that is found by evaluating `subPath` starting at `base`. + */ +pragma[nomagic] +private API::Node getNodeFromSubPath(API::Node base, AccessPath subPath) { + result = getNodeFromSubPath(base, subPath, subPath.getNumToken()) } /** Gets the node identified by the given `(package, type, path)` tuple. */ @@ -368,6 +461,20 @@ API::Node getNodeFromPath(string package, string type, AccessPath path) { result = getNodeFromPath(package, type, path, path.getNumToken()) } +pragma[nomagic] +private predicate typeStepModel(string package, string type, AccessPath basePath, AccessPath output) { + summaryModel(package, type, basePath, "", output, "type") +} + +pragma[nomagic] +private predicate typeStep(API::Node pred, API::Node succ) { + exists(string package, string type, AccessPath basePath, AccessPath output | + typeStepModel(package, type, basePath, output) and + pred = getNodeFromPath(package, type, basePath) and + succ = getNodeFromSubPath(pred, output) + ) +} + /** * Gets an invocation identified by the given `(package, type, path)` tuple. * @@ -390,7 +497,7 @@ Specific::InvokeNode getInvocationFromPath(string package, string type, AccessPa */ bindingset[name] predicate isValidTokenNameInIdentifyingAccessPath(string name) { - name = ["Argument", "Parameter", "ReturnValue", "WithArity"] + name = ["Argument", "Parameter", "ReturnValue", "WithArity", "TypeVar"] or Specific::isExtraValidTokenNameInIdentifyingAccessPath(name) } @@ -418,6 +525,9 @@ predicate isValidTokenArgumentInIdentifyingAccessPath(string name, string argume name = "WithArity" and argument.regexpMatch("\\d+(\\.\\.(\\d+)?)?") or + name = "TypeVar" and + exists(argument) + or Specific::isExtraValidTokenArgumentInIdentifyingAccessPath(name, argument) } @@ -489,6 +599,8 @@ module ModelOutput { any(SummaryModelCsv csv).row(row) and kind = "summary" and expectedArity = 6 or any(TypeModelCsv csv).row(row) and kind = "type" and expectedArity = 5 + or + any(TypeVariableModelCsv csv).row(row) and kind = "type-variable" and expectedArity = 2 | actualArity = count(row.indexOf(";")) + 1 and actualArity != expectedArity and @@ -499,7 +611,7 @@ module ModelOutput { or // Check names and arguments of access path tokens exists(AccessPath path, AccessPathToken token | - isRelevantFullPath(_, _, path) and + (isRelevantFullPath(_, _, path) or typeVariableModel(_, path)) and token = path.getToken(_) | not isValidTokenNameInIdentifyingAccessPath(token.getName()) and From 51d6f752abe9e8f49fe1f75a85a1059bacbe0a00 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 30 Aug 2022 13:55:26 +0200 Subject: [PATCH 5/9] JS: Add partially failing test --- .../test/library-tests/frameworks/data/test.expected | 6 ++++++ .../ql/test/library-tests/frameworks/data/test.js | 5 +++++ .../ql/test/library-tests/frameworks/data/test.ql | 11 +++++++++++ 3 files changed, 22 insertions(+) diff --git a/javascript/ql/test/library-tests/frameworks/data/test.expected b/javascript/ql/test/library-tests/frameworks/data/test.expected index eba68431510..6879ff47d74 100644 --- a/javascript/ql/test/library-tests/frameworks/data/test.expected +++ b/javascript/ql/test/library-tests/frameworks/data/test.expected @@ -1,4 +1,6 @@ consistencyIssue +| library-tests/frameworks/data/test.js:231 | expected an alert, but found none | NOT OK | | +| library-tests/frameworks/data/test.js:232 | expected an alert, but found none | NOT OK | | taintFlow | paramDecorator.ts:6:54:6:54 | x | paramDecorator.ts:7:10:7:10 | x | | test.js:5:30:5:37 | source() | test.js:5:8:5:38 | testlib ... urce()) | @@ -62,6 +64,8 @@ taintFlow | test.js:223:45:223:52 | source() | test.js:223:45:223:52 | source() | | test.js:225:39:225:46 | source() | test.js:225:39:225:46 | source() | | test.js:226:50:226:57 | source() | test.js:226:50:226:57 | source() | +| test.js:230:59:230:66 | source() | test.js:230:59:230:66 | source() | +| test.js:233:59:233:66 | source() | test.js:233:59:233:66 | source() | isSink | test.js:54:18:54:25 | source() | test-sink | | test.js:55:22:55:29 | source() | test-sink | @@ -128,6 +132,8 @@ isSink | test.js:223:45:223:52 | source() | test-sink | | test.js:225:39:225:46 | source() | test-sink | | test.js:226:50:226:57 | source() | test-sink | +| test.js:230:59:230:66 | source() | test-sink | +| test.js:233:59:233:66 | source() | test-sink | syntaxErrors | Member[foo | | Member[foo] .Member[bar] | diff --git a/javascript/ql/test/library-tests/frameworks/data/test.js b/javascript/ql/test/library-tests/frameworks/data/test.js index ba06cb75569..d34940bd065 100644 --- a/javascript/ql/test/library-tests/frameworks/data/test.js +++ b/javascript/ql/test/library-tests/frameworks/data/test.js @@ -226,4 +226,9 @@ function typeVars() { testlib.typevar.left.left.x.right.right.mySink(source()); // NOT OK testlib.typevar.left.x.right.right.mySink(source()); // OK - mismatched left and right testlib.typevar.left.left.x.right.mySink(source()); // OK - mismatched left and right + + testlib.typevar.getThis().getThis().left.x.right.mySink(source()); // NOT OK + testlib.typevar.left.getThis().getThis().x.right.mySink(source()); // NOT OK + testlib.typevar.left.x.getThis().getThis().right.mySink(source()); // NOT OK + testlib.typevar.left.x.right.getThis().getThis().mySink(source()); // NOT OK } diff --git a/javascript/ql/test/library-tests/frameworks/data/test.ql b/javascript/ql/test/library-tests/frameworks/data/test.ql index 608f82e6644..053e41e22ff 100644 --- a/javascript/ql/test/library-tests/frameworks/data/test.ql +++ b/javascript/ql/test/library-tests/frameworks/data/test.ql @@ -14,6 +14,17 @@ class Steps extends ModelInput::SummaryModelCsv { "testlib;;Member[preserveAllButFirstArgument];Argument[1..];ReturnValue;taint", "testlib;;Member[preserveAllIfCall].Call;Argument[0..];ReturnValue;taint", "testlib;;Member[getSource].ReturnValue.Member[continue];Argument[this];ReturnValue;taint", + "testlib;~HasThisFlow;;;Member[getThis].ReturnValue;type", + ] + } +} + +class TypeDefs extends ModelInput::TypeModelCsv { + override predicate row(string row) { + row = + [ + "testlib;~HasThisFlow;testlib;;Member[typevar]", + "testlib;~HasThisFlow;testlib;~HasThisFlow;Member[left,right,x]", ] } } From f8b1aa7e7e83ada9f868ed1e763e8e9c09ac6f9b Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 30 Aug 2022 13:55:58 +0200 Subject: [PATCH 6/9] Also apply type step in getNodeFromSubPath --- .../javascript/frameworks/data/internal/ApiGraphModels.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll index 20621942b8a..093d99dd402 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll @@ -436,6 +436,8 @@ private API::Node getNodeFromSubPath(API::Node base, AccessPath subPath, int n) or result = getNodeFromSubPath(getNodeFromSubPath(base, subPath, n - 1), getSubPathAt(subPath, n - 1)) + or + typeStep(getNodeFromSubPath(base, subPath, n), result) } /** From b107dd6855dbdaffaf4e034146480735ed757cb9 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 30 Aug 2022 13:56:15 +0200 Subject: [PATCH 7/9] Fix typo: receiver step -> type step --- .../javascript/frameworks/data/internal/ApiGraphModels.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll index 093d99dd402..7c65e0e0b84 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll @@ -395,7 +395,7 @@ private API::Node getNodeFromPath(string package, string type, AccessPath path, result = getNodeFromSubPath(getNodeFromPath(package, type, path, n - 1), getSubPathAt(path, n - 1)) or - // Apply a receiver step + // Apply a type step typeStep(getNodeFromPath(package, type, path, n), result) } From dd44187aedbfe950de845515234e9c0c6abb54bc Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 30 Aug 2022 13:56:30 +0200 Subject: [PATCH 8/9] Sync files again --- .../semmle/python/frameworks/data/internal/ApiGraphModels.qll | 4 +++- .../codeql/ruby/frameworks/data/internal/ApiGraphModels.qll | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll b/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll index 20621942b8a..7c65e0e0b84 100644 --- a/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll +++ b/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll @@ -395,7 +395,7 @@ private API::Node getNodeFromPath(string package, string type, AccessPath path, result = getNodeFromSubPath(getNodeFromPath(package, type, path, n - 1), getSubPathAt(path, n - 1)) or - // Apply a receiver step + // Apply a type step typeStep(getNodeFromPath(package, type, path, n), result) } @@ -436,6 +436,8 @@ private API::Node getNodeFromSubPath(API::Node base, AccessPath subPath, int n) or result = getNodeFromSubPath(getNodeFromSubPath(base, subPath, n - 1), getSubPathAt(subPath, n - 1)) + or + typeStep(getNodeFromSubPath(base, subPath, n), result) } /** diff --git a/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll b/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll index 20621942b8a..7c65e0e0b84 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll @@ -395,7 +395,7 @@ private API::Node getNodeFromPath(string package, string type, AccessPath path, result = getNodeFromSubPath(getNodeFromPath(package, type, path, n - 1), getSubPathAt(path, n - 1)) or - // Apply a receiver step + // Apply a type step typeStep(getNodeFromPath(package, type, path, n), result) } @@ -436,6 +436,8 @@ private API::Node getNodeFromSubPath(API::Node base, AccessPath subPath, int n) or result = getNodeFromSubPath(getNodeFromSubPath(base, subPath, n - 1), getSubPathAt(subPath, n - 1)) + or + typeStep(getNodeFromSubPath(base, subPath, n), result) } /** From 0d88d20b56adaa3d9bae848cbac404f1de73a523 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 30 Aug 2022 16:44:01 +0200 Subject: [PATCH 9/9] JS: Actually update test output this time --- .../ql/test/library-tests/frameworks/data/test.expected | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/javascript/ql/test/library-tests/frameworks/data/test.expected b/javascript/ql/test/library-tests/frameworks/data/test.expected index 6879ff47d74..138547cc7ff 100644 --- a/javascript/ql/test/library-tests/frameworks/data/test.expected +++ b/javascript/ql/test/library-tests/frameworks/data/test.expected @@ -1,6 +1,4 @@ consistencyIssue -| library-tests/frameworks/data/test.js:231 | expected an alert, but found none | NOT OK | | -| library-tests/frameworks/data/test.js:232 | expected an alert, but found none | NOT OK | | taintFlow | paramDecorator.ts:6:54:6:54 | x | paramDecorator.ts:7:10:7:10 | x | | test.js:5:30:5:37 | source() | test.js:5:8:5:38 | testlib ... urce()) | @@ -65,6 +63,8 @@ taintFlow | test.js:225:39:225:46 | source() | test.js:225:39:225:46 | source() | | test.js:226:50:226:57 | source() | test.js:226:50:226:57 | source() | | test.js:230:59:230:66 | source() | test.js:230:59:230:66 | source() | +| test.js:231:59:231:66 | source() | test.js:231:59:231:66 | source() | +| test.js:232:59:232:66 | source() | test.js:232:59:232:66 | source() | | test.js:233:59:233:66 | source() | test.js:233:59:233:66 | source() | isSink | test.js:54:18:54:25 | source() | test-sink | @@ -133,6 +133,8 @@ isSink | test.js:225:39:225:46 | source() | test-sink | | test.js:226:50:226:57 | source() | test-sink | | test.js:230:59:230:66 | source() | test-sink | +| test.js:231:59:231:66 | source() | test-sink | +| test.js:232:59:232:66 | source() | test-sink | | test.js:233:59:233:66 | source() | test-sink | syntaxErrors | Member[foo |