654912 - "Open all..." for attachments is available in File -> Attachments menu, but not attachments pane

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, enhancement)

Description

[Jim Porter (:squib)]

Attachment: Add "Open all..." to attachment list and toolbar button

Under File -> Attachments, there's an "Open all..." menu item, but there isn't one in the attachment pane, either in the list itself, or as a menu item from the toolbar button. Here's a patch to add that.

Comment 1

[rsx11m]

Just a note of caution: I wouldn't use "Open All" on a closed attachment list where all information I have is "3 attachments" or so, not knowing which type of attachment it is and if it potentially disguises a virus...

It's probably ok to offer "Open All" when the attachment pane is open and the user has verified that it's safe to open them all at the same time.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 530228 [details] [diff] [review]
Add "Open all..." to attachment list and toolbar button

Review of attachment 530228 [details] [diff] [review]:

First off, the patch failed to apply for me on trunk.  :(

So I manually fixed it up, I think, but you should probably look into that…

Secondly, I think we want tests for this, cause I'm like that.

Other than that, it looks good to me, so r=me with the fixup and some tests.

Comment 3

[rsx11m]

So, you don't see this being a possible security hazard per comment #1,
or did I read Jim's patch wrong? (I didn't apply it.)

Comment 4

[Blake Winton (:bwinton) (:☕️)]

I agree that it's a potential security hazard, but not much more of one than the Attachments menu already is, and we are partially mitigating it by hiding the option behind a drop-down button.

Furthermore, forcing the user to open the attachment list doesn't guarantee that they'll look at all the attachments, much less look for things that might be viruses, it just adds an extra roadblock they need to go through to do what they want.

And who's to say that they didn't already open the attachment pane in a previous session, and now really just want to show Gramma the pictures of their grandkids?  ;)

So, to sum up, you're right about the security hazard, but I think it's minor enough that it shouldn't block the UI.

Thanks,
Blake.

Comment 5

[rsx11m]

(In reply to comment #4)
> I agree that it's a potential security hazard, but not much more of one than
> the Attachments menu already is, and we are partially mitigating it by hiding
> the option behind a drop-down button.

Wrong, in contrast to the drop-down menu File > Attachments also shows you the actual attachments with name and icon (unless someone changed that by now), thus it's easy to spot an ".scr" or something else that may look suspicious.

> Furthermore, forcing the user to open the attachment list doesn't guarantee
> that they'll look at all the attachments, much less look for things that might
> be viruses, it just adds an extra roadblock they need to go through to do what
> they want.

In that case it's their fault at least, at some point one has to accept that the user still wants to do something stupid though we've tried to prevent that.

> And who's to say that they didn't already open the attachment pane in a
> previous session, and now really just want to show Gramma the pictures of their
> grandkids?  ;)

That's the only point I'd accept, though one could keep track of that.

Comment 6

[rsx11m]

> (comment #4) it just adds an extra roadblock they need to go through to do what
> they want.

BTW: That's an interesting statement given that you denied removing that roadblock in bug 647036 as part of Thunderbird to start with...

Comment 7

[Joe Sabash [:JoeS1]]

(In reply to comment #5)
> (In reply to comment #4)
> > I agree that it's a potential security hazard, but not much more of one than
> > the Attachments menu already is, and we are partially mitigating it by hiding
> > the option behind a drop-down button.
> 
> Wrong, in contrast to the drop-down menu File > Attachments also shows you the
> actual attachments with name and icon (unless someone changed that by now),
> thus it's easy to spot an ".scr" or something else that may look suspicious.
> 

Yes, File >Attachments most definitely shows you the filenames and extension.
TB won't open executables but we should give some thought to providing the ability to "save all" attachments without first displaying them also.

Make it easier to show Grandma the pictures, but lets not encourage careless behaviors along the way.

Comment 8

[Jim Porter (:squib)]

I don't think it's actually possible to run an attached executable in Thunderbird without saving it first.

Comment 9

[rsx11m]

That's probably true for "real" executables which don't need any helper application to run. Something like a ".vbs" script may still work though.
It's hard for me to estimate any security implications of different file
types, but I nevertheless wouldn't open what I don't recognize, which means
I'd have to see it first to make that determination...

> Make it easier to show Grandma the pictures, but lets not encourage careless
> behaviors along the way.

That about sums it up.

Comment 10

[Jim Porter (:squib)]

Checked in: http://hg.mozilla.org/comm-central/rev/b69d9a4cc169

(Worst-case scenario, we back this out, but I wanted to get it in before string freeze since it has r+ anyway.)

Comment 11

[rsx11m]

To be precise:
> (comment #2) so r=me with the fixup and some tests.

Thus, if Blake is picky he'd request this bug to be reopened for the tests or that you open a follow-up bug to take care of those - or that you back it out. ;-)

And, I still think it's encouraging careless behavior unless "Open All" is blocked at least for new messages before the attachments are made visible, but that logic also could be added after the string freeze...

Comment 12

[Blake Winton (:bwinton) (:☕️)]

squib, how soon do you think you can get the tests written?

Thanks,
Blake.

Comment 13

[Jim Porter (:squib)]

(In reply to comment #12)
> squib, how soon do you think you can get the tests written?

Probably this evening. I forgot to enable/disable the "Open all..." menuitem properly, so I'll be fixing that too. It should be straightforward, however.