[tor-commits] [Git][tpo/applications/tor-browser][tor-browser-128.6.0esr-14.5-1] 5 commits: fixup! TB 40597: Implement TorSettings module

Pier Angelo Vendrame pushed to branch tor-browser-128.6.0esr-14.5-1 at The Tor Project / Applications / Tor Browser

Commits: bd7602fe by Henry Wilkes at 2025-01-20T18:25:01+00:00 fixup! TB 40597: Implement TorSettings module

TB 41921: Only use TorSettings.changeSettings to change the Tor settings and apply them in one method.

The granular methods have been made private.

- - - - - e43836ca by Henry Wilkes at 2025-01-20T18:26:14+00:00 fixup! TB 42247: Android helpers for the TorProvider

TB 41921: Only use TorSettings.changeSettings to change the Tor settings and apply them in one method.

We drop redundant arguments and unused methods from android integration.

- - - - - 6864f8ab by Henry Wilkes at 2025-01-20T18:26:15+00:00 fixup! TB 41878: [android] Add standalone Tor Bootstrap

TB 41921: Drop redundant arguments to getTorIntegration().setSettings.

All calls to this method passed true for the last two arguments.

- - - - - d6536e87 by Henry Wilkes at 2025-01-20T18:26:16+00:00 fixup! TB 31286: Implementation of bridge, proxy, and firewall settings in about:preferences#connection

TB 41921: Only use TorSettings.changeSettings to change the Tor settings and apply them in one method.

- - - - - 3dd18040 by Henry Wilkes at 2025-01-20T18:26:16+00:00 fixup! TB 27476: Implement about:torconnect captive portal within Tor Browser

TB 41921: Only use TorSettings.changeSettings to change the Tor settings and apply them in one method.

- - - - -

7 changed files:

- browser/components/torpreferences/content/connectionPane.js - browser/components/torpreferences/content/connectionSettingsDialog.js - mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tor/TorControllerGV.kt - mobile/android/geckoview/src/main/java/org/mozilla/geckoview/TorIntegrationAndroid.java - toolkit/components/torconnect/TorConnectParent.sys.mjs - toolkit/modules/TorAndroidIntegration.sys.mjs - toolkit/modules/TorSettings.sys.mjs

Changes:

===================================== browser/components/torpreferences/content/connectionPane.js ===================================== @@ -96,33 +96,6 @@ const Lox = { }; */

-/** - * Make changes to TorSettings and save them. - * - * Bulk changes will be frozen together. - * - * @param {Function} changes - Method to apply changes to TorSettings. - */ -async function setTorSettings(changes) { - if (!TorSettings.initialized) { - log.warning("Ignoring changes to uninitialized TorSettings"); - return; - } - TorSettings.freezeNotifications(); - try { - changes(); - // This will trigger TorSettings.#cleanupSettings() - TorSettings.saveToPrefs(); - try { - await TorSettings.applySettings(); - } catch (e) { - console.error("Failed to apply Tor settings", e); - } - } finally { - TorSettings.thawNotifications(); - } -} - /** * Get the ID/fingerprint of the bridge used in the most recent Tor circuit. * @@ -757,6 +730,7 @@ const gBridgeGrid = { }); removeItem.addEventListener("click", () => { const bridgeLine = row.bridgeLine; + const source = TorSettings.bridges.source; const strings = TorSettings.bridges.bridge_strings; const index = strings.indexOf(bridgeLine); if (index === -1) { @@ -764,8 +738,8 @@ const gBridgeGrid = { } strings.splice(index, 1);

- setTorSettings(() => { - TorSettings.bridges.bridge_strings = strings; + TorSettings.changeSettings({ + bridges: { source, bridge_strings: strings }, }); }); }, @@ -1829,8 +1803,8 @@ const gBridgeSettings = { if (!this._haveBridges) { return; } - setTorSettings(() => { - TorSettings.bridges.enabled = this._toggleButton.pressed; + TorSettings.changeSettings({ + bridges: { enabled: this._toggleButton.pressed }, }); });

@@ -2215,10 +2189,10 @@ const gBridgeSettings = { return; }

- setTorSettings(() => { + TorSettings.changeSettings({ // This should always have the side effect of disabling bridges as // well. - TorSettings.bridges.source = TorBridgeSource.Invalid; + bridges: { source: TorBridgeSource.Invalid }, }); });

@@ -2319,10 +2293,12 @@ const gBridgeSettings = { if (!result.type) { return null; } - return setTorSettings(() => { - TorSettings.bridges.enabled = true; - TorSettings.bridges.source = TorBridgeSource.BuiltIn; - TorSettings.bridges.builtin_type = result.type; + return TorSettings.changeSettings({ + bridges: { + enabled: true, + source: TorBridgeSource.BuiltIn, + builtin_type: result.type, + }, }); } ); @@ -2339,10 +2315,12 @@ const gBridgeSettings = { if (!result.bridges?.length) { return null; } - return setTorSettings(() => { - TorSettings.bridges.enabled = true; - TorSettings.bridges.source = TorBridgeSource.BridgeDB; - TorSettings.bridges.bridge_strings = result.bridges.join("\n"); + return TorSettings.changeSettings({ + bridges: { + enabled: true, + source: TorBridgeSource.BridgeDB, + bridge_strings: result.bridges.join("\n"), + }, }); } ); @@ -2363,16 +2341,15 @@ const gBridgeSettings = { if (!loxId && !result.addresses?.length) { return null; } - return setTorSettings(() => { - TorSettings.bridges.enabled = true; - if (loxId) { - TorSettings.bridges.source = TorBridgeSource.Lox; - TorSettings.bridges.lox_id = loxId; - } else { - TorSettings.bridges.source = TorBridgeSource.UserProvided; - TorSettings.bridges.bridge_strings = result.addresses; - } - }); + const bridges = { enabled: true }; + if (loxId) { + bridges.source = TorBridgeSource.Lox; + bridges.lox_id = loxId; + } else { + bridges.source = TorBridgeSource.UserProvided; + bridges.bridge_strings = result.addresses; + } + return TorSettings.changeSettings({ bridges }); } ); }, @@ -2586,9 +2563,9 @@ const gConnectionPane = (function () { "torPreferences-quickstart-toggle" ); this._enableQuickstartCheckbox.addEventListener("command", () => { - const checked = this._enableQuickstartCheckbox.checked; - TorSettings.quickstart.enabled = checked; - TorSettings.saveToPrefs().applySettings(); + TorSettings.changeSettings({ + quickstart: { enabled: this._enableQuickstartCheckbox.checked }, + }); }); this._enableQuickstartCheckbox.checked = TorSettings.quickstart.enabled; Services.obs.addObserver(this, TorSettingsTopics.SettingsChanged);

===================================== browser/components/torpreferences/content/connectionSettingsDialog.js ===================================== @@ -270,33 +270,34 @@ const gConnectionSettingsDialog = { const port = this._proxyPortTextbox.value; const username = this._proxyUsernameTextbox.value; const password = this._proxyPasswordTextbox.value; + const settings = { proxy: {}, firewall: {} }; switch (type) { case TorProxyType.Invalid: - TorSettings.proxy.enabled = false; + settings.proxy.enabled = false; break; case TorProxyType.Socks4: - TorSettings.proxy.enabled = true; - TorSettings.proxy.type = type; - TorSettings.proxy.address = address; - TorSettings.proxy.port = port; - TorSettings.proxy.username = ""; - TorSettings.proxy.password = ""; + settings.proxy.enabled = true; + settings.proxy.type = type; + settings.proxy.address = address; + settings.proxy.port = port; + settings.proxy.username = ""; + settings.proxy.password = ""; break; case TorProxyType.Socks5: - TorSettings.proxy.enabled = true; - TorSettings.proxy.type = type; - TorSettings.proxy.address = address; - TorSettings.proxy.port = port; - TorSettings.proxy.username = username; - TorSettings.proxy.password = password; + settings.proxy.enabled = true; + settings.proxy.type = type; + settings.proxy.address = address; + settings.proxy.port = port; + settings.proxy.username = username; + settings.proxy.password = password; break; case TorProxyType.HTTPS: - TorSettings.proxy.enabled = true; - TorSettings.proxy.type = type; - TorSettings.proxy.address = address; - TorSettings.proxy.port = port; - TorSettings.proxy.username = username; - TorSettings.proxy.password = password; + settings.proxy.enabled = true; + settings.proxy.type = type; + settings.proxy.address = address; + settings.proxy.port = port; + settings.proxy.username = username; + settings.proxy.password = password; break; }

@@ -304,16 +305,15 @@ const gConnectionSettingsDialog = { ? this._allowedPortsTextbox.value : ""; if (portListString) { - TorSettings.firewall.enabled = true; - TorSettings.firewall.allowed_ports = portListString; + settings.firewall.enabled = true; + settings.firewall.allowed_ports = portListString; } else { - TorSettings.firewall.enabled = false; + settings.firewall.enabled = false; }

- TorSettings.saveToPrefs(); // FIXME: What if this fails? Should we prevent the dialog to close and show // an error? - TorSettings.applySettings(); + TorSettings.changeSettings(settings); }, };

===================================== mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tor/TorControllerGV.kt ===================================== @@ -80,7 +80,7 @@ class TorControllerGV( set(value) { getTorSettings()?.let { it.quickstart = value - getTorIntegration().setSettings(it, true, true) + getTorIntegration().setSettings(it) } }

@@ -110,7 +110,7 @@ class TorControllerGV( getTorSettings()?.let { if (!value || it.bridgesSource != BridgeSource.Invalid) { it.bridgesEnabled = value - getTorIntegration().setSettings(it, true, true) + getTorIntegration().setSettings(it) } } } @@ -150,7 +150,7 @@ class TorControllerGV( } it.bridgesBuiltinType = bbt } - getTorIntegration().setSettings(it, true, true) + getTorIntegration().setSettings(it) } }

@@ -175,7 +175,7 @@ class TorControllerGV( val userProvidedLines: Array<String> = value?.split("\n")?.filter { it.length > 4 }?.toTypedArray() ?: arrayOf<String>() it.bridgesSource = BridgeSource.UserProvided it.bridgeBridgeStrings = userProvidedLines - getTorIntegration().setSettings(it, true, true) + getTorIntegration().setSettings(it) } }

===================================== mobile/android/geckoview/src/main/java/org/mozilla/geckoview/TorIntegrationAndroid.java ===================================== @@ -48,8 +48,6 @@ public class TorIntegrationAndroid implements BundleEventListener { // Events we emit private static final String EVENT_SETTINGS_GET = "GeckoView:Tor:SettingsGet"; private static final String EVENT_SETTINGS_SET = "GeckoView:Tor:SettingsSet"; - private static final String EVENT_SETTINGS_APPLY = "GeckoView:Tor:SettingsApply"; - private static final String EVENT_SETTINGS_SAVE = "GeckoView:Tor:SettingsSave"; private static final String EVENT_BOOTSTRAP_BEGIN = "GeckoView:Tor:BootstrapBegin"; private static final String EVENT_BOOTSTRAP_BEGIN_AUTO = "GeckoView:Tor:BootstrapBeginAuto"; private static final String EVENT_BOOTSTRAP_CANCEL = "GeckoView:Tor:BootstrapCancel"; @@ -194,7 +192,7 @@ public class TorIntegrationAndroid implements BundleEventListener { protected void onPostExecute(TorSettings torSettings) { mSettings = torSettings; if (TorLegacyAndroidSettings.unmigrated()) { - setSettings(mSettings, true, true); + setSettings(mSettings); TorLegacyAndroidSettings.setMigrated(); } } @@ -657,10 +655,10 @@ public class TorIntegrationAndroid implements BundleEventListener { return mSettings; }

- public void setSettings(final TorSettings settings, boolean save, boolean apply) { + public void setSettings(final TorSettings settings) { mSettings = settings;

- emitSetSettings(settings, save, apply) + emitSetSettings(settings) .then( new GeckoResult.OnValueListener<Void, Void>() { public GeckoResult<Void> onValue(Void v) { @@ -677,22 +675,12 @@ public class TorIntegrationAndroid implements BundleEventListener { }

private @NonNull GeckoResult<Void> emitSetSettings( - final TorSettings settings, boolean save, boolean apply) { - GeckoBundle bundle = new GeckoBundle(3); - bundle.putBoolean("save", save); - bundle.putBoolean("apply", apply); + final TorSettings settings) { + GeckoBundle bundle = new GeckoBundle(1); bundle.putBundle("settings", settings.asGeckoBundle()); return EventDispatcher.getInstance().queryVoid(EVENT_SETTINGS_SET, bundle); }

- public @NonNull GeckoResult<Void> applySettings() { - return EventDispatcher.getInstance().queryVoid(EVENT_SETTINGS_APPLY); - } - - public @NonNull GeckoResult<Void> saveSettings() { - return EventDispatcher.getInstance().queryVoid(EVENT_SETTINGS_SAVE); - } - public @NonNull GeckoResult<Void> beginBootstrap() { return EventDispatcher.getInstance().queryVoid(EVENT_BOOTSTRAP_BEGIN); }

===================================== toolkit/components/torconnect/TorConnectParent.sys.mjs ===================================== @@ -104,8 +104,7 @@ export class TorConnectParent extends JSWindowActorParent { // If there are multiple home pages, just load the first one. return Promise.resolve(TorConnect.fixupURIs(lazy.HomePage.get())[0]); case "torconnect:set-quickstart": - TorSettings.quickstart.enabled = message.data; - TorSettings.saveToPrefs().applySettings(); + TorSettings.changeSettings({ quickstart: { enabled: message.data } }); break; case "torconnect:open-tor-preferences": this.browsingContext.top.embedderElement.ownerGlobal.openPreferences(

===================================== toolkit/modules/TorAndroidIntegration.sys.mjs ===================================== @@ -36,8 +36,6 @@ const ListenedEvents = Object.freeze({ settingsGet: "GeckoView:Tor:SettingsGet", // The data is passed directly to TorSettings. settingsSet: "GeckoView:Tor:SettingsSet", - settingsApply: "GeckoView:Tor:SettingsApply", - settingsSave: "GeckoView:Tor:SettingsSave", bootstrapBegin: "GeckoView:Tor:BootstrapBegin", // Optionally takes a countryCode, as data.countryCode. bootstrapBeginAuto: "GeckoView:Tor:BootstrapBeginAuto", @@ -145,20 +143,10 @@ class TorAndroidIntegrationImpl { callback?.onSuccess(lazy.TorSettings.getSettings()); return; case ListenedEvents.settingsSet: - // This does not throw, so we do not have any way to report the error! - lazy.TorSettings.setSettings(data.settings); - if (data.save) { - lazy.TorSettings.saveToPrefs(); - } - if (data.apply) { - await lazy.TorSettings.applySettings(); - } - break; - case ListenedEvents.settingsApply: - await lazy.TorSettings.applySettings(); - break; - case ListenedEvents.settingsSave: - await lazy.TorSettings.saveToPrefs(); + // TODO: Handle setting throw? This can throw if data.settings is + // incorrectly formatted, but more like it can throw when the settings + // fail to be passed onto the TorProvider. tor-browser#43405. + await lazy.TorSettings.changeSettings(data.settings); break; case ListenedEvents.bootstrapBegin: lazy.TorConnect.beginBootstrapping();

===================================== toolkit/modules/TorSettings.sys.mjs ===================================== @@ -422,7 +422,7 @@ class TorSettingsImpl { * not contradict each other. */ #cleanupSettings() { - this.freezeNotifications(); + this.#freezeNotifications(); try { if (this.bridges.source === TorBridgeSource.Invalid) { this.bridges.enabled = false; @@ -449,7 +449,7 @@ class TorSettingsImpl { this.firewall.allowed_ports = []; } } finally { - this.thawNotifications(); + this.#thawNotifications(); } }

@@ -488,7 +488,7 @@ class TorSettingsImpl { * changes you make with a `try` block and call thawNotifications in the * `finally` block. */ - freezeNotifications() { + #freezeNotifications() { this.#freezeNotificationsCount++; } /** @@ -497,7 +497,7 @@ class TorSettingsImpl { * Note, if some other method has also frozen the notifications, this will * only release them once it has also called this method. */ - thawNotifications() { + #thawNotifications() { this.#freezeNotificationsCount--; this.#tryNotification(); } @@ -571,7 +571,7 @@ class TorSettingsImpl { this.#checkIfInitialized(); } const prevVal = this.#settings[groupname][name]; - this.freezeNotifications(); + this.#freezeNotifications(); try { if (transform) { val = transform(val, addError); @@ -589,7 +589,7 @@ class TorSettingsImpl { } catch (e) { addError(e.message); } finally { - this.thawNotifications(); + this.#thawNotifications(); } }, }); @@ -717,7 +717,7 @@ class TorSettingsImpl { Services.prefs.getBoolPref(TorSettingsPrefs.enabled, false) ) { // Do not want notifications for initially loaded prefs. - this.freezeNotifications(); + this.#freezeNotifications(); try { this.#allowUninitialized = true; this.#loadFromPrefs(); @@ -726,7 +726,7 @@ class TorSettingsImpl { } finally { this.#allowUninitialized = false; this.#notificationQueue.clear(); - this.thawNotifications(); + this.#thawNotifications(); } }

@@ -755,7 +755,7 @@ class TorSettingsImpl { // source. But we do pass on the changes to TorProvider. // FIXME: This can compete with TorConnect to reach TorProvider. // tor-browser#42316 - this.applySettings(); + this.#applySettings(); } break; } @@ -876,7 +876,7 @@ class TorSettingsImpl { /** * Save our settings to prefs. */ - saveToPrefs() { + #saveToPrefs() { lazy.logger.debug("saveToPrefs()");

this.#checkIfInitialized(); @@ -968,8 +968,6 @@ class TorSettingsImpl {

// all tor settings now stored in prefs :) Services.prefs.setBoolPref(TorSettingsPrefs.enabled, true); - - return this; }

/** @@ -978,23 +976,28 @@ class TorSettingsImpl { * Even though this introduces a circular depdency, it makes the API nicer for * frontend consumers. */ - async applySettings() { + async #applySettings() { this.#checkIfInitialized(); const provider = await lazy.TorProviderBuilder.build(); await provider.writeSettings(); }

/** - * Set blocks of settings at once from an object. + * Change the Tor settings in use. + * + * It is possible to set all settings, or only some sections: * - * It is possible to set all settings, or only some sections (e.g., only - * bridges), but if a key is present, its settings must make sense (e.g., if - * bridges are enabled, a valid source must be provided). + * + quickstart.enabled can be set individually. + * + bridges.enabled can be set individually. + * + bridges.source can be set with a corresponding bridge specification for + * the source (bridge_strings, lox_id, builtin_type). + * + proxy settings can be set as a group. + * + firewall settings can be set a group. * - * @param {object} settings The settings object to set + * @param {object} settings - The settings object to set. */ - setSettings(settings) { - lazy.logger.debug("setSettings()"); + async changeSettings(settings) { + lazy.logger.debug("changeSettings()", settings); this.#checkIfInitialized();

const backup = this.getSettings(); @@ -1003,38 +1006,39 @@ class TorSettingsImpl { this.#settingErrors = [];

// Hold off on lots of notifications until all settings are changed. - this.freezeNotifications(); + this.#freezeNotifications(); try { - if ("quickstart" in settings) { + if ("quickstart" in settings && "enabled" in settings.quickstart) { this.quickstart.enabled = !!settings.quickstart.enabled; }

if ("bridges" in settings) { - this.bridges.enabled = !!settings.bridges.enabled; - // Currently, disabling bridges in the UI does not remove the lines, - // because we call only the `enabled` setter. - // So, if the bridge source is undefined but bridges are disabled, - // do not force Invalid. Instead, keep the current source. - if (this.bridges.enabled || settings.bridges.source !== undefined) { - this.bridges.source = settings.bridges.source; + if ("enabled" in settings.bridges) { + this.bridges.enabled = !!settings.bridges.enabled; } - switch (settings.bridges.source) { - case TorBridgeSource.BridgeDB: - case TorBridgeSource.UserProvided: - this.bridges.bridge_strings = settings.bridges.bridge_strings; - break; - case TorBridgeSource.BuiltIn: - this.bridges.builtin_type = settings.bridges.builtin_type; - break; - case TorBridgeSource.Lox: - this.bridges.lox_id = settings.bridges.lox_id; - break; - case TorBridgeSource.Invalid: - break; + if ("source" in settings.bridges) { + this.bridges.source = settings.bridges.source; + switch (settings.bridges.source) { + case TorBridgeSource.BridgeDB: + case TorBridgeSource.UserProvided: + this.bridges.bridge_strings = settings.bridges.bridge_strings; + break; + case TorBridgeSource.BuiltIn: + this.bridges.builtin_type = settings.bridges.builtin_type; + break; + case TorBridgeSource.Lox: + this.bridges.lox_id = settings.bridges.lox_id; + break; + case TorBridgeSource.Invalid: + break; + case undefined: + break; + } } }

if ("proxy" in settings) { + // proxy settings have to be set as a group. this.proxy.enabled = !!settings.proxy.enabled; if (this.proxy.enabled) { this.proxy.type = settings.proxy.type; @@ -1046,6 +1050,7 @@ class TorSettingsImpl { }

if ("firewall" in settings) { + // firewall settings have to be set as a group. this.firewall.enabled = !!settings.firewall.enabled; if (this.firewall.enabled) { this.firewall.allowed_ports = settings.firewall.allowed_ports; @@ -1057,27 +1062,33 @@ class TorSettingsImpl { if (this.#settingErrors.length) { throw Error(this.#settingErrors.join("; ")); } + this.#saveToPrefs(); } catch (ex) { // Restore the old settings without any new notifications generated from // the above code. - // NOTE: Since this code is not async, it should not be possible for - // some other call to TorSettings to change anything whilst we are - // in this context (other than lower down in this call stack), so it is - // safe to discard all changes to settings and notifications. + // NOTE: Since the code that changes #settings is not async, it should not + // be possible for some other call to TorSettings to change anything + // whilst we are in this context (other than lower down in this call + // stack), so it is safe to discard all changes to settings and + // notifications. this.#settings = backup; this.#notificationQueue.clear(); for (const notification of backupNotifications) { this.#notificationQueue.add(notification); }

- lazy.logger.error("setSettings failed", ex); + throw ex; } finally { - this.thawNotifications(); + this.#thawNotifications(); // Stop collecting errors. this.#settingErrors = null; }

lazy.logger.debug("setSettings result", this.#settings); + + // After we have sent out the notifications for the changed settings and + // saved the preferences we send the new settings to TorProvider. + await this.#applySettings(); }

/** @@ -1162,7 +1173,7 @@ class TorSettingsImpl {

// After checks are complete, we commit them. this.#temporaryBridgeSettings = bridgeSettings; - await this.applySettings(); + await this.#applySettings(); }

/** @@ -1174,10 +1185,9 @@ class TorSettingsImpl { lazy.logger.warn("No temporary bridges to save"); return; } - this.setSettings({ bridges: this.#temporaryBridgeSettings }); + const bridgeSettings = this.#temporaryBridgeSettings; this.#temporaryBridgeSettings = null; - this.saveToPrefs(); - await this.applySettings(); + await this.changeSettings({ bridges: bridgeSettings }); }

/** @@ -1190,7 +1200,7 @@ class TorSettingsImpl { return; } this.#temporaryBridgeSettings = null; - await this.applySettings(); + await this.#applySettings(); } }

View it on GitLab: https://gitlab.torproject.org/tpo/applications/tor-browser/-/compare/6ea6bd7...