Bootstrap MetaMask compatibility mode and replace EIP-6963 announcements - #1568
KillariDev wants to merge 24 commits into
Conversation
|
/review |
…-eip6963-message # Conflicts: # app/ts/background/popupMessageHandlers.ts # app/ts/utils/contentScriptsUpdating.ts # test/tests/contentScriptsUpdating.test.ts # test/tests/popupMessageDispatcher.test.ts
|
/review |
KillariDev
changed the title
…-eip6963-message # Conflicts: # app/ts/background/popupMessageHandlers.ts
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
CI Agent Review
MetaMask compatibility mode is wired for MV2, but the MV3 (Chrome) main-world injection path will fail: the new inpage/js/metamaskCompatibilityMode.js prelude is prepended to the MAIN-world inpage registration in updateContentScriptInjectionStrategyManifestV3, yet it was only added to manifestV2.json's web_accessible_resources and not to manifestV3.json. Chrome refuses to inject MAIN-world scripts that are not web-accessible, so the compatibility flag is never set on the page and all compat behavior silently fails on Chrome/MV3; depending on Chrome's validation timing, the whole MAIN-world registration update could also fail.
| excludeMatches, | ||
| js: ['/inpage/js/inpage.js'], | ||
| js: [...(metamaskCompatibilityMode ? ['/inpage/js/metamaskCompatibilityMode.js'] : []), '/inpage/js/inpage.js'], | ||
| runAt: 'document_start', |
There was a problem hiding this comment.
This MAIN-world registration now prepends '/inpage/js/metamaskCompatibilityMode.js' to the 'inpage' content script when MetaMask compatibility mode is active. Chrome requires MAIN-world injected scripts to be listed in the manifest's web_accessible_resources (which is why inpage.js is declared there). The new prelude was added to app/manifestV2.json's web_accessible_resources but NOT to app/manifestV3.json. As a result, on Chrome/MV3 the prelude injection is refused, the TheInterceptor.metamaskCompatibilityMode global is never set, and compatibility mode (EIP-6963 announcement replacement, isMetaMask property at load, web3 globals) silently fails despite the setting being enabled; in the worst case Chrome rejects the whole MAIN-world registration update. Add "inpage/js/metamaskCompatibilityMode.js" to app/manifestV3.json's web_accessible_resources resources array (mirroring the existing inpage.js entry).
|
/review |
There was a problem hiding this comment.
CI Agent Review
The MetaMask compatibility mode feature is functionally coherent and free of security issues (no injection vector, signing/consent bypass, or data exposure found across the Defender, Bug Hunter, and Security reviews). The remaining concern is structural: the injection strategy is duplicated rather than abstracted. In document_start.ts the setup/insert/remove scaffolding is now repeated across the compat and standard branches while the injection decision reads from a runtime globalThis symbol that depends on a separately-deployed prelude having run first in the correct world. The 'inject prelude before inpage.js' rule is also hand-encoded in three independent injection paths plus the manifest/bundler/inline scripts and the settings layer, so any future change to the strategy must be reproduced in lockstep across several files.
| container.insertBefore(scriptTag, container.children[1]) | ||
| container.removeChild(scriptTag) | ||
| const metamaskCompatibilityMode = Reflect.get(globalThis, Symbol.for('TheInterceptor.metamaskCompatibilityMode')) === true | ||
| if (metamaskCompatibilityMode) { |
There was a problem hiding this comment.
Structural concern: the script setup/insert/remove scaffolding (createElement, set async/src/textContent, insertBefore container.children[1], removeChild) is now duplicated four times across the compat and standard branches, diverging between src and textContent and between the two inpage file references. This duplicated structural logic must be kept consistent by hand; a future change to script creation or the insert/remove mechanics would have to be reproduced in every site and could easily drift. Consider extracting injectExternalScript(url) / injectInlineScript(content) helpers. Note also that this branch now reads its behavior from a shared runtime global (Reflect.get(globalThis, Symbol.for('TheInterceptor.metamaskCompatibilityMode'))), which silently falls back to the non-compat path if the separately-deployed prelude has not run first in the expected world.
| @@ -57,7 +58,7 @@ export const updateContentScriptInjectionStrategyManifestV3 = async () => { | |||
| allFrames: true, | |||
| matches: injectableSitesWildcard, | |||
| excludeMatches, | |||
There was a problem hiding this comment.
Structural concern: the rule 'when compatibility mode is on, inject metamaskCompatibilityMode.js immediately before inpage.js in the correct world' is now hand-encoded in multiple independent places — this MAIN-world registration, the MV2 injectLogic executeScript sequence, document_start.ts, plus the manifest web-accessible resources, bundler runtime entrypoints, and inline-inpage-document-start.mts — and the settings layer (changeSettings / importSettings) invokes the re-registration for the MV3 path. Any evolution of the injection strategy (renaming/add a prelude, reordering, changing world selection) must be modified in lockstep across all of these sites. A single module owning the page-world script list/order consumed by all injection paths would confine this knowledge to one layer and avoid the new bidirectional dependency between background/popupMessageHandlers and utils/contentScriptsUpdating.
- Unify page-world injection configuration across Chrome, Firefox, and generated scripts - Preserve MetaMask compatibility mode during inpage initialization
|
/review |
There was a problem hiding this comment.
CI Agent Review
One architectural concern was identified across the review: the new content-script injection configuration module creates a dependency-direction inversion between the build toolchain and the application runtime code. The security-focused reviews (Defender, Security) and Bug Hunter found no newly-introduced exploitable vulnerabilities, security regressions, or runtime bugs; the MetaMask compatibility mode feature routes through the existing interception pipeline and the 'isMetamask' → 'isMetaMask' change is a correctness fix, not a regression.
| ] as const | ||
| } | ||
|
|
||
| export const inpageRuntimeEntrypointPaths = [ |
There was a problem hiding this comment.
Architectural concern: this module is both build-only configuration and shipped runtime code. inpageRuntimeEntrypointPaths (and its consumers in build/bundler.mts ~line 5 and scripts/inline-inpage-document-start.mts ~line 4) exists solely so the bundler knows which inpage entrypoints to compile/rewrite, yet it lives inside the app's runtime utility layer and ships (until tree-shaken) in the extension bundle alongside the runtime-needed getPageWorldScriptPaths/getManifestV2IsolatedWorldInjections. This inverts the layer ownership: the build toolchain now reaches into app/ts/utils/ for entrypoint knowledge, so any future move/rename of that file silently breaks the build, while bundling must be careful not to ship a module created only to serve build tooling. Two responsibilities (which scripts the extension injects vs which entrypoints the bundler compiles) are conflated in one module. Recommendation: keep the genuinely runtime-shared path/order config where it is, but move the build-only inpageRuntimeEntrypointPaths list into the build layer (build/ or scripts/), deriving it in bundler.mts from the runtime config only where sync is required.
- Keep inpage runtime entrypoint paths rooted correctly during bundling - Avoid duplicating the app directory in generated paths
|
/review |
There was a problem hiding this comment.
CI Agent Review
The MetaMask compatibility-mode feature contains a functional regression reported by the Bug Hunter agent: in compatibility mode the Interceptor suppresses its own eip6963:announceProvider and only surfaces itself by re-labeling a real MetaMask announcement, so when no MetaMask wallet is present the Interceptor becomes undiscoverable via EIP-6963. Other agents (Defender, Security, Architect) reported no security, injection, or architectural findings.
| private readonly onPageLoad = () => { | ||
| const interceptorMessageListener = this | ||
| function announceProvider() { | ||
| if (interceptorMessageListener.replaceMetaMaskEip6963AnnouncementsAtPageLoad) return |
There was a problem hiding this comment.
Functional regression: in MetaMask compatibility mode this guard suppresses the Interceptor's own eip6963:announceProvider entirely. The only way the Interceptor surfaces itself to dapps in that state is the replaceMetaMaskAnnouncementForCompatibilityMode capture handler, which swaps in the Interceptor provider only when a real MetaMask wallet actually emits an eip6963:announceProvider event (it returns early when no MetaMask announcement is present). For a user who enables compatibility mode without a real MetaMask wallet installed (the common case — injectEthereumIntoWindow falls through to useNoSigner so the Interceptor acts as its own wallet), no MetaMask announcement exists to be replaced, so a dapp performing eip6963:requestProvider/announceProvider discovery receives zero announcements and cannot connect. Previously the Interceptor always announced itself regardless of compatibility mode. The replacement of a real MetaMask announcement should supplement, not replace, the Interceptor's own announcement. The new tests only cover compatibility mode with a MetaMask announcement present, so this no-wallet path is untested but reachable.
- Preserve MetaMask announcements in compatibility mode - Cover announcements with and without a MetaMask wallet
|
/review |
There was a problem hiding this comment.
CI Agent Review
Architecture concern: the requirement to re-register MV3 content scripts whenever metamaskCompatibilityMode changes is hand-duplicated at two call sites (changeSettings and importSettings) with no structural coupling between the stored setting and the registration it invalidates, and these sites refresh the MV3 path without reloading connected tabs (unlike disableInterceptorForPage). Robustness concern: the MV2 isolated-world injection interpolates a storage-sourced boolean directly into an executeScript code string, which is a latent code-injection footgun if that value ever became non-boolean.
| if (parsedRequest.data.metamaskCompatibilityMode !== undefined) await setMetamaskCompatibilityMode(parsedRequest.data.metamaskCompatibilityMode) | ||
| if (parsedRequest.data.metamaskCompatibilityMode !== undefined) { | ||
| await setMetamaskCompatibilityMode(parsedRequest.data.metamaskCompatibilityMode) | ||
| if (browser.runtime.getManifest().manifest_version === 3) await updateContentScriptInjectionStrategyManifestV3() |
There was a problem hiding this comment.
Duplicated MV3 re-registration: whenever metamaskCompatibilityMode changes, the registered MV3 content scripts must be re-registered because the MAIN-world js list now includes/excludes the metamaskCompatibilityMode.js prelude. This inline manifest-version branch is hand-duplicated in both changeSettings (here) and importSettings (settings.ts). The setting and the external registration it invalidates are coupled, but nothing enforces that coupling: any future code path that mutates metamaskCompatibilityMode (or imports settings for a new purpose) has no structural signal that it must also refresh the MV3 registration, so the two call sites can drift out of step. Note also that, unlike disableInterceptorForPage, this re-registration does not reload connected tabs and only refreshes the MV3 path, relying on MV2's injectLogic to read the setting live on the next navigation. Consider encapsulating the mutation in one place (e.g., a setMetamaskCompatibilityMode that persists and then refreshes the content-script strategy for the current manifest and reloads connected tabs) so callers don't each have to remember the MV3 branch.
| return { method: 'popup_initiate_export_settings_reply', data: { success: false, errorMessage: 'Failed to read the file. It is not a valid interceptor settings file' } } | ||
| } | ||
| await importSettingsAndAddressBook(parsed.value) | ||
| if (browser.runtime.getManifest().manifest_version === 3) await updateContentScriptInjectionStrategyManifestV3() |
There was a problem hiding this comment.
Duplicated MV3 content-script re-registration: after importSettingsAndAddressBook, the MV3 registration is refreshed inline here, duplicating the block already added in changeSettings (popupMessageHandlers.ts). Because this is hand-written at each call site that mutates metamaskCompatibilityMode/imports settings, there is no structural guarantee the registration stays in sync with the stored setting, and the two call sites can fall out of step. Consider encapsulating the invariant so a setting change and the registration refresh it requires are enforced in a single place.
| return [ | ||
| { file: 'vendor/webextension-polyfill/dist/browser-polyfill.js' }, | ||
| { file: `${ inpageScriptDirectory }/listenContentScript.js` }, | ||
| { code: `Reflect.set(globalThis, Symbol.for(${ JSON.stringify(metamaskCompatibilityModeGlobalSymbolKey) }), ${ metamaskCompatibilityMode })` }, |
There was a problem hiding this comment.
Potential code-injection footgun: the MV2 isolated-world injection builds an executeScript code string by interpolating the metamaskCompatibilityMode value read from storage (Reflect.set(globalThis, Symbol.for(...), ${ metamaskCompatibilityMode })). If that value were ever a non-boolean string it would be interpolated verbatim into the code string intended for execution. In practice the value is only ever written by typed boolean setters or a user-imported settings file (reachable by explicit user action equivalent to importing any settings file), so it is not currently exploitable, but the string-interpolation of a storage-sourced value into executable code is a latent robustness concern worth guarding against (e.g. a type guard or more defensive serialization).
- Apply compatibility mode updates across MV2 and MV3 - Preserve the setting during legacy settings imports
|
/review |
There was a problem hiding this comment.
CI Agent Review
A behavioral regression was introduced in the refactored MetaMask announcement-handling logic in app/inpage/ts/inpage.ts. The one-time announcedMetaMaskUuid latch is now set from the case where the announced provider is already the current signer, permanently blocking a later genuine MetaMask wallet from being selected as the signer.
| if (this.announcedMetaMaskUuid !== undefined) return | ||
| if (!canAnnouncedMetaMaskReplaceSigner(this.signerName)) return | ||
| if (provider === this.announcedMetaMaskProvider && info.uuid === this.announcedMetaMaskUuid) return true | ||
| if (!this.acceptingAnnouncedMetaMaskProviders) return false |
There was a problem hiding this comment.
Behavioral regression in useMetaMaskAnnouncement: the one-time announcedMetaMaskUuid latch is now set in the branch where the announced provider is already the current signer (provider === this.signerWindowEthereumProvider with signerName === 'MetaMask'). In the base implementation this was an early return that did NOT set the latch. As a result, the first time the current (possibly spoofed/polyfilled or isMetaMask: true-marked) provider announces itself as MetaMask, the latch is written, and any later distinct genuine MetaMask announcement is rejected by the if (this.announcedMetaMaskUuid !== undefined) return false guard because announcedMetaMaskProvider/announcedMetaMaskUuid no longer match. This disables the security-relevant signer fallback that would otherwise switch to the genuine MetaMask wallet on a discovery retry, leaving the page bound to a provider the user does not actually control.
- Add regression coverage for genuine signer discovery
|
/review |
|
/review |
There was a problem hiding this comment.
CI Agent Review
The changeset adds a MetaMask-compatibility mode (a conditional page-world prelude + EIP-6963 announcement replacement) and refactors the content-script injection strategy. Security and correctness reviews found no exploitable defect and no malicious behavior; the one substantive concern is architectural. In MV2 non-compat mode the built artifact injects the page-world provider inline rather than through the shared getPageWorldScriptPaths(false) external list, so the abstraction that is supposed to be the single source of truth for page-world scripts is bypassed on that path and only authoritative for MV3 and the MV2 compat branch.
| for (const scriptPath of pageWorldScriptPathsByCompatibilityMode.disabled) injectExternalScript(scriptPath) | ||
| } else injectInlineScript(_content) | ||
| } | ||
| checkAndThrowRuntimeLastError() |
There was a problem hiding this comment.
Architectural concern (dual source of truth): getPageWorldScriptPaths(false) returns ['inpage.js'] and is shared by the MV3 registerContentScripts list and the build-time marker substitution into document_start. However, in the built MV2 artifact _content has already been replaced with the inline inpage source, so this non-compat branch (else injectInlineScript(_content)) injects the page-world provider inline and never loads the disabled external list. Only the compat-on MV2 branch and MV3 are truly driven by the shared function. As more page-world preludes are added (the stated purpose of the compat-prelude mechanism), a maintainer editing getPageWorldScriptPaths will reasonably expect MV2 non-compat to pick it up, but it will not. Recommend making the non-compat MV2 path also fall through to the disabled external list (so getPageWorldScriptPaths is authoritative in both modes), or explicitly document why MV2 non-compat intentionally diverges from the shared list.
- Remove inline inpage script injection from document-start setup - Update tests for external script loading in MV2 and MV3
|
/review |
There was a problem hiding this comment.
CI Agent Review
One architectural concern was raised: the new shared configuration module in the utils layer re-enters the background layer to read settings, which the Architect identifies as a dependency-direction inversion that the diff extends into a shared, cross-layer contract.
| @@ -0,0 +1,18 @@ | |||
| import { getInterceptorDisabledSites, getMetamaskCompatibilityMode, getSettings } from '../background/settings.js' | |||
There was a problem hiding this comment.
Architectural concern (medium confidence): this new module lives in the utils leaf layer but imports getInterceptorDisabledSites, getMetamaskCompatibilityMode, and getSettings from ../background/settings.js, wiring up a background → utils → background dependency hop. Both the settings-import feature (popupMessageHandlers/settings.ts) and the injection-strategy orchestrator (contentScriptInjectionStrategy.ts) now reach background settings by routing through this util, which the Architect argues inverts the intended layer direction and will make the import seam harder to change without disturbing call sites in two layers. Consider relocating this to the background layer with contentScriptsUpdating (utils) depending on it, rather than making the util the shared entry point into background storage.
|
/review |
There was a problem hiding this comment.
CI Agent Review
The consolidated review surfaced a single structural concern: the MetaMask compatibility-mode bootstrap is implemented by two independent mechanisms (a real page-world script metamaskCompatibilityMode.js and a template-literal code string in the MV2 isolated-world bootstrap) whose behavior must remain semantically identical, and the page-world script path set produced by getPageWorldScriptPaths is duplicated across both manifests, the bundler entrypoint list, and the marker-inlined document_start.ts. Adding any future page-world script requires touching contentScriptInjectionArtifacts.ts, both manifests, build/bundler.mts, and the build-time inlining script in lockstep. The design mitigates this coupling with a central artifact module and sync tests, but the duplicated symbol-key / boolean-serialization and script-path logic is a maintainability cost that should be kept explicit in future changes. All security and correctness reviews found no exploitable defects in the changeset.
| @@ -0,0 +1,23 @@ | |||
| const inpageScriptDirectory = 'inpage/js' | |||
There was a problem hiding this comment.
Maintainability concern: the compatibility-mode flag is now set through two independent mechanisms that must be kept in lockstep — (1) the real page-world script metamaskCompatibilityMode.js (Reflect.set(globalThis, Symbol.for(key), true), consumed in the MAIN world and injected as an external script by document_start.ts in MV2), and (2) the template-literal code string built in this module (Reflect.set(globalThis, Symbol.for(key), enabled)) executed as an isolated-world executeScript injection. The page-world script path set from getPageWorldScriptPaths is also duplicated across manifestV2.json, manifestV3.json, build/bundler.mts, and document_start.ts. Adding a future page-world script requires synchronized edits across all of these sites. The central artifact module and the sync tests mitigate this, but the symbol-key / boolean-serialization and path duplication remain a genuine coupling surface worth documenting as part of this refactor.
- Centralize generated compatibility-mode assignment and script paths - Keep page-world and isolated-world bootstraps consistent
|
/review |
There was a problem hiding this comment.
CI Agent Review
One architectural concern was identified: the new shared module app/ts/utils/contentScriptInjectionArtifacts.ts straddles the build and runtime layers, coupling the build tooling to a runtime source tree module while the runtime module simultaneously carries build-templating concerns. Implementation is currently safe (the module is import-free and static), but the entanglement is a layer-boundary tension introduced by this diff. No security, correctness, or injection issues were found by the other agents.
| @@ -0,0 +1,29 @@ | |||
| const inpageScriptDirectory = 'inpage/js' | |||
There was a problem hiding this comment.
Layer-coupling concern: this module serves both build-time entrypoint generation (build/bundler.mts derives inpageRuntimeEntrypointPaths from getPageWorldScriptPaths(true)) and build-time templating (scripts/inline-inpage-document-start.mts imports the marker constants and symbol key), while also being consumed at runtime by contentScriptsUpdating.ts for live MV2/MV3 injection registration. This creates bidirectional entanglement between the build layer and the runtime layer via a module co-located in the runtime tree. If getPageWorldScriptPaths ever accreted environment-sensitive logic, the build would silently break; and the runtime module carries build-templating concerns into the app tree. The need for the new test that statically asserts this module has zero imports to keep it from dragging runtime modules into the build is a strong signal it lives in the wrong layer. Consider moving this source-of-truth config to a build-owned location (e.g. build/ or scripts/) that the runtime imports from, giving a one-directional dependency. Moderate confidence — the implementation is currently correct.
|
/review |
There was a problem hiding this comment.
CI Agent Review
No security or correctness issues found. The new MetaMask compatibility mode (page-world script injection, EIP-6963 announcement replacement) preserves existing guards, the stored compatibility value cannot inject arbitrary code, script paths are build-time constants, and the injection-strategy/import refactors are behavior-preserving. The only flagged concern (asynchronous external-script insertion in document_start.ts) was not substantiated by the reviewing agent as a definite runtime failure, so it is not reported as a finding.
…-eip6963-message # Conflicts: # app/inpage/ts/inpage.ts # app/ts/background/popupMessageHandlerRegistries/settings.ts # app/ts/background/popupMessageHandlers.ts # app/ts/background/settings.ts # test/tests/inpageSignerBridge.test.ts # test/tests/refreshHomeDataRpcStatus.test.ts
|
/review |
There was a problem hiding this comment.
CI Agent Review
Two issues were identified in this changeset. First, the new MetaMask compatibility mode handling is asymmetric between manifest strategies: MV2 coerces the stored metamaskCompatibilityMode value with === true before generating the bootstrap assignment, while the MV3 path uses plain truthiness in getPageWorldScriptPaths. A truthy non-boolean stored value (e.g. the string "true", which getMetamaskCompatibilityMode() returns unchanged) silently enables MetaMask compatibility mode on Chromium/MV3 (injecting the prelude that assigns true and activating window.ethereum.isMetaMask, the web3 shim, and EIP-6963 announcement replacement) while the same value is coerced to false on Firefox/MV2 — divergent behavior for identical configuration. Second, the build pipeline now imports a runtime TypeScript module (app/ts/config/contentScriptInjectionArtifacts.ts) to derive its entrypoint list, fusing build configuration and runtime behavior in a single module that ships inside the extension bundle, creating a new dependency direction that did not exist at the base commit.
|
|
||
| // These paths drive runtime registration, MV2 document-start generation, and bundler entrypoints; static manifest copies are validated against this list in contentScriptsUpdating.test.ts. | ||
| export function getPageWorldScriptPaths(metamaskCompatibilityMode: boolean): readonly string[] { | ||
| return [ |
There was a problem hiding this comment.
MV3/MV2 asymmetry in handling a non-boolean stored compatibility flag. getPageWorldScriptPaths uses plain truthiness (metamaskCompatibilityMode ? [metamaskCompatibilityModeScriptPath] : []) while the MV2 path (getMetamaskCompatibilityModeGlobalAssignmentSource) coerces with metamaskCompatibilityMode === true. Because getMetamaskCompatibilityMode() returns the raw stored value (only ?? false, no boolean sanitization) and getContentScriptInjectionConfiguration() forwards it verbatim, a truthy non-boolean value (e.g. the string "true") on Chromium/MV3 injects metamaskCompatibilityMode.js, which always assigns Reflect.set(globalThis, Symbol.for("TheInterceptor.metamaskCompatibilityMode"), true). Combined with metamaskCompatibilityModeAtPageLoad in inpage.ts, this silently activates MetaMask compatibility (isMetaMask, web3 shim, EIP-6963 announcement replacement) without the user enabling it. The identical stored value on Firefox/MV2 is coerced to false, so the same profile behaves differently per platform. Additionally, the forwarded raw string causes the connected_to_signer reply parser (which requires typeof metamaskCompatibilityMode === 'boolean') to reject, producing a signer discovery error. Coerce the stored value to a boolean (as done for MV2) before it reaches getPageWorldScriptPaths.
| import * as url from 'node:url' | ||
| import * as fs from 'node:fs' | ||
| import * as ts from 'typescript' | ||
| import { getPageWorldScriptPaths } from '../app/ts/config/contentScriptInjectionArtifacts.ts' |
There was a problem hiding this comment.
The build pipeline now imports a runtime TypeScript module (getPageWorldScriptPaths from app/ts/config/contentScriptInjectionArtifacts.ts) to derive its entrypoint list. This is a new dependency direction that did not exist at the base commit: the build was previously a closed, self-contained pipeline. The same symbol now means two different things in two contexts — getPageWorldScriptPaths(true) means 'scripts to inject when compatibility mode is active' to runtime code in contentScriptsUpdating.ts, but the bundler relies on its return value to mean 'the set of entrypoint files the build must emit'. A future runtime-justified change to that function silently changes what the build produces, and the module ships inside the runtime bundle so build tooling executes app source at build time. Injection knowledge also remains duplicated across this module, both manifests, the bundler, and the inline generator, held together by tests rather than a single authoritative source. Consider keeping the shared path/symbol constants importable by both runtime and build, but moving the build-only source generators and marker-templating into scripts/, and having the bundler read its entrypoint list from the same config that drives them.
…-eip6963-message # Conflicts: # app/ts/background/popupMessageHandlers.ts # app/ts/background/popupMessageHandlers/websiteAccess.ts # app/ts/utils/contentScriptsUpdating.ts
|
/review |
There was a problem hiding this comment.
CI Agent Review
Critical MV2 (Firefox) regression: page-world scripts are inserted into the DOM and synchronously removed before they can load, so inpage.js (and the MetaMask compatibility prelude when enabled) never executes, leaving the Interceptor provider non-functional in the MV2 build. Additionally, the content-script injection refresh policy is applied inconsistently: the settings path unconditionally reloads all connected tabs even when the compatibility mode did not change, while the import path diff-checks the configuration first.
|
/review |
There was a problem hiding this comment.
CI Agent Review
Three issues were found. (1) MV3/Chrome: toggling MetaMask compatibility mode passes a changed js array to chrome.scripting.updateContentScripts(), which ignores js/css changes on already-registered content scripts, so the metamaskCompatibilityMode.js prelude is never added/removed and the reloaded tabs keep the stale registration; the tests' mocks model updateContentScripts as overwriting js, masking this in a real browser. (2) MV2/Firefox: page-world scripts are now injected as external async <script src> elements instead of one inlined synchronous script, so window.ethereum is no longer synchronously available at document_start (dapps reading it in early head scripts will see undefined), and a load failure is silent because interceptorInjected is already set and the error listener only removes the element with no retry. (3) In inpage.ts, the page-load snapshot replaceMetaMaskEip6963AnnouncementsAtPageLoad and the runtime-mutable metamaskCompatibilityMode can diverge if the mode changes after page load without a reload, producing contradictory EIP-6963/provider behavior with no test covering the divergence.
|
/review |
There was a problem hiding this comment.
CI Agent Review
The changeset introduces a single abstraction for "what affects content-script injection" — ContentScriptInjectionConfiguration with a snapshot/compare/refresh coordinator (contentScriptInjectionConfiguration.ts + contentScriptInjectionStrategy.ts) — but the pre-existing website-access update path was not migrated onto it, leaving two parallel change-detection and refresh paths for the same conceptual event. websiteAccessUpdating.ts still re-implements its own set-equality predicate and drives its own refresh directly through updateContentScriptInjectionStrategy(), so a change to interceptorDisabledSites produces different observable behavior than the compatibility-mode path (no tab reload, no MV3 registration-failure short-circuit). Future injection-affecting settings must be threaded through both paths in lockstep.
|
/review |
There was a problem hiding this comment.
CI Agent Review
Global MetaMask compatibility mode introduces page-triggerable impersonation: enabling it (globally, for all sites) installs a capture-phase EIP-6963 listener on every page that substitutes the Interceptor provider for any 'io.metamask' announcement and can adopt a page-supplied provider as the signing provider during discovery. Separately, setInterceptorDisabledForWebsite now replaces stored website metadata (title/icon) rather than preserving it; the injection-strategy refactor creates a background↔utils dependency cycle and a non-atomic update path in which a failed MV3 re-registration leaves stored settings diverged from the live registration.
| private pendingSignerAddressRequest: Promise<SignerAccountsResolution> | undefined = undefined | ||
|
|
||
| public constructor() { | ||
| window.addEventListener('eip6963:announceProvider', this.replaceMetaMaskAnnouncementForCompatibilityMode, { capture: true }) |
There was a problem hiding this comment.
Security issue: enabling the (global) MetaMask compatibility mode registers a permanent capture-phase eip6963:announceProvider listener at document start that matches MetaMask's rdns and replaces every such announcement with the Interceptor's provider, regardless of the announcement's origin. Because the capture listener runs before page scripts and the signer-adoption path is open during discovery (eth_accounts/eth_requestAccounts with no external signer), a malicious page can dispatch its own announceProvider cycle with a page-constructed provider that satisfies the signer interface; the Interceptor will adopt that page-supplied provider as the signing provider (useMetaMaskAnnouncement) and, via announcedMetaMaskUuid/announcedMetaMaskProvider, lock out later genuine MetaMask announcements. Subsequent wallet operations, including transactions approved in the Interceptor popup, are then forwarded to attacker-controlled code. The signer-adoption mechanism predates this diff, but the permanent capture listener and the unconditional global re-branding of MetaMask announcements are introduced here. Additional concern: the mode is global rather than per-site, so one setting enable causes every visited site (without any per-site authorization) to expose window.ethereum.isMetaMask === true and a MetaMask EIP-6963 identity, a confused-deputy/phishing vector in a wallet whose purpose is preventing confused signing. Consider scoping the impersonation per-site (or to an explicit allowlist) and requiring explicit user confirmation before a page-supplied provider is adopted as the signing provider.
|
|
||
| export async function setInterceptorDisabledForWebsite(website: Website, interceptorDisabled: boolean) { | ||
| return await updateWebsiteAccessAndContentScriptInjectionStrategy((previousWebsiteAccess) => { | ||
| export async function setInterceptorDisabledForWebsite(websiteTabConnections: WebsiteTabConnections, website: Website, interceptorDisabled: boolean) { |
There was a problem hiding this comment.
Regression: the update callback below now replaces the stored website object wholesale with the Website from the DisableInterceptor message instead of preserving the stored metadata (the previous { ...previousAccess, interceptorDisabled } retained title/icon). Sibling update paths (setAccess, updateKnownWebsiteMetadata) merge metadata rather than replacing it, and sanitizeWebsiteAccess/sanitizeStoredWebsiteIcon drop non-data: icons. If the popup toggles 'interceptor disabled' before tab metadata finished loading (or after a favicon/title lookup failure), the stored websiteAccess entry is rewritten with title/icon undefined, so the Website Access list and address-access UI lose the previously resolved name/icon until some later flow merges it back. Preserve the existing stored metadata when only toggling the disabled flag.
| import { checkAndThrowRuntimeLastError, getHostWithPort, getTabIfExists, isMissingBrowserTargetError } from './requests.js' | ||
| import { reportLocalRecoveryBestEffort, reportUnexpectedError } from './errors.js' | ||
| import { getManifestV2IsolatedWorldInjections, getPageWorldScriptPaths } from '../config/contentScriptInjectionArtifacts.js' | ||
| import { getContentScriptInjectionConfiguration } from '../background/contentScriptInjectionConfiguration.js' |
There was a problem hiding this comment.
Architectural concern: this import makes the content-script injection configuration reach from the utils leaf layer up into the background layer, while the new background/contentScriptInjectionStrategy.ts reaches down into utils/contentScriptsUpdating.ts for the manifest mechanics. The feature now spans background/strategy → utils/contentScriptsUpdating → background/contentScriptInjectionConfiguration in both dependency directions, so the two layers are mutually entangled around 'what should be injected' and neither module can be changed or tested without holding the other layer's contract in mind. Consider keeping the pure derivation (which settings feed injection, how to compare configs) in a config/utility layer and having the background strategy coordinator call down into it, preserving a single background → config → utils direction.
| import { updateContentScriptInjectionConfigurationAndReloadTabsIfChanged } from './contentScriptInjectionStrategy.js' | ||
|
|
||
| export async function setMetamaskCompatibilityMode(websiteTabConnections: WebsiteTabConnections, metamaskCompatibilityMode: boolean) { | ||
| await updateContentScriptInjectionConfigurationAndReloadTabsIfChanged(websiteTabConnections, async () => await persistMetamaskCompatibilityMode(metamaskCompatibilityMode)) |
There was a problem hiding this comment.
Non-atomic failure mode: setMetamaskCompatibilityMode persists the new value first, then refreshes the content-script injection strategy. If the MV3 re-registration fails, refreshContentScriptInjectionStrategyAndReloadConnectedTabs rolls back the runtime registration and skips the tab reload, but the new value is already stored — storage and the live injection strategy silently diverge until some unrelated refresh reconciles them. Previously this setting was only a persisted flag with no strategy/reload coupling; folding it into ContentScriptInjectionConfiguration widens the partial-failure divergence to a user-visible wallet behavior. Consider making the persist + strategy refresh atomic (e.g., roll back the stored value when the registration refresh fails).
This branch has not been deployed
1 participant
Summary
Validation
bun run test— 1,209 passed, 0 failedbun run setup-chromebun run typecheckbun run lintFinal project review found no High, Medium, or Low issues (96/100). The Chrome bundle was rebuilt successfully; a live browser communication check was not rerun after the main merge because the integration behavior is covered by the content-script, inpage bridge, generated-output, and settings-refresh regression suites.