New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 752370 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug
Team-Security-UX


Sign in to add a comment

Interstitial refactor: make a SSLErrorThrottle NavigationThrottle in //chrome

Project Member Reported by est...@chromium.org, Aug 4 2017

Issue description

Design doc pointer: https://docs.google.com/document/d/1rEBpw5V-Nn1UIi8CIFa5ZZvwlR08SkY3CogvWE2UMFs/edit#bookmark=id.eqx4o74cracw

The blocking bugs introduce plumbing so that certificate errors can be exposed as NavigationThrottle methods. Now we need something to actually implement that NavigationThrottle.

Add a new NavigationThrottle class in chrome/browser/ssl, called, say, SSLErrorThrottle. This class should implement the OnSSLCertificateError method that we're introducing in  issue 751941 .

The implementation of this method should be similar to what SSLErrorHandler does today to decide what interstitial to show. We might need to factor some code out of SSLErrorHandler or expose it so that SSLErrorThrottle can use it. (When we get rid of the old interstitials eventually, we probably want to delete SSLErrorHandler.)

When SSLErrorThrottle decides what type of error to show, it should do something similar to the pseudocode in the design doc (https://docs.google.com/document/d/1rEBpw5V-Nn1UIi8CIFa5ZZvwlR08SkY3CogvWE2UMFs/edit#bookmark=id.eqx4o74cracw): construct a blocking page, call its GetHTMLContents() method (which we will need to make public for this purpose), and call CancelDeferredNavigationWithCustomError() on the navigation handle.

If we want to start working on this bug before the blocking bugs are finished, we can implement all the SSLErrorThrottle logic; it just won't yet override a real NavigationThrottle method until  issue 751941  is done and it won't yet be able to call CancelDeferredNavigationWithCustomError() until it's introduced in  issue 751946  is done. 
 
Blocking: 448486
By the way, SSLErrorThrottle's logic should be gated behind a flag so that we don't actually trigger the new interstitial code path until we're ready for it. (In particular, tests have to be adapted and we need to do manual testing and such.)
Labels: -Pri-3 M-63 Pri-2
Blocking: 752372
Update: per revised design, SSLErrorThrottle is no longer going to implement a method that observes certificate errors, but rather a method that observes request failures (NavigationThrottle::OnRequestFailed). When it observes this method being called for a main-frame navigation with a certificate error net error code, then it should defer the navigation, decide what type of error page to show, and then call CancelDeferredNavigationWithCustomError().

Comment 6 by est...@chromium.org, Aug 18 2017

Cc: est...@chromium.org
Owner: lgar...@chromium.org
Labels: Proj-CommittedInterstitials
Blockedon: 768105
Blockedon: 778857
Blockedon: 780212
Labels: Hotlist-EnamelAndFriendsFixIt
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/94b16051b8d459526af6e72203205d8c3c496ae6

commit 94b16051b8d459526af6e72203205d8c3c496ae6
Author: Lucas Garron <lgarron@chromium.org>
Date: Sat Nov 11 01:58:12 2017

Create SSLErrorNavigationThrottle.

The committed interstitials project uses SSLErrorNavigationThrottle to watch for
failing requests that are certificate errors. It defers such requests, and then
cancels with the appropriate error code and error page content HTML based on the
blocking page it receives from calling SSLErrorHandler::HandleSSLError, via a
"blocking page ready callback".

To support this, HandleSSLError is updated to pass its constructed blocking page
to the blocking page ready callback when the callback is present, *instead* of
calling Show() on the blocking page. Since this passes ownership to the
SSLErrorNavigationThrottle, we also create a new SSLErrorTabHelper that
SSLErrorNavigationThrottle uses to associate a blocking page with its web
contents for as long as it'sr needed (i.e. until its navigation has committed
and the tab has subsequently navigated away).

Bug:  752370 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I6525912a28e48b9268aa0f9798b5e1b276cb25a6
Reviewed-on: https://chromium-review.googlesource.com/621236
Commit-Queue: Lucas Garron <lgarron@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515789}
[modify] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/chrome/browser/BUILD.gn
[modify] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/chrome/browser/ssl/ssl_blocking_page.h
[modify] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/chrome/browser/ssl/ssl_browser_tests.cc
[modify] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/chrome/browser/ssl/ssl_error_handler.cc
[modify] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/chrome/browser/ssl/ssl_error_handler.h
[add] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/chrome/browser/ssl/ssl_error_navigation_throttle.cc
[add] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/chrome/browser/ssl/ssl_error_navigation_throttle.h
[add] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/chrome/browser/ssl/ssl_error_navigation_throttle_unittest.cc
[add] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/chrome/browser/ssl/ssl_error_tab_helper.cc
[add] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/chrome/browser/ssl/ssl_error_tab_helper.h
[add] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/chrome/browser/ssl/ssl_error_tab_helper_unittest.cc
[modify] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/chrome/test/BUILD.gn
[modify] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/components/security_interstitials/content/security_interstitial_page.cc
[modify] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/components/security_interstitials/content/security_interstitial_page.h
[modify] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/content/browser/frame_host/navigation_handle_impl.h
[modify] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/content/public/browser/navigation_handle.h
[modify] https://crrev.com/94b16051b8d459526af6e72203205d8c3c496ae6/content/public/browser/navigation_throttle.h

Project Member

Comment 13 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cde41d11f134b91918bfc75d93369e0575f4f333

commit cde41d11f134b91918bfc75d93369e0575f4f333
Author: John Abd-El-Malek <jam@chromium.org>
Date: Sat Nov 11 17:38:35 2017

Update network service test filter

TBR=yzshen@chromium.org
NOTRY=true

Bug:  781565 ,  752370 
Change-Id: Ic14467588e28081173625323dea6dd5dd99a4f0e
Reviewed-on: https://chromium-review.googlesource.com/764811
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515846}
[modify] https://crrev.com/cde41d11f134b91918bfc75d93369e0575f4f333/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter
[modify] https://crrev.com/cde41d11f134b91918bfc75d93369e0575f4f333/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService

Labels: -M-63 M-64
Status: Fixed (was: Assigned)

Sign in to add a comment