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:
[email protected]
