Details
P2: Important Description
In src/core/user_script.cpp when the greasemonkey metadata is parsed from a user script, url pattern strings from @match directives are not added to the script data struct if they fail to parse correctly. If all such match patterns in the script are invalid, and there are no @include directives in the metadata, then the script defaults to being included on all pages. I think in this case it would make sense to include the script on no pages. Here is my proposed change to address this:
- don't test the patterns for validity at parse time
- check them at match time and don't bother trying to match if pattern parsing fails
For example:
diff --git i/src/core/renderer/user_resource_controller.cpp w/src/core/renderer/user_resource_controller.cpp index f530b7680803..e9878832c8f3 100644 --- i/src/core/renderer/user_resource_controller.cpp +++ w/src/core/renderer/user_resource_controller.cpp @@ -95,8 +95,8 @@ static bool scriptMatchesURL(const UserScriptData &scriptData, const GURL &url) if (!scriptData.urlPatterns.empty()) { matchFound = false; for (auto it = scriptData.urlPatterns.begin(), end = scriptData.urlPatterns.end(); it != end; ++it) { - URLPattern urlPattern(QtWebEngineCore::UserScript::validUserScriptSchemes(), *it); - if (urlPattern.MatchesURL(url)) + URLPattern urlPattern(QtWebEngineCore::UserScript::validUserScriptSchemes()); + if (urlPattern.Parse(*it)==URLPattern::PARSE_SUCCESS && urlPattern.MatchesURL(url)) matchFound = true; } if (!matchFound) diff --git i/src/core/user_script.cpp w/src/core/user_script.cpp index 9b9d66d55678..622deb911918 100644 --- i/src/core/user_script.cpp +++ w/src/core/user_script.cpp @@ -260,8 +260,7 @@ void UserScript::parseMetadataHeader() } scriptData->excludeGlobs.push_back(value); } else if (GetDeclarationValue(line, kMatchDeclaration, &value)) { - if (URLPattern::PARSE_SUCCESS == urlPatternParser.Parse(value)) - scriptData->urlPatterns.push_back(value); + scriptData->urlPatterns.push_back(value); } else if (GetDeclarationValue(line, kRunAtDeclaration, &value)) { if (value == kRunAtDocumentStartValue) scriptData->injectionPoint = DocumentElementCreation;
There will be no additional runtime cost because the patterns were already being fully parsed at the point where they were being matched against URLs.
An alternative is what chromium appears to do for extensions (in extensions/common/user_script.cc) which is just to exit out of the metadata parsing method if a pattern fails to parse, so the include = "*" fallback would never be executed. This isn't ideal because the caller wouldn't know the state of the script; like if it had correct match patterns proceeding the broken one and other things like run-at afterwards, the script would still be injected into some pages but not at the expected time.
I don't think this is a problem for chrome because extensions with bad patterns are rejected at install time.
Here is a patch for a test that reproduces:
--- tests/auto/quick/qmltests/data/tst_userScripts.qml
+++ tests/auto/quick/qmltests/data/tst_userScripts.qml
@@ -50,6 +50,11 @@
}
WebEngineScript {
+ id: scriptWithBadMatchMetadata
+ sourceUrl: Qt.resolvedUrl("script-with-bad-match-metadata.js")
+ }
+
+ WebEngineScript {
id: scriptWithMetadata
sourceUrl: Qt.resolvedUrl("script-with-metadata.js")
}
@@ -164,13 +169,25 @@
tryCompare(webEngineView , "title", "Big user script changed title");
}
+ function test_dontInjectBadUrlPatternsEverywhere() {
+ compare(scriptWithBadMatchMetadata.name, "Test bad match script");
+ compare(scriptWithBadMatchMetadata.injectionPoint, WebEngineScript.DocumentReady);
+
+ webEngineView.userScripts = [ scriptWithBadMatchMetadata ];
+
+ // @match some:junk
+ webEngineView.url = Qt.resolvedUrl("test2.html");
+ webEngineView.waitForLoadSucceeded();
+ tryCompare(webEngineView, "title", "Test page with huge link area");
+ }
+
function test_parseMetadataHeader() {
compare(scriptWithMetadata.name, "Test script");
compare(scriptWithMetadata.injectionPoint, WebEngineScript.DocumentReady);
And script-with-bad-match-metadata.js is just:
// ==UserScript== // @name Test bad match script // @homepageURL http://www.qt.io/ // @description Test script with metadata block with an invalid match directive // @match some:junk // @run-at document-end // ==/UserScript== document.title = "New title";