Bug 148966 - Content blockers should be able to promote http to https
Summary: Content blockers should be able to promote http to https
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks: 153428
  Show dependency treegraph
 
Reported: 2015-09-08 10:38 PDT by Alex Christensen
Modified: 2016-01-25 10:25 PST (History)
7 users (show)

See Also:


Attachments
Patch (21.80 KB, patch)
2015-09-08 10:41 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
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 Details
Patch (22.17 KB, patch)
2015-10-13 17:29 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (22.10 KB, patch)
2015-10-13 23:39 PDT, Alex Christensen
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details
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 Details
Patch (25.10 KB, patch)
2015-12-13 02:02 PST, Chris Aljoudi
no flags Details | Formatted Diff | Diff
Patch (25.28 KB, patch)
2015-12-13 02:19 PST, Chris Aljoudi
benjamin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details
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 Details
Patch (25.31 KB, patch)
2015-12-22 22:02 PST, Chris Aljoudi
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details
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 Details
Patch (26.54 KB, patch)
2015-12-23 00:16 PST, Chris Aljoudi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2015-09-08 10:38:29 PDT
MakeHTTPS would be a good feature.
Comment 1 Alex Christensen 2015-09-08 10:41:46 PDT
Created attachment 260770 [details]
Patch
Comment 2 Alex Christensen 2015-09-08 10:42:37 PDT
rdar://problem/21373383
Comment 3 Build Bot 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
Comment 4 Build Bot 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
Comment 5 Benjamin Poulain 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://"
Comment 6 Alex Christensen 2015-10-13 17:29:20 PDT
Created attachment 263043 [details]
Patch
Comment 7 Alex Christensen 2015-10-13 23:39:31 PDT
Created attachment 263062 [details]
Patch
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 Alexey Proskuryakov 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.
Comment 11 Build Bot 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.
Comment 12 Build Bot 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
Comment 13 Chris Aljoudi 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.
Comment 14 WebKit Commit Bot 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.
Comment 15 Chris Aljoudi 2015-12-13 02:19:11 PST
Created attachment 267258 [details]
Patch
Comment 16 Build Bot 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.
Comment 17 Build Bot 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
Comment 18 Build Bot 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
Comment 19 Build Bot 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
Comment 20 Darin Adler 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.
Comment 21 Benjamin Poulain 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.
Comment 22 Chris Aljoudi 2015-12-22 22:02:51 PST
Created attachment 267828 [details]
Patch
Comment 23 Build Bot 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.
Comment 24 Build Bot 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
Comment 25 Build Bot 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
Comment 26 Build Bot 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
Comment 27 Chris Aljoudi 2015-12-23 00:16:21 PST
Created attachment 267834 [details]
Patch

Rewrite test to be portable; fix bug that was triggering asserts.
Comment 28 WebKit Commit Bot 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>
Comment 29 WebKit Commit Bot 2015-12-23 02:05:18 PST
All reviewed patches have been landed.  Closing bug.