This is an automated email from the ASF dual-hosted git repository.
snoopdave pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/roller.git
The following commit(s) were added to refs/heads/master by this push:
new faafe379e Implement one-time-salt use and add comprehensive tests
(#142)
faafe379e is described below
commit faafe379e353dc6a44d350734e4d8113d666a3be
Author: David M. Johnson <[email protected]>
AuthorDate: Sun Oct 6 16:45:44 2024 -0400
Implement one-time-salt use and add comprehensive tests (#142)
* Implement one-time-salt use and add comprehensive tests
* Add test for ignored URLs
* Remove unneeded mocking
* Don't allow web analytics if weblog admins untrusted
* Don't allow web analytics if weblog admins untrusted
---
.../weblogger/pojos/wrapper/WeblogWrapper.java | 3 +-
.../weblogger/ui/core/filters/LoadSaltFilter.java | 15 +-
.../ui/core/filters/ValidateSaltFilter.java | 42 ++---
.../weblogger/ui/struts2/editor/WeblogConfig.java | 15 +-
.../main/resources/ApplicationResources.properties | 1 +
.../webapp/WEB-INF/jsps/editor/WeblogConfig.jsp | 4 +-
.../ui/core/filters/LoadSaltFilterTest.java | 88 ++++++++++
.../ui/core/filters/ValidateSaltFilterTest.java | 179 +++++++++++++++++++++
8 files changed, 312 insertions(+), 35 deletions(-)
diff --git
a/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogWrapper.java
b/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogWrapper.java
index afc297a29..71ff4d33e 100644
---
a/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogWrapper.java
+++
b/app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogWrapper.java
@@ -141,10 +141,9 @@ public final class WeblogWrapper {
}
public String getAnalyticsCode() {
- return this.pojo.getAnalyticsCode();
+ return
HTMLSanitizer.conditionallySanitize(this.pojo.getAnalyticsCode());
}
-
public Boolean getEmailComments() {
return this.pojo.getEmailComments();
}
diff --git
a/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/LoadSaltFilter.java
b/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/LoadSaltFilter.java
index e6416ce96..b2e63915d 100644
---
a/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/LoadSaltFilter.java
+++
b/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/LoadSaltFilter.java
@@ -37,13 +37,14 @@ public class LoadSaltFilter implements Filter {
throws IOException, ServletException {
HttpServletRequest httpReq = (HttpServletRequest) request;
- RollerSession rses = RollerSession.getRollerSession(httpReq);
- String userId = rses != null && rses.getAuthenticatedUser() != null ?
rses.getAuthenticatedUser().getId() : "";
-
- SaltCache saltCache = SaltCache.getInstance();
- String salt = RandomStringUtils.random(20, 0, 0, true, true, null, new
SecureRandom());
- saltCache.put(salt, userId);
- httpReq.setAttribute("salt", salt);
+ RollerSession rollerSession = RollerSession.getRollerSession(httpReq);
+ if (rollerSession != null) {
+ String userId = rollerSession.getAuthenticatedUser() != null ?
rollerSession.getAuthenticatedUser().getId() : "";
+ SaltCache saltCache = SaltCache.getInstance();
+ String salt = RandomStringUtils.random(20, 0, 0, true, true, null,
new SecureRandom());
+ saltCache.put(salt, userId);
+ httpReq.setAttribute("salt", salt);
+ }
chain.doFilter(request, response);
}
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 275ccd328..9744551bc 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
@@ -51,17 +51,31 @@ public class ValidateSaltFilter implements Filter {
FilterChain chain) throws IOException, ServletException {
HttpServletRequest httpReq = (HttpServletRequest) request;
- if ("POST".equals(httpReq.getMethod()) &&
!isIgnoredURL(httpReq.getServletPath())) {
- RollerSession rses = RollerSession.getRollerSession(httpReq);
- String userId = rses != null && rses.getAuthenticatedUser() !=
null ? rses.getAuthenticatedUser().getId() : "";
+ 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() : "";
+
+ String 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");
+ }
- String salt = httpReq.getParameter("salt");
- SaltCache saltCache = SaltCache.getInstance();
- if (salt == null || !Objects.equals(saltCache.get(salt), userId)) {
+ // Remove salt from cache after successful validation
+ saltCache.remove(salt);
if (log.isDebugEnabled()) {
- log.debug("Valid salt value not found on POST to URL : " +
httpReq.getServletPath());
+ log.debug("Salt used and invalidated: " + salt);
}
- throw new ServletException("Security Violation");
}
}
@@ -70,8 +84,6 @@ public class ValidateSaltFilter implements Filter {
@Override
public void init(FilterConfig filterConfig) throws ServletException {
-
- // Construct our list of ignored urls
String urls = WebloggerConfig.getProperty("salt.ignored.urls");
ignored = Set.of(StringUtils.stripAll(StringUtils.split(urls, ",")));
}
@@ -82,16 +94,10 @@ public class ValidateSaltFilter implements Filter {
/**
* Checks if this is an ignored url defined in the salt.ignored.urls
property
- * @param theUrl the the url
+ * @param theUrl the url
* @return true, if is ignored resource
*/
private boolean isIgnoredURL(String theUrl) {
- int i = theUrl.lastIndexOf('/');
-
- // If it's not a resource then don't ignore it
- if (i <= 0 || i == theUrl.length() - 1) {
- return false;
- }
- return ignored.contains(theUrl.substring(i + 1));
+ return ignored.contains(theUrl);
}
}
\ No newline at end of file
diff --git
a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/WeblogConfig.java
b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/WeblogConfig.java
index 912ffe122..0cfe9a352 100644
---
a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/WeblogConfig.java
+++
b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/WeblogConfig.java
@@ -29,6 +29,7 @@ import
org.apache.roller.weblogger.business.plugins.PluginManager;
import org.apache.roller.weblogger.business.WebloggerFactory;
import org.apache.roller.weblogger.business.WeblogEntryManager;
import org.apache.roller.weblogger.business.plugins.entry.WeblogEntryPlugin;
+import org.apache.roller.weblogger.config.WebloggerConfig;
import org.apache.roller.weblogger.config.WebloggerRuntimeConfig;
import org.apache.roller.weblogger.pojos.Weblog;
import org.apache.roller.weblogger.pojos.WeblogCategory;
@@ -61,6 +62,8 @@ public class WeblogConfig extends UIAction {
// list of available plugins
private List<WeblogEntryPlugin> pluginsList = Collections.emptyList();
+
+ private final boolean weblogAdminsUntrusted =
WebloggerConfig.getBooleanProperty("weblogAdminsUntrusted");
public WeblogConfig() {
@@ -71,7 +74,7 @@ public class WeblogConfig extends UIAction {
@Override
public void myPrepare() {
-
+
try {
WeblogEntryManager wmgr =
WebloggerFactory.getWeblogger().getWeblogEntryManager();
@@ -88,10 +91,7 @@ public class WeblogConfig extends UIAction {
// set plugins list
PluginManager ppmgr =
WebloggerFactory.getWeblogger().getPluginManager();
Map<String, WeblogEntryPlugin> pluginsMap =
ppmgr.getWeblogEntryPlugins(getActionWeblog());
- List<WeblogEntryPlugin> plugins = new ArrayList<>();
- for (WeblogEntryPlugin entryPlugin : pluginsMap.values()) {
- plugins.add(entryPlugin);
- }
+ List<WeblogEntryPlugin> plugins = new
ArrayList<>(pluginsMap.values());
// sort
setPluginsList(plugins);
@@ -231,5 +231,8 @@ public class WeblogConfig extends UIAction {
public void setPluginsList(List<WeblogEntryPlugin> pluginsList) {
this.pluginsList = pluginsList;
}
-
+
+ public boolean getWeblogAdminsUntrusted() {
+ return weblogAdminsUntrusted;
+ }
}
diff --git a/app/src/main/resources/ApplicationResources.properties
b/app/src/main/resources/ApplicationResources.properties
index b318ff328..46c37454a 100644
--- a/app/src/main/resources/ApplicationResources.properties
+++ b/app/src/main/resources/ApplicationResources.properties
@@ -1759,6 +1759,7 @@ websiteSettings.formatting=Formatting
websiteSettings.spamPrevention=Spam Prevention
websiteSettings.ignoreUrls=List of words and regex expressions listed one per \
line to be added to the banned words list used to check comments, trackbacks
and referrers.
+websiteSettings.bannedWordsList=Words banned in comments (regex allowed)
websiteSettings.acceptedBannedwordslist=Accepted {0} string and {1} regex
banned-words list rules
websiteSettings.error.processingBannedwordslist=Error processing banned-words
list: {0}
diff --git a/app/src/main/webapp/WEB-INF/jsps/editor/WeblogConfig.jsp
b/app/src/main/webapp/WEB-INF/jsps/editor/WeblogConfig.jsp
index 770e01ef7..c0437c721 100644
--- a/app/src/main/webapp/WEB-INF/jsps/editor/WeblogConfig.jsp
+++ b/app/src/main/webapp/WEB-INF/jsps/editor/WeblogConfig.jsp
@@ -130,11 +130,11 @@
<h3><s:text name="websiteSettings.spamPrevention"/></h3>
<s:textarea name="bean.bannedwordslist" rows="7" cols="40"
- label="%{getText('websiteSettings.analyticsTrackingCode')}"/>
+ label="%{getText('websiteSettings.bannedWordsList')}"/>
<%-- ***** Web analytics settings ***** --%>
- <s:if test="getBooleanProp('analytics.code.override.allowed')">
+ <s:if test="getBooleanProp('analytics.code.override.allowed') &&
!weblogAdminsUntrusted">
<h3><s:text name="configForm.webAnalytics"/></h3>
<s:textarea name="bean.analyticsCode" rows="10" cols="70"
diff --git
a/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/LoadSaltFilterTest.java
b/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/LoadSaltFilterTest.java
new file mode 100644
index 000000000..5ace927a2
--- /dev/null
+++
b/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/LoadSaltFilterTest.java
@@ -0,0 +1,88 @@
+package org.apache.roller.weblogger.ui.core.filters;
+
+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.Mock;
+import org.mockito.MockedStatic;
+import org.mockito.MockitoAnnotations;
+
+import javax.servlet.FilterChain;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import static org.mockito.Mockito.*;
+
+public class LoadSaltFilterTest {
+
+ private LoadSaltFilter filter;
+
+ @Mock
+ private HttpServletRequest request;
+
+ @Mock
+ private HttpServletResponse response;
+
+ @Mock
+ private FilterChain chain;
+
+ @Mock
+ private RollerSession rollerSession;
+
+ @Mock
+ private SaltCache saltCache;
+
+ @BeforeEach
+ public void setUp() {
+ MockitoAnnotations.initMocks(this);
+ filter = new LoadSaltFilter();
+ }
+
+ @Test
+ public void testDoFilterGeneratesSalt() throws Exception {
+ try (MockedStatic<RollerSession> mockedRollerSession =
mockStatic(RollerSession.class);
+ MockedStatic<SaltCache> mockedSaltCache =
mockStatic(SaltCache.class)) {
+
+ mockedRollerSession.when(() ->
RollerSession.getRollerSession(request)).thenReturn(rollerSession);
+ mockedSaltCache.when(SaltCache::getInstance).thenReturn(saltCache);
+
+ when(rollerSession.getAuthenticatedUser()).thenReturn(new
TestUser("userId"));
+
+ filter.doFilter(request, response, chain);
+
+ verify(request).setAttribute(eq("salt"), anyString());
+ verify(saltCache).put(anyString(), eq("userId"));
+ verify(chain).doFilter(request, response);
+ }
+ }
+
+ @Test
+ public void testDoFilterWithNullRollerSession() throws Exception {
+ try (MockedStatic<RollerSession> mockedRollerSession =
mockStatic(RollerSession.class);
+ MockedStatic<SaltCache> mockedSaltCache =
mockStatic(SaltCache.class)) {
+
+ mockedRollerSession.when(() ->
RollerSession.getRollerSession(request)).thenReturn(null);
+ mockedSaltCache.when(SaltCache::getInstance).thenReturn(saltCache);
+
+ filter.doFilter(request, response, chain);
+
+ verify(request, never()).setAttribute(eq("salt"), anyString());
+ verify(saltCache, never()).put(anyString(), anyString());
+ verify(chain).doFilter(request, response);
+ }
+ }
+
+ private static class TestUser extends User {
+ private final String id;
+
+ TestUser(String id) {
+ this.id = id;
+ }
+
+ public String getId() {
+ return id;
+ }
+ }
+}
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
new file mode 100644
index 000000000..ab866d080
--- /dev/null
+++
b/app/src/test/java/org/apache/roller/weblogger/ui/core/filters/ValidateSaltFilterTest.java
@@ -0,0 +1,179 @@
+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.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;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.Mockito.*;
+
+public class ValidateSaltFilterTest {
+
+ private ValidateSaltFilter filter;
+
+ @Mock
+ private HttpServletRequest request;
+
+ @Mock
+ private HttpServletResponse response;
+
+ @Mock
+ private FilterChain chain;
+
+ @Mock
+ private RollerSession rollerSession;
+
+ @Mock
+ private SaltCache saltCache;
+
+ @BeforeEach
+ public void setUp() {
+ MockitoAnnotations.openMocks(this);
+ filter = new ValidateSaltFilter();
+ }
+
+ @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);
+
+ verify(chain).doFilter(request, response);
+ }
+
+ @Test
+ public void testDoFilterWithPostMethodAndValidSalt() throws Exception {
+ try (MockedStatic<RollerSession> mockedRollerSession =
mockStatic(RollerSession.class);
+ MockedStatic<SaltCache> 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(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);
+ verify(saltCache).remove("validSalt");
+ }
+ }
+
+ @Test
+ public void testDoFilterWithPostMethodAndInvalidSalt() throws Exception {
+ try (MockedStatic<RollerSession> mockedRollerSession =
mockStatic(RollerSession.class);
+ MockedStatic<SaltCache> 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("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);
+ });
+ }
+ }
+
+ @Test
+ public void testDoFilterWithPostMethodAndMismatchedUserId() throws
Exception {
+ try (MockedStatic<RollerSession> mockedRollerSession =
mockStatic(RollerSession.class);
+ MockedStatic<SaltCache> 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(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);
+ });
+ }
+ }
+
+ @Test
+ public void testDoFilterWithPostMethodAndNullRollerSession() throws
Exception {
+ try (MockedStatic<RollerSession> mockedRollerSession =
mockStatic(RollerSession.class);
+ MockedStatic<SaltCache> mockedSaltCache =
mockStatic(SaltCache.class)) {
+
+ mockedRollerSession.when(() ->
RollerSession.getRollerSession(request)).thenReturn(null);
+ mockedSaltCache.when(SaltCache::getInstance).thenReturn(saltCache);
+
+ 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");
+ }
+ }
+
+ @Test
+ public void testDoFilterWithIgnoredURL() throws Exception {
+ try (MockedStatic<RollerSession> mockedRollerSession =
mockStatic(RollerSession.class);
+ MockedStatic<SaltCache> mockedSaltCache =
mockStatic(SaltCache.class);
+ MockedStatic<WebloggerConfig> mockedWebloggerConfig =
mockStatic(WebloggerConfig.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
+
+ filter.init(mock(FilterConfig.class));
+ filter.doFilter(request, response, chain);
+
+ verify(chain).doFilter(request, response);
+ verify(saltCache, never()).get(anyString());
+ verify(saltCache, never()).remove(anyString());
+ }
+ }
+
+ private static class TestUser extends User {
+ private final String id;
+
+ TestUser(String id) {
+ this.id = id;
+ }
+
+ @Override
+ public String getId() {
+ return id;
+ }
+ }
+}
