695631 - Making Google search more secure

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Eiichi]

Google says "Making search more secure".
http://googleblog.blogspot.com/2011/10/making-search-more-secure.html

Now that we can use https://www.google.com/, so make it default in WebSearchEngines.plist.

Comment 1

[philippe (part-time)]

That sounds like a good idea.
I wonder though, how would that be applied for people with an existing profile ?

While doing this, we should also update the google search bookmark in the default bookmark file.

Comment 2

[Stuart Morgan]

We should definitely at least change the default for new profiles for 2.1.

It would also be good to do migration on existing engines; should be easy to add fixup code.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

What about l10n?

Policy for WebSearchEngines.plist is that the engine names get localized (so "Google Bilder" for "Google Images"), but not the URLs, but it's possible to customize the list with our permission.

Bookmarks, on the other hand, are not supposed to localize the name of the "Google" bookmarks (both of them), nor the URLs, but again it's possible to customize the list with our permission.

We may as well change the default URLs in both WebSearchEngines.plist and bookmarks.plist since we're going to have to break l10n freeze to do the former; I care less about migration code for the bookmarks (but if there were a good, simple way to do it, that would be awesome).

Comment 4

[Samuel Sidler (:ss)]

Given the majority of our users are en-US, I'd be okay with just changing this for en-US and letting l10n change it in 2.1.1 if they've already finished their localizations.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Thinking it over this afternoon, I don't think this will be a big deal for l10n to fix in their plists, particularly if we finalize the changes quickly so they'll have plenty of time.  (I mailed the list this afternoon as a heads-up.)

It might make Stuart's migration code more difficult; it'd have to be strictly URL matching rather than being able to look at the engine name key and then replace the associated URL key, but I don't think that's insurmountable.

(Given that non-google.com Google sites seem not to have the SSL version, it seems like a good thing in retrospect that we set the l01n policies to require these items to use www.google.com ;) )

Comment 6

[waverider]

We open "https://www.google.com" with Camino 2.1b, google check "intl.accept_languages" and show results page with primary language.

Camino contain the code that adjusting intl.accpet_languages to Mac OS X's primary language.
languages are ja,en-us,en,fr,de,es,it,pt,pt-pt,nl.
If user set Mac OS X's primary language to other language, google show English page, maybe.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to waverider from comment #6)
> We open "https://www.google.com" with Camino 2.1b, google check
> "intl.accept_languages" and show results page with primary language.

That's the reason why we set l10n policy to use www.google.com everywhere, rather than www.google.$country.

To be clear, the only impact to localizers from this bug is:

* Localizers will need to s/http/https/g the Google URLs in both WebSearchEngines.plist and bookmarks.plist

Impact to Camino developers from this bug is:

* Migration code won't be able to use the engine name to find the URLs to update, since engine names can be localized (and also since users can rename the engines, anyway)

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Oh, hrm.

It looks like we'll have to change the URL for Google Images search, because the existing URL, set to https, redirects to https://www.google.com/ and throws away the search.

I think https://www.google.com/search?tbm=isch&ie=UTF-8&oe=UTF-8&q=%s is what will work for the plist.

Comment 9

[Stuart Morgan]

Attachment: fix

Changes defaults to the URLs above, and adds migration code for existing users based on URLs. Adds a version to make sure we don't do this repeatedly (e.g., if someone wants http).

Localizers should also add:
       <key>BuiltInEngineSetVersion</key>
       <integer>1</integer>
to the plist, as in the patch. Worst case scenario, localizers don't do anything and the searches are wrong for the first launch then get fixed on the second. If they don't add the version but do change the URLs, there will just be a bit of unnecessary work on the second launch.

I ripped out the old search engine migration code while I was there, since it's been in since 1.6 (and I don't anticipate a lot of straight 1.5->2.1 happening).

Comment 10

[Stuart Morgan]

Attachment: v2

Adds site search, which I spaced on somehow. Also changes the Google search properties we have in bookmarks that support https (web, maps, news)

Comment 11

[Stuart Morgan]

Attachment: v3

Now with less leaking and forgetting how dictionaries work; Smokey FTW.

God knows what else is wrong with this. I'll look at it again in the morning :)

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 568912 [details] [diff] [review]
v3

[5:51pm] smorgan: sauron: hopefully the new version makes more sense

Yes, indeed it does (once I realized that urlMap was |dictionaryWithObjectsAndKeys:| instead of the nonexistent |dictionaryWithKeysAndObjects:| I expected) :P

I can't find anything else, and it works according to my simple testing, so r=ardissone

(I wish you'd used more than 3 lines of context, though: http://wiki.caminobrowser.org/Development:Coding#Proper_patch_format , http://wiki.caminobrowser.org/Development:Building:Mozilla_1.9.2_Branch#Setting_up_your_Hg_Environment )

Comment 13

[Stuart Morgan]

Looks sane in the morning light; landed at http://hg.mozilla.org/camino/rev/bc0269a14727

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The downside to this fix is that if you don't open a result in a new foo, then you get the secure->insecure warning.

Comment 15

[philippe (part-time)]

(In reply to Smokey Ardisson (back-ish; no bugmail - do not email) from comment #14)
> The downside to this fix is that if you don't open a result in a new foo,
> then you get the secure->insecure warning.

But that only happens if you have the 'Moving from a secure to an insecure page' pref checked (off by default here, with TrC).

Comment 16

[Stuart Morgan]

I didn't even realize we still had UI for that pref.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Ah, I didn't realize we had it off by default now.  Well, I like it on so that I can tell when stupid sites try and break my SSL path through them ;-)  Sadly, this is still too prevalent :-(

(In reply to Stuart Morgan from comment #16)
> I didn't even realize we still had UI for that pref.

We're down to just that and mixed content.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Ugh, I officially fail at QA and reviewing :(  Luckily, I seem to do a bit better when writing documentation :P  

(Note to self: when reviewing things that include migration code, try testing against copies of all those *ancient* profiles you have hanging around, not just ones that are only 1.5 years old.)

The good news is I upgraded to 2.1 the other day and happened to test searching today.

The bad news is I got sent to http://www.google.com instead of https://www.google.com.

Anyone who created their Camino profile before 1.5.1 (when bug 384723 landed) is going to have one or more default search engines URLs that don't match the URLs that we ship today, and thus won't be migrated to the new https:// URLs.

Looking through the diffs to SearchURLList.plist (this bug was the first change to WebSearchEngines.plist since its creation), here are the *additional* URLs-for-engines that we need to support migrating to the new values:

==Google==
http://www.google.com/search?q=%s
http://www.google.com/search?ie=UTF-8&oe=UTF-8&q=%s
http://www.google.com/search?q=%s&sourceid=mozilla2&ie=utf-8&oe=utf-8

==Google Images==
http://images.google.com/images?q=%s

==Search this Site==
http://www.google.com/search?q=%s site:%d

I assume the fix here is just another set of |dictionaryWithObjectsAndKeys:| mappings, plus revving the BuiltInEngineSetVersion / kCurrentBuiltInEngineSetVersion / |if (oldVersion < 1)| values to 2 to (re)migrate anyone using 2.1rc1 / 2.1.1pre or older to the new set?

Obviously, I'd like to get this for rc2 (but I'm going to sleep now, so I can't write the patch).  (Gecko's pushed their fixes but hasn't tagged yet; I'm guessing they want to QA the forthcoming nightlies before respinning their RC2, but I'm not yet in a super rush to build, given all the other things I'm juggling.)

Comment 19

[Stuart Morgan]

Ah, I didn't realized we'd changed these over time. Yes, just adding more mappings to the dictionary (different keys, same values) and revving the version should work. If you write the patch, keep in mind that you need to s/&/&/ everywhere because the strings aren't SGML-escaped any more once they get to code.

Comment 20

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Follow-up: migrate older engine URLs, too

Attached is the patch to migrate the old URLs, too.  I fudged the comment about which version upgraded to HTTPS and just made version 2 encompass the added and existing URL objects-and-keys, but for this situation I don't think it matters/is worth splitting out into a v1 and v2.  People using 2.1rc1 and recent nightlies will have a bit of extra match-checking done this once, but the vast majority of our users will jump from "version 0" straight to version 2 in one step and be done with it.

Also the comments for the URL sets probably aren't that great, so if you'd like them improved, I'm definitely open to suggestions.

I tested this patch by pulling all 4 revs of SearchURLList.plist from Hg (the last rev of SearchURLList.plist is URL-equivalent to the first rev of WebSearchEngines.plist), running them through 2.0.x to turn them into WebSearchEngines.plist, and then upgrading each of those in a build with this patch.  The result is 4 identical plists, all at version 2, all upgraded to HTTPS.  I also ran a copy of my own WebSearchEngines.plist through this patch, and verified that the default engines I still had were all upgraded to HTTPS.

I'd like to spin rc2 Friday evening my time if possible, so if you can get to this review Friday your time, that would be great!

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Oh, and I also mento'd WebSearchEngines.plist to put the keys in alpha order; I'd noticed I'd missed that only once you'd landed (review-fail again).

Comment 22

[Stuart Morgan]

Comment on attachment 571756 [details] [diff] [review]
Follow-up: migrate older engine URLs, too

sr=smorgan

Thanks for cleaning this up. Looks like we should have come up with a better approach for these URLs a long time ago!

Comment 23

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/1f5df9bb682e and CAMINO_2_1_MINIBRANCH http://hg.mozilla.org/camino/rev/7c1c60a18648

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to Stuart Morgan from comment #22)
> Looks like we should have come up with a better
> approach for these URLs a long time ago!

Despite having made one of the URL changes, I didn't realize we never did any migration, either (other than the whole-plist-changed-names one).

I mailed l10n about the extra version-bump part of this last patch, too (though as noted before, it'll just cause a little bit of unnecessary work if the version is still at 1).