384897 - in-product links to AMO should be https:// (and not http://)

Status: VERIFIED FIXED

(Firefox :: General, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

should our in-product links to AMO be https:// (and not http://)?

with a new profile, I noticed that the pre-configured "Get Bookmark Add-ons" bookmark took me to http://en-US.add-ons.mozilla.com/en-US/firefox/bookmarks/ which re-directed to https://addons.mozilla.org/en-US/firefox/browse/type:1/cat:22/sort:popular

Shouldn't the bookmark in bookmarks.html be https://...add-ons.mozilla.com and not http://...add-ons.mozilla.com?

browser/locales/en-US/profile/bookmarks.html, 
line 10 -- <DT><A HREF="http://en-US.add-ons.mozilla.com/en-US/firefox/bookmarks/"

If so, using http://mxr.mozilla.org/seamonkey/search?string=ons.mozilla there might be other http:// references that should be https://


/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,

line 35 -- extensions.getMoreExtensionsURL=http://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/%VERSION%/extensions/
line 36 -- extensions.getMoreThemesURL=http://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/%VERSION%/themes/

/browser/app/profile/firefox.js, 
line 151 -- pref("extensions.getMoreExtensionsURL", "http://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/%VERSION%/extensions/");
line 152 -- pref("extensions.getMoreThemesURL", "http://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/%VERSION%/themes/");

browser/components/places/tests/unit/bookmarks.preplaces.html, 
line 10 -- <DT><A HREF="http://en-US.add-ons.mozilla.com/en-US/firefox/bookmarks/" ICON="data:image/

/mail/app/profile/all-thunderbird.js, 
line 132 -- pref("extensions.getMoreExtensionsURL", "http://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/%VERSION%/extensions/");
line 133 -- pref("extensions.getMoreThemesURL", "http://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/%VERSION%/themes/");

Comment 1

[Mike Connor [:mconnor]]

Yes, we should definitely do this...

Comment 2

[Simon Paquet [:sipaq]]

Attachment: (checked in)Patch to fix FF3 blocker

Fix for mozilla/browser and mozilla/toolkit

Comment 3

[(not reading, please use [e-mail] instead)]

Comment on attachment 269279 [details] [diff] [review]
(checked in)Patch to fix FF3 blocker

looks good to me, r=sspitzer

please wait for morgamic's review as well, as this is related to a.m.o.

Comment 4

[(not reading, please use [e-mail] instead)]

> /mail/app/profile/all-thunderbird.js,

tbird version spun off to bug #385385

Comment 5

[Michael Morgan [:morgamic]]

Comment on attachment 269279 [details] [diff] [review]
(checked in)Patch to fix FF3 blocker

Looks good.  Links work fine under https.

Comment 6

[Phil Ringnalda (:philor)]

Hmm, blocking1.8.1.5?, you say? Here's an interesting 1.8.1 search:
http://lxr.mozilla.org/l10n-mozilla1.8/search?string=getmoreextensionsurl

(1.8.0 has much more consistent URLs, though three are not https.)

Comment 7

[Axel Hecht [:Pike]]

May I ask why "definitely"? I see a point to verify the identity of amo at the point of installing extensions, but before that, isn't this just a regular website?

If so, there should probably be a check in some tool to verify that it actually is in all our localizations. It would also imply a semantic change, thus would require a more semantic name indicating the relevance of a secure connection, probably.

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to comment #7)
> May I ask why "definitely"? I see a point to verify the identity of amo at the
> point of installing extensions, but before that, isn't this just a regular
> website?

If your original link is not SSL then a MITM version of AMO will simply not redirect to https -- who is going to notice?  So you stay on the fake AMO all the way through until you download and install malware. This is akin to the flaw of having a non-SSL login page thinking you're ok because the POST is to a secure URL. If everything goes fine then this is perfectly secure, but the point is to protect against maliciousness.

Comment 9

[Phil Ringnalda (:philor)]

Are there bugs on Sunbird, SeaMonkey, and Composer, to use non-localized prefs, rather than the clearly-a-bad-idea properties? Getting rid of the properties on the trunk, and dealing with the various branch locale issues separately, seems like a better way to go.

Comment 10

[Axel Hecht [:Pike]]

Is this only affecting links to AMO?

We'll need a patch for l10n, and a test, too.

Regarding other apps, I recall that at least Robert wanted to keep the links in localized content as SeaMonkey doesn't have a central website structure.

Comment 11

[Robert Kaiser]

SeaMonkey has all https on trunk (in 
suite/locales/en-US/chrome/branding/brand.properties) and on 1.8 branch (where we only have the dictionaries UI anyways), with the exception of the branch mailnews start page that links to "http://addons.mozilla.org/" - is that one critical or OK for branch?

For branch, I think our localizers should have picked up the https URL when we recently pointed out the should use the correct URLs for dictionaries on branch. On trunk, we have no localizations for SeaMonkey landed, and as those are in brand.properties, it's probably something we should routinely check when new localizations will be added there.

So, unless we need to change the branch mailnews page link, SeaMonkey should be in good shape there.

Comment 12

[Simon Paquet [:sipaq]]

Thanks for the reviews sspitzer and morgamic. 

Since I only have checkin rights for mozilla/calendar I would really appreciate it, if you or someone else watching this bug could do the checkin for me.

Comment 13

[Phil Ringnalda (:philor)]

Yeah, I was confused - only Calendar and Composer are consumers of the getMore(Extensions|Themes)URL properties. However, they still are, and the state of those URLs in 1.8 is the result of at least one huge change in the format of the URL without a corresponding change in the property name, so I would really appreciate it if nobody checked in this patch which will repeat that mistake, without cleaning up the l10n results, from either this time or the last time we blew it.

Comment 14

[(not reading, please use [e-mail] instead)]

I'm a little confused.  phil, axel, can you guys clarify what needs to be done before we land?

Is the problem that if we don't rename the entities in extensions.properties, localizers won't see the change from http:// to https:// and not make it?  

What was the format change for 1.8?

http://lxr.mozilla.org/l10n/search?string=extensions.getMore

For that part of the fix, are you saying that we need to also go in and fix the http://addon.mozilla.org entries in mozilla/l10n

If the rest of the fix is good, and don't impact l10n, and is all that firefox needs, I think we should split it out and get that part into trunk and branch for the next release.

Comment 15

[Phil Ringnalda (:philor)]

You have two problems, and two branches, so, four problems:

extensions.properties on the trunk: if you land as-is, without changing the name, then just like with bug 350123, which changed to the current http URL as rather late l10n without a property name change, some locales will notice the change and follow suit, and others will not. If you land with a changed name, then the l10n tinderbox will tell them all very clearly that they need to remove the property extensions.getMoreExtensionsURL and add the property extensions.getMoreExtensionsSecureURL, and when they look at why the change and what the new en-US value is, they'll make the change.

extensions.properties on the 1.8.1 branch: hmm, I'm not sure, I've never been allowed to break a string freeze - Axel, does it work the same way, or does someone who wants a localized URL changed on a frozen branch need to fix all the locales themselves?

bookmarks.html on the trunk: make the change, and assuming that bookmarks.html still exists by the time Fx3 ships, chances are fair that trademark verification makes sure that URL is correct, probably, though you maybe need to find the wiki page for that, if it exists yet, and make sure it clearly says that the AMO link must be https. 

bookmarks.html on the branch: there's certainly no way of making that change evident to localizers, so I think you have to change them all.

Comment 16

[Robert Kaiser]

We can't change the string names easily anywhere:
On 1.8 branch, we break the branch localization freeze, which is a no-no in my eyes.
On trunk, we'd need to change all other apps that use EM at the same time, as every app defines those URLs through ther own (possibly localized) prefs.

The easies, and IMHO correct way to do this is to just grep for http AMO URLs in a full checkout of the L10n repository (on trunk and the releavant branch[es]) and check in a fix for those along with the en-US fix. This should catch all relevant strings and not cause more breakage than necessary.

Comment 17

[Phil Ringnalda (:philor)]

Attachment: l10n-trunk

No idea what or where the test harness for l10n is, so I have high hopes that someone else will pick up the "and a test, too" of comment 10.

Comment 18

[Phil Ringnalda (:philor)]

Comment on attachment 269597 [details] [diff] [review]
l10n-trunk

Hmm, all-locales != http://mxr.mozilla.org/l10n/source/ != http://lxr.mozilla.org/l10n/source/ so apparently I need to look elsewhere for a list of everything that actually needs to be changed.

Comment 19

[Phil Ringnalda (:philor)]

Attachment: l10n-trunk, take 2

A few more, then.

Comment 20

[Phil Ringnalda (:philor)]

Attachment: l10n-branch

Remind me to look for the nearest SEP shelter, next one of these that comes around, please.

Comment 21

[Axel Hecht [:Pike]]

So, to clarify, there's no need for an entity name change, I guess. There's no point to it - if we really care about AMO links being https, we need a test for this, and the test needs to be run and turn builds red. These tests are currently on Clint's table. We'll need both Clint and timr to sign up for this, which will probably require a test specification. Is comment 0 covering the requirements?

Comment 22

[Marek Stępień [:marcoos, inactive]]

(In reply to comment #21)
> So, to clarify, there's no need for an entity name change, I guess.

What is more, this is a change in localization files that doesn't actually need to be done by the localizers. I see no reason why we couldn't make sth like attachment 269628 [details] [diff] [review] for trunk as well. You don't need to speak Chinese to add the "s" to a zh-CN http URL, do you? 

Comment 23

[Phil Ringnalda (:philor)]

If we do a patch for trunk as well, we should probably call it attachment 269620 [details] [diff] [review] ;)

Comment 24

[(not reading, please use [e-mail] instead)]

Comment on attachment 269620 [details] [diff] [review]
l10n-trunk, take 2

phil, thanks for the patch.

since this is all l10n, axel's review should be enough.

Comment 25

[Mike Connor [:mconnor]]

pushing to M8, no l10n builds planned for M7 (a7), and the en-US version is fixed.

Comment 26

[(not reading, please use [e-mail] instead)]

> the en-US version is fixed.

Simon's fix was ever checked into the trunk.

Comment 27

[(not reading, please use [e-mail] instead)]

simon's fix (for en-US) landed on the trunk for M7, a=mconnor.

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.191; previous revision: 1.190
done
Checking in browser/components/places/tests/unit/bookmarks.preplaces.html;
/cvsroot/mozilla/browser/components/places/tests/unit/bookmarks.preplaces.html,v
  <--  bookmarks.preplaces.html
new revision: 1.5; previous revision: 1.4
done
Checking in browser/locales/en-US/profile/bookmarks.html;
/cvsroot/mozilla/browser/locales/en-US/profile/bookmarks.html,v  <--  bookmarks.
html
new revision: 1.16; previous revision: 1.15
done
Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.propertie
s;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.prop
erties,v  <--  extensions.properties
new revision: 1.35; previous revision: 1.34
done

Comment 28

[(not reading, please use [e-mail] instead)]

note, the l10n patches (for trunk and branch) are still pending review (and checkin)

Comment 29

[Mike Connor [:mconnor]]

-> Phil for the l10n bits, bumping to M9 since there's no l10n builds planned for M8

Axel, what's the holdup here?

Comment 30

[Axel Hecht [:Pike]]

We need an automated test for this. Marking it up to be included in minotaur.

To do that though, we need a clear specification. Is ensuring

toolkit - chrome/mozapps/extensions/extensions.properties
 - extensions.getMoreExtensionsURL
 - extensions.getMoreThemesURL

as well as a bookmarks test complete?

Comment 31

[Phil Ringnalda (:philor)]

If it's gated on a test tool I've never heard of, it's not my bug.

Comment 32

[Axel Hecht [:Pike]]

It is gated on a specification on what we require to not regress this bug.

I explicitly CCed clint weeks ago on the actual implementation.

Comment 33

[cmtalbert]

The Minotaur test tool is mine (still in development).  I'll take care of integrating the test into there.  I'm not sure if that means I should take the review or not though.

Comment 34

[Axel Hecht [:Pike]]

Clint, nothing for you to do here right now. 

We need someone to actually make a claim that those preferences I listed are all we need to test, in addition to bookmarks.

Comment 35

[Axel Hecht [:Pike]]

Comment on attachment 269620 [details] [diff] [review]
l10n-trunk, take 2

r- due to lack of testing spec.

Comment 36

[Robert Strong (they/them - no direct email)]

It appears that using https causes a significant slowdown in opening urls via contentAreaUtils.js openURL (see bug 395425).

Comment 37

[Mike Connor [:mconnor]]

moving out bugs that don't need to block b1

Comment 38

[Mike Connor [:mconnor]]

What are we waiting on here? We need this fixed by b3 at the latest...

Comment 39

[Phil Ringnalda (:philor)]

mconnor: oh, okay, I'll stop trying to trade this information for a review elsewhere.

It's waiting on exactly one thing: you to type a comment saying "Axel, sorry for the confusion - we need just one l10n test, that the bookmark in bookmarks.html is exactly https://ab-cd.add-ons.mozilla.com/ab-CD/firefox/bookmarks/ including the https."

Nothing shipping but Sunbird is using the extensions.properties URLs, trunk or branch, and them not using them is bug 405039, and removing them so nobody will accidently use them is bug 405068, so other than telling Axel what you need tested, the only thing left to do is decide whether to assign this to ctalbert for that test, or put it to sleep and start fresh with a new bug wherever Minotaur bugs go.

Comment 40

[Phil Ringnalda (:philor)]

^

Comment 41

[Mike Connor [:mconnor]]

Axel, what Phil said is correct.  We need this for final, full stop.

Comment 42

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

Instead of blocking on Axel, I might humbly propose just fixing the prefs (which get formatted by the URI formatter automatically) and forgetting about the default bookmark.

More better sooner. :)

Comment 43

[Phil Ringnalda (:philor)]

Um, what's blocked on Axel, and what prefs? Axel filed bug 422665 (with backwards dependency) for tests for the bookmark link, and as far as I know I did everything that needed to be done with prefs and properties last fall.

Much as I like the sound of "forgetting about the default bookmark" I fear you don't mean "it's no real man-in-the-middle risk because nobody uses it, so let's take it out and have Clint write a test to ensure every locale removes it."

And if you don't mean to take it out, then you're saying it's fine for users of (the ones that actually ship of) ne-NP, gu-IN, ga-IE, de, sq, eu, sk, hi-IN, mr, el, tr, lv, en-GB, cs, pl, da, ml, fa, pt-PT, ta, nb-NO, es-ES, bn-IN, zh-CN, ku, ro, nn-NO, te, kn, pa-IN, sr, lt, rw, si, it, and fi to be exposed to man-in-the-middle attacks because it's too hard to tell them that they need to make sure their Get Bookmark Add-ons bookmark is https://ab-cd.add-ons.mozilla.com/ab-CD/firefox/bookmarks/ with their locale code and with the http_s_. I can't help thinking we ought to be able to come up with *some* way of letting them know that.

Comment 44

[Axel Hecht [:Pike]]

I have a patch for this in the works, ETA today.

Comment 45

[Axel Hecht [:Pike]]

The affected locales in the following patch are

be
cs
da
de
el
en-GB
es-ES
eu
fa
fi
ga-IE
gu-IN
hi-IN
it
kn
ku
lt
lv
mr
nb-NO
ne-NP
pa-IN
pl
pt-PT
ro
rw
si
sq
sr
te
tr
zh-CN

CCing the respective owners. I'll be checking in the patch without further ado, if you're contributing to one of the mentioned localizations, please update browser/profile/bookmarks.html in your working copy, and make sure that you don't revert the change to the amo link.

Comment 46

[Axel Hecht [:Pike]]

Attachment: fix amo links to be https

Comment 47

[Axel Hecht [:Pike]]

Checking in be/browser/profile/bookmarks.html;
/l10n/l10n/be/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.5; previous revision: 1.4
done
Checking in cs/browser/profile/bookmarks.html;
/l10n/l10n/cs/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.5; previous revision: 1.4
done
Checking in da/browser/profile/bookmarks.html;
/l10n/l10n/da/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.5; previous revision: 1.4
done
Checking in de/browser/profile/bookmarks.html;
/l10n/l10n/de/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.7; previous revision: 1.6
done
Checking in el/browser/profile/bookmarks.html;
/l10n/l10n/el/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.8; previous revision: 1.7
done
Checking in en-GB/browser/profile/bookmarks.html;
/l10n/l10n/en-GB/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.5; previous revision: 1.4
done
Checking in es-ES/browser/profile/bookmarks.html;
/l10n/l10n/es-ES/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.3; previous revision: 1.2
done
Checking in eu/browser/profile/bookmarks.html;
/l10n/l10n/eu/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.11; previous revision: 1.10
done
Checking in fa/browser/profile/bookmarks.html;
/l10n/l10n/fa/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.4; previous revision: 1.3
done
Checking in fi/browser/profile/bookmarks.html;
/l10n/l10n/fi/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.4; previous revision: 1.3
done
Checking in ga-IE/browser/profile/bookmarks.html;
/l10n/l10n/ga-IE/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.6; previous revision: 1.5
done
Checking in gu-IN/browser/profile/bookmarks.html;
/l10n/l10n/gu-IN/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.7; previous revision: 1.6
done
Checking in hi-IN/browser/profile/bookmarks.html;
/l10n/l10n/hi-IN/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.5; previous revision: 1.4
done
Checking in it/browser/profile/bookmarks.html;
/l10n/l10n/it/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.13; previous revision: 1.12
done
Checking in kn/browser/profile/bookmarks.html;
/l10n/l10n/kn/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.2; previous revision: 1.1
done
Checking in ku/browser/profile/bookmarks.html;
/l10n/l10n/ku/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.9; previous revision: 1.8
done
Checking in lt/browser/profile/bookmarks.html;
/l10n/l10n/lt/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.4; previous revision: 1.3
done
Checking in lv/browser/profile/bookmarks.html;
/l10n/l10n/lv/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.4; previous revision: 1.3
done
Checking in mr/browser/profile/bookmarks.html;
/l10n/l10n/mr/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.2; previous revision: 1.1
done
Checking in nb-NO/browser/profile/bookmarks.html;
/l10n/l10n/nb-NO/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.7; previous revision: 1.6
done
Checking in ne-NP/browser/profile/bookmarks.html;
/l10n/l10n/ne-NP/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.3; previous revision: 1.2
done
Checking in pa-IN/browser/profile/bookmarks.html;
/l10n/l10n/pa-IN/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.4; previous revision: 1.3
done
Checking in pl/browser/profile/bookmarks.html;
/l10n/l10n/pl/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.20; previous revision: 1.19
done
Checking in pt-PT/browser/profile/bookmarks.html;
/l10n/l10n/pt-PT/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.4; previous revision: 1.3
done
Checking in ro/browser/profile/bookmarks.html;
/l10n/l10n/ro/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.7; previous revision: 1.6
done
Checking in rw/browser/profile/bookmarks.html;
/l10n/l10n/rw/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.2; previous revision: 1.1
done
Checking in si/browser/profile/bookmarks.html;
/l10n/l10n/si/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.2; previous revision: 1.1
done
Checking in sq/browser/profile/bookmarks.html;
/l10n/l10n/sq/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.5; previous revision: 1.4
done
Checking in sr/browser/profile/bookmarks.html;
/l10n/l10n/sr/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.4; previous revision: 1.3
done
Checking in te/browser/profile/bookmarks.html;
/l10n/l10n/te/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.2; previous revision: 1.1
done
Checking in tr/browser/profile/bookmarks.html;
/l10n/l10n/tr/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.7; previous revision: 1.6
done
Checking in zh-CN/browser/profile/bookmarks.html;
/l10n/l10n/zh-CN/browser/profile/bookmarks.html,v  <--  bookmarks.html
new revision: 1.10; previous revision: 1.9
done

Comment 48

[Axel Hecht [:Pike]]

Marking FIXED, this is what we need to do for Fx3. I have a test script that can be run locally, currently only checking bookmarks, though. I'm not sure how I'll set this up to be run automatically. Same goes for Minotaur, to add runtime testing for this.

This should be mostly fixed for incubator, i.e., new locales on Firefox 2, too, only getMoreFoo in toolkit/chrome/mozapps/extensions/extensions.properties is missing. Not fixed for branch locales yet. Not sure how important that is, too, given that Fx3 will fix that for a huge chunk of our users soon.

Comment 49

[Guillermo López :willyaranda (probably SLOW response)]

Thanks for your job Axel.

Verified and updated my copy of trunk of es-ES . Thanks

Comment 50

[Phil Ringnalda (:philor)]

getMoreFoo on the branch only mattered for Sunbird (Thunderbird and Firefox 2 shipped with https URIs in prefs, not using the .properties one), and Sunbird's no longer using it, so unless you think a locale overrode the https URI in prefs with a chrome URI in l10n-prefs (spectacularly unlikely), the extensions.properties URIs are totally unused.

Comment 51

[Tony Chung [:tchung]]

Confirming verification of es-ES from comment #49 on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; es-ES; rv:1.9b5pre) Gecko/2008032504 Minefield/3.0b5pre.  

Also verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032504 Minefield/3.0b5pre