From d96a42477f84d82bba5cbbd4fc112779b35dd6c2 Mon Sep 17 00:00:00 2001 From: Tyrone Johnson Date: Wed, 29 Jul 2026 15:05:04 +0300 Subject: [PATCH] fix(security): strip trezor-connect-src override by substring, not exact key @trezor/connect-web parseConnectSettings picks up connectSrc from any &-segment whose text contains the substring trezor-connect-src (indexOf >= 0), then uses it verbatim as the popup/iframe URL with no scheme validation for non-http values, so a javascript: value executes in our origin (Immunefi #40463). The previous guard used URLSearchParams.has(trezor-connect-src), an exact-key match, trivially bypassed by variants like trezor-connect-srcz or x-trezor-connect-src that the library still matches by substring. Replace it with stripTrezorConnectSrcParam(), which removes any query segment containing the substring case-insensitively (a superset of connect-web case-sensitive matching), before TrezorConnect.init() can read the query string. Unit-tested: exact param, the -srcz / x- prefixed / mixed-case bypasses, multi-param preservation, and no-op cases. --- apps/frontend/src/index.tsx | 22 +++++--- apps/frontend/src/utils/sanitizeUrl.test.ts | 61 +++++++++++++++++++++ apps/frontend/src/utils/sanitizeUrl.ts | 45 +++++++++++++++ 3 files changed, 121 insertions(+), 7 deletions(-) create mode 100644 apps/frontend/src/utils/sanitizeUrl.test.ts create mode 100644 apps/frontend/src/utils/sanitizeUrl.ts diff --git a/apps/frontend/src/index.tsx b/apps/frontend/src/index.tsx index b4b9e7343..63366b7e1 100644 --- a/apps/frontend/src/index.tsx +++ b/apps/frontend/src/index.tsx @@ -29,21 +29,29 @@ import './locales/i18n'; import { router } from './router'; import './styles/tailwindcss/index.css'; import { rskClient } from './utils/clients'; +import { stripTrezorConnectSrcParam } from './utils/sanitizeUrl'; -const checkAndRemoveQueryParam = () => { - const urlParams = new URLSearchParams(window.location.search); +// Neutralise the `@trezor/connect-web` `connectSrc`-override XSS (Immunefi #40463 and +// its `trezor-connect-srcz` substring bypass) before anything can read the query +// string. connect-web matches the override param by *substring*, so we must strip on +// the same basis rather than by exact key — see stripTrezorConnectSrcParam(). This runs +// synchronously at module load, well before the Trezor onboarding module ever calls +// TrezorConnect.init() (which only happens on user wallet selection). +const removeTrezorConnectSrcParam = () => { + const sanitized = stripTrezorConnectSrcParam(window.location.search); - if (urlParams.has('trezor-connect-src')) { - urlParams.delete('trezor-connect-src'); - - const newUrl = `${window.location.pathname}?${urlParams.toString()}`; + if (sanitized !== null) { + const newUrl = + window.location.pathname + + (sanitized ? `?${sanitized}` : '') + + window.location.hash; window.history.replaceState({}, document.title, newUrl); window.location.reload(); } }; -checkAndRemoveQueryParam(); +removeTrezorConnectSrcParam(); const root = ReactDOM.createRoot( document.getElementById('root') as HTMLElement, diff --git a/apps/frontend/src/utils/sanitizeUrl.test.ts b/apps/frontend/src/utils/sanitizeUrl.test.ts new file mode 100644 index 000000000..63622b432 --- /dev/null +++ b/apps/frontend/src/utils/sanitizeUrl.test.ts @@ -0,0 +1,61 @@ +import { stripTrezorConnectSrcParam } from './sanitizeUrl'; + +describe('utils/sanitizeUrl.ts', () => { + describe('stripTrezorConnectSrcParam()', () => { + // @trezor/connect-web's parseConnectSettings picks up `connectSrc` from ANY + // '&'-delimited query segment whose text *contains* the substring + // "trezor-connect-src" (`v.indexOf(...) >= 0`), then uses its value verbatim as + // the popup/iframe URL with no scheme validation for non-http values — so a + // `javascript:` value executes in our origin (Immunefi #40463). These cases pin + // down that we strip on the SAME substring basis, not an exact key, so the + // `…-srcz` (and similar) bypasses cannot slip through. + + it('strips the exact trezor-connect-src param', () => { + expect( + stripTrezorConnectSrcParam('?trezor-connect-src=javascript:alert(1)'), + ).toBe(''); + }); + + it('strips the trailing-character bypass variant (trezor-connect-srcz)', () => { + expect( + stripTrezorConnectSrcParam( + '?trezor-connect-srcz=javascript://ex.com/%250aalert(document.domain)//', + ), + ).toBe(''); + }); + + it('strips a leading-character variant (x-prefixed)', () => { + expect( + stripTrezorConnectSrcParam('?xtrezor-connect-src=javascript:alert(1)'), + ).toBe(''); + }); + + it('strips case-insensitively (superset of connect-web’s case-sensitive match)', () => { + expect( + stripTrezorConnectSrcParam('?Trezor-Connect-Srcz=javascript:alert(1)'), + ).toBe(''); + }); + + it('removes only the offending segment and preserves the rest', () => { + expect( + stripTrezorConnectSrcParam('?a=1&trezor-connect-srcz=evil&b=2'), + ).toBe('a=1&b=2'); + }); + + it('removes every offending segment when several are present', () => { + expect( + stripTrezorConnectSrcParam( + '?trezor-connect-src=1&x-trezor-connect-src=2', + ), + ).toBe(''); + }); + + it('returns null when no matching param is present', () => { + expect(stripTrezorConnectSrcParam('?a=1&b=2')).toBeNull(); + }); + + it('returns null for an empty query string', () => { + expect(stripTrezorConnectSrcParam('')).toBeNull(); + }); + }); +}); diff --git a/apps/frontend/src/utils/sanitizeUrl.ts b/apps/frontend/src/utils/sanitizeUrl.ts new file mode 100644 index 000000000..95b9f8856 --- /dev/null +++ b/apps/frontend/src/utils/sanitizeUrl.ts @@ -0,0 +1,45 @@ +/** + * The substring `@trezor/connect-web` scans the query string for when deciding + * whether to override `connectSrc` (the popup/iframe origin). See + * `@trezor/connect-web`'s `parseConnectSettings`, which does + * `window.location.search.split('&').find(v => v.indexOf('trezor-connect-src') >= 0)`. + */ +export const TREZOR_CONNECT_SRC_NEEDLE = 'trezor-connect-src'; + +/** + * Removes any query-string segment that `@trezor/connect-web` would treat as a + * `connectSrc` override. + * + * connect-web's `parseConnectSettings` scans `window.location.search.split('&')` + * and takes the first segment whose text *contains* the substring + * `trezor-connect-src` (`v.indexOf(...) >= 0`), then uses its value verbatim as the + * popup/iframe URL with no scheme validation for non-`http` values. A `javascript:` + * value therefore executes in our origin (Immunefi #40463). + * + * Because the library matches by substring, an exact-key filter + * (`URLSearchParams.has('trezor-connect-src')`) is trivially bypassed by variants + * such as `trezor-connect-srcz` or `x-trezor-connect-src`. We strip on the same + * substring basis instead — case-insensitively, a superset of connect-web's + * case-sensitive match — so no variant survives. + * + * @param search Raw `window.location.search` (with or without a leading `?`). + * @returns The rebuilt query string WITHOUT a leading `?` (an empty string when + * every segment was stripped), or `null` when nothing matched and the URL should + * be left untouched. + */ +export const stripTrezorConnectSrcParam = (search: string): string | null => { + if (!search) { + return null; + } + + const segments = search.replace(/^\?/, '').split('&'); + const kept = segments.filter( + segment => !segment.toLowerCase().includes(TREZOR_CONNECT_SRC_NEEDLE), + ); + + if (kept.length === segments.length) { + return null; + } + + return kept.join('&'); +};