402620 - protocol handling dialog should display icons and hostnames for web applications

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P2)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Both web apps and local handler apps.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

I suspect this wants to block Firefox3, as it looks somewhat goofy now.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Probably also want to give an icon for web apps to the dropdown in Applications pref pane at the same time (local handler apps already have them there).

Comment 3

[Florian Quèze [:florian]]

(In reply to comment #2)
> Probably also want to give an icon for web apps to the dropdown in Applications
> pref pane at the same time (local handler apps already have them there).
> 

In my gmail mailto: handler I already see the gmail favicon so I suppose your are talking about a default icon for handlers without favicon?

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Yeah, I just added a comment to bug 391576 about coming up with an icon for that case.

Comment 5

[Florian Quèze [:florian]]

For web application, we need to display the hostname because otherwise it would be easy to register a protocol handler spoofing a local application.

Comment 6

[Florian Quèze [:florian]]

Attachment: patch v1

Comment 7

[Florian Quèze [:florian]]

Attachment: screenshot of the window with the previous patch applied

Comment 8

[Shawn Wilsher :sdwilsh]

Hmm, shouldn't we be using TLD like the download manager does?  This could be subject to some form of fraud:
http://gmail.com.some.super.malicious.site.that.wants.to.steal.your.login....
It'd be cut off, but the user would probably see gmail.com and think it'd be OK.

And good call changing the try catch to instanceof.  I thought I had filed a bug on that, but clearly I didn't (I should have done it right in the first place though).

Comment 9

[Florian Quèze [:florian]]

(In reply to comment #8)
> Hmm, shouldn't we be using TLD like the download manager does?  This could be
> subject to some form of fraud:
> http://gmail.com.some.super.malicious.site.that.wants.to.steal.your.login....
> It'd be cut off, but the user would probably see gmail.com and think it'd be
> OK.
> 

Hmm, I was more thinking about a fraud like http://mozilla.thunderbird.blahblahblahblahblah.evil.com and that's why I wanted to display the scheme at the beginning. Also I would probably not use a mailto handler which is not over https.

Maybe I should add something like crop="center".

Comment 10

[Shawn Wilsher :sdwilsh]

Well, we could display a lock icon (probably a bad idea there) for secure ones, but will everything be over http?  I mean, (dmose correct me if I'm wrong since you are more familiar with the spec), couldn't it dispatch to another protocol if it wanted to?

Also, shouldn't we be using the favicon service to get the favorite icon?  I'm about 90% sure that's toolkit code.

Comment 11

[Florian Quèze [:florian]]

(In reply to comment #10)

> Also, shouldn't we be using the favicon service to get the favorite icon?  I'm
> about 90% sure that's toolkit code.
> 

According to the comment here we can't:
http://mxr.mozilla.org/seamonkey/source/browser/components/preferences/applications.js#1657

I assumed this comment was right so I haven't tried.

Comment 12

[Shawn Wilsher :sdwilsh]

I would ask some places devs about that to be sure (maybe that should change?).

Comment 13

[Shawn Wilsher :sdwilsh]

cc'ing Johnathan since he's been doing lots of security UI work.

Comment 14

[Johnathan Nightingale [:johnath]]

Yes, what's there right now does look sort of silly.  :)

My reactions are that yeah, eTLD+1 makes sense here, and should be easier now that nsIEffectiveTLDService has been improved.

I understand what Florian's saying in comment 9 about the risks of sending, e.g., confidential information over http, but I think whether or not we choose to talk about the security of the channel, we can do so better than just preserving the scheme, which users don't find all that intuitive anyhow.

I don't think a lock overlay is a terrible idea here - despite being a conflated signal, the lock here would be a bit more accurate - closer to "this channel is secure" than "you can trust this site" because at this point, they've already made the trust decision about the site/app operator.  As Shawn points out though, this assumes they are using a scheme like https, and I also think we could fix the rest of this bug without worrying about signaling channel encryption, and handle that discussion separately.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #10)
> Well, we could display a lock icon (probably a bad idea there) for secure ones,
> but will everything be over http?  I mean, (dmose correct me if I'm wrong since
> you are more familiar with the spec), couldn't it dispatch to another protocol
> if it wanted to?

Yes.  All the lock could really tell you is whether the handler entry page is over https.

(In reply to comment #11)
> According to the comment here we can't:
> http://mxr.mozilla.org/seamonkey/source/browser/components/preferences/applications.js#1657

I suspect that whether that comment is accurate may depend on how the particular web application is structured.  It seems like in at least some (most? all?) cases, we should be able to ask the favicon service for an entry corresponding to uri.prePath.  I suspect myk knows more, as it was his comment; I've added him to the CC for advice...

(In reply to comment #14)
> My reactions are that yeah, eTLD+1 makes sense here, and should be easier now
> that nsIEffectiveTLDService has been improved.

Yeah, this sounds right to me too.

As far as the lock goes, I kinda like it, but I see that it has some issues too.  I guess I'd vote for including it unless our ui-reviewer of record objects.  beltzner?

Comment 16

[Jeff Walden [:Waldo]]

Do remember that the icon is site-controlled, and I'd hesitate to allow a site to display whatever icon it liked here if it was interested in spoofing and redirecting to the actual site.

Comment 17

[Florian Quèze [:florian]]

That's why if we display the icon we need the hostname too.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #15)
> (In reply to comment #14)
> > My reactions are that yeah, eTLD+1 makes sense here, and should be easier now
> > that nsIEffectiveTLDService has been improved.
> 
> Yeah, this sounds right to me too.

Agreed.

> As far as the lock goes, I kinda like it, but I see that it has some issues
> too.  I guess I'd vote for including it unless our ui-reviewer of record
> objects.  beltzner?

We're deprecating the lock metaphor in Fx3, so I'd say "no".

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 288790 [details] [diff] [review]
patch v1

Let's get this in for now, we can discuss the protocol issue later. My feeling is that we should only show the protocol if it's https:// as a way of satisfying people who say "I don't want to use it if it isn't https!" more than anything else.

As I stated above, I don't think the lock icon is a good idea - it's a nice shorthand, yes, but a little too overloaded in meaning.

Comment 20

[Florian Quèze [:florian]]

Attachment: patch v1.1 (same unbitrotted)

As per comment 19, let's get this reviewed and move the discussion about eTLD+1/hostname/protocol/lock icon/etc... to a followup bug.

Comment 21

[Asaf Romano (gone)]

Looking at it again, i disagree with the comment in _getIconURLForWebApp (applications.js), we should still try to use the favicon service for uri.*prePath* and only then default to favicon.icon.

Comment 22

[Florian Quèze [:florian]]

Attachment: patch v2

Ok, using the favicon service and falling back to favicon.ico if it fails.

Comment 23

[Asaf Romano (gone)]

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

s/padding-left/-moz-padding-start/

r=mano otherwise.

Comment 24

[Florian Quèze [:florian]]

Attachment: patch v2bis (as checked in)

(In reply to comment #23)
> (From update of attachment 298406 [details] [diff] [review])
> s/padding-left/-moz-padding-start/
>

I should make a sticky note of this.  It's at least the third time you notice it in my code!

Comment 25

[Florian Quèze [:florian]]

Checking in toolkit/mozapps/handling/content/handler.xml;
/cvsroot/mozilla/toolkit/mozapps/handling/content/handler.xml,v  <--  handler.xml
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/mozapps/handling/content/dialog.js;
/cvsroot/mozilla/toolkit/mozapps/handling/content/dialog.js,v  <--  dialog.js
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/themes/winstripe/mozapps/handling/handling.css;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/handling/handling.css,v  <--  handling.css
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/themes/pinstripe/mozapps/handling/handling.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/handling/handling.css,v  <--  handling.css
new revision: 1.3; previous revision: 1.2
done

Comment 26

[Florian Quèze [:florian]]

(In reply to comment #20)

> As per comment 19, let's get this reviewed and move the discussion about
> eTLD+1/hostname/protocol/lock icon/etc... to a followup bug.
> 

Filed bug 413567 to track this issue.