Tighten definition of what's accepted as a URI, to exclude some potentially malicious ones

core/src/main/java/com/google/zxing/client/result/URIParsedResult.java

@@ -16,8 +16,6 @@
 
 package com.google.zxing.client.result;
 
-import java.util.regex.Pattern;
-
 /**
  * A simple result type encapsulating a URI that has no further interpretation.
  *
@@ -25,8 +23,6 @@ import java.util.regex.Pattern;
  */
 public final class URIParsedResult extends ParsedResult {
 
-  private static final Pattern USER_IN_HOST = Pattern.compile(":/*([^/@]+)@[^/]+");
-
   private final String uri;
   private final String title;
 
@@ -45,15 +41,11 @@ public final class URIParsedResult extends ParsedResult {
   }
 
   /**
-   * @return true if the URI contains suspicious patterns that may suggest it intends to
-   *  mislead the user about its true nature. At the moment this looks for the presence
-   *  of user/password syntax in the host/authority portion of a URI which may be used
-   *  in attempts to make the URI's host appear to be other than it is. Example:
-   *  http://yourbank.com@phisher.com  This URI connects to phisher.com but may appear
-   *  to connect to yourbank.com at first glance.
+   * @deprecated see {@link URIResultParser#isPossiblyMaliciousURI(String)}
    */
+  @Deprecated
   public boolean isPossiblyMaliciousURI() {
-    return USER_IN_HOST.matcher(uri).find();
+    return URIResultParser.isPossiblyMaliciousURI(uri);
   }
 
   @Override

core/src/main/java/com/google/zxing/client/result/URIResultParser.java

@@ -28,6 +28,9 @@ import java.util.regex.Pattern;
  */
 public final class URIResultParser extends ResultParser {
 
+  private static final Pattern ALLOWED_URI_CHARS_PATTERN =
+      Pattern.compile("[-._~:/?#\\[\\]@!$&'()*+,;=%A-Za-z0-9]+");
+  private static final Pattern USER_IN_HOST = Pattern.compile(":/*([^/@]+)@[^/]+");
   // See http://www.ietf.org/rfc/rfc2396.txt
   private static final Pattern URL_WITH_PROTOCOL_PATTERN = Pattern.compile("[a-zA-Z][a-zA-Z0-9+-.]+:");
   private static final Pattern URL_WITHOUT_PROTOCOL_PATTERN = Pattern.compile(
@@ -44,7 +47,22 @@ public final class URIResultParser extends ResultParser {
       return new URIParsedResult(rawText.substring(4).trim(), null);
     }
     rawText = rawText.trim();
-    return isBasicallyValidURI(rawText) ? new URIParsedResult(rawText, null) : null;
+    if (!isBasicallyValidURI(rawText) || isPossiblyMaliciousURI(rawText)) {
+      return null;
+    }
+    return new URIParsedResult(rawText, null);
+  }
+
+  /**
+   * @return true if the URI contains suspicious patterns that may suggest it intends to
+   *  mislead the user about its true nature. At the moment this looks for the presence
+   *  of user/password syntax in the host/authority portion of a URI which may be used
+   *  in attempts to make the URI's host appear to be other than it is. Example:
+   *  http://yourbank.com@phisher.com  This URI connects to phisher.com but may appear
+   *  to connect to yourbank.com at first glance.
+   */
+  static boolean isPossiblyMaliciousURI(String uri) {
+    return !ALLOWED_URI_CHARS_PATTERN.matcher(uri).matches() || USER_IN_HOST.matcher(uri).find();
   }
 
   static boolean isBasicallyValidURI(String uri) {

core/src/test/java/com/google/zxing/client/result/CalendarParsedResultTestCase.java

@@ -146,7 +146,7 @@ public final class CalendarParsedResultTestCase extends Assert {
         "GEO:-12.345\r\n" +
         "END:VEVENT\r\nEND:VCALENDAR", null, null, BarcodeFormat.QR_CODE);
     ParsedResult result = ResultParser.parseResult(fakeResult);
-    assertSame(ParsedResultType.URI, result.getType());
+    assertSame(ParsedResultType.TEXT, result.getType());
   }
 
   @Test

core/src/test/java/com/google/zxing/client/result/ParsedReaderResultTestCase.java

@@ -238,7 +238,7 @@ public final class ParsedReaderResultTestCase extends Assert {
     doTestResult("BEGIN:VEVENT\r\nSUMMARY:foo\r\nDTSTART:20080504\r\nEND:VEVENT",
         "foo\n" + formatDate(2008, 5, 4), ParsedResultType.CALENDAR);
     doTestResult("BEGIN:VEVENT\r\nDTEND:20080505T\r\nEND:VEVENT",
-        "BEGIN:VEVENT\r\nDTEND:20080505T\r\nEND:VEVENT", ParsedResultType.URI);
+        "BEGIN:VEVENT\r\nDTEND:20080505T\r\nEND:VEVENT", ParsedResultType.TEXT);
     // Yeah, it's OK that this is thought of as maybe a URI as long as it's not CALENDAR
     // Make sure illegal entries without newlines don't crash
     doTestResult(

core/src/test/java/com/google/zxing/client/result/URIParsedResultTestCase.java

@@ -93,6 +93,12 @@ public final class URIParsedResultTestCase extends Assert {
     doTestIsPossiblyMalicious("http://google.com/@@", false);
   }
 
+  @Test
+  public void testMaliciousUnicode() {
+    doTestIsPossiblyMalicious("https://google.com\u2215.evil.com/stuff", true);
+    doTestIsPossiblyMalicious("\u202ehttps://dylankatz.com/moc.elgoog.www//:sptth", true);
+  }
+
   @Test
   public void testExotic() {
     doTest("bitcoin:mySD89iqpmptrK3PhHFW9fa7BXiP7ANy3Y", "bitcoin:mySD89iqpmptrK3PhHFW9fa7BXiP7ANy3Y", null);
@@ -124,9 +130,10 @@ public final class URIParsedResultTestCase extends Assert {
     assertEquals(text, result.getDisplayResult());
   }
 
-  private static void doTestIsPossiblyMalicious(String uri, boolean expected) {
-    URIParsedResult result = new URIParsedResult(uri, null);
-    assertEquals(expected, result.isPossiblyMaliciousURI());
+  private static void doTestIsPossiblyMalicious(String uri, boolean malicious) {
+    Result fakeResult = new Result(uri, null, null, BarcodeFormat.QR_CODE);
+    ParsedResult result = ResultParser.parseResult(fakeResult);
+    assertSame(malicious ? ParsedResultType.TEXT : ParsedResultType.URI, result.getType());
   }
 
 }