Fixed bug: srcset element joining missing space.

The string

    /url1, /url2, /url3

have two invalid URLs since the comma attaches to the URL.

When there is no metadata this should be rendered

    /url1 , /url2 , /url3

In practice this doesn't matter for non-malicious code
since the only URL that has no metadata should be the last,
fallback URL; and there's no attack scenario where appending
a comma to the end of an attacker controlled URL helps the
attacker.

This commit uses " , " to separate URL&metadata pairs in
srcset attributes.

It also adds tests for corner cases of the srcset parser and
valid floating point number parser.

Author: mikesamuel

src/main/java/org/owasp/html/SrcsetAttributePolicy.java

@@ -102,7 +102,7 @@ public String apply(String elementName, String attributeName, String value) {
             elementName, "src", value.substring(urlStart, urlEnd));
         if (okUrl != null && !okUrl.isEmpty()) {
           if (sb.length() != 0) {
-            sb.append(", ");
+            sb.append(" , ");
           }
           sb.append(okUrl.replace(",", "%2c"));
           if (metadataStart < metadataEnd) {

src/main/java/org/owasp/html/Strings.java

@@ -28,8 +28,6 @@
 
 package org.owasp.html;
 
-import javax.annotation.Nullable;
-
 /**
  * Locale independent versions of String case-insensitive operations.
  * <p>
@@ -52,6 +50,7 @@
  * @author Mike Samuel (mikesamuel@gmail.com)
  */
 final class Strings {
+  /*
   public static boolean equalsIgnoreCase(
       @Nullable String a, @Nullable String b) {
     if (a == null) { return b == null; }
@@ -71,6 +70,7 @@ public static boolean equalsIgnoreCase(
     }
     return true;
   }
+  */
 
   public static boolean regionMatchesIgnoreCase(
       CharSequence a, int aoffset, CharSequence b, int boffset, int n) {
@@ -90,6 +90,7 @@ public static boolean regionMatchesIgnoreCase(
   }
 
   /** True iff {@code s.equals(String.toLowerCase(s))}. */
+  /*
   public static boolean isLowerCase(CharSequence s) {
     for (int i = s.length(); --i >= 0;) {
       char c = s.charAt(i);
@@ -99,6 +100,7 @@ public static boolean isLowerCase(CharSequence s) {
     }
     return true;
   }
+  */
 
   private static final char[] LCASE_CHARS = new char['Z' + 1];
   private static final char[] UCASE_CHARS = new char['z' + 1];
@@ -126,6 +128,7 @@ public static String toLowerCase(String s) {
     return s;
   }
 
+  /*
   public static String toUpperCase(String s) {
     for (int i = s.length(); --i >= 0;) {
       char c = s.charAt(i);
@@ -143,7 +146,7 @@ public static String toUpperCase(String s) {
     }
     return s;
   }
-
+  */
 
   private static final long HTML_SPACE_CHAR_BITMASK =
       (1L << ' ')
@@ -202,13 +205,13 @@ static int skipValidFloatingPointNumber(String value, int start) {
       ++i;
     }
     // 2. One or both of the following, in the given order:
-    boolean hasIntegerPart = false;
+    boolean hasMantissa = false;
     //    1. A series of one or more ASCII digits.
     while (i < n) {
       char ch = value.charAt(i);
       if ('0' <= ch && ch <= '9') {
         ++i;
-        hasIntegerPart = true;
+        hasMantissa = true;
       } else {
         break;
       }
@@ -218,17 +221,19 @@ static int skipValidFloatingPointNumber(String value, int start) {
     //       2. A series of one or more ASCII digits.
     if (i < n && value.charAt(i) == '.') {
       ++i;
+      // Even if there's an integer, you need digits after the decimal point.
+      hasMantissa = false;
       while (i < n) {
         char ch = value.charAt(i);
         if ('0' <= ch && ch <= '9') {
           ++i;
-          hasIntegerPart = true;
+          hasMantissa = true;
         } else {
           break;
         }
       }
     }
-    if (!hasIntegerPart) {
+    if (!hasMantissa) {
       return -1;
     }
     // 3. Optionally:

src/test/java/org/owasp/html/HtmlPolicyBuilderTest.java

@@ -360,10 +360,14 @@ public static final void testImgSrcsetSyntax() {
         + "<img srcset=\"http://example.com/foo.png 48x\" />\n"
         + "<img srcset=\"http://example.com/foo.png .123x\" />\n"
         + "<img srcset=\"http://example.com/foo.png .123e2x\" />\n"
-        + "<img srcset=\"http://example.com/foo.png 123.456x\" />\n"
-        + "<img srcset=\"/big.png 64w, /little.png\" />\n"
-        + "<img srcset=\"/big.png 64w, /little.png\" />\n"
-        + "<img srcset=\"/big.png 64w, /little.png\" />\n"
+        + "<img srcset=\"http://example.com/foo.png 123.456E-1x\" />\n"
+        + "<img srcset=\"http://example.com/foo.png -123x\" />\n"
+        + "no float: \n"
+        + "no fraction: \n"
+        + "no exponent: \n"
+        + "<img srcset=\"/big.png 64w , /little.png\" />\n"
+        + "<img srcset=\"/big.png 64w , /little.png\" />\n"
+        + "<img srcset=\"/big.png 64w , /little.png\" />\n"
         + "<img srcset=\"foo%2cbar.png\" />\n"
         + "empty: \n"
         + "only space: \n"
@@ -386,7 +390,11 @@ public static final void testImgSrcsetSyntax() {
             + "<img srcset=\"http://example.com/foo.png 48x\" />\n"
             + "<img srcset=\"http://example.com/foo.png .123x\" />\n"
             + "<img srcset=\"http://example.com/foo.png .123e2x\" />\n"
-            + "<img srcset=\"http://example.com/foo.png 123.456x\" />\n"
+            + "<img srcset=\"http://example.com/foo.png 123.456E-1x\" />\n"
+            + "<img srcset=\"http://example.com/foo.png -123x\" />\n"
+            + "no float: <img srcset=\"http://example.com/foo.png -x\" />\n"
+            + "no fraction: <img srcset=\"http://example.com/foo.png -.e1\" />\n"
+            + "no exponent: <img srcset=\"http://example.com/foo.png -1e+x\" />\n"
             + "<img srcset=\"/big.png 64w, /little.png\" />\n"
             + "<img srcset=\" /big.png 64w , /little.png\" />\n"
             + "<img srcset=\"\t\t/big.png 64w\r\n,/little.png\t\t\" />\n"

src/test/java/org/owasp/html/StringsTest.java

@@ -0,0 +1,50 @@
+package org.owasp.html;
+
+import org.junit.Test;
+
+import junit.framework.TestCase;
+
+@SuppressWarnings({ "javadoc" })
+public final class StringsTest extends TestCase {
+
+  @Test
+  public static void testValidFloatingPointNumber() {
+    assertEquals(
+        -1,
+        Strings.skipValidFloatingPointNumber("", 0));
+    assertEquals(
+        3,
+        Strings.skipValidFloatingPointNumber("123", 0));
+    assertEquals(
+        7,
+        Strings.skipValidFloatingPointNumber("123.456", 0));
+    assertEquals(
+        7,
+        Strings.skipValidFloatingPointNumber("123.456f", 0));
+    assertEquals(
+        8,
+        Strings.skipValidFloatingPointNumber(" 123.456 ", 1));
+    assertEquals(
+        -1,
+        Strings.skipValidFloatingPointNumber("1e", 0));
+    assertEquals(
+        -1,
+        Strings.skipValidFloatingPointNumber("1.", 0));
+    assertEquals(
+        -1,
+        Strings.skipValidFloatingPointNumber("1e+", 0));
+    assertEquals(
+        -1,
+        Strings.skipValidFloatingPointNumber("1e+e", 0));
+    assertEquals(
+        5,
+        Strings.skipValidFloatingPointNumber(" 1E-1", 1));
+    assertEquals(
+        5,
+        Strings.skipValidFloatingPointNumber(" 1E+2 ", 1));
+    assertEquals(
+        5,
+        Strings.skipValidFloatingPointNumber(" 1E+2,", 1));
+  }
+
+}