Skip to content

Commit d26fc5a

Browse files
idodeclareidodeclare
committed
Address review feedback
1 parent 20463cf commit d26fc5a

File tree

9 files changed

+173
-61
lines changed

9 files changed

+173
-61
lines changed

‎opengrok-indexer/src/main/java/org/opengrok/indexer/authorization/AuthorizationFramework.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
import org.opengrok.indexer.configuration.RuntimeEnvironment;
3939
import org.opengrok.indexer.framework.PluginFramework;
4040
import org.opengrok.indexer.logger.LoggerFactory;
41-
import org.opengrok.indexer.web.LaunderUtil;
41+
import org.opengrok.indexer.web.Laundromat;
4242
import org.opengrok.indexer.web.Statistics;
4343

4444
/**
@@ -486,7 +486,7 @@ private boolean checkAll(HttpServletRequest request, String cache, Nameable enti
486486

487487
if (entity == null) {
488488
LOGGER.log(Level.WARNING, "entity was null for request with parameters: {}",
489-
LaunderUtil.logging(request.getParameterMap()));
489+
Laundromat.launderLog(request.getParameterMap()));
490490
return false;
491491
}
492492

‎opengrok-indexer/src/main/java/org/opengrok/indexer/index/PendingFileCompleter.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -554,6 +554,7 @@ private void findFilelessChildren(SkeletonDirs skels, File directory) {
554554

555555
/**
556556
* Counts segments arising from {@code File.separatorChar} or '\\'.
557+
* @param path a defined instance
557558
* @return a natural number
558559
*/
559560
private static int countPathSegments(String path) {

‎opengrok-indexer/src/main/java/org/opengrok/indexer/web/LaunderUtil.java renamed to ‎opengrok-indexer/src/main/java/org/opengrok/indexer/web/Laundromat.java

Lines changed: 38 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -25,37 +25,35 @@
2525

2626
import java.util.HashMap;
2727
import java.util.Map;
28+
import java.util.stream.Collectors;
2829

2930
/**
3031
* Represents a container for sanitizing methods for avoiding classifications as
3132
* taint bugs.
3233
*/
33-
public class LaunderUtil {
34+
public class Laundromat {
35+
36+
private static final String ESC_N_R_T_F = "[\\n\\r\\t\\f]";
37+
private static final String ESG_N_R_T_F__1_n = ESC_N_R_T_F + "+";
3438

3539
/**
3640
* Sanitize {@code value} where it will be used in subsequent OpenGrok
3741
* (non-logging) processing.
3842
* @return {@code null} if null or else {@code value} with "pattern-breaking
3943
* characters" (tabs, CR, LF, FF) replaced as underscores (one for one)
4044
*/
41-
public static String userInput(String value) {
42-
if (value == null) {
43-
return null;
44-
}
45-
return value.replaceAll("[\\n\\r\\t\\f]", "_");
45+
public static String launderInput(String value) {
46+
return replaceAll(value, ESC_N_R_T_F, "_");
4647
}
4748

4849
/**
49-
* Sanitize {@code query} where it will be used in a Lucene query.
50-
* @return {@code null} if null or else {@code query} with "pattern-breaking
50+
* Sanitize {@code value} where it will be used in a Lucene query.
51+
* @return {@code null} if null or else {@code value} with "pattern-breaking
5152
* characters" (tabs, CR, LF, FF) replaced as spaces. Contiguous matches are
5253
* replaced with one space.
5354
*/
54-
public static String luceneQuery(String query) {
55-
if (query == null) {
56-
return null;
57-
}
58-
return query.replaceAll("[\\n\\r\\t\\f]+", " ");
55+
public static String launderQuery(String value) {
56+
return replaceAll(value, ESG_N_R_T_F__1_n, " ");
5957
}
6058

6159
/**
@@ -64,7 +62,7 @@ public static String luceneQuery(String query) {
6462
* characters" tabs, CR, LF, and FF replaced as {@code "<TAB>"},
6563
* {@code "<CR>"}, {@code "<LF>"}, and {@code "<FF>"} resp.
6664
*/
67-
public static String logging(String value) {
65+
public static String launderLog(String value) {
6866
if (value == null) {
6967
return null;
7068
}
@@ -77,39 +75,54 @@ public static String logging(String value) {
7775
/**
7876
* Sanitize {@code map} where it will be used in a log message only.
7977
* @return {@code null} if null or else {@code map} with keys and values
80-
* sanitized with {@link #logging(String)}. If the sanitizing causes key
78+
* sanitized with {@link #launderLog(String)}. If the sanitizing causes key
8179
* collisions, the colliding keys' values are combined.
8280
*/
83-
public static Map<String, String[]> logging(Map<String, String[]> map) {
81+
public static Map<String, String[]> launderLog(Map<String, String[]> map) {
8482
if (map == null) {
8583
return null;
8684
}
8785

8886
HashMap<String, String[]> safes = new HashMap<>();
89-
for (Map.Entry<String, String[]> entry : map.entrySet()) {
90-
String k = logging(entry.getKey());
91-
String[] safeValues = safes.get(k);
87+
for (Map.Entry<String, String[]> entry : map.entrySet().stream().sorted(
88+
Map.Entry.comparingByKey()).collect(Collectors.toList())) {
89+
String key = launderLog(entry.getKey());
90+
String[] safeValues = safes.get(key);
9291
String[] fullySafe = mergeLogArrays(entry.getValue(), safeValues);
93-
safes.put(k, fullySafe);
92+
safes.put(key, fullySafe);
9493
}
9594
return safes;
9695
}
9796

9897
private static String[] mergeLogArrays(String[] values, String[] safeValues) {
99-
int n = values.length + (safeValues != null ? safeValues.length : 0);
98+
if (values == null && safeValues == null) {
99+
return null;
100+
}
101+
102+
int n = (values != null ? values.length : 0) +
103+
(safeValues != null ? safeValues.length : 0);
100104
String[] result = new String[n];
101105

102-
int i;
103-
for (i = 0; i < values.length; ++i) {
104-
result[i] = logging(values[i]);
106+
int i = 0;
107+
if (values != null) {
108+
for (; i < values.length; ++i) {
109+
result[i] = launderLog(values[i]);
110+
}
105111
}
106112
if (safeValues != null) {
107113
System.arraycopy(safeValues, 0, result, i, safeValues.length);
108114
}
109115
return result;
110116
}
111117

118+
private static String replaceAll(String value, String regex, String replacement) {
119+
if (value == null) {
120+
return null;
121+
}
122+
return value.replaceAll(regex, replacement);
123+
}
124+
112125
/* private to enforce static */
113-
private LaunderUtil() {
126+
private Laundromat() {
114127
}
115128
}

‎opengrok-indexer/src/main/java/org/opengrok/indexer/web/PageConfig.java

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -618,12 +618,12 @@ public List<SortOrder> getSortOrder() {
618618
public QueryBuilder getQueryBuilder() {
619619
if (queryBuilder == null) {
620620
queryBuilder = new QueryBuilder().
621-
setFreetext(LaunderUtil.luceneQuery(req.getParameter(QueryBuilder.FULL)))
622-
.setDefs(LaunderUtil.luceneQuery(req.getParameter(QueryBuilder.DEFS)))
623-
.setRefs(LaunderUtil.luceneQuery(req.getParameter(QueryBuilder.REFS)))
624-
.setPath(LaunderUtil.luceneQuery(req.getParameter(QueryBuilder.PATH)))
625-
.setHist(LaunderUtil.luceneQuery(req.getParameter(QueryBuilder.HIST)))
626-
.setType(LaunderUtil.luceneQuery(req.getParameter(QueryBuilder.TYPE)));
621+
setFreetext(Laundromat.launderQuery(req.getParameter(QueryBuilder.FULL)))
622+
.setDefs(Laundromat.launderQuery(req.getParameter(QueryBuilder.DEFS)))
623+
.setRefs(Laundromat.launderQuery(req.getParameter(QueryBuilder.REFS)))
624+
.setPath(Laundromat.launderQuery(req.getParameter(QueryBuilder.PATH)))
625+
.setHist(Laundromat.launderQuery(req.getParameter(QueryBuilder.HIST)))
626+
.setType(Laundromat.launderQuery(req.getParameter(QueryBuilder.TYPE)));
627627
}
628628

629629
return queryBuilder;
@@ -684,7 +684,8 @@ public String getDefineTagsIndex() {
684684
*/
685685
public String getRequestedRevision() {
686686
if (rev == null) {
687-
String tmp = LaunderUtil.userInput(req.getParameter(QueryParameters.REVISION_PARAM));
687+
String tmp = Laundromat.launderInput(
688+
req.getParameter(QueryParameters.REVISION_PARAM));
688689
rev = (tmp != null && tmp.length() > 0) ? tmp : "";
689690
}
690691
return rev;
@@ -1101,7 +1102,8 @@ public Prefix getPrefix() {
11011102
*/
11021103
public String getPath() {
11031104
if (path == null) {
1104-
path = Util.getCanonicalPath(LaunderUtil.userInput(req.getPathInfo()), PATH_SEPARATOR);
1105+
path = Util.getCanonicalPath(Laundromat.launderInput(
1106+
req.getPathInfo()), PATH_SEPARATOR);
11051107
if (PATH_SEPARATOR_STRING.equals(path)) {
11061108
path = "";
11071109
}

‎opengrok-indexer/src/test/java/org/opengrok/indexer/web/LaundromatTest.java

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
/*
2+
* CDDL HEADER START
3+
*
4+
* The contents of this file are subject to the terms of the
5+
* Common Development and Distribution License (the "License").
6+
* You may not use this file except in compliance with the License.
7+
*
8+
* See LICENSE.txt included in this distribution for the specific
9+
* language governing permissions and limitations under the License.
10+
*
11+
* When distributing Covered Code, include this CDDL HEADER in each
12+
* file and include the License file at LICENSE.txt.
13+
* If applicable, add the following below this CDDL HEADER, with the
14+
* fields enclosed by brackets "[]" replaced with your own identifying
15+
* information: Portions Copyright [yyyy] [name of copyright owner]
16+
*
17+
* CDDL HEADER END
18+
*/
19+
20+
/*
21+
* Copyright (c) 2020, Chris Fraire <[email protected]>.
22+
*/
23+
24+
package org.opengrok.indexer.web;
25+
26+
import org.junit.Test;
27+
28+
import static org.junit.Assert.assertEquals;
29+
30+
import java.util.Arrays;
31+
import java.util.HashMap;
32+
import java.util.Map;
33+
34+
public class LaundromatTest {
35+
36+
private static final String TEST_CONTENT = "\n\r\f\tContent\r\nConclusion\f\t";
37+
private static final String TEST_CONTENT_LOG_LAUNDRY =
38+
"<LF><CR><FF><TAB>Content<CR><LF>Conclusion<FF><TAB>";
39+
40+
@Test
41+
public void launderInput() {
42+
String laundry = Laundromat.launderInput(TEST_CONTENT);
43+
assertEquals("____Content__Conclusion__", laundry);
44+
}
45+
46+
@Test
47+
public void launderQuery() {
48+
String laundry = Laundromat.launderQuery(TEST_CONTENT);
49+
assertEquals(" Content Conclusion ", laundry);
50+
}
51+
52+
@Test
53+
public void launderLog() {
54+
String laundry = Laundromat.launderLog(TEST_CONTENT);
55+
assertEquals(TEST_CONTENT_LOG_LAUNDRY, laundry);
56+
}
57+
58+
@Test
59+
public void launderLogMap() {
60+
HashMap<String, String[]> testMap = new HashMap<>();
61+
testMap.put("a", null);
62+
testMap.put("b", new String[]{TEST_CONTENT});
63+
testMap.put(TEST_CONTENT, new String[]{"c", "d"});
64+
// for merging two non-null collisions
65+
testMap.put("e\rf", new String[]{"c", "d"});
66+
testMap.put("e<CR>f", new String[]{"g", "h"});
67+
// for merging LHS null on collisions
68+
testMap.put("\fi\n", null);
69+
testMap.put("<FF>i\n", new String[]{"j"});
70+
testMap.put("<FF>i<LF>", new String[]{"k"});
71+
// for merging RHS null on collisions
72+
testMap.put("l\t\r", null);
73+
testMap.put("l<TAB>\r", null);
74+
testMap.put("l\t<CR>", null);
75+
76+
Map<String, String[]> laundry = Laundromat.launderLog(testMap);
77+
78+
HashMap<String, String[]> expected = new HashMap<>();
79+
expected.put("a", null);
80+
expected.put("b", new String[]{TEST_CONTENT_LOG_LAUNDRY});
81+
expected.put(TEST_CONTENT_LOG_LAUNDRY, new String[]{"c", "d"});
82+
expected.put("e<CR>f", new String[]{"g", "h", "c", "d"});
83+
expected.put("<FF>i<LF>", new String[]{"k", "j"});
84+
expected.put("l<TAB><CR>", null);
85+
86+
assertEquals("maps″ should be equal", hashedValues(expected), hashedValues(laundry));
87+
}
88+
89+
private Map<String, Integer> hashedValues(Map<String, String[]> map) {
90+
HashMap<String, Integer> result = new HashMap<>();
91+
for (Map.Entry<String, String[]> entry : map.entrySet()) {
92+
result.put(entry.getKey(), Arrays.hashCode(entry.getValue()));
93+
}
94+
return result;
95+
}
96+
}

‎opengrok-web/src/main/java/org/opengrok/web/AuthorizationFilter.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
import org.opengrok.indexer.configuration.Project;
3838
import org.opengrok.indexer.logger.LoggerFactory;
3939
import org.opengrok.indexer.web.PageConfig;
40-
import org.opengrok.indexer.web.LaunderUtil;
40+
import org.opengrok.indexer.web.Laundromat;
4141
import org.opengrok.web.api.v1.RestApp;
4242

4343
public class AuthorizationFilter implements Filter {
@@ -59,7 +59,7 @@ public void doFilter(ServletRequest sr, ServletResponse sr1, FilterChain fc) thr
5959
if (httpReq.getServletPath().startsWith(RestApp.API_PATH)) {
6060
if (LOGGER.isLoggable(Level.FINER)) {
6161
LOGGER.log(Level.FINER, "Allowing request to {0} in {1}",
62-
new Object[] {LaunderUtil.logging(httpReq.getServletPath()),
62+
new Object[] {Laundromat.launderLog(httpReq.getServletPath()),
6363
AuthorizationFilter.class.getName()});
6464
}
6565
fc.doFilter(sr, sr1);
@@ -74,11 +74,11 @@ public void doFilter(ServletRequest sr, ServletResponse sr1, FilterChain fc) thr
7474
if (LOGGER.isLoggable(Level.INFO)) {
7575
if (httpReq.getRemoteUser() != null) {
7676
LOGGER.log(Level.INFO, "Access denied for user ''{0}'' for URI: {1}",
77-
new Object[] {LaunderUtil.logging(httpReq.getRemoteUser()),
78-
LaunderUtil.logging(httpReq.getRequestURI())});
77+
new Object[] {Laundromat.launderLog(httpReq.getRemoteUser()),
78+
Laundromat.launderLog(httpReq.getRequestURI())});
7979
} else {
8080
LOGGER.log(Level.INFO, "Access denied for URI: {0}",
81-
LaunderUtil.logging(httpReq.getRequestURI()));
81+
Laundromat.launderLog(httpReq.getRequestURI()));
8282
}
8383
}
8484

0 commit comments

Comments
 (0)