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

Commits: 169cebe0 by Henry Wilkes at 2025-01-28T10:06:19+00:00 fixup! TB 40597: Implement TorSettings module

TB 43254: Allow some DomainFrontedRequests to be cancelled.

This is used by some of the Moat calls to stop abandoned requests running in the background:

1. Cancelled AutoBootstraps. 2. Cancelled CAPTCHA request/submissions.

- - - - - 400e4ab1 by Henry Wilkes at 2025-01-28T10:06:20+00:00 fixup! TB 31286: Implementation of bridge, proxy, and firewall settings in about:preferences#connection

TB 43254: Cancel Moat requests when the CAPTHCA dialog is closed.

- - - - -

6 changed files:

- browser/app/profile/000-tor-browser.js - browser/components/torpreferences/content/requestBridgeDialog.js - toolkit/modules/BridgeDB.sys.mjs - toolkit/modules/DomainFrontedRequests.sys.mjs - toolkit/modules/Moat.sys.mjs - toolkit/modules/TorConnect.sys.mjs

Changes:

===================================== browser/app/profile/000-tor-browser.js ===================================== @@ -143,6 +143,7 @@ pref("browser.tor_provider.cp_log_level", "Warn"); pref("lox.log_level", "Warn"); pref("torbrowser.bootstrap.log_level", "Info"); pref("browser.torsettings.log_level", "Warn"); +pref("browser.torMoat.loglevel", "Warn"); pref("browser.tordomainisolator.loglevel", "Warn"); pref("browser.torcircuitpanel.loglevel", "Log"); pref("browser.tor_android.log_level", "Info");

===================================== browser/components/torpreferences/content/requestBridgeDialog.js ===================================== @@ -119,6 +119,9 @@ const gRequestBridgeDialog = { },

_setcaptchaImage(uri) { + if (!uri) { + return; + } if (uri != this._captchaImage.src) { this._captchaImage.src = uri; this._dialogHeader.setAttribute(

===================================== toolkit/modules/BridgeDB.sys.mjs ===================================== @@ -13,6 +13,16 @@ export var BridgeDB = { _challenge: null, _image: null, _bridges: null, + /** + * A collection of controllers to abort any ongoing Moat requests if the + * dialog is closed. + * + * NOTE: We do not expect this set to ever contain more than one instance. + * However the public API has no assurances to prevent multiple calls. + * + * @type {Set<AbortController>} + */ + _moatAbortControllers: new Set(),

get currentCaptchaImage() { return this._image; @@ -28,13 +38,18 @@ export var BridgeDB = { await this._moatRPC.init(); }

- const response = await this._moatRPC.check( - "obfs4", - this._challenge, - solution, - false - ); - this._bridges = response?.bridges; + const abortController = new AbortController(); + this._moatAbortControllers.add(abortController); + try { + this._bridges = await this._moatRPC.check( + "obfs4", + this._challenge, + solution, + abortController.signal + ); + } finally { + this._moatAbortControllers.delete(abortController); + } return this._bridges; },

@@ -45,10 +60,20 @@ export var BridgeDB = { await this._moatRPC.init(); }

- const response = await this._moatRPC.fetch(["obfs4"]); - this._challenge = response.challenge; - this._image = - "data:image/jpeg;base64," + encodeURIComponent(response.image); + const abortController = new AbortController(); + this._moatAbortControllers.add(abortController); + let response; + try { + response = await this._moatRPC.fetch(["obfs4"], abortController.signal); + } finally { + this._moatAbortControllers.delete(abortController); + } + if (response) { + // Not cancelled. + this._challenge = response.challenge; + this._image = + "data:image/jpeg;base64," + encodeURIComponent(response.image); + } } catch (err) { console.error("Could not request a captcha image", err); } @@ -56,6 +81,11 @@ export var BridgeDB = { },

close() { + // Abort any ongoing requests. + for (const controller of this._moatAbortControllers) { + controller.abort(); + } + this._moatAbortControllers.clear(); this._moatRPC?.uninit(); this._moatRPC = null; this._challenge = null;

===================================== toolkit/modules/DomainFrontedRequests.sys.mjs ===================================== @@ -377,6 +377,15 @@ export class DomainFrontRequestResponseError extends Error { } }

+/** + * Thrown when the caller cancels the request. + */ +export class DomainFrontRequestCancelledError extends Error { + constructor(url) { + super(`Cancelled request to ${url}`); + } +} + /** * Callback object to promisify the XPCOM request. */ @@ -397,8 +406,12 @@ class ResponseListener { }); }

- // callers wait on this for final response - response() { + /** + * A promise that resolves to the response body from the request. + * + * @type {Promise<string>} + */ + get response() { return this.#responsePromise; }

@@ -530,24 +543,74 @@ export class DomainFrontRequestBuilder { * @param {string} args.method The request method. * @param {string} args.body The request body. * @param {string} args.contentType The "Content-Type" header to set. - * @returns {string} The response body. + * @param {AbortSignal} [signal] args.signal An optional means of cancelling + * the request early. Will throw DomainFrontRequestCancelledError if + * aborted. + * @returns {Promise<string>} A promise that resolves to the response body. */ - async buildRequest(url, args) { - const ch = this.buildHttpHandler(url); - - const inStream = Cc["@mozilla.org/io/string-input-stream;1"].createInstance( - Ci.nsIStringInputStream - ); - inStream.setData(args.body, args.body.length); - const upChannel = ch.QueryInterface(Ci.nsIUploadChannel); - upChannel.setUploadStream(inStream, args.contentType, args.body.length); - ch.requestMethod = args.method; + buildRequest(url, args) { + // Pre-fetch the argument values from `args` so the caller cannot change the + // parameters mid-call. + const { body, method, contentType, signal } = args; + let cancel = null; + const promise = new Promise((resolve, reject) => { + if (signal?.aborted) { + // Unexpected, cancel immediately. + reject(new DomainFrontRequestCancelledError(url)); + return; + }

- // Make request - const listener = new ResponseListener(); - await ch.asyncOpen(listener, ch); + let ch = null; + + if (signal) { + cancel = () => { + // Reject prior to calling cancel, since we want to ignore any error + // responses from ResponseListener. + // NOTE: In principle we could let ResponseListener throw this error + // when it receives NS_ERROR_ABORT, but that would rely on mozilla + // never calling this error either. + reject(new DomainFrontRequestCancelledError(url)); + ch?.cancel(Cr.NS_ERROR_ABORT); + }; + signal.addEventListener("abort", cancel); + }

- // wait for response - return listener.response(); + ch = this.buildHttpHandler(url); + + const inStream = Cc[ + "@mozilla.org/io/string-input-stream;1" + ].createInstance(Ci.nsIStringInputStream); + inStream.setData(body, body.length); + const upChannel = ch.QueryInterface(Ci.nsIUploadChannel); + upChannel.setUploadStream(inStream, contentType, body.length); + ch.requestMethod = method; + + // Make request + const listener = new ResponseListener(); + ch.asyncOpen(listener); + listener.response.then( + body => { + resolve(body); + }, + error => { + reject(error); + } + ); + }); + // Clean up. Do not return this `Promise.finally` since the caller should + // not depend on it. + // We pre-catch and suppress all errors for this `.finally` to stop the + // errors from being duplicated in the console log. + promise + .catch(() => {}) + .finally(() => { + // Remove the callback for the AbortSignal so that it doesn't hold onto + // our channel reference if the caller continues to hold a reference to + // AbortSignal. + if (cancel) { + signal.removeEventListener("abort", cancel); + } + }); + return promise; } }

===================================== toolkit/modules/Moat.sys.mjs ===================================== @@ -5,13 +5,15 @@ const lazy = {};

const log = console.createInstance({ - maxLogLevel: "Warn", prefix: "Moat", + maxLogLevelPref: "browser.torMoat.loglevel", });

ChromeUtils.defineESModuleGetters(lazy, { DomainFrontRequestBuilder: "resource://gre/modules/DomainFrontedRequests.sys.mjs", + DomainFrontRequestCancelledError: + "resource://gre/modules/DomainFrontedRequests.sys.mjs", TorBridgeSource: "resource://gre/modules/TorSettings.sys.mjs", });

@@ -91,6 +93,19 @@ class InternetTestResponseListener { * @property {string} [country] - The detected country (region). */

+/** + * @typedef {Object} CaptchaChallenge + * + * The details for a captcha challenge. + * + * @property {string} transport - The transport type selected by the Moat + * server. + * @property {string} image - A base64 encoded jpeg with the captcha to + * complete. + * @property {string} challenge - A nonce/cookie string associated with this + * request. + */ + /** * Constructs JSON objects and sends requests over Moat. * The documentation about the JSON schemas to use are available at @@ -122,17 +137,51 @@ export class MoatRPC { this.#requestBuilder = null; }

- async #makeRequest(procedure, args) { + /** + * @typedef {Object} MoatResult + * + * The result of a Moat request. + * + * @property {any} response - The parsed JSON response from the Moat server, + * or `undefined` if the request was cancelled. + * @property {boolean} cancelled - Whether the request was cancelled. + */ + + /** + * Make a request to Moat. + * + * @param {string} procedure - The name of the procedure. + * @param {object} args - The arguments to pass in as a JSON string. + * @param {AbortSignal} [abortSignal] - An optional signal to be able to abort + * the request early. + * @returns {MoatResult} - The result of the request. + */ + async #makeRequest(procedure, args, abortSignal) { const procedureURIString = `${Services.prefs.getStringPref( TorLauncherPrefs.moat_service )}/${procedure}`; - return JSON.parse( - await this.#requestBuilder.buildRequest(procedureURIString, { - method: "POST", - contentType: "application/vnd.api+json", - body: JSON.stringify(args), - }) - ); + log.info(`Making request to ${procedureURIString}:`, args); + let response = undefined; + let cancelled = false; + try { + response = JSON.parse( + await this.#requestBuilder.buildRequest(procedureURIString, { + method: "POST", + contentType: "application/vnd.api+json", + body: JSON.stringify(args), + signal: abortSignal, + }) + ); + log.info(`Response to ${procedureURIString}:`, response); + } catch (e) { + if (abortSignal && e instanceof lazy.DomainFrontRequestCancelledError) { + log.info(`Request to ${procedureURIString} cancelled`); + cancelled = true; + } else { + throw e; + } + } + return { response, cancelled }; }

async testInternetConnection() { @@ -147,15 +196,16 @@ export class MoatRPC { return listener.status; }

- // Receive a CAPTCHA challenge, takes the following parameters: - // - transports: array of transport strings available to us eg: ["obfs4", "meek"] - // - // returns an object with the following fields: - // - transport: a transport string the moat server decides it will send you selected - // from the list of provided transports - // - image: a base64 encoded jpeg with the captcha to complete - // - challenge: a nonce/cookie string associated with this request - async fetch(transports) { + /** + * Request a CAPTCHA challenge. + * + * @param {string[]} transports - List of transport strings available to us + * eg: ["obfs4", "meek"]. + * @param {AbortSignal} abortSignal - A signal to abort the request early. + * @returns {?CaptchaChallenge} - The captcha challenge, or `null` if the + * request was aborted by the caller. + */ + async fetch(transports, abortSignal) { if ( // ensure this is an array Array.isArray(transports) && @@ -173,7 +223,15 @@ export class MoatRPC { }, ], }; - const response = await this.#makeRequest("fetch", args); + const { response, cancelled } = await this.#makeRequest( + "fetch", + args, + abortSignal + ); + if (cancelled) { + return null; + } + if ("errors" in response) { const code = response.errors[0].code; const detail = response.errors[0].detail; @@ -189,18 +247,17 @@ export class MoatRPC { throw new Error("MoatRPC: fetch() expects a non-empty array of strings"); }

- // Submit an answer for a CAPTCHA challenge and get back bridges, takes the following - // parameters: - // - transport: the transport string associated with a previous fetch request - // - challenge: the nonce string associated with the fetch request - // - solution: solution to the CAPTCHA associated with the fetch request - // - qrcode: true|false whether we want to get back a qrcode containing the bridge strings - // - // returns an object with the following fields: - // - bridges: an array of bridge line strings - // - qrcode: base64 encoded jpeg of bridges if requested, otherwise null - // if the provided solution is incorrect, returns an empty object - async check(transport, challenge, solution, qrcode) { + /** + * Submit an answer for a previous CAPTCHA fetch to get bridges. + * + * @param {string} transport - The transport associated with the fetch. + * @param {string} challenge - The nonce string associated with the fetch. + * @param {string} solution - The solution to the CAPTCHA. + * @param {AbortSignal} abortSignal - A signal to abort the request early. + * @returns {?string[]} - The bridge lines for a correct solution, or `null` + * if the solution was incorrect or the request was aborted by the caller. + */ + async check(transport, challenge, solution, abortSignal) { const args = { data: [ { @@ -210,25 +267,30 @@ export class MoatRPC { transport, challenge, solution, - qrcode: qrcode ? "true" : "false", + qrcode: "false", }, ], }; - const response = await this.#makeRequest("check", args); + const { response, cancelled } = await this.#makeRequest( + "check", + args, + abortSignal + ); + if (cancelled) { + return null; + } + if ("errors" in response) { const code = response.errors[0].code; const detail = response.errors[0].detail; if (code == 419 && detail === "The CAPTCHA solution was incorrect.") { - return {}; + return null; }

throw new Error(`MoatRPC: ${detail} (${code})`); }

- const bridges = response.data[0].bridges; - const qrcodeImg = qrcode ? response.data[0].qrcode : null; - - return { bridges, qrcode: qrcodeImg }; + return response.data[0].bridges; }

/** @@ -296,15 +358,24 @@ export class MoatRPC { * @param {?string} country - The region to request bridges for, as an * ISO 3166-1 alpha-2 region code, or `null` to have the server * automatically determine the region. + * @param {AbortSignal} abortSignal - A signal to abort the request early. * @returns {?MoatSettings} - The returned settings from the server, or `null` - * if the region could not be determined by the server. + * if the region could not be determined by the server or the caller + * cancelled the request. */ - async circumvention_settings(transports, country) { + async circumvention_settings(transports, country, abortSignal) { const args = { transports: transports ? transports : [], country, }; - const response = await this.#makeRequest("circumvention/settings", args); + const { response, cancelled } = await this.#makeRequest( + "circumvention/settings", + args, + abortSignal + ); + if (cancelled) { + return null; + } let settings = {}; if ("errors" in response) { const code = response.errors[0].code; @@ -334,7 +405,11 @@ export class MoatRPC { // settings for. async circumvention_countries() { const args = {}; - return this.#makeRequest("circumvention/countries", args); + const { response } = await this.#makeRequest( + "circumvention/countries", + args + ); + return response; }

// Request a copy of the builtin bridges, takes the following parameters: @@ -347,7 +422,7 @@ export class MoatRPC { const args = { transports: transports ? transports : [], }; - const response = await this.#makeRequest("circumvention/builtin", args); + const { response } = await this.#makeRequest("circumvention/builtin", args); if ("errors" in response) { const code = response.errors[0].code; const detail = response.errors[0].detail; @@ -366,13 +441,22 @@ export class MoatRPC { * Request a copy of the default/fallback bridge settings. * * @param {string[]} transports - A list of transports we support. - * @returns {MoatBridges[]} - The list of bridges found. + * @param {AbortSignal} abortSignal - A signal to abort the request early. + * @returns {?MoatBridges[]} - The list of bridges found, or `null` if the + * caller cancelled the request. */ - async circumvention_defaults(transports) { + async circumvention_defaults(transports, abortSignal) { const args = { transports: transports ? transports : [], }; - const response = await this.#makeRequest("circumvention/defaults", args); + const { response, cancelled } = await this.#makeRequest( + "circumvention/defaults", + args, + abortSignal + ); + if (cancelled) { + return null; + } if ("errors" in response) { const code = response.errors[0].code; const detail = response.errors[0].detail;

===================================== toolkit/modules/TorConnect.sys.mjs ===================================== @@ -570,6 +570,7 @@ class AutoBootstrapAttempt { return; }

+ let moatAbortController = new AbortController(); // For now, throw any errors we receive from the backend, except when it // was unable to detect user's country/region. // If we use specialized error objects, we could pass the original errors @@ -577,11 +578,20 @@ class AutoBootstrapAttempt { const maybeSettings = await Promise.race([ moat.circumvention_settings( [...lazy.TorSettings.builtinBridgeTypes, "vanilla"], - options.regionCode === "automatic" ? null : options.regionCode + options.regionCode === "automatic" ? null : options.regionCode, + moatAbortController.signal ), // This might set maybeSettings to undefined. this.#cancelledPromise, ]); + if (this.#cancelled) { + // Ended early due to being cancelled. Abort the ongoing Moat request so + // that it does not continue unnecessarily in the background. + // NOTE: We do not care about circumvention_settings return value or + // errors at this point. Nor do we need to await its return. We just + // want it to resolve quickly. + moatAbortController.abort(); + } if (this.#cancelled || this.#resolved) { return; } @@ -591,15 +601,25 @@ class AutoBootstrapAttempt { if (maybeSettings?.bridgesList?.length) { this.#bridgesList = maybeSettings.bridgesList; } else { + // In principle we could reuse the existing moatAbortController + // instance, since its abort method has not been called. But it is + // cleaner to use a new instance to avoid triggering any potential + // lingering callbacks attached to the AbortSignal. + moatAbortController = new AbortController(); // Keep consistency with the other call. this.#bridgesList = await Promise.race([ - moat.circumvention_defaults([ - ...lazy.TorSettings.builtinBridgeTypes, - "vanilla", - ]), + moat.circumvention_defaults( + [...lazy.TorSettings.builtinBridgeTypes, "vanilla"], + moatAbortController.signal + ), // This might set this.#bridgesList to undefined. this.#cancelledPromise, ]); + if (this.#cancelled) { + // Ended early due to being cancelled. Abort the ongoing Moat request + // so that it does not continue in the background. + moatAbortController.abort(); + } } } finally { // Do not await the uninit.

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