Downgrade lock icon for broken-HTTPS subresources

Downgrade lock icon for broken-HTTPS subresources

This CL attaches a boolean to resource responses to indicate if they
have certificate errors. If Blink sees a resource with a cert error, it
notifies the renderer via FrameLoaderClient, who then notifies the
browser, who treats the situation like mixed content.

The browser (//content) ignores subresources with cert errors on HTTP
pages, and subresources with the same cert errors as the main
resource. This allows embedders to distinguish broken-HTTPS foo.com with
a subresource from broken-HTTPS bar.com and broken-HTTPS foo.com with a
subresource from broken-HTTPS foo.com.

BUG=477868
Committed: https://crrev.com/8bfb78c859ab5993eada6db30e4de50aa7403f1c
Cr-Commit-Position: refs/heads/master@{#362246}

[estark, 2015-11-19 23:28:55]

estark@chromium.org changed reviewers:
+ jww@chromium.org

[estark, 2015-11-19 23:28:56]

jww: do you think you could take a first pass at reviewing this? It implements
an old TODO from abarth to plumb certificate-error info on subresources into
Blink, and then send a signal out of Blink when the page displays or runs
broken-https subresources.

Two notes/caveats:

1.) I think we should do this as a first step, and then I'll look into showing
the mixed content shield for broken-https subresources or just blocking them
outright all the time. (The latter is somewhat appealing because we want to get
rid of the mixed content shield at some point anyway.)

2.) I can't quite convince myself that the following race is impossible: on page
1, Blink submits a broken-https subresource request to the browser, then the
page navigates to page 2, then the subresource response comes in and causes the
UI to downgrade for page 2. Would appreciate thoughts on whether this does or
doesn't seem possible, or how to write a test for it. (So far my efforts to
write a test for this case have been unsuccessful.)

[jww, 2015-11-20 01:25:09]

Woo-hoo! Thank you so much for putting in the effort on this one. This is
looking pretty good. Mostly nits, some questions, and a handful of larger
issues. Thanks!

https://codereview.chromium.org/1415923015/diff/80001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1415923015/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:2202: // would be confusing and
duplicative.)
Mention that this is testing *active* content?

https://codereview.chromium.org/1415923015/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:2213: // Like the test above, but only
displaying an image.
Mention that this is testing *inactive* content?

https://codereview.chromium.org/1415923015/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:2221:
ASSERT_TRUE(https_server_mismatched_.Start());
It seems like a lot of this is similar/identical boilerplate to the above test.
Can you factor it out into a helper function? Would be much easier to read and
understand this test if it was just:
loadBoilerplate();
doInterestingStuff();

https://codereview.chromium.org/1415923015/diff/80001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1415923015/diff/80001/content/browser/loader/...
content/browser/loader/resource_loader.cc:115: response->head.security_info =
SerializeSecurityInfo(ssl_status);
nit: This doesn't seem like a great name; it's really more like
has_major_certificate_errors (or something like that).

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:141: if (!entry || !entry->GetSSL().cert_id)
We talked about this a while back, and I forget all the details, but doesn't a
cert_id of 0 indicate that it's unset *or* invalid? And if the SSLStatus is
invalid, is that really "no errors"? These are really honest questions, because
I can't remember.

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:150: // subresource errors duplicative.
Why is security_style not covered by this? Because all of the following bits
imply a particular style?

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.h (right):

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.h:85: // errors are relevant to the SSL UI for
the page, or false if it should
nit: missing word? "errors are" -> "errors that are"?

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.h:92: bool
IsContentWithCertificateErrorsRelevant(const GURL& url,
nit: I'd love for this name to say something like "...RelevantToUI", b/c the
name is unclear about what it's relevant to.

https://codereview.chromium.org/1415923015/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1415923015/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3160: void
WebContentsImpl::OnDidDisplayContentWithCertificateErrors(
Ignorant question: It seems really heavy weight to have an IPC to query on every
resource load with cert errors (esp. since it generally will do nothing and
return). Is there no way we can cache stuff in the renderer so that it can
answer its own question? I assume that's the case, but I just want to make sure
it has been considered :-)

https://codereview.chromium.org/1415923015/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3172: ssl)) {
nit: Was this a git cl format decision? If so, fine, but looks weird.

https://codereview.chromium.org/1415923015/diff/80001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1415923015/diff/80001/content/common/frame_me...
content/common/frame_messages.h:1224: std::string /* security_origin */,
While you're digging around in here... :-) Can you convert this std::string for
security_origin to a GURL? Given that it originates in a WebURL, and I believe
you've removed the only other calling location, and ultimately, in it's one use
spot
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ss...)
it's converted *back* to a GURL, I think it would be better to just leave it
as-is. I've found this string security_origin a source of confusion over the
years.

https://codereview.chromium.org/1415923015/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1415923015/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:3647: routing_id_,
origin.toString().utf8(), url, security_info));
As I said earlier, I think it would make much more sense to just send this
through as a WebURL.

https://codereview.chromium.org/1415923015/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1415923015/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:524: const blink::WebCString&
security_info) override;
Why are these WebCStrings instead of WebStrings? I don't see WebCStrings passed
around very much.

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:461: void
MixedContentChecker::handleCertificateError(LocalFrame* frame, const KURL& url,
WebURLRequest::RequestContext requestContext, WebURLRequest::FrameType
frameType, const CString& securityInfo)
Do we have any MixedContentChecker unit tests yet? I'd love see some unit tests
for this function to verify that inputs give you expected IPCs out of the
renderer. Maybe it would be easier to do that on the content/ side?

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:463: if (frameType
== WebURLRequest::FrameTypeTopLevel || !frame)
How can we can !frame? Can this be an ASSERT?

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:472:
ASSERT(contextType != ContextTypeNotMixedContent);
I guess this ASSERT exists because handleCertificateError should never be called
when there isn't mixed content? If so, can you add a comment with this invariant
above the function?

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.h (right):

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.h:78: static void
handleCertificateError(LocalFrame*, const KURL&, WebURLRequest::RequestContext,
WebURLRequest::FrameType, const CString& securityInfo);
Yeah, this should almost certainly be a WTF::String instead of a CString.

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ResourceResponse.h (right):

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ResourceResponse.h:164: void
setHasCertificateErrors(bool value) { m_hasCertificateErrors = value; }
nit: rename 'value' to something more meaningful, like has_cert_errors.

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:452: // active content (such as a
script) from a connection with
Please make it more clear that the first half of this comment refers to the
first function, and the second half refers to the second function.

[estark, 2015-11-23 23:40:24]

Thanks for the review, jww! I did most of what you suggested but had a couple
questions inline -- mostly about the WTF::string vs CString issue -- and also am
still looking into how to best write unit tests for
MixedContentChecker::handleCertificateErrors().

https://codereview.chromium.org/1415923015/diff/80001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1415923015/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:2202: // would be confusing and
duplicative.)
On 2015/11/20 01:25:08, jww wrote:
> Mention that this is testing *active* content?

Done.

https://codereview.chromium.org/1415923015/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:2213: // Like the test above, but only
displaying an image.
On 2015/11/20 01:25:08, jww wrote:
> Mention that this is testing *inactive* content?

Done.

https://codereview.chromium.org/1415923015/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:2221:
ASSERT_TRUE(https_server_mismatched_.Start());
On 2015/11/20 01:25:08, jww wrote:
> It seems like a lot of this is similar/identical boilerplate to the above
test.
> Can you factor it out into a helper function? Would be much easier to read and
> understand this test if it was just:
> loadBoilerplate();
> doInterestingStuff();

Done.

https://codereview.chromium.org/1415923015/diff/80001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1415923015/diff/80001/content/browser/loader/...
content/browser/loader/resource_loader.cc:115: response->head.security_info =
SerializeSecurityInfo(ssl_status);
On 2015/11/20 01:25:08, jww wrote:
> nit: This doesn't seem like a great name; it's really more like
> has_major_certificate_errors (or something like that).

Done.

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:141: if (!entry || !entry->GetSSL().cert_id)
On 2015/11/20 01:25:08, jww wrote:
> We talked about this a while back, and I forget all the details, but doesn't a
> cert_id of 0 indicate that it's unset *or* invalid? And if the SSLStatus is
> invalid, is that really "no errors"? These are really honest questions,
because
> I can't remember.

So I ended up moving this to the renderer and changing the check to just be
SchemeIsCryptographic() on the main resource. Does that seem right to you?

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:150: // subresource errors duplicative.
On 2015/11/20 01:25:08, jww wrote:
> Why is security_style not covered by this? Because all of the following bits
> imply a particular style?

I think just an omission on my part. (The following bits do imply a particular
style, but I think it's better not to rely on that.)

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.h (right):

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.h:85: // errors are relevant to the SSL UI for
the page, or false if it should
On 2015/11/20 01:25:08, jww wrote:
> nit: missing word? "errors are" -> "errors that are"?

Done.

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.h:92: bool
IsContentWithCertificateErrorsRelevant(const GURL& url,
On 2015/11/20 01:25:08, jww wrote:
> nit: I'd love for this name to say something like "...RelevantToUI", b/c the
> name is unclear about what it's relevant to.

Done.

https://codereview.chromium.org/1415923015/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1415923015/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3160: void
WebContentsImpl::OnDidDisplayContentWithCertificateErrors(
On 2015/11/20 01:25:08, jww wrote:
> Ignorant question: It seems really heavy weight to have an IPC to query on
every
> resource load with cert errors (esp. since it generally will do nothing and
> return). Is there no way we can cache stuff in the renderer so that it can
> answer its own question? I assume that's the case, but I just want to make
sure
> it has been considered :-)

Ah, when I was first working on this, I couldn't figure out how to do this in
the renderer, but I think I was just being slow. Done.

https://codereview.chromium.org/1415923015/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3172: ssl)) {
On 2015/11/20 01:25:08, jww wrote:
> nit: Was this a git cl format decision? If so, fine, but looks weird.

yep :(

https://codereview.chromium.org/1415923015/diff/80001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1415923015/diff/80001/content/common/frame_me...
content/common/frame_messages.h:1224: std::string /* security_origin */,
On 2015/11/20 01:25:08, jww wrote:
> While you're digging around in here... :-) Can you convert this std::string
for
> security_origin to a GURL? Given that it originates in a WebURL, and I believe
> you've removed the only other calling location, and ultimately, in it's one
use
> spot
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ss...)
> it's converted *back* to a GURL, I think it would be better to just leave it
> as-is. I've found this string security_origin a source of confusion over the
> years.

Done.

https://codereview.chromium.org/1415923015/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1415923015/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:3647: routing_id_,
origin.toString().utf8(), url, security_info));
On 2015/11/20 01:25:08, jww wrote:
> As I said earlier, I think it would make much more sense to just send this
> through as a WebURL.

Done.

https://codereview.chromium.org/1415923015/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1415923015/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:524: const blink::WebCString&
security_info) override;
On 2015/11/20 01:25:08, jww wrote:
> Why are these WebCStrings instead of WebStrings? I don't see WebCStrings
passed
> around very much.

I was going off of other APIs that expose security_info, which is an opaque
serialized blob of data, e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
and
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I'm not sure why these existing APIs used WebCString though. Any thoughts?

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:461: void
MixedContentChecker::handleCertificateError(LocalFrame* frame, const KURL& url,
WebURLRequest::RequestContext requestContext, WebURLRequest::FrameType
frameType, const CString& securityInfo)
On 2015/11/20 01:25:08, jww wrote:
> Do we have any MixedContentChecker unit tests yet? I'd love see some unit
tests
> for this function to verify that inputs give you expected IPCs out of the
> renderer. Maybe it would be easier to do that on the content/ side?

Acknowledged, but haven't done this yet. MixedContentChecker unit tests seem
tricky because a.) I'm not sure there's a way to mock enough stuff in unit
tests, e.g. I can create a DummyPageHolder but I'm not sure if there's a way to
set the securityInfo() on the resulting document, and b.) they wouldn't actually
test what you're interested in testing (that the expected IPCs come out of the
renderer). On the other hand, there would be render_frame_impl_browsertest.cc,
but those seem pretty limited.

So, tl;dr I'm still looking into this.

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:463: if (frameType
== WebURLRequest::FrameTypeTopLevel || !frame)
On 2015/11/20 01:25:08, jww wrote:
> How can we can !frame? Can this be an ASSERT?

I put this here because shouldBlockFetch() has a similar check; I'm not sure how
a request can be frameless but it does seem to be a possibility (see line 82).

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:472:
ASSERT(contextType != ContextTypeNotMixedContent);
On 2015/11/20 01:25:08, jww wrote:
> I guess this ASSERT exists because handleCertificateError should never be
called
> when there isn't mixed content? If so, can you add a comment with this
invariant
> above the function?

Actually, it's because contextTypeFromContext() never returns anything but
Blockable/ShouldBeBlockable/OptionallyBlockable. I'll add a comment here.

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.h (right):

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.h:78: static void
handleCertificateError(LocalFrame*, const KURL&, WebURLRequest::RequestContext,
WebURLRequest::FrameType, const CString& securityInfo);
On 2015/11/20 01:25:08, jww wrote:
> Yeah, this should almost certainly be a WTF::String instead of a CString.

See response to previous comment about WebCString; do you think so even though
ResourceResponse::getSecurityInfo() returns a CString?

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ResourceResponse.h (right):

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ResourceResponse.h:164: void
setHasCertificateErrors(bool value) { m_hasCertificateErrors = value; }
On 2015/11/20 01:25:08, jww wrote:
> nit: rename 'value' to something more meaningful, like has_cert_errors.

Done.

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:452: // active content (such as a
script) from a connection with
On 2015/11/20 01:25:09, jww wrote:
> Please make it more clear that the first half of this comment refers to the
> first function, and the second half refers to the second function.

Done.

[estark, 2015-11-25 00:18:08]

added a MixedContentChecker unit test

[jww, 2015-11-25 19:24:03]

lgtm, thanks!

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/1415923015/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:141: if (!entry || !entry->GetSSL().cert_id)
On 2015/11/23 23:40:24, estark wrote:
> On 2015/11/20 01:25:08, jww wrote:
> > We talked about this a while back, and I forget all the details, but doesn't
a
> > cert_id of 0 indicate that it's unset *or* invalid? And if the SSLStatus is
> > invalid, is that really "no errors"? These are really honest questions,
> because
> > I can't remember.
> 
> So I ended up moving this to the renderer and changing the check to just be
> SchemeIsCryptographic() on the main resource. Does that seem right to you?

It doesn't seem like there's any increased security risk here, since ultimately
the renderer is deciding the state anyway, and his could all be ignored. The
only consideration is if this will someday all end up being in the Browser
anyway, but I'm not too worried about that.

https://codereview.chromium.org/1415923015/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1415923015/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:524: const blink::WebCString&
security_info) override;
On 2015/11/23 23:40:24, estark wrote:
> On 2015/11/20 01:25:08, jww wrote:
> > Why are these WebCStrings instead of WebStrings? I don't see WebCStrings
> passed
> > around very much.
> 
> I was going off of other APIs that expose security_info, which is an opaque
> serialized blob of data, e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> and
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> I'm not sure why these existing APIs used WebCString though. Any thoughts?

Nope, not really. Can you email chromium-dev or blink-dev to try and figure out
why we would use one over the other? I don't care too much, though, so we don't
need to block on that.

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:463: if (frameType
== WebURLRequest::FrameTypeTopLevel || !frame)
On 2015/11/23 23:40:24, estark wrote:
> On 2015/11/20 01:25:08, jww wrote:
> > How can we can !frame? Can this be an ASSERT?
> 
> I put this here because shouldBlockFetch() has a similar check; I'm not sure
how
> a request can be frameless but it does seem to be a possibility (see line 82).

Hm, maybe Service Worker or something? Anyway, this is fine.

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/MixedContentChecker.h (right):

https://codereview.chromium.org/1415923015/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/MixedContentChecker.h:78: static void
handleCertificateError(LocalFrame*, const KURL&, WebURLRequest::RequestContext,
WebURLRequest::FrameType, const CString& securityInfo);
On 2015/11/23 23:40:24, estark wrote:
> On 2015/11/20 01:25:08, jww wrote:
> > Yeah, this should almost certainly be a WTF::String instead of a CString.
> 
> See response to previous comment about WebCString; do you think so even though
> ResourceResponse::getSecurityInfo() returns a CString?

Ah, I see. I missed that. Nevermind then.

[estark, 2015-11-25 19:43:55]

estark@chromium.org changed reviewers:
+ mkwst@chromium.org

[estark, 2015-11-25 19:43:55]

Thanks Joel! Mike, another one for you take a look at when you have a chance.
(Sorry, this CL is kind of large... I can break it up into smaller CLs if the
overall design looks reasonable to you.)

[Mike West, 2015-11-27 05:30:38]

The Blink side of things LGTM % comments. I haven't looked at the content side,
as I expect Joel's more familiar with that code than I am.

Regarding WebString vs WebCString, did you get enough of an answer to your
question on blink-dev@?

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:242:
MixedContentChecker::handleCertificateError(MixedContentChecker::effectiveFrameForFrameType(frame(),
resourceLoader->originalRequest().frameType()), response.url(),
resourceLoader->originalRequest().requestContext(),
resourceLoader->originalRequest().frameType(), response.getSecurityInfo());
Nit: This could be simpler to understand if you just passed the frame, the
original request, and the response into the new MixedContentChecker method.
Extracting the relevant data there seems like a cleaner approach than extracting
it all here. I suppose it does make things easier to test, however. *shrug* I
can live with it either way, this just seems inelegant. :)

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:476: }
Should we output a console message noting the responsible resource? Or are we
leaving that up to the security panel? If the latter, will we be moving all
mixed content messages there?

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/MixedContentCheckerTest.cpp (right):

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentCheckerTest.cpp:73: enum
CertErrorsContentType {
Nit: No indentation.

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentCheckerTest.cpp:131: };
This is fine, but I suspect you could do it more simply with a mock and some
EXPECT_CALL macros.

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentCheckerTest.cpp:150:
MixedContentChecker::handleCertificateError(&dummyPageHolder->frame(),
displayedUrl, WebURLRequest::RequestContextImage, WebURLRequest::FrameTypeNone,
"");
In a magical future world where we've blocked all mixed content, this will fail,
because RequestContextImage will be blockable. Would you mind humoring me by
adding an `ASSERT_EQ` that `MixedContentChecker::contextTypeFromContext` returns
optionally blockable? :)

[estark, 2015-11-28 02:46:56]

estark@chromium.org changed reviewers:
+ jochen@chromium.org

[estark, 2015-11-28 02:46:56]

Thanks Mike!

jochen: do you think you could review the content/ part of this?

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:242:
MixedContentChecker::handleCertificateError(MixedContentChecker::effectiveFrameForFrameType(frame(),
resourceLoader->originalRequest().frameType()), response.url(),
resourceLoader->originalRequest().requestContext(),
resourceLoader->originalRequest().frameType(), response.getSecurityInfo());
On 2015/11/27 05:30:38, Mike West wrote:
> Nit: This could be simpler to understand if you just passed the frame, the
> original request, and the response into the new MixedContentChecker method.
> Extracting the relevant data there seems like a cleaner approach than
extracting
> it all here. I suppose it does make things easier to test, however. *shrug* I
> can live with it either way, this just seems inelegant. :)

Done.

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:476: }
On 2015/11/27 05:30:38, Mike West wrote:
> Should we output a console message noting the responsible resource? Or are we
> leaving that up to the security panel? If the latter, will we be moving all
> mixed content messages there?

Done. I think we should keep the console messages in addition to making note of
mixed content in the security panel. (In  a follow-up CL, I'll be tweaking the
security panel UI for resources with certificate errors. Right now they'll show
up as normal mixed content in the security panel.)

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/MixedContentCheckerTest.cpp (right):

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentCheckerTest.cpp:73: enum
CertErrorsContentType {
On 2015/11/27 05:30:38, Mike West wrote:
> Nit: No indentation.

Hmm, `git cl format` seems to like it this way.

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentCheckerTest.cpp:131: };
On 2015/11/27 05:30:38, Mike West wrote:
> This is fine, but I suspect you could do it more simply with a mock and some
> EXPECT_CALL macros.

Ah, that is much better, thanks!

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentCheckerTest.cpp:150:
MixedContentChecker::handleCertificateError(&dummyPageHolder->frame(),
displayedUrl, WebURLRequest::RequestContextImage, WebURLRequest::FrameTypeNone,
"");
On 2015/11/27 05:30:38, Mike West wrote:
> In a magical future world where we've blocked all mixed content, this will
fail,
> because RequestContextImage will be blockable. Would you mind humoring me by
> adding an `ASSERT_EQ` that `MixedContentChecker::contextTypeFromContext`
returns
> optionally blockable? :)

Done.

[dcheng, 2015-11-28 09:52:11]

Drive by

https://codereview.chromium.org/1415923015/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1415923015/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:454: // This frame has run
inactive content (such as a script) from a
Isn't script "active" content?

[jochen (gone - plz use gerrit), 2015-11-30 15:53:50]

lgtm

[estark, 2015-11-30 18:13:27]

Thanks jochen.

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/1415923015/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/MixedContentChecker.cpp:476: }
On 2015/11/28 02:46:56, estark wrote:
> On 2015/11/27 05:30:38, Mike West wrote:
> > Should we output a console message noting the responsible resource? Or are
we
> > leaving that up to the security panel? If the latter, will we be moving all
> > mixed content messages there?
> 
> Done. I think we should keep the console messages in addition to making note
of
> mixed content in the security panel. (In  a follow-up CL, I'll be tweaking the
> security panel UI for resources with certificate errors. Right now they'll
show
> up as normal mixed content in the security panel.)

Mike: Oops, this turned out to be more complicated than I thought. Logging to
console here doesn't do quite the right thing (as revealed by the test
failures): it'll print a console message even if content/ decides that the
certificate error isn't relevant. I think to fix this the console message will
have to be initiated from content/ after it decides what to do with the
certificate error, which is totally do-able, but I think I'll do it in a
follow-up CL since this one is already quite large.

https://codereview.chromium.org/1415923015/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1415923015/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:454: // This frame has run
inactive content (such as a script) from a
On 2015/11/28 09:52:11, dcheng wrote:
> Isn't script "active" content?

Thanks -- that was a mistake, fixed.

[estark, 2015-11-30 21:12:49]

The CQ bit was checked by estark@chromium.org

[estark, 2015-11-30 21:12:49]

The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org,
mkwst@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1415923015/#ps200001
(title: "remove console message; see comment to mike")

[commit-bot: I haz the power, 2015-11-30 21:14:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415923015/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415923015/200001

[commit-bot: I haz the power, 2015-11-30 22:39:58]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-11-30 22:40:48]

Description was changed from

==========
Downgrade lock icon for broken-HTTPS subresources

This CL attaches a boolean to resource responses to indicate if they
have certificate errors. If Blink sees a resource with a cert error, it
notifies the renderer via FrameLoaderClient, who then notifies the
browser, who treats the situation like mixed content.

The browser (//content) ignores subresources with cert errors on HTTP
pages, and subresources with the same cert errors as the main
resource. This allows embedders to distinguish broken-HTTPS foo.com with
a subresource from broken-HTTPS bar.com and broken-HTTPS foo.com with a
subresource from broken-HTTPS foo.com.

BUG=477868
==========

to

==========
Downgrade lock icon for broken-HTTPS subresources

This CL attaches a boolean to resource responses to indicate if they
have certificate errors. If Blink sees a resource with a cert error, it
notifies the renderer via FrameLoaderClient, who then notifies the
browser, who treats the situation like mixed content.

The browser (//content) ignores subresources with cert errors on HTTP
pages, and subresources with the same cert errors as the main
resource. This allows embedders to distinguish broken-HTTPS foo.com with
a subresource from broken-HTTPS bar.com and broken-HTTPS foo.com with a
subresource from broken-HTTPS foo.com.

BUG=477868

Committed: https://crrev.com/8bfb78c859ab5993eada6db30e4de50aa7403f1c
Cr-Commit-Position: refs/heads/master@{#362246}
==========

[commit-bot: I haz the power, 2015-11-30 22:40:49]

Patchset 11 (id:??) landed as
https://crrev.com/8bfb78c859ab5993eada6db30e4de50aa7403f1c
Cr-Commit-Position: refs/heads/master@{#362246}

[estark, 2015-12-05 00:21:32]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/1497423002/ by estark@chromium.org.

The reason for reverting is: Speculatively reverting to see if it makes
https://code.google.com/p/chromium/issues/detail?id=565540 go away.