WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Alex Christensen
Comment 1
2015-09-08 10:41:46 PDT
Created
attachment 260770
[details]
Patch
Alex Christensen
Comment 2
2015-09-08 10:42:37 PDT
rdar://problem/21373383
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
Created
attachment 263043
[details]
Patch
Alex Christensen
Comment 7
2015-10-13 23:39:31 PDT
Created
attachment 263062
[details]
Patch
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
Created
attachment 267258
[details]
Patch
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
Created
attachment 267828
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug