From cc95c77cbc230457e827d207accc9c2cdfd5b709 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Mar 2025 11:44:10 +0100 Subject: [PATCH 1/8] JS: Add failing test --- .../ql/test/library-tests/frameworks/data/test.ext.yml | 1 + javascript/ql/test/library-tests/frameworks/data/test.js | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/javascript/ql/test/library-tests/frameworks/data/test.ext.yml b/javascript/ql/test/library-tests/frameworks/data/test.ext.yml index eed5c054fbf..b8e12739746 100644 --- a/javascript/ql/test/library-tests/frameworks/data/test.ext.yml +++ b/javascript/ql/test/library-tests/frameworks/data/test.ext.yml @@ -10,6 +10,7 @@ extensions: - ['testlib', 'Member[MethodDecorator].DecoratedMember.Parameter[0]', 'test-source'] - ['testlib', 'Member[ParamDecoratorSource].DecoratedParameter', 'test-source'] - ['testlib', 'Member[getSource].ReturnValue', 'test-source'] + - ['testlib', 'Member[getSourceArray].ReturnValue.ArrayElement', 'test-source'] - ['(testlib)', 'Member[parenthesizedPackageName].ReturnValue', 'test-source'] - ['danger-constant', 'Member[danger]', 'test-source'] diff --git a/javascript/ql/test/library-tests/frameworks/data/test.js b/javascript/ql/test/library-tests/frameworks/data/test.js index 71fd64e10f6..fa570eb0789 100644 --- a/javascript/ql/test/library-tests/frameworks/data/test.js +++ b/javascript/ql/test/library-tests/frameworks/data/test.js @@ -278,3 +278,11 @@ function dangerConstant() { sink("danger-constant".safe); // OK sink("danger-constant"); // OK } + +function arraySource() { + const source = testlib.getSourceArray(); + sink(source[0]); // NOT OK [INCONSISTENCY] + sink(source.pop()); // NOT OK [INCONSISTENCY] + source.forEach(e => sink(e)); // NOT OK [INCONSISTENCY] + source.map(e => sink(e)); // NOT OK [INCONSISTENCY] +} From 4c1c0b79a60b3e40cc1e67b021564815f699c5f8 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Mar 2025 11:44:52 +0100 Subject: [PATCH 2/8] JS: Make API-graphs use Content internally, and use steps from flow summaries --- .../ql/lib/semmle/javascript/ApiGraphs.qll | 122 ++++++++++++++---- .../javascript/dataflow/internal/Contents.qll | 10 ++ .../data/internal/ApiGraphModelsSpecific.qll | 9 +- .../frameworks/data/test.expected | 4 + .../library-tests/frameworks/data/test.js | 8 +- .../Xss.expected | 14 ++ .../testUseQueries.vue | 4 +- 7 files changed, 131 insertions(+), 40 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll index 9e866424c61..d0e207197bc 100644 --- a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll @@ -8,6 +8,8 @@ import javascript private import semmle.javascript.dataflow.internal.FlowSteps as FlowSteps private import semmle.javascript.dataflow.internal.PreCallGraphStep +private import semmle.javascript.dataflow.internal.StepSummary +private import semmle.javascript.dataflow.internal.sharedlib.SummaryTypeTracker as SummaryTypeTracker private import internal.CachedStages /** @@ -229,6 +231,40 @@ module API { result = this.getASuccessor(Label::unknownMember()) } + cached + private Node getContentRaw(DataFlow::Content content) { + Stages::ApiStage::ref() and + result = this.getASuccessor(Label::content(content)) + } + + /** + * Gets a representative for the `content` of this value. + * + * When possible, it is preferrable to use one of the specialized variants of this predicate, such as `getMember`. + */ + pragma[inline] + Node getContent(DataFlow::Content content) { + result = this.getContentRaw(content) + or + result = this.getMember(content.asPropertyName()) + } + + /** + * Gets a representative for the `contents` of this value. + */ + bindingset[this, contents] + pragma[inline_late] + Node getContents(DataFlow::ContentSet contents) { + // We always use getAStoreContent when generating content edges, and we always use getAReadContent when querying the graph. + result = this.getContent(contents.getAReadContent()) + } + + /** + * Gets a node representing an arbitrary array element in the array represented by this node. + */ + cached + Node getArrayElement() { result = this.getContents(DataFlow::ContentSet::arrayElement()) } + /** * Gets a node representing a member of this API component where the name of the member may * or may not be known statically. @@ -790,6 +826,11 @@ module API { not DataFlow::PseudoProperties::isPseudoProperty(prop) ) or + exists(DataFlow::ContentSet contents | + SummaryTypeTracker::basicStoreStep(rhs, pred.getALocalUse(), contents) and + lbl = Label::content(contents.getAStoreContent()) + ) + or exists(DataFlow::FunctionNode fn | fn = pred and lbl = Label::return() @@ -982,6 +1023,11 @@ module API { // avoid generating member edges like "$arrayElement$" not DataFlow::PseudoProperties::isPseudoProperty(prop) ) + or + exists(DataFlow::ContentSet contents | + SummaryTypeTracker::basicLoadStep(pred.getALocalUse(), ref, contents) and + lbl = Label::content(contents.getAStoreContent()) + ) ) or exists(DataFlow::Node def, DataFlow::FunctionNode fn | @@ -1199,8 +1245,6 @@ module API { t = useStep(nd, promisified, boundArgs, prop, result) } - private import semmle.javascript.dataflow.internal.StepSummary - /** * Holds if `nd`, which is a use of an API-graph node, flows in zero or more potentially * inter-procedural steps to some intermediate node, and then from that intermediate node to @@ -1458,8 +1502,11 @@ module API { bindingset[result] LabelMember member(string m) { result.getProperty() = m } + /** Gets the `content` edge label for content `c`. */ + LabelContent content(ContentPrivate::Content c) { result.getContent() = c } + /** Gets the `member` edge label for the unknown member. */ - LabelUnknownMember unknownMember() { any() } + LabelContent unknownMember() { result.getContent().isUnknownArrayElement() } /** * Gets a property name referred to by the given dynamic property access, @@ -1516,10 +1563,10 @@ module API { LabelForwardingFunction forwardingFunction() { any() } /** Gets the `promised` edge label connecting a promise to its contained value. */ - LabelPromised promised() { any() } + LabelContent promised() { result.getContent() = ContentPrivate::MkPromiseValue() } /** Gets the `promisedError` edge label connecting a promise to its rejected value. */ - LabelPromisedError promisedError() { any() } + LabelContent promisedError() { result.getContent() = ContentPrivate::MkPromiseError() } /** Gets the label for an edge leading from a value `D` to any class that has `D` as a decorator. */ LabelDecoratedClass decoratedClass() { any() } @@ -1533,6 +1580,7 @@ module API { /** Gets an entry-point label for the entry-point `e`. */ LabelEntryPoint entryPoint(API::EntryPoint e) { result.getEntryPoint() = e } + private import semmle.javascript.dataflow.internal.Contents::Private as ContentPrivate private import LabelImpl private module LabelImpl { @@ -1542,18 +1590,12 @@ module API { exists(Impl::MkModuleImport(mod)) } or MkLabelInstance() or - MkLabelMember(string prop) { - exports(_, prop, _) or - exists(any(DataFlow::ClassNode c).getInstanceMethod(prop)) or - prop = "exports" or - prop = any(CanonicalName c).getName() or - prop = any(DataFlow::PropRef p).getPropertyName() or - exists(Impl::MkTypeUse(_, prop)) or - exists(any(Module m).getAnExportedValue(prop)) or - PreCallGraphStep::loadStep(_, _, prop) or - PreCallGraphStep::storeStep(_, _, prop) + MkLabelContent(DataFlow::Content content) or + MkLabelMember(string name) { + name instanceof PropertyName + or + exists(Impl::MkTypeUse(_, name)) } or - MkLabelUnknownMember() or MkLabelParameter(int i) { i = [0 .. max(int args | @@ -1564,8 +1606,6 @@ module API { } or MkLabelReceiver() or MkLabelReturn() or - MkLabelPromised() or - MkLabelPromisedError() or MkLabelDecoratedClass() or MkLabelDecoratedMember() or MkLabelDecoratedParameter() or @@ -1585,13 +1625,13 @@ module API { } /** A label that gets a promised value. */ - class LabelPromised extends ApiLabel, MkLabelPromised { - override string toString() { result = "getPromised()" } + deprecated class LabelPromised extends ApiLabel { + LabelPromised() { this = MkLabelContent(ContentPrivate::MkPromiseValue()) } } /** A label that gets a rejected promise. */ - class LabelPromisedError extends ApiLabel, MkLabelPromisedError { - override string toString() { result = "getPromisedError()" } + deprecated class LabelPromisedError extends ApiLabel { + LabelPromisedError() { this = MkLabelContent(ContentPrivate::MkPromiseError()) } } /** A label that gets the return value of a function. */ @@ -1617,9 +1657,39 @@ module API { override string toString() { result = "getInstance()" } } + /** A label for a content. */ + class LabelContent extends ApiLabel, MkLabelContent { + private DataFlow::Content content; + + LabelContent() { + this = MkLabelContent(content) and + // Property names are represented by LabelMember to ensure additional property + // names from PreCallGraph step are included, as well as those from MkTypeUse. + not content instanceof ContentPrivate::MkPropertyContent + } + + /** Gets the content associated with this label. */ + DataFlow::Content getContent() { result = content } + + private string specialisedToString() { + content instanceof ContentPrivate::MkPromiseValue and result = "getPromised()" + or + content instanceof ContentPrivate::MkPromiseError and result = "getPromisedError()" + or + content instanceof ContentPrivate::MkArrayElementUnknown and result = "getUnknownMember()" + } + + override string toString() { + result = this.specialisedToString() + or + not exists(this.specialisedToString()) and + result = "getContent(" + content + ")" + } + } + /** A label for the member named `prop`. */ class LabelMember extends ApiLabel, MkLabelMember { - string prop; + private string prop; LabelMember() { this = MkLabelMember(prop) } @@ -1630,10 +1700,8 @@ module API { } /** A label for a member with an unknown name. */ - class LabelUnknownMember extends ApiLabel, MkLabelUnknownMember { - LabelUnknownMember() { this = MkLabelUnknownMember() } - - override string toString() { result = "getUnknownMember()" } + deprecated class LabelUnknownMember extends LabelContent { + LabelUnknownMember() { this.getContent().isUnknownArrayElement() } } /** A label for parameter `i`. */ diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/Contents.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/Contents.qll index 23b2311594e..5cf5bf1e48e 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/Contents.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/Contents.qll @@ -57,6 +57,16 @@ module Private { this = getAPreciseArrayIndex().toString() or isAccessPathTokenPresent("Member", this) + or + this = any(ImportSpecifier spec).getImportedName() + or + this = any(ExportSpecifier n).getExportedName() + or + this = any(ExportNamedDeclaration d).getAnExportedDecl().getName() + or + this = any(MemberDefinition m).getName() + or + this = ["exports", "default"] } /** Gets the array index corresponding to this property name. */ diff --git a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll index 1b1df4ceef3..677aa6063f7 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll @@ -162,8 +162,8 @@ API::Node getExtraSuccessorFromNode(API::Node node, AccessPathTokenBase token) { token.getName() = "Awaited" and result = node.getPromised() or - token.getName() = "ArrayElement" and - result = node.getMember(DataFlow::PseudoProperties::arrayElement()) + token.getName() = ["ArrayElement", "Element"] and + result = node.getArrayElement() or token.getName() = "Element" and result = node.getMember(DataFlow::PseudoProperties::arrayLikeElement()) @@ -172,11 +172,6 @@ API::Node getExtraSuccessorFromNode(API::Node node, AccessPathTokenBase token) { token.getName() = "MapValue" and result = node.getMember(DataFlow::PseudoProperties::mapValueAll()) or - // Currently we need to include the "unknown member" for ArrayElement and Element since - // API graphs do not use store/load steps for arrays - token.getName() = ["ArrayElement", "Element"] and - result = node.getUnknownMember() - or token.getName() = "Parameter" and token.getAnArgument() = "this" and result = node.getReceiver() diff --git a/javascript/ql/test/library-tests/frameworks/data/test.expected b/javascript/ql/test/library-tests/frameworks/data/test.expected index 70fc4b00eab..6586eaeff15 100644 --- a/javascript/ql/test/library-tests/frameworks/data/test.expected +++ b/javascript/ql/test/library-tests/frameworks/data/test.expected @@ -81,6 +81,10 @@ taintFlow | test.js:272:6:272:40 | new MyS ... ource() | test.js:272:6:272:40 | new MyS ... ource() | | test.js:274:6:274:39 | testlib ... eName() | test.js:274:6:274:39 | testlib ... eName() | | test.js:277:8:277:31 | "danger ... .danger | test.js:277:8:277:31 | "danger ... .danger | +| test.js:284:8:284:16 | source[0] | test.js:284:8:284:16 | source[0] | +| test.js:285:8:285:19 | source.pop() | test.js:285:8:285:19 | source.pop() | +| test.js:286:18:286:18 | e | test.js:286:28:286:28 | e | +| test.js:287:14:287:14 | e | test.js:287:24:287:24 | e | isSink | test.js:54:18:54:25 | source() | test-sink | | test.js:55:22:55:29 | source() | test-sink | diff --git a/javascript/ql/test/library-tests/frameworks/data/test.js b/javascript/ql/test/library-tests/frameworks/data/test.js index fa570eb0789..a67c4244bdf 100644 --- a/javascript/ql/test/library-tests/frameworks/data/test.js +++ b/javascript/ql/test/library-tests/frameworks/data/test.js @@ -281,8 +281,8 @@ function dangerConstant() { function arraySource() { const source = testlib.getSourceArray(); - sink(source[0]); // NOT OK [INCONSISTENCY] - sink(source.pop()); // NOT OK [INCONSISTENCY] - source.forEach(e => sink(e)); // NOT OK [INCONSISTENCY] - source.map(e => sink(e)); // NOT OK [INCONSISTENCY] + sink(source[0]); // NOT OK + sink(source.pop()); // NOT OK + source.forEach(e => sink(e)); // NOT OK + source.map(e => sink(e)); // NOT OK } diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXssWithResponseThreat/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXssWithResponseThreat/Xss.expected index 7aa3957d64d..ef0c88098b9 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXssWithResponseThreat/Xss.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXssWithResponseThreat/Xss.expected @@ -16,6 +16,7 @@ | testReactUseQueries.jsx:37:25:37:38 | repoQuery.data | testReactUseQueries.jsx:4:26:4:53 | fetch(' ... e.com') | testReactUseQueries.jsx:37:25:37:38 | repoQuery.data | Cross-site scripting vulnerability due to $@. | testReactUseQueries.jsx:4:26:4:53 | fetch(' ... e.com') | user-provided value | | testUseQueries2.vue:40:10:40:23 | v-html=data3 | testUseQueries2.vue:6:28:6:63 | fetch(" ... ntent") | testUseQueries2.vue:40:10:40:23 | v-html=data3 | Cross-site scripting vulnerability due to $@. | testUseQueries2.vue:6:28:6:63 | fetch(" ... ntent") | user-provided value | | testUseQueries2.vue:40:10:40:23 | v-html=data3 | testUseQueries2.vue:12:28:12:41 | fetch("${id}") | testUseQueries2.vue:40:10:40:23 | v-html=data3 | Cross-site scripting vulnerability due to $@. | testUseQueries2.vue:12:28:12:41 | fetch("${id}") | user-provided value | +| testUseQueries.vue:25:10:25:23 | v-html=data2 | testUseQueries.vue:11:36:11:49 | fetch("${id}") | testUseQueries.vue:25:10:25:23 | v-html=data2 | Cross-site scripting vulnerability due to $@. | testUseQueries.vue:11:36:11:49 | fetch("${id}") | user-provided value | edges | test.jsx:5:11:5:63 | response | test.jsx:6:24:6:31 | response | provenance | | | test.jsx:5:22:5:63 | await f ... ntent") | test.jsx:5:11:5:63 | response | provenance | | @@ -88,6 +89,12 @@ edges | testUseQueries2.vue:13:12:13:19 | response | testUseQueries2.vue:13:12:13:26 | response.json() | provenance | | | testUseQueries2.vue:13:12:13:26 | response.json() | testUseQueries2.vue:33:22:33:36 | results[0].data | provenance | | | testUseQueries2.vue:33:22:33:36 | results[0].data | testUseQueries2.vue:40:10:40:23 | v-html=data3 | provenance | | +| testUseQueries.vue:11:19:11:49 | response | testUseQueries.vue:12:20:12:27 | response | provenance | | +| testUseQueries.vue:11:30:11:49 | await fetch("${id}") | testUseQueries.vue:11:19:11:49 | response | provenance | | +| testUseQueries.vue:11:36:11:49 | fetch("${id}") | testUseQueries.vue:11:30:11:49 | await fetch("${id}") | provenance | | +| testUseQueries.vue:12:20:12:27 | response | testUseQueries.vue:12:20:12:34 | response.json() | provenance | | +| testUseQueries.vue:12:20:12:34 | response.json() | testUseQueries.vue:18:22:18:36 | results[0].data | provenance | | +| testUseQueries.vue:18:22:18:36 | results[0].data | testUseQueries.vue:25:10:25:23 | v-html=data2 | provenance | | nodes | test.jsx:5:11:5:63 | response | semmle.label | response | | test.jsx:5:22:5:63 | await f ... ntent") | semmle.label | await f ... ntent") | @@ -174,4 +181,11 @@ nodes | testUseQueries2.vue:13:12:13:26 | response.json() | semmle.label | response.json() | | testUseQueries2.vue:33:22:33:36 | results[0].data | semmle.label | results[0].data | | testUseQueries2.vue:40:10:40:23 | v-html=data3 | semmle.label | v-html=data3 | +| testUseQueries.vue:11:19:11:49 | response | semmle.label | response | +| testUseQueries.vue:11:30:11:49 | await fetch("${id}") | semmle.label | await fetch("${id}") | +| testUseQueries.vue:11:36:11:49 | fetch("${id}") | semmle.label | fetch("${id}") | +| testUseQueries.vue:12:20:12:27 | response | semmle.label | response | +| testUseQueries.vue:12:20:12:34 | response.json() | semmle.label | response.json() | +| testUseQueries.vue:18:22:18:36 | results[0].data | semmle.label | results[0].data | +| testUseQueries.vue:25:10:25:23 | v-html=data2 | semmle.label | v-html=data2 | subpaths diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXssWithResponseThreat/testUseQueries.vue b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXssWithResponseThreat/testUseQueries.vue index c02c0a79023..c0e58af5b21 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXssWithResponseThreat/testUseQueries.vue +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXssWithResponseThreat/testUseQueries.vue @@ -8,7 +8,7 @@ export default { queries: ids.map((id) => ({ queryKey: ['post', id], queryFn: async () => { - const response = await fetch("${id}"); // $ MISSING: Source + const response = await fetch("${id}"); // $ Source return response.json(); }, staleTime: Infinity, @@ -22,6 +22,6 @@ export default { From ab74898bbb490479867266a1fb8c92dfbb6d9c41 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Mar 2025 13:14:50 +0100 Subject: [PATCH 3/8] JS: Deprecate getUnknownMember() and replace its uses with getArrayElement() Although they mean slightly different things, every single call site of getUnknownMember() just used it as a way to get array elements. Since there is no known use-case for the original meaning of getUnknownMember() I am deprecating it for now. --- .../ql/lib/semmle/javascript/ApiGraphs.qll | 25 +++++++++++++------ .../lib/semmle/javascript/frameworks/D3.qll | 2 +- .../javascript/frameworks/Puppeteer.qll | 6 ++--- .../lib/semmle/javascript/frameworks/Vuex.qll | 3 +-- .../data/internal/ApiGraphModelsSpecific.qll | 2 +- .../javascript/internal/CachedStages.qll | 3 +-- ...APIUsedWithUntrustedDataCustomizations.qll | 6 +---- .../src/experimental/Security/CWE-347/JWT.qll | 4 +-- .../experimental/semmle/javascript/Execa.qll | 2 +- 9 files changed, 28 insertions(+), 25 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll index d0e207197bc..d152804cee2 100644 --- a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll @@ -10,6 +10,7 @@ private import semmle.javascript.dataflow.internal.FlowSteps as FlowSteps private import semmle.javascript.dataflow.internal.PreCallGraphStep private import semmle.javascript.dataflow.internal.StepSummary private import semmle.javascript.dataflow.internal.sharedlib.SummaryTypeTracker as SummaryTypeTracker +private import semmle.javascript.dataflow.internal.Contents::Private as ContentPrivate private import internal.CachedStages /** @@ -222,13 +223,17 @@ module API { } /** - * Gets a node representing a member of this API component where the name of the member is - * not known statically. + * DEPRECATED. Use either `getArrayElement()` or `getAMember()` instead. + */ + deprecated Node getUnknownMember() { result = this.getArrayElement() } + + /** + * Gets an array element of unknown index. */ cached - Node getUnknownMember() { + Node getUnknownArrayElement() { Stages::ApiStage::ref() and - result = this.getASuccessor(Label::unknownMember()) + result = this.getASuccessor(Label::content(ContentPrivate::MkArrayElementUnknown())) } cached @@ -274,7 +279,7 @@ module API { Stages::ApiStage::ref() and result = this.getMember(_) or - result = this.getUnknownMember() + result = this.getUnknownArrayElement() } /** @@ -1505,7 +1510,12 @@ module API { /** Gets the `content` edge label for content `c`. */ LabelContent content(ContentPrivate::Content c) { result.getContent() = c } - /** Gets the `member` edge label for the unknown member. */ + /** + * Gets the edge label for an unknown member. + * + * Currently this is represented the same way as an unknown array element, but this may + * change in the future. + */ LabelContent unknownMember() { result.getContent().isUnknownArrayElement() } /** @@ -1580,7 +1590,6 @@ module API { /** Gets an entry-point label for the entry-point `e`. */ LabelEntryPoint entryPoint(API::EntryPoint e) { result.getEntryPoint() = e } - private import semmle.javascript.dataflow.internal.Contents::Private as ContentPrivate private import LabelImpl private module LabelImpl { @@ -1676,7 +1685,7 @@ module API { or content instanceof ContentPrivate::MkPromiseError and result = "getPromisedError()" or - content instanceof ContentPrivate::MkArrayElementUnknown and result = "getUnknownMember()" + content instanceof ContentPrivate::MkArrayElementUnknown and result = "getArrayElement()" } override string toString() { diff --git a/javascript/ql/lib/semmle/javascript/frameworks/D3.qll b/javascript/ql/lib/semmle/javascript/frameworks/D3.qll index 76bdeb1324a..cc7c07c80c1 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/D3.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/D3.qll @@ -80,7 +80,7 @@ module D3 { or this = d3Selection().getMember("node").getReturn().asSource() or - this = d3Selection().getMember("nodes").getReturn().getUnknownMember().asSource() + this = d3Selection().getMember("nodes").getReturn().getArrayElement().asSource() } } diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Puppeteer.qll b/javascript/ql/lib/semmle/javascript/frameworks/Puppeteer.qll index 0834d81e0a1..59bb2fc2b13 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/Puppeteer.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/Puppeteer.qll @@ -32,7 +32,7 @@ module Puppeteer { or result = [browser(), context()].getMember("newPage").getReturn().getPromised() or - result = [browser(), context()].getMember("pages").getReturn().getPromised().getUnknownMember() + result = [browser(), context()].getMember("pages").getReturn().getPromised().getArrayElement() or result = target().getMember("page").getReturn().getPromised() } @@ -45,7 +45,7 @@ module Puppeteer { or result = [page(), browser()].getMember("target").getReturn() or - result = context().getMember("targets").getReturn().getUnknownMember() + result = context().getMember("targets").getReturn().getArrayElement() or result = target().getMember("opener").getReturn() } @@ -58,7 +58,7 @@ module Puppeteer { or result = [page(), target()].getMember("browserContext").getReturn() or - result = browser().getMember("browserContexts").getReturn().getUnknownMember() + result = browser().getMember("browserContexts").getReturn().getArrayElement() or result = browser().getMember("createIncognitoBrowserContext").getReturn().getPromised() or diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Vuex.qll b/javascript/ql/lib/semmle/javascript/frameworks/Vuex.qll index 6e111207790..49a9ded864b 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/Vuex.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/Vuex.qll @@ -104,8 +104,7 @@ module Vuex { storeName = this.getNamespace() + localName or // mapGetters(['foo', 'bar']) - this.getLastParameter().getUnknownMember().getAValueReachingSink().getStringValue() = - localName and + this.getLastParameter().getArrayElement().getAValueReachingSink().getStringValue() = localName and storeName = this.getNamespace() + localName or // mapGetters({foo: 'bar'}) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll index 677aa6063f7..29cd5da8da1 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll @@ -368,7 +368,7 @@ bindingset[pred] predicate apiGraphHasEdge(API::Node pred, string path, API::Node succ) { exists(string name | succ = pred.getMember(name) and path = "Member[" + name + "]") or - succ = pred.getUnknownMember() and path = "AnyMember" + succ = pred.getUnknownArrayElement() and path = "ArrayElement" or succ = pred.getInstance() and path = "Instance" or diff --git a/javascript/ql/lib/semmle/javascript/internal/CachedStages.qll b/javascript/ql/lib/semmle/javascript/internal/CachedStages.qll index 470435ce23c..98a35692822 100644 --- a/javascript/ql/lib/semmle/javascript/internal/CachedStages.qll +++ b/javascript/ql/lib/semmle/javascript/internal/CachedStages.qll @@ -297,13 +297,12 @@ module Stages { exists( API::moduleImport("foo") .getMember("bar") - .getUnknownMember() + .getArrayElement() .getAMember() .getAParameter() .getPromised() .getReturn() .getParameter(2) - .getUnknownMember() .getInstance() .getReceiver() .getForwardingFunction() diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/ExternalAPIUsedWithUntrustedDataCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/ExternalAPIUsedWithUntrustedDataCustomizations.qll index 72fdafaad50..7a6575f8647 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/ExternalAPIUsedWithUntrustedDataCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/ExternalAPIUsedWithUntrustedDataCustomizations.qll @@ -179,7 +179,6 @@ module ExternalApiUsedWithUntrustedData { or exists(string member | node = base.getMember(member) and - not node = base.getUnknownMember() and not isNumericString(member) and not (member = "default" and base = API::moduleImport(_)) and not member = "then" // use the 'promised' edges for .then callbacks @@ -189,10 +188,7 @@ module ExternalApiUsedWithUntrustedData { else result = basename + "['" + member.regexpReplaceAll("'", "\\'") + "']" ) or - ( - node = base.getUnknownMember() or - node = base.getMember(any(string s | isNumericString(s))) - ) and + node = base.getArrayElement() and result = basename + "[]" or // just collapse promises diff --git a/javascript/ql/src/experimental/Security/CWE-347/JWT.qll b/javascript/ql/src/experimental/Security/CWE-347/JWT.qll index ad4a82b6763..2f70938dd8a 100644 --- a/javascript/ql/src/experimental/Security/CWE-347/JWT.qll +++ b/javascript/ql/src/experimental/Security/CWE-347/JWT.qll @@ -7,7 +7,7 @@ DataFlow::Node unverifiedDecode() { verify .getParameter(2) .getMember("algorithms") - .getUnknownMember() + .getArrayElement() .asSink() .mayHaveStringValue("none") and result = verify.getParameter(0).asSink() @@ -32,7 +32,7 @@ DataFlow::Node verifiedDecode() { not verify .getParameter(2) .getMember("algorithms") - .getUnknownMember() + .getArrayElement() .asSink() .mayHaveStringValue("none") or not exists(verify.getParameter(2).getMember("algorithms")) diff --git a/javascript/ql/src/experimental/semmle/javascript/Execa.qll b/javascript/ql/src/experimental/semmle/javascript/Execa.qll index 2f301ae0bf8..624b21c5dac 100644 --- a/javascript/ql/src/experimental/semmle/javascript/Execa.qll +++ b/javascript/ql/src/experimental/semmle/javascript/Execa.qll @@ -72,7 +72,7 @@ module Execa { override predicate isShellInterpreted(DataFlow::Node arg) { // if shell: true then first and second args are sinks // options can be third argument - arg = [this.getArgument(0), this.getParameter(1).getUnknownMember().asSink()] and + arg = [this.getArgument(0), this.getParameter(1).getArrayElement().asSink()] and isExecaShellEnable(this.getParameter(2)) or // options can be second argument From fe1bdf24682d32265f3cab52fbf52fdc7181f3fe Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Mar 2025 13:20:07 +0100 Subject: [PATCH 4/8] JS: Update a test --- javascript/ql/test/ApiGraphs/dynamic-prop-read/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/test/ApiGraphs/dynamic-prop-read/index.js b/javascript/ql/test/ApiGraphs/dynamic-prop-read/index.js index 14d8b69a741..cf900bc7525 100644 --- a/javascript/ql/test/ApiGraphs/dynamic-prop-read/index.js +++ b/javascript/ql/test/ApiGraphs/dynamic-prop-read/index.js @@ -2,4 +2,4 @@ const MyStream = require('classes').MyStream; var s = new MyStream(); for (let m of ["write"]) - s[m]("Hello, world!"); /* use=moduleImport("classes").getMember("exports").getMember("MyStream").getInstance().getUnknownMember() */ \ No newline at end of file + s[m]("Hello, world!"); /* use=moduleImport("classes").getMember("exports").getMember("MyStream").getInstance().getArrayElement() */ From cd3909245d985ae0acf96988f8119de6a91a0c6b Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 14 Mar 2025 22:50:28 +0100 Subject: [PATCH 5/8] JS: Bugfix in Array constructor summary --- .../lib/semmle/javascript/internal/flow_summaries/Arrays.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll index d9f0836d739..00fed9c4f09 100644 --- a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll +++ b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll @@ -96,7 +96,8 @@ class ArrayConstructorSummary extends SummarizedCallable { ArrayConstructorSummary() { this = "Array constructor" } override DataFlow::InvokeNode getACallSimple() { - result = arrayConstructorRef().getAnInvocation() + result = arrayConstructorRef().getAnInvocation() and + result.getNumArgument() > 1 } override predicate propagatesFlow(string input, string output, boolean preservesValue) { From 125e732c4ce36fc0999e542875ace54a7ddb8666 Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Mar 2025 13:44:33 +0100 Subject: [PATCH 6/8] JS: Fix bad join order --- javascript/ql/lib/semmle/javascript/ApiGraphs.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll index d152804cee2..a31a3ae3e0f 100644 --- a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll @@ -257,9 +257,9 @@ module API { /** * Gets a representative for the `contents` of this value. */ - bindingset[this, contents] + bindingset[contents] pragma[inline_late] - Node getContents(DataFlow::ContentSet contents) { + private Node getContents(DataFlow::ContentSet contents) { // We always use getAStoreContent when generating content edges, and we always use getAReadContent when querying the graph. result = this.getContent(contents.getAReadContent()) } From 1516029cf5fd1a04e9d114d180b59ef1b2491d5c Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 17 Mar 2025 13:48:13 +0100 Subject: [PATCH 7/8] JS: Avoid generating ArrayElement edges for extend-like patterns --- .../ql/lib/semmle/javascript/ApiGraphs.qll | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll index a31a3ae3e0f..4ef187de4d8 100644 --- a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll @@ -11,6 +11,7 @@ private import semmle.javascript.dataflow.internal.PreCallGraphStep private import semmle.javascript.dataflow.internal.StepSummary private import semmle.javascript.dataflow.internal.sharedlib.SummaryTypeTracker as SummaryTypeTracker private import semmle.javascript.dataflow.internal.Contents::Private as ContentPrivate +private import semmle.javascript.DynamicPropertyAccess private import internal.CachedStages /** @@ -1516,7 +1517,12 @@ module API { * Currently this is represented the same way as an unknown array element, but this may * change in the future. */ - LabelContent unknownMember() { result.getContent().isUnknownArrayElement() } + ApiLabel unknownMember() { result = arrayElement() } + + /** + * Gets the edge label for an unknown array element. + */ + LabelContent arrayElement() { result.getContent().isUnknownArrayElement() } /** * Gets a property name referred to by the given dynamic property access, @@ -1539,6 +1545,11 @@ module API { result = unique(string s | s = getAnIndirectPropName(ref)) } + pragma[nomagic] + private predicate isEnumeratedPropName(DataFlow::Node node) { + node.getAPredecessor*() instanceof EnumeratedPropName + } + /** Gets the `member` edge label for the given property reference. */ ApiLabel memberFromRef(DataFlow::PropRef pr) { exists(string pn | pn = pr.getPropertyName() or pn = getIndirectPropName(pr) | @@ -1550,7 +1561,9 @@ module API { or not exists(pr.getPropertyName()) and not exists(getIndirectPropName(pr)) and - result = unknownMember() + // Avoid assignments in an extend-like pattern + not isEnumeratedPropName(pr.getPropertyNameExpr().flow()) and + result = arrayElement() } /** Gets the `instance` edge label. */ From 53ba588993515096824f61815006337e01ffdf64 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 18 Mar 2025 09:26:02 +0100 Subject: [PATCH 8/8] JS: Use ArrayElement instead of AnyMember The use of AnyMember was a workaround until the bugfix in this PR landed. --- javascript/ql/lib/ext/tanstack.model.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/ext/tanstack.model.yml b/javascript/ql/lib/ext/tanstack.model.yml index d2b0bc0d782..2c07fcf9727 100644 --- a/javascript/ql/lib/ext/tanstack.model.yml +++ b/javascript/ql/lib/ext/tanstack.model.yml @@ -6,6 +6,6 @@ extensions: - ["@tanstack/angular-query-experimental", "Member[injectQuery]", "Argument[0].ReturnValue.Member[queryFn].ReturnValue", "ReturnValue.Member[data].Awaited", "value"] - ["@tanstack/angular-query", "Member[injectQuery]", "Argument[0].ReturnValue.Member[queryFn].ReturnValue", "ReturnValue.Member[data].Awaited", "value"] - ["@tanstack/vue-query", "Member[useQuery]", "Argument[0].Member[queryFn].ReturnValue.Awaited", "ReturnValue.Member[data]", "value"] - - ["@tanstack/vue-query", "Member[useQueries]", "Argument[0].Member[queries].ArrayElement.Member[queryFn].ReturnValue.Awaited", "ReturnValue.AnyMember.Member[data]", "value"] - - ["@tanstack/react-query", "Member[useQueries]", "Argument[0].Member[queries].ArrayElement.Member[queryFn].ReturnValue.Awaited", "ReturnValue.AnyMember.Member[data]", "value"] + - ["@tanstack/vue-query", "Member[useQueries]", "Argument[0].Member[queries].ArrayElement.Member[queryFn].ReturnValue.Awaited", "ReturnValue.ArrayElement.Member[data]", "value"] + - ["@tanstack/react-query", "Member[useQueries]", "Argument[0].Member[queries].ArrayElement.Member[queryFn].ReturnValue.Awaited", "ReturnValue.ArrayElement.Member[data]", "value"] - ["@tanstack/react-query", "Member[useQuery]", "Argument[0].Member[queryFn].ReturnValue.Awaited", "ReturnValue.Member[data]", "value"]