Skip to content

Commit c983b9c

Browse files
mikesamuelmikesamuel
committed
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.
1 parent 3ec5c20 commit c983b9c

File tree

4 files changed

+76
-13
lines changed

4 files changed

+76
-13
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ public String apply(String elementName, String attributeName, String value) {
102102
elementName, "src", value.substring(urlStart, urlEnd));
103103
if (okUrl != null && !okUrl.isEmpty()) {
104104
if (sb.length() != 0) {
105-
sb.append(", ");
105+
sb.append(" , ");
106106
}
107107
sb.append(okUrl.replace(",", "%2c"));
108108
if (metadataStart < metadataEnd) {

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

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,6 @@
2828

2929
package org.owasp.html;
3030

31-
import javax.annotation.Nullable;
32-
3331
/**
3432
* Locale independent versions of String case-insensitive operations.
3533
* <p>
@@ -52,6 +50,7 @@
5250
* @author Mike Samuel ([email protected])
5351
*/
5452
final class Strings {
53+
/*
5554
public static boolean equalsIgnoreCase(
5655
@Nullable String a, @Nullable String b) {
5756
if (a == null) { return b == null; }
@@ -71,6 +70,7 @@ public static boolean equalsIgnoreCase(
7170
}
7271
return true;
7372
}
73+
*/
7474

7575
public static boolean regionMatchesIgnoreCase(
7676
CharSequence a, int aoffset, CharSequence b, int boffset, int n) {
@@ -90,6 +90,7 @@ public static boolean regionMatchesIgnoreCase(
9090
}
9191

9292
/** True iff {@code s.equals(String.toLowerCase(s))}. */
93+
/*
9394
public static boolean isLowerCase(CharSequence s) {
9495
for (int i = s.length(); --i >= 0;) {
9596
char c = s.charAt(i);
@@ -99,6 +100,7 @@ public static boolean isLowerCase(CharSequence s) {
99100
}
100101
return true;
101102
}
103+
*/
102104

103105
private static final char[] LCASE_CHARS = new char['Z' + 1];
104106
private static final char[] UCASE_CHARS = new char['z' + 1];
@@ -126,6 +128,7 @@ public static String toLowerCase(String s) {
126128
return s;
127129
}
128130

131+
/*
129132
public static String toUpperCase(String s) {
130133
for (int i = s.length(); --i >= 0;) {
131134
char c = s.charAt(i);
@@ -143,7 +146,7 @@ public static String toUpperCase(String s) {
143146
}
144147
return s;
145148
}
146-
149+
*/
147150

148151
private static final long HTML_SPACE_CHAR_BITMASK =
149152
(1L << ' ')
@@ -202,13 +205,13 @@ static int skipValidFloatingPointNumber(String value, int start) {
202205
++i;
203206
}
204207
// 2. One or both of the following, in the given order:
205-
boolean hasIntegerPart = false;
208+
boolean hasMantissa = false;
206209
// 1. A series of one or more ASCII digits.
207210
while (i < n) {
208211
char ch = value.charAt(i);
209212
if ('0' <= ch && ch <= '9') {
210213
++i;
211-
hasIntegerPart = true;
214+
hasMantissa = true;
212215
} else {
213216
break;
214217
}
@@ -218,17 +221,19 @@ static int skipValidFloatingPointNumber(String value, int start) {
218221
// 2. A series of one or more ASCII digits.
219222
if (i < n && value.charAt(i) == '.') {
220223
++i;
224+
// Even if there's an integer, you need digits after the decimal point.
225+
hasMantissa = false;
221226
while (i < n) {
222227
char ch = value.charAt(i);
223228
if ('0' <= ch && ch <= '9') {
224229
++i;
225-
hasIntegerPart = true;
230+
hasMantissa = true;
226231
} else {
227232
break;
228233
}
229234
}
230235
}
231-
if (!hasIntegerPart) {
236+
if (!hasMantissa) {
232237
return -1;
233238
}
234239
// 3. Optionally:

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

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -360,10 +360,14 @@ public static final void testImgSrcsetSyntax() {
360360
+ "<img srcset=\"http://example.com/foo.png 48x\" />\n"
361361
+ "<img srcset=\"http://example.com/foo.png .123x\" />\n"
362362
+ "<img srcset=\"http://example.com/foo.png .123e2x\" />\n"
363-
+ "<img srcset=\"http://example.com/foo.png 123.456x\" />\n"
364-
+ "<img srcset=\"/big.png 64w, /little.png\" />\n"
365-
+ "<img srcset=\"/big.png 64w, /little.png\" />\n"
366-
+ "<img srcset=\"/big.png 64w, /little.png\" />\n"
363+
+ "<img srcset=\"http://example.com/foo.png 123.456E-1x\" />\n"
364+
+ "<img srcset=\"http://example.com/foo.png -123x\" />\n"
365+
+ "no float: \n"
366+
+ "no fraction: \n"
367+
+ "no exponent: \n"
368+
+ "<img srcset=\"/big.png 64w , /little.png\" />\n"
369+
+ "<img srcset=\"/big.png 64w , /little.png\" />\n"
370+
+ "<img srcset=\"/big.png 64w , /little.png\" />\n"
367371
+ "<img srcset=\"foo%2cbar.png\" />\n"
368372
+ "empty: \n"
369373
+ "only space: \n"
@@ -386,7 +390,11 @@ public static final void testImgSrcsetSyntax() {
386390
+ "<img srcset=\"http://example.com/foo.png 48x\" />\n"
387391
+ "<img srcset=\"http://example.com/foo.png .123x\" />\n"
388392
+ "<img srcset=\"http://example.com/foo.png .123e2x\" />\n"
389-
+ "<img srcset=\"http://example.com/foo.png 123.456x\" />\n"
393+
+ "<img srcset=\"http://example.com/foo.png 123.456E-1x\" />\n"
394+
+ "<img srcset=\"http://example.com/foo.png -123x\" />\n"
395+
+ "no float: <img srcset=\"http://example.com/foo.png -x\" />\n"
396+
+ "no fraction: <img srcset=\"http://example.com/foo.png -.e1\" />\n"
397+
+ "no exponent: <img srcset=\"http://example.com/foo.png -1e+x\" />\n"
390398
+ "<img srcset=\"/big.png 64w, /little.png\" />\n"
391399
+ "<img srcset=\" /big.png 64w , /little.png\" />\n"
392400
+ "<img srcset=\"\t\t/big.png 64w\r\n,/little.png\t\t\" />\n"

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
package org.owasp.html;
2+
3+
import org.junit.Test;
4+
5+
import junit.framework.TestCase;
6+
7+
@SuppressWarnings({ "javadoc" })
8+
public final class StringsTest extends TestCase {
9+
10+
@Test
11+
public static void testValidFloatingPointNumber() {
12+
assertEquals(
13+
-1,
14+
Strings.skipValidFloatingPointNumber("", 0));
15+
assertEquals(
16+
3,
17+
Strings.skipValidFloatingPointNumber("123", 0));
18+
assertEquals(
19+
7,
20+
Strings.skipValidFloatingPointNumber("123.456", 0));
21+
assertEquals(
22+
7,
23+
Strings.skipValidFloatingPointNumber("123.456f", 0));
24+
assertEquals(
25+
8,
26+
Strings.skipValidFloatingPointNumber(" 123.456 ", 1));
27+
assertEquals(
28+
-1,
29+
Strings.skipValidFloatingPointNumber("1e", 0));
30+
assertEquals(
31+
-1,
32+
Strings.skipValidFloatingPointNumber("1.", 0));
33+
assertEquals(
34+
-1,
35+
Strings.skipValidFloatingPointNumber("1e+", 0));
36+
assertEquals(
37+
-1,
38+
Strings.skipValidFloatingPointNumber("1e+e", 0));
39+
assertEquals(
40+
5,
41+
Strings.skipValidFloatingPointNumber(" 1E-1", 1));
42+
assertEquals(
43+
5,
44+
Strings.skipValidFloatingPointNumber(" 1E+2 ", 1));
45+
assertEquals(
46+
5,
47+
Strings.skipValidFloatingPointNumber(" 1E+2,", 1));
48+
}
49+
50+
}

0 commit comments

Comments
 (0)