Bug 148966 – Content blockers should be able to promote http to https

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alex Christensen]

MakeHTTPS would be a good feature.

[Alex Christensen]

Created attachment 260770 [details]
Patch

[Alex Christensen]

rdar://problem/21373383

[Build Bot]

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]

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 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]

Created attachment 263043 [details]
Patch

[Alex Christensen]

Created attachment 263062 [details]
Patch

[Build Bot]

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]

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]

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 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]

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]

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]

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]

Created attachment 267258 [details]
Patch

[Build Bot]

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]

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 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]

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 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 on attachment 267258 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267258&action=review

Great patch!

> Source/WebCore/ChangeLog:1
> [phone]  Chris Aljoudi  <[e-mail]>

strictly this is 
    "Chris Aljoudi <[e-mail]> and Alex Christensen <[e-mail]>"
:)

> 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]

Created attachment 267828 [details]
Patch

[Build Bot]

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]

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 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]

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]

Created attachment 267834 [details]
Patch

Rewrite test to be portable; fix bug that was triggering asserts.

[WebKit Commit Bot]

Comment on attachment 267834 [details]
Patch

Clearing flags on attachment: 267834

Committed r194386: <http://trac.webkit.org/changeset/194386>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.