Java: update/add some TODO comments

Author: Jami Cogswell

java/ql/lib/semmle/code/java/security/UrlForward.qll

@@ -8,48 +8,49 @@ private import semmle.code.java.dataflow.StringPrefixes
 /** A URL forward sink. */
 abstract class UrlForwardSink extends DataFlow::Node { }
 
-/** A default sink representing methods susceptible to URL forwarding attacks. */
+/**
+ * A default sink representing methods susceptible to URL
+ * forwarding attacks.
+ */
 private class DefaultUrlForwardSink extends UrlForwardSink {
   DefaultUrlForwardSink() { sinkNode(this, "url-forward") }
 }
 
 /**
- * An expression appended (perhaps indirectly) to `"forward:"`, and which
- * is reachable from a Spring entry point.
+ * An expression appended (perhaps indirectly) to `"forward:"`
+ * and reachable from a Spring entry point.
  */
 private class SpringUrlForwardSink extends UrlForwardSink {
   SpringUrlForwardSink() {
-    // TODO: check if can use MaD "Annotated" for `SpringRequestMappingMethod` or if too complicated for MaD (probably too complicated).
-    any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable()) and
+    any(SpringRequestMappingMethod srmm).polyCalls*(this.getEnclosingCallable()) and
     this.asExpr() = any(ForwardPrefix fp).getAnAppendedExpression()
   }
 }
 
-// TODO: should this potentially be "include:" as well? Or does that not work similarly?
 private class ForwardPrefix extends InterestingPrefix {
   ForwardPrefix() { this.getStringValue() = "forward:" }
 
   override int getOffset() { result = 0 }
 }
 
-/** A URL forward sanitizer. */
-abstract class UrlForwardSanitizer extends DataFlow::Node { }
+/** A URL forward barrier. */
+abstract class UrlForwardBarrier extends DataFlow::Node { }
 
-private class PrimitiveSanitizer extends UrlForwardSanitizer {
-  PrimitiveSanitizer() {
+private class PrimitiveBarrier extends UrlForwardBarrier {
+  PrimitiveBarrier() {
     this.getType() instanceof PrimitiveType or
     this.getType() instanceof BoxedType or
     this.getType() instanceof NumberType
   }
 }
 
-// TODO: double-check this sanitizer (and should I switch all "sanitizer" naming to "barrier" instead?)
-private class FollowsSanitizingPrefix extends UrlForwardSanitizer {
-  FollowsSanitizingPrefix() { this.asExpr() = any(SanitizingPrefix fp).getAnAppendedExpression() }
+private class FollowsBarrierPrefix extends UrlForwardBarrier {
+  FollowsBarrierPrefix() { this.asExpr() = any(BarrierPrefix fp).getAnAppendedExpression() }
 }
 
-private class SanitizingPrefix extends InterestingPrefix {
-  SanitizingPrefix() {
+private class BarrierPrefix extends InterestingPrefix {
+  BarrierPrefix() {
+    // TODO: why not META-INF here as well? (and are `/` correct?)
     not this.getStringValue().matches("/WEB-INF/%") and
     not this.getStringValue() = "forward:"
   }

java/ql/lib/semmle/code/java/security/UrlForwardQuery.qll

@@ -12,25 +12,24 @@ import semmle.code.java.security.PathSanitizer
 module UrlForwardFlowConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
     source instanceof ThreatModelFlowSource and
-    // TODO: move below logic to class in UrlForward.qll? And check exactly why these were excluded.
-    not exists(MethodCall ma, Method m | ma.getMethod() = m |
+    // excluded due to FPs
+    not exists(MethodCall mc, Method m | mc.getMethod() = m |
       (
         m instanceof HttpServletRequestGetRequestUriMethod or
         m instanceof HttpServletRequestGetRequestUrlMethod or
         m instanceof HttpServletRequestGetPathMethod
       ) and
-      ma = source.asExpr()
+      mc = source.asExpr()
     )
   }
 
   predicate isSink(DataFlow::Node sink) { sink instanceof UrlForwardSink }
 
   predicate isBarrier(DataFlow::Node node) {
-    node instanceof UrlForwardSanitizer or
+    node instanceof UrlForwardBarrier or
     node instanceof PathInjectionSanitizer
   }
 
-  // TODO: check if the below is still needed after removing path-injection related sinks.
   DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext }
 }