Disable dev mode bubble and some infobars during remote debugging.

Disable dev mode bubble and some infobars during remote debugging.

BUG=chromedriver:1625

[samuong, 2017-02-09 22:28:07]

samuong@chromium.org changed reviewers:
+ pfeldman@chromium.org, rdevlin.cronin@chromium.org

[samuong, 2017-02-09 22:28:08]

Pavel, here's an alternative CL that implements the method that you suggested in
https://codereview.chromium.org/2564973002

I had some pushback when I first suggested disabling these the dev mode bubble
and warning infobars, which is why we went with the chromedriver infobar. But if
everyone is happy with this from a security perspective, then I'm OK with
landing this instead.

PTAL.

[pfeldman, 2017-02-09 22:53:46]

lgtm

[pfeldman, 2017-02-09 22:55:20]

pfeldman@chromium.org changed reviewers:
+ sky@chromium.org

[pfeldman, 2017-02-09 22:55:21]

+sky@ for OWNERS: this disables some of the UI bubbles and infobars while remote
debugging.

[Devlin, 2017-02-09 23:42:03]

Hmm... I'm not sure I follow.  How does this help prevent the abuse case?

[samuong, 2017-02-10 17:31:16]

The argument is that --remote-debugging-port leaves a browser wide open anyway.
If a user has this switch on, they're already compromised, probably even worse
than if they had a malicious dev mode extension installed.

[pfeldman, 2017-02-10 19:28:12]

Description was changed from

==========
Disable dev mode bubble and some infobars during remote debugging.

BUG=chromedriver:1625
==========

to

==========
Disable dev mode bubble and some infobars during remote debugging.

BUG=chromedriver:1625
==========

[pfeldman, 2017-02-10 19:28:12]

pfeldman@chromium.org changed reviewers:
+ tsepez@chromium.org

[pfeldman, 2017-02-10 19:29:54]

+ tsepez@ for the security aspect.

context: we block various bubbles when chrome is running with the
--remote-debugging-port=9222. That flag already allows connecting to chrome,
fetching all the cookies, etc. We just hide the bubbles that pop up and break
automation.

[Devlin, 2017-02-10 19:37:56]

On 2017/02/10 19:29:54, pfeldman wrote:
> + tsepez@ for the security aspect.
> 
> context: we block various bubbles when chrome is running with the
> --remote-debugging-port=9222. That flag already allows connecting to chrome,
> fetching all the cookies, etc. We just hide the bubbles that pop up and break
> automation.

Wait - so if I want to disable all chrome's security bubbles and infobars, I
just have to add the commandline switch --remote-debugging-port?

[pfeldman, 2017-02-10 19:50:06]

>> Wait - so if I want to disable all chrome's security bubbles and infobars, I
just have to add the commandline switch --remote-debugging-port?

- We aren't disabling all chrome's security bubbles and infobars
- Passing --remote-debugging-port compromises the browser in a way that infobars
and bubbles are irrelevant.

[Tom Sepez, 2017-02-10 21:51:14]

tsepez@chromium.org changed reviewers:
+ estark@chromium.org, lgarron@chromium.org
- tsepez@chromium.org

[Tom Sepez, 2017-02-10 21:51:36]

tsepez@chromium.org changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2017-02-10 21:51:36]

Adding some of the security UI folks instead.  Thanks.

[lgarron, 2017-02-10 21:54:45]

On 2017/02/10 at 21:51:36, tsepez wrote:
> Adding some of the security UI folks instead.  Thanks.

Is there a Chromium bug that documents why this change is warranted?
The code implies that this is done just to accommodate tests.

We have infobars for a bunch of insecure testing flags, and "this flag is even
less secure than usual" doesn't sound like a great reason.

[pfeldman, 2017-02-10 22:05:33]

> We have infobars for a bunch of insecure testing flags, and "this flag is even
> less secure than usual" doesn't sound like a great reason.

The greatest difference is that --remote-debugging-port implies remote /
automated / headless operation. Showing additional UI for the automated browser
does not make too much sense, because automated browser can be simply hidden by
the automation.

I believe there is a confusion: this change is not about questioning the
remote-debugging-port flag implications, it is rather a cosmetics change that
hides popups that are stealing focus or changing the page layout. These bits are
essential to remote debugging and automation. Figuring out whether
remote-debugging-port itself should be reflected in the UI and alike should be
considered separately if the need be. That's why I thought it was more about
core security rather than security UI. It seems to be rather internal design
topic that Tom was typically reviewing for us.

[pfeldman, 2017-02-10 22:19:41]

pfeldman@chromium.org changed reviewers:
+ pkasting@chromium.org

[pfeldman, 2017-02-10 22:47:50]

Talked to Peter. We seem to all realize that there are two aspects of the
change:

1) enable / fix the automation via preventing bubbles and infobars from stealing
focus
2) make sure the fact that automation is in progress is obvious to the user

Present CL is about (1), for (2) I just filed
https://bugs.chromium.org/p/chromium/issues/detail?id=691117.

[Peter Kasting, 2017-02-11 03:57:09]

LGTM

https://codereview.chromium.org/2685203002/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/2685203002/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:815:
!command_line_.HasSwitch(switches::kRemoteDebuggingPort)) {
Maybe this should comment about why it's not on for remote debugging?

[samuong, 2017-02-13 18:12:02]

Patchset #2 (id:20001) has been deleted

[samuong, 2017-02-13 18:14:17]

https://codereview.chromium.org/2685203002/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/2685203002/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:815:
!command_line_.HasSwitch(switches::kRemoteDebuggingPort)) {
On 2017/02/11 03:57:09, Peter Kasting wrote:
> Maybe this should comment about why it's not on for remote debugging?

Done. It's the same reason as why it's not on for --test-type, so I've tried to
write the comment in a way that applies to both. If you want to tweak the
wording I can follow up.

[pfeldman, 2017-02-13 18:26:59]

Sam, my understanding is that you are fine to land this. I will follow up on
crbug.com/691117.

[samuong, 2017-02-13 18:32:46]

The CQ bit was checked by samuong@chromium.org

[samuong, 2017-02-13 18:32:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2685203002/#ps40001
(title: "add comment")

[commit-bot: I haz the power, 2017-02-13 18:33:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-02-13 18:38:27]

not lgtm.

Sorry, but I don't think we can land this until we have some kind of alternative
API to take its place, or at least run it by more folks.  Sam, will ping you
offline.

[samuong, 2017-02-13 18:39:22]

The CQ bit was unchecked by samuong@chromium.org

[pfeldman, 2017-02-13 18:40:28]

Hey, Devlin, could you please keep me in the loop and resolve this in a timely
manner?

[Devlin, 2017-02-13 18:44:37]

On 2017/02/13 18:40:28, pfeldman wrote:
> Hey, Devlin, could you please keep me in the loop and resolve this in a timely
> manner?

Yep, will send out an email thread with us + others soon.