Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
768753003:
Downgrade the security of a secured page when plugins make insecure calls (Closed)
DescriptionDowngrade the security of a secured page when plugins make insecure calls
Send message for lowering the security of a page when context of the
request is ContextTypeShouldBeBlockable.
BUG=432485
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187260
Patch Set 1 #Patch Set 2 : Layout test for prefetching from insecure source added #
Messages
Total messages: 19 (3 generated)
atanasb@opera.com changed reviewers: + japhet@chromium.org, tyoshino@chromium.org
On 2014/12/04 15:31:45, atanasb wrote: > mailto:atanasb@opera.com changed reviewers: > + mailto:japhet@chromium.org, mailto:tyoshino@chromium.org Could you please cc: me to bug 432485, so I can see the context of this change?
atanasb@opera.com changed reviewers: + mkwst@chromium.org
On 2014/12/04 19:00:46, Nate Chapin wrote: > On 2014/12/04 15:31:45, atanasb wrote: > > mailto:atanasb@opera.com changed reviewers: > > + mailto:japhet@chromium.org, mailto:tyoshino@chromium.org > > Could you please cc: me to bug 432485, so I can see the context of this change? Probably Mike will be able to do this ?
On 2014/12/04 at 22:12:26, atanasb wrote: > On 2014/12/04 19:00:46, Nate Chapin wrote: > > On 2014/12/04 15:31:45, atanasb wrote: > > > mailto:atanasb@opera.com changed reviewers: > > > + mailto:japhet@chromium.org, mailto:tyoshino@chromium.org > > > > Could you please cc: me to bug 432485, so I can see the context of this change? > > Probably Mike will be able to do this ? CC'd you, Nate. I don't think we should land this patch as-is. We want to start blocking insecure plugin requests entirely (see https://code.google.com/p/chromium/issues/detail?id=388651), but that's quite likely going to effect a significant number of sites using Flash for a variety of weirdnesses. We can't degrade the UI for those sites without giving developers a console warning, and we shouldn't add such a warning without adding a UseCounter for the various effected types. Would you mind reworking this patch to: a) Move RequestContextInternal to a new "not blockable" type. We shouldn't have any HTTP requests from internal sources, but if we do, they come from Chrome, not the page, and shouldn't be blocked. b) Add UseCounter entries for mixed resources in internal, plugin, and prefetch contexts. c) Add a console log entry when these types appear. a) Count the
On 2014/12/04 19:00:46, Nate Chapin wrote: > On 2014/12/04 15:31:45, atanasb wrote: > > mailto:atanasb@opera.com changed reviewers: > > + mailto:japhet@chromium.org, mailto:tyoshino@chromium.org > > Could you please cc: me to bug 432485, so I can see the context of this change? me too please, Mike.
On 2014/12/06 05:52:28, Mike West (OOO until 8th) wrote: ... > c) Add a console log entry when these types appear. > a) Count the hit send button by mistake?
CC'd you. On 2014/12/08 at 09:44:37, tyoshino wrote: > On 2014/12/06 05:52:28, Mike West (OOO until 8th) wrote: > ... > > c) Add a console log entry when these types appear. > > a) Count the > > hit send button by mistake? Yeah. That last `a)` shouldn't be there.
Looking at this one more time, it's simpler than I made it out to be: I'd
suggest changing your code to something like:
case ContentTypeShouldBeBlockable:
allowed = true;
client->didDisplayInsecureContent();
break;
That should do the console logging for you at the bottom of the function. I'm
adding usecounters for mixed content types in a separate CL, so I don't think
you need to worry about that bit here. I would appreciate you adding a layout
test to verify the console message is written, however.
Thanks!
On 2014/12/09 08:44:21, Mike West wrote: > Looking at this one more time, it's simpler than I made it out to be: I'd > suggest changing your code to something like: > > case ContentTypeShouldBeBlockable: > allowed = true; > client->didDisplayInsecureContent(); > break; > > That should do the console logging for you at the bottom of the function. I'm > adding usecounters for mixed content types in a separate CL, so I don't think > you need to worry about that bit here. I would appreciate you adding a layout > test to verify the console message is written, however. > > Thanks! Great, I'll prepare the test and will update the CL.
> Great, > I'll prepare the test and will update the CL. https://codereview.chromium.org/785133004/ is the UseCounter CL I mentioned. It will likely land in the near future; I hope you won't have to rebase too much, so, just FYI. :)
On 2014/12/09 18:19:16, Mike West (sick) wrote: > > Great, > > I'll prepare the test and will update the CL. > > https://codereview.chromium.org/785133004/ is the UseCounter CL I mentioned. It > will likely land in the near future; I hope you won't have to rebase too much, > so, just FYI. :) I've been trying with the layout test, but unfortunately it seems that PPAPI support in content_shell is missing and since this patch is only applicable to PPAPI, I can't force the Console Error in content_shell. I have no clues how to continue with the layout test.
On 2014/12/10 16:21:46, atanasb wrote: > On 2014/12/09 18:19:16, Mike West (sick) wrote: > > > Great, > > > I'll prepare the test and will update the CL. > > > > https://codereview.chromium.org/785133004/ is the UseCounter CL I mentioned. > It > > will likely land in the near future; I hope you won't have to rebase too much, > > so, just FYI. :) > > I've been trying with the layout test, but unfortunately it seems that PPAPI > support in content_shell is missing and since this patch is only applicable to > PPAPI, I can't force the Console Error in content_shell. I have no clues how to > continue with the layout test. Since adding a layout test for requests performed by a PPAPI plugin seems not possible at the moment, I've attached a layout test for links with prefetch. The code path for them in the context of MixedContentChecker.cpp/ContextTypeShouldBeBlockable is the same as for the PPAPI plugins.
On 2014/12/16 13:04:56, atanasb wrote: > On 2014/12/10 16:21:46, atanasb wrote: > > On 2014/12/09 18:19:16, Mike West (sick) wrote: > > > > Great, > > > > I'll prepare the test and will update the CL. > > > > > > https://codereview.chromium.org/785133004/ is the UseCounter CL I mentioned. > > It > > > will likely land in the near future; I hope you won't have to rebase too > much, > > > so, just FYI. :) > > > > I've been trying with the layout test, but unfortunately it seems that PPAPI > > support in content_shell is missing and since this patch is only applicable to > > PPAPI, I can't force the Console Error in content_shell. I have no clues how > to > > continue with the layout test. > > Since adding a layout test for requests performed by a PPAPI plugin seems not > possible at the moment, > I've attached a layout test for links with prefetch. The code path for them in > the context of > MixedContentChecker.cpp/ContextTypeShouldBeBlockable is the same as for the > PPAPI plugins. Ah, sorry. I didn't see your comment regarding PPAPI; I was out sick most of last week, and have done a bad job catching up. Apologies. :( Grabbing one of the other "should be blockable" types would have been my suggestion, so I'm glad you went that route. This LGTM, though I'm a bit surprised it doesn't effect any other test... I'd suggest running a chromium-side bot as well, just to make sure the browsertests are happy. Since you're not blocking anything in this patch, I don't really expect breakage, but better safe than sorry.
On 2014/12/16 13:10:49, Mike West wrote: > On 2014/12/16 13:04:56, atanasb wrote: > > On 2014/12/10 16:21:46, atanasb wrote: > > > On 2014/12/09 18:19:16, Mike West (sick) wrote: > > > > > Great, > > > > > I'll prepare the test and will update the CL. > > > > > > > > https://codereview.chromium.org/785133004/ is the UseCounter CL I > mentioned. > > > It > > > > will likely land in the near future; I hope you won't have to rebase too > > much, > > > > so, just FYI. :) > > > > > > I've been trying with the layout test, but unfortunately it seems that PPAPI > > > support in content_shell is missing and since this patch is only applicable > to > > > PPAPI, I can't force the Console Error in content_shell. I have no clues how > > to > > > continue with the layout test. > > > > Since adding a layout test for requests performed by a PPAPI plugin seems not > > possible at the moment, > > I've attached a layout test for links with prefetch. The code path for them in > > the context of > > MixedContentChecker.cpp/ContextTypeShouldBeBlockable is the same as for the > > PPAPI plugins. > > Ah, sorry. I didn't see your comment regarding PPAPI; I was out sick most of > last week, and have done a bad job catching up. Apologies. :( > > Grabbing one of the other "should be blockable" types would have been my > suggestion, so I'm glad you went that route. This LGTM, though I'm a bit > surprised it doesn't effect any other test... I'd suggest running a > chromium-side bot as well, just to make sure the browsertests are happy. Since > you're not blocking anything in this patch, I don't really expect breakage, but > better safe than sorry. NP :) But for some reason, I can't invite the win chromium bots here, they just go purple without more info.
On 2014/12/16 14:34:05, atanasb wrote: > NP :) > But for some reason, I can't invite the win chromium bots here, they just go > purple without more info. Windows is nuts. The Linux and ChromeOS bots went through fine; that's good enough for me. :)
The CQ bit was checked by atanasb@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/768753003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187260 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
