Closed Bug 303085 Opened 19 years ago Closed 6 months ago

No secure indicator after window.open() with toolbar=no (padlock icon)

Categories

(Camino Graveyard :: Security, defect, P3)

Product:
Camino Graveyard
Component:
Security
Platform:
PowerPC
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: torben, Unassigned)

References

(
URL
)

Details

Attachments

(4 files, 3 obsolete files)

Transmit's lock-in-the-corner.
3.48 KB, image/png
Details
Broken lock icon (run through tiffutil)
402 bytes, image/tiff
Details
Secure lock icon (run through tiffutil)
402 bytes, image/tiff
Details
Patch v2
13.62 KB, patch
hwaara
: review-
Details | Diff | Splinter Review 
If a page opens a page with window.open() and toolbar=no there is no indicator
for whether the new page is secure or not. Even manually showing the toolbar may
not tell, if the window is very narrow the location bar will not be shown.

Suggested fix:

Either always show the location bar on secure pages, or when the locationbar is
not shown (and only then) show the secure icon (and preferably the site/owner)
somewhere else (eg the status bar as Firefox does). In the later case the new
location must always be shown on secure sites.
This is a "regression" of sorts from removing the lock from the status bar a
while ago.

This seems bad security-wise, so I'm putting it on the 1.0 list to start.
Priority: -- → P3
Target Milestone: --- → Camino1.0
I just had an idea. When connected via SFTP, Transmit 3 shows a little lock icon
next to the toolbar widget in the title bar of the window. It's not super-hard
to do, either. Maybe this violates some HIG thing, but I like it, and I think
it's at least worth talking about.
Okay, I've got code working that does this. If we decide to go this route, all
we'll need is a lock icon from Jasper (something other than the one currently in
the location bar so it doesn't look garish in the titlebar).
this could be interesting, maybe we should always do it when the url bar is
hidden or text-only?

the question is will users understand why it's in a different place and looks
different?
I think we should always do it, always. Less confusion that way.

(Also, I just discovered that Safari does it this way, too.)
It could be a little confusing with tabs, though, since the title bar is more
removed from the tab content than the URL bar is. Perhaps having the lock icon
in front of the window title would be better, since that way it's more closely
associated with the page?
I dunno; both Safari and Transmit use the little lock in the upper-right, and
they both use tabs. The lock is just shown/hidden according to the front tab.
Here's how it looks in Transmit (the code I threw together earlier positions it
identically).
Attached file titlebar icons 10px (obsolete) — 
Attached are two tif images resembling the current security icons we have now
and the ones safari/transmit use. Images are for the broken and secure states.

Question I have is wether or not we should still show a lock icon and
background color if we now start using the lock icon in the titlebar. It's not
very good to have 2 or more identical indicators in a single window.
I think we should keep the yellow background color no matter what we decide,
personally.
I'm of the opinion that we should only do this when we're in a situation where
there's no toolbar (come to think of it, this is needed for people who browse
without the toolbar; they've been "unprotected" since the lock was removed from
the status bar), or, if we can tell, there's no location bar; otherwise, we have
a perfectly good security UI that would be weakened for 90% of our users/90% of
browsing if we started disassembling it to prevent overlap/duplication of UI
features for this one odd case.

I agree with Simon that the icon should be just to the left of the page title;
it's a familiar metaphor (the favicon to the left of the site name in the
location bar, the Mac OS X proxy icon can-of-worms) and it's very strongly
associated with the site in question (as strongly as can be in a toolbarless
window).  I find Safari's lock too far away from where my eyes are looking and
non-discoverable.  It's a security feature, so we should put it "front and
center", so to speak.

The yellow background in the URL bar is the single biggest step forward in
ease-of-security notification for users.  It's readily apparent, even at a
glance, that the page is secure, and I think it should stay no matter what (I
also think it should do something else when the page has mixed content, for the
same reason, but that's another bug...).  I really miss that feature when I'm in
other browsers.
I still have to disagree that the window proxy is the right place for the lock.
The window proxy is for a representation of the page, not an attribute of the
page. It's also a huge pain in the butt to put something there properly, as to
my knowledge there's no way to do it without using setRepresentedFilename:, and
that has to point to a local file (and also means there will be a command-click
menu which we don't want). There are Carbon APIs for it, but as far as I can
tell, they don't work on Cocoa windows (possibly because Cocoa uses Cocoa views
to do all the window drawing).
K reading trough the opinions I think we should do the following:

- keep our current implementation for all window with a toolbar visible
- when the toolbar is hidden show the attached lock/broken icon in the titlebar
- display the lock/broken icon at the same location as Safari/Transmit do (we
piggie back, users wil expect it there more then anywhere else). 
- we DON'T display it at the location of the proxy (site/fav icon) as that is
confusing in the bigger picture.
I think this should block 1.0, especially as Wevah says (comment 3) he has working code to do this.  This is, I think, the most critical security-related bug on the 1.0 list.

The main issue remaining seems to be where the lock icon goes in the toolbar in  location bar-less windows (either called by JS or user-set, as people do browse without the bar--we keep getting bugs about things not working in the sheet).
Flags: camino1.0?
Wevah: Do you think you can get a patch doing what Jasper mentions in comment 13? Smokey's right, this should be fixed by 1.0.
I don't think we want to land code to put the lock icon in the titlebar for 1.0.
What if we just force the location bar to visible for https:// popups?
I should be more clear: I have code that puts the lock in the title bar, but not code that tells that to happen for a secure site (yet).
(In reply to comment #16)
> What if we just force the location bar to visible for https:// popups?

That's good enough for me for popups, especially as I misunderstood what code Wevah had ready :-)

But what about the case(s) where users are browsing without the toolbar or without the location bar in the toolbar (i.e., using the Location sheet and links exclusively)?  Is the argument that they've made a conscious choice to disable the security UI, and we can't force them not to put their hand on the hot stove?


i think we should think more about this and try to be more consistent, not land at the 11th hour.
Flags: camino1.0? → camino1.0-
*** Bug 319720 has been marked as a duplicate of this bug. ***
Updating summary for easier searching. Also moving to security component (though keeping pink as assignee).
Component: General → Security
Summary: No secure indicator after window.open() with toolbar=no → No secure indicator after window.open() with toolbar=no (padlock icon)
Attached file Sample Controller.m file (obsolete) — 
Here's the Controller.m file I used for my test app; the applicable stuff is in -initWithWindow: (for setting up the lock image view) and -toggleSecureIcon: (which does the actual show/hide work). The |+ 1.0| in the alignment is to make the the lock icon appear to be positioned correctly (I assume because the 1px bottom border on the title bar is part of the title bar rect). The code also accounts for the toolbar show/hide widget, and positions the lock appropriately, though this might not be needed now that we always show the widget) and it doesn't reposition the lock if a toolbar is added or removed while the lock is in the title bar (which is an unlikely occurrence, anyway).
Oh yeah; I used tabs (sorry). I'll make sure not to use tabs if I submit a real patch for this. ;)
-> 1.1
Target Milestone: Camino1.0 → Camino1.1
Wevah, it'd be great if you could look at this and see if you can implement it. You seem to have a lot already done.
Assignee: mikepinkerton → dveditz
QA Contact: camino
hate to add a wrinkle... but what about the odd case where a user has customized their toolbar to display without the location bar, yet has that main toolbar visible? Not saying that should be a worry of the same sort that the bug was originally about, just that if code is being written to me "smart" about the lock icon as outlined in comment 13 it should probably take this into account when it tests
Why did Camino remove the lock from the status bar? It was put there for just this sort of case.
Assignee: dveditz → nobody
(In reply to comment #26)

I'm taking this into account now.
Here's a video of the current functionality in a test app (pretend SecItem is the location field):

http://www.derailer.org/temp/secure_titlebar_icon.mp4
Wevah, how's the patch coming? ;)
Flags: camino1.1b1?
Any chance of getting that patch here before the MBP visits FruitCo? ;)
Punting, per status meeting.
Flags: camino1.1b1? → camino1.1b1-
Target Milestone: Camino1.1 → ---
Removing keyword since this punted.
Whiteboard: l10n
I'm finally building Camino, and I have this working in my tree.
Attached patch Patch v1 (obsolete) — — Splinter Review 
Here's a patch.

(Aside: I set the "original developer" in the MPL comment for the 2 new files to myself since I wrote those two files myself; I don't know if that's appropriate or not. If it is, I might look into changing the appropriate bits of NSURL+Utils eventually.)
Attachment #205423 - Attachment is obsolete: true

  

  



    
    

    
  


  
Both of these icons are the files from the old zip attachment, but they've been run through tiffutil and so are much smaller. I put these in camino/resources/images/chrome.

  

  



        Attachment #191507 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2
        — 
          — Splinter Review
      

    


  
I forgot to release the secure icon view in BrowserWindow's dealloc.

  

  



        Attachment #281069 -
        Attachment is obsolete: true

    
    

    
  
I just noticed that I forgot to wrap some of the {s on method calls, but I'll take care of that after people tell me what else I've missed. ;)

  

  



    
    

    
  
Comment on attachment 281072 [details] [diff] [review]
Patch v2

I know Ian was itching to look at this a while back....

  

  



        Attachment #281072 -
        Flags: review?(froodian)

    
    

    
  
Comment on attachment 281072 [details] [diff] [review]
Patch v2

I already had a look at this, so I'll do a full review if OK. :)

>+- (id)initWithContentRect:(NSRect)contentRect styleMask:(unsigned int)styleMask backing:(NSBackingStoreType)bufferingType defer:(BOOL)deferCreation
>+{
>+ 	if ( (self = [super initWithContentRect:contentRect styleMask:styleMask backing:bufferingType defer:deferCreation]) )
>+		[self setUpSecureIconView];
>+  
>+ 	return self;
>+}

You have tabs here and possibly other places, and please fix the spacing to be consistent with the rest of the code.


>+#pragma mark Secure Titlebar Icon
>+
>+- (void)setUpSecureIconView {
>+  mSecureIconView = [[TitlebarSecureIconView alloc] init];
>+  [mSecureIconView setWindow:self];
>+  [mSecureIconView setSecureToolbarItemIdentifier:@"Combined Location Toolbar Item"];
>+}

I think setup is one word. If these setters are not something we'll use at runtime (only for setup), I recommend merging them all into a designated initializer. Something like: initWithWindow: toolbarItemIdentifier: etc.

>@@ -4526,16 +4527,17 @@ enum BWCOpenDest {
> 
> // updateLock:
> //
> // updates the url bar display of security state appropriately.
> - (void)updateLock:(unsigned int)inSecurityState
> {
>   unsigned char securityState = inSecurityState & 0x000000FF;
>   [mURLBar setSecureState:securityState];
>+  [[(BrowserWindow *)[self window] secureIconView] setSecureState:securityState];
> }

Hm, any idea what the deal with this mask is? Is that just a hardcoding of one of the nsIWebProgressListener constants or something else?

In order to make things cleaner, I think we should typedef the nsIWebProgressListener constants in some central camino header, to kInsecureState, kSecureState, etc. Then every place that wants to tell about its security state doesn't have to directly drag in the nsIWebProgressListener dependency either. I know people want us to separate gecko stuff from UI stuff, so this would be one such thing.

>+- (void)setSecureState:(unsigned char)inSecureState;
>+- (void)setWindow:(NSWindow *)inWindow;
>+- (void)setToolbar:(NSToolbar *)inToolbar;
>+- (void)hideIfAppropriate;
>+- (void)setSecureToolbarItemIdentifier:(NSString *)inIdentifier;

As noted above, look over if any of these can be merged in an initializer.

  

  



        Attachment #281072 -
        Flags: review?(froodian) → review-

    
    

    
  
(In reply to comment #41)
> (From update of attachment 281072 [details] [diff] [review])
> I already had a look at this, so I'll do a full review if OK. :)
> 
> >+- (id)initWithContentRect:(NSRect)contentRect styleMask:(unsigned int)styleMask backing:(NSBackingStoreType)bufferingType defer:(BOOL)deferCreation
> >+{
> >+ 	if ( (self = [super initWithContentRect:contentRect styleMask:styleMask backing:bufferingType defer:deferCreation]) )
> >+		[self setUpSecureIconView];
> >+  
> >+ 	return self;
> >+}
> 
> You have tabs here and possibly other places, and please fix the spacing to be
> consistent with the rest of the code.
> 

Good catch. I tried to catch most of these, but I obviously missed a few spots. ;)
> 
> >+#pragma mark Secure Titlebar Icon
> >+
> >+- (void)setUpSecureIconView {
> >+  mSecureIconView = [[TitlebarSecureIconView alloc] init];
> >+  [mSecureIconView setWindow:self];
> >+  [mSecureIconView setSecureToolbarItemIdentifier:@"Combined Location Toolbar Item"];
> >+}
> 
> I think setup is one word. If these setters are not something we'll use at
> runtime (only for setup), I recommend merging them all into a designated
> initializer. Something like: initWithWindow: toolbarItemIdentifier: etc.

"Setup" is a noun, "set up" is a verb/adjective.
> 
> >@@ -4526,16 +4527,17 @@ enum BWCOpenDest {
> > 
> > // updateLock:
> > //
> > // updates the url bar display of security state appropriately.
> > - (void)updateLock:(unsigned int)inSecurityState
> > {
> >   unsigned char securityState = inSecurityState & 0x000000FF;
> >   [mURLBar setSecureState:securityState];
> >+  [[(BrowserWindow *)[self window] secureIconView] setSecureState:securityState];
> > }
> 
> Hm, any idea what the deal with this mask is? Is that just a hardcoding of one
> of the nsIWebProgressListener constants or something else?
> 
> In order to make things cleaner, I think we should typedef the
> nsIWebProgressListener constants in some central camino header, to
> kInsecureState, kSecureState, etc. Then every place that wants to tell about
> its security state doesn't have to directly drag in the nsIWebProgressListener
> dependency either. I know people want us to separate gecko stuff from UI stuff,
> so this would be one such thing.

I had it this way in my sample code so I didn't have to import a bunch of Gecko stuff, so it doesn't matter to me too much.
> 
> >+- (void)setSecureState:(unsigned char)inSecureState;
> >+- (void)setWindow:(NSWindow *)inWindow;
> >+- (void)setToolbar:(NSToolbar *)inToolbar;
> >+- (void)hideIfAppropriate;
> >+- (void)setSecureToolbarItemIdentifier:(NSString *)inIdentifier;
> 
> As noted above, look over if any of these can be merged in an initializer.
> 

-setSecureState: is called every time the secure state changes (on page load, tab switch, etc.) there is only one of these views per window.

As for the rest, I wanted to make it as modular as I could; I don't want to hard-code the identifier into the view code, and keeping the other bits as separate methods makes more sense to me. -hideIfAppropriate could be named differently, but it's called in a few places and is what decides to hide or show the view on toolbar collapse and adding/removing the location field.

Perhaps our toolbar identifiers should be const NSString variables, though; then I could use the variable in the setup stuff instead, which would help stave off potential problems...

  

  



    
    

    
  
(In reply to comment #42)
> (In reply to comment #41)
> > >+#pragma mark Secure Titlebar Icon
> > >+
> > >+- (void)setUpSecureIconView {
> > >+  mSecureIconView = [[TitlebarSecureIconView alloc] init];
> > >+  [mSecureIconView setWindow:self];
> > >+  [mSecureIconView setSecureToolbarItemIdentifier:@"Combined Location Toolbar Item"];
> > >+}
> > 
> > I think setup is one word. If these setters are not something we'll use at
> > runtime (only for setup), I recommend merging them all into a designated
> > initializer. Something like: initWithWindow: toolbarItemIdentifier: etc.
> 
> "Setup" is a noun, "set up" is a verb/adjective.

You are most likely right in a grammatical sense. But in our code (http://mxr.mozilla.org/seamonkey/search?find=%2Fcamino%2F&string=setup) I find only one case of using "setUp" camel-cased in a method name and far more uses of "setup".

> > 
> > >@@ -4526,16 +4527,17 @@ enum BWCOpenDest {
> > > 
> > > // updateLock:
> > > //
> > > // updates the url bar display of security state appropriately.
> > > - (void)updateLock:(unsigned int)inSecurityState
> > > {
> > >   unsigned char securityState = inSecurityState & 0x000000FF;
> > >   [mURLBar setSecureState:securityState];
> > >+  [[(BrowserWindow *)[self window] secureIconView] setSecureState:securityState];
> > > }
> > 
> > Hm, any idea what the deal with this mask is? Is that just a hardcoding of one
> > of the nsIWebProgressListener constants or something else?
> > 
> > In order to make things cleaner, I think we should typedef the
> > nsIWebProgressListener constants in some central camino header, to
> > kInsecureState, kSecureState, etc. Then every place that wants to tell about
> > its security state doesn't have to directly drag in the nsIWebProgressListener
> > dependency either. I know people want us to separate gecko stuff from UI stuff,
> > so this would be one such thing.
> 
> I had it this way in my sample code so I didn't have to import a bunch of Gecko
> stuff, so it doesn't matter to me too much.

Sorry, I don't understand this reply. Does that mean you will change it or not? :) I would prefer you do for the reasons above. Also, that hardcoded mask is confusing so let's fix that at the same time.

> > 
> > >+- (void)setSecureState:(unsigned char)inSecureState;
> > >+- (void)setWindow:(NSWindow *)inWindow;
> > >+- (void)setToolbar:(NSToolbar *)inToolbar;
> > >+- (void)hideIfAppropriate;
> > >+- (void)setSecureToolbarItemIdentifier:(NSString *)inIdentifier;
> > 
> > As noted above, look over if any of these can be merged in an initializer.
> > 
> 
> -setSecureState: is called every time the secure state changes (on page load,
> tab switch, etc.) there is only one of these views per window.
> 
> As for the rest, I wanted to make it as modular as I could; I don't want to
> hard-code the identifier into the view code, and keeping the other bits as
> separate methods makes more sense to me. -hideIfAppropriate could be named
> differently, but it's called in a few places and is what decides to hide or
> show the view on toolbar collapse and adding/removing the location field.

Just make it as useful and modular as a "TitlebarSecureIconView" can ever be. As one hears by the name, it is already very specific. Please use the initializer for the properties that will never in practice change (e.g., the owning window, if I understand you right - and also toolbar?). If we one day decide that a TitlebarSecureIconView's window would need to change on the fly, we can add the setter.

> Perhaps our toolbar identifiers should be const NSString variables, though;
> then I could use the variable in the setup stuff instead, which would help
> stave off potential problems...

As for the string identifier: it's a good idea to make it an extern const NSString in the header.

  

  



    
    

    
  
(In reply to comment #43)
> (In reply to comment #42)
> > (In reply to comment #41)
> > > >+#pragma mark Secure Titlebar Icon
> > > >+
> > > >+- (void)setUpSecureIconView {
> > > >+  mSecureIconView = [[TitlebarSecureIconView alloc] init];
> > > >+  [mSecureIconView setWindow:self];
> > > >+  [mSecureIconView setSecureToolbarItemIdentifier:@"Combined Location Toolbar Item"];
> > > >+}
> > > 
> > > I think setup is one word. If these setters are not something we'll use at
> > > runtime (only for setup), I recommend merging them all into a designated
> > > initializer. Something like: initWithWindow: toolbarItemIdentifier: etc.
> > 
> > "Setup" is a noun, "set up" is a verb/adjective.
> 
> You are most likely right in a grammatical sense. But in our code
> (http://mxr.mozilla.org/seamonkey/search?find=%2Fcamino%2F&string=setup) I find
> only one case of using "setUp" camel-cased in a method name and far more uses
> of "setup".

Fine with me (though see also NSPopUpButton).
> 
> > > 
> > > >@@ -4526,16 +4527,17 @@ enum BWCOpenDest {
> > > > 
> > > > // updateLock:
> > > > //
> > > > // updates the url bar display of security state appropriately.
> > > > - (void)updateLock:(unsigned int)inSecurityState
> > > > {
> > > >   unsigned char securityState = inSecurityState & 0x000000FF;
> > > >   [mURLBar setSecureState:securityState];
> > > >+  [[(BrowserWindow *)[self window] secureIconView] setSecureState:securityState];
> > > > }
> > > 
> > > Hm, any idea what the deal with this mask is? Is that just a hardcoding of one
> > > of the nsIWebProgressListener constants or something else?
> > > 
> > > In order to make things cleaner, I think we should typedef the
> > > nsIWebProgressListener constants in some central camino header, to
> > > kInsecureState, kSecureState, etc. Then every place that wants to tell about
> > > its security state doesn't have to directly drag in the nsIWebProgressListener
> > > dependency either. I know people want us to separate gecko stuff from UI stuff,
> > > so this would be one such thing.
> > 
> > I had it this way in my sample code so I didn't have to import a bunch of Gecko
> > stuff, so it doesn't matter to me too much.
> 
> Sorry, I don't understand this reply. Does that mean you will change it or not?
> :) I would prefer you do for the reasons above. Also, that hardcoded mask is
> confusing so let's fix that at the same time.
> 

I guess the hardcoded mask could be replaced with (securestate | insecurestate | brokenstate) (where the names I wrote would be replaced with the actual constants.
> > > 
> > > >+- (void)setSecureState:(unsigned char)inSecureState;
> > > >+- (void)setWindow:(NSWindow *)inWindow;
> > > >+- (void)setToolbar:(NSToolbar *)inToolbar;
> > > >+- (void)hideIfAppropriate;
> > > >+- (void)setSecureToolbarItemIdentifier:(NSString *)inIdentifier;
> > > 
> > > As noted above, look over if any of these can be merged in an initializer.
> > > 
> > 
> > -setSecureState: is called every time the secure state changes (on page load,
> > tab switch, etc.) there is only one of these views per window.
> > 
> > As for the rest, I wanted to make it as modular as I could; I don't want to
> > hard-code the identifier into the view code, and keeping the other bits as
> > separate methods makes more sense to me. -hideIfAppropriate could be named
> > differently, but it's called in a few places and is what decides to hide or
> > show the view on toolbar collapse and adding/removing the location field.
> 
> Just make it as useful and modular as a "TitlebarSecureIconView" can ever be.
> As one hears by the name, it is already very specific. Please use the
> initializer for the properties that will never in practice change (e.g., the
> owning window, if I understand you right - and also toolbar?). If we one day
> decide that a TitlebarSecureIconView's window would need to change on the fly,
> we can add the setter.

You're right about the window, it shouldn't ever change. However, the toolbar is added in BWC after the window has already been initialized, hence the override to setToolbar: in BrowserWindow that calls the view's setToolbar:

I could have had BWC be the owner of the view (as it is for the toolbar), but then I'd have to have BrowserWindow call back to BWC when the toolbar was shown and hidden (and since the window owns the rest of the titlebar widgets, I decided to go this route).
> 
> > Perhaps our toolbar identifiers should be const NSString variables, though;
> > then I could use the variable in the setup stuff instead, which would help
> > stave off potential problems...
> 
> As for the string identifier: it's a good idea to make it an extern const
> NSString in the header.
> 

Aye, though it should probably be used by BWC also. (Also it'd be extern NSString * const; I could set it up for just the window for now, I suppose.)

  

  



    
    

    
  
(In reply to comment #43)

> > > I think setup is one word. If these setters are not something we'll use at
> > > runtime (only for setup), I recommend merging them all into a designated
> > > initializer. Something like: initWithWindow: toolbarItemIdentifier: etc.
> > 
> > "Setup" is a noun, "set up" is a verb/adjective.
> 
> You are most likely right in a grammatical sense. But in our code
> (http://mxr.mozilla.org/seamonkey/search?find=%2Fcamino%2F&string=setup) I find
> only one case of using "setUp" camel-cased in a method name and far more uses
> of "setup".

We should strive to make our code correct in terms of spelling and grammar, too ;)

  

  



    
    

    
  
Wevah, are you going to have time to work up a new patch based on review comments any time soon?

  

  



    
    

    
  
Unassigning myself so it doesn't show up in my dashboard anymore.

  

  


Assignee: moz → nobody

    
  
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → INCOMPLETE

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: