Merge pull request #18451 from asgerf/jss/cleanup-todos

JS: Clean up some TODO comments

Author: asgerf

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 }
 }