Closed Bug 1184974 Opened 4 years ago Closed 4 years ago

The submissionURL should not be recorded for packed add-ons in the profile

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
firefox39 --- unaffected
firefox40 + verified
firefox41 + verified
firefox42 + verified

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 file, 1 obsolete file)

Looking at the data collected since bug 1164159 landed, it turns out we are collecting the submissionURL for engines with a loadpath like jar:[profile]/extensions/addon.xpi!browser/engine.xml

This was not intended, as after the privacy review we had decided to not collect the submissionURL for user-installed search engines.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → florian
Attachment #8635315 - Flags: review?(markh)
[Tracking Requested - why for this release]: we are accidentally collecting some data that we decided not to collect during the privacy review.
Attachment #8635315 - Flags: review?(markh) → review+
Looking at this again, it's not as simple as I thought yesterday.

The patch, as-is, breaks the toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js test.

Removing this line http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js?rev=39afb04cde61#1033 would make the test pass, but I'm not convinced the test is actually broken.

I think we do want to collect the submissionURL for engines loaded from a jar: URL, if that jar: URL is somehow coming from the browser.search.jarURIs preference. (Note: given that bug 1169459 is coming to remove that pref completely, maybe I shouldn't worry too much about this case.)

We don't want to collect the submissionURL for add-on search plugins that just happen to have a jar: URL because they are inside an xpi file.

I'll need to think some more about how to differentiate the two in the code.
QA Contact: petruta.rasa
Tracking enabled for 40, 41, and 42, so that it is sorted out, we do not collect submissionURLs when we shouldn't, but collect them when we should, as outlines in Comment 3.
Attached patch Patch v2Splinter Review
(In reply to Florian Quèze [:florian] [:flo] from comment #3)

> I think we do want to collect the submissionURL for engines loaded from a
> jar: URL, if that jar: URL is somehow coming from the browser.search.jarURIs
> preference. (Note: given that bug 1169459 is coming to remove that pref
> completely, maybe I shouldn't worry too much about this case.)
> 
> We don't want to collect the submissionURL for add-on search plugins that
> just happen to have a jar: URL because they are inside an xpi file.
> 
> I'll need to think some more about how to differentiate the two in the code.

Thinking about this again, I think the only reason why this matters is to detect search hijacking or impersonating done through abuse of the browser.search.jarURIs pref. The point of detecting abuse is to decide how to prioritize our efforts. Given that we have already decided to remove that pref (bug 1169459), I don't think having records of abuse there has any actual value, so I don't think it's worth complicating the code to deal with that case.

This new version of the patch tweaks the telemetry environment test to no longer expect the submissionURL to be recorded for the test engine loaded from a browser.search.jarURIs preference change. The reduced test coverage is a bit unfortunate, but I'll think about improving it when updating the patch in bug 1169459.

Carrying forward Mark's review for the straightforward test change.
Attachment #8635315 - Attachment is obsolete: true
Attachment #8636784 - Flags: review+
Comment on attachment 8636784 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 1164159
[User impact if declined]: small privacy leak for users with custom engines installed as add-ons.
[Describe test coverage new/current, TreeHerder]: none, unfortunately.
[Risks and why]: Low, only tweaking a regexp.
[String/UUID change made/needed]: none.
Attachment #8636784 - Flags: approval-mozilla-beta?
Attachment #8636784 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b30ddb111a55
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
How can we verify that this fix is good? Can you test on Nightly? Can you force and check a specific ping?
Flags: needinfo?(florian)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #9)
> How can we verify that this fix is good? Can you test on Nightly? Can you
> force and check a specific ping?

QA can verify by looking at the environment in about:telemetry. We should still report the submissionURL in defaultSearchEngineData.submissionURL if the current default engine is an engine that ships with Firefox. We should NOT report it if the current default engine is an engine that was installed as an add-on (ie. a .xpi file that got installed in the extensions subdirectory of the profile directory).
Flags: needinfo?(florian)
Florin - Does our team have time to attempt to verify this fix in the next Nightly build before uplifting to Beta on Thursday?
Flags: qe-verify?
Flags: needinfo?(florin.mezei)
Test environments: Win 7 64-bit, Mac OS X 10.9.5 and Ubuntu 12.04 32-bit
Nightly 2015-07-22:
After installing PL Language Pack and setting the pref general.useragent.locale to "pl", having Allegro as default search engines resulted in the following in about:telemetry:
defaultSearchEngineData.loadPath jar:[profile]/extensions/langpack-pl@firefox.mozilla.org.xpi!browser/allegro-pl.xml
defaultSearchEngineData.submissionURL http://allegro.pl/listing/listing.php?string=&sourceid=Mozilla-search 

With Nightly 2015-07-23, the submissionURL field was not reported in about:telemetry for none of the search engines that were installed with pl language pack.

Marking as verified based on these results.
Status: RESOLVED → VERIFIED
Flags: needinfo?(florin.mezei)
Comment on attachment 8636784 [details] [diff] [review]
Patch v2

Thank you for the quick trunaround on verification of this bug. Now that the fix is verified, let's get it into beta7. Beta+ Aurora+
Attachment #8636784 - Flags: approval-mozilla-beta?
Attachment #8636784 - Flags: approval-mozilla-beta+
Attachment #8636784 - Flags: approval-mozilla-aurora?
Attachment #8636784 - Flags: approval-mozilla-aurora+
Flags: qe-verify? → qe-verify+
Verified as fixed using the steps from comment 12 with Firefox 40 beta 7 and Developer Edition 41.0a2 2015-07-24 under Win 7 64-bit, Mac OS X 10.9.5 and Ubuntu 12.04 32-bit.
You need to log in before you can comment on or make changes to this bug.