Bug 24696 - Enable mixed content detection
: Enable mixed content detection
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-19 01:22 PDT by Adam Barth
Modified: 2009-09-04 10:26 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2009-03-19 01:22:09 PDT
In order to properly compute our mixed content state, we need to attach some additional security information to resource requests.  Patch forthcoming.
Comment 1 Adam Barth 2009-03-19 01:25:46 PDT
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....
Comment 2 Sam Weinig 2009-03-24 10:54:22 PDT
Adam, is this really only helpful for Chromium?  Could other ports not benefit from having security context information here?
Comment 3 Adam Barth 2009-03-24 11:44:28 PDT
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.
Comment 4 David Levin 2009-03-24 11:57:34 PDT
When you add this field, please make sure to update  
  ResourceRequest::adopt
  ResourceRequest::copyData()
to handle it as well.

Comment 5 Adam Barth 2009-05-20 23:06:28 PDT
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).
Comment 6 Alexey Proskuryakov 2009-05-21 03:39:26 PDT
The reason I suggested this was that clients are already making very similar policy decisions, e.g. in DocumentThreadableLoader::willSendRequest().
Comment 7 Adam Barth 2009-05-21 07:52:41 PDT
(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 Maciej Stachowiak 2009-05-22 02:09:16 PDT
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.
Comment 9 Adam Barth 2009-05-24 15:03:44 PDT
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?
Comment 10 Eric Seidel 2009-06-10 00:45:26 PDT
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...
Comment 11 Adam Barth 2009-06-10 00:48:23 PDT
> 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 Oliver Hunt 2009-06-18 01:41:34 PDT
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
Comment 13 Adam Barth 2009-06-18 09:10:53 PDT
> 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 Eric Seidel 2009-06-18 17:14:35 PDT
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.
Comment 15 Adam Barth 2009-06-21 02:08:49 PDT
Created attachment 31607 [details]
Step 1: FrameLoaderClient methods (work-in-progress)
Comment 16 Adam Barth 2009-06-21 02:09:20 PDT
Created attachment 31608 [details]
Step 2: WebFrameLoadDelegate methods (work-in-progress)
Comment 17 Adam Barth 2009-06-21 02:12:35 PDT
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 Adam Barth 2009-08-20 22:30:30 PDT
Created attachment 38350 [details]
Step 4: Tests!


---
 11 files changed, 137 insertions(+), 0 deletions(-)
Comment 19 Adam Barth 2009-08-20 22:30:34 PDT
Created attachment 38351 [details]
Step 3: Detection logic


---
 6 files changed, 63 insertions(+), 0 deletions(-)
Comment 20 Adam Barth 2009-08-20 22:30:38 PDT
Created attachment 38352 [details]
Step 2: Delegate callbacks and DumpRenderTree changes.


---
 15 files changed, 251 insertions(+), 9 deletions(-)
Comment 21 Adam Barth 2009-08-20 22:30:42 PDT
Created attachment 38353 [details]
Step 1: FrameLoaderClient methods


---
 18 files changed, 155 insertions(+), 1 deletions(-)
Comment 22 Adam Barth 2009-08-20 22:31:49 PDT
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 Adam Barth 2009-08-22 08:39:21 PDT
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 Eric Seidel 2009-09-03 00:40:47 PDT
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?
Comment 25 Eric Seidel 2009-09-03 00:43:00 PDT
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.
Comment 26 Eric Seidel 2009-09-03 00:46:34 PDT
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. :)
Comment 27 Eric Seidel 2009-09-03 00:47:31 PDT
Comment on attachment 38353 [details]
Step 1: FrameLoaderClient methods

LGTM.
Comment 28 Adam Barth 2009-09-03 06:14:22 PDT
(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 Adam Barth 2009-09-03 16:00:03 PDT
Committed r48032: <http://trac.webkit.org/changeset/48032>
Comment 30 Eric Seidel 2009-09-03 16:02:00 PDT
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 Adam Barth 2009-09-03 16:13:01 PDT
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
Comment 32 Eric Seidel 2009-09-04 01:18:22 PDT
I'm not sure if this is still supposed to have patches marked for review or not?
Comment 33 Adam Barth 2009-09-04 08:31:39 PDT
Oh, i think bugzilla-tool closed this by mistake.
Comment 34 Adam Barth 2009-09-04 09:35:02 PDT
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.
Comment 35 Adam Barth 2009-09-04 10:26:13 PDT
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.