758696 - Add a dialog to the debugger to deny or allow incoming server connections

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Rob Campbell [:rc] (:robcee)]

As a security measure, we need to allow the user of a browser with the devtools.debugger.remote-enabled to accept or reject an incoming remote connection.

A third option to disable the remote-enabled pref would be nice to have as well.

Comment 1

[Jonathan Griffin (:jgriffin)]

Will this affect Marionette?  If so, we'd need some way for test automation to allow the connection automatically.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

The summary mentions a dialog, which on mobile usually means nsIPromptService. This would work, but if we need something more custom, I'd suggest using a notification and then adding some code to Fennec to display a doorhanger.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v1

This appears to work in my tests so far with desktop Firefox and desktop B2G and has the extra button to disable remote debugging. I'm using nsIPromptService at mfinkle's suggestion to get something working for all products. We can later file a followup for making a prettier prompt for each product. The easier way to check out the dialog, is probably to select "Browser Debugger", since it also establishes a remote debugging connection automatically.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Marionette patch

This will make Marionette work like nothing has changed.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Fennec patch

This should presumably work for Fennec. I haven't actually tested it, but I'll try to get my first Fennec build tomorrow.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: B2G patch

This appears to work on desktop B2G, but I haven't tested it on my Nexus S, yet. I'm not requesting a formal review at this point, because I haven't figured out how to properly localize the messages. Vivien, any pointers?

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Panos Astithas [:past] from comment #6)
> Created attachment 627736 [details] [diff] [review]
> B2G patch
> 
> This appears to work on desktop B2G, but I haven't tested it on my Nexus S,
> yet. I'm not requesting a formal review at this point, because I haven't
> figured out how to properly localize the messages. Vivien, any pointers?

There is no XUL UI in B2G/Gaia so instead of using the internal prompter you want to exchange some message with the main application in order to retrieve a window. All the UI look and feel and l10n localization will be done in Gaia. Does that help?

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) from comment #7)
> (In reply to Panos Astithas [:past] from comment #6)
> > Created attachment 627736 [details] [diff] [review]
> > B2G patch
> > 
> > This appears to work on desktop B2G, but I haven't tested it on my Nexus S,
> > yet. I'm not requesting a formal review at this point, because I haven't
> > figured out how to properly localize the messages. Vivien, any pointers?
> 
> There is no XUL UI in B2G/Gaia so instead of using the internal prompter you
> want to exchange some message with the main application in order to retrieve
> a window. All the UI look and feel and l10n localization will be done in
> Gaia. Does that help?

And in order to exchange some informations between the chrome and the content you may want to use mozChromeEvent/mozContentEvent. This is a temporary 'API' until there is something more robust/well designed/etc...

Comment 9

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 627730 [details] [diff] [review]
Patch v1

Do we want to set any of these buttons to be the default other than the OK button? Are we happy with the OK button being default here considering some people are just going to to slap their enter key to dismiss a dialog.

   openListener: function DH_openListener(aPort, aLocalOnly) {
+    if (!canListen) {
+      return false;
+    }

not used to seeing variables without a this. in front of them. OK here.

What are you thinking about how to test this?

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 627734 [details] [diff] [review]
Fennec patch

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+  _allowConnection: function rd_allowConnection() {

>+    let result = prompt.confirmEx(null, title, msg, flags, null, null, btn,
>+                                  null, { value: false });

nit: We keep mobile a bit more relaxed for line breaks like this. feel free to keep it all on one line

>+    if (result == 0) {
>+      return true;
>+    }

nit: No {} needed for one-lines

>diff --git a/mobile/android/locales/en-US/chrome/browser.properties b/mobile/android/locales/en-US/chrome/browser.properties

>+# Debugger
>+# LOCALIZATION NOTE (remoteIncomingPromptTitle): The title displayed on the
>+# dialog that prompts the user to allow the incoming connection.
>+remoteIncomingPromptTitle=Incoming Connection
>+# LOCALIZATION NOTE (remoteIncomingPromptMessage): The message displayed on the
>+# dialog that prompts the user to allow the incoming connection.
>+remoteIncomingPromptMessage=An incoming request to permit remote debugging connection was detected. With a remote debugging connection, the remote client can take complete control over your browser! Allow connection?

UX might want to shorten this a bit since it will wrap quickly on mobile. Maybe:

An incoming request to permit remote debugging connection was detected. A remote client can take complete control over your browser! Allow connection?

>+# LOCALIZATION NOTE (remoteIncomingPromptDisable): The label displayed on the
>+# third button in the incoming connection dialog that lets the user disable the
>+# remote debugger server.
>+remoteIncomingPromptDisable=Disable

nit: LOCALIZATION NOTEs are useful for some strings, but I don't know if we need them for simple stuff like this. up to you.

r+, I think the modal-ness of the prompt on mobile is warranted.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #9)
> Comment on attachment 627730 [details] [diff] [review]
> Patch v1
> 
> Do we want to set any of these buttons to be the default other than the OK
> button? Are we happy with the OK button being default here considering some
> people are just going to to slap their enter key to dismiss a dialog.

I opted for less harassment, but certainly we need the experts' opinion on this. mgoodwin, curtisk, what do you think? Would 'Cancel' be a better default than 'OK'?

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Fennec patch v2

Addressed nits. I like the shortened message, I'll use it in desktop as well, thanks!

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: B2G patch v2

Vivien, thanks for the detailed comments. I don't think I have the time to implement that right now, and since there is no imminent release for B2G that I know of, would it be OK with you to leave it as it is (i.e. accept all connections) for now, and file a followup to implement the prompt?

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v2

(In reply to Rob Campbell [:rc] (:robcee) from comment #9)
> What are you thinking about how to test this?

I spent some time today trying to write a test and I don't think I can do it. I mean, since we don't provide a UI for enabling the server to listen for remote connections yet, all I can test is if the API works. That is, if the callback is called when a connection is made, which seems not very useful. I propose we write a decent test when we implement a UI that starts the debugger server without the debugger frontend, for the remote debugging scenario.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 628761 [details] [diff] [review]
B2G patch v2

Sounds good. I will do a patch for the dialog myself if you don't have the time.

Comment 16

[Tanvi Vyas[:tanvi]]

(In reply to Panos Astithas [:past] from comment #11)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #9)
> > Comment on attachment 627730 [details] [diff] [review]
> > Patch v1
> > 
> > Do we want to set any of these buttons to be the default other than the OK
> > button? Are we happy with the OK button being default here considering some
> > people are just going to to slap their enter key to dismiss a dialog.
> 
> I opted for less harassment, but certainly we need the experts' opinion on
> this. mgoodwin, curtisk, what do you think? Would 'Cancel' be a better
> default than 'OK'?

We talked about this in the Devtools meeting today.  The default should be "Cancel".  Users who want to enable it can hit okay.  This will prevent users who don't know what it is from just clicking through it.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v3

Now with 'Cancel' as the default.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Fennec patch v3

Now with 'Cancel' as the default.

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Vivien Nicolas (:vingtetun) from comment #15)
> Sounds good. I will do a patch for the dialog myself if you don't have the
> time.

Thanks, I will take you up on that!

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=3cd6acf8be12

Comment 21

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 629064 [details] [diff] [review]
Patch v3

yes. cancel should be default.

This looks great! Land'er.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/ae9aa5be8ca2
https://hg.mozilla.org/integration/fx-team/rev/af3834c1d9cb
https://hg.mozilla.org/integration/fx-team/rev/3cbd3908f497
https://hg.mozilla.org/integration/fx-team/rev/3f0594ee130d

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/ae9aa5be8ca2
https://hg.mozilla.org/mozilla-central/rev/af3834c1d9cb
https://hg.mozilla.org/mozilla-central/rev/3cbd3908f497
https://hg.mozilla.org/mozilla-central/rev/3f0594ee130d

Comment 24

[Jonathan Griffin (:jgriffin)]

Somehow, this ended up breaking Marionette on B2G.  I'll investigate and file a separate bug.

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jonathan Griffin (:jgriffin) from comment #24)
> Somehow, this ended up breaking Marionette on B2G.  I'll investigate and
> file a separate bug.

Can you see if the patch in bug 761153 fixes it?