Remove broken ethereum properties (#2296)

Removes access to `on` and `removeListener` on the `ethereum` global since these are broken in Snaps anyway.
MetaMask · Mar 20, 2024 · 5f18720 · 5f18720
1 parent 2418d99
commit 5f18720
Show file tree

Hide file tree

Showing 6 changed files with 17 additions and 45 deletions.
diff --git a/packages/snaps-execution-environments/coverage.json b/packages/snaps-execution-environments/coverage.json
@@ -1,6 +1,6 @@
 {
-  "branches": 80.66,
-  "functions": 90.19,
-  "lines": 90.83,
-  "statements": 90.2
+  "branches": 80,
+  "functions": 90.06,
+  "lines": 90.75,
+  "statements": 90.12
 }
diff --git a/packages/snaps-execution-environments/src/common/BaseSnapExecutor.test.browser.ts b/packages/snaps-execution-environments/src/common/BaseSnapExecutor.test.browser.ts
@@ -390,9 +390,6 @@ describe('BaseSnapExecutor', () => {
   it('allows direct access to ethereum public properties', async () => {
     const CODE = `
       module.exports.onRpcRequest = () => {
-        const listener = () => undefined;
-        ethereum.on('accountsChanged', listener);
-        ethereum.removeListener('accountsChanged', listener);
         return ethereum.request({ method: 'eth_blockNumber', params: [] });
       };
     `;
@@ -2415,8 +2412,8 @@ describe('BaseSnapExecutor', () => {
           hasMethods: {
             ethereum: {
               request: true,
-              on: true,
-              removeListener: true,
+              on: false,
+              removeListener: false,
               rpcEngine: false,
             },
             snap: {

diff --git a/packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts b/packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts
@@ -494,23 +494,7 @@ export class BaseSnapExecutor {
       );
     };
 
-    // Proxy target is intentionally set to be an empty object, to ensure
-    // that access to the prototype chain is not possible.
-    const snapGlobalProxy = new Proxy(
-      {},
-      {
-        has(_target: object, prop: string | symbol) {
-          return typeof prop === 'string' && ['request'].includes(prop);
-        },
-        get(_target, prop: keyof StreamProvider) {
-          if (prop === 'request') {
-            return request;
-          }
-
-          return undefined;
-        },
-      },
-    ) as SnapsProvider;
+    const snapGlobalProxy = proxyStreamProvider(request) as SnapsProvider;
 
     return harden(snapGlobalProxy);
   }
@@ -546,7 +530,7 @@ export class BaseSnapExecutor {
       );
     };
 
-    const streamProviderProxy = proxyStreamProvider(provider, request);
+    const streamProviderProxy = proxyStreamProvider(request);
 
     return harden(streamProviderProxy);
   }

diff --git a/packages/snaps-execution-environments/src/common/utils.ts b/packages/snaps-execution-environments/src/common/utils.ts
@@ -54,33 +54,24 @@ export async function withTeardown<Type>(
 }
 
 /**
- * Returns a Proxy that narrows down (attenuates) the fields available on
- * the StreamProvider and replaces the request implementation.
+ * Returns a Proxy that only allows access to a `request` function.
+ * This is useful for replacing StreamProvider with an attenuated version.
  *
- * @param provider - Instance of a StreamProvider to be limited.
- * @param request - Custom attenuated request object.
- * @returns Proxy to the StreamProvider instance.
+ * @param request - Custom attenuated request function.
+ * @returns Proxy that mimics a StreamProvider instance.
  */
-export function proxyStreamProvider(
-  provider: StreamProvider,
-  request: unknown,
-): StreamProvider {
+export function proxyStreamProvider(request: unknown): StreamProvider {
   // Proxy target is intentionally set to be an empty object, to ensure
   // that access to the prototype chain is not possible.
   const proxy = new Proxy(
     {},
     {
       has(_target: object, prop: string | symbol) {
-        return (
-          typeof prop === 'string' &&
-          ['request', 'on', 'removeListener'].includes(prop)
-        );
+        return typeof prop === 'string' && ['request'].includes(prop);
       },
       get(_target, prop: keyof StreamProvider) {
         if (prop === 'request') {
           return request;
-        } else if (['on', 'removeListener'].includes(prop)) {
-          return provider[prop];
         }
 
         return undefined;

diff --git a/packages/snaps-sdk/src/types/provider.test.ts b/packages/snaps-sdk/src/types/provider.test.ts
@@ -5,8 +5,8 @@ import type { SnapsEthereumProvider, SnapsProvider } from './provider';
 describe('SnapsEthereumProvider', () => {
   it('only has the expected methods', () => {
     expectTypeOf<SnapsEthereumProvider>().toHaveProperty('request');
-    expectTypeOf<SnapsEthereumProvider>().toHaveProperty('on');
-    expectTypeOf<SnapsEthereumProvider>().toHaveProperty('removeListener');
+    expectTypeOf<SnapsEthereumProvider>().not.toHaveProperty('on');
+    expectTypeOf<SnapsEthereumProvider>().not.toHaveProperty('removeListener');
     expectTypeOf<SnapsEthereumProvider>().not.toHaveProperty('isConnected');
   });
 });

diff --git a/packages/snaps-sdk/src/types/provider.ts b/packages/snaps-sdk/src/types/provider.ts
@@ -11,7 +11,7 @@ export type SnapsEthereumProvider = Pick<
   BaseProviderInstance,
   // Snaps have access to a limited set of methods. This is enforced by the
   // `proxyStreamProvider` function in `@metamask/snaps-execution-environments`.
-  'request' | 'on' | 'removeListener'
+  'request'
 >;
 
 /**