Bug 24696 – Enable mixed content detection

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Adam Barth]

In order to properly compute our mixed content state, we need to attach some additional security information to resource requests.  Patch forthcoming.

[Adam Barth]

Created attachment 28749 [details]
patchzor

I don't actually have a way to build the WebKit chromium port, so hopefully this actually compiles...  Also, I'll take suggestions on better names.  Later in the pipeline, we call these variables frame_origin and main_frame_origin....

[Sam Weinig]

Adam, is this really only helpful for Chromium?  Could other ports not benefit from having security context information here?

[Adam Barth]

Comment on attachment 28749 [details]
patchzor

> Adam, is this really only helpful for Chromium?  Could other ports not benefit
> from having security context information here?

You're right, we should move this to ResourceRequestBase.  You need this information to correctly compute the mixed content state for HTTPS.  I think we haven't noticed this before because Safari doesn't have a mixed content indicator.  Clearing review flag while I work up a new patch.

[David Levin]

When you add this field, please make sure to update  
  ResourceRequest::adopt
  ResourceRequest::copyData()
to handle it as well.

[Adam Barth]

Created attachment 30522 [details]
patch v2

Here is a second attempt.  ap and I discussed this patch on #webkit.  He thought it might be better to use the willSendRequest to do the mixed content check on the theory that the client should be making these policy decisions.  fishd thought this approach was better because checking the mixed content policy in willSendRequest adds a synchronous IPC message to the browser process in chromium, which affects performance.

There seems to be some amount of precedence for adding security context information to ResourceRequests.  For example, we already have the mainDocumentURL (which is actually very quirky).

[Alexey Proskuryakov]

The reason I suggested this was that clients are already making very similar policy decisions, e.g. in DocumentThreadableLoader::willSendRequest().

[Adam Barth]

(In reply to comment #6)
> The reason I suggested this was that clients are already making very similar
> policy decisions, e.g. in DocumentThreadableLoader::willSendRequest().

In this case, we need browser-wide information to make the right decision.  A user might have opted into allowing mixed content for a particular origin in another tab by clicking through a dialog or info bar, which is why we need to ask browser (and can't figure it out at renderer level).

[Maciej Stachowiak]

Comment on attachment 30522 [details]
patch v2

Unflagging. Adam and I discussed some alternate design approaches, and Adam will discuss with further people in turn, so it's too early to review until we decide if this is the right design approach.

[Adam Barth]

Created attachment 30635 [details]
Patch (via client interface)

Here's a patch based on discussions with mjs.  Is there any way to write a LayoutTest for this?

[Eric Seidel (no email)]

Comment on attachment 30635 [details]
Patch (via client interface)

Argument names are not needed in headers unless they add clarity.  None of the ones you add here do...

[Adam Barth]

> Argument names are not needed in headers unless they add clarity.  None of the
> ones you add here do...

Yeah, "const KURL& url" isn't very insightful, huh?  :)

[Oliver Hunt]

Comment on attachment 30635 [details]
Patch (via client interface)

I'm concerned that this patch doesn't add any testable functionality -- i think we need to have some mechanism to validate the behaviour in build bots before it gets landed

[Adam Barth]

> I'm concerned that this patch doesn't add any testable functionality

What's the best way to do that?  Maybe plumb the notifications to DumpRenderTree so it can print some sort of message?

[Eric Seidel (no email)]

Comment on attachment 30635 [details]
Patch (via client interface)

In order to test this, you'd need to add delegate callbacks to WebKit.  Like any other WebChromeClient.mm implementation.  These would have to be private delegate calls, probably on the policy delegate.  And then you'd have to have DRT's policy delegate implement them and log.

A lot of work... but mostly boiler plate, and seems like the easiest way I know to test.

Then you'd need to add some mixed-content tests in the http tests.

r- since Olliver said above that this was blocked by lack of tests.

[Adam Barth]

Created attachment 31607 [details]
Step 1: FrameLoaderClient methods (work-in-progress)

[Adam Barth]

Created attachment 31608 [details]
Step 2: WebFrameLoadDelegate methods (work-in-progress)

[Adam Barth]

Based on an IRC conversation with Maciej about how to plumb these notifications, I've uploaded two work-in-progress patches that contain only plumbing.  Are these on the right track?  Is it legit to add methods to WebFrameLoadDelegatePrivate and IWebFrameLoadDelegatePrivate?  How do those interact with legacy delegates that don't have those methods?

[Adam Barth]

Created attachment 38350 [details]
Step 4: Tests!


---
 11 files changed, 137 insertions(+), 0 deletions(-)

[Adam Barth]

Created attachment 38351 [details]
Step 3: Detection logic


---
 6 files changed, 63 insertions(+), 0 deletions(-)

[Adam Barth]

Created attachment 38352 [details]
Step 2: Delegate callbacks and DumpRenderTree changes.


---
 15 files changed, 251 insertions(+), 9 deletions(-)

[Adam Barth]

Created attachment 38353 [details]
Step 1: FrameLoaderClient methods


---
 18 files changed, 155 insertions(+), 1 deletions(-)

[Adam Barth]

Let's see...  I had some snarky comment here before about testing on Mac but not on the other platforms.  I expect carnage when landing this change, but that's what they pay me the big bucks for.

[Adam Barth]

I should also say that there are a bunch of mixed content corner cases that these patches don't cover.  The goal here is to get the APIs in place and have something minimally functional.  We can handle the strange cases (e.g., redirects) in follow up bugs to ensure that we get good test coverage.

[Eric Seidel (no email)]

Comment on attachment 38350 [details]
Step 4: Tests!

Seems a better testing infrastructure would be if we could query if a certain callback ever was called instead of dumping every one. :(

Would be helpful to list what the name of the callback is supposed to be:
+This test loads a secure iframe that loads an insecure iframe. We should *not* trigger a delegate callback because the frame's origin is *not* contaminated.

so that people looking at your test have some prayer of knowing what to look for. :)

I'm surprised this is not default:
 9     layoutTestController.setCloseRemainingWindowsWhenComplete(true);

I'm surprised boring.html is not just replaced by "about:blank" or a data url?

[Eric Seidel (no email)]

Comment on attachment 38351 [details]
Step 3: Detection logic

Hum... I think I see now why you didn't use data: urls or about:blank in your previous test.

         static bool isMixedContent(SecurityOrigin* context, const KURL& url);
 312         void checkIfDisplayInsecureContent(SecurityOrigin* context, const KURL& url);
 313         void checkIfRunInsecureContent(SecurityOrigin* context, const KURL& url);
no arg names needed.

In general this looks fine to me though.

[Eric Seidel (no email)]

Comment on attachment 38352 [details]
Step 2: Delegate callbacks and DumpRenderTree changes.

Most roundabout method to print a string, ever:
         NSString *string = [NSString stringWithFormat:@"didDisplayInsecureContent"];
 361         printf ("%s\n", [string UTF8String]);

Not only could you have used @"string" to make a literal, or just printf("mystring\n").  Instead you went for full style points. ;)

In general this looks fine though.  So fixing your string printing nit is all i see.  (That said, that one nit makes me question the rest of the mac code. :)

[Eric Seidel (no email)]

Comment on attachment 38353 [details]
Step 1: FrameLoaderClient methods

LGTM.

[Adam Barth]

(In reply to comment #26)
> Most roundabout method to print a string, ever:
[...]
> In general this looks fine though.  So fixing your string printing nit is all i
> see.  (That said, that one nit makes me question the rest of the mac code. :)

I have zero understanding of how Objective-C++ works.  I just adapted some printing code from elsewhere in this file.  :)

[Adam Barth]

Committed r48032: <http://trac.webkit.org/changeset/48032>

[Eric Seidel (no email)]

Objective-C++ is just a c++ file you can write obj-c in, or alternatively an obj-c file in which you can write c++ in.  Obj-C is relatively simple once you get over the message passing syntax.  Obj-C is a strict super-set of C.

[object messsageNamePartOne:argument moreMessageName:argument2]

this just turns into:
objc_msgsend(object, "messageNamePartOne:moreMessageName:", argument, argument2);

That's all. :)

[Adam Barth]

Comment on attachment 38353 [details]
Step 1: FrameLoaderClient methods

Landed step 1 in
http://trac.webkit.org/changeset/48032 and
http://trac.webkit.org/changeset/48033

[Eric Seidel (no email)]

I'm not sure if this is still supposed to have patches marked for review or not?

[Adam Barth]

Oh, i think bugzilla-tool closed this by mistake.

[Adam Barth]

Comment on attachment 38352 [details]
Step 2: Delegate callbacks and DumpRenderTree changes.

Landed in http://trac.webkit.org/changeset/48051 with some subsequent fixes for the windows build.

[Adam Barth]

Comment on attachment 38351 [details]
Step 3: Detection logic

This bug is too long.  I'm going to move the remaining work to another bug.