From 3cc1525985775c2e398303297a1ef68fa32e95bc Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 09:19:30 +0100 Subject: [PATCH 01/14] JS: Remove obsolete TODOs --- .../lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll | 1 - 1 file changed, 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll index f015cc1e5c90..954b40678045 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll @@ -20,7 +20,6 @@ private class Node = DataFlow::Node; class PostUpdateNode = DataFlow::PostUpdateNode; -// TODO: this bypasses refinement nodes, and therefore some sanitisers class SsaUseNode extends DataFlow::Node, TSsaUseNode { private ControlFlowNode expr; From 8ac08db5c23bf304fe1af1e045e0e55ce8a8fe79 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 09:24:58 +0100 Subject: [PATCH 02/14] JS: Remove TODOs about WithArrayElement not being a taint step This isn't going to become a taint step, the workaround is the permanent solution --- .../javascript/internal/flow_summaries/Arrays.qll | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) 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 702936b46c3c..d9f0836d739d 100644 --- a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll +++ b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll @@ -140,7 +140,7 @@ class CopyWithin extends SummarizedCallable { input = "Argument[this].WithArrayElement" and output = "ReturnValue" or - // TODO: workaround for WithArrayElement not being converted to a taint step + // Explicitly add a taint step since WithArrayElement is not implicitly converted to a taint step preservesValue = false and input = "Argument[this]" and output = "ReturnValue" @@ -186,7 +186,7 @@ class Filter extends SummarizedCallable { output = "ReturnValue" ) or - // TODO: workaround for WithArrayElement not being converted to a taint step + // Explicitly add a taint step since WithArrayElement is not implicitly converted to a taint step preservesValue = false and input = "Argument[this]" and output = "ReturnValue" @@ -328,10 +328,7 @@ class From1Arg extends SummarizedCallable { output = "ReturnValue[exception]" ) or - // TODO: we currently convert ArrayElement read/store steps to taint steps, but this does not - // work for WithArrayElement because it's just an expectsContent node, and there's no way easy - // to omit the expectsContent restriction in taint tracking. - // Work around this for now. + // Explicitly add a taint step since WithArrayElement is not implicitly converted to a taint step preservesValue = false and input = "Argument[0]" and output = "ReturnValue" @@ -561,7 +558,7 @@ class ArrayCoercionPackage extends FunctionalPackageSummary { output = "ReturnValue.ArrayElement" ) or - // TODO: workaround for WithArrayElement not being converted to a taint step + // Explicitly add a taint step since WithArrayElement is not implicitly converted to a taint step preservesValue = false and input = "Argument[0]" and output = "ReturnValue" From 7766f97232a702c857db50c103b33f6d50f07e18 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 09:25:08 +0100 Subject: [PATCH 03/14] JS: Remove obsolete TODO --- .../lib/semmle/javascript/dataflow/internal/VariableCapture.qll | 1 - 1 file changed, 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll index 6e381a792efe..e5428d96a4cd 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll @@ -114,7 +114,6 @@ module VariableCaptureConfig implements InputSig { class Callable extends js::StmtContainer { predicate isConstructor() { - // TODO: clarify exactly what the library wants to know here as the meaning of "constructor" varies between languages. // JS constructors should not be seen as "constructors" in this context. none() } From b29ee2acdeffacdbb09e63d19f85752893ac934d Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 09:26:53 +0100 Subject: [PATCH 04/14] JS: Remove references to localFieldStep These are tracked in https://github.com/github/codeql-javascript-team/issues/456 --- .../security/dataflow/PrototypePollutingAssignmentQuery.qll | 5 ----- .../javascript/security/dataflow/UnsafeCodeConstruction.qll | 4 ---- .../security/dataflow/UnsafeHtmlConstructionQuery.qll | 6 ------ .../security/dataflow/UnsafeJQueryPluginQuery.qll | 5 ----- .../dataflow/UnsafeShellCommandConstructionQuery.qll | 6 ------ .../javascript/security/regexp/PolynomialReDoSQuery.qll | 6 +----- 6 files changed, 1 insertion(+), 31 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll index e385640d6960..f58481bb689d 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll @@ -99,11 +99,6 @@ module PrototypePollutingAssignmentConfig implements DataFlow::StateConfigSig { state2 = FlowState::objectPrototype() ) or - // TODO: local field step becomes a jump step, resulting in FPs (closure-lib) - // TODO: localFieldStep is too expensive with dataflow2 - // DataFlow::localFieldStep(pred, succ) - none() - or state1 = FlowState::taint() and TaintTracking::defaultTaintStep(node1, node2) and state1 = state2 diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeCodeConstruction.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeCodeConstruction.qll index fc1e6c79b384..a7580b161ecc 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeCodeConstruction.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeCodeConstruction.qll @@ -29,10 +29,6 @@ module UnsafeCodeConstruction { predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { // HTML sanitizers are insufficient protection against code injection node1 = node2.(HtmlSanitizerCall).getInput() - or - none() - // TODO: localFieldStep is too expensive with dataflow2 - // DataFlow::localFieldStep(pred, succ) } DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll index 5fdf3825405f..700c47565fc3 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll @@ -47,12 +47,6 @@ module UnsafeHtmlConstructionConfig implements DataFlow::StateConfigSig { predicate isAdditionalFlowStep( DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2 ) { - // TODO: localFieldStep is too expensive with dataflow2 - // DataFlow::localFieldStep(pred, succ) and - // inlbl.isTaint() and - // outlbl.isTaint() - none() - or TaintedObject::isAdditionalFlowStep(node1, state1, node2, state2) or // property read from a tainted object is considered tainted diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll index e7bf16cf0c32..8cdd57f6a104 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll @@ -23,11 +23,6 @@ module UnsafeJQueryPluginConfig implements DataFlow::ConfigSig { } predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node sink) { - // jQuery plugins tend to be implemented as classes that store data in fields initialized by the constructor. - // TODO: localFieldStep is too expensive with dataflow2 - // DataFlow::localFieldStep(pred, succ) - none() - or aliasPropertyPresenceStep(node1, sink) } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll index 5eb11826bffd..6abcdf25be10 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll @@ -26,12 +26,6 @@ module UnsafeShellCommandConstructionConfig implements DataFlow::ConfigSig { node = TaintTracking::AdHocWhitelistCheckSanitizer::getABarrierNode() } - predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - none() - // TODO: localFieldStep is too expensive with dataflow2 - // DataFlow::localFieldStep(pred, succ) - } - DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } } diff --git a/javascript/ql/lib/semmle/javascript/security/regexp/PolynomialReDoSQuery.qll b/javascript/ql/lib/semmle/javascript/security/regexp/PolynomialReDoSQuery.qll index 0c0f502bb06b..8b21f8e98b39 100644 --- a/javascript/ql/lib/semmle/javascript/security/regexp/PolynomialReDoSQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/regexp/PolynomialReDoSQuery.qll @@ -22,11 +22,7 @@ module PolynomialReDoSConfig implements DataFlow::ConfigSig { DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } - predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - none() - // TODO: localFieldStep is too expensive with dataflow2 - // DataFlow::localFieldStep(pred, succ) - } + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { none() } int fieldFlowBranchLimit() { result = 1 } // library inputs are too expensive on some projects } From fb54a3bde89fc4a30f1b7c4f371105e30cebfbcc Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 09:28:24 +0100 Subject: [PATCH 05/14] JS: Remove obsolete TODO comment --- .../security/dataflow/PrototypePollutingAssignmentQuery.qll | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll index f58481bb689d..2793757add8d 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll @@ -65,10 +65,7 @@ module PrototypePollutingAssignmentConfig implements DataFlow::StateConfigSig { state = FlowState::objectPrototype() } - predicate isBarrierIn(DataFlow::Node node, FlowState state) { - // FIXME: This should only be an in-barrier for the corresponding flow state, but flow-state specific in-barriers are not supported right now. - isSource(node, state) - } + predicate isBarrierIn(DataFlow::Node node, FlowState state) { isSource(node, state) } predicate isAdditionalFlowStep( DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2 From dd37c474d8adad11dba0a27dfb880a36deaa2487 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 09:36:31 +0100 Subject: [PATCH 06/14] JS: Remove mention of results from comments --- .../semmle/javascript/security/dataflow/XssThroughDomQuery.qll | 2 -- 1 file changed, 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll index 2313751d9e70..74a840b05b26 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll @@ -12,8 +12,6 @@ private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizati * A taint-tracking configuration for reasoning about XSS through the DOM. */ module XssThroughDomConfig implements DataFlow::ConfigSig { - // NOTE: Gained FP in Lucifier due to spurious source but with more data flow (I think). - // TODO: Seen unexplained FP in meteor, likely due to spurious flow into a callback coming from another call site predicate isSource(DataFlow::Node source) { source instanceof Source } predicate isSink(DataFlow::Node sink) { sink instanceof DomBasedXss::Sink } From a8f93cac05de981a6a13d77524dd1b0ba7b75b85 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 09:37:59 +0100 Subject: [PATCH 07/14] JS: Remove obsolete comment The test case actually has the correct result now --- .../lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll | 1 - 1 file changed, 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll index 6979ec12a2e7..f27f61822a82 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll @@ -105,7 +105,6 @@ module DomBasedXssConfig implements DataFlow::StateConfigSig { state1.isTaintedUrlSuffix() and state2.isTaintedPrefix() or - // FIXME: this fails to work in the test case at jquery.js:37 exists(DataFlow::FunctionNode callback, DataFlow::Node arg | any(JQuery::MethodCall c).interpretsArgumentAsHtml(arg) and callback = arg.getABoundFunctionValue(_) and From 8b060c4294355c05eb0b31324073a5bd1026a94c Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 09:40:29 +0100 Subject: [PATCH 08/14] JS: Remove TODO about evaluating legacy steps There is an issue for tracking this. It's not a small fix. --- .../lib/semmle/javascript/dataflow/AdditionalTaintSteps.qll | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/AdditionalTaintSteps.qll b/javascript/ql/lib/semmle/javascript/dataflow/AdditionalTaintSteps.qll index fa52b5681738..a0752d768fa5 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/AdditionalTaintSteps.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/AdditionalTaintSteps.qll @@ -232,10 +232,7 @@ class LegacyTaintStep extends Unit { cached private module Cached { cached - predicate forceStage() { - // TODO: ensure that this stage is only evaluated if using the old data flow library - Stages::Taint::ref() - } + predicate forceStage() { Stages::Taint::ref() } /** * Holds if `pred` → `succ` should be considered a taint-propagating From 388dd871e1e9bb808ccb8cfcd7abbd8906ac746d Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 09:41:40 +0100 Subject: [PATCH 09/14] JS: Remove TODO tracked by an issue. This requires changes to the shared data flow library, not something we should track with a TODO in the JS codebase --- .../lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll index 954b40678045..3a0473aec262 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll @@ -639,7 +639,7 @@ predicate nodeIsHidden(Node node) { node instanceof CaptureNode or // Hide function expressions, as capture-flow causes them to appear in unhelpful ways - // TODO: Instead hide PathNodes with a capture content as the head of its access path? + // In the future we could hide PathNodes with a capture content as the head of its access path. node.asExpr() instanceof Function or // Also hide post-update nodes for function expressions From 3def8ecdeea95d18e86e4a1d8f64f617958cba8a Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 09:43:03 +0100 Subject: [PATCH 10/14] JS: Remove unimportant TODO --- .../lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll index 3a0473aec262..fe69a1fc8670 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll @@ -1401,7 +1401,7 @@ predicate readStep(Node node1, ContentSet c, Node node2) { // shift known array indices c.asSingleton().asArrayIndex() = content.asArrayIndex() + restIndex or - content.isUnknownArrayElement() and // TODO: don't read unknown array elements from static array + content.isUnknownArrayElement() and c = ContentSet::arrayElementUnknown() ) or From d9da9444faee65d0626bb6431828918da627df89 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 09:45:39 +0100 Subject: [PATCH 11/14] JS: Rephrase TODO This is useful info, but not something that can be fixed locally in this query, so a TODO comment isn't helping --- .../javascript/security/dataflow/UnsafeJQueryPluginQuery.qll | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll index 8cdd57f6a104..61bb129aa377 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll @@ -11,7 +11,9 @@ import UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin * A taint-tracking configuration for reasoning about XSS in unsafe jQuery plugins. */ module UnsafeJQueryPluginConfig implements DataFlow::ConfigSig { - // TODO: PropertyPresenceSanitizer should not block values in a content. + // Note: This query currently misses some results due to two issues: + // - PropertyPresenceSanitizer blocks values in a content + // - localFieldStep has been omitted for performance reaons predicate isSource(DataFlow::Node source) { source instanceof Source } predicate isSink(DataFlow::Node sink) { sink instanceof Sink } From b2d62a080b77d5d8614f32756f527d5d55880443 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 09:57:44 +0100 Subject: [PATCH 12/14] JS: Move a test failure explanation into the test suite We have an issue for fixing the underlying problem --- .../security/dataflow/UnsafeShellCommandConstructionQuery.qll | 2 -- .../Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll index 6abcdf25be10..54fa18795d8f 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll @@ -14,8 +14,6 @@ import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruct * A taint-tracking configuration for reasoning about shell command constructed from library input vulnerabilities. */ module UnsafeShellCommandConstructionConfig implements DataFlow::ConfigSig { - // TODO: we get a FP in the test case due to SanitizingRegExpTest not being able to generate a barrier edge - // for an edge into a phi node. predicate isSource(DataFlow::Node source) { source instanceof Source } predicate isSink(DataFlow::Node sink) { sink instanceof Sink } diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js index 09488f0a8871..75fda0090000 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js @@ -250,7 +250,7 @@ module.exports.goodSanitizer = function (name) { var cleaned = cleanInput(name); - cp.exec("rm -rf " + cleaned); // OK + cp.exec("rm -rf " + cleaned); // OK - But FP due to SanitizingRegExpTest not being able to generate a barrier edge for an edge into a phi node. } var fs = require("fs"); From 3f2882e1c6eaa8979509ee1dd0f642f9f980119a Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 09:59:23 +0100 Subject: [PATCH 13/14] JS: Remove an obsolete comment The RHS of an assignment actually has a post-update node now --- .../lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll index fe69a1fc8670..0e95d3511559 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll @@ -523,7 +523,7 @@ private predicate isArgumentNodeImpl(Node n, DataFlowCall call, ArgumentPosition // receiver of accessor call pos.isThis() and n = call.asAccessorCall().getBase() or - // argument to setter (TODO: this has no post-update node) + // argument to setter pos.asPositional() = 0 and n = call.asAccessorCall().(DataFlow::PropWrite).getRhs() or FlowSummaryImpl::Private::summaryArgumentNode(call.(SummaryCall).getReceiver(), From 9c4d378a1d9229a4752078287a7c203bb6246db1 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 9 Jan 2025 10:17:16 +0100 Subject: [PATCH 14/14] JS: Remove TODO comment It is not subsumed by the other case, both cases are needed --- .../lib/semmle/javascript/dataflow/internal/VariableCapture.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll index e5428d96a4cd..32ce88169e67 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll @@ -253,7 +253,7 @@ js::DataFlow::Node getNodeFromClosureNode(VariableCaptureOutput::ClosureNode nod TValueNode(node.(VariableCaptureOutput::ParameterNode) .getParameter() .asLocalVariable() - .getADeclaration()) // TODO: is this subsumed by the ExprNode case? + .getADeclaration()) or result = TThisNode(node.(VariableCaptureOutput::ParameterNode).getParameter().asThisContainer()) or