Bug 24696 - Enable mixed content detection
: Enable mixed content detection
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-03-19 01:22 PST by
Modified: 2009-09-04 10:26 PST (History)


Attachments
patchzor (2.55 KB, patch)
2009-03-19 01:25 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
patch v2 (6.20 KB, patch)
2009-05-20 23:06 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch (via client interface) (19.65 KB, patch)
2009-05-24 15:03 PST, Adam Barth
eric: review-
Review Patch | Details | Formatted Diff | Diff
Step 1: FrameLoaderClient methods (work-in-progress) (14.77 KB, patch)
2009-06-21 02:08 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Step 2: WebFrameLoadDelegate methods (work-in-progress) (13.83 KB, patch)
2009-06-21 02:09 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Step 4: Tests! (9.52 KB, patch)
2009-08-20 22:30 PST, Adam Barth
eric: review-
Review Patch | Details | Formatted Diff | Diff
Step 3: Detection logic (5.27 KB, patch)
2009-08-20 22:30 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Step 2: Delegate callbacks and DumpRenderTree changes. (18.94 KB, patch)
2009-08-20 22:30 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Step 1: FrameLoaderClient methods (13.53 KB, patch)
2009-08-20 22:30 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-03-19 01:22:09 PST
In order to properly compute our mixed content state, we need to attach some additional security information to resource requests.  Patch forthcoming.
------- Comment #1 From 2009-03-19 01:25:46 PST -------
Created an attachment (id=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....
------- Comment #2 From 2009-03-24 10:54:22 PST -------
Adam, is this really only helpful for Chromium?  Could other ports not benefit from having security context information here?
------- Comment #3 From 2009-03-24 11:44:28 PST -------
(From update of attachment 28749 [details])
> 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.
------- Comment #4 From 2009-03-24 11:57:34 PST -------
When you add this field, please make sure to update  
  ResourceRequest::adopt
  ResourceRequest::copyData()
to handle it as well.
------- Comment #5 From 2009-05-20 23:06:28 PST -------
Created an attachment (id=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).
------- Comment #6 From 2009-05-21 03:39:26 PST -------
The reason I suggested this was that clients are already making very similar policy decisions, e.g. in DocumentThreadableLoader::willSendRequest().
------- Comment #7 From 2009-05-21 07:52:41 PST -------
(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).
------- Comment #8 From 2009-05-22 02:09:16 PST -------
(From update of attachment 30522 [details])
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.
------- Comment #9 From 2009-05-24 15:03:44 PST -------
Created an attachment (id=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?
------- Comment #10 From 2009-06-10 00:45:26 PST -------
(From update of attachment 30635 [details])
Argument names are not needed in headers unless they add clarity.  None of the ones you add here do...
------- Comment #11 From 2009-06-10 00:48:23 PST -------
> 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?  :)
------- Comment #12 From 2009-06-18 01:41:34 PST -------
(From update of attachment 30635 [details])
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
------- Comment #13 From 2009-06-18 09:10:53 PST -------
> 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?
------- Comment #14 From 2009-06-18 17:14:35 PST -------
(From update of attachment 30635 [details])
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.
------- Comment #15 From 2009-06-21 02:08:49 PST -------
Created an attachment (id=31607) [details]
Step 1: FrameLoaderClient methods (work-in-progress)
------- Comment #16 From 2009-06-21 02:09:20 PST -------
Created an attachment (id=31608) [details]
Step 2: WebFrameLoadDelegate methods (work-in-progress)
------- Comment #17 From 2009-06-21 02:12:35 PST -------
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?
------- Comment #18 From 2009-08-20 22:30:30 PST -------
Created an attachment (id=38350) [details]
Step 4: Tests!


---
 11 files changed, 137 insertions(+), 0 deletions(-)
------- Comment #19 From 2009-08-20 22:30:34 PST -------
Created an attachment (id=38351) [details]
Step 3: Detection logic


---
 6 files changed, 63 insertions(+), 0 deletions(-)
------- Comment #20 From 2009-08-20 22:30:38 PST -------
Created an attachment (id=38352) [details]
Step 2: Delegate callbacks and DumpRenderTree changes.


---
 15 files changed, 251 insertions(+), 9 deletions(-)
------- Comment #21 From 2009-08-20 22:30:42 PST -------
Created an attachment (id=38353) [details]
Step 1: FrameLoaderClient methods


---
 18 files changed, 155 insertions(+), 1 deletions(-)
------- Comment #22 From 2009-08-20 22:31:49 PST -------
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.
------- Comment #23 From 2009-08-22 08:39:21 PST -------
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.
------- Comment #24 From 2009-09-03 00:40:47 PST -------
(From update of attachment 38350 [details])
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?
------- Comment #25 From 2009-09-03 00:43:00 PST -------
(From update of attachment 38351 [details])
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.
------- Comment #26 From 2009-09-03 00:46:34 PST -------
(From update of attachment 38352 [details])
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. :)
------- Comment #27 From 2009-09-03 00:47:31 PST -------
(From update of attachment 38353 [details])
LGTM.
------- Comment #28 From 2009-09-03 06:14:22 PST -------
(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.  :)
------- Comment #29 From 2009-09-03 16:00:03 PST -------
Committed r48032: <http://trac.webkit.org/changeset/48032>
------- Comment #30 From 2009-09-03 16:02:00 PST -------
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. :)
------- Comment #31 From 2009-09-03 16:13:01 PST -------
(From update of attachment 38353 [details])
Landed step 1 in
http://trac.webkit.org/changeset/48032 and
http://trac.webkit.org/changeset/48033
------- Comment #32 From 2009-09-04 01:18:22 PST -------
I'm not sure if this is still supposed to have patches marked for review or not?
------- Comment #33 From 2009-09-04 08:31:39 PST -------
Oh, i think bugzilla-tool closed this by mistake.
------- Comment #34 From 2009-09-04 09:35:02 PST -------
(From update of attachment 38352 [details])
Landed in http://trac.webkit.org/changeset/48051 with some subsequent fixes for the windows build.
------- Comment #35 From 2009-09-04 10:26:13 PST -------
(From update of attachment 38351 [details])
This bug is too long.  I'm going to move the remaining work to another bug.