285517 - show better message in helper app dialog when the type was sniffed from text/plain (determined to by binary content)

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

[I believed this to be a duplicate, but can't find a bug]

nsIExternalHelperAppLauncherDialog::show should differentiate between normal
unknown type, and type sniffed from text/plain, i.e. change aForced into a
PRUint32 w/ some constants

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176967 [details] [diff] [review]
patch

>Index: embedding/components/ui/helperAppDlg/nsHelperAppDlg.js
>          // Initialize "always ask me" box. This should always be disabled
>          // and set to true for the ambiguous type application/octet-stream.
>-         // Same if this dialog was forced
>+         // Same if this dialog was forced due to a server request

Actually, I think it should be for the text/plain case too... Unless we plan to
not show the dialog in that case, depending on user input, and just run the
handler automatically?	That has fun security implications, and I somewhat
suspect that's what we do now, and I think we have bugs on it.	I think we
should pose the dialog for cases when we sniffed.  If individual apps want the
security issues, they can always just call back immediately (they can check the
"alwaysAsk" on the mime info and the reason we opened the dialog to make the
decision).

>         // Only care about the state of "always ask" if this dialog wasn't forced
>-        if ( !this.mForced )
>+        if ( this.mReason != REASON_SERVERREQUEST )

Same issue here.

>Index: toolkit/locales/en-US/chrome/global/nsHelperAppDlg.dtd
>+<!ENTITY intro.sniffed.label "#4 has detected that the file &quot;#3&quot; is of type #2 (#1). The file is located at:">
>+<!ENTITY intro.sniffed.noDesc.label "#4 has detected that the file &quot;#3&quot; is of type #2. The file is located at:">
>+

How about "might be" instead of "is"?

Also, this part probably needs review from a toolkit peer....

>Index: toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in

Hmmm... This doesn't depend on the reason the dialog was opened?  Worth filing
a bug on?  And ccing mconnor, darin, me?  Darin seemed pretty concerned that
this sort of thing work at least similarly in SeaMonkey and Firefox...

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp
>+    reason = nsIHelperAppLauncherDialog::REASON_TEXTPLAINSNIFFED;

Do we really want to do that if reason is already REASON_SERVERREQUEST?  I
would say we don't.

> nsExternalAppHandler::nsExternalAppHandler()
>-  mHandlingAttachment = PR_FALSE;

Probably still want to init mReason, just in case.

>   // If we're handling an attachment we want to default to saving but
>   // always ask just in case
>-  if (!mHandlingAttachment)
>+  if (mReason != nsIHelperAppLauncherDialog::REASON_SERVERREQUEST)
>   {
>     mMimeInfo->GetAlwaysAskBeforeHandling(&alwaysAsk);

And here I think we should ask even if the reason is sniffing.

Comment 3

[Boris Zbarsky [:bzbarsky]]

ccing Scott, Mike, Ben, and Neil, so they're aware of what's going on.  In
particular, if my review comments are implemented and no changes are made to the
toolkit/mozapps implementation of this interface, the UI behavior there will
change.  Note that I think this would be a good change overall, but it may need
UI tweaks in general, and I'd like to coordinate that with you guys.

Also, note that I'm somewhat concerned by the current lack of checks of the
"forced" boolean in the toolkit code.  I just tested, and it looks like it has
the problem SeaMonkey used to -- allowing users to select "remember this choice"
when it can't honor that request (because the core code will still pose the
dialog the next time it hits a Content-Disposition header, no matter what the
value of the "alwaysAsk" property in the MIME info).

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

[really cc'ing neil]

hm... I obviously did not file a bug for firefox after fixing bug 193698.

> How about "might be" instead of "is"?

"The file might be located at:"? ;)
(just kidding, I know what you mean)

> Also, this part probably needs review from a toolkit peer....

That file is not used by toolkit. It should not even live in that directory, imo.

Good point about always showing the dialog for sniffing.

>Do we really want to do that if reason is already REASON_SERVERREQUEST?  I
>would say we don't.

That part I weren't sure about. But consider
http://lxr.mozilla.org/seamonkey/source/uriloader/base/nsURILoader.cpp#291 - we
sniff the type even for Content-Disposition:attachment. So showing the text that
says we sniffed the type seems correct behaviour to me.

> Probably still want to init mReason, just in case.

I think I'd rather remove Init(), replacing it with constructor arguments...

> allowing users to select "remember this choice" when it can't honor that request

indeed. I don't particularly care about firefox's impl.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> we sniff the type even for Content-Disposition:attachment

Hmm... Do we want to do that?  I guess we sort of do, but add some comments to
the code where we're setting the reason pointing to this as justification for
clobbering the reason, ok?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Though about this some more....  If sniffing clobbers content-disposition, then
we absolutely need to show the dialog on sniffed content, no questions asked. 
This means that consumers who want to have their text/plain security hole can't
without breaking content-disposition handling.

Should we use a bitfield and indicate "all reasons" the dialog is posed or
something?  Or perhaps just not clobber the reason as initially suggested...
When we sniff an attachment, we may be coming up with a different type, but the
_primary_ reason we show the dialog is that it's an attachment.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177047 [details] [diff] [review]
patch v2

>Index: embedding/components/ui/helperAppDlg/nsHelperAppDlg.js
>-         // Same if this dialog was forced
>+         // Same if this dialog was forced due to a server request

"or type sniffing"?

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp
> nsExternalHelperAppService::CreateNewExternalHandler(nsIMIMEInfo * aMIMEInfo, 

Why bother with this method at all, if it just calls the constructor?  Remove
it, ok?

>+nsExternalAppHandler::nsExternalAppHandler(nsIMIMEInfo * 

Use initializers to set the members?

>Index: uriloader/exthandler/nsIHelperAppLauncherDialog.idl
>+   * Gecko detected that the type sent by the server (text/plain) does not

"e.g. text/plain" in the parentheses?

r=bzbarsky with that.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v3

changes made.

Comment 10

[Darin Fisher]

Comment on attachment 177050 [details] [diff] [review]
patch v3

Do we need to bump the UUID here?  Or are you saying that this is compatible
from an ABI point of view?

sr=darin

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in, with a new IID.

Bug 285976 filed on making use of this in firefox.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 249696 has been marked as a duplicate of this bug. ***