Separate submitted and response salts in UI filters

app/src/main/java/org/apache/roller/weblogger/ui/core/filters/SaltValidator.java

@@ -0,0 +1,108 @@
+/*
+ * 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.  For additional information regarding
+ * copyright in this work, please see the NOTICE file in the top level
+ * directory of this distribution.
+ */
+
+package org.apache.roller.weblogger.ui.core.filters;
+
+import java.util.Locale;
+import java.util.Objects;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+
+import org.apache.roller.weblogger.ui.core.RollerSession;
+import org.apache.roller.weblogger.ui.rendering.util.cache.SaltCache;
+
+/**
+ * Shared validation for salts submitted by UI forms.
+ */
+public final class SaltValidator {
+
+    private static final String MULTIPART_FORM_DATA = "multipart/form-data";
+
+    public static final String VALIDATED_REQUEST_ATTRIBUTE =
+            SaltValidator.class.getName() + ".validated";
+
+    private SaltValidator() {
+    }
+
+    /**
+     * Validates and consumes the salt submitted as a request parameter.
+     *
+     * @param request current request
+     * @return true when no Roller session is present or the submitted salt is valid
+     */
+    public static boolean consumeSubmittedSalt(HttpServletRequest request) {
+        RollerSession rollerSession = RollerSession.getRollerSession(request);
+        if (rollerSession == null) {
+            return true;
+        }
+
+        String userId = rollerSession.getAuthenticatedUser() != null
+                ? rollerSession.getAuthenticatedUser().getId() : "";
+        String salt = request.getParameter("salt");
+        if (salt == null) {
+            return false;
+        }
+
+        SaltCache saltCache = SaltCache.getInstance();
+        synchronized (saltCache) {
+            if (!Objects.equals(saltCache.get(salt), userId)) {
+                return false;
+            }
+            saltCache.remove(salt);
+        }
+        return true;
+    }
+
+    /**
+     * Validates and consumes the submitted salt or rejects the request.
+     *
+     * @param request current request
+     * @throws ServletException when the submitted salt is missing or invalid
+     */
+    public static void requireSubmittedSalt(HttpServletRequest request)
+            throws ServletException {
+        if (!consumeSubmittedSalt(request)) {
+            throw new ServletException("Security Violation");
+        }
+    }
+
+    /**
+     * Returns true for a multipart form POST, which Struts parses after the
+     * servlet filters have run.
+     *
+     * @param request current request
+     * @return true for multipart/form-data POST requests
+     */
+    public static boolean isMultipartFormPost(HttpServletRequest request) {
+        if (!"POST".equalsIgnoreCase(request.getMethod())) {
+            return false;
+        }
+
+        String contentType = request.getContentType();
+        if (contentType == null) {
+            return false;
+        }
+
+        int parameterStart = contentType.indexOf(';');
+        String mediaType = parameterStart >= 0
+                ? contentType.substring(0, parameterStart) : contentType;
+        return MULTIPART_FORM_DATA.equals(mediaType.trim().toLowerCase(Locale.ENGLISH));
+    }
+}

app/src/main/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilter.java

@@ -19,9 +19,6 @@
 package org.apache.roller.weblogger.ui.core.filters;
 
 import java.io.IOException;
-import java.util.Collections;
-import java.util.Objects;
-import java.util.Set;
 
 import javax.servlet.Filter;
 import javax.servlet.FilterChain;
@@ -31,53 +28,37 @@
 import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
 
-import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-import org.apache.roller.weblogger.config.WebloggerConfig;
-import org.apache.roller.weblogger.ui.rendering.util.cache.SaltCache;
-import org.apache.roller.weblogger.ui.core.RollerSession;
 
 /**
  * Filter checks all POST request for presence of valid salt value and rejects those without
  * a salt value or with a salt value not generated by this Roller instance.
  */
 public class ValidateSaltFilter implements Filter {
     private static final Log log = LogFactory.getLog(ValidateSaltFilter.class);
-    private Set<String> ignored = Collections.emptySet();
 
     @Override
     public void doFilter(ServletRequest request, ServletResponse response,
             FilterChain chain) throws IOException, ServletException {
         HttpServletRequest httpReq = (HttpServletRequest) request;
 
-        String requestURL = httpReq.getRequestURL().toString();
-        String queryString = httpReq.getQueryString();
-        if (queryString != null) {
-            requestURL += "?" + queryString;
-        }
-
-        if ("POST".equals(httpReq.getMethod()) && !isIgnoredURL(requestURL)) {
-            RollerSession rollerSession = RollerSession.getRollerSession(httpReq);
-            if (rollerSession != null) {
-                String userId = rollerSession.getAuthenticatedUser() != null ? rollerSession.getAuthenticatedUser().getId() : "";
-
-                Object saltObject = httpReq.getAttribute("salt"); // multi-form post case
-                String salt = saltObject != null ? saltObject.toString() : null;
-                salt = salt != null ? salt : httpReq.getParameter("salt");
-                SaltCache saltCache = SaltCache.getInstance();
-                if (salt == null || !Objects.equals(saltCache.get(salt), userId)) {
-                    if (log.isDebugEnabled()) {
-                        log.debug("Valid salt value not found on POST to URL : " + httpReq.getServletPath());
-                    }
-                    throw new ServletException("Security Violation");
-                }
+        if ("POST".equalsIgnoreCase(httpReq.getMethod())) {
+            if (SaltValidator.isMultipartFormPost(httpReq) && isStrutsAction(httpReq)) {
+                // Struts wraps multipart requests before its interceptor stack;
+                // ValidateSaltInterceptor handles these requests.
+                chain.doFilter(request, response);
+                return;
+            }
 
-                // Remove salt from cache after successful validation
-                saltCache.remove(salt);
+            try {
+                SaltValidator.requireSubmittedSalt(httpReq);
+            } catch (ServletException e) {
                 if (log.isDebugEnabled()) {
-                    log.debug("Salt used and invalidated: " + salt);
+                    log.debug("Valid salt value not found on POST to URL : "
+                            + httpReq.getServletPath());
                 }
+                throw e;
             }
         }
 
@@ -86,20 +67,14 @@ public void doFilter(ServletRequest request, ServletResponse response,
 
     @Override
     public void init(FilterConfig filterConfig) throws ServletException {
-        String urls = WebloggerConfig.getProperty("salt.ignored.urls");
-        ignored = Set.of(StringUtils.stripAll(StringUtils.split(urls, ",")));
     }
 
     @Override
     public void destroy() {
     }
 
-    /**
-     * Checks if this is an ignored url defined in the salt.ignored.urls property
-     * @param theUrl the url
-     * @return true, if is ignored resource
-     */
-    private boolean isIgnoredURL(String theUrl) {
-        return ignored.contains(theUrl);
+    private boolean isStrutsAction(HttpServletRequest request) {
+        String servletPath = request.getServletPath();
+        return servletPath != null && servletPath.endsWith(".rol");
     }
-}
+}

app/src/main/java/org/apache/roller/weblogger/ui/struts2/ajax/CommentDataServlet.java

@@ -98,7 +98,8 @@ public void doPut(HttpServletRequest request,
             WeblogEntryManager wmgr = roller.getWeblogEntryManager();
             WeblogEntryComment c = wmgr.getComment(request.getParameter("id"));
             if (c == null) {
-                response.setStatus(HttpServletResponse.SC_NOT_FOUND);
+                writeSaveResponse(request, response,
+                        HttpServletResponse.SC_NOT_FOUND, null, null);
             } else {
                 // need post permission to edit comments
                 RollerSession rses = RollerSession.getRollerSession(request);
@@ -114,17 +115,11 @@ public void doPut(HttpServletRequest request,
                     c = wmgr.getComment(request.getParameter("id"));
                     content = Utilities.escapeHTML(c.getContent());
                     content = WordUtils.wrap(content, 72);
-                    content = StringEscapeUtils.escapeEcmaScript(content);
-                    String json = "{ id: \"" + c.getId() + "\"," + "content: \"" + content + "\" }";
-                    response.setStatus(HttpServletResponse.SC_OK);
-                    response.setContentType("text/html; charset=utf-8");
-                    response.getWriter().print(json);
-                    response.flushBuffer();
-                    response.getWriter().flush();
-                    response.getWriter().close();
-                    response.setStatus(HttpServletResponse.SC_OK);
+                    writeSaveResponse(request, response,
+                            HttpServletResponse.SC_OK, c.getId(), content);
                 } else {
-                    response.setStatus(HttpServletResponse.SC_FORBIDDEN);
+                    writeSaveResponse(request, response,
+                            HttpServletResponse.SC_FORBIDDEN, null, null);
                 }
             }
 
@@ -140,4 +135,32 @@ public void doPost(HttpServletRequest request,
         // not all browsers support PUT
         doPut(request, response);
     }
+
+    private void writeSaveResponse(HttpServletRequest request,
+            HttpServletResponse response, int status, String id, String content)
+            throws IOException {
+        String salt = request.getAttribute("salt") != null
+                ? request.getAttribute("salt").toString() : "";
+        StringBuilder json = new StringBuilder("{");
+        if (id != null) {
+            json.append("\"id\":\"")
+                    .append(StringEscapeUtils.escapeJson(id))
+                    .append("\",");
+        }
+        if (content != null) {
+            json.append("\"content\":\"")
+                    .append(StringEscapeUtils.escapeJson(content))
+                    .append("\",");
+        }
+        json.append("\"salt\":\"")
+                .append(StringEscapeUtils.escapeJson(salt))
+                .append("\"}");
+
+        response.setStatus(status);
+        response.setContentType("application/json; charset=utf-8");
+        response.getWriter().print(json.toString());
+        response.flushBuffer();
+        response.getWriter().flush();
+        response.getWriter().close();
+    }
 }

app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptor.java

@@ -0,0 +1,71 @@
+/*
+ * 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.  For additional information regarding
+ * copyright in this work, please see the NOTICE file in the top level
+ * directory of this distribution.
+ */
+
+package org.apache.roller.weblogger.ui.struts2.util;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.roller.weblogger.ui.core.filters.SaltValidator;
+import org.apache.struts2.StrutsStatics;
+import org.apache.struts2.dispatcher.multipart.MultiPartRequestWrapper;
+
+import com.opensymphony.xwork2.ActionContext;
+import com.opensymphony.xwork2.ActionInvocation;
+import com.opensymphony.xwork2.interceptor.AbstractInterceptor;
+
+/**
+ * Validates salts after Struts has wrapped a multipart form request.
+ */
+public class ValidateSaltInterceptor extends AbstractInterceptor implements StrutsStatics {
+
+    private static final long serialVersionUID = 2446434402795510394L;
+    private static final Log log = LogFactory.getLog(ValidateSaltInterceptor.class);
+
+    @Override
+    public String intercept(ActionInvocation invocation) throws Exception {
+        ActionContext context = invocation.getInvocationContext();
+        HttpServletRequest request = (HttpServletRequest) context.get(HTTP_REQUEST);
+
+        if (SaltValidator.isMultipartFormPost(request)
+                && !Boolean.TRUE.equals(request.getAttribute(
+                        SaltValidator.VALIDATED_REQUEST_ATTRIBUTE))) {
+            if (request instanceof MultiPartRequestWrapper
+                    && ((MultiPartRequestWrapper) request).hasErrors()) {
+                return invocation.invoke();
+            }
+
+            try {
+                SaltValidator.requireSubmittedSalt(request);
+            } catch (ServletException e) {
+                if (log.isDebugEnabled()) {
+                    log.debug("Valid salt value not found on multipart POST to URL : "
+                            + request.getServletPath());
+                }
+                throw e;
+            }
+            request.setAttribute(
+                    SaltValidator.VALIDATED_REQUEST_ATTRIBUTE, Boolean.TRUE);
+        }
+
+        return invocation.invoke();
+    }
+}

app/src/main/resources/org/apache/roller/weblogger/config/roller.properties

@@ -388,9 +388,6 @@ schemeenforcement.https.urls=/roller_j_security_check,\
 # Ignored extensions otherwise we get SSL mixed content issues
 schemeenforcement.https.ignored=css,gif,png,js
 
-# Ignored urls for salt.  These are for multipart/form-data submissions as we do not get any parameters
-salt.ignored.urls=mediaFileAdd!save.rol,mediaFileEdit!save.rol,bookmarksImport!save.rol
-
 #---------------------------------------------------------------------
 # LDAP authentication properties -- valid only if LDAP authentication
 # authentication.method via authentication.method setting.

app/src/main/resources/struts.xml

@@ -37,6 +37,8 @@
                          class="org.apache.roller.weblogger.ui.struts2.util.UISecurityInterceptor" />
             <interceptor name="UIActionPrepareInterceptor" 
                          class="org.apache.roller.weblogger.ui.struts2.util.UIActionPrepareInterceptor" />
+            <interceptor name="ValidateSaltInterceptor"
+                         class="org.apache.roller.weblogger.ui.struts2.util.ValidateSaltInterceptor" />
 
             <!-- Define a custom interceptor stack for Roller so that we can 
                  add in our own custom interceptors.  We basically copy the 
@@ -45,6 +47,7 @@
                  default stack -->
             <interceptor-stack name="rollerStack">
                 <interceptor-ref name="exception"/>
+                <interceptor-ref name="ValidateSaltInterceptor"/>
                 <!-- <interceptor-ref name="alias"/> -->
                 <interceptor-ref name="servletConfig"/>
                 <!-- <interceptor-ref name="prepare"/> -->