In order to properly compute our mixed content state, we need to attach some additional security information to resource requests. Patch forthcoming.
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....
Adam, is this really only helpful for Chromium? Could other ports not benefit from having security context information here?
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.
When you add this field, please make sure to update ResourceRequest::adopt ResourceRequest::copyData() to handle it as well.
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).
The reason I suggested this was that clients are already making very similar policy decisions, e.g. in DocumentThreadableLoader::willSendRequest().
(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 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.
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 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...
> 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 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
> 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 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.
Created attachment 31607 [details] Step 1: FrameLoaderClient methods (work-in-progress)
Created attachment 31608 [details] Step 2: WebFrameLoadDelegate methods (work-in-progress)
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?
Created attachment 38350 [details] Step 4: Tests! --- 11 files changed, 137 insertions(+), 0 deletions(-)
Created attachment 38351 [details] Step 3: Detection logic --- 6 files changed, 63 insertions(+), 0 deletions(-)
Created attachment 38352 [details] Step 2: Delegate callbacks and DumpRenderTree changes. --- 15 files changed, 251 insertions(+), 9 deletions(-)
Created attachment 38353 [details] Step 1: FrameLoaderClient methods --- 18 files changed, 155 insertions(+), 1 deletions(-)
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.
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 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 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 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 on attachment 38353 [details] Step 1: FrameLoaderClient methods LGTM.
(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. :)
Committed r48032: <http://trac.webkit.org/changeset/48032>
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 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
I'm not sure if this is still supposed to have patches marked for review or not?
Oh, i think bugzilla-tool closed this by mistake.
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 on attachment 38351 [details] Step 3: Detection logic This bug is too long. I'm going to move the remaining work to another bug.