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..84cd0676d2 --- /dev/null +++ b/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)); + } +} 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..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 @@ -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,28 @@ */ 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 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"); } -} \ No newline at end of file +} 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 new file mode 100644 index 0000000000..76afb21da5 --- /dev/null +++ b/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(); + } +} 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..db46594c59 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/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/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..8f28874bb1 --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/SaltConfigurationTest.java @@ -0,0 +1,79 @@ +/* + * 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.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.assertTrue; + +public class SaltConfigurationTest { + + @Test + public void testSubmittedSaltIsValidatedBeforeResponseSaltIsLoaded() throws Exception { + 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"); + + 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 testMultipartSaltValidationImmediatelyFollowsExceptionInterceptor() throws Exception { + String strutsXml = readResource("/struts.xml"); + + Pattern adjacentInterceptors = Pattern.compile( + "\\s*" + + ""); + + assertTrue(adjacentInterceptors.matcher(strutsXml).find(), + "ValidateSaltInterceptor must immediately follow the exception interceptor"); + } + + 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 (java.io.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..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 @@ -1,6 +1,5 @@ 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; @@ -11,7 +10,6 @@ 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 +45,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 +63,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 +81,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 +99,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 +116,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 +123,81 @@ 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); + } + } + 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/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 new file mode 100644 index 0000000000..1aea1b6642 --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptorTest.java @@ -0,0 +1,219 @@ +/* + * 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.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; +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 MultiPartRequestWrapper multipartRequest; + + @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(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)) { + 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; + } + } +}