RESOLVED FIXED 148966
Content blockers should be able to promote http to https
https://bugs.webkit.org/show_bug.cgi?id=148966
Summary Content blockers should be able to promote http to https
Alex Christensen
Reported 2015-09-08 10:38:29 PDT
MakeHTTPS would be a good feature.
Attachments
Patch (21.80 KB, patch)
2015-09-08 10:41 PDT, Alex Christensen
no flags
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (658.15 KB, application/zip)
2015-09-08 11:42 PDT, Build Bot
no flags
Patch (22.17 KB, patch)
2015-10-13 17:29 PDT, Alex Christensen
no flags
Patch (22.10 KB, patch)
2015-10-13 23:39 PDT, Alex Christensen
buildbot: commit-queue-
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (714.15 KB, application/zip)
2015-10-14 00:18 PDT, Build Bot
no flags
Archive of layout-test-results from ews117 for mac-yosemite (1.09 MB, application/zip)
2015-10-20 11:29 PDT, Build Bot
no flags
Patch (25.10 KB, patch)
2015-12-13 02:02 PST, Chris Aljoudi
no flags
Patch (25.28 KB, patch)
2015-12-13 02:19 PST, Chris Aljoudi
benjamin: review+
buildbot: commit-queue-
Archive of layout-test-results from ews115 for mac-yosemite (1.04 MB, application/zip)
2015-12-13 03:19 PST, Build Bot
no flags
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.18 MB, application/zip)
2015-12-13 03:21 PST, Build Bot
no flags
Patch (25.31 KB, patch)
2015-12-22 22:02 PST, Chris Aljoudi
buildbot: commit-queue-
Archive of layout-test-results from ews116 for mac-yosemite (1.04 MB, application/zip)
2015-12-22 23:03 PST, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (979.39 KB, application/zip)
2015-12-22 23:07 PST, Build Bot
no flags
Patch (26.54 KB, patch)
2015-12-23 00:16 PST, Chris Aljoudi
no flags
Alex Christensen
Comment 1 2015-09-08 10:41:46 PDT
Alex Christensen
Comment 2 2015-09-08 10:42:37 PDT
Build Bot
Comment 3 2015-09-08 11:42:44 PDT
Comment on attachment 260770 [details] Patch Attachment 260770 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/151501 New failing tests: http/tests/contentextensions/make-https-main.html http/tests/contentextensions/make-https-all.html http/tests/contentextensions/make-https-already-https.html http/tests/contentextensions/make-https-sub.html
Build Bot
Comment 4 2015-09-08 11:42:47 PDT
Created attachment 260774 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Benjamin Poulain
Comment 5 2015-10-07 15:32:32 PDT
Comment on attachment 260770 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260770&action=review Please ping me when you update. > Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:140 > + ResourceFlags flags = rule.trigger().flags; // I'm not sure this is necessary. Can't we combine things with different flags? Definitely not. Triggers with different flags must be differentiable because the flags are handled after the url-filter matches. You don't want the state machine compiler to nuke the information. > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:195 > + // ALSO, WHAT ABOUT THE PORT???? Let's make sure we only promote the URL if it uses the port 80. > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:199 > + String oldURL = request.url().string(); > + ASSERT(oldURL.startsWith("http://")); > + String newURL = makeString("https://", oldURL.substring(strlen("http://"))); Instead, you should get a copy URL, use "setProtocol" to change its protocol. > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:200 > + request.setURL(URL(ParsedURLString, newURL)); This is very wrong. You can only use ParsedURLString if your string was canonicalized by the URL class. Any other use is unsafe. > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:202 > + currentDocument->addConsoleMessage(MessageSource::ContentBlocker, MessageLevel::Info, makeString("Content blocker changed URL from ", oldURL, " to ", newURL)); "changed" -> "promoted"? > LayoutTests/ChangeLog:7 > + Let's have more tests: -explicit ports: 80, 443, > 1000 -invalid URLs starting with "http://"
Alex Christensen
Comment 6 2015-10-13 17:29:20 PDT
Alex Christensen
Comment 7 2015-10-13 23:39:31 PDT
Build Bot
Comment 8 2015-10-14 00:18:39 PDT
Comment on attachment 263062 [details] Patch Attachment 263062 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/282364 New failing tests: http/tests/contentextensions/make-https.html
Build Bot
Comment 9 2015-10-14 00:18:42 PDT
Created attachment 263064 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Alexey Proskuryakov
Comment 10 2015-10-17 14:52:11 PDT
Looms like this introduces assertions on tests, but for some reason, the debug EWS bubble remains orange. I'll investigate why EWS doesn't work properly.
Build Bot
Comment 11 2015-10-20 11:29:42 PDT
Comment on attachment 263062 [details] Patch Attachment 263062 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/312540 Number of test failures exceeded the failure limit.
Build Bot
Comment 12 2015-10-20 11:29:45 PDT
Created attachment 263595 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Chris Aljoudi
Comment 13 2015-12-13 02:02:31 PST
Created attachment 267257 [details] Patch This patch is similar to and largely based on Alex's most recent one. The changes: * ContentExtensionBackend now notifies the DocumentLoader and frame loader client of URL changes to the main document, so (for example) the location bar in UI will reflect the correct URL. * If there is a port specified in the original URL and it matches the default for the protocol (http), then if we promote to HTTPS, we also set the port correctly to HTTPS' default (443). * Per above, URL.cpp and URL.h were slightly modified: * There now exists a utility function, `defaultPortForProtocol()`, which is now used by the already-existent `isDefaultPortForProtocol()` * Instead of using DEPRECATED_DEFINE_STATIC_LOCAL for the HashMap mapping each protocol to its default port, the proper new replacement using `static NeverDestroyed<T>` is used. Hope this helps — I wasn't sure how to attribute this patch in the ChangeLogs, so right now they just reflect the defaults based on my env variables.
WebKit Commit Bot
Comment 14 2015-12-13 02:04:55 PST
Attachment 267257 [details] did not pass style-queue: ERROR: Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:197: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:199: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:202: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/WebCore/platform/URL.cpp:39: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/URL.cpp:2004: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 5 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Chris Aljoudi
Comment 15 2015-12-13 02:19:11 PST
Build Bot
Comment 16 2015-12-13 03:19:13 PST
Comment on attachment 267258 [details] Patch Attachment 267258 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/553381 Number of test failures exceeded the failure limit.
Build Bot
Comment 17 2015-12-13 03:19:17 PST
Created attachment 267260 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 18 2015-12-13 03:20:56 PST
Comment on attachment 267258 [details] Patch Attachment 267258 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/553390 New failing tests: http/tests/contentextensions/make-https.html
Build Bot
Comment 19 2015-12-13 03:21:00 PST
Created attachment 267261 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Darin Adler
Comment 20 2015-12-14 08:59:23 PST
Comment on attachment 267258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267258&action=review Not sure about this feature; someone more familiar with our plans for content blockers should weigh in. > Source/WebCore/platform/URL.cpp:2001 > + static NeverDestroyed<const DefaultPortsMap> defaultPortsMap(DefaultPortsMap({ > + { "http", 80 }, > + { "https", 443 }, > + { "ftp", 21 }, > + { "ftps", 990 } > + })); There’s an extra DefaultPortsMap() that is not needed here.
Benjamin Poulain
Comment 21 2015-12-22 12:55:55 PST
Comment on attachment 267258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267258&action=review Great patch! > Source/WebCore/ChangeLog:1 > +2015-12-13 Chris Aljoudi <chris@chrismatic.io> strictly this is "Chris Aljoudi <chris@chrismatic.io> and Alex Christensen <achristensen@webkit.org>" :) > Source/WebCore/loader/cache/CachedResourceLoader.cpp:530 > + url = request.resourceRequest().url(); // The content extension could have changed it from http to https. This could be in the scope of the previous if() branch. It is only needed if we invoke the UserContentController.
Chris Aljoudi
Comment 22 2015-12-22 22:02:51 PST
Build Bot
Comment 23 2015-12-22 23:03:54 PST
Comment on attachment 267828 [details] Patch Attachment 267828 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/596466 Number of test failures exceeded the failure limit.
Build Bot
Comment 24 2015-12-22 23:03:58 PST
Created attachment 267829 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 25 2015-12-22 23:07:43 PST
Comment on attachment 267828 [details] Patch Attachment 267828 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/596486 New failing tests: http/tests/contentextensions/make-https.html
Build Bot
Comment 26 2015-12-22 23:07:47 PST
Created attachment 267831 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Chris Aljoudi
Comment 27 2015-12-23 00:16:21 PST
Created attachment 267834 [details] Patch Rewrite test to be portable; fix bug that was triggering asserts.
WebKit Commit Bot
Comment 28 2015-12-23 02:05:10 PST
Comment on attachment 267834 [details] Patch Clearing flags on attachment: 267834 Committed r194386: <http://trac.webkit.org/changeset/194386>
WebKit Commit Bot
Comment 29 2015-12-23 02:05:18 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.