Process permission requests sequentially (#150)

MoMannn · web-flow · commit 1063b2267cf7 · 2025-09-04T09:53:30.000+02:00
* handle permission request sequentially

* add test and linter updates

* only store permissions if all permissions in a batch are approved

* Restrict in kernal that only one rpc can be handled at a time.

* Allow on testing page for multiple permissions to be requested in parallel.

* linter fixes

* remove unnecessary check
diff --git a/packages/gator-permissions-snap/src/rpc/rpcHandler.ts b/packages/gator-permissions-snap/src/rpc/rpcHandler.ts
@@ -1,3 +1,4 @@
+import type { PermissionResponse } from '@metamask/7715-permissions-shared/types';
 import { logger } from '@metamask/7715-permissions-shared/utils';
 import { InvalidInputError, type Json } from '@metamask/snaps-sdk';
 
@@ -58,30 +59,37 @@ export function createRpcHandler(config: {
     const { permissionsRequest, siteOrigin } =
       validatePermissionRequestParam(params);
 
-    const responses = await Promise.all(
-      permissionsRequest.map(async (request) => {
-        const handler =
-          permissionHandlerFactory.createPermissionHandler(request);
+    const permissionsToStore: {
+      permissionResponse: PermissionResponse;
+      siteOrigin: string;
+    }[] = [];
 
-        const permissionResponse =
-          await handler.handlePermissionRequest(siteOrigin);
+    // First, process all permissions to collect responses and validate all are approved
+    for (const request of permissionsRequest) {
+      const handler = permissionHandlerFactory.createPermissionHandler(request);
 
-        if (!permissionResponse.approved) {
-          throw new InvalidInputError(permissionResponse.reason);
-        }
+      const permissionResponse =
+        await handler.handlePermissionRequest(siteOrigin);
 
-        if (permissionResponse.response) {
-          await profileSyncManager.storeGrantedPermission({
-            permissionResponse: permissionResponse.response,
-            siteOrigin,
-          });
-        }
+      if (!permissionResponse.approved) {
+        throw new InvalidInputError(permissionResponse.reason);
+      }
 
-        return permissionResponse.response;
-      }),
-    );
+      permissionsToStore.push({
+        permissionResponse: permissionResponse.response,
+        siteOrigin,
+      });
+    }
+
+    // Only after all permissions have been successfully processed, store them all in batch
+    if (permissionsToStore.length > 0) {
+      await profileSyncManager.storeGrantedPermissionBatch(permissionsToStore);
+    }
 
-    return responses as Json[];
+    // Return the permission responses
+    return permissionsToStore.map(
+      (permission) => permission.permissionResponse as Json,
+    );
   };
 
   /**
diff --git a/packages/gator-permissions-snap/test/rpc/rpcHandler.test.ts b/packages/gator-permissions-snap/test/rpc/rpcHandler.test.ts
@@ -287,6 +287,67 @@ describe('RpcHandler', () => {
       expect(mockHandler.handlePermissionRequest).toHaveBeenCalledTimes(2);
     });
 
+    it('should process multiple permission requests sequentially (no concurrency)', async () => {
+      const secondPermissionRequest = {
+        ...VALID_PERMISSION_REQUEST,
+        chainId: '0x2' as const,
+      };
+
+      const secondResponse = {
+        ...VALID_PERMISSION_RESPONSE,
+        chainId: '0x2' as const,
+      };
+
+      const request: Json = {
+        permissionsRequest: [
+          VALID_PERMISSION_REQUEST,
+          secondPermissionRequest,
+        ] as unknown as Json[],
+        siteOrigin: TEST_SITE_ORIGIN,
+      };
+
+      let resolveFirst: (value: unknown) => void;
+      const firstPromise = new Promise((resolve) => {
+        resolveFirst = resolve;
+      });
+
+      const firstMockHandler: jest.Mocked<PermissionHandlerType> = {
+        handlePermissionRequest: jest
+          .fn()
+          .mockImplementation(
+            async () => firstPromise as unknown as Promise<any>,
+          ),
+      } as unknown as jest.Mocked<PermissionHandlerType>;
+
+      const secondMockHandler: jest.Mocked<PermissionHandlerType> = {
+        handlePermissionRequest: jest.fn().mockImplementation(async () => ({
+          approved: true,
+          response: secondResponse,
+        })),
+      } as unknown as jest.Mocked<PermissionHandlerType>;
+
+      mockHandlerFactory.createPermissionHandler
+        .mockImplementationOnce(() => firstMockHandler)
+        .mockImplementationOnce(() => secondMockHandler);
+
+      const resultPromise = handler.grantPermission(request);
+
+      // Yield to allow the first await to be hit
+      await Promise.resolve();
+      expect(secondMockHandler.handlePermissionRequest).not.toHaveBeenCalled();
+
+      // Resolve the first request and then ensure the second starts
+      // @ts-expect-error - resolveFirst is assigned above
+      resolveFirst({ approved: true, response: VALID_PERMISSION_RESPONSE });
+      const result = await resultPromise;
+
+      expect(firstMockHandler.handlePermissionRequest).toHaveBeenCalledTimes(1);
+      expect(secondMockHandler.handlePermissionRequest).toHaveBeenCalledTimes(
+        1,
+      );
+      expect(result).toStrictEqual([VALID_PERMISSION_RESPONSE, secondResponse]);
+    });
+
     it('should throw an error if orchestrator creation fails', async () => {
       mockHandlerFactory.createPermissionHandler.mockImplementation(() => {
         throw new Error('Failed to create orchestrator');
diff --git a/packages/permissions-kernel-snap/src/index.ts b/packages/permissions-kernel-snap/src/index.ts
@@ -1,5 +1,6 @@
 import { logger } from '@metamask/7715-permissions-shared/utils';
 import {
+  LimitExceededError,
   MethodNotFoundError,
   type Json,
   type JsonRpcParams,
@@ -27,6 +28,10 @@ const boundRpcHandlers: {
     rpcHandler.requestExecutionPermissions.bind(rpcHandler),
 };
 
+// Processing lock to ensure only one RPC request is processed at a time
+// Use a token-based lock to avoid race conditions across async boundaries
+let activeProcessingLock: symbol | null = null;
+
 /**
  * Handle incoming JSON-RPC requests, sent through `wallet_invokeSnap`.
  *
@@ -41,26 +46,48 @@ export const onRpcRequest: OnRpcRequestHandler = async ({
   origin,
   request,
 }) => {
-  logger.info(
-    `Custom request (origin="${origin}"):`,
-    JSON.stringify(request, null, 2),
-  );
-
-  // Use Object.prototype.hasOwnProperty.call() to prevent prototype pollution attacks
-  // This ensures we only access methods that exist on boundRpcHandlers itself
-  if (!Object.prototype.hasOwnProperty.call(boundRpcHandlers, request.method)) {
-    throw new MethodNotFoundError(`Method ${request.method} not found.`);
+  // Check if another request is already being processed
+  if (activeProcessingLock !== null) {
+    logger.warn(
+      `RPC request rejected (origin="${origin}"): another request is already being processed`,
+    );
+    throw new LimitExceededError('Another request is already being processed.');
   }
 
-  // We know that the method exists, so we can cast to NonNullable
-  const handler = boundRpcHandlers[request.method] as NonNullable<
-    (typeof boundRpcHandlers)[string]
-  >;
+  // Acquire the processing lock
+  const myLock = Symbol('processing-lock');
+  activeProcessingLock = myLock;
+
+  try {
+    logger.info(
+      `Custom request (origin="${origin}"):`,
+      JSON.stringify(request, undefined, 2),
+    );
 
-  const result = await handler({
-    siteOrigin: origin,
-    params: request.params as JsonRpcParams,
-  });
+    // Use Object.prototype.hasOwnProperty.call() to prevent prototype pollution attacks
+    // This ensures we only access methods that exist on boundRpcHandlers itself
+    if (
+      !Object.prototype.hasOwnProperty.call(boundRpcHandlers, request.method)
+    ) {
+      throw new MethodNotFoundError(`Method ${request.method} not found.`);
+    }
 
-  return result;
+    // We know that the method exists, so we can cast to NonNullable
+    const handler = boundRpcHandlers[request.method] as NonNullable<
+      (typeof boundRpcHandlers)[string]
+    >;
+
+    const result = await handler({
+      siteOrigin: origin,
+      params: request.params as JsonRpcParams,
+    });
+
+    return result;
+  } finally {
+    // Always release the processing lock we acquired, regardless of success or failure
+    // Only release if we still hold the lock to avoid clobbering a newer lock
+    if (activeProcessingLock === myLock) {
+      activeProcessingLock = null;
+    }
+  }
 };
diff --git a/packages/permissions-kernel-snap/test/end-to-end/index.test.tsx b/packages/permissions-kernel-snap/test/end-to-end/index.test.tsx
@@ -48,5 +48,67 @@ describe('Kernel Snap', () => {
         }
       });
     });
+
+    describe('processing lock', () => {
+      beforeEach(async () => {
+        const { request } = await installSnap({
+          options: {
+            state: {
+              permissionOfferRegistry: {},
+            },
+          },
+        });
+
+        snapRequest = request;
+      });
+
+      it('should reject a concurrent request, and allow a later one', async () => {
+        const requestBody: RequestOptions = {
+          method: 'wallet_requestExecutionPermissions',
+          params: [
+            {
+              chainId: '0x1',
+              signer: {
+                type: 'account',
+                data: {
+                  address: '0x1234567890123456789012345678901234567890',
+                },
+              },
+              permission: {
+                type: 'native-token-transfer',
+                isAdjustmentAllowed: true,
+                data: {
+                  justification: 'Test permission',
+                  allowance: '0x1000',
+                },
+              },
+            },
+          ],
+        };
+
+        // Fire two requests without awaiting the first
+        const first = snapRequest(requestBody);
+        const second = snapRequest(requestBody);
+
+        // Assert the second one is rejected with the internal error that wraps LimitExceededError
+        const secondResponse = await second;
+        expect(secondResponse).toRespondWithError({
+          code: -32005,
+          message: 'Another request is already being processed.',
+          stack: expect.any(String),
+        });
+
+        // Allow the first to settle (it may error due to test setup, which is fine)
+        await first.catch(() => undefined);
+
+        // Make a third request; it must not fail due to the lock
+        const thirdResponse = await snapRequest(requestBody);
+        expect(thirdResponse).not.toRespondWithError({
+          code: -32005,
+          message: 'Another request is already being processed.',
+          stack: expect.any(String),
+        });
+      });
+    });
   });
 });
diff --git a/packages/site/src/pages/index.tsx b/packages/site/src/pages/index.tsx
@@ -135,7 +135,7 @@ const Index = () => {
   const [data, setData] = useState<Hex>('0x');
   const [value, setValue] = useState<bigint>(0n);
   const [receipt, setReceipt] = useState<UserOperationReceipt | null>(null);
-  const [isWorking, setIsWorking] = useState(false);
+  const [pendingPermissionRequests, setPendingPermissionRequests] = useState<Set<string>>(new Set());
 
   const handleChainChange = ({
     target: { value: inputValue },
@@ -175,7 +175,13 @@ const Index = () => {
     if (!delegateAccount) {
       throw new Error('Delegate account not found');
     }
-    setIsWorking(true);
+    
+    // Generate a unique identifier for this redemption request
+    const requestId = `redeem-${Date.now()}-${Math.random()}`;
+    
+    // Add this request to pending requests
+    setPendingPermissionRequests(prev => new Set(prev).add(requestId));
+    
     setReceipt(null);
     setPermissionResponseError(null);
 
@@ -214,8 +220,14 @@ const Index = () => {
       setReceipt(operationReceipt);
     } catch (error) {
       setPermissionResponseError(error as Error);
+    } finally {
+      // Remove this request from pending requests
+      setPendingPermissionRequests(prev => {
+        const newSet = new Set(prev);
+        newSet.delete(requestId);
+        return newSet;
+      });
     }
-    setIsWorking(false);
   };
 
   const handleGrantPermissions = async () => {
@@ -253,7 +265,12 @@ const Index = () => {
       } as const,
     ];
 
-    setIsWorking(true);
+    // Generate a unique identifier for this permission request
+    const requestId = `${type}-${Date.now()}-${Math.random()}`;
+    
+    // Add this request to pending requests
+    setPendingPermissionRequests(prev => new Set(prev).add(requestId));
+    
     setPermissionResponse(null);
     setReceipt(null);
     setPermissionResponseError(null);
@@ -265,8 +282,14 @@ const Index = () => {
     } catch (error) {
       setPermissionResponse(null);
       setPermissionResponseError(error as Error);
+    } finally {
+      // Remove this request from pending requests
+      setPendingPermissionRequests(prev => {
+        const newSet = new Set(prev);
+        newSet.delete(requestId);
+        return newSet;
+      });
     }
-    setIsWorking(false);
   };
 
   const handleCopyToClipboard = () => {
@@ -304,7 +327,6 @@ const Index = () => {
       <Subtitle>
         Get started by installing snaps and sending permissions requests.
       </Subtitle>
-      {isWorking && <p>Loading...</p>}
       <CardContainer>
         {errors.map((error, idx) => (
           <ErrorMessage key={idx}>
@@ -372,7 +394,7 @@ const Index = () => {
             <CustomMessageButton
               $text="Redeem Permission"
               onClick={handleRedeemPermission}
-              disabled={isWorking}
+              disabled={pendingPermissionRequests.size > 0}
             />
           </Box>
         )}
@@ -459,7 +481,6 @@ const Index = () => {
             <CustomMessageButton
               $text="Grant Permission"
               onClick={handleGrantPermissions}
-              disabled={isWorking}
             />
           </Box>
         )}
