Closed Bug 1162569 Opened 9 years ago Closed 9 years ago

default engine files should be in the omni.ja file

Categories

(Firefox :: Search, defect)

defect
Not set
normal
Points:
13

Tracking

()

VERIFIED FIXED
Firefox 41
Iteration:
41.1 - May 25
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: florian, Assigned: florian)

References

Details

(Whiteboard: [fxsearch][hijacking])

Attachments

(2 files, 2 obsolete files)

Telemetry data gave us some evidence that in lots of profiles the default engine files in the application directory don't match the list of engines we ship.

Instead of tracking these individual files, we should just have the default search engines be part of the omni.ja file that is updated to a known state for each Firefox update.
Attached patch WIP (obsolete) — Splinter Review
I think this works, but I intend to test it some more next week.

Mike, I'm requesting review from you on the Makefile.in changes. I'm wondering if there's anything I should pay particular attention to in order to minimize chances of breaking l10n repackaging.


Gavin, only requesting feedback from now on the search service changes, because I feel this probably wants some tests, although I'm not sure what needs testing exactly. The goal of my changes in the search service is to avoid loading engines from both a jar file and a directory in the application folder.

I additionally made the code reading the relevant preferences ignore user-set values, to prevent the user from shooting himself in the foot with about:config, or some lines added in user.js in the profile.
Using the default pref branch shouldn't prevent customizations from add-ons or other applications using engine files shipped in the application folder.
Attachment #8603024 - Flags: review?(mh+mozilla)
Attachment #8603024 - Flags: feedback?(gavin.sharp)
connor probably has opinions, CCing.

I'm surprised that this looks different to what mobile/locales/Makefile.in does. Is that intentional? I would hope that we could do both.

On the approach, if malicious software can fiddle with our plugins, can't they fiddle with the default pref, too?
Flags: needinfo?(mconnor)
(In reply to Axel Hecht [:Pike] from comment #2)
> connor probably has opinions, CCing.
> 
> I'm surprised that this looks different to what mobile/locales/Makefile.in
> does. Is that intentional?

Yes and no. I tried to keep the code changes to this Makefile minimal. mobile/locales/Makefile.in and browser/locales/Makefile.in already had some different code doing more or less the same thing, except the browser/ version seemed to have been more recently/frequently updated, eg with changes like bug 1111607.

> On the approach, if malicious software can fiddle with our plugins, can't
> they fiddle with the default pref, too?

The default pref files are either stored in omni.ja (with currently the exception of channel-prefs.js, but I'm hoping that's something that can be fixed eventually), or in extensions, that in the near future will be signed.
Comment on attachment 8603024 [details] [diff] [review]
WIP

Review of attachment 8603024 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/Makefile.in
@@ +108,5 @@
> +	$(call py_action,jar_maker,\
> +          $(QUIET) -j $(FINAL_TARGET)/chrome \
> +          -s $(SEARCHPLUGINS_PATH) \
> +          $(MAKE_JARS_FLAGS) $(search-jar))
> +

Thanks to bug 846740, this might not cause problems. That being said, I'd prefer if we could avoid the same horror show as mobile/android/locale if we can. Specifically, I landed a change last week that allows wildcards in jar.mn[1]. So if you can stage the en-us+l10n search plugins in some directory, you shouldn't need the temporary jar.mn and custom jar_maker command... Just make sure the staging happens before the jar_maker command in rules.mk.

1. https://hg.mozilla.org/mozilla-central/rev/07c6eccd05c2de8e17e30256661ddc0d301c09c5
Attachment #8603024 - Flags: review?(mh+mozilla) → feedback-
Attached patch Patch v2Splinter Review
- Addressed comment 4

- Fixed the tests in toolkit/components/search/tests/xpcshell.
-- test_json_cache.js didn't work because the detection of whether the cache is outdated failed (always returned true) for engines in flat chrome; I fixed that in nsSearchService.js.
-- Added some more xpcshell tests.
-- Verified that the tests in browser/components/search/test pass locally.
-- Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=195f81502295 (I haven't seen results yet).

- Verified that this works for a French repackaged Mac build.
Attachment #8603024 - Attachment is obsolete: true
Attachment #8603024 - Flags: feedback?(gavin.sharp)
Attachment #8604710 - Flags: review?(mh+mozilla)
Attachment #8604710 - Flags: review?(gavin.sharp)
* Pref signing is not a panacea against abuse of prefs. Unless we're going to kill default overrides, I would like to make this a build-time flag.

* Glad to see this version doesn't break all non-default plugins!  Feels a tad hacky to just skip the directory in the loop.

* We're considering disallowing overrides of search defaults from add-ons, but we'll still need distribution.ini to work for custom bundles.

* Overall, can we document, in one place, how search plugins are loaded and how we prioritize loads?

* While we're mucking with load order/approach, can we ensure that we have documented, in a single place, the expected order for loads?  I think the old order is hijacking-prone (just stick something called "Google" in an add-on or profile, and it'll override the default).

http://mxr.mozilla.org/mozilla-central/source/browser/components/dirprovider/DirectoryProvider.cpp#219 covers the current order. I believe app shipped should trump everything except for distribution plugins.
Flags: needinfo?(mconnor)
Mconnor, florian just left for a few days pto, so your answer might have to wait :)
Comment on attachment 8604710 [details] [diff] [review]
Patch v2

Just some high-level feedback:

- I agree that we should remove the loadFromJars/jarURIs prefs and just make that the default behavior / hardcode the chrome URI in the code.

- It looks to me like this eliminates the loading of searchplugins from <appdir>/searchplugins entirely, which is good. We should further rip out all of the code supporting that (e.g. the relevant directory service definitions, search service code) too, eventually. Doesn't necessarily need to be bundled with this.

Otherwise looks good, but I did not dive into the logic changes. I'm hoping Mark can help with a more detailed code review of the search service changes.
Attachment #8604710 - Flags: review?(markh)
Attachment #8604710 - Flags: review?(gavin.sharp)
Attachment #8604710 - Flags: feedback+
(In reply to Mike Connor [:mconnor] from comment #6)

> * Overall, can we document, in one place, how search plugins are loaded and
> how we prioritize loads?
> 
> * While we're mucking with load order/approach, can we ensure that we have
> documented, in a single place, the expected order for loads?  I think the
> old order is hijacking-prone (just stick something called "Google" in an
> add-on or profile, and it'll override the default).
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/dirprovider/
> DirectoryProvider.cpp#219 covers the current order. I believe app shipped
> should trump everything except for distribution plugins.

The current order also surprised me, so I agree it would be good to change it. I would however prefer if we could do this in a different bug, as the patch here is already larger than I wanted.
Flags: needinfo?(florian)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #8)

> - I agree that we should remove the loadFromJars/jarURIs prefs and just make
> that the default behavior / hardcode the chrome URI in the code.

I tried to avoid affecting non-Firefox-desktop applications to limit risk, as I have some hope we could uplift the patch at least to aurora.

> - It looks to me like this eliminates the loading of searchplugins from
> <appdir>/searchplugins entirely, which is good. We should further rip out
> all of the code supporting that (e.g. the relevant directory service
> definitions, search service code) too, eventually. Doesn't necessarily need
> to be bundled with this.

Looking forward to this simplification :-). Doing it in a separate patch/bug sounds good to me, as I think changes to the directory service will need to ride the trains and be documented.

Thanks for the feedback! And btw, Greg is correct, I'm away for the rest of the week.
(In reply to Florian Quèze [:florian] [:flo] from comment #10)
> (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #8)
> 
> > - I agree that we should remove the loadFromJars/jarURIs prefs and just make
> > that the default behavior / hardcode the chrome URI in the code.
> 
> I tried to avoid affecting non-Firefox-desktop applications to limit risk,
> as I have some hope we could uplift the patch at least to aurora.

Also, hardcoding to chrome://browser/... in nsSearchService.js would feel wrong.
Comment on attachment 8604710 [details] [diff] [review]
Patch v2

Review of attachment 8604710 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the build parts.

I'll note that I agree with Florian that a chrome://browser/ url should not appear in toolkit/, /but/ it doesn't have to be set with a pref. It could be harcoded to e.g. resource://search-engines/, which the app can then set to the right path with a resource entry in some chrome.manifest.
Attachment #8604710 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8604710 [details] [diff] [review]
Patch v2

Review of attachment 8604710 [details] [diff] [review]:
-----------------------------------------------------------------

/me wipes brow...

The search changes looks fine to me. I didn't actually test it and didn't look at the build changes but see no significant issues.

::: toolkit/components/search/tests/xpcshell/head_search.js
@@ +73,5 @@
>  
>  /**
> + * Load engines from chrome://testsearchplugin/locale/searchplugins/
> + */
> +function loadJarEngines(loadFromJars = true)

ISTM this name and comment is misleading - it looks like it configures things such that the search service will load from jar files rather than actually loading anything.

::: toolkit/components/search/tests/xpcshell/test_645970.js
@@ +11,5 @@
>    updateAppInfo();
>  
>    do_load_manifest("data/chrome.manifest");
>  
> +  loadJarEngines();

It seems another reasonable test (here or somewhere else) might be to use Services.pref.setBoolPref("browser.search.loadFromJars", false) and make sure it is ignored.

::: toolkit/components/search/tests/xpcshell/test_json_cache.js
@@ +27,5 @@
>    }
>    return _dirSvc.get(aKey, aIFace || Ci.nsIFile);
>  }
>  
> +function makeURI(uri)

brace should be on this line

@@ +61,5 @@
> +  let defaultBranch = Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF);
> +
> +  let loadFromJARs = false;
> +  try {
> +    loadFromJARs = defaultBranch.getBoolPref("loadFromJars");

it looks like we should be arranging for this test to run with the pref both true and false?
Attachment #8604710 - Flags: review?(mh+mozilla)
Attachment #8604710 - Flags: review?(markh)
Attachment #8604710 - Flags: review+
Did you mean to re-ask me for review, or is it the result of an old splinter tab submitting old state?
Comment on attachment 8604710 [details] [diff] [review]
Patch v2

oops, it was exactly that - sorry for the noise!
Attachment #8604710 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Connor [:mconnor] from comment #6)

> * We're considering disallowing overrides of search defaults from add-ons,
> but we'll still need distribution.ini to work for custom bundles.

> http://mxr.mozilla.org/mozilla-central/source/browser/components/dirprovider/
> DirectoryProvider.cpp#219 covers the current order. I believe app shipped
> should trump everything except for distribution plugins.

I'm a bit confused about the meaning of this, and how it relates to bug 1144127. Could you please expend a bit on which kind of distribution bundle we still want to support and which we no longer want to support?
Flags: needinfo?(mconnor)
(In reply to Mark Hammond [:markh] from comment #13)
> Comment on attachment 8604710 [details] [diff] [review]

> @@ +61,5 @@
> > +  let defaultBranch = Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF);
> > +
> > +  let loadFromJARs = false;
> > +  try {
> > +    loadFromJARs = defaultBranch.getBoolPref("loadFromJars");
> 
> it looks like we should be arranging for this test to run with the pref both
> true and false?

I don't think this matters much in practice, because the only engine this test actually loads is one it installs itself in the user profile. The part of the test that depends on the value of the pref is just collecting the list of directories where we would normally load engines from, so that fake data about them can be written to the cache file, to ensure only the engine provided by the test will be loaded. Testing with the other value would actually just test another part of the test, nor any code we ship.

I addressed your other comments, thanks for the review!
The way to verify this is to:
- verify that the browser/searchplugins directory no longer exists in the application directory after the fix.
- verify that if that folder is manually recreated, and a .xml engine file is dropped there, this engine file won't be read by Firefox.
Points: --- → 13
Flags: qe-verify+
Flags: firefox-backlog+
Status: NEW → ASSIGNED
Iteration: --- → 41.1 - May 25
(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> (In reply to Mike Connor [:mconnor] from comment #6)
> 
> > * We're considering disallowing overrides of search defaults from add-ons,
> > but we'll still need distribution.ini to work for custom bundles.
> 
> > http://mxr.mozilla.org/mozilla-central/source/browser/components/dirprovider/
> > DirectoryProvider.cpp#219 covers the current order. I believe app shipped
> > should trump everything except for distribution plugins.
> 
> I'm a bit confused about the meaning of this, and how it relates to bug
> 1144127. Could you please expend a bit on which kind of distribution bundle
> we still want to support and which we no longer want to support?

No relation to to that at all.  Distribution.ini was the wrong term, sorry.

We ship custom builds with a number of partners.  Any additional/modified search plugins are located in /distribution/searchplugins/common or /distribution/searchplugins/<ab-CD> for locale-specific plugins.  Those must continue to work, and must override app defaults in all cases.
Flags: needinfo?(mconnor)
https://hg.mozilla.org/mozilla-central/rev/8a177d785dd5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Reading through this bug I think the intention is not to affect SeaMonkey, right?
(In reply to Philip Chee from comment #22)
> Reading through this bug I think the intention is not to affect SeaMonkey,
> right?

I expect only Firefox desktop to be affected by this patch.
Depends on: 1169459
QA Contact: petruta.rasa
Attached file mdew.xml
I've verified this issue using latest Nightly 41.0a1 2015-06-02 under Win 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5 by following the steps from comment 19 with existing and newly installed Nightly builds. 

Under Windows, I also unpacked omni.ja, added a search engine and then packed the archive back. The search engine was added to one-off search engine list.

Florian, please review the potential issues found:

*Win: the Nightly build that I frequently use and update still has the "searchplugins" folder and it contains the mdew.xml file (attached)

*OSX: If I install the latest Nightly and before opening it, I add searchplugins folder and a search engine xml inside it, I wont be able to open the build, the damaged file error with option to send to trash is shown.
This wont happen if I add the .xml file after the build was opened at least once.
Flags: needinfo?(florian)
(In reply to Petruta Rasa [QA] [:petruta] from comment #24)

> I've verified this issue using latest Nightly 41.0a1 2015-06-02 under Win 7
> 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5 by following the steps from
> comment 19 with existing and newly installed Nightly builds. 

Thanks!

> Under Windows, I also unpacked omni.ja, added a search engine and then
> packed the archive back. The search engine was added to one-off search
> engine list.

For this engine you added in omni.ja to be found, you also had to add it in the list.txt file, right?

> *Win: the Nightly build that I frequently use and update still has the
> "searchplugins" folder and it contains the mdew.xml file (attached)

This is a little bit unfortunate, but I don't think it will cause any issue for users. I guess the removed-files (especially http://mxr.mozilla.org/mozilla-central/source/browser/installer/removed-files.in#125) is now only taken into account when re-installing using the install wizard.

> *OSX: If I install the latest Nightly and before opening it, I add
> searchplugins folder and a search engine xml inside it, I wont be able to
> open the build, the damaged file error with option to send to trash is shown.
> This wont happen if I add the .xml file after the build was opened at least
> once.

This is likely due to signing of the build. This probably affects neither updates nor clean installs, so I would guess it can only happen if a user downloads and installs Firefox but never starts it, then has search plugin file added by some hijacking script, and then try to use Firefox. I don't expect this to _actually_ happen.
Flags: needinfo?(florian)
Whiteboard: [fxsearch][hijacking]
(In reply to Florian Quèze [:florian] [:flo] from comment #25)
> (In reply to Petruta Rasa [QA] [:petruta] from comment #24)
> > Under Windows, I also unpacked omni.ja, added a search engine and then
> > packed the archive back. The search engine was added to one-off search
> > engine list.
> 
> For this engine you added in omni.ja to be found, you also had to add it in
> the list.txt file, right?
Yes, that's right. 

> > *Win: the Nightly build that I frequently use and update still has the
> > "searchplugins" folder and it contains the mdew.xml file (attached)
> 
> This is a little bit unfortunate, but I don't think it will cause any issue
> for users. I guess the removed-files (especially
> http://mxr.mozilla.org/mozilla-central/source/browser/installer/removed-
> files.in#125) is now only taken into account when re-installing using the
> install wizard.
I could perform searches and everything seemed normal.

> > *OSX: If I install the latest Nightly and before opening it, I add
> > searchplugins folder and a search engine xml inside it, I wont be able to
> > open the build, the damaged file error with option to send to trash is shown.
> > This wont happen if I add the .xml file after the build was opened at least
> > once.
> 
> This is likely due to signing of the build. This probably affects neither
> updates nor clean installs, so I would guess it can only happen if a user
> downloads and installs Firefox but never starts it, then has search plugin
> file added by some hijacking script, and then try to use Firefox. I don't
> expect this to _actually_ happen.
Agree this is an edge case since only happened and modifying the build before opening it.

Marking as verified in this case, thanks for your answers.
Status: RESOLVED → VERIFIED
(In reply to Florian Quèze [:florian] [:flo] from comment #25)
> This is a little bit unfortunate, but I don't think it will cause any issue
> for users. I guess the removed-files (especially
> http://mxr.mozilla.org/mozilla-central/source/browser/installer/removed-
> files.in#125) is now only taken into account when re-installing using the
> install wizard.

This shouldn't be the case, and the behavior Petruta describes shouldn't happen (updates should clear the folder). Can you follow up with nthomas/rstrong about this?
Depends on: 1173712
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #27)

> This shouldn't be the case, and the behavior Petruta describes shouldn't
> happen (updates should clear the folder). Can you follow up with
> nthomas/rstrong about this?

Sure, I filed bug 1173712 to figure this out.
Comment on attachment 8604710 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: search hijacking mitigation
[User impact if declined]: third party software can ruin the search experience by messing with plain text files in the application folder.
[Describe test coverage new/current, TreeHerder]: xpcshell tests included in the patch, the patch baked on Nightlies for almost a month, and QA verified.
[Risks and why]: high enough that I don't want to request beta uplift, but given the testing done, acceptable for aurora in my opinion.
[String/UUID change made/needed]: none.
Attachment #8604710 - Flags: approval-mozilla-aurora?
Comment on attachment 8604710 [details] [diff] [review]
Patch v2

Improve the UX of the search, taking it.
Attachment #8604710 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed using Firefox 40 beta 1 build under Win 7 x64, Ubuntu 14.04 LTS x64 and Mac OS X 10.9.5.
It does not protect from %userprofile%/searchplugins

For example, malware can create %userprofile%/searchplugins/google.xml or %userprofile%/searchplugins/blablabla.xml which contains

<ShortName>Google </ShortName>

(with space), and just hide original Google search via "[app]/google.xml":{"hidden":true}" in search-metadata.json
(In reply to dartraiden from comment #33)

Thanks for your thougths about this. Note that the change in this bug alone isn't meant to solve the search hijacking problem; we totally expect malware to adapt, and we'll keep an eye on what happens.

> It does not protect from %userprofile%/searchplugins

This is where engines get installed when users install them using the Open Search discovery mechanism, so I don't think we have any plan to stop reading plain xml files from that folder.

> <ShortName>Google </ShortName>
> 
> (with space)

We should maybe trim leading/trailing whitespace before comparing engine names when we detect duplicates and skip loading engines with conflicting names.

> just hide original Google search via
> "[app]/google.xml":{"hidden":true}" in search-metadata.json

This seems nastier, someday we may end up signing that whole file.
(In reply to Florian Quèze [:florian] [:flo] from comment #34)
> > <ShortName>Google </ShortName>
> > 
> > (with space)
> 
> We should maybe trim leading/trailing whitespace before comparing engine
> names when we detect duplicates and skip loading engines with conflicting
> names.

Hardly seems worth it - they'll just use a unicode 'o' or something...
In our school we maintained our search plugins in C:\Program Files\Mozilla Firefox\browser\searchplugins. Now this will be impossible and we must settle with Firefox default engines? That doesn't seem user (and admin) friendly change. :(
Turns out everything that's "user-friendly" is also attacker friendly.

That said, modding that directory has always been the wrong way to do a custom distribution.  Customizations shouldn't touch existing files, they should be in the /distribution folder. See https://wiki.mozilla.org/Distribution_INI_File for background.  Note that we're likely to limit open use of /distribution to ESR builds at some point.
So you saying, we can block default Firefox search plugins using distribution.ini and use our search plugins instead?
This is not the right place to discuss how to configure Firefox for an enterprise distribution.  Please see https://wiki.mozilla.org/Deployment:Deploying_Firefox for the docs, and https://mail.mozilla.org/listinfo/enterprise for any followup questions).
> So there has to be implemented a way to delete or at least to hide the default search engines.

https://support.mozilla.org/en-US/kb/add-or-remove-search-engine-firefox
(In reply to Mike Hommey [:glandium] from comment #44)
> > So there has to be implemented a way to delete or at least to hide the default search engines.
> 
> https://support.mozilla.org/en-US/kb/add-or-remove-search-engine-firefox

Not an option either.

1. Where do I have those settings with browser.search.showOneOffButtons;false?
2. If I hide or delete the default search engines, my own search engines with the same name are also hidden or deleted.

And, btw., this is NOT off-topic. This IS related to this bug, because you, the devs, invented this. So, please, fix it.
(In reply to Heiko Baums from comment #45)

> 1. Where do I have those settings with
> browser.search.showOneOffButtons;false?

This hidden preference no longer exists in Firefox 43 and later.

> 2. If I hide or delete the default search engines, my own search engines
> with the same name are also hidden or deleted.

Adding an engine with the same name as a default engine is intentionally not supported. If you want to add a custom engine, give it a custom name.

> And, btw., this is NOT off-topic.

This does not contribute to fixing this bug (which is already fixed, and verified), so it is off-topic. If something is currently broken, please file a new bug (and mark it as blocking this one if you think the new bug was caused by this).
This bugfix breaks existing functionality.

I have 36 search engines, amongst them are a lot without an icon.
Without icons the new search UI is unusable.

I'd like to add icons to them, but newly added engines are not situated in the 
"searchplugins/" directory any more and hence aren't user configurable any more.

As I understand it, this bugfix is responsible for moving search engine configuration to not user editable files like "omni.ja".

Please revert this so called fix or change it, so that it will not break existing functionality.

(BTW I was always taught that introducing security by obscurity is a bad thing.)
(In reply to marty from comment #48)

> I'd like to add icons to them, but newly added engines are not situated in
> the 
> "searchplugins/" directory any more and hence aren't user configurable any
> more.

The related change was from bug 1203167, but really you were looking for bug 1236498.

> Please revert this so called fix or change it, so that it will not break
> existing functionality.

Editing files in the profile directory by hand has never been a supported feature. See bug 1236498 comment 7 for a workaround to get the result you want.
Dear Florian,

Thank you for your referrals, but when I look at the status of them, then I get sad.

Especially the status of Bug 1236498 WONTFIX (Unable to replace user added search engine icon by editing search.json(.mozlz4) or xml file).

Thanks for your workaround tip, but as I understand it's kind of a hack, that's not really supported and quite work intensive to maintain.

The current search becomes rather unusable for power users that have 30+ search engines without icons (see bug 1233810 comment 2).

The new search forces one to have search engine icons, but adding icons oneself is not supported. So users are stuck between a rock and a hard place as it is.
(In reply to marty from comment #50)

> Thanks for your workaround tip, but as I understand it's kind of a hack,
> that's not really supported and quite work intensive to maintain.

More supported than what you were doing before, but yes it's a hack.

> The current search becomes rather unusable for power users that have 30+
> search engines without icons (see bug 1233810 comment 2).

Having more than 30 search engines seems to me like a very rare case (although we don't have data to say confidently how rare it is; I filed bug 1268424 to collect some data about it).

Having engines without icons seems like a bug, as we either use the icon provided inside the plugin, or the favicon of the site that offered the plugin.
Attachment #9183663 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.