Request filtering in main process2 #16264

peter-sanderson · 2025-01-08T13:55:55Z

Resolve #7171

This PR adds a whitelist for all requests coming out from the main thread.
Custom back-ends are dynamically whitelisted.

❗ First the Small refactoring (code-splitting & rename) of the interceptor.ts code for main process of the Electron #16263 needs to be merged
Next I need to fix interceptor.test.ts test
and make it run in the CI 🙀
custom back-ends (probably needs to be dynamically adjusted)
~~The whitelist needs to be probably different from the one used in render process~~ (no, consulted with Carlos)
make sure the test works with native fetch from node.js as well and not just with node-fetch => this needs to be discussed with Carlos

github-actions · 2025-01-09T11:06:36Z

🚀 Expo preview is ready!

Project → trezor-suite-preview
Platforms → android, ios
Scheme → trezorsuitelite
Runtime Version → 21
More info

Learn more about 𝝠 Expo Github Action

peter-sanderson · 2025-01-10T13:13:05Z

packages/request-manager/src/interceptor/interceptorTypesAndUtils.ts

+    return whitelist.some(
+        whitelistedUrl =>
+            whitelistedUrl === hostname ||
+            // Todo: this .endsWith() seems fishy to me, why we need this?


I plan to try to remove this together with sldev in next PR and we'll see what breaks 😁

Since this fn is called only with hostname extracted from the url, then we are safe that you cannot loophole it like that:
http://scammy-url.net?uselessParam=trezor.io

I think this is necessary to allows subdomains: btc1.trezor.io (would break? ), or anything.that.ends.with.trezor.io which is guaranteed not to be scammy-url.net so it's ok ✔️

peter-sanderson · 2025-01-13T12:30:10Z

packages/request-manager/e2e/interceptor.test.ts

@@ -2,9 +2,14 @@ import path from 'path';
 import http from 'http';
 import WebSocket from 'ws';

+// Todo: Currently this needs to be done in order to interceptor in test to work.
+//       This shall be taken care of, as we shall be intercepting the native fetch as well.
+// import fetch from 'node-fetch';


@karliatto FYI, this is what we have been talking about

peter-sanderson · 2025-01-13T12:48:37Z

packages/suite-desktop-core/src/modules/request-interceptor.ts

+
+                return;
+
+            case 'INTERCEPTED_HEADERS':


Those events does not seem to be handled anywhere, despite being (potentially) emitted.

I am not sure if this switch is supposed to be actually exhaustive, but as those are not handled anywhere, else (and also not here originally).

peter-sanderson · 2025-01-13T17:42:06Z

packages/request-manager/src/interceptor/interceptNetSocketConnect.ts

@@ -73,6 +76,9 @@ export const interceptNetSocketConnect = (context: InterceptorContext) => {
            details = typeof callback === 'string' ? `${callback}:${request}` : request.toString();
        }

+        const hostname = details.split(':')[0];


Todo: This is sub-optimal and it wont probably work in all scenarios. Any ideas?

peter-sanderson · 2025-01-13T17:42:11Z

packages/request-manager/src/interceptor/interceptTlsConnect.ts

+            return '';
+        };
+
+        const hostname = getHostname().split(':')[0] ?? '';


Todo: This is sub-optimal and it wont probably work in all scenarios. Any ideas?

as we discussed the new URL(address).hostname - I just now realised you could create a helper that would do it optionally, something like:

optionalGetUrlHostname = (address: string) => { try { return new URL(address).hostname; } catch(_err) { return null; } }

then use it like hostname = optionalGetUrlHostname(address) ?? ''
file:///... ➡️ '' ✔️
invalid URLs ➡️ null ➡️ '' ✔️

…ess of the electrum

Lemonexe

Did not test yet, CR'd most of code, so dropping off comments 💬

Lemonexe · 2025-01-14T12:35:14Z

packages/request-manager/src/interceptor/overloadHttpRequest.ts

+    callback,
+    validateRequest,
+}: OverloadHttpRequestParams) => {
+    const hostname = typeof url === 'object' ? url.hostname ?? url.host ?? '' : '';


this means that for string url, hostname will always be '', which will always be false in isWhitelistedHost

Lemonexe · 2025-01-14T12:40:55Z

packages/suite-desktop-core/src/modules/coinjoin.ts

+            mainThreadEmitter.emit('module/request-interceptor', {
+                type: 'ADD_WHITELISTED_DOMAIN',
+                domain: urlObj.hostname ?? urlObj.host,
+            });


If I understand correctly this effectively adds coinjoin coordinator URL to allowed domains?

Lemonexe · 2025-01-14T12:45:31Z

packages/suite-desktop-core/src/modules/request-interceptor.ts

-        if (event.type === 'CIRCUIT_MISBEHAVING') {
-            mainThreadEmitter.emit('module/reset-tor-circuits', event);
+            default: {
+                const _exhaustiveCheck: never = event; // Poor-man's `switch-exhaustiveness-check`


why poor man's? This is absolutely fine 😄
Although typed record is better when mapping primitive values to cases, I would not use it to map functions. That would not read very well IMO.

Lemonexe · 2025-01-14T12:47:38Z

packages/suite-desktop-core/src/modules/request-interceptor.ts

+                return;
+
+            case 'SET_WHITELISTED_DOMAINS_FOR_CUSTOM_BACKENDS':
+                mainThreadAllowedDomain.customBackends[event.coin] = event.domains;


there is no reverse action when resetting a custom backend to default, so it will probably persist as enabled until you restart app or set custom backend again for the same coin.
I'm not saying it's needed, just a note. I think it's OK.

Lemonexe · 2025-01-14T15:17:21Z

packages/suite-desktop-core/src/modules/trezor-connect.ts

+
+            const urlObj = new URL(url);
+
+            return urlObj.hostname ?? urlObj.host;


FYI here you can see difference between hostname and host https://developer.mozilla.org/en-US/docs/Web/API/Location
I'd only use hostname (w/o port), so the condition is broader = more restrictive

Also, both are typed as string, so I expect the ?? to be useless, maybe you meant || ?

peter-sanderson force-pushed the request-filtering-in-main-process branch from 0d4c182 to e9c5d25 Compare January 8, 2025 13:56

peter-sanderson force-pushed the request-filtering-in-main-process2 branch 2 times, most recently from 4c3b80a to 7c52991 Compare January 9, 2025 11:02

peter-sanderson force-pushed the request-filtering-in-main-process2 branch from 7c52991 to 80d2247 Compare January 9, 2025 11:18

trezor-ci force-pushed the request-filtering-in-main-process branch from e9c5d25 to b5528cc Compare January 9, 2025 11:21

peter-sanderson changed the base branch from request-filtering-in-main-process to develop January 9, 2025 11:22

peter-sanderson force-pushed the request-filtering-in-main-process2 branch 6 times, most recently from 2023d2c to 053106c Compare January 10, 2025 12:36

komret assigned peter-sanderson Jan 10, 2025

peter-sanderson force-pushed the request-filtering-in-main-process2 branch from 053106c to 685fb18 Compare January 10, 2025 13:04

peter-sanderson commented Jan 10, 2025

View reviewed changes

peter-sanderson force-pushed the request-filtering-in-main-process2 branch 4 times, most recently from 81d0bd8 to a40a5be Compare January 13, 2025 12:29

peter-sanderson commented Jan 13, 2025

View reviewed changes

peter-sanderson force-pushed the request-filtering-in-main-process2 branch from a40a5be to 22d43d9 Compare January 13, 2025 17:27

peter-sanderson marked this pull request as ready for review January 13, 2025 17:28

peter-sanderson force-pushed the request-filtering-in-main-process2 branch from 22d43d9 to 834759a Compare January 13, 2025 17:39

peter-sanderson commented Jan 13, 2025

View reviewed changes

feat: implement request blocking based on whitelist for the main proc…

e66a922

…ess of the electrum

peter-sanderson force-pushed the request-filtering-in-main-process2 branch from 834759a to e66a922 Compare January 13, 2025 17:53

Lemonexe reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request filtering in main process2 #16264

Request filtering in main process2 #16264

peter-sanderson commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

peter-sanderson Jan 10, 2025

Lemonexe Jan 14, 2025 •

edited

Loading

peter-sanderson Jan 13, 2025

peter-sanderson Jan 13, 2025

peter-sanderson Jan 13, 2025

peter-sanderson Jan 13, 2025

Lemonexe Jan 14, 2025

Lemonexe left a comment

Lemonexe Jan 14, 2025

Lemonexe Jan 14, 2025

Lemonexe Jan 14, 2025

Lemonexe Jan 14, 2025

Lemonexe Jan 14, 2025 •

edited

Loading


		const urlObj = new URL(url);

		return urlObj.hostname ?? urlObj.host;

Request filtering in main process2 #16264

Are you sure you want to change the base?

Request filtering in main process2 #16264

Conversation

peter-sanderson commented Jan 8, 2025 • edited Loading

Resolve #7171

github-actions bot commented Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Lemonexe Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lemonexe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lemonexe Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

peter-sanderson commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

Lemonexe Jan 14, 2025 •

edited

Loading

Lemonexe Jan 14, 2025 •

edited

Loading