Added New RPC for Marking Delegation as Revoked #171

hanzel98 · 2025-09-23T16:14:48Z

Description

This PR implements the isRevoked flag feature for permissions stored in profile sync to ensure off-chain storage stays in sync with on-chain contract state after revocation transactions reach finality.

Key Changes:

Added isRevoked flag to the StoredGrantedPermission type that defaults to false when permissions are stored
Implemented new RPC method permissionsProvider_submitRevocation accessible only by the MetaMask origin to update the revocation status
Added validation and on-chain verification to ensure revocations are only processed for delegations that are actually disabled on-chain
Added filter in the method to pull the permissions, it only pulls the ones that are not revoked.
Implemented a validation to skip the transaction to disable a delegation if it is already disabled.

The RPC method validates the delegation hash, fetches the existing permission, verifies the delegation is disabled on-chain via the delegation manager contract, and updates the permission's isRevoked status to true.

Related issues

Fixes: #357

Manual testing steps

Build and run the gator-permissions-snap
Grant a permission through the snap interface
Revoke the permission on-chain through the delegation manager contract
Call the permissionsProvider_submitRevocation RPC method with the delegation hash
Verify the permission's isRevoked flag is set to true in profile sync
Attempt to call the RPC method with an invalid delegation hash and verify it rejects
Attempt to call the RPC method for a delegation that is not disabled on-chain and verify it rejects

Screenshots/Recordings

Before

Permissions stored in profile sync had no revocation tracking
No mechanism to sync off-chain permission state with on-chain revocations
Once granted, permissions appeared active in profile sync even after on-chain revocation

After

Permissions now include an isRevoked boolean flag (defaults to false)
New RPC method permissionsProvider_submitRevocation allows MetaMask to update revocation status
Off-chain permission state accurately reflects on-chain delegation status
Proper validation ensures only legitimate revocations are processed

Pre-merge author checklist

I've followed MetaMask 7715 Permissions Snaps Contributor Docs and MetaMask 7715 Permissions Snaps Coding Standards.
I've completed the PR template to the best of my ability
I've included tests if applicable
I've documented my code using JSDoc format if applicable

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Additional Technical Details:

Note

Adds isRevoked tracking to stored permissions and a MetaMask-only permissionsProvider_submitRevocation RPC with on-chain validation and filtering support.

Profile Sync (src/profileSync/profileSync.ts):
- Add isRevoked to StoredGrantedPermission; default false when storing.
- New helpers to update revocation status: updatePermissionRevocationStatus(…) and optimized updatePermissionRevocationStatusWithPermission(…).
- Introduce on-chain check checkDelegationDisabledOnChain(…) using eth_call; inject ethereumProvider via createProfileSyncManager config.
- Export generateObjectKey and improve auth error logging.
RPC Layer:
- New method RpcMethod.PermissionsProviderSubmitRevocation and binding in src/index.ts.
- Restrict access in src/rpc/permissions.ts to metamask origin.
- Extend getGrantedPermissions(params?) to support filters: isRevoked, siteOrigin, chainId, delegationManager.
- Implement submitRevocation(params) in src/rpc/rpcHandler.ts: validate params, fetch permission, verify delegation disabled on-chain, then mark isRevoked=true.
Validation (src/utils/validate.ts):
- Add validateRevocationParams (hex permissionContext) with Zod-based errors.
Integration:
- Pass ethereum to createProfileSyncManager in src/index.ts.
Tests:
- Update/add unit and e2e tests for new RPC, filtering, isRevoked persistence, and on-chain check plumbing.

^{Written by Cursor Bugbot for commit 5059b2b. This will update automatically on new commits. Configure here.}

socket-security · 2025-10-25T00:15:39Z

No dependency changes detected. Learn more about Socket for GitHub.

👍 No dependency changes detected in pull request

…ions

cursor · 2025-10-26T05:05:16Z

packages/gator-permissions-snap/src/profileSync/profileSync.ts

+      // In case of error, assume not disabled to avoid blocking legitimate operations
+      return false;
+    }
+  }


Bug: Network Mismatch in Delegation Check

The checkDelegationDisabledOnChain function accepts a chainId parameter but doesn't use it in the eth_call request. This can result in the contract call executing on the wrong network, leading to incorrect delegation status validation.

MoMannn · 2025-10-27T13:29:44Z

packages/gator-permissions-snap/src/profileSync/profileSync.ts

+          'latest',
+          chainId,
+        ],
+      });


Yeah this is definetly a problem. Do we have any function like: findNetworkClientIdByChainId in the snap ? This exists in extension.

MoMannn · 2025-10-27T13:34:59Z

packages/gator-permissions-snap/src/rpc/rpcHandler.ts

-    const grantedPermission =
-      await profileSyncManager.getAllGrantedPermissions();
-    return grantedPermission as Json[];
+  const getGrantedPermissions = async (params?: Json): Promise<Json> => {


Can we add a definition for params with Zod and then validate the params like in grantPermission.

MoMannn · 2025-10-27T13:38:44Z

packages/gator-permissions-snap/src/rpc/rpcHandler.ts

+      signerMeta,
+    });
+
+    // Check if the delegation is actually disabled on-chain


comment does not match code

MoMannn · 2025-10-27T13:39:51Z

packages/gator-permissions-snap/src/rpc/rpcHandler.ts

+
+    if (!isDelegationDisabled) {
+      throw new InvalidInputError(
+        `Delegation ${delegationHash} is not disabled on-chain. Cannot process revocation.`,


If its not disabled then its an error? So the logic for this is to only update profile sync and not start a revocation process?

MoMannn · 2025-10-27T13:42:23Z

packages/gator-permissions-snap/src/rpc/rpcHandler.ts

+
+    // Check if any delegation is disabled on-chain
+    // For now, we'll check the first delegation. This might need adjustment based on business logic
+    const firstDelegation = delegations[0];


shouldn't we then just iterate through if there is more then one and validate all?

MoMannn · 2025-10-27T13:43:07Z

packages/gator-permissions-snap/src/rpc/rpcHandler.ts

+      );
+    }
+
+    logger.debug(


Maybe a bit too much logging here?

MoMannn · 2025-10-27T13:45:18Z

packages/gator-permissions-snap/src/profileSync/profileSync.ts

+   * @param delegationManagerAddress - The address of the DelegationManager contract.
+   * @returns True if the delegation is disabled, false otherwise.
+   */
+  async function checkDelegationDisabledOnChain(


Actually maybe it would be better for this to be in: blockchainMetadataClient which also has a retry and check we are on the correct chain?

V00D00-child · 2025-10-30T17:32:01Z

packages/gator-permissions-snap/src/profileSync/profileSync.ts

+   * @param isRevoked - The new revocation status.
+   */
+  async function updatePermissionRevocationStatusWithPermission(
+    existingPermission: StoredGrantedPermission,


Can we remove updatePermissionRevocationStatusWithPermission() function?

Consumers of the revocation flag feature are restricted to the MetaMask client and should only need to call updatePermissionRevocationStatus(permissionContext, isRevoked), which are provided as params on the permissionsProvider_submitRevocation rpc method.

When we allow the consumer to update a permission by providing an existingPermission, the consumer could unintentionally corrupt the user storage (i.e., changing other fields in the permission data).

There is also no check to ensure the item is in the store, which morphs the expected behavior of an 'update' function into a proxy for creating a new store entry. The only field the consumer can update is the isRevoked flag, but this function escalates that privilege to allow mutation of other properties.

I have created a branch against your PR with my suggestions. Here is the commit: c628a5a

V00D00-child · 2025-10-30T17:51:02Z

packages/gator-permissions-snap/src/rpc/rpcHandler.ts

+   * @param params - The parameters for the revocation.
+   * @returns Success confirmation.
+   */
+  const submitRevocation = async (params: Json): Promise<Json> => {


I think the submitRevocation() function can be simplified further by moving onchain check business logic to the updatePermissionRevocationStatus() function in profile sync and then calling updatePermissionRevocationStatus() here.

We should check onchain revoke status anytime we attempt to update the isRevoke flag on a storage entry in profile sync, not just on rpc calls.

hanzel98 requested a review from a team as a code owner September 23, 2025 16:14

hanzel98 marked this pull request as draft September 23, 2025 16:14

hanzel98 self-assigned this Sep 23, 2025

This comment was marked as outdated.

Sign in to view

hanzel98 force-pushed the chore/is-revoke-flag branch from 02f55e2 to d4be06e Compare October 3, 2025 20:13

hanzel98 added 6 commits October 24, 2025 14:08

chore: added new rpc for marking delegation as revoked

0befaa4

fix: get delegation by hash

e89c045

fix: permissions in plural

83fa627

docs: added console logs

9630eb5

chore: changed to permission context

853f216

chore: added filters for permissions, reverted logger

52417dc

hanzel98 force-pushed the chore/is-revoke-flag branch from ef7bcf7 to 52417dc Compare October 24, 2025 20:40

hanzel98 added 2 commits October 24, 2025 18:02

chore: code cleanup

1b22e64

Update yarn.lock to match package.json dependencies

e1a0c92

hanzel98 added 3 commits October 24, 2025 18:23

Restore yarn.lock from main branch to fix dependency sync

37937ad

Update yarn.lock with missing dependencies (esbuild, tsx, get-tsconfig)

d613dbc

Revert yarn.lock and packages/shared/package.json to main branch vers…

11203b2

…ions

hanzel98 marked this pull request as ready for review October 26, 2025 02:03

This comment was marked as outdated.

Sign in to view

chore: deleted try-catch

00ab24f

This comment was marked as outdated.

Sign in to view

chore: added chain id in the call for disabledDelegation

75316d9

This comment was marked as outdated.

Sign in to view

chore: reverting chainid

b1ac613

This comment was marked as outdated.

Sign in to view

chore: fixed permission type

ca62aaa

This comment was marked as outdated.

Sign in to view

test: improved name of variable

f36c5b0

This comment was marked as outdated.

Sign in to view

hanzel98 mentioned this pull request Oct 26, 2025

Add Gator Permissions Revocation Flow MetaMask/metamask-extension#37209

Open

chore: error msg delegation hash to permissions context

168e4e7

This comment was marked as outdated.

Sign in to view

test: updated tests

5059b2b

cursor bot reviewed Oct 26, 2025

View reviewed changes

MoMannn requested changes Oct 27, 2025

View reviewed changes

V00D00-child requested changes Oct 30, 2025

View reviewed changes

V00D00-child mentioned this pull request Oct 30, 2025

[DRAFT]Chore/is revoke flag suggestions #214

Closed

Uh oh!

Added New RPC for Marking Delegation as Revoked #171

Are you sure you want to change the base?

Added New RPC for Marking Delegation as Revoked #171

Uh oh!

Conversation

hanzel98 commented Sep 23, 2025 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

This comment was marked as outdated.

Uh oh!

socket-security bot commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

cursor bot Oct 26, 2025

Choose a reason for hiding this comment

Bug: Network Mismatch in Delegation Check

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

V00D00-child Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hanzel98 commented Sep 23, 2025 •

edited by cursor bot

Loading

socket-security bot commented Oct 25, 2025 •

edited

Loading

V00D00-child Oct 30, 2025 •

edited

Loading