[GitHub] [nifi] thenatog commented on a change in pull request #4206: NIFI-7304 Disabled CLF by default and allowed S2S & cluster comms to bypass length check

[GitBox <gi...@apache.org>]

thenatog commented on a change in pull request #4206:
URL: https://github.com/apache/nifi/pull/4206#discussion_r422274317



##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -659,11 +667,27 @@ private void addDocsServlets(WebAppContext docsContext) {
     }
 
     /**
-     * Adds configurable filters to the given context.  Currently, this implementation adds `DosFilter` and `ContentLengthFilter` filters.
-     * @param path path spec for filters
-     * @param webappContext context to which filters will be added
+     * Adds configurable filters to the given context.  Currently, this implementation adds {@code DosFilter} and {@link ContentLengthFilter} filters.
+     *
+     * @param path          path spec for filters ({@code /*} by convention in this class)
+     * @param webAppContext context to which filters will be added
+     * @param props         the {@link NiFiProperties}
      */
-    private void addFiltersWithProps(String path, WebAppContext webappContext) {
+    private static void addContentLengthFilters(String path, WebAppContext webAppContext, NiFiProperties props) {

Review comment:
       Can we rename this method to addDoSFilters() which in turn calls addWebRequestRateLimitFilter() (renamed from addDoSFilter()) and addContentLengthFilter()? Presumably limiting content length can also be considered a DoS protection, so now we have a DoS method which adds two filters instead of the content length filter method adding a DoS filter in addition to a content length filter. Alternatively, we could call methods named addContentLengthFilter() and addWebRequestRateLimitFilter() separately from loadWar().

##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-site-to-site/src/main/java/org/apache/nifi/remote/StandardPublicPort.java
##########
@@ -264,7 +265,7 @@ private int receiveFlowFiles(final ProcessContext context, final ProcessSession
 
     @Override
     public boolean isValid() {
-        return getConnectableType() == ConnectableType.INPUT_PORT ? !getConnections(Relationship.ANONYMOUS).isEmpty() : true;
+        return getConnectableType() != ConnectableType.INPUT_PORT || !getConnections(Relationship.ANONYMOUS).isEmpty();

Review comment:
       What is this checking for? It's a valid StandardPublicPort if it's not an INPUT_PORT or if there's anonymous relationships? Both the before and after are taking me far too long to understand what the check does - may be a result of tired brain. I think this could be simplified with a well named helper method or comment which explains why these conditions make it valid.

##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/java/org/apache/nifi/web/filter/ContentLengthFilterTest.java
##########
@@ -133,6 +133,36 @@ public void doPost(HttpServletRequest req, HttpServletResponse resp) throws Serv
         Assert.assertTrue(StringUtils.containsIgnoreCase(response, "Timeout"));
     }
 

Review comment:
       Whilst not a part of these changes, I see that there are tests in this class for content lengths of varying sizes compared to the header Content-Length claim. On line 123 the filter rejects a request that has a Content-Length greater than the set limit with an incomplete payload - great, that's what we want. 
   
   But, on line 127 we see a small claim is made, and a large payload is accepted. Is this not what the content filter should be blocking? Or are we only blocking based on the header claim? We could potentially also check the request input stream for available bytes with httpRequest.getInputStream().available()) - but I'm not sure under what circumstances this is accurate or even populated.
   
   Further we see that on line 131 we get a request with a claim of 150 bytes but receive only 10 bytes and as a result the request times-out waiting for the rest of the expected bytes. I don't believe there's a great way of avoiding this though I am looking into it.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org