[webkit-changes] [206758] trunk

[achristensen]

Title: [206758] trunk

Revision

206758

Author

achristen...@apple.com

Date

2016-10-03 15:55:46 -0700 (Mon, 03 Oct 2016)

Log Message

Source/WebCore:
URLParser should strip tabs at all locations
https://bugs.webkit.org/show_bug.cgi?id=162836

Reviewed by Geoffrey Garen.

Covered by adding tabs to each location of each API test
except tests that test the encoding of surrogate pairs,
because inserting a tab between the pairs changes the encoding.

* platform/URLParser.cpp:
(WebCore::URLParser::takesTwoAdvancesUntilEnd):
(WebCore::URLParser::parse):
(WebCore::URLParser::parseIPv4Number):
(WebCore::URLParser::parseIPv4Host):
* platform/URLParser.h:

Tools:
URLParser should ignore tabs at all locations
https://bugs.webkit.org/show_bug.cgi?id=162836

Reviewed by Geoffrey Garen.

* TestWebKitAPI/Tests/WebCore/URLParser.cpp:
(TestWebKitAPI::checkURL):
(TestWebKitAPI::checkRelativeURL):
(TestWebKitAPI::checkURLDifferences):
(TestWebKitAPI::checkRelativeURLDifferences):
(TestWebKitAPI::TEST_F):

Modified Paths

• trunk/Source/WebCore/ChangeLog
• trunk/Source/WebCore/platform/URLParser.cpp
• trunk/Source/WebCore/platform/URLParser.h
• trunk/Tools/ChangeLog
• trunk/Tools/TestWebKitAPI/Tests/WebCore/URLParser.cpp

Diff

Modified: trunk/Source/WebCore/ChangeLog (206757 => 206758)

--- trunk/Source/WebCore/ChangeLog	2016-10-03 22:53:43 UTC (rev 206757)
+++ trunk/Source/WebCore/ChangeLog	2016-10-03 22:55:46 UTC (rev 206758)
@@ -1,3 +1,21 @@
+2016-10-03  Alex Christensen  <achristen...@webkit.org>
+
+        URLParser should strip tabs at all locations
+        https://bugs.webkit.org/show_bug.cgi?id=162836
+
+        Reviewed by Geoffrey Garen.
+
+        Covered by adding tabs to each location of each API test
+        except tests that test the encoding of surrogate pairs,
+        because inserting a tab between the pairs changes the encoding.
+
+        * platform/URLParser.cpp:
+        (WebCore::URLParser::takesTwoAdvancesUntilEnd):
+        (WebCore::URLParser::parse):
+        (WebCore::URLParser::parseIPv4Number):
+        (WebCore::URLParser::parseIPv4Host):
+        * platform/URLParser.h:
+
 2016-10-03  Antti Koivisto  <an...@apple.com>
 
         Remove Document::elementSheet()

Modified: trunk/Source/WebCore/platform/URLParser.cpp (206757 => 206758)

--- trunk/Source/WebCore/platform/URLParser.cpp	2016-10-03 22:53:43 UTC (rev 206757)
+++ trunk/Source/WebCore/platform/URLParser.cpp	2016-10-03 22:55:46 UTC (rev 206758)
@@ -426,6 +426,18 @@
 }
 
 template<typename CharacterType>
+bool URLParser::takesTwoAdvancesUntilEnd(CodePointIterator<CharacterType> iterator)
+{
+    if (iterator.atEnd())
+        return false;
+    advance<CharacterType, ReportSyntaxViolation::No>(iterator);
+    if (iterator.atEnd())
+        return false;
+    advance<CharacterType, ReportSyntaxViolation::No>(iterator);
+    return iterator.atEnd();
+}
+
+template<typename CharacterType>
 ALWAYS_INLINE bool URLParser::isWindowsDriveLetter(CodePointIterator<CharacterType> iterator)
 {
     if (iterator.atEnd() || !isASCIIAlpha(*iterator))
@@ -1542,7 +1554,8 @@
             do {
                 LOG_STATE("FileHost");
                 if (isSlashQuestionOrHash(*c)) {
-                    bool windowsQuirk = c.codeUnitsSince(authorityOrHostBegin) == 2 && isWindowsDriveLetter(authorityOrHostBegin);
+                    bool windowsQuirk = takesTwoAdvancesUntilEnd(CodePointIterator<CharacterType>(authorityOrHostBegin, c))
+                        && isWindowsDriveLetter(authorityOrHostBegin);
                     if (windowsQuirk) {
                         syntaxViolation(authorityOrHostBegin);
                         appendToASCIIBuffer('/');
@@ -2100,6 +2113,11 @@
     if (!iterator.atEnd() && *iterator == '.')
         return Nullopt;
     while (!iterator.atEnd()) {
+        if (isTabOrNewline(*iterator)) {
+            didSeeSyntaxViolation = true;
+            ++iterator;
+            continue;
+        }
         if (*iterator == '.') {
             ASSERT(!value.hasOverflowed());
             return value.unsafeGet();
@@ -2173,6 +2191,11 @@
     items.reserveInitialCapacity(4);
     bool didSeeSyntaxViolation = false;
     while (!iterator.atEnd()) {
+        if (isTabOrNewline(*iterator)) {
+            didSeeSyntaxViolation = true;
+            ++iterator;
+            continue;
+        }
         if (items.size() >= 4)
             return Nullopt;
         if (auto item = parseIPv4Number(iterator, didSeeSyntaxViolation))

Modified: trunk/Source/WebCore/platform/URLParser.h (206757 => 206758)

--- trunk/Source/WebCore/platform/URLParser.h	2016-10-03 22:53:43 UTC (rev 206757)
+++ trunk/Source/WebCore/platform/URLParser.h	2016-10-03 22:55:46 UTC (rev 206758)
@@ -71,6 +71,7 @@
     void advance(CodePointIterator<CharacterType>& iterator) { advance<CharacterType, reportSyntaxViolation>(iterator, iterator); }
     template<typename CharacterType, ReportSyntaxViolation = ReportSyntaxViolation::Yes>
     void advance(CodePointIterator<CharacterType>&, const CodePointIterator<CharacterType>& iteratorForSyntaxViolationPosition);
+    template<typename CharacterType> bool takesTwoAdvancesUntilEnd(CodePointIterator<CharacterType>);
     template<typename CharacterType> void syntaxViolation(const CodePointIterator<CharacterType>&);
     template<typename CharacterType> void fragmentSyntaxViolation(const CodePointIterator<CharacterType>&);
     template<typename CharacterType> bool isPercentEncodedDot(CodePointIterator<CharacterType>);

Modified: trunk/Tools/ChangeLog (206757 => 206758)

--- trunk/Tools/ChangeLog	2016-10-03 22:53:43 UTC (rev 206757)
+++ trunk/Tools/ChangeLog	2016-10-03 22:55:46 UTC (rev 206758)
@@ -1,5 +1,19 @@
 2016-10-03  Alex Christensen  <achristen...@webkit.org>
 
+        URLParser should ignore tabs at all locations
+        https://bugs.webkit.org/show_bug.cgi?id=162836
+
+        Reviewed by Geoffrey Garen.
+
+        * TestWebKitAPI/Tests/WebCore/URLParser.cpp:
+        (TestWebKitAPI::checkURL):
+        (TestWebKitAPI::checkRelativeURL):
+        (TestWebKitAPI::checkURLDifferences):
+        (TestWebKitAPI::checkRelativeURLDifferences):
+        (TestWebKitAPI::TEST_F):
+
+2016-10-03  Alex Christensen  <achristen...@webkit.org>
+
         URLParser: fragment-only URLs relative to file URLs should just add a fragment
         https://bugs.webkit.org/show_bug.cgi?id=162871
 

Modified: trunk/Tools/TestWebKitAPI/Tests/WebCore/URLParser.cpp (206757 => 206758)

--- trunk/Tools/TestWebKitAPI/Tests/WebCore/URLParser.cpp	2016-10-03 22:53:43 UTC (rev 206757)
+++ trunk/Tools/TestWebKitAPI/Tests/WebCore/URLParser.cpp	2016-10-03 22:55:46 UTC (rev 206758)
@@ -69,8 +69,24 @@
     return s1.utf8() == s2.utf8();
 }
 
-static void checkURL(const String& urlString, const ExpectedParts& parts, bool checkTabs = true)
+static String insertTabAtLocation(const String& string, size_t location)
 {
+    ASSERT(location <= string.length());
+    return makeString(string.substring(0, location), "\t", string.substring(location));
+}
+
+static ExpectedParts invalidParts(const String& urlStringWithTab)
+{
+    return {"", "", "", "", 0, "" , "", "", urlStringWithTab};
+}
+
+enum class TestTabs { No, Yes };
+
+// Inserting tabs between surrogate pairs changes the encoded value instead of being skipped by the URLParser.
+const TestTabs testTabsValueForSurrogatePairs = TestTabs::No;
+
+static void checkURL(const String& urlString, const ExpectedParts& parts, TestTabs testTabs = TestTabs::Yes)
+{
     bool wasEnabled = URLParser::enabled();
     URLParser::setEnabled(true);
     auto url = "" urlString);
@@ -102,11 +118,12 @@
     EXPECT_TRUE(URLParser::internalValuesConsistent(url));
     EXPECT_TRUE(URLParser::internalValuesConsistent(oldURL));
 
-    if (checkTabs) {
+    if (testTabs == TestTabs::Yes) {
         for (size_t i = 0; i < urlString.length(); ++i) {
-            String urlStringWithTab = makeString(urlString.substring(0, i), "\t", urlString.substring(i));
-            ExpectedParts invalidPartsWithTab = {"", "", "", "", 0, "" , "", "", urlStringWithTab};
-            checkURL(urlStringWithTab, parts.isInvalid() ? invalidPartsWithTab : parts, false);
+            String urlStringWithTab = insertTabAtLocation(urlString, i);
+            checkURL(urlStringWithTab,
+                parts.isInvalid() ? invalidParts(urlStringWithTab) : parts,
+                TestTabs::No);
         }
     }
 }
@@ -273,8 +290,6 @@
     checkURL("http://127.\t0.0.1/", {"http", "", "", "127.0.0.1", 0, "/", "", "", "http://127.0.0.1/"});
     checkURL("http://./", {"http", "", "", ".", 0, "/", "", "", "http://./"});
     checkURL("http://.", {"http", "", "", ".", 0, "/", "", "", "http://./"});
-    checkURL("http://123\t.256/", {"http", "", "", "123.256", 0, "/", "", "", "http://123.256/"});
-    checkURL("http://123.\t256/", {"http", "", "", "123.256", 0, "/", "", "", "http://123.256/"});
     checkURL("notspecial:/a", {"notspecial", "", "", "", 0, "/a", "", "", "notspecial:/a"});
     checkURL("notspecial:", {"notspecial", "", "", "", 0, "", "", "", "notspecial:"});
     checkURL("notspecial:/", {"notspecial", "", "", "", 0, "/", "", "", "notspecial:/"});
@@ -305,7 +320,7 @@
     checkURL("http://:@host", {"http", "", "", "host", 0, "/", "", "", "http://host/"});
 }
 
-static void checkRelativeURL(const String& urlString, const String& baseURLString, const ExpectedParts& parts, bool checkTabs = true)
+static void checkRelativeURL(const String& urlString, const String& baseURLString, const ExpectedParts& parts, TestTabs testTabs = TestTabs::Yes)
 {
     bool wasEnabled = URLParser::enabled();
     URLParser::setEnabled(true);
@@ -338,11 +353,13 @@
     EXPECT_TRUE(URLParser::internalValuesConsistent(url));
     EXPECT_TRUE(URLParser::internalValuesConsistent(oldURL));
     
-    if (checkTabs) {
+    if (testTabs == TestTabs::Yes) {
         for (size_t i = 0; i < urlString.length(); ++i) {
-            String urlStringWithTab = makeString(urlString.substring(0, i), "\t", urlString.substring(i));
-            ExpectedParts invalidPartsWithTab = {"", "", "", "", 0, "" , "", "", urlStringWithTab};
-            checkRelativeURL(urlStringWithTab, baseURLString, parts.isInvalid() ? invalidPartsWithTab : parts, false);
+            String urlStringWithTab = insertTabAtLocation(urlString, i);
+            checkRelativeURL(urlStringWithTab,
+                baseURLString,
+                parts.isInvalid() ? invalidParts(urlStringWithTab) : parts,
+                TestTabs::No);
         }
     }
 }
@@ -422,7 +439,7 @@
     checkRelativeURL("http:\\\\host\\foo", "about:blank", {"http", "", "", "host", 0, "/foo", "", "", "http://host/foo"});
 }
 
-static void checkURLDifferences(const String& urlString, const ExpectedParts& partsNew, const ExpectedParts& partsOld)
+static void checkURLDifferences(const String& urlString, const ExpectedParts& partsNew, const ExpectedParts& partsOld, TestTabs testTabs = TestTabs::Yes)
 {
     bool wasEnabled = URLParser::enabled();
     URLParser::setEnabled(true);
@@ -455,10 +472,18 @@
     EXPECT_TRUE(URLParser::internalValuesConsistent(url));
     EXPECT_TRUE(URLParser::internalValuesConsistent(oldURL));
     
-    // FIXME: check tabs here like we do for checkURL and checkRelativeURL.
+    if (testTabs == TestTabs::Yes) {
+        for (size_t i = 0; i < urlString.length(); ++i) {
+            String urlStringWithTab = insertTabAtLocation(urlString, i);
+            checkURLDifferences(urlStringWithTab,
+                partsNew.isInvalid() ? invalidParts(urlStringWithTab) : partsNew,
+                partsOld.isInvalid() ? invalidParts(urlStringWithTab) : partsOld,
+                TestTabs::No);
+        }
+    }
 }
 
-static void checkRelativeURLDifferences(const String& urlString, const String& baseURLString, const ExpectedParts& partsNew, const ExpectedParts& partsOld)
+static void checkRelativeURLDifferences(const String& urlString, const String& baseURLString, const ExpectedParts& partsNew, const ExpectedParts& partsOld, TestTabs testTabs = TestTabs::Yes)
 {
     bool wasEnabled = URLParser::enabled();
     URLParser::setEnabled(true);
@@ -491,7 +516,15 @@
     EXPECT_TRUE(URLParser::internalValuesConsistent(url));
     EXPECT_TRUE(URLParser::internalValuesConsistent(oldURL));
 
-    // FIXME: check tabs here like we do for checkURL and checkRelativeURL.
+    if (testTabs == TestTabs::Yes) {
+        for (size_t i = 0; i < urlString.length(); ++i) {
+            String urlStringWithTab = insertTabAtLocation(urlString, i);
+            checkRelativeURLDifferences(urlStringWithTab, baseURLString,
+                partsNew.isInvalid() ? invalidParts(urlStringWithTab) : partsNew,
+                partsOld.isInvalid() ? invalidParts(urlStringWithTab) : partsOld,
+                TestTabs::No);
+        }
+    }
 }
 
 // These are differences between the new URLParser and the old URL::parse which make URLParser more standards compliant.
@@ -617,7 +650,7 @@
     checkRelativeURLDifferences("http:/example.com/", "http://example.org/foo/bar",
         {"http", "", "", "example.org", 0, "/example.com/", "", "", "http://example.org/example.com/"},
         {"http", "", "", "example.com", 0, "/", "", "", "http://example.com/"});
-    
+
     // This behavior matches Chrome and Firefox, but not WebKit using URL::parse.
     // The behavior of URL::parse is clearly wrong because reparsing file://path would make path the host.
     // The spec is unclear.
@@ -647,7 +680,7 @@
 
     checkRelativeURLDifferences(utf16String(u"http://foo:💩@example.com/bar"), "http://other.com/",
         {"http", "foo", utf16String(u"💩"), "example.com", 0, "/bar", "", "", "http://foo:%f0%9f%92...@example.com/bar"},
-        {"", "", "", "", 0, "", "", "", utf16String(u"http://foo:💩@example.com/bar")});
+        {"", "", "", "", 0, "", "", "", utf16String(u"http://foo:💩@example.com/bar")}, testTabsValueForSurrogatePairs);
     checkRelativeURLDifferences("http://&a:foo(b]c@d:2/", "http://example.org/foo/bar",
         {"http", "&a", "foo(b]c", "d", 2, "/", "", "", "http://&a:foo(b%5Dc@d:2/"},
         {"", "", "", "", 0, "", "", "", "http://&a:foo(b]c@d:2/"});
@@ -701,7 +734,7 @@
         {"foo", "", "", "", 0, "//", "", "", "foo://"});
     checkURLDifferences(utf16String(u"http://host?ß😍#ß😍"),
         {"http", "", "", "host", 0, "/", "%C3%9F%F0%9F%98%8D", utf16String(u"ß😍"), utf16String(u"http://host/?%C3%9F%F0%9F%98%8D#ß😍")},
-        {"http", "", "", "host", 0, "/", "%C3%9F%F0%9F%98%8D", "%C3%9F%F0%9F%98%8D", "http://host/?%C3%9F%F0%9F%98%8D#%C3%9F%F0%9F%98%8D"});
+        {"http", "", "", "host", 0, "/", "%C3%9F%F0%9F%98%8D", "%C3%9F%F0%9F%98%8D", "http://host/?%C3%9F%F0%9F%98%8D#%C3%9F%F0%9F%98%8D"}, testTabsValueForSurrogatePairs);
     checkURLDifferences(utf16String(u"http://host/path#💩\t💩"),
         {"http", "", "", "host", 0, "/path", "", utf16String(u"💩💩"), utf16String(u"http://host/path#💩💩")},
         {"http", "", "", "host", 0, "/path", "", "%F0%9F%92%A9%F0%9F%92%A9", "http://host/path#%F0%9F%92%A9%F0%9F%92%A9"});
@@ -975,7 +1008,7 @@
     const wchar_t validSurrogateEnd = 0xDD55;
     const wchar_t invalidSurrogateEnd = 'A';
     checkURL(utf16String<12>({'h', 't', 't', 'p', ':', '/', '/', 'w', '/', surrogateBegin, validSurrogateEnd, '\0'}),
-        {"http", "", "", "w", 0, "/%F0%90%85%95", "", "", "http://w/%F0%90%85%95"}, false);
+        {"http", "", "", "w", 0, "/%F0%90%85%95", "", "", "http://w/%F0%90%85%95"}, testTabsValueForSurrogatePairs);
 
     // URLParser matches Chrome and Firefox but not URL::parse.
     checkURLDifferences(utf16String<12>({'h', 't', 't', 'p', ':', '/', '/', 'w', '/', surrogateBegin, invalidSurrogateEnd}),
@@ -995,7 +1028,7 @@
         {"http", "", "", "w", 0, "/", "%ED%A0%80", "", "http://w/?%ED%A0%80"});
 }
 
-static void checkURL(const String& urlString, const TextEncoding& encoding, const ExpectedParts& parts, bool checkTabs = true)
+static void checkURL(const String& urlString, const TextEncoding& encoding, const ExpectedParts& parts, TestTabs testTabs = TestTabs::Yes)
 {
     URLParser parser(urlString, { }, encoding);
     auto url = ""
@@ -1009,11 +1042,12 @@
     EXPECT_TRUE(eq(parts.fragment, url.fragmentIdentifier()));
     EXPECT_TRUE(eq(parts.string, url.string()));
 
-    if (checkTabs) {
+    if (testTabs == TestTabs::Yes) {
         for (size_t i = 0; i < urlString.length(); ++i) {
-            String urlStringWithTab = makeString(urlString.substring(0, i), "\t", urlString.substring(i));
-            ExpectedParts invalidPartsWithTab = {"", "", "", "", 0, "" , "", "", urlStringWithTab};
-            checkURL(urlStringWithTab, encoding, parts.isInvalid() ? invalidPartsWithTab : parts, false);
+            String urlStringWithTab = insertTabAtLocation(urlString, i);
+            checkURL(urlStringWithTab, encoding,
+                parts.isInvalid() ? invalidParts(urlStringWithTab) : parts,
+                TestTabs::No);
         }
     }
 }
@@ -1020,8 +1054,8 @@
 
 TEST_F(URLParserTest, QueryEncoding)
 {
-    checkURL(utf16String(u"http://host?ß😍#ß😍"), UTF8Encoding(), {"http", "", "", "host", 0, "/", "%C3%9F%F0%9F%98%8D", utf16String(u"ß😍"), utf16String(u"http://host/?%C3%9F%F0%9F%98%8D#ß😍")}, false);
-    checkURL(utf16String(u"http://host?ß😍#ß😍"), UTF8Encoding(), {"http", "", "", "host", 0, "/", "%C3%9F%F0%9F%98%8D", utf16String(u"ß😍"), utf16String(u"http://host/?%C3%9F%F0%9F%98%8D#ß😍")}, false);
+    checkURL(utf16String(u"http://host?ß😍#ß😍"), UTF8Encoding(), {"http", "", "", "host", 0, "/", "%C3%9F%F0%9F%98%8D", utf16String(u"ß😍"), utf16String(u"http://host/?%C3%9F%F0%9F%98%8D#ß😍")}, testTabsValueForSurrogatePairs);
+    checkURL(utf16String(u"http://host?ß😍#ß😍"), UTF8Encoding(), {"http", "", "", "host", 0, "/", "%C3%9F%F0%9F%98%8D", utf16String(u"ß😍"), utf16String(u"http://host/?%C3%9F%F0%9F%98%8D#ß😍")}, testTabsValueForSurrogatePairs);
 
     TextEncoding latin1(String("latin1"));
     checkURL("http://host/?query with%20spaces", latin1, {"http", "", "", "host", 0, "/", "query%20with%20spaces", "", "http://host/?query%20with%20spaces"});

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes