RESOLVED CONFIGURATION CHANGED 185273
Use SafeBrowsing in WKWebView
https://bugs.webkit.org/show_bug.cgi?id=185273
Summary Use SafeBrowsing in WKWebView
Ali Juma
Reported 2018-05-03 16:06:30 PDT
Use the SafeBrowsing SPI added in bug 181804 to check URLs before loading them.
Attachments
WIP (42.68 KB, patch)
2018-05-04 10:09 PDT, Ali Juma
no flags
WIP (41.62 KB, patch)
2018-05-04 12:08 PDT, Ali Juma
no flags
WIP (41.82 KB, patch)
2018-05-07 10:44 PDT, Ali Juma
no flags
WIP (64.07 KB, patch)
2018-05-16 11:08 PDT, Ali Juma
no flags
WIP (63.88 KB, patch)
2018-05-16 12:21 PDT, Ali Juma
no flags
WIP (63.93 KB, patch)
2018-05-16 13:16 PDT, Ali Juma
no flags
WIP (83.33 KB, patch)
2018-05-31 11:08 PDT, Ali Juma
no flags
WIP (84.75 KB, patch)
2018-05-31 11:15 PDT, Ali Juma
no flags
WIP (131.12 KB, patch)
2018-06-11 10:25 PDT, Ali Juma
no flags
WIP (133.26 KB, patch)
2018-06-11 10:52 PDT, Ali Juma
no flags
WIP (130.79 KB, patch)
2018-06-11 10:57 PDT, Ali Juma
no flags
WIP (131.00 KB, patch)
2018-06-11 13:41 PDT, Ali Juma
no flags
WIP (131.02 KB, patch)
2018-06-12 10:09 PDT, Ali Juma
no flags
WIP (131.02 KB, patch)
2018-06-12 13:49 PDT, Ali Juma
no flags
WIP (131.15 KB, patch)
2018-06-13 10:48 PDT, Ali Juma
no flags
Archive of layout-test-results from ews112 for mac-sierra (3.02 MB, application/zip)
2018-06-13 12:44 PDT, EWS Watchlist
no flags
Patch (75.65 KB, patch)
2018-06-18 15:19 PDT, Ali Juma
no flags
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.71 MB, application/zip)
2018-06-19 04:02 PDT, EWS Watchlist
no flags
Patch (77.49 KB, patch)
2018-06-19 13:28 PDT, Ali Juma
no flags
patch (77.16 KB, patch)
2018-07-04 11:42 PDT, Ali Juma
no flags
Patch (76.33 KB, patch)
2018-07-04 12:04 PDT, Ali Juma
achristensen: review-
Ali Juma
Comment 1 2018-05-04 10:09:46 PDT
Ali Juma
Comment 2 2018-05-04 10:14:59 PDT
(In reply to Ali Juma from comment #1) > Created attachment 339558 [details] > WIP The main things that still need to be done here: 1) Add tests. 2) Make the warning page more detailed (i.e., match Safari's warning page), with different content for different warning types. 3) Figure out what needs to be done to make text on the warning page localizable (that is, where to put the strings -- maybe in LocalizedStringsCocoa.mm?). 4) Decide on a persistence model for user decisions to bypass warning -- should they persist for that URL for the lifetime of the WKWebView, or only for back/forward navigation to the same item, or not persist at all.
Ali Juma
Comment 3 2018-05-04 12:08:23 PDT
Created attachment 339580 [details] WIP Rebased
Ali Juma
Comment 4 2018-05-07 10:44:14 PDT
Created attachment 339730 [details] WIP Try to fix Sierra build
Ali Juma
Comment 5 2018-05-16 11:08:32 PDT
Created attachment 340500 [details] WIP Fixed back/forward navigation to/from a warning page
Ali Juma
Comment 6 2018-05-16 11:16:11 PDT
(In reply to Ali Juma from comment #2) > (In reply to Ali Juma from comment #1) > > Created attachment 339558 [details] > > WIP > > The main things that still need to be done here: > > 1) Add tests. > > 2) Make the warning page more detailed (i.e., match Safari's warning page), > with different content for different warning types. > > 3) Figure out what needs to be done to make text on the warning page > localizable (that is, where to put the strings -- maybe in > LocalizedStringsCocoa.mm?). > > 4) Decide on a persistence model for user decisions to bypass warning -- > should they persist for that URL for the lifetime of the WKWebView, or only > for back/forward navigation to the same item, or not persist at all. Another thing that still needs to be done: 5) Figure out how this feature interacts with process-swap-on-navigation. In particular, make sure swapping works as expected when navigating to/from a SafeBrowsing warning, and when bypassing a warning.
Ali Juma
Comment 7 2018-05-16 12:21:04 PDT
Created attachment 340514 [details] WIP Rebased
Ali Juma
Comment 8 2018-05-16 13:16:11 PDT
Created attachment 340519 [details] WIP Fix Release builds
Ali Juma
Comment 9 2018-05-31 11:08:10 PDT
Created attachment 341674 [details] WIP More back/forward list fixes
EWS Watchlist
Comment 10 2018-05-31 11:10:55 PDT
Attachment 341674 [details] did not pass style-queue: ERROR: Source/WebCore/loader/FrameLoader.cpp:1456: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ali Juma
Comment 11 2018-05-31 11:15:15 PDT
Created attachment 341677 [details] WIP Fix style
Ali Juma
Comment 12 2018-06-11 10:25:58 PDT
Created attachment 342442 [details] WIP Add tests
Ali Juma
Comment 13 2018-06-11 10:52:55 PDT
Created attachment 342444 [details] WIP Fix build
Ali Juma
Comment 14 2018-06-11 10:57:50 PDT
Created attachment 342446 [details] WIP Fix build
Ali Juma
Comment 15 2018-06-11 13:41:13 PDT
Created attachment 342459 [details] WIP More build fixes
Ali Juma
Comment 16 2018-06-12 10:09:56 PDT
Ali Juma
Comment 17 2018-06-12 13:49:14 PDT
Ali Juma
Comment 18 2018-06-13 10:48:24 PDT
EWS Watchlist
Comment 19 2018-06-13 12:44:51 PDT
Comment on attachment 342674 [details] WIP Attachment 342674 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/8165750 New failing tests: js/dom/JSON-stringify.html
EWS Watchlist
Comment 20 2018-06-13 12:44:53 PDT
Created attachment 342682 [details] Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Chris Dumez
Comment 21 2018-06-14 11:52:42 PDT
As explained offline, to facilitate landing the change, we have the following suggestions: 1. Do not include any new API in the first patch (i.e. no client delegate) 2. Do not try and fix history just yet 3. I believe Alex might have some concerns about the global SafeBrowsingContextProvider too? Alex, could you please clarify? This should keep the patch a lot simpler and facilitate its review / prompt landing. We can build on it in follow-up patches.
Ali Juma
Comment 22 2018-06-18 15:19:24 PDT
Ali Juma
Comment 23 2018-06-18 15:20:28 PDT
(In reply to Chris Dumez from comment #21) > As explained offline, to facilitate landing the change, we have the > following suggestions: > 1. Do not include any new API in the first patch (i.e. no client delegate) Done, removed the client delegate method. > 2. Do not try and fix history just yet Removed too.
Alex Christensen
Comment 24 2018-06-18 15:40:57 PDT
(In reply to Chris Dumez from comment #21) > 3. I believe Alex might have some concerns about the global > SafeBrowsingContextProvider too? Alex, could you please clarify? Yes, right now it's being used to hold a global static boolean that is set via an instance method with no corresponding accessor. We should really avoid global things in the WKWebView API design because they prevent us from having multiple WKWebViews with different settings.
EWS Watchlist
Comment 25 2018-06-19 04:02:20 PDT
Comment on attachment 342977 [details] Patch Attachment 342977 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/8245511 New failing tests: accessibility/mac/selection-notification-focus-change.html
EWS Watchlist
Comment 26 2018-06-19 04:02:22 PDT
Created attachment 343043 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Ali Juma
Comment 27 2018-06-19 13:28:32 PDT
Ali Juma
Comment 28 2018-06-19 13:29:51 PDT
(In reply to Alex Christensen from comment #24) > (In reply to Chris Dumez from comment #21) > > 3. I believe Alex might have some concerns about the global > > SafeBrowsingContextProvider too? Alex, could you please clarify? > Yes, right now it's being used to hold a global static boolean that is set > via an instance method with no corresponding accessor. We should really > avoid global things in the WKWebView API design because they prevent us from > having multiple WKWebViews with different settings. Good point, moved the boolean to WKWebViewConfiguration.
Ali Juma
Comment 29 2018-07-04 11:42:19 PDT
Created attachment 344296 [details] patch Rebased
Ali Juma
Comment 30 2018-07-04 12:04:09 PDT
Created attachment 344297 [details] Patch Rebased
Radar WebKit Bug Importer
Comment 31 2020-01-06 21:25:03 PST
Brent Fulgham
Comment 32 2020-01-14 12:31:12 PST
This was shipped in WebKit -- I'm not sure why this bug wasn't updated to reflect that.
Alex Christensen
Comment 33 2021-11-01 12:13:45 PDT
Comment on attachment 344297 [details] Patch This was done elsewhere.
Note You need to log in before you can comment on or make changes to this bug.