Commit 60092ea1 authored by Xiaocheng Hu's avatar Xiaocheng Hu Committed by Commit Bot

Make CSSPreloadScanner less broken

When scanning for @import rules to preload, CSSPreloadScanner terminates
a URL at ';' unconditionally. This is wrong as there might be ';' in a
URL, in which case we make a preload of a broken URL.

This patch scans URLs in a saner way:
- For urls given as a raw string, terminate at the ending quote
- Other those wrapped in a 'url()', terminate at the closing parenthesis

Note: We do not intend to write a full parser here, so it's still broken
in some cases (e.g., when the url contains ' ' or ')'). It's out of the
scope of this patch to fix them.

Bug: 1087854
Change-Id: I666c196ac1e12b259136823e0faed09958a896c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2451598Reviewed-by: default avatarRune Lillesveen <futhark@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815269}
parent 529643b0
...@@ -59,6 +59,9 @@ void CSSPreloadScanner::ScanCommon(const Char* begin, ...@@ -59,6 +59,9 @@ void CSSPreloadScanner::ScanCommon(const Char* begin,
++it) ++it)
Tokenize(*it, source); Tokenize(*it, source);
if (state_ == kRuleValue || state_ == kAfterRuleValue)
EmitRule(source);
requests_ = nullptr; requests_ = nullptr;
predicted_base_element_url_ = nullptr; predicted_base_element_url_ = nullptr;
} }
...@@ -159,15 +162,10 @@ inline void CSSPreloadScanner::Tokenize(UChar c, ...@@ -159,15 +162,10 @@ inline void CSSPreloadScanner::Tokenize(UChar c,
case kRuleValue: case kRuleValue:
if (IsHTMLSpace<UChar>(c)) { if (IsHTMLSpace<UChar>(c)) {
state_ = kAfterRuleValue; state_ = kAfterRuleValue;
} else if (c == ';') {
EmitRule(source);
} else { } else {
rule_value_.Append(c); rule_value_.Append(c);
// When reading the rule and hitting ')', which signifies the URL end, if (HasFinishedRuleValue())
// emit the rule. state_ = kAfterRuleValue;
if (c == ')') {
EmitRule(source);
}
} }
break; break;
case kAfterRuleValue: case kAfterRuleValue:
...@@ -188,6 +186,16 @@ inline void CSSPreloadScanner::Tokenize(UChar c, ...@@ -188,6 +186,16 @@ inline void CSSPreloadScanner::Tokenize(UChar c,
} }
} }
bool CSSPreloadScanner::HasFinishedRuleValue() const {
if (rule_value_.length() < 2 || rule_value_[rule_value_.length() - 2] == '\\')
return false;
// String
if (rule_value_[0] == '\'' || rule_value_[0] == '"')
return rule_value_[0] == rule_value_[rule_value_.length() - 1];
// url()
return rule_value_[rule_value_.length() - 1] == ')';
}
static String ParseCSSStringOrURL(const String& string) { static String ParseCSSStringOrURL(const String& string) {
wtf_size_t offset = 0; wtf_size_t offset = 0;
wtf_size_t reduced_length = string.length(); wtf_size_t reduced_length = string.length();
......
...@@ -80,6 +80,8 @@ class CSSPreloadScanner { ...@@ -80,6 +80,8 @@ class CSSPreloadScanner {
inline void Tokenize(UChar, const SegmentedString&); inline void Tokenize(UChar, const SegmentedString&);
void EmitRule(const SegmentedString&); void EmitRule(const SegmentedString&);
bool HasFinishedRuleValue() const;
State state_ = kInitial; State state_ = kInitial;
StringBuilder rule_; StringBuilder rule_;
StringBuilder rule_value_; StringBuilder rule_value_;
......
...@@ -1452,4 +1452,36 @@ TEST_F(HTMLPreloadScannerTest, ...@@ -1452,4 +1452,36 @@ TEST_F(HTMLPreloadScannerTest,
Test(test2); Test(test2);
} }
// https://crbug.com/1087854
TEST_F(HTMLPreloadScannerTest, CSSImportWithSemicolonInUrl) {
PreloadScannerTestCase test_cases[] = {
{"https://example.test",
"<style>@import "
"url(\"https://example2.test/css?foo=a;b&bar=d\");</style>",
"https://example2.test/css?foo=a;b&bar=d", "https://example.test/",
ResourceType::kCSSStyleSheet, 0},
{"https://example.test",
"<style>@import "
"url('https://example2.test/css?foo=a;b&bar=d');</style>",
"https://example2.test/css?foo=a;b&bar=d", "https://example.test/",
ResourceType::kCSSStyleSheet, 0},
{"https://example.test",
"<style>@import "
"url(https://example2.test/css?foo=a;b&bar=d);</style>",
"https://example2.test/css?foo=a;b&bar=d", "https://example.test/",
ResourceType::kCSSStyleSheet, 0},
{"https://example.test",
"<style>@import \"https://example2.test/css?foo=a;b&bar=d\";</style>",
"https://example2.test/css?foo=a;b&bar=d", "https://example.test/",
ResourceType::kCSSStyleSheet, 0},
{"https://example.test",
"<style>@import 'https://example2.test/css?foo=a;b&bar=d';</style>",
"https://example2.test/css?foo=a;b&bar=d", "https://example.test/",
ResourceType::kCSSStyleSheet, 0},
};
for (const auto& test : test_cases)
Test(test);
}
} // namespace blink } // namespace blink
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment