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

RESOLVED FIXED in mozilla1.8beta2

Status

defect
P1
normal
RESOLVED FIXED
15 years ago
3 years ago

People

(Reporter: Biesinger, Assigned: Biesinger)

Tracking

Trunk
mozilla1.8beta2
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

[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
Posted patch patch (obsolete) — Splinter Review
Attachment #176967 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
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.
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).
[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.
> 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?
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.
Posted patch patch v2 (obsolete) — Splinter Review
Attachment #176967 - Attachment is obsolete: true
Attachment #177047 - Flags: review?(bzbarsky)
Attachment #176967 - Flags: review?(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.
Attachment #177047 - Flags: review?(bzbarsky) → review+
Posted patch patch v3Splinter Review
changes made.
Attachment #177047 - Attachment is obsolete: true
Attachment #177050 - Flags: superreview?(darin)

Comment 10

15 years ago
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
Attachment #177050 - Flags: superreview?(darin) → superreview+
checked in, with a new IID.

Bug 285976 filed on making use of this in firefox.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
*** Bug 249696 has been marked as a duplicate of this bug. ***
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.