From 33899422e7a9fac96f1ba9804dc29f853b696a69 Mon Sep 17 00:00:00 2001 From: Mihai Alexandru Michis Date: Mon, 25 May 2020 16:43:03 +0300 Subject: [PATCH] Backed out 2 changesets (bug 1635235, bug 1635239) for causing Bug 1640583. CLOSED TREE Backed out changeset 97ecda13df18 (bug 1635239) Backed out changeset c9f80397bbec (bug 1635235) --- .../parent/ext-chrome-settings-overrides.js | 4 +- .../test_ext_settings_overrides_defaults.js | 15 +- browser/installer/allowed-dupes.mn | 1 - services/settings/dumps/main/moz.build | 1 - .../search-default-override-allowlist.json | 1 - toolkit/components/search/SearchEngine.jsm | 7 +- toolkit/components/search/SearchService.jsm | 142 ++------ toolkit/components/search/SearchUtils.jsm | 18 +- .../tests/xpcshell/test_override_allowlist.js | 341 ------------------ .../search/tests/xpcshell/xpcshell-common.ini | 1 - 10 files changed, 39 insertions(+), 492 deletions(-) delete mode 100644 services/settings/dumps/main/search-default-override-allowlist.json delete mode 100644 toolkit/components/search/tests/xpcshell/test_override_allowlist.js diff --git a/browser/components/extensions/parent/ext-chrome-settings-overrides.js b/browser/components/extensions/parent/ext-chrome-settings-overrides.js index 0d6ab8a46384..14a6a5a21341 100644 --- a/browser/components/extensions/parent/ext-chrome-settings-overrides.js +++ b/browser/components/extensions/parent/ext-chrome-settings-overrides.js @@ -267,7 +267,7 @@ this.chrome_settings_overrides = class extends ExtensionAPI { static async onUninstall(id) { let searchStartupPromise = pendingSearchSetupTasks.get(id); if (searchStartupPromise) { - await searchStartupPromise.catch(Cu.reportError); + await searchStartupPromise; } // Note: We do not have to deal with homepage here as it is managed by // the ExtensionPreferencesManager. @@ -423,7 +423,7 @@ this.chrome_settings_overrides = class extends ExtensionAPI { } else { // Needs to be called every time to handle reenabling, but // only sets default for install or enable. - await this.setDefault(engineName); + this.setDefault(engineName); } } } diff --git a/browser/components/extensions/test/xpcshell/test_ext_settings_overrides_defaults.js b/browser/components/extensions/test/xpcshell/test_ext_settings_overrides_defaults.js index 8979ebfb7906..1646bbde1470 100644 --- a/browser/components/extensions/test/xpcshell/test_ext_settings_overrides_defaults.js +++ b/browser/components/extensions/test/xpcshell/test_ext_settings_overrides_defaults.js @@ -11,16 +11,6 @@ const { SearchTestUtils } = ChromeUtils.import( "resource://testing-common/SearchTestUtils.jsm" ); -const { SearchUtils } = ChromeUtils.import( - "resource://gre/modules/SearchUtils.jsm" -); - -const { RemoteSettings } = ChromeUtils.import( - "resource://services-settings/remote-settings.js" -); - -const { sinon } = ChromeUtils.import("resource://testing-common/Sinon.jsm"); - const URLTYPE_SUGGEST_JSON = "application/x-suggestions+json"; AddonTestUtils.init(this); @@ -128,8 +118,7 @@ add_task(async function test_extension_changing_to_app_provided_default() { }); add_task(async function test_extension_overriding_app_provided_default() { - const settings = await RemoteSettings(SearchUtils.SETTINGS_ALLOWLIST_KEY); - sinon.stub(settings, "get").returns([ + Services.search.wrappedJSObject._getSearchDefaultOverrideAllowlist = () => [ { thirdPartyId: "test@thirdparty.example.com", overridesId: "test2@search.mozilla.org", @@ -139,7 +128,7 @@ add_task(async function test_extension_overriding_app_provided_default() { }, ], }, - ]); + ]; let ext1 = ExtensionTestUtils.loadExtension({ manifest: { diff --git a/browser/installer/allowed-dupes.mn b/browser/installer/allowed-dupes.mn index 382881ff9cea..1154516f0f45 100644 --- a/browser/installer/allowed-dupes.mn +++ b/browser/installer/allowed-dupes.mn @@ -62,7 +62,6 @@ browser/chrome/browser/res/payments/formautofill/autofillEditForms.js # Bug 1451050 - Remote settings empty dumps (will be populated with data eventually) browser/defaults/settings/pinning/pins.json browser/defaults/settings/main/example.json -browser/defaults/settings/main/search-default-override-allowlist.json #ifdef MOZ_EME_WIN32_ARTIFACT gmp-clearkey/0.1/manifest.json diff --git a/services/settings/dumps/main/moz.build b/services/settings/dumps/main/moz.build index 3628fa00b5e6..f0e8becdd40f 100644 --- a/services/settings/dumps/main/moz.build +++ b/services/settings/dumps/main/moz.build @@ -9,7 +9,6 @@ FINAL_TARGET_FILES.defaults.settings.main += [ 'language-dictionaries.json', 'onboarding.json', 'search-config.json', - 'search-default-override-allowlist.json', 'sites-classification.json', 'url-classifier-skip-urls.json', ] diff --git a/services/settings/dumps/main/search-default-override-allowlist.json b/services/settings/dumps/main/search-default-override-allowlist.json deleted file mode 100644 index ccb562190823..000000000000 --- a/services/settings/dumps/main/search-default-override-allowlist.json +++ /dev/null @@ -1 +0,0 @@ -{"data":[]} \ No newline at end of file diff --git a/toolkit/components/search/SearchEngine.jsm b/toolkit/components/search/SearchEngine.jsm index f4d86ccfa226..708348a130bd 100644 --- a/toolkit/components/search/SearchEngine.jsm +++ b/toolkit/components/search/SearchEngine.jsm @@ -1919,12 +1919,7 @@ SearchEngine.prototype = { * @returns {string} */ get telemetryId() { - let telemetryId = - this._telemetryId || this.identifier || `other-${this.name}`; - if (this.getAttr("overriddenBy")) { - return telemetryId + "-addon"; - } - return telemetryId; + return this._telemetryId || this.identifier || `other-${this.name}`; }, /** diff --git a/toolkit/components/search/SearchService.jsm b/toolkit/components/search/SearchService.jsm index b93f53636288..ac5dea107465 100644 --- a/toolkit/components/search/SearchService.jsm +++ b/toolkit/components/search/SearchService.jsm @@ -21,7 +21,6 @@ XPCOMUtils.defineLazyModuleGetters(this, { IgnoreLists: "resource://gre/modules/IgnoreLists.jsm", OS: "resource://gre/modules/osfile.jsm", Region: "resource://gre/modules/Region.jsm", - RemoteSettings: "resource://services-settings/remote-settings.js", SearchEngine: "resource://gre/modules/SearchEngine.jsm", SearchEngineSelector: "resource://gre/modules/SearchEngineSelector.jsm", SearchStaticData: "resource://gre/modules/SearchStaticData.jsm", @@ -52,13 +51,6 @@ XPCOMUtils.defineLazyPreferenceGetter( } ); -XPCOMUtils.defineLazyGetter(this, "logConsole", () => { - return console.createInstance({ - prefix: "SearchService", - maxLogLevel: SearchUtils.loggingEnabled ? "Debug" : "Warn", - }); -}); - // A text encoder to UTF8, used whenever we commit the cache to disk. XPCOMUtils.defineLazyGetter(this, "gEncoder", function() { return new TextEncoder(); @@ -606,9 +598,6 @@ SearchService.prototype = { */ _metaData: {}, - // A reference to the handler for the default override allow list. - _defaultOverrideAllowlist: null, - // This reflects the combined values of the prefs for enabling the separate // private default UI, and for the user choosing a separate private engine. // If either one is disabled, then we don't enable the separate private default. @@ -853,6 +842,32 @@ SearchService.prototype = { return false; }, + async _getSearchDefaultOverrideAllowlist() { + return []; + }, + + async _canOverrideDefault(extension, appProvidedExtensionId) { + const overrideTable = await this._getSearchDefaultOverrideAllowlist(); + + let entry = overrideTable.find(e => e.thirdPartyId == extension.id); + if (!entry) { + return false; + } + + if (appProvidedExtensionId != entry.overridesId) { + return false; + } + let searchProvider = + extension.manifest.chrome_settings_overrides.search_provider; + return entry.urls.some( + e => + searchProvider.search_url == e.search_url && + searchProvider.search_form == e.search_form && + searchProvider.search_url_get_params == e.search_url_get_params && + searchProvider.search_url_post_params == e.search_url_post_params + ); + }, + async maybeSetAndOverrideDefault(extension) { let searchProvider = extension.manifest.chrome_settings_overrides.search_provider; @@ -866,10 +881,6 @@ SearchService.prototype = { SearchUtils.DEFAULT_TAG ); - if (!this._defaultOverrideAllowlist) { - this._defaultOverrideAllowlist = new SearchDefaultOverrideAllowlistHandler(); - } - if ( extension.startupReason === "ADDON_INSTALL" || extension.startupReason === "ADDON_ENABLE" @@ -878,16 +889,7 @@ SearchService.prototype = { if (this.defaultEngine.name == searchProvider.name) { return false; } - if ( - !(await this._defaultOverrideAllowlist.canOverride( - extension, - engine._extensionID - )) - ) { - logConsole.debug( - "Allowing default engine to be set to app-provided.", - extension.id - ); + if (!(await this._canOverrideDefault(extension, engine._extensionID))) { // We don't allow overriding the engine in this case, but we can allow // the extension to change the default engine. return true; @@ -895,26 +897,15 @@ SearchService.prototype = { if (extension.startupReason === "ADDON_INSTALL") { // We're ok to override. engine.overrideWithExtension(params); - logConsole.debug( - "Allowing default engine to be set to app-provided and overridden.", - extension.id - ); return true; } } if ( engine.getAttr("overriddenBy") == extension.id && - (await this._defaultOverrideAllowlist.canOverride( - extension, - engine._extensionID - )) + (await this._canOverrideDefault(extension, engine._extensionID)) ) { engine.overrideWithExtension(params); - logConsole.debug( - "Re-enabling overriding of core extension by", - extension.id - ); return true; } @@ -3892,81 +3883,4 @@ XPCOMUtils.defineLazyServiceGetter( "nsIIdleService" ); -/** - * Handles getting and checking extensions against the allow list. - */ -class SearchDefaultOverrideAllowlistHandler { - /** - * @param {function} listener - * A listener for configuration update changes. - */ - constructor(listener) { - this._remoteConfig = RemoteSettings(SearchUtils.SETTINGS_ALLOWLIST_KEY); - } - - /** - * Determines if a search engine extension can override a default one - * according to the allow list. - * - * @param {object} extension - * The extension object (from add-on manager) that will override the - * app provided search engine. - * @param {string} appProvidedExtensionId - * The id of the search engine that will be overriden. - * @returns {boolean} - * Returns true if the search engine extension may override the app provided - * instance. - */ - async canOverride(extension, appProvidedExtensionId) { - const overrideTable = await this._getAllowlist(); - - let entry = overrideTable.find(e => e.thirdPartyId == extension.id); - if (!entry) { - return false; - } - - if (appProvidedExtensionId != entry.overridesId) { - return false; - } - - let searchProvider = - extension.manifest.chrome_settings_overrides.search_provider; - - return entry.urls.some( - e => - searchProvider.search_url == e.search_url && - searchProvider.search_form == e.search_form && - searchProvider.search_url_get_params == e.search_url_get_params && - searchProvider.search_url_post_params == e.search_url_post_params - ); - } - - /** - * Obtains the configuration from remote settings. This includes - * verifying the signature of the record within the database. - * - * If the signature in the database is invalid, the database will be wiped - * and the stored dump will be used, until the settings next update. - * - * Note that this may cause a network check of the certificate, but that - * should generally be quick. - * - * @returns {array} - * An array of objects in the database, or an empty array if none - * could be obtained. - */ - async _getAllowlist() { - let result = []; - try { - result = await this._remoteConfig.get(); - } catch (ex) { - // Don't throw an error just log it, just continue with no data, and hopefully - // a sync will fix things later on. - Cu.reportError(ex); - } - logConsole.debug("Allow list is:", result); - return result; - } -} - var EXPORTED_SYMBOLS = ["SearchService"]; diff --git a/toolkit/components/search/SearchUtils.jsm b/toolkit/components/search/SearchUtils.jsm index 9820dd44b7de..da9092948fe0 100644 --- a/toolkit/components/search/SearchUtils.jsm +++ b/toolkit/components/search/SearchUtils.jsm @@ -23,18 +23,6 @@ var SearchUtils = { SETTINGS_KEY: "search-config", - /** - * This is the Remote Settings key that we use to get the ignore lists for - * engines. - */ - SETTINGS_IGNORELIST_KEY: "hijack-blocklists", - - /** - * This is the Remote Settings key that we use to get the allow lists for - * overriding the default engines. - */ - SETTINGS_ALLOWLIST_KEY: "search-default-override-allowlist", - /** * Topic used for events involving the service itself. */ @@ -80,6 +68,12 @@ var SearchUtils = { // resolution causes windows-1252 to be actually used. DEFAULT_QUERY_CHARSET: "ISO-8859-1", + /** + * This is the Remote Settings key that we use to get the ignore lists for + * engines. + */ + SETTINGS_IGNORELIST_KEY: "hijack-blocklists", + // A tag to denote when we are using the "default_locale" of an engine. DEFAULT_TAG: "default", diff --git a/toolkit/components/search/tests/xpcshell/test_override_allowlist.js b/toolkit/components/search/tests/xpcshell/test_override_allowlist.js deleted file mode 100644 index 1cadd2df0bac..000000000000 --- a/toolkit/components/search/tests/xpcshell/test_override_allowlist.js +++ /dev/null @@ -1,341 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - http://creativecommons.org/publicdomain/zero/1.0/ */ - -"use strict"; - -const kBaseURL = "https://example.com/"; -const kSearchEngineURL = `${kBaseURL}?q={searchTerms}&foo=myparams`; -const kOverriddenEngineName = "Simple Engine"; - -SearchTestUtils.initXPCShellAddonManager(this); - -const whitelist = [ - { - thirdPartyId: "test@thirdparty.example.com", - overridesId: "simple@search.mozilla.org", - urls: [], - }, -]; - -const tests = [ - { - title: "test_not_changing_anything", - startupReason: "ADDON_INSTALL", - search_provider: { - is_default: true, - name: "MozParamsTest2", - keyword: "MozSearch", - search_url: kSearchEngineURL, - }, - expected: { - switchToDefaultAllowed: false, - overridesEngine: false, - }, - }, - { - title: "test_changing_default_engine", - startupReason: "ADDON_INSTALL", - search_provider: { - is_default: true, - name: kOverriddenEngineName, - keyword: "MozSearch", - search_url: kSearchEngineURL, - }, - expected: { - switchToDefaultAllowed: true, - overridesEngine: false, - }, - }, - { - title: "test_overriding_default_engine", - startupReason: "ADDON_INSTALL", - search_provider: { - is_default: true, - name: kOverriddenEngineName, - keyword: "MozSearch", - search_url: kSearchEngineURL, - }, - whitelistUrls: [ - { - search_url: kSearchEngineURL, - }, - ], - expected: { - switchToDefaultAllowed: true, - overridesEngine: true, - searchUrl: kSearchEngineURL, - }, - }, - { - title: "test_overriding_default_engine_different_url", - startupReason: "ADDON_INSTALL", - search_provider: { - is_default: true, - name: kOverriddenEngineName, - keyword: "MozSearch", - search_url: kSearchEngineURL + "a", - }, - whitelistUrls: [ - { - search_url: kSearchEngineURL, - }, - ], - expected: { - switchToDefaultAllowed: true, - overridesEngine: false, - }, - }, - { - title: "test_overriding_default_engine_get_params", - startupReason: "ADDON_INSTALL", - search_provider: { - is_default: true, - name: kOverriddenEngineName, - keyword: "MozSearch", - search_url: kBaseURL, - search_url_get_params: "q={searchTerms}&enc=UTF-8", - }, - whitelistUrls: [ - { - search_url: kBaseURL, - search_url_get_params: "q={searchTerms}&enc=UTF-8", - }, - ], - expected: { - switchToDefaultAllowed: true, - overridesEngine: true, - searchUrl: `${kBaseURL}?q={searchTerms}&enc=UTF-8`, - }, - }, - { - title: "test_overriding_default_engine_different_get_params", - startupReason: "ADDON_INSTALL", - search_provider: { - is_default: true, - name: kOverriddenEngineName, - keyword: "MozSearch", - search_url: kBaseURL, - search_url_get_params: "q={searchTerms}&enc=UTF-8a", - }, - whitelistUrls: [ - { - search_url: kBaseURL, - search_url_get_params: "q={searchTerms}&enc=UTF-8", - }, - ], - expected: { - switchToDefaultAllowed: true, - overridesEngine: false, - }, - }, - { - title: "test_overriding_default_engine_post_params", - startupReason: "ADDON_INSTALL", - search_provider: { - is_default: true, - name: kOverriddenEngineName, - keyword: "MozSearch", - search_url: kBaseURL, - search_url_post_params: "q={searchTerms}&enc=UTF-8", - }, - whitelistUrls: [ - { - search_url: kBaseURL, - search_url_post_params: "q={searchTerms}&enc=UTF-8", - }, - ], - expected: { - switchToDefaultAllowed: true, - overridesEngine: true, - searchUrl: `${kBaseURL}`, - postData: "q={searchTerms}&enc=UTF-8", - }, - }, - { - title: "test_overriding_default_engine_different_post_params", - startupReason: "ADDON_INSTALL", - search_provider: { - is_default: true, - name: kOverriddenEngineName, - keyword: "MozSearch", - search_url: kBaseURL, - search_url_post_params: "q={searchTerms}&enc=UTF-8a", - }, - whitelistUrls: [ - { - search_url: kBaseURL, - search_url_post_params: "q={searchTerms}&enc=UTF-8", - }, - ], - expected: { - switchToDefaultAllowed: true, - overridesEngine: false, - }, - }, - { - title: "test_overriding_default_engine_search_form", - startupReason: "ADDON_INSTALL", - search_provider: { - is_default: true, - name: kOverriddenEngineName, - keyword: "MozSearch", - search_url: kBaseURL, - search_form: "https://example.com/form", - }, - whitelistUrls: [ - { - search_url: kBaseURL, - search_form: "https://example.com/form", - }, - ], - expected: { - switchToDefaultAllowed: true, - overridesEngine: true, - searchUrl: `${kBaseURL}`, - searchForm: "https://example.com/form", - }, - }, - { - title: "test_overriding_default_engine_different_search_form", - startupReason: "ADDON_INSTALL", - search_provider: { - is_default: true, - name: kOverriddenEngineName, - keyword: "MozSearch", - search_url: kBaseURL, - search_form: "https://example.com/forma", - }, - whitelistUrls: [ - { - search_url: kBaseURL, - search_form: "https://example.com/form", - }, - ], - expected: { - switchToDefaultAllowed: true, - overridesEngine: false, - }, - }, -]; - -let baseExtension; -let remoteSettingsStub; - -add_task(async function setup() { - await SearchTestUtils.useTestEngines("simple-engines"); - await AddonTestUtils.promiseStartupManager(); - await Services.search.init(); - - baseExtension = ExtensionTestUtils.loadExtension({ - manifest: { - applications: { - gecko: { - id: "test@thirdparty.example.com", - }, - }, - }, - useAddonManager: "permanent", - }); - await baseExtension.startup(); - - const settings = await RemoteSettings(SearchUtils.SETTINGS_ALLOWLIST_KEY); - remoteSettingsStub = sinon.stub(settings, "get").returns([]); - - registerCleanupFunction(async () => { - await baseExtension.unload(); - }); -}); - -for (const test of tests) { - add_task(async () => { - info(test.title); - - let extension = { - ...baseExtension, - startupReason: test.startupReason, - manifest: { - chrome_settings_overrides: { - search_provider: test.search_provider, - }, - }, - }; - - if (test.expected.overridesEngine) { - remoteSettingsStub.returns([ - { ...whitelist[0], urls: test.whitelistUrls }, - ]); - } - - Assert.equal( - await Services.search.maybeSetAndOverrideDefault(extension), - test.expected.switchToDefaultAllowed, - "Should have returned the correct value for allowing switch to default or not." - ); - - let engine = await Services.search.getEngineByName(kOverriddenEngineName); - Assert.equal( - !!engine.wrappedJSObject.getAttr("overriddenBy"), - test.expected.overridesEngine, - "Should have correctly overridden or not." - ); - - Assert.equal( - engine.telemetryId, - "simple" + (test.expected.overridesEngine ? "-addon" : ""), - "Should set the correct telemetry Id" - ); - - if (test.expected.overridesEngine) { - let submission = engine.getSubmission("{searchTerms}"); - Assert.equal( - decodeURI(submission.uri.spec), - test.expected.searchUrl, - "Should have set the correct url on an overriden engine" - ); - - if (test.expected.search_form) { - Assert.equal( - engine.wrappedJSObject._searchForm, - test.expected.searchForm, - "Should have overridden the search form." - ); - } - - if (test.expected.postData) { - let sis = Cc["@mozilla.org/scriptableinputstream;1"].createInstance( - Ci.nsIScriptableInputStream - ); - sis.init(submission.postData); - let data = sis.read(submission.postData.available()); - Assert.equal( - decodeURIComponent(data), - test.expected.postData, - "Should have overridden the postData" - ); - } - - // As we're not testing the WebExtension manager as well, - // set this engine as default so we can check the telemetry data. - let oldDefaultEngine = Services.search.defaultEngine; - Services.search.defaultEngine = engine; - - let engineInfo = await Services.search.getDefaultEngineInfo(); - Assert.deepEqual( - engineInfo, - { - defaultSearchEngine: "simple-addon", - defaultSearchEngineData: { - loadPath: "[other]addEngineWithDetails:simple@search.mozilla.org", - name: "Simple Engine", - origin: "default", - submissionURL: test.expected.searchUrl.replace("{searchTerms}", ""), - }, - }, - "Should return the extended identifier and alternate submission url to telemetry" - ); - Services.search.defaultEngine = oldDefaultEngine; - - engine.wrappedJSObject.removeExtensionOverride(); - } - }); -} diff --git a/toolkit/components/search/tests/xpcshell/xpcshell-common.ini b/toolkit/components/search/tests/xpcshell/xpcshell-common.ini index 4c6944af9062..9b267d2adf60 100644 --- a/toolkit/components/search/tests/xpcshell/xpcshell-common.ini +++ b/toolkit/components/search/tests/xpcshell/xpcshell-common.ini @@ -52,7 +52,6 @@ support-files = [test_nodb_pluschanges.js] [test_notifications.js] [test_originalDefaultEngine.js] -[test_override_allowlist.js] [test_paramSubstitution.js] [test_parseSubmissionURL.js] [test_pref.js]