Skip to content

GitLab

T
tangled
Commit 6fbca674 authored by David Van Cleve's avatar David Van Cleve Committed by Commit Bot
Browse files
Options

Revert URLPattern wildcard to again match some invalid URLs 

This is a partial revert of r715320, which changed
URLPattern::MatchesURL to decline to match all invalid
(!GURL::is_valid) URLs. This was an inadvertent behavior change;
it wasn't clear what the intended behavior was in this case.

Reverting this seems to resolve crbug.com/1041880, a bug involving
some PDF loads from blob: URLs getting stuck as gray boxes. While
r715320 was originally made to resolve a crash---namely, MatchesURL
would crash on URLs with empty paths---the follow-up commit r718423
adds logic that will still suffice to prevent this crash (this
is tested by ExtensionURLPatternTest.WildcardMatchesPathlessUrl and
ExtensionURLPatternTest.NonwildcardDoesntMatchPathlessUrl).

Changes:
- URLPattern: revert the behavior change and update documentation
to reflect that intended behavior is actually to allow the wildcard
'<all_urls>' pattern to match invalid URLs
- URLPattern tests: update to reflect the intended new behavior

Bug: 1041880

R=devlin

Change-Id: I4f4571c70d0f0dbd7b52cdadbf96a7ea6854cfe0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2022251Reviewed-by: default avatarCharlie Reis <creis@chromium.org>
Reviewed-by: default avatarDevlin <rdevlin.cronin@chromium.org>
Commit-Queue: David Van Cleve <davidvc@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735510}
parent 885d2e1e
Hide whitespace changes
Inline Side-by-side
Showing with 16 additions and 10 deletions
extensions/common/url_pattern.cc
View file @ 6fbca674
...@@ -414,10 +414,6 @@ bool URLPattern::SetPort(base::StringPiece port) { ...@@ -414,10 +414,6 @@ bool URLPattern::SetPort(base::StringPiece port) {
} }
bool URLPattern::MatchesURL(const GURL& test) const { bool URLPattern::MatchesURL(const GURL& test) const {
// Invalid URLs can never match.
if (!test.is_valid())
return false;
const GURL* test_url = &test; const GURL* test_url = &test;
bool has_inner_url = test.inner_url() != nullptr; bool has_inner_url = test.inner_url() != nullptr;
......
extensions/common/url_pattern.h
View file @ 6fbca674
...@@ -27,8 +27,8 @@ class GURL; ...@@ -27,8 +27,8 @@ class GURL;
// //
// * Host is not used when the scheme is 'file'. // * Host is not used when the scheme is 'file'.
// * The path can have embedded '*' characters which act as glob wildcards. // * The path can have embedded '*' characters which act as glob wildcards.
// * '<all_urls>' is a special pattern that matches any valid URL that contains // * '<all_urls>' is a special pattern that matches any (possibly invalid) URL
// a valid scheme (as specified by valid_schemes_). // that contains a valid scheme (as specified by valid_schemes_).
// * The '*' scheme pattern excludes file URLs. // * The '*' scheme pattern excludes file URLs.
// //
// Examples of valid patterns: // Examples of valid patterns:
...@@ -152,8 +152,10 @@ class URLPattern { ...@@ -152,8 +152,10 @@ class URLPattern {
// false otherwise. Uses valid_schemes_ to determine validity. // false otherwise. Uses valid_schemes_ to determine validity.
bool IsValidScheme(base::StringPiece scheme) const; bool IsValidScheme(base::StringPiece scheme) const;
// Returns true if this instance matches the specified URL. Always returns // Returns true if this instance matches the specified URL. If the pattern
// false for invalid URLs. // is the wildcard '<all_urls>', this may return true for invalid URLs
// (!test.is_valid()) that nonetheless have valid schemes (as determined
// by MatchesScheme).
bool MatchesURL(const GURL& test) const; bool MatchesURL(const GURL& test) const;
// Returns true if this instance matches the specified security origin. // Returns true if this instance matches the specified security origin.
......
extensions/common/url_pattern_unittest.cc
View file @ 6fbca674
...@@ -370,11 +370,19 @@ TEST(ExtensionURLPatternTest, Match12) { ...@@ -370,11 +370,19 @@ TEST(ExtensionURLPatternTest, Match12) {
GURL("data:text/html;charset=utf-8,<html>asdf</html>"))); GURL("data:text/html;charset=utf-8,<html>asdf</html>")));
} }
TEST(ExtensionURLPatternTest, DoesntMatchInvalid) { TEST(ExtensionURLPatternTest, MatchInvalid) {
URLPattern pattern(kAllSchemes); URLPattern pattern(kAllSchemes);
// Even the all_urls pattern shouldn't match an invalid URL. // The all_urls pattern should match even an invalid URL.
EXPECT_EQ(URLPattern::ParseResult::kSuccess, EXPECT_EQ(URLPattern::ParseResult::kSuccess,
pattern.Parse(URLPattern::kAllUrlsPattern)); pattern.Parse(URLPattern::kAllUrlsPattern));
EXPECT_TRUE(pattern.MatchesURL(GURL("http:")));
}
TEST(ExtensionURLPatternTest, DoesntMatchInvalidIfNotWildcard) {
URLPattern pattern(kAllSchemes);
// A non-all_urls pattern shouldn't match an invalid URL,
// even if the scheme matches.
EXPECT_EQ(URLPattern::ParseResult::kSuccess, pattern.Parse("*://*/*"));
EXPECT_FALSE(pattern.MatchesURL(GURL("http:"))); EXPECT_FALSE(pattern.MatchesURL(GURL("http:")));
} }
......
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