Merge pull request apache#648 from atlassian/WW-5270-forwarding-from-…

…excluded-url

WW-5270 Fix forwarding from Struts excluded URL

apps/showcase/src/main/java/org/apache/struts2/showcase/servlet/TestServlet.java

@@ -0,0 +1,41 @@
+/*
+ * $Id$
+ *
+ * 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.struts2.showcase.servlet;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+public class TestServlet extends HttpServlet {
+    @Override
+    public void service(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
+        switch (request.getPathInfo()) {
+            case "/forward":
+                getServletContext().getRequestDispatcher("/dispatcher/dispatch.action").forward(request, response);
+                break;
+            default:
+                response.sendError(404);
+                break;
+        }
+    }
+}

apps/showcase/src/main/resources/struts.xml

@@ -44,7 +44,7 @@
     <constant name="struts.serve.static" value="true" />
     <constant name="struts.serve.static.browserCache" value="false" />
 
-    <constant name="struts.action.excludePattern" value=".*/images/.*\.gif,.*/img/.*\.gif,.*/styles/.*\.css,.*/js/.*\.js"/>
+    <constant name="struts.action.excludePattern" value=".*/images/.*\.gif,.*/img/.*\.gif,.*/styles/.*\.css,.*/js/.*\.js,/testServlet/.*"/>
 
     <include file="struts-interactive.xml" />
 

apps/showcase/src/main/webapp/WEB-INF/web.xml

@@ -142,6 +142,11 @@
         <load-on-startup>4</load-on-startup>
     </servlet>
 
+    <servlet>
+        <servlet-name>testServlet</servlet-name>
+        <servlet-class>org.apache.struts2.showcase.servlet.TestServlet</servlet-class>
+    </servlet>
+
     <servlet-mapping>
         <servlet-name>dwr</servlet-name>
         <url-pattern>/dwr/*</url-pattern>
@@ -162,6 +167,11 @@
         <url-pattern>/async/receiveNewMessages</url-pattern>
     </servlet-mapping>
 
+    <servlet-mapping>
+        <servlet-name>testServlet</servlet-name>
+        <url-pattern>/testServlet/*</url-pattern>
+    </servlet-mapping>
+
     <!-- END SNIPPET: dwr -->
 
     <!-- SNIPPET START: example.velocity.filter.chain

apps/showcase/src/test/java/it/org/apache/struts2/showcase/DispatcherResultTest.java

@@ -32,15 +32,13 @@ public void testDispatchingToJSP() throws Exception {
             final HtmlPage page = webClient.getPage(ParameterUtils.getBaseUrl() + "/dispatcher/dispatch.action");
 
             DomElement div = page.getElementById("dispatcher-result");
-
             Assert.assertEquals("This page is a result of \"dispatching\" to it from an action", div.asNormalizedText());
         }
     }
 
     @Test
     public void testDispatchingToAction() throws Exception {
         try (final WebClient webClient = new WebClient()) {
-            webClient.getOptions().setThrowExceptionOnFailingStatusCode(false);
             final HtmlPage page = webClient.getPage(ParameterUtils.getBaseUrl() + "/dispatcher/forward.action");
 
             DomElement div = page.getElementById("dispatcher-result");

apps/showcase/src/test/java/it/org/apache/struts2/showcase/ForwardTest.java

@@ -0,0 +1,41 @@
+/*
+ * $Id$
+ *
+ * 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 it.org.apache.struts2.showcase;
+
+import com.gargoylesoftware.htmlunit.WebClient;
+import com.gargoylesoftware.htmlunit.html.DomElement;
+import com.gargoylesoftware.htmlunit.html.HtmlPage;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class ForwardTest {
+
+    @Test
+    public void testServletForwardingToAction() throws Exception {
+        try (final WebClient webClient = new WebClient()) {
+            // Struts excluded URL, as defined by struts.action.excludePattern
+            final HtmlPage page = webClient.getPage(ParameterUtils.getBaseUrl() + "/testServlet/forward");
+
+            DomElement div = page.getElementById("dispatcher-result");
+            Assert.assertEquals("This page is a result of \"dispatching\" to it from an action", div.asNormalizedText());
+        }
+    }
+}

core/src/main/java/org/apache/struts2/dispatcher/PrepareOperations.java

@@ -47,18 +47,43 @@ public class PrepareOperations {
     /**
      * Maintains per-request override of devMode configuration.
      */
-    private static ThreadLocal<Boolean> devModeOverride = new InheritableThreadLocal<>();
+    private static final ThreadLocal<Boolean> devModeOverride = new InheritableThreadLocal<>();
 
 
-    private Dispatcher dispatcher;
+    private final Dispatcher dispatcher;
     private static final String STRUTS_ACTION_MAPPING_KEY = "struts.actionMapping";
     private static final String NO_ACTION_MAPPING = "noActionMapping";
-    public static final String CLEANUP_RECURSION_COUNTER = "__cleanup_recursion_counter";
+    private static final String PREPARE_COUNTER = "__prepare_recursion_counter";
+    private static final String WRAP_COUNTER = "__wrap_recursion_counter";
 
     public PrepareOperations(Dispatcher dispatcher) {
         this.dispatcher = dispatcher;
     }
 
+    /**
+     * Should be called by {@link org.apache.struts2.dispatcher.filter.StrutsPrepareFilter} to track how many times this
+     * request has been filtered.
+     */
+    public void trackRecursion(HttpServletRequest request) {
+        incrementRecursionCounter(request, PREPARE_COUNTER);
+    }
+
+    /**
+     * Cleans up request. When paired with {@link #trackRecursion}, only cleans up once the first filter instance has
+     * completed, preventing cleanup by recursive filter calls - i.e. before the request is completely processed.
+     */
+    public void cleanupRequest(final HttpServletRequest request) {
+        decrementRecursionCounter(request, PREPARE_COUNTER, () -> {
+            try {
+                dispatcher.cleanUpRequest(request);
+            } finally {
+                ActionContext.clear();
+                Dispatcher.setInstance(null);
+                devModeOverride.remove();
+            }
+        });
+    }
+
     /**
      * Creates the action context and initializes the thread local
      *
@@ -69,12 +94,6 @@ public PrepareOperations(Dispatcher dispatcher) {
      */
     public ActionContext createActionContext(HttpServletRequest request, HttpServletResponse response) {
         ActionContext ctx;
-        int counter = 1;
-        Integer oldCounter = (Integer) request.getAttribute(CLEANUP_RECURSION_COUNTER);
-        if (oldCounter != null) {
-            counter = oldCounter + 1;
-        }
-
         ActionContext oldContext = ActionContext.getContext();
         if (oldContext != null) {
             // detected existing context, so we are probably in a forward
@@ -87,35 +106,9 @@ public ActionContext createActionContext(HttpServletRequest request, HttpServlet
                 ctx = ActionContext.of(stack.getContext()).bind();
             }
         }
-        request.setAttribute(CLEANUP_RECURSION_COUNTER, counter);
         return ctx;
     }
 
-    /**
-     * Cleans up a request of thread locals
-     *
-     * @param request servlet request
-     */
-    public void cleanupRequest(HttpServletRequest request) {
-        Integer counterVal = (Integer) request.getAttribute(CLEANUP_RECURSION_COUNTER);
-        if (counterVal != null) {
-            counterVal -= 1;
-            request.setAttribute(CLEANUP_RECURSION_COUNTER, counterVal);
-            if (counterVal > 0 ) {
-                LOG.debug("skipping cleanup counter={}", counterVal);
-                return;
-            }
-        }
-        // always clean up the thread request, even if an action hasn't been executed
-        try {
-            dispatcher.cleanUpRequest(request);
-        } finally {
-            ActionContext.clear();
-            Dispatcher.setInstance(null);
-            devModeOverride.remove();
-        }
-    }
-
     /**
      * Assigns the dispatcher to the dispatcher thread local
      */
@@ -135,14 +128,15 @@ public void setEncodingAndLocale(HttpServletRequest request, HttpServletResponse
 
     /**
      * Wraps the request with the Struts wrapper that handles multipart requests better
+     * Also tracks additional calls to this method on the same request.
      *
-     * @param oldRequest servlet request
+     * @param request servlet request
      *
      * @return The new request, if there is one
      * @throws ServletException on any servlet related error
      */
-    public HttpServletRequest wrapRequest(HttpServletRequest oldRequest) throws ServletException {
-        HttpServletRequest request = oldRequest;
+    public HttpServletRequest wrapRequest(HttpServletRequest request) throws ServletException {
+        incrementRecursionCounter(request, WRAP_COUNTER);
         try {
             // Wrap request first, just in case it is multipart/form-data
             // parameters might not be accessible through before encoding (ww-1278)
@@ -154,6 +148,14 @@ public HttpServletRequest wrapRequest(HttpServletRequest oldRequest) throws Serv
         return request;
     }
 
+    /**
+     * Should be called after whenever {@link #wrapRequest} is called. Ensures the request is only cleaned up at the
+     * instance it was initially wrapped in the case of multiple wrap calls - i.e. filter recursion.
+     */
+    public void cleanupWrappedRequest(final HttpServletRequest request) {
+        decrementRecursionCounter(request, WRAP_COUNTER, () -> dispatcher.cleanUpRequest(request));
+    }
+
     /**
      * Finds and optionally creates an {@link ActionMapping}.  It first looks in the current request to see if one
      * has already been found, otherwise, it creates it and stores it in the request.  No mapping will be created in the
@@ -260,7 +262,6 @@ public static Boolean getDevModeOverride()
 
     /**
      * Clear any override of the static devMode value being applied to the current thread.
-     *
      * This can be useful for any situation where {@link #overrideDevMode(boolean)} might be called
      * in a flow where {@link #cleanupRequest(javax.servlet.http.HttpServletRequest)} does not get called.
      * May be very situational (such as some unit tests), but may have other utility as well.
@@ -269,4 +270,30 @@ public static void clearDevModeOverride() {
         devModeOverride.remove();  // Remove current thread's value, enxure next read returns it to initialValue (typically null).
     }
 
+    /**
+     * Helper method to potentially count recursive executions with a request attribute. Should be used in conjunction
+     * with {@link #decrementRecursionCounter}.
+     */
+    public static void incrementRecursionCounter(HttpServletRequest request, String attributeName) {
+        Integer setCounter = (Integer) request.getAttribute(attributeName);
+        if (setCounter == null) {
+            setCounter = 0;
+        }
+        request.setAttribute(attributeName, ++setCounter);
+    }
+
+    /**
+     * Helper method to count execution completions with a request attribute, and optionally execute some code
+     * (e.g. cleanup) once all recursive executions have completed. Should be used in conjunction with
+     * {@link #incrementRecursionCounter}.
+     */
+    public static void decrementRecursionCounter(HttpServletRequest request, String attributeName, Runnable runnable) {
+        Integer setCounter = (Integer) request.getAttribute(attributeName);
+        if (setCounter != null) {
+            request.setAttribute(attributeName, --setCounter);
+        }
+        if ((setCounter == null || setCounter == 0) && runnable != null) {
+            runnable.run();
+        }
+    }
 }

core/src/main/java/org/apache/struts2/dispatcher/filter/StrutsExecuteFilter.java

@@ -81,7 +81,7 @@ public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain)
     }
 
     private boolean excludeUrl(HttpServletRequest request) {
-        return request.getAttribute(StrutsPrepareFilter.REQUEST_EXCLUDED_FROM_ACTION_MAPPING) != null;
+        return Boolean.TRUE.equals(request.getAttribute(StrutsPrepareFilter.REQUEST_EXCLUDED_FROM_ACTION_MAPPING));
     }
 
     public void destroy() {

core/src/main/java/org/apache/struts2/dispatcher/filter/StrutsPrepareAndExecuteFilter.java

@@ -118,6 +118,7 @@ public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain)
         HttpServletResponse response = (HttpServletResponse) res;
 
         try {
+            prepare.trackRecursion(request);
             String uri = RequestUtils.getUri(request);
             if (isRequestExcluded(request)) {
                 LOG.trace("Request: {} is excluded from handling by Struts, passing request to other filters", uri);
@@ -155,7 +156,7 @@ private void handleRequest(FilterChain chain, HttpServletRequest request, HttpSe
                 execute.executeAction(wrappedRequest, response, mapping);
             }
         } finally {
-            prepare.cleanupRequest(wrappedRequest);
+            prepare.cleanupWrappedRequest(wrappedRequest);
         }
     }
 

core/src/main/java/org/apache/struts2/dispatcher/filter/StrutsPrepareFilter.java

@@ -78,18 +78,25 @@ public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain)
         HttpServletRequest request = (HttpServletRequest) req;
         HttpServletResponse response = (HttpServletResponse) res;
 
+        boolean didWrap = false;
         try {
+            prepare.trackRecursion(request);
             if (excludedPatterns != null && prepare.isUrlExcluded(request, excludedPatterns)) {
-                request.setAttribute(REQUEST_EXCLUDED_FROM_ACTION_MAPPING, new Object());
+                request.setAttribute(REQUEST_EXCLUDED_FROM_ACTION_MAPPING, true);
             } else {
+                request.setAttribute(REQUEST_EXCLUDED_FROM_ACTION_MAPPING, false);
                 prepare.setEncodingAndLocale(request, response);
                 prepare.createActionContext(request, response);
                 prepare.assignDispatcherToThread();
                 request = prepare.wrapRequest(request);
+                didWrap = true;
                 prepare.findActionMapping(request, response, true);
             }
             chain.doFilter(request, response);
         } finally {
+            if (didWrap) {
+                prepare.cleanupWrappedRequest(request);
+            }
             prepare.cleanupRequest(request);
         }
     }

core/src/main/java/org/apache/struts2/dispatcher/servlet/StrutsServlet.java

@@ -19,11 +19,10 @@
 package org.apache.struts2.dispatcher.servlet;
 
 import org.apache.struts2.dispatcher.Dispatcher;
-import org.apache.struts2.dispatcher.mapper.ActionMapping;
 import org.apache.struts2.dispatcher.ExecuteOperations;
 import org.apache.struts2.dispatcher.InitOperations;
 import org.apache.struts2.dispatcher.PrepareOperations;
-import org.apache.struts2.dispatcher.servlet.ServletHostConfig;
+import org.apache.struts2.dispatcher.mapper.ActionMapping;
 
 import javax.servlet.ServletConfig;
 import javax.servlet.ServletException;