1305177 - Provide observer notification to allow extensions to cancel external app launch (Tor 19273)

Status: NEW

(Firefox :: File Handling, defect, P2)

Description

[Arthur Edelstein [:arthur]]

Tor Browser patched the nsExternalHelperAppService to broadcast an
"external-app-requested" observer service notification to allow
other modules, including extensions, a chance to cancel the launch. Tor Browser uses this notification to warn users that opening an external app could compromise anonymity, and gives users an opportunity to cancel opening the application.

Here is the current patch:
https://torpat.ch/19273

Comment 1

[William Hsu [:whsu]]

It seems to me that it's a feature request. Set priority to P3 and add to product backlog.

Comment 2

[Arthur Edelstein [:arthur]]

Yes, sorry(In reply to William Hsu [:whsu] from comment #1)
> It seems to me that it's a feature request. Set priority to P3 and add to
> product backlog.

Yes, sorry for not making this clear. We would like to propose uplifting the patch.

Comment 3

[William Hsu [:whsu]]

(In reply to Arthur Edelstein [:arthuredelstein] from comment #2)
> Yes, sorry(In reply to William Hsu [:whsu] from comment #1)
> > It seems to me that it's a feature request. Set priority to P3 and add to
> > product backlog.
> 
> Yes, sorry for not making this clear. We would like to propose uplifting the
> patch.

No worries. Thanks for the help!
Have a nice day.

Comment 4

[William Hsu [:whsu]]

Hi, Ben,

May I have your help? Could you please review the request?
Thank you.

Comment 5

[Ben Tian (inactive)]

Arthur,

Please submit your patch as [1] via MozReview or file attachment. Also I suggest you request review from :Gijs.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Submitting_the_patch

Comment 6

[Arthur Edelstein [:arthur]]

(In reply to Ben Tian [:btian] from comment #5)
> Arthur,
> 
> Please submit your patch as [1] via MozReview or file attachment. Also I
> suggest you request review from :Gijs.

Will do! Thanks, Ben.

Comment 7

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1305177-Allow-extensions-to-cancel-external-app-.patch

Here's a patch rebased from Tor Browser to the latest mozilla-central.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8798676 [details] [diff] [review]
0001-Bug-1305177-Allow-extensions-to-cancel-external-app-.patch

Review of attachment 8798676 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a good reviewer for this change. uriloader/ is basically docshell (https://wiki.mozilla.org/Modules/Core#docshell ), so you should ask one of :bz, :smaug, or :jst . (And we should probably bug them to update the list of peers, I don't think :biesi and Justin Lebar are active, and I know all 3 of bz, smaug and jst are very busy, so it'd be good if there were other people to ask...).

Some initial feedback:

- please generate patches with 8 or more lines of context. It's very hard to see what this patch does on bugzilla because there's basically no context to your changes.
- for a static helper method that isn't exposed through xpcom and really is returning a boolean... IMHO it should actually return a boolean rather than an nsresult. That will simplify the callsites and eliminate NS_ENSURE_ARG_POINTER and other XPCOM boilerplate in the helper itself.
- the blocking happens very early on in the function, meaning we don't have useful data to present to the user. Rather than telling the user "some app is being launched, which might compromise your anonymity", it would be useful if we could tell them *which* app, or at least the mimetype/URL we're handling (which we could pass in the data param of the observer notification). So I would suggest moving the blocking further down the method and passing on some kind of descriptor of what we'll launch/handle if the user says "yes" (even if the tor browser code currently doesn't take advantage of that, it would make sense if it did. For instance, maybe I know that using gedit to read a downloaded .log file is OK, and firing up a bittorrent client would be a bad idea - but with a context-less prompt I can't make an educated decision (and also you couldn't keep a list of 'allowed' apps for which you stop prompting to stop annoying the user))
- this could do with a test, especially as the code is unused otherwise. Ideally it should be exercising both code paths where you're currently inserting calls to your helper.
- this will need a webextensions proxying API so you can keep doing this once XUL/XPCOM add-ons are gone. Andy McKay would be the best person to talk to about that.

Comment 9

[Arthur Edelstein [:arthur]]

(In reply to :Gijs Kruitbosch from comment #8)
> Comment on attachment 8798676 [details] [diff] [review]
> 0001-Bug-1305177-Allow-extensions-to-cancel-external-app-.patch
> 
> Review of attachment 8798676 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not a good reviewer for this change. uriloader/ is basically docshell
> (https://wiki.mozilla.org/Modules/Core#docshell ), so you should ask one of
> :bz, :smaug, or :jst . (And we should probably bug them to update the list
> of peers, I don't think :biesi and Justin Lebar are active, and I know all 3
> of bz, smaug and jst are very busy, so it'd be good if there were other
> people to ask...).
> 
> Some initial feedback:
> 
> - please generate patches with 8 or more lines of context. It's very hard to
> see what this patch does on bugzilla because there's basically no context to
> your changes.
> - for a static helper method that isn't exposed through xpcom and really is
> returning a boolean... IMHO it should actually return a boolean rather than
> an nsresult. That will simplify the callsites and eliminate
> NS_ENSURE_ARG_POINTER and other XPCOM boilerplate in the helper itself.
> - the blocking happens very early on in the function, meaning we don't have
> useful data to present to the user. Rather than telling the user "some app
> is being launched, which might compromise your anonymity", it would be
> useful if we could tell them *which* app, or at least the mimetype/URL we're
> handling (which we could pass in the data param of the observer
> notification). So I would suggest moving the blocking further down the
> method and passing on some kind of descriptor of what we'll launch/handle if
> the user says "yes" (even if the tor browser code currently doesn't take
> advantage of that, it would make sense if it did. For instance, maybe I know
> that using gedit to read a downloaded .log file is OK, and firing up a
> bittorrent client would be a bad idea - but with a context-less prompt I
> can't make an educated decision (and also you couldn't keep a list of
> 'allowed' apps for which you stop prompting to stop annoying the user))
> - this could do with a test, especially as the code is unused otherwise.
> Ideally it should be exercising both code paths where you're currently
> inserting calls to your helper.
> - this will need a webextensions proxying API so you can keep doing this
> once XUL/XPCOM add-ons are gone. Andy McKay would be the best person to talk
> to about that.

Thank you for the feedback, Gijs! Much appreciated.