518601 - Troubleshooting Information page should not allow copy-and-paste of the profile directory.

Status: VERIFIED FIXED

(Firefox :: General, defect, P2)

Description

[Curtis Bartley [:cbartley]]

The Troubleshooting Information page (about:support) should not allow copy-and-paste of the profile directory.  It should, however, provide a way for the user to easily open the profile directory. 

This should block Firefox 3.6 final (security issue), but not 3.6 beta.

Comment 1

[Johnathan Nightingale [:johnath]]

We either need the button to "show in finder/explorer" or we need to remove the line. This blocks final release, but not beta.

Comment 2

[Majken Connor [:Kensie]]

By remove the line do you mean remove said button, or to not show the profile directory at all? Sorry if this is an obvious question to others!

Comment 3

[Johnathan Nightingale [:johnath]]

(In reply to comment #2)
> By remove the line do you mean remove said button, or to not show the profile
> directory at all? Sorry if this is an obvious question to others!

Jesse points out in bug 367596 that we go to some effort to obscure the profile directory from accidental disclosure, since that can be used as a launchpad for turning low-severity attacks into critical ones - putting the path in here, in an easy-to-copy/paste fashion, undoes some of that protection (albeit requiring attackers to do some social engineering).

Since SUMO's use case seems to be more around getting people to *open* their profile directory (to, e.g. remove or check for a particular file), a button that said "Open profile directory" would get us the support win, without disclosing in an easy-to-copy/paste way what that directory actually was. But if we can't get that button done for 3.6, then we should remove the profile directory information from the page entirely, and get it right in 3.7.

My preference and expectation, though, is that we'll get the button.

Comment 4

[Daniel Veditz [:dveditz]]

This is not a "security" problem that needs a [sg] whiteboard marker, rather it's a potential privacy problem.

Comment 5

[Curtis Bartley [:cbartley]]

Attachment: Replaced profile path with a button to reveal the profile in the OS GUI, v1

L10N: 

The new button uses the same "Show in Finder/Open Containing Folder" labels as in the Downloads window context menu.  This patch duplicates the entities used for these labels in aboutSupport.dtd.  Localization notes indicate that they should be translated the same way as the originals.

We could alternatively reference downloads.dtd (mozapps/locale/downloads/downloads.dtd) directly and use the original entities from there.

Johnath:

This patch replaces the profile path with a button which will open the profile directory in the Finder/Explorer/etc.  The path to the profile directory is no longer displayed at all and consequently can't be copied from this page.

I'll attach a screenshot showing what this looks like.

I don't know if this will work on Mobile.

Comment 6

[Curtis Bartley [:cbartley]]

Attachment: Mac OS X screenshot of the profile button.

The label "Show in Finder" is used on Mac; "Open Containing Folder" is used on the other platforms.

Comment 7

[Johnathan Nightingale [:johnath]]

(In reply to comment #5)
> L10N: 
> 
> The new button uses the same "Show in Finder/Open Containing Folder" labels as
> in the Downloads window context menu.  This patch duplicates the entities used
> for these labels in aboutSupport.dtd.  Localization notes indicate that they
> should be translated the same way as the originals.
> 
> We could alternatively reference downloads.dtd
> (mozapps/locale/downloads/downloads.dtd) directly and use the original entities
> from there.

I'll defer my review until we get an answer from Axel here - is it false economy to just use the download manager dtd on 3.6 to avoid more string churn? Or is that genuinely a good idea, and we can land the real strings on -central only?

Comment 8

[Axel Hecht [:Pike]]

I think we should fix this on central independent of what we do on 1.9.2, i.e., a patch with new strings is the right thing to do there.

For 1.9.2, we need to triage all strings together. For this bug, reusing the strings seems to be low risk, but that doesn't make it the right thing. Iff we need that string in bug 518468, and it's only three in total then, I'd take the patch as is on 1.9.2. In all other cases, doing the "least wrong thing" is a tougher call.

Comment 9

[Johnathan Nightingale [:johnath]]

(In reply to comment #8)
> I think we should fix this on central independent of what we do on 1.9.2, i.e.,
> a patch with new strings is the right thing to do there.
> 
> For 1.9.2, we need to triage all strings together. For this bug, reusing the
> strings seems to be low risk, but that doesn't make it the right thing. Iff we
> need that string in bug 518468, and it's only three in total then, I'd take the
> patch as is on 1.9.2. In all other cases, doing the "least wrong thing" is a
> tougher call.

Okay, thanks Axel. I'll review the trunk patch now, and then we'll figure out a branch strategy in the context of other strings.

(to be clear, the patch in bug 518468 adds 5 string to browser.properties - https://bugzilla.mozilla.org/attachment.cgi?id=405107&action=diff#a/browser/locales/en-US/chrome/browser/browser.properties_sec1 )

Comment 10

[Johnathan Nightingale [:johnath]]

Comment on attachment 406079 [details] [diff] [review]
Replaced profile path with a button to reveal the profile in the OS GUI, v1 

>diff -r 59dcd1b1a220 browser/base/content/aboutSupport.xhtml
>-            <td id="profile-box">
>+            <td>

why did you remove the id?

>diff -r 59dcd1b1a220 browser/locales/en-US/chrome/browser/aboutSupport.dtd
>--- a/browser/locales/en-US/chrome/browser/aboutSupport.dtd	Mon Oct 12 18:27:24 2009 -0400
>+++ b/browser/locales/en-US/chrome/browser/aboutSupport.dtd	Tue Oct 13 13:32:51 2009 -0700
>@@ -20,6 +20,14 @@
> <!ENTITY aboutSupport.appBasicsPlugins "Installed Plugins">
> <!ENTITY aboutSupport.appBasicsBuildConfig "Build Configuration">
> 
>+<!-- LOCALIZATION NOTE (aboutSupport.show.label): this entity should be
>+translated the same way as cmd.show.label in downloads.dtd. -->
>+<!ENTITY aboutSupport.show.label "Open Containing Folder">
>+
>+<!-- LOCALIZATION NOTE (aboutSupport.showMac.label): this entity should be
>+translated the same way as cmd.showMac.label in downloads.dtd. -->
>+<!ENTITY aboutSupport.showMac.label "Show in Finder">

Not sure these localization notes need to link the two entities. A localization note telling people that one is mac-specific and should use the "finder" language makes sense, but if a localizer actually did want to use different terms in this context for some reason, I don't think that would be a problem, would it?

r=me for central, and we'll figure out what approach to take on branch, as Axel says.

Comment 11

[Curtis Bartley [:cbartley]]

(In reply to comment #10)
> (From update of attachment 406079 [details] [diff] [review])
> >diff -r 59dcd1b1a220 browser/base/content/aboutSupport.xhtml
> >-            <td id="profile-box">
> >+            <td>
> 
> why did you remove the id?


I'm not using it anymore.  In the previous patch I was using it to insert the profile directory path into the DOM, but now we're not doing that.


> >diff -r 59dcd1b1a220 browser/locales/en-US/chrome/browser/aboutSupport.dtd
> >--- a/browser/locales/en-US/chrome/browser/aboutSupport.dtd	Mon Oct 12 18:27:24 2009 -0400
> >+++ b/browser/locales/en-US/chrome/browser/aboutSupport.dtd	Tue Oct 13 13:32:51 2009 -0700
> >@@ -20,6 +20,14 @@
> > <!ENTITY aboutSupport.appBasicsPlugins "Installed Plugins">
> > <!ENTITY aboutSupport.appBasicsBuildConfig "Build Configuration">
> > 
> >+<!-- LOCALIZATION NOTE (aboutSupport.show.label): this entity should be
> >+translated the same way as cmd.show.label in downloads.dtd. -->
> >+<!ENTITY aboutSupport.show.label "Open Containing Folder">
> >+
> >+<!-- LOCALIZATION NOTE (aboutSupport.showMac.label): this entity should be
> >+translated the same way as cmd.showMac.label in downloads.dtd. -->
> >+<!ENTITY aboutSupport.showMac.label "Show in Finder">
> 
> Not sure these localization notes need to link the two entities. A localization
> note telling people that one is mac-specific and should use the "finder"
> language makes sense, but if a localizer actually did want to use different
> terms in this context for some reason, I don't think that would be a problem,
> would it?


They were written that way in case we did decide to take this patch on branch, in which case we'd want to make the localizers' job as easy as possible.  I'll adjust the comments appropriately for mozilla-central.


> r=me for central, and we'll figure out what approach to take on branch, as Axel
> says.

Comment 12

[Johnathan Nightingale [:johnath]]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 406079 [details] [diff] [review] [details])
> > >diff -r 59dcd1b1a220 browser/base/content/aboutSupport.xhtml
> > >-            <td id="profile-box">
> > >+            <td>
> > 
> > why did you remove the id?
> 
> I'm not using it anymore.  In the previous patch I was using it to insert the
> profile directory path into the DOM, but now we're not doing that.

K. I don't think it hurts anything to leave it in, but I'm not attached to it either.

Ship it.

Comment 13

[Curtis Bartley [:cbartley]]

Attachment: Replaced profile path with a button to reveal the profile in the OS GUI, v2

Identical to v1 except that the localization notes in aboutSupport.dtd have been updated.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/1917a92c3a7c

please add back checkin-needed once a branch patch is attached and ready.

Comment 15

[Johnathan Nightingale [:johnath]]

Curtis - on today's engineering call, Axel said that getting this bug in with the string changes should be containable if it gets in this week. Can you please attach a cleanly-applying branch patch post-haste (if this one doesn't), and get some help landing it?

Axel, please correct me if I've misunderstood. I know you and beltzner were talking about an overall list, so that we know how large the overall size is, but this is one of the two bugs I believe you called out explicitly, right? (The other being the personas undo support, bug 518468).

Comment 16

[Curtis Bartley [:cbartley]]

Attachment: [patch for 1.9.2] Replaced profile path with a button to reveal the profile in the OS GUI, v2

Comment 17

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

Comment on attachment 407431 [details] [diff] [review]
[patch for 1.9.2] Replaced profile path with a button to reveal the profile in the OS GUI, v2

a192=beltzner

Comment 18

[Tony Chung [:tchung]]

Verified fix on trunk.   Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091021 Minefield/3.7a1pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091021 Minefield/3.7a1pre

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ad80351ed9da