Decode percent-encoded characters in search parameter.task-24384-2

So far we only decoded percent-encoded characters in other parameters
than the search parameter. Now we're using our own code for parsing
the query string into a parameter map. This also gives us more control
over whether or not to decode the + sign to a space sign, which we're
not doing anymore with this patch.

Fixes #24384.

3 files changed, 83 insertions, 24 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d5877d3..a72fbc7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,12 @@
 # Changes in version 8.0-1.2?.? - 2020-0?-??
 
+ * Medium changes
+
+   - Decode percent-encoded characters in the search parameter in the
+     same way as in the other parameters.
+   - Stop decoding a + sign to a space character in any of the
+     parameters.
+
 
 # Changes in version 8.0-1.25.0 - 2020-02-20
 
diff --git a/src/main/java/org/torproject/metrics/onionoo/server/ResourceServlet.java b/src/main/java/org/torproject/metrics/onionoo/server/ResourceServlet.java
index 4983a31..ea52c47 100644
--- a/src/main/java/org/torproject/metrics/onionoo/server/ResourceServlet.java
+++ b/src/main/java/org/torproject/metrics/onionoo/server/ResourceServlet.java
@@ -9,6 +9,8 @@ import org.apache.commons.lang3.StringUtils;
 
 import java.io.IOException;
 import java.io.PrintWriter;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
@@ -16,7 +18,6 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import javax.servlet.ServletConfig;
@@ -134,13 +135,15 @@ public class ResourceServlet extends HttpServlet {
     RequestHandler rh = new RequestHandler(nodeIndex);
     rh.setResourceType(resourceType);
 
-    /* Extract parameters either from the old-style URI or from request
-     * parameters. */
-    Map<String, String> parameterMap = new HashMap<>();
-    for (String parameterKey : request.getParameterMap().keySet()) {
-      String[] parameterValues =
-          request.getParameterValues(parameterKey);
-      parameterMap.put(parameterKey, parameterValues[0]);
+    /* Parse request parameters from the query string using our own code rather
+     * than relying on HttpServletRequest#getParameterMap(), because we want
+     * percent-encoded characters to be decoded in all parameters and don't want
+     * the + sign to be decoded to the space sign in any of our parameters. */
+    Map<String, String> parameterMap
+        = this.parseQueryString(request.getQueryString());
+    if (null == parameterMap) {
+      response.sendError(HttpServletResponse.SC_BAD_REQUEST);
+      return;
     }
 
     /* Make sure that the request doesn't contain any unknown
@@ -154,8 +157,7 @@ public class ResourceServlet extends HttpServlet {
 
     /* Filter relays and bridges matching the request. */
     if (parameterMap.containsKey("search")) {
-      String[] searchTerms = parseSearchParameters(
-          request.getQueryString());
+      String[] searchTerms = parseSearchParameters(parameterMap.get("search"));
       if (searchTerms == null) {
         response.sendError(HttpServletResponse.SC_BAD_REQUEST);
         return;
@@ -426,11 +428,6 @@ public class ResourceServlet extends HttpServlet {
         bridgeDocumentsWritten, charsWritten, writtenResponseMillis);
   }
 
-  private static Pattern searchQueryStringPattern =
-      Pattern.compile("(?:.*[?&])*?" // lazily skip other parameters
-          + "search=([\\p{Graph} &&[^&]]+)" // capture parameter
-          + "(?:&.*)*"); // skip remaining parameters
-
   private static Pattern searchParameterPattern =
       Pattern.compile("^\\$?[0-9a-fA-F]{1,40}$|" /* Hex fingerprint. */
       + "^[0-9a-zA-Z+/]{1,27}$|" /* Base64 fingerprint. */
@@ -438,16 +435,44 @@ public class ResourceServlet extends HttpServlet {
       + ipv6AddressPatternString + "|" /* IPv6 address. */
       + "^[a-zA-Z_]+:\"?[\\p{Graph} ]+\"?$"); /* Qualified search term. */
 
-  protected static String[] parseSearchParameters(String queryString) {
-    Matcher searchQueryStringMatcher = searchQueryStringPattern.matcher(
-        queryString);
-    if (!searchQueryStringMatcher.matches()) {
-      /* Search query contains illegal character(s). */
-      return null;
+  /**
+   * Parse the given query string and return a map with parameter keys and first
+   * encountered parameter values.
+   *
+   * @param queryString Query string obtained from the request.
+   * @return Map with parameter keys and values.
+   */
+  protected Map<String, String> parseQueryString(String queryString) {
+    Map<String, String> parameterMap = new HashMap<>();
+    if (null != queryString) {
+      for (String parameter : queryString.split("&")) {
+        String[] parameterParts = parameter.split("=", 2);
+        if (parameterParts.length < 2) {
+          /* We don't support parameter keys without values. */
+          return null;
+        }
+        String parameterKey = parameterParts[0];
+        if (parameterMap.containsKey(parameterKey)) {
+          /* Only keep the first parameter value for any given parameter key. */
+          continue;
+        }
+        String parameterValue = parameterParts[1];
+        try {
+          /* Percent-encode the plus sign before decoding it to avoid it being
+           * decoded to the space sign. */
+          String percentDecodedParameterValue = URLDecoder.decode(
+              parameterValue.replaceAll("\\+", "%2B"), "UTF-8");
+          parameterMap.put(parameterKey, percentDecodedParameterValue);
+        } catch (UnsupportedEncodingException e) {
+          return null;
+        }
+      }
     }
-    String parameter = searchQueryStringMatcher.group(1);
-    String[] spaceSeparatedParts =
-        parameter.replaceAll("%20", " ").split(" ");
+    return parameterMap;
+  }
+
+  protected static String[] parseSearchParameters(String parameter) {
+    String[] spaceSeparatedParts = parameter.split(" ");
     List<String> searchParameters = new ArrayList<>();
     StringBuilder doubleQuotedSearchTerm = null;
     for (String spaceSeparatedPart : spaceSeparatedParts) {
diff --git a/src/test/java/org/torproject/metrics/onionoo/server/ResourceServletTest.java b/src/test/java/org/torproject/metrics/onionoo/server/ResourceServletTest.java
index 4cbb370..d655ce8 100644
--- a/src/test/java/org/torproject/metrics/onionoo/server/ResourceServletTest.java
+++ b/src/test/java/org/torproject/metrics/onionoo/server/ResourceServletTest.java
@@ -640,6 +640,13 @@ public class ResourceServletTest {
   }
 
   @Test(timeout = 100)
+  public void testSearchBase64FingerprintPlusEncoded() {
+    this.assertSummaryDocument(
+        "/summary?search=ACXBNsHzqe7%2BKuP5GPA7+iG1Bws", 1,
+        new String[] { "TimMayTribute" }, 0, null);
+  }
+
+  @Test(timeout = 100)
   public void testSearchBase64FingerprintBridge() {
     this.assertSummaryDocument(
         "/summary?search=AACDGyNt/3PUCa0XtA4qcopTmU8", 0, null, 0, null);
@@ -678,6 +685,12 @@ public class ResourceServletTest {
   }
 
   @Test(timeout = 100)
+  public void testSearchPlusSeparatedFingerprintLastEight() {
+    this.assertSummaryDocument(
+        "/summary?search=F1A3+B33C", 0, null, 0, null);
+  }
+
+  @Test(timeout = 100)
   public void testSearchSpaceSeparatedFingerprintLastThree() {
     this.assertSummaryDocument(
         "/summary?search=33C", 0, null, 0, null);
@@ -949,6 +962,13 @@ public class ResourceServletTest {
   }
 
   @Test(timeout = 100)
+  public void testSearchEmailAddressEncodedPlus() {
+    this.assertSummaryDocument(
+        "/summary?search=contact:<tor%2Bsteven.murdoch@cl.cam.ac.uk>", 1,
+        new String[] { "TimMayTribute" }, 0, null);
+  }
+
+  @Test(timeout = 100)
   public void testSearchDoubleQuotedEmailAddress() {
     this.assertSummaryDocument(
         "/summary?search=contact:\"klaus dot zufall at gmx dot de\"", 1,
@@ -956,6 +976,13 @@ public class ResourceServletTest {
   }
 
   @Test(timeout = 100)
+  public void testSearchDoubleQuotedEmailAddressReversed() {
+    this.assertSummaryDocument(
+        "/summary?search=contact:\"de dot gmx at zufall dot klaus\"", 1,
+        new String[] { "TorkaZ" }, 0, null);
+  }
+
+  @Test(timeout = 100)
   public void testSearchDoubleQuotedContactAndNickname() {
     this.assertSummaryDocument(
         "/summary?search=contact:\"dot de\" TorkaZ", 1,