From 5a71d10a8ddae50dbf812dde23f7d202a55475c8 Mon Sep 17 00:00:00 2001 From: "David M. Johnson" Date: Sun, 16 Aug 2026 08:14:26 -0400 Subject: [PATCH 1/2] Separate submitted and response salts in UI filters Validate the salt submitted with the request rather than a request attribute, and run validation before the response salt is generated. Move multipart validation into a Struts interceptor after the upload interceptor, since filters cannot read multipart fields. Drop the unused salt.ignored.urls bypass. Claude-Session: https://claude.ai/code/session_01A1fhY1E2PCFU6UAPXu2WtV --- .../ui/core/filters/SaltValidator.java | 91 +++++++++ .../ui/core/filters/ValidateSaltFilter.java | 57 ++---- .../struts2/util/ValidateSaltInterceptor.java | 58 ++++++ .../roller/weblogger/config/roller.properties | 3 - app/src/main/resources/struts.xml | 3 + app/src/main/webapp/WEB-INF/web.xml | 8 +- .../core/filters/SaltConfigurationTest.java | 85 +++++++++ .../core/filters/ValidateSaltFilterTest.java | 119 +++++++++--- .../util/ValidateSaltInterceptorTest.java | 173 ++++++++++++++++++ 9 files changed, 520 insertions(+), 77 deletions(-) create mode 100644 app/src/main/java/org/apache/roller/weblogger/ui/core/filters/SaltValidator.java create mode 100644 app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptor.java create mode 100644 app/src/test/java/org/apache/roller/weblogger/ui/core/filters/SaltConfigurationTest.java create mode 100644 app/src/test/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptorTest.java diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/SaltValidator.java b/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/SaltValidator.java new file mode 100644 index 0000000000..dbe8abd594 --- /dev/null +++ b/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/SaltValidator.java @@ -0,0 +1,91 @@ +/* + * 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.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"; + + 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; + } + + /** + * 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)); + } +} diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilter.java b/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilter.java index 586bff185b..f1a90e156d 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilter.java +++ b/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,12 +28,8 @@ 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 @@ -44,40 +37,26 @@ */ public class ValidateSaltFilter implements Filter { private static final Log log = LogFactory.getLog(ValidateSaltFilter.class); - private Set 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 makes multipart parameters available after its upload + // interceptor; ValidateSaltInterceptor handles these requests. + chain.doFilter(request, response); + return; + } - // Remove salt from cache after successful validation - saltCache.remove(salt); + if (!SaltValidator.consumeSubmittedSalt(httpReq)) { if (log.isDebugEnabled()) { - log.debug("Salt used and invalidated: " + salt); + log.debug("Valid salt value not found on POST to URL : " + + httpReq.getServletPath()); } + throw new ServletException("Security Violation"); } } @@ -86,20 +65,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"); } -} \ No newline at end of file +} diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptor.java b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptor.java new file mode 100644 index 0000000000..b64f103d6a --- /dev/null +++ b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptor.java @@ -0,0 +1,58 @@ +/* + * 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 com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.ActionInvocation; +import com.opensymphony.xwork2.interceptor.AbstractInterceptor; + +/** + * Validates salts after Struts has parsed 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) + && !SaltValidator.consumeSubmittedSalt(request)) { + if (log.isDebugEnabled()) { + log.debug("Valid salt value not found on multipart POST to URL : " + + request.getServletPath()); + } + throw new ServletException("Security Violation"); + } + + return invocation.invoke(); + } +} diff --git a/app/src/main/resources/org/apache/roller/weblogger/config/roller.properties b/app/src/main/resources/org/apache/roller/weblogger/config/roller.properties index d73e7f9ca1..b71a48689c 100644 --- a/app/src/main/resources/org/apache/roller/weblogger/config/roller.properties +++ b/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. diff --git a/app/src/main/resources/struts.xml b/app/src/main/resources/struts.xml index cc94ba6588..51763b1b50 100644 --- a/app/src/main/resources/struts.xml +++ b/app/src/main/resources/struts.xml @@ -37,6 +37,8 @@ class="org.apache.roller.weblogger.ui.struts2.util.UISecurityInterceptor" /> + + diff --git a/app/src/main/webapp/WEB-INF/web.xml b/app/src/main/webapp/WEB-INF/web.xml index 0418832da1..746d20d267 100644 --- a/app/src/main/webapp/WEB-INF/web.xml +++ b/app/src/main/webapp/WEB-INF/web.xml @@ -142,15 +142,15 @@ - LoadSaltFilter + ValidateSaltFilter /roller-ui/* - REQUEST - FORWARD - ValidateSaltFilter + LoadSaltFilter /roller-ui/* + REQUEST + FORWARD diff --git a/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/SaltConfigurationTest.java b/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/SaltConfigurationTest.java new file mode 100644 index 0000000000..1c3a9431b8 --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/SaltConfigurationTest.java @@ -0,0 +1,85 @@ +/* + * 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.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class SaltConfigurationTest { + + @Test + public void testSubmittedSaltIsValidatedBeforeResponseSaltIsLoaded() throws Exception { + String webXml = Files.readString(Path.of("src/main/webapp/WEB-INF/web.xml")); + + int validateMapping = filterMappingPosition(webXml, "ValidateSaltFilter"); + int loadMapping = filterMappingPosition(webXml, "LoadSaltFilter"); + + assertTrue(validateMapping >= 0, "ValidateSaltFilter mapping is missing"); + assertTrue(loadMapping >= 0, "LoadSaltFilter mapping is missing"); + assertTrue(validateMapping < loadMapping, + "ValidateSaltFilter must run before LoadSaltFilter"); + } + + @Test + public void testMultipartSaltValidationImmediatelyFollowsUploadInterceptor() throws Exception { + String strutsXml = readResource("/struts.xml"); + + Pattern adjacentInterceptors = Pattern.compile( + "\\s*" + + ""); + + assertTrue(adjacentInterceptors.matcher(strutsXml).find(), + "ValidateSaltInterceptor must immediately follow the upload interceptor"); + } + + @Test + public void testConfigurableSaltBypassIsRemoved() throws Exception { + String properties = readResource( + "/org/apache/roller/weblogger/config/roller.properties"); + + assertFalse(properties.contains("salt.ignored.urls")); + } + + private int filterMappingPosition(String webXml, String filterName) { + Pattern pattern = Pattern.compile("\\s*" + + Pattern.quote(filterName) + ""); + Matcher matcher = pattern.matcher(webXml); + return matcher.find() ? matcher.start() : -1; + } + + private String readResource(String path) throws IOException { + try (InputStream stream = SaltConfigurationTest.class.getResourceAsStream(path)) { + if (stream == null) { + throw new IOException("Test resource not found: " + path); + } + return new String(stream.readAllBytes(), StandardCharsets.UTF_8); + } + } +} diff --git a/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilterTest.java b/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilterTest.java index ab866d080a..fcc5226f1d 100644 --- a/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilterTest.java +++ b/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilterTest.java @@ -1,17 +1,16 @@ package org.apache.roller.weblogger.ui.core.filters; -import org.apache.roller.weblogger.config.WebloggerConfig; import org.apache.roller.weblogger.pojos.User; import org.apache.roller.weblogger.ui.core.RollerSession; import org.apache.roller.weblogger.ui.rendering.util.cache.SaltCache; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.MockedStatic; import org.mockito.MockitoAnnotations; import javax.servlet.FilterChain; -import javax.servlet.FilterConfig; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -47,8 +46,6 @@ public void setUp() { @Test public void testDoFilterWithGetMethod() throws Exception { when(request.getMethod()).thenReturn("GET"); - StringBuffer requestURL = new StringBuffer("https://example.com/app/ignoredurl"); - when(request.getRequestURL()).thenReturn(requestURL); filter.doFilter(request, response, chain); @@ -67,9 +64,6 @@ public void testDoFilterWithPostMethodAndValidSalt() throws Exception { when(request.getParameter("salt")).thenReturn("validSalt"); when(saltCache.get("validSalt")).thenReturn("userId"); when(rollerSession.getAuthenticatedUser()).thenReturn(new TestUser("userId")); - StringBuffer requestURL = new StringBuffer("https://example.com/app/ignoredurl"); - when(request.getRequestURL()).thenReturn(requestURL); - filter.doFilter(request, response, chain); verify(chain).doFilter(request, response); @@ -88,9 +82,6 @@ public void testDoFilterWithPostMethodAndInvalidSalt() throws Exception { when(request.getMethod()).thenReturn("POST"); when(request.getParameter("salt")).thenReturn("invalidSalt"); when(saltCache.get("invalidSalt")).thenReturn(null); - StringBuffer requestURL = new StringBuffer("https://example.com/app/ignoredurl"); - when(request.getRequestURL()).thenReturn(requestURL); - assertThrows(ServletException.class, () -> { filter.doFilter(request, response, chain); }); @@ -109,9 +100,6 @@ public void testDoFilterWithPostMethodAndMismatchedUserId() throws Exception { when(request.getParameter("salt")).thenReturn("validSalt"); when(saltCache.get("validSalt")).thenReturn("differentUserId"); when(rollerSession.getAuthenticatedUser()).thenReturn(new TestUser("userId")); - StringBuffer requestURL = new StringBuffer("https://example.com/app/ignoredurl"); - when(request.getRequestURL()).thenReturn(requestURL); - assertThrows(ServletException.class, () -> { filter.doFilter(request, response, chain); }); @@ -129,9 +117,6 @@ public void testDoFilterWithPostMethodAndNullRollerSession() throws Exception { when(request.getMethod()).thenReturn("POST"); when(request.getParameter("salt")).thenReturn("validSalt"); when(saltCache.get("validSalt")).thenReturn(""); - StringBuffer requestURL = new StringBuffer("https://example.com/app/ignoredurl"); - when(request.getRequestURL()).thenReturn(requestURL); - filter.doFilter(request, response, chain); verify(saltCache, never()).remove("validSalt"); @@ -139,32 +124,110 @@ public void testDoFilterWithPostMethodAndNullRollerSession() throws Exception { } @Test - public void testDoFilterWithIgnoredURL() throws Exception { + public void testPostWithoutParameterRejectsRequestAttributeSalt() throws Exception { try (MockedStatic mockedRollerSession = mockStatic(RollerSession.class); - MockedStatic mockedSaltCache = mockStatic(SaltCache.class); - MockedStatic mockedWebloggerConfig = mockStatic(WebloggerConfig.class)) { + MockedStatic mockedSaltCache = mockStatic(SaltCache.class)) { mockedRollerSession.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); mockedSaltCache.when(SaltCache::getInstance).thenReturn(saltCache); - mockedWebloggerConfig.when(() -> WebloggerConfig.getProperty("salt.ignored.urls")) - .thenReturn("https://example.com/app/ignoredurl?param1=value1&m2=value2"); when(request.getMethod()).thenReturn("POST"); - StringBuffer requestURL = new StringBuffer("https://example.com/app/ignoredurl"); - when(request.getRequestURL()).thenReturn(requestURL); - when(request.getQueryString()).thenReturn("param1=value1&m2=value2"); - when(request.getParameter("salt")).thenReturn(null); // No salt provided + when(request.getAttribute("salt")).thenReturn("responseSalt"); + when(request.getParameter("salt")).thenReturn(null); + when(rollerSession.getAuthenticatedUser()).thenReturn(new TestUser("userId")); + when(saltCache.get("responseSalt")).thenReturn("userId"); - filter.init(mock(FilterConfig.class)); - filter.doFilter(request, response, chain); + assertThrows(ServletException.class, + () -> filter.doFilter(request, response, chain)); - verify(chain).doFilter(request, response); + verify(chain, never()).doFilter(request, response); verify(saltCache, never()).get(anyString()); verify(saltCache, never()).remove(anyString()); } } + @Test + public void testSubmittedSaltCanOnlyBeUsedOnce() throws Exception { + try (MockedStatic mockedRollerSession = mockStatic(RollerSession.class); + MockedStatic mockedSaltCache = mockStatic(SaltCache.class)) { + + mockedRollerSession.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + mockedSaltCache.when(SaltCache::getInstance).thenReturn(saltCache); + + when(request.getMethod()).thenReturn("POST"); + when(request.getParameter("salt")).thenReturn("validSalt"); + when(rollerSession.getAuthenticatedUser()).thenReturn(new TestUser("userId")); + when(saltCache.get("validSalt")).thenReturn("userId", (String) null); + + filter.doFilter(request, response, chain); + assertThrows(ServletException.class, + () -> filter.doFilter(request, response, chain)); + + verify(chain, times(1)).doFilter(request, response); + verify(saltCache, times(1)).remove("validSalt"); + } + } + + @Test + public void testMultipartStrutsPostIsDeferred() throws Exception { + when(request.getMethod()).thenReturn("POST"); + when(request.getContentType()).thenReturn("multipart/form-data; boundary=abc123"); + when(request.getServletPath()).thenReturn("/roller-ui/mediaFileAdd!save.rol"); + + filter.doFilter(request, response, chain); + + verify(chain).doFilter(request, response); + verify(request, never()).getParameter("salt"); + } + + @Test + public void testMultipartNonStrutsPostIsNotDeferred() throws Exception { + try (MockedStatic mockedRollerSession = mockStatic(RollerSession.class)) { + mockedRollerSession.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + + when(request.getMethod()).thenReturn("POST"); + when(request.getContentType()).thenReturn("multipart/form-data; boundary=abc123"); + when(request.getServletPath()).thenReturn("/roller-ui/upload"); + when(request.getParameter("salt")).thenReturn(null); + + assertThrows(ServletException.class, + () -> filter.doFilter(request, response, chain)); + + verify(chain, never()).doFilter(request, response); + } + } + + @Test + public void testValidationRunsBeforeResponseSaltGeneration() throws Exception { + try (MockedStatic mockedRollerSession = mockStatic(RollerSession.class); + MockedStatic mockedSaltCache = mockStatic(SaltCache.class)) { + + mockedRollerSession.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + mockedSaltCache.when(SaltCache::getInstance).thenReturn(saltCache); + + when(request.getMethod()).thenReturn("POST"); + when(request.getParameter("salt")).thenReturn("submittedSalt"); + when(rollerSession.getAuthenticatedUser()).thenReturn(new TestUser("userId")); + when(saltCache.get("submittedSalt")).thenReturn("userId"); + + LoadSaltFilter loadSaltFilter = new LoadSaltFilter(); + FilterChain terminalChain = mock(FilterChain.class); + FilterChain loadSaltChain = (servletRequest, servletResponse) -> + loadSaltFilter.doFilter(servletRequest, servletResponse, terminalChain); + + filter.doFilter(request, response, loadSaltChain); + + InOrder order = inOrder(saltCache, request, terminalChain); + order.verify(saltCache).get("submittedSalt"); + order.verify(saltCache).remove("submittedSalt"); + order.verify(saltCache).put(anyString(), eq("userId")); + order.verify(request).setAttribute(eq("salt"), anyString()); + order.verify(terminalChain).doFilter(request, response); + } + } + private static class TestUser extends User { + private static final long serialVersionUID = 1L; private final String id; TestUser(String id) { diff --git a/app/src/test/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptorTest.java b/app/src/test/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptorTest.java new file mode 100644 index 0000000000..ba99bb2b01 --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptorTest.java @@ -0,0 +1,173 @@ +/* + * 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.roller.weblogger.pojos.User; +import org.apache.roller.weblogger.ui.core.RollerSession; +import org.apache.roller.weblogger.ui.rendering.util.cache.SaltCache; +import org.apache.struts2.StrutsStatics; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mock; +import org.mockito.MockedStatic; +import org.mockito.MockitoAnnotations; + +import com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.ActionInvocation; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.*; + +public class ValidateSaltInterceptorTest { + + private ValidateSaltInterceptor interceptor; + + @Mock + private ActionInvocation invocation; + + @Mock + private ActionContext context; + + @Mock + private HttpServletRequest request; + + @Mock + private RollerSession rollerSession; + + @Mock + private SaltCache saltCache; + + @BeforeEach + public void setUp() { + MockitoAnnotations.openMocks(this); + interceptor = new ValidateSaltInterceptor(); + when(invocation.getInvocationContext()).thenReturn(context); + when(context.get(StrutsStatics.HTTP_REQUEST)).thenReturn(request); + } + + @Test + public void testValidMultipartSaltIsConsumed() throws Exception { + try (MockedStatic mockedRollerSession = mockStatic(RollerSession.class); + MockedStatic mockedSaltCache = mockStatic(SaltCache.class)) { + mockedRollerSession.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + mockedSaltCache.when(SaltCache::getInstance).thenReturn(saltCache); + + configureMultipartPost("/roller-ui/mediaFileAdd!save.rol"); + when(request.getParameter("salt")).thenReturn("validSalt"); + when(rollerSession.getAuthenticatedUser()).thenReturn(new TestUser("userId")); + when(saltCache.get("validSalt")).thenReturn("userId"); + when(invocation.invoke()).thenReturn("success"); + + assertEquals("success", interceptor.intercept(invocation)); + + verify(saltCache).remove("validSalt"); + verify(invocation).invoke(); + } + } + + @Test + public void testMultipartPostWithoutSaltIsRejectedEvenWithResponseSaltAttribute() throws Exception { + try (MockedStatic mockedRollerSession = mockStatic(RollerSession.class)) { + mockedRollerSession.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + + configureMultipartPost("/roller-ui/bookmarksImport!save.rol"); + when(request.getParameter("salt")).thenReturn(null); + when(request.getAttribute("salt")).thenReturn("responseSalt"); + + assertThrows(ServletException.class, () -> interceptor.intercept(invocation)); + + verify(invocation, never()).invoke(); + } + } + + @Test + public void testInvalidMultipartSaltIsRejectedForAnyRollerAction() throws Exception { + try (MockedStatic mockedRollerSession = mockStatic(RollerSession.class); + MockedStatic mockedSaltCache = mockStatic(SaltCache.class)) { + mockedRollerSession.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + mockedSaltCache.when(SaltCache::getInstance).thenReturn(saltCache); + + configureMultipartPost("/roller-ui/arbitrary!save.rol"); + when(request.getParameter("salt")).thenReturn("invalidSalt"); + when(saltCache.get("invalidSalt")).thenReturn(null); + + assertThrows(ServletException.class, () -> interceptor.intercept(invocation)); + + verify(invocation, never()).invoke(); + verify(saltCache, never()).remove(anyString()); + } + } + + @Test + public void testMultipartSaltCannotBeReplayed() throws Exception { + try (MockedStatic mockedRollerSession = mockStatic(RollerSession.class); + MockedStatic mockedSaltCache = mockStatic(SaltCache.class)) { + mockedRollerSession.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + mockedSaltCache.when(SaltCache::getInstance).thenReturn(saltCache); + + configureMultipartPost("/roller-ui/mediaFileEdit!save.rol"); + when(request.getParameter("salt")).thenReturn("validSalt"); + when(rollerSession.getAuthenticatedUser()).thenReturn(new TestUser("userId")); + when(saltCache.get("validSalt")).thenReturn("userId", (String) null); + + interceptor.intercept(invocation); + assertThrows(ServletException.class, () -> interceptor.intercept(invocation)); + + verify(invocation, times(1)).invoke(); + verify(saltCache, times(1)).remove("validSalt"); + } + } + + @Test + public void testOrdinaryPostIsNotValidatedTwice() throws Exception { + when(request.getMethod()).thenReturn("POST"); + when(request.getContentType()).thenReturn("application/x-www-form-urlencoded"); + when(invocation.invoke()).thenReturn("success"); + + assertEquals("success", interceptor.intercept(invocation)); + + verify(request, never()).getParameter("salt"); + verify(invocation).invoke(); + } + + private void configureMultipartPost(String servletPath) { + when(request.getMethod()).thenReturn("POST"); + when(request.getContentType()).thenReturn("multipart/form-data; boundary=abc123"); + when(request.getServletPath()).thenReturn(servletPath); + } + + private static class TestUser extends User { + private static final long serialVersionUID = 1L; + private final String id; + + TestUser(String id) { + this.id = id; + } + + @Override + public String getId() { + return id; + } + } +} From 5811cc9e49b5e734dd3d8d1cc3ad38c9fd2099c7 Mon Sep 17 00:00:00 2001 From: "David M. Johnson" Date: Tue, 1 Sep 2026 19:20:59 -0400 Subject: [PATCH 2/2] Handle chained and repeated salt-validated requests --- .../ui/core/filters/SaltValidator.java | 17 ++ .../ui/core/filters/ValidateSaltFilter.java | 10 +- .../ui/struts2/ajax/CommentDataServlet.java | 45 +++-- .../struts2/util/ValidateSaltInterceptor.java | 25 ++- app/src/main/resources/struts.xml | 2 +- .../webapp/WEB-INF/jsps/editor/Comments.jsp | 22 ++- .../core/filters/SaltConfigurationTest.java | 24 +-- .../core/filters/ValidateSaltFilterTest.java | 30 ---- .../struts2/ajax/CommentDataServletTest.java | 165 ++++++++++++++++++ .../util/ValidateSaltInterceptorTest.java | 46 +++++ 10 files changed, 316 insertions(+), 70 deletions(-) create mode 100644 app/src/test/java/org/apache/roller/weblogger/ui/struts2/ajax/CommentDataServletTest.java diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/SaltValidator.java b/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/SaltValidator.java index dbe8abd594..84cd0676d2 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/SaltValidator.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/SaltValidator.java @@ -22,6 +22,7 @@ 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; @@ -34,6 +35,9 @@ 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() { } @@ -66,6 +70,19 @@ public static boolean consumeSubmittedSalt(HttpServletRequest request) { 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. diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilter.java b/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilter.java index f1a90e156d..78a32474b9 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilter.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilter.java @@ -45,18 +45,20 @@ public void doFilter(ServletRequest request, ServletResponse response, if ("POST".equalsIgnoreCase(httpReq.getMethod())) { if (SaltValidator.isMultipartFormPost(httpReq) && isStrutsAction(httpReq)) { - // Struts makes multipart parameters available after its upload - // interceptor; ValidateSaltInterceptor handles these requests. + // Struts wraps multipart requests before its interceptor stack; + // ValidateSaltInterceptor handles these requests. chain.doFilter(request, response); return; } - if (!SaltValidator.consumeSubmittedSalt(httpReq)) { + try { + SaltValidator.requireSubmittedSalt(httpReq); + } catch (ServletException e) { if (log.isDebugEnabled()) { log.debug("Valid salt value not found on POST to URL : " + httpReq.getServletPath()); } - throw new ServletException("Security Violation"); + throw e; } } diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/ajax/CommentDataServlet.java b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/ajax/CommentDataServlet.java index 59db62bd7f..6868c27ebd 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/ajax/CommentDataServlet.java +++ b/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(); + } } diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptor.java b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptor.java index b64f103d6a..76afb21da5 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptor.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptor.java @@ -26,13 +26,14 @@ 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 parsed a multipart form request. + * Validates salts after Struts has wrapped a multipart form request. */ public class ValidateSaltInterceptor extends AbstractInterceptor implements StrutsStatics { @@ -45,12 +46,24 @@ public String intercept(ActionInvocation invocation) throws Exception { HttpServletRequest request = (HttpServletRequest) context.get(HTTP_REQUEST); if (SaltValidator.isMultipartFormPost(request) - && !SaltValidator.consumeSubmittedSalt(request)) { - if (log.isDebugEnabled()) { - log.debug("Valid salt value not found on multipart POST to URL : " - + request.getServletPath()); + && !Boolean.TRUE.equals(request.getAttribute( + SaltValidator.VALIDATED_REQUEST_ATTRIBUTE))) { + if (request instanceof MultiPartRequestWrapper + && ((MultiPartRequestWrapper) request).hasErrors()) { + return invocation.invoke(); } - throw new ServletException("Security Violation"); + + 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(); diff --git a/app/src/main/resources/struts.xml b/app/src/main/resources/struts.xml index 51763b1b50..db46594c59 100644 --- a/app/src/main/resources/struts.xml +++ b/app/src/main/resources/struts.xml @@ -47,6 +47,7 @@ default stack --> + @@ -57,7 +58,6 @@ - diff --git a/app/src/main/webapp/WEB-INF/jsps/editor/Comments.jsp b/app/src/main/webapp/WEB-INF/jsps/editor/Comments.jsp index f13a8d59e6..78c3f7d71f 100644 --- a/app/src/main/webapp/WEB-INF/jsps/editor/Comments.jsp +++ b/app/src/main/webapp/WEB-INF/jsps/editor/Comments.jsp @@ -434,9 +434,9 @@ dataType: "text", processData: "false", contentType: "text/plain", - success: function (rdata) { - if (status != "success") { - var cdata = eval("(" + rdata + ")"); + success: function (rdata, status) { + var cdata = updateCommentSalt(rdata); + if (status === "success" && cdata && cdata.content !== undefined) { $("#editlink-" + id).show(); $("#savelink-" + id).hide(); $("#cancellink-" + id).hide(); @@ -444,10 +444,26 @@ } else { alert(''); } + }, + error: function (xhr) { + updateCommentSalt(xhr.responseText); + alert(''); } }); } + function updateCommentSalt(rdata) { + try { + var cdata = JSON.parse(rdata); + if (cdata.salt) { + $("#comments_salt").val(cdata.salt); + } + return cdata; + } catch (error) { + return null; + } + } + function editCommentCancel(id) { $("#editlink-" + id).show(); $("#savelink-" + id).hide(); diff --git a/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/SaltConfigurationTest.java b/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/SaltConfigurationTest.java index 1c3a9431b8..8f28874bb1 100644 --- a/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/SaltConfigurationTest.java +++ b/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/SaltConfigurationTest.java @@ -20,7 +20,6 @@ package org.apache.roller.weblogger.ui.core.filters; import java.io.IOException; -import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; @@ -29,14 +28,16 @@ import org.junit.jupiter.api.Test; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; public class SaltConfigurationTest { @Test public void testSubmittedSaltIsValidatedBeforeResponseSaltIsLoaded() throws Exception { - String webXml = Files.readString(Path.of("src/main/webapp/WEB-INF/web.xml")); + Path projectDirectory = Path.of( + System.getProperty("project.build.directory")).getParent(); + String webXml = Files.readString(projectDirectory.resolve( + "src/main/webapp/WEB-INF/web.xml")); int validateMapping = filterMappingPosition(webXml, "ValidateSaltFilter"); int loadMapping = filterMappingPosition(webXml, "LoadSaltFilter"); @@ -48,23 +49,15 @@ public void testSubmittedSaltIsValidatedBeforeResponseSaltIsLoaded() throws Exce } @Test - public void testMultipartSaltValidationImmediatelyFollowsUploadInterceptor() throws Exception { + public void testMultipartSaltValidationImmediatelyFollowsExceptionInterceptor() throws Exception { String strutsXml = readResource("/struts.xml"); Pattern adjacentInterceptors = Pattern.compile( - "\\s*" + "\\s*" + ""); assertTrue(adjacentInterceptors.matcher(strutsXml).find(), - "ValidateSaltInterceptor must immediately follow the upload interceptor"); - } - - @Test - public void testConfigurableSaltBypassIsRemoved() throws Exception { - String properties = readResource( - "/org/apache/roller/weblogger/config/roller.properties"); - - assertFalse(properties.contains("salt.ignored.urls")); + "ValidateSaltInterceptor must immediately follow the exception interceptor"); } private int filterMappingPosition(String webXml, String filterName) { @@ -75,7 +68,8 @@ private int filterMappingPosition(String webXml, String filterName) { } private String readResource(String path) throws IOException { - try (InputStream stream = SaltConfigurationTest.class.getResourceAsStream(path)) { + try (java.io.InputStream stream = + SaltConfigurationTest.class.getResourceAsStream(path)) { if (stream == null) { throw new IOException("Test resource not found: " + path); } diff --git a/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilterTest.java b/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilterTest.java index fcc5226f1d..5f0715bee0 100644 --- a/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilterTest.java +++ b/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilterTest.java @@ -5,7 +5,6 @@ import org.apache.roller.weblogger.ui.rendering.util.cache.SaltCache; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.MockedStatic; import org.mockito.MockitoAnnotations; @@ -197,35 +196,6 @@ public void testMultipartNonStrutsPostIsNotDeferred() throws Exception { } } - @Test - public void testValidationRunsBeforeResponseSaltGeneration() throws Exception { - try (MockedStatic mockedRollerSession = mockStatic(RollerSession.class); - MockedStatic mockedSaltCache = mockStatic(SaltCache.class)) { - - mockedRollerSession.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); - mockedSaltCache.when(SaltCache::getInstance).thenReturn(saltCache); - - when(request.getMethod()).thenReturn("POST"); - when(request.getParameter("salt")).thenReturn("submittedSalt"); - when(rollerSession.getAuthenticatedUser()).thenReturn(new TestUser("userId")); - when(saltCache.get("submittedSalt")).thenReturn("userId"); - - LoadSaltFilter loadSaltFilter = new LoadSaltFilter(); - FilterChain terminalChain = mock(FilterChain.class); - FilterChain loadSaltChain = (servletRequest, servletResponse) -> - loadSaltFilter.doFilter(servletRequest, servletResponse, terminalChain); - - filter.doFilter(request, response, loadSaltChain); - - InOrder order = inOrder(saltCache, request, terminalChain); - order.verify(saltCache).get("submittedSalt"); - order.verify(saltCache).remove("submittedSalt"); - order.verify(saltCache).put(anyString(), eq("userId")); - order.verify(request).setAttribute(eq("salt"), anyString()); - order.verify(terminalChain).doFilter(request, response); - } - } - private static class TestUser extends User { private static final long serialVersionUID = 1L; private final String id; diff --git a/app/src/test/java/org/apache/roller/weblogger/ui/struts2/ajax/CommentDataServletTest.java b/app/src/test/java/org/apache/roller/weblogger/ui/struts2/ajax/CommentDataServletTest.java new file mode 100644 index 0000000000..9c3e87618a --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/ui/struts2/ajax/CommentDataServletTest.java @@ -0,0 +1,165 @@ +/* + * 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.ajax; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.nio.charset.StandardCharsets; + +import javax.servlet.ReadListener; +import javax.servlet.ServletInputStream; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.apache.roller.weblogger.business.Weblogger; +import org.apache.roller.weblogger.business.WebloggerFactory; +import org.apache.roller.weblogger.business.WeblogEntryManager; +import org.apache.roller.weblogger.pojos.User; +import org.apache.roller.weblogger.pojos.Weblog; +import org.apache.roller.weblogger.pojos.WeblogEntry; +import org.apache.roller.weblogger.pojos.WeblogEntryComment; +import org.apache.roller.weblogger.pojos.WeblogPermission; +import org.apache.roller.weblogger.ui.core.RollerSession; +import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; + +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +public class CommentDataServletTest { + + @Test + public void testRepeatedSavesReturnFreshResponseSalts() throws Exception { + Weblogger weblogger = mock(Weblogger.class); + WeblogEntryManager manager = mock(WeblogEntryManager.class); + WeblogEntryComment comment = mock(WeblogEntryComment.class); + WeblogEntry entry = mock(WeblogEntry.class); + Weblog weblog = mock(Weblog.class); + RollerSession session = mock(RollerSession.class); + User user = mock(User.class); + + when(weblogger.getWeblogEntryManager()).thenReturn(manager); + when(manager.getComment("comment-id")).thenReturn(comment); + when(comment.getWeblogEntry()).thenReturn(entry); + when(entry.getWebsite()).thenReturn(weblog); + when(session.getAuthenticatedUser()).thenReturn(user); + when(weblog.hasUserPermission(user, WeblogPermission.POST)).thenReturn(true); + when(comment.getId()).thenReturn("comment-id"); + when(comment.getContent()).thenReturn("updated"); + + try (MockedStatic factory = mockStatic(WebloggerFactory.class); + MockedStatic sessions = mockStatic(RollerSession.class)) { + factory.when(WebloggerFactory::getWeblogger).thenReturn(weblogger); + + HttpServletRequest firstRequest = saveRequest("first-response-salt"); + HttpServletResponse firstResponse = mock(HttpServletResponse.class); + StringWriter firstBody = responseBody(firstResponse); + sessions.when(() -> RollerSession.getRollerSession(firstRequest)).thenReturn(session); + + new CommentDataServlet().doPost(firstRequest, firstResponse); + + HttpServletRequest secondRequest = saveRequest("second-response-salt"); + HttpServletResponse secondResponse = mock(HttpServletResponse.class); + StringWriter secondBody = responseBody(secondResponse); + sessions.when(() -> RollerSession.getRollerSession(secondRequest)).thenReturn(session); + + new CommentDataServlet().doPost(secondRequest, secondResponse); + + assertTrue(firstBody.toString().contains( + "\"salt\":\"first-response-salt\"")); + assertTrue(secondBody.toString().contains( + "\"salt\":\"second-response-salt\"")); + verify(manager, times(2)).saveComment(comment); + } + } + + @Test + public void testHandledErrorReturnsFreshResponseSalt() throws Exception { + Weblogger weblogger = mock(Weblogger.class); + WeblogEntryManager manager = mock(WeblogEntryManager.class); + when(weblogger.getWeblogEntryManager()).thenReturn(manager); + when(manager.getComment("missing-id")).thenReturn(null); + + HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getParameter("id")).thenReturn("missing-id"); + when(request.getAttribute("salt")).thenReturn("replacement-salt"); + HttpServletResponse response = mock(HttpServletResponse.class); + PrintWriter writer = mock(PrintWriter.class); + when(response.getWriter()).thenReturn(writer); + + try (MockedStatic factory = mockStatic(WebloggerFactory.class)) { + factory.when(WebloggerFactory::getWeblogger).thenReturn(weblogger); + + new CommentDataServlet().doPost(request, response); + } + + verify(response).setStatus(HttpServletResponse.SC_NOT_FOUND); + verify(writer).print("{\"salt\":\"replacement-salt\"}"); + } + + private HttpServletRequest saveRequest(String responseSalt) throws Exception { + HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getParameter("id")).thenReturn("comment-id"); + when(request.getAttribute("salt")).thenReturn(responseSalt); + when(request.getInputStream()).thenReturn( + new ByteArrayServletInputStream("updated")); + return request; + } + + private StringWriter responseBody(HttpServletResponse response) throws Exception { + StringWriter body = new StringWriter(); + when(response.getWriter()).thenReturn(new PrintWriter(body)); + return body; + } + + private static class ByteArrayServletInputStream extends ServletInputStream { + private final ByteArrayInputStream input; + + ByteArrayServletInputStream(String value) { + input = new ByteArrayInputStream( + value.getBytes(StandardCharsets.UTF_8)); + } + + @Override + public boolean isFinished() { + return input.available() == 0; + } + + @Override + public boolean isReady() { + return true; + } + + @Override + public void setReadListener(ReadListener readListener) { + } + + @Override + public int read() throws IOException { + return input.read(); + } + } +} diff --git a/app/src/test/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptorTest.java b/app/src/test/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptorTest.java index ba99bb2b01..1aea1b6642 100644 --- a/app/src/test/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptorTest.java +++ b/app/src/test/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptorTest.java @@ -24,8 +24,10 @@ import org.apache.roller.weblogger.pojos.User; import org.apache.roller.weblogger.ui.core.RollerSession; +import org.apache.roller.weblogger.ui.core.filters.SaltValidator; import org.apache.roller.weblogger.ui.rendering.util.cache.SaltCache; import org.apache.struts2.StrutsStatics; +import org.apache.struts2.dispatcher.multipart.MultiPartRequestWrapper; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mock; @@ -52,6 +54,9 @@ public class ValidateSaltInterceptorTest { @Mock private HttpServletRequest request; + @Mock + private MultiPartRequestWrapper multipartRequest; + @Mock private RollerSession rollerSession; @@ -82,10 +87,51 @@ public void testValidMultipartSaltIsConsumed() throws Exception { assertEquals("success", interceptor.intercept(invocation)); verify(saltCache).remove("validSalt"); + verify(request).setAttribute( + SaltValidator.VALIDATED_REQUEST_ATTRIBUTE, Boolean.TRUE); verify(invocation).invoke(); } } + @Test + public void testChainedMultipartRequestIsValidatedOnlyOnce() throws Exception { + try (MockedStatic mockedRollerSession = mockStatic(RollerSession.class); + MockedStatic mockedSaltCache = mockStatic(SaltCache.class)) { + mockedRollerSession.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession); + mockedSaltCache.when(SaltCache::getInstance).thenReturn(saltCache); + + configureMultipartPost("/roller-ui/bookmarksImport!save.rol"); + when(request.getParameter("salt")).thenReturn("validSalt"); + when(rollerSession.getAuthenticatedUser()).thenReturn(new TestUser("userId")); + when(saltCache.get("validSalt")).thenReturn("userId"); + when(request.getAttribute(SaltValidator.VALIDATED_REQUEST_ATTRIBUTE)) + .thenReturn(null, Boolean.TRUE); + when(invocation.invoke()).thenReturn("success"); + + assertEquals("success", interceptor.intercept(invocation)); + assertEquals("success", interceptor.intercept(invocation)); + + verify(saltCache).get("validSalt"); + verify(saltCache).remove("validSalt"); + verify(invocation, times(2)).invoke(); + } + } + + @Test + public void testMultipartParsingErrorsReachUploadHandling() throws Exception { + when(context.get(StrutsStatics.HTTP_REQUEST)).thenReturn(multipartRequest); + when(multipartRequest.getMethod()).thenReturn("POST"); + when(multipartRequest.getContentType()).thenReturn( + "multipart/form-data; boundary=abc123"); + when(multipartRequest.hasErrors()).thenReturn(true); + when(invocation.invoke()).thenReturn("input"); + + assertEquals("input", interceptor.intercept(invocation)); + + verify(multipartRequest, never()).getParameter("salt"); + verify(invocation).invoke(); + } + @Test public void testMultipartPostWithoutSaltIsRejectedEvenWithResponseSaltAttribute() throws Exception { try (MockedStatic mockedRollerSession = mockStatic(RollerSession.class)) {