New issue
Advanced search Search tips

Issue 659713 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Sub-resource warning doesn't trigger "dangerous" indicator

Project Member Reported by nparker@chromium.org, Oct 26 2016

Issue description

Version: 54.0.2840.71 (stable) linux, and 56.0.2897.0 (dev)

Repro:
* Visit http://parkerly.com/sb-tests/testing_links.html
* Click "bad-subresources"
* It'll show a warning, but w/o a "dangerous" indicator
* Click back-to-safety or back, and the dangerous indicator will then appear

Similar thing happens on HTTPS.
 
pre-warning.png
10.1 KB View Download
warning.png
6.2 KB View Download
post-warning.png
9.6 KB View Download

Comment 1 by est...@chromium.org, Oct 26 2016

On the HTTPS version, we strike-through the https:// when you go back. Yikes!!!

Comment 2 by est...@chromium.org, Oct 26 2016

Spun out the crossed-out scheme bug into issue 659725.

For the missing Dangerous icon, it looks like we need to trigger an omnibox update from SafeBrowsingUIManager::AddToWhitelistUrlSet. We probably need a new WebContents method to do that, along the lines of WebContentsImpl::DidChangeVisibleSSLState and its corresponding WebContentsDelegate method.

Comment 3 by est...@chromium.org, Oct 27 2016

Status: Started (was: Untriaged)
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 28 2016

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

commit 8bb181295c47ba4d74a626f809a1a27423544f2f
Author: estark <estark@chromium.org>
Date: Thu Oct 27 23:57:18 2016

Trigger Dangerous indicator for unsafe subresources

Previously, the Dangerous indicator was not firing for subresources
flagged by Safe Browsing as unsafe. This is because marking a
subresource as unsafe was not triggering an omnibox update. We want to
trigger an omnibox update whenever a URL is marked as
whitelisted (either pending, meaning that an interstitial is showing, or
whitelisted, meaning that an interstitial has been clicked through) in
SafeBrowsingUIManager.

To do so, this CL renames WebContentsImpl::DidChangeVisibleSSLState() to
DidChangeVisibleSecurityState() (since it's no longer just SSL
information that can affect the omnibox security UI) and moves it to
WebContents so that it can be called from SafeBrowsingUIManager.

BUG= 659713 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2444383007
Cr-Commit-Position: refs/heads/master@{#428223}

[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/chrome/browser/safe_browsing/ui_manager.cc
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/chrome/browser/safe_browsing/ui_manager_unittest.cc
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/chrome/browser/ssl/chrome_security_state_model_client.cc
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/chrome/browser/ssl/chrome_security_state_model_client.h
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/chrome/browser/ui/browser.cc
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/chrome/browser/ui/browser.h
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/components/web_contents_delegate_android/web_contents_delegate_android.cc
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/components/web_contents_delegate_android/web_contents_delegate_android.h
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/content/browser/frame_host/interstitial_page_impl.cc
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/content/browser/ssl/ssl_manager.cc
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/content/browser/ssl/ssl_manager.h
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/content/browser/ssl/ssl_manager_unittest.cc
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/content/public/browser/web_contents.h
[modify] https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f/content/public/browser/web_contents_delegate.h

Comment 5 by est...@chromium.org, Oct 28 2016

Labels: M-56
Status: Fixed (was: Started)
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 28 2016

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

commit 201e61b48c2f4ee497ca1fc09217ad8a190b829b
Author: tommycli <tommycli@chromium.org>
Date: Fri Oct 28 22:22:33 2016

Revert of Trigger Dangerous indicator for unsafe subresources (patchset #4 id:60001 of https://codereview.chromium.org/2444383007/ )

Reason for revert:
Breaks MSan tests:

First breaking build:
https://build.chromium.org/p/chromium.memory.full/builders/Linux%20MSan%20Tests/builds/2755

Original issue's description:
> Trigger Dangerous indicator for unsafe subresources
>
> Previously, the Dangerous indicator was not firing for subresources
> flagged by Safe Browsing as unsafe. This is because marking a
> subresource as unsafe was not triggering an omnibox update. We want to
> trigger an omnibox update whenever a URL is marked as
> whitelisted (either pending, meaning that an interstitial is showing, or
> whitelisted, meaning that an interstitial has been clicked through) in
> SafeBrowsingUIManager.
>
> To do so, this CL renames WebContentsImpl::DidChangeVisibleSSLState() to
> DidChangeVisibleSecurityState() (since it's no longer just SSL
> information that can affect the omnibox security UI) and moves it to
> WebContents so that it can be called from SafeBrowsingUIManager.
>
> BUG= 659713 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Committed: https://crrev.com/8bb181295c47ba4d74a626f809a1a27423544f2f
> Cr-Commit-Position: refs/heads/master@{#428223}

TBR=nparker@chromium.org,jam@chromium.org,estark@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 659713 

Review-Url: https://codereview.chromium.org/2461093002
Cr-Commit-Position: refs/heads/master@{#428520}

[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/chrome/browser/safe_browsing/ui_manager.cc
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/chrome/browser/safe_browsing/ui_manager_unittest.cc
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/chrome/browser/ssl/chrome_security_state_model_client.cc
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/chrome/browser/ssl/chrome_security_state_model_client.h
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/chrome/browser/ui/browser.cc
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/chrome/browser/ui/browser.h
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/components/web_contents_delegate_android/web_contents_delegate_android.cc
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/components/web_contents_delegate_android/web_contents_delegate_android.h
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/content/browser/frame_host/interstitial_page_impl.cc
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/content/browser/ssl/ssl_manager.cc
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/content/browser/ssl/ssl_manager.h
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/content/browser/ssl/ssl_manager_unittest.cc
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/content/public/browser/web_contents.h
[modify] https://crrev.com/201e61b48c2f4ee497ca1fc09217ad8a190b829b/content/public/browser/web_contents_delegate.h

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 29 2016

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

commit 379e8d63ac82486f3b8423fde91132cd4abb77da
Author: estark <estark@chromium.org>
Date: Sat Oct 29 00:11:20 2016

Initialize UnsafeResource::is_subframe

https://codereview.chromium.org/2444383007/ triggered msan failures
because this field was not being initialized.

BUG= 659713 
TBR=nparker@chromium.org

Review-Url: https://codereview.chromium.org/2458223002
Cr-Commit-Position: refs/heads/master@{#428553}

[modify] https://crrev.com/379e8d63ac82486f3b8423fde91132cd4abb77da/chrome/browser/safe_browsing/ui_manager.cc

Components: -Security>UX
Labels: Team-Security-UX
Security>UX component is deprecated in favor of the Team-Security-UX label

Sign in to add a comment