Skip to content

Commit faafe37

Browse files
snoopdavesnoopdave
authored
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
1 parent 78a2c78 commit faafe37

File tree

8 files changed

+312
-35
lines changed

8 files changed

+312
-35
lines changed

‎app/src/main/java/org/apache/roller/weblogger/pojos/wrapper/WeblogWrapper.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,10 +141,9 @@ public Boolean getModerateComments() {
141141
}
142142

143143
public String getAnalyticsCode() {
144-
return this.pojo.getAnalyticsCode();
144+
return HTMLSanitizer.conditionallySanitize(this.pojo.getAnalyticsCode());
145145
}
146146

147-
148147
public Boolean getEmailComments() {
149148
return this.pojo.getEmailComments();
150149
}

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

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,13 +37,14 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
3737
throws IOException, ServletException {
3838

3939
HttpServletRequest httpReq = (HttpServletRequest) request;
40-
RollerSession rses = RollerSession.getRollerSession(httpReq);
41-
String userId = rses != null && rses.getAuthenticatedUser() != null ? rses.getAuthenticatedUser().getId() : "";
42-
43-
SaltCache saltCache = SaltCache.getInstance();
44-
String salt = RandomStringUtils.random(20, 0, 0, true, true, null, new SecureRandom());
45-
saltCache.put(salt, userId);
46-
httpReq.setAttribute("salt", salt);
40+
RollerSession rollerSession = RollerSession.getRollerSession(httpReq);
41+
if (rollerSession != null) {
42+
String userId = rollerSession.getAuthenticatedUser() != null ? rollerSession.getAuthenticatedUser().getId() : "";
43+
SaltCache saltCache = SaltCache.getInstance();
44+
String salt = RandomStringUtils.random(20, 0, 0, true, true, null, new SecureRandom());
45+
saltCache.put(salt, userId);
46+
httpReq.setAttribute("salt", salt);
47+
}
4748

4849
chain.doFilter(request, response);
4950
}

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

Lines changed: 24 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -51,17 +51,31 @@ public void doFilter(ServletRequest request, ServletResponse response,
5151
FilterChain chain) throws IOException, ServletException {
5252
HttpServletRequest httpReq = (HttpServletRequest) request;
5353

54-
if ("POST".equals(httpReq.getMethod()) && !isIgnoredURL(httpReq.getServletPath())) {
55-
RollerSession rses = RollerSession.getRollerSession(httpReq);
56-
String userId = rses != null && rses.getAuthenticatedUser() != null ? rses.getAuthenticatedUser().getId() : "";
54+
String requestURL = httpReq.getRequestURL().toString();
55+
String queryString = httpReq.getQueryString();
56+
if (queryString != null) {
57+
requestURL += "?" + queryString;
58+
}
59+
60+
if ("POST".equals(httpReq.getMethod()) && !isIgnoredURL(requestURL)) {
61+
RollerSession rollerSession = RollerSession.getRollerSession(httpReq);
62+
if (rollerSession != null) {
63+
String userId = rollerSession.getAuthenticatedUser() != null ? rollerSession.getAuthenticatedUser().getId() : "";
64+
65+
String salt = httpReq.getParameter("salt");
66+
SaltCache saltCache = SaltCache.getInstance();
67+
if (salt == null || !Objects.equals(saltCache.get(salt), userId)) {
68+
if (log.isDebugEnabled()) {
69+
log.debug("Valid salt value not found on POST to URL : " + httpReq.getServletPath());
70+
}
71+
throw new ServletException("Security Violation");
72+
}
5773

58-
String salt = httpReq.getParameter("salt");
59-
SaltCache saltCache = SaltCache.getInstance();
60-
if (salt == null || !Objects.equals(saltCache.get(salt), userId)) {
74+
// Remove salt from cache after successful validation
75+
saltCache.remove(salt);
6176
if (log.isDebugEnabled()) {
62-
log.debug("Valid salt value not found on POST to URL : " + httpReq.getServletPath());
77+
log.debug("Salt used and invalidated: " + salt);
6378
}
64-
throw new ServletException("Security Violation");
6579
}
6680
}
6781

@@ -70,8 +84,6 @@ public void doFilter(ServletRequest request, ServletResponse response,
7084

7185
@Override
7286
public void init(FilterConfig filterConfig) throws ServletException {
73-
74-
// Construct our list of ignored urls
7587
String urls = WebloggerConfig.getProperty("salt.ignored.urls");
7688
ignored = Set.of(StringUtils.stripAll(StringUtils.split(urls, ",")));
7789
}
@@ -82,16 +94,10 @@ public void destroy() {
8294

8395
/**
8496
* Checks if this is an ignored url defined in the salt.ignored.urls property
85-
* @param theUrl the the url
97+
* @param theUrl the url
8698
* @return true, if is ignored resource
8799
*/
88100
private boolean isIgnoredURL(String theUrl) {
89-
int i = theUrl.lastIndexOf('/');
90-
91-
// If it's not a resource then don't ignore it
92-
if (i <= 0 || i == theUrl.length() - 1) {
93-
return false;
94-
}
95-
return ignored.contains(theUrl.substring(i + 1));
101+
return ignored.contains(theUrl);
96102
}
97103
}

‎app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/WeblogConfig.java‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import org.apache.roller.weblogger.business.WebloggerFactory;
3030
import org.apache.roller.weblogger.business.WeblogEntryManager;
3131
import org.apache.roller.weblogger.business.plugins.entry.WeblogEntryPlugin;
32+
import org.apache.roller.weblogger.config.WebloggerConfig;
3233
import org.apache.roller.weblogger.config.WebloggerRuntimeConfig;
3334
import org.apache.roller.weblogger.pojos.Weblog;
3435
import org.apache.roller.weblogger.pojos.WeblogCategory;
@@ -61,6 +62,8 @@ public class WeblogConfig extends UIAction {
6162

6263
// list of available plugins
6364
private List<WeblogEntryPlugin> pluginsList = Collections.emptyList();
65+
66+
private final boolean weblogAdminsUntrusted = WebloggerConfig.getBooleanProperty("weblogAdminsUntrusted");
6467

6568

6669
public WeblogConfig() {
@@ -71,7 +74,7 @@ public WeblogConfig() {
7174

7275
@Override
7376
public void myPrepare() {
74-
77+
7578
try {
7679
WeblogEntryManager wmgr = WebloggerFactory.getWeblogger().getWeblogEntryManager();
7780

@@ -88,10 +91,7 @@ public void myPrepare() {
8891
// set plugins list
8992
PluginManager ppmgr = WebloggerFactory.getWeblogger().getPluginManager();
9093
Map<String, WeblogEntryPlugin> pluginsMap = ppmgr.getWeblogEntryPlugins(getActionWeblog());
91-
List<WeblogEntryPlugin> plugins = new ArrayList<>();
92-
for (WeblogEntryPlugin entryPlugin : pluginsMap.values()) {
93-
plugins.add(entryPlugin);
94-
}
94+
List<WeblogEntryPlugin> plugins = new ArrayList<>(pluginsMap.values());
9595

9696
// sort
9797
setPluginsList(plugins);
@@ -231,5 +231,8 @@ public List<WeblogEntryPlugin> getPluginsList() {
231231
public void setPluginsList(List<WeblogEntryPlugin> pluginsList) {
232232
this.pluginsList = pluginsList;
233233
}
234-
234+
235+
public boolean getWeblogAdminsUntrusted() {
236+
return weblogAdminsUntrusted;
237+
}
235238
}

‎app/src/main/resources/ApplicationResources.properties‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1759,6 +1759,7 @@ websiteSettings.formatting=Formatting
17591759
websiteSettings.spamPrevention=Spam Prevention
17601760
websiteSettings.ignoreUrls=List of words and regex expressions listed one per \
17611761
line to be added to the banned words list used to check comments, trackbacks and referrers.
1762+
websiteSettings.bannedWordsList=Words banned in comments (regex allowed)
17621763
websiteSettings.acceptedBannedwordslist=Accepted {0} string and {1} regex banned-words list rules
17631764
websiteSettings.error.processingBannedwordslist=Error processing banned-words list: {0}
17641765

‎app/src/main/webapp/WEB-INF/jsps/editor/WeblogConfig.jsp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,11 +130,11 @@
130130
<h3><s:text name="websiteSettings.spamPrevention"/></h3>
131131

132132
<s:textarea name="bean.bannedwordslist" rows="7" cols="40"
133-
label="%{getText('websiteSettings.analyticsTrackingCode')}"/>
133+
label="%{getText('websiteSettings.bannedWordsList')}"/>
134134

135135
<%-- ***** Web analytics settings ***** --%>
136136

137-
<s:if test="getBooleanProp('analytics.code.override.allowed')">
137+
<s:if test="getBooleanProp('analytics.code.override.allowed') && !weblogAdminsUntrusted">
138138
<h3><s:text name="configForm.webAnalytics"/></h3>
139139

140140
<s:textarea name="bean.analyticsCode" rows="10" cols="70"

‎app/src/test/java/org/apache/roller/weblogger/ui/core/filters/LoadSaltFilterTest.java‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
package org.apache.roller.weblogger.ui.core.filters;
2+
3+
import org.apache.roller.weblogger.pojos.User;
4+
import org.apache.roller.weblogger.ui.core.RollerSession;
5+
import org.apache.roller.weblogger.ui.rendering.util.cache.SaltCache;
6+
import org.junit.jupiter.api.BeforeEach;
7+
import org.junit.jupiter.api.Test;
8+
import org.mockito.Mock;
9+
import org.mockito.MockedStatic;
10+
import org.mockito.MockitoAnnotations;
11+
12+
import javax.servlet.FilterChain;
13+
import javax.servlet.http.HttpServletRequest;
14+
import javax.servlet.http.HttpServletResponse;
15+
16+
import static org.mockito.Mockito.*;
17+
18+
public class LoadSaltFilterTest {
19+
20+
private LoadSaltFilter filter;
21+
22+
@Mock
23+
private HttpServletRequest request;
24+
25+
@Mock
26+
private HttpServletResponse response;
27+
28+
@Mock
29+
private FilterChain chain;
30+
31+
@Mock
32+
private RollerSession rollerSession;
33+
34+
@Mock
35+
private SaltCache saltCache;
36+
37+
@BeforeEach
38+
public void setUp() {
39+
MockitoAnnotations.initMocks(this);
40+
filter = new LoadSaltFilter();
41+
}
42+
43+
@Test
44+
public void testDoFilterGeneratesSalt() throws Exception {
45+
try (MockedStatic<RollerSession> mockedRollerSession = mockStatic(RollerSession.class);
46+
MockedStatic<SaltCache> mockedSaltCache = mockStatic(SaltCache.class)) {
47+
48+
mockedRollerSession.when(() -> RollerSession.getRollerSession(request)).thenReturn(rollerSession);
49+
mockedSaltCache.when(SaltCache::getInstance).thenReturn(saltCache);
50+
51+
when(rollerSession.getAuthenticatedUser()).thenReturn(new TestUser("userId"));
52+
53+
filter.doFilter(request, response, chain);
54+
55+
verify(request).setAttribute(eq("salt"), anyString());
56+
verify(saltCache).put(anyString(), eq("userId"));
57+
verify(chain).doFilter(request, response);
58+
}
59+
}
60+
61+
@Test
62+
public void testDoFilterWithNullRollerSession() throws Exception {
63+
try (MockedStatic<RollerSession> mockedRollerSession = mockStatic(RollerSession.class);
64+
MockedStatic<SaltCache> mockedSaltCache = mockStatic(SaltCache.class)) {
65+
66+
mockedRollerSession.when(() -> RollerSession.getRollerSession(request)).thenReturn(null);
67+
mockedSaltCache.when(SaltCache::getInstance).thenReturn(saltCache);
68+
69+
filter.doFilter(request, response, chain);
70+
71+
verify(request, never()).setAttribute(eq("salt"), anyString());
72+
verify(saltCache, never()).put(anyString(), anyString());
73+
verify(chain).doFilter(request, response);
74+
}
75+
}
76+
77+
private static class TestUser extends User {
78+
private final String id;
79+
80+
TestUser(String id) {
81+
this.id = id;
82+
}
83+
84+
public String getId() {
85+
return id;
86+
}
87+
}
88+
}

0 commit comments

Comments
 (0)