From 34f2a592df8996b5f9e65039a35ecd8c31417fbd Mon Sep 17 00:00:00 2001
From: Andy LoPresto <alopresto@apache.org>
Date: Wed, 15 Jan 2020 19:16:57 -0800
Subject: [PATCH] NIFI-7023 This closes #3991. Removed SLF4J and Log4J
 transitive dependencies from Zookeeper so tests log correctly. Changed
 template handling. Added unit tests.

Signed-off-by: Joe Witt <joewitt@apache.org>
---
 .../nifi-framework-core/pom.xml               |  10 +
 .../nifi/web/api/ProcessGroupResource.java    | 137 +++++++-------
 .../web/api/ProcessGroupResourceTest.groovy   | 172 ++++++++++++++++++
 3 files changed, 254 insertions(+), 65 deletions(-)
 create mode 100644 nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/groovy/org/apache/nifi/web/api/ProcessGroupResourceTest.groovy

diff --git a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/pom.xml b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/pom.xml
index 07467ebf65..6ef7cc2b6c 100644
--- a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/pom.xml
+++ b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/pom.xml
@@ -161,6 +161,16 @@
         <dependency>
             <groupId>org.apache.zookeeper</groupId>
             <artifactId>zookeeper</artifactId>
+            <exclusions>
+                <exclusion>
+                    <groupId>org.slf4j</groupId>
+                    <artifactId>slf4j-log4j12</artifactId>
+                </exclusion>
+                <exclusion>
+                    <groupId>log4j</groupId>
+                    <artifactId>log4j</artifactId>
+                </exclusion>
+            </exclusions>
         </dependency>
         <dependency>
             <groupId>org.apache.curator</groupId>
diff --git a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ProcessGroupResource.java b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ProcessGroupResource.java
index 142e25c890..d3b0b5b8a6 100644
--- a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ProcessGroupResource.java
+++ b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ProcessGroupResource.java
@@ -24,7 +24,58 @@ import io.swagger.annotations.ApiParam;
 import io.swagger.annotations.ApiResponse;
 import io.swagger.annotations.ApiResponses;
 import io.swagger.annotations.Authorization;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import javax.servlet.http.HttpServletRequest;
+import javax.ws.rs.Consumes;
+import javax.ws.rs.DELETE;
+import javax.ws.rs.DefaultValue;
+import javax.ws.rs.GET;
+import javax.ws.rs.HttpMethod;
+import javax.ws.rs.POST;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.MultivaluedHashMap;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.Response;
+import javax.ws.rs.core.Response.Status;
+import javax.ws.rs.core.UriBuilder;
+import javax.ws.rs.core.UriInfo;
+import javax.xml.bind.JAXBContext;
+import javax.xml.bind.JAXBElement;
+import javax.xml.bind.JAXBException;
+import javax.xml.bind.Unmarshaller;
+import javax.xml.stream.XMLStreamReader;
 import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.text.StringEscapeUtils;
 import org.apache.nifi.authorization.AuthorizableLookup;
 import org.apache.nifi.authorization.AuthorizeAccess;
 import org.apache.nifi.authorization.AuthorizeControllerServiceReference;
@@ -120,57 +171,6 @@ import org.slf4j.LoggerFactory;
 import org.springframework.security.core.Authentication;
 import org.springframework.security.core.context.SecurityContextHolder;
 
-import javax.servlet.http.HttpServletRequest;
-import javax.ws.rs.Consumes;
-import javax.ws.rs.DELETE;
-import javax.ws.rs.DefaultValue;
-import javax.ws.rs.GET;
-import javax.ws.rs.HttpMethod;
-import javax.ws.rs.POST;
-import javax.ws.rs.PUT;
-import javax.ws.rs.Path;
-import javax.ws.rs.PathParam;
-import javax.ws.rs.Produces;
-import javax.ws.rs.QueryParam;
-import javax.ws.rs.core.Context;
-import javax.ws.rs.core.HttpHeaders;
-import javax.ws.rs.core.MediaType;
-import javax.ws.rs.core.MultivaluedHashMap;
-import javax.ws.rs.core.MultivaluedMap;
-import javax.ws.rs.core.Response;
-import javax.ws.rs.core.Response.Status;
-import javax.ws.rs.core.UriBuilder;
-import javax.ws.rs.core.UriInfo;
-import javax.xml.bind.JAXBContext;
-import javax.xml.bind.JAXBElement;
-import javax.xml.bind.JAXBException;
-import javax.xml.bind.Unmarshaller;
-import javax.xml.stream.XMLStreamReader;
-import java.io.IOException;
-import java.io.InputStream;
-import java.net.URI;
-import java.net.URISyntaxException;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.Date;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Set;
-import java.util.UUID;
-import java.util.concurrent.ArrayBlockingQueue;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
-import java.util.concurrent.ThreadFactory;
-import java.util.concurrent.ThreadPoolExecutor;
-import java.util.concurrent.TimeUnit;
-import java.util.function.Consumer;
-import java.util.function.Function;
-import java.util.stream.Collectors;
-
 /**
  * RESTful endpoint for managing a Group.
  */
@@ -1100,18 +1100,10 @@ public class ProcessGroupResource extends ApplicationResource {
                         return false;
                     }
 
-                    if (desiredState == ScheduledState.STOPPED && status.getAggregateSnapshot().getActiveThreadCount() != 0) {
-                        return false;
-                    }
-
-                    return true;
+                    return desiredState != ScheduledState.STOPPED || status.getAggregateSnapshot().getActiveThreadCount() == 0;
                 });
 
-        if (!allProcessorsMatch) {
-            return false;
-        }
-
-        return true;
+        return allProcessorsMatch;
     }
 
     /**
@@ -3631,6 +3623,7 @@ public class ProcessGroupResource extends ApplicationResource {
         // unmarshal the template
         final TemplateDTO template;
         try {
+            // TODO: Potentially refactor the template parsing to a service layer outside of the resource for web request handling
             JAXBContext context = JAXBContext.newInstance(TemplateDTO.class);
             Unmarshaller unmarshaller = context.createUnmarshaller();
             XMLStreamReader xsr = XmlUtils.createSafeReader(in);
@@ -3642,12 +3635,12 @@ public class ProcessGroupResource extends ApplicationResource {
             return Response.status(Response.Status.OK).entity(responseXml).type("application/xml").build();
         } catch (IllegalArgumentException iae) {
             logger.warn("Unable to import template.", iae);
-            String responseXml = String.format("<errorResponse status=\"%s\" statusText=\"%s\"/>", Response.Status.BAD_REQUEST.getStatusCode(), iae.getMessage());
+            String responseXml = String.format("<errorResponse status=\"%s\" statusText=\"%s\"/>", Response.Status.BAD_REQUEST.getStatusCode(), sanitizeErrorResponse(iae.getMessage()));
             return Response.status(Response.Status.OK).entity(responseXml).type("application/xml").build();
         } catch (Exception e) {
             logger.warn("An error occurred while importing a template.", e);
             String responseXml = String.format("<errorResponse status=\"%s\" statusText=\"Unable to import the specified template: %s\"/>",
-                    Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), e.getMessage());
+                    Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), sanitizeErrorResponse(e.getMessage()));
             return Response.status(Response.Status.OK).entity(responseXml).type("application/xml").build();
         }
 
@@ -3683,6 +3676,20 @@ public class ProcessGroupResource extends ApplicationResource {
         return importTemplate(httpServletRequest, groupId, entity);
     }
 
+    /**
+     * Returns the sanitized error response which can safely be displayed on the error page.
+     *
+     * @param errorResponse the initial error response
+     * @return the HTML-escaped error response
+     */
+    private String sanitizeErrorResponse(String errorResponse) {
+        if (errorResponse == null || StringUtils.isEmpty(errorResponse)) {
+            return "";
+        }
+
+        return StringEscapeUtils.escapeHtml4(errorResponse);
+    }
+
     /**
      * Imports the specified template.
      *
@@ -3751,12 +3758,12 @@ public class ProcessGroupResource extends ApplicationResource {
                         return generateCreatedResponse(URI.create(template.getUri()), entity).build();
                     } catch (IllegalArgumentException | IllegalStateException e) {
                         logger.info("Unable to import template: " + e);
-                        String responseXml = String.format("<errorResponse status=\"%s\" statusText=\"%s\"/>", Response.Status.BAD_REQUEST.getStatusCode(), e.getMessage());
+                        String responseXml = String.format("<errorResponse status=\"%s\" statusText=\"%s\"/>", Response.Status.BAD_REQUEST.getStatusCode(), sanitizeErrorResponse(e.getMessage()));
                         return Response.status(Response.Status.OK).entity(responseXml).type("application/xml").build();
                     } catch (Exception e) {
                         logger.warn("An error occurred while importing a template.", e);
                         String responseXml = String.format("<errorResponse status=\"%s\" statusText=\"Unable to import the specified template: %s\"/>",
-                                Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), e.getMessage());
+                                Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), sanitizeErrorResponse(e.getMessage()));
                         return Response.status(Response.Status.OK).entity(responseXml).type("application/xml").build();
                     }
                 }
diff --git a/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/groovy/org/apache/nifi/web/api/ProcessGroupResourceTest.groovy b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/groovy/org/apache/nifi/web/api/ProcessGroupResourceTest.groovy
new file mode 100644
index 0000000000..8ecf90558e
--- /dev/null
+++ b/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/groovy/org/apache/nifi/web/api/ProcessGroupResourceTest.groovy
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.api
+
+import org.apache.nifi.authorization.AuthorizeAccess
+import org.apache.nifi.util.NiFiProperties
+import org.apache.nifi.web.NiFiServiceFacade
+import org.apache.nifi.web.api.dto.FlowSnippetDTO
+import org.apache.nifi.web.api.dto.TemplateDTO
+import org.apache.nifi.web.api.entity.TemplateEntity
+import org.junit.After
+import org.junit.Before
+import org.junit.BeforeClass
+import org.junit.Rule
+import org.junit.Test
+import org.junit.rules.TestName
+import org.junit.runner.RunWith
+import org.junit.runners.JUnit4
+import org.slf4j.Logger
+import org.slf4j.LoggerFactory
+
+import javax.servlet.http.HttpServletRequest
+import javax.ws.rs.core.Response
+import javax.ws.rs.core.UriInfo
+
+@RunWith(JUnit4.class)
+class ProcessGroupResourceTest extends GroovyTestCase {
+    private static final Logger logger = LoggerFactory.getLogger(ProcessGroupResourceTest.class)
+
+    @Rule
+    public TestName testName = new TestName()
+
+    @BeforeClass
+    static void setUpOnce() throws Exception {
+        logger.metaClass.methodMissing = { String name, args ->
+            logger.debug("[${name?.toUpperCase()}] ${(args as List).join(" ")}")
+        }
+    }
+
+    @Before
+    void setUp() throws Exception {
+    }
+
+    @After
+    void tearDown() throws Exception {
+    }
+
+    /** This test creates a malformed template upload request to exercise error handling and sanitization */
+    @Test
+    void testUploadShouldHandleMalformedTemplate() {
+        // Arrange
+        ProcessGroupResource pgResource = new ProcessGroupResource()
+
+        // Mocking the returned template object to throw a specific exception would be nice
+        final String TEMPLATE_WITH_XSS_PLAIN = "<?xml version=\"1.0\" encoding='><script xmlns=\"http://www.w3.org/1999/xhtml\">alert(JSON.stringify(localstorage));</script><errorResponse test='?>"
+        logger.info("Malformed template XML: ${TEMPLATE_WITH_XSS_PLAIN}")
+        InputStream contentInputStream = new ByteArrayInputStream(TEMPLATE_WITH_XSS_PLAIN.bytes)
+
+        HttpServletRequest mockRequest = [:] as HttpServletRequest
+        UriInfo mockUriInfo = [:] as UriInfo
+        String groupId = "1"
+
+        // Build a malformed template object which can be unmarshalled from XML
+
+        // Act
+
+        // Try to submit the malformed template
+        Response response = pgResource.uploadTemplate(mockRequest, mockUriInfo, groupId, false, contentInputStream)
+        logger.info("Response: ${response}")
+
+        // Assert
+
+        // Assert that the expected error response was returned
+        assert response.status == Response.Status.OK.statusCode
+
+        // Assert that the error response is sanitized
+        String responseEntity = response.entity as String
+        logger.info("Error response: ${responseEntity}")
+        assert !(responseEntity =~ /<script.*>/)
+    }
+
+    /** This test creates a malformed template import request to exercise error handling and sanitization */
+    @Test
+    void testImportShouldHandleMalformedTemplate() {
+        // Arrange
+        ProcessGroupResource pgResource = new ProcessGroupResource()
+
+        // Configure parent fields for write lock process
+        pgResource.properties = [isNode: { -> return false }] as NiFiProperties
+        pgResource.serviceFacade = [
+                authorizeAccess     : { AuthorizeAccess a -> },
+                verifyCanAddTemplate: { String gid, String templateName -> },
+                importTemplate      : { TemplateDTO template, String gid, Optional<String> seedId ->
+                    logger.mock("Called importTemplate;")
+                    template
+                }
+        ] as NiFiServiceFacade
+        pgResource.templateResource = [
+                populateRemainingTemplateContent: { TemplateDTO td -> }
+        ] as TemplateResource
+
+        final String TEMPLATE_WITH_XSS_PLAIN = "<?xml version=\"1.0\" encoding='><script xmlns=\"http://www.w3.org/1999/xhtml\">alert(JSON.stringify(localstorage));</script><errorResponse test='?>"
+        logger.info("Malformed template XML: ${TEMPLATE_WITH_XSS_PLAIN}")
+
+        TemplateDTO mockIAETemplate = [
+                getName   : { -> "mockIAETemplate" },
+                getUri    : { ->
+                    throw new IllegalArgumentException("Expected exception with <script> element")
+                },
+                getSnippet: { -> new FlowSnippetDTO() }
+        ] as TemplateDTO
+
+        TemplateDTO mockExceptionTemplate = [
+                getName   : { -> "mockExceptionTemplate" },
+                getUri    : { ->
+                    throw new RuntimeException("Expected exception with <script> element")
+                },
+                getSnippet: { -> new FlowSnippetDTO() }
+        ] as TemplateDTO
+
+        TemplateEntity mockIAETemplateEntity = [getTemplate: { ->
+            mockIAETemplate
+        }] as TemplateEntity
+
+        TemplateEntity mockExceptionTemplateEntity = [getTemplate: { ->
+            mockExceptionTemplate
+        }] as TemplateEntity
+
+        // Override the request object and store it for ApplicationResource#withWriteLock
+        HttpServletRequest mockRequest = [getHeader: { String headerName ->
+            logger.mock("Requesting header ${headerName}; returning null")
+            null
+        }] as HttpServletRequest
+
+        // Set the persisted request object so the parent ApplicationResource can use it
+        pgResource.httpServletRequest = mockRequest
+        String groupId = "1"
+
+        // Act
+        List<Response> responses = [mockIAETemplateEntity, mockExceptionTemplateEntity].collect { TemplateEntity te ->
+            // Try to submit the malformed template which throws some kind of exception
+            Response response = pgResource.importTemplate(mockRequest, groupId, te)
+            logger.info("Response: ${response}")
+            response
+        }
+
+        // Assert
+        responses.each { Response r ->
+            // Assert that the expected error response was returned
+            assert r.status == Response.Status.OK.statusCode
+
+            // Assert that the error response is sanitized
+            String entity = r.entity as String
+            logger.info("Error response: ${entity}")
+            assert !(entity =~ /<script.*>/)
+        }
+    }
+}