JS: Clean up some TODO comments

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

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;
 
@@ -524,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(),
@@ -640,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
@@ -1402,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

javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll

@@ -114,7 +114,6 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
 
  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()
  }
@@ -254,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

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"

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

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
@@ -99,11 +96,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

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 }

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

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 }
@@ -23,11 +25,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)
  }
 

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 }
@@ -26,12 +24,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 }
 }
 

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 }

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
 }

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");