Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/gator-permissions-snap/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ const profileSyncManager = createProfileSyncManager({
storage: profileSyncOptions.keyStorageOptions,
},
),
ethereumProvider: ethereum,
});

const userEventDispatcher = new UserEventDispatcher();
Expand Down Expand Up @@ -189,6 +190,9 @@ const boundRpcHandlers: {
rpcHandler.getPermissionOffers.bind(rpcHandler),
[RpcMethod.PermissionsProviderGetGrantedPermissions]:
rpcHandler.getGrantedPermissions.bind(rpcHandler),
[RpcMethod.PermissionsProviderSubmitRevocation]: async (
params?: JsonRpcParams,
) => rpcHandler.submitRevocation(params as Json),
};

/**
Expand Down
196 changes: 180 additions & 16 deletions packages/gator-permissions-snap/src/profileSync/profileSync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
LimitExceededError,
ParseError,
UnsupportedMethodError,
type SnapsEthereumProvider,
} from '@metamask/snaps-sdk';
import { z } from 'zod';

Expand All @@ -31,6 +32,7 @@ const MAX_STORAGE_SIZE_BYTES = 400 * 1024; // 400kb limit as documented
const zStoredGrantedPermission = z.object({
permissionResponse: zPermissionResponse,
siteOrigin: z.string().min(1, 'Site origin cannot be empty'),
isRevoked: z.boolean().default(false),
});

/**
Expand Down Expand Up @@ -100,17 +102,46 @@ export type ProfileSyncManager = {
storeGrantedPermissionBatch: (
storedGrantedPermission: StoredGrantedPermission[],
) => Promise<void>;
updatePermissionRevocationStatus: (
permissionContext: Hex,
isRevoked: boolean,
) => Promise<void>;
updatePermissionRevocationStatusWithPermission: (
existingPermission: StoredGrantedPermission,
isRevoked: boolean,
) => Promise<void>;
checkDelegationDisabledOnChain: (
delegationHash: Hex,
chainId: Hex,
delegationManagerAddress: Hex,
) => Promise<boolean>;
};

export type StoredGrantedPermission = {
permissionResponse: PermissionResponse;
siteOrigin: string;
isRevoked: boolean;
};

/**
* Generates an object key for the permission response stored in profile sync.
* @param permissionContext - The encoded delegation(ie. permissions context).
* @returns The object key by concatenating the delegation hashes.
*/
export function generateObjectKey(permissionContext: Hex): Hex {
const delegations = decodeDelegations(permissionContext);
const hashes = delegations.map((delegation) =>
hashDelegation(delegation).slice(2),
);

return `0x${hashes.join('')}`;
}

export type ProfileSyncManagerConfig = {
auth: JwtBearerAuth;
userStorage: UserStorage;
isFeatureEnabled: boolean;
ethereumProvider: SnapsEthereumProvider;
};

/**
Expand All @@ -122,7 +153,7 @@ export function createProfileSyncManager(
config: ProfileSyncManagerConfig,
): ProfileSyncManager {
const FEATURE = 'gator_7715_permissions';
const { auth, userStorage, isFeatureEnabled } = config;
const { auth, userStorage, isFeatureEnabled, ethereumProvider } = config;
const unConfiguredProfileSyncManager = {
getAllGrantedPermissions: async () => {
logger.debug('unConfiguredProfileSyncManager.getAllGrantedPermissions()');
Expand All @@ -143,22 +174,27 @@ export function createProfileSyncManager(
'unConfiguredProfileSyncManager.storeGrantedPermissionBatch()',
);
},
updatePermissionRevocationStatus: async (_: Hex, __: boolean) => {
logger.debug(
'unConfiguredProfileSyncManager.updatePermissionRevocationStatus()',
);
},
updatePermissionRevocationStatusWithPermission: async (
_: StoredGrantedPermission,
__: boolean,
) => {
logger.debug(
'unConfiguredProfileSyncManager.updatePermissionRevocationStatusWithPermission()',
);
},
checkDelegationDisabledOnChain: async (_: Hex, __: Hex, ___: Hex) => {
logger.debug(
'unConfiguredProfileSyncManager.checkDelegationDisabledOnChain()',
);
return false; // Default to not disabled when feature is disabled
},
};

/**
* Generates an object key for the permission response stored in profile sync.
* @param permissionContext - The encoded delegation(ie. permissions context).
* @returns The object key by concatenating the delegation hashes.
*/
function generateObjectKey(permissionContext: Hex): Hex {
const delegations = decodeDelegations(permissionContext);
const hashes = delegations.map((delegation) =>
hashDelegation(delegation).slice(2),
);

return `0x${hashes.join('')}`;
}

/**
* Authenticates the user with profile sync.
*
Expand All @@ -167,7 +203,7 @@ export function createProfileSyncManager(
try {
await auth.getAccessToken();
} catch (error) {
logger.error('Error fetching access token');
logger.error('Error fetching access token:', error);
throw error;
}
}
Expand Down Expand Up @@ -306,6 +342,131 @@ export function createProfileSyncManager(
}
}

/**
* Updates the revocation status of a granted permission in profile sync.
*
* @param permissionContext - The context of the granted permission to update.
* @param isRevoked - The new revocation status.
* @throws InvalidInputError if the permission is not found.
*/
async function updatePermissionRevocationStatus(
permissionContext: Hex,
isRevoked: boolean,
): Promise<void> {
try {
await authenticate();

const existingPermission = await getGrantedPermission(permissionContext);
if (!existingPermission) {
throw new InvalidInputError(
`Permission not found for permission context: ${permissionContext}`,
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Permission Context Mislabeling

The error message in updatePermissionRevocationStatus incorrectly labels the permissionContext parameter as a "delegation hash." This is misleading, as permissionContext represents an encoded permission context, and creates an inconsistency with other error messages that correctly use "permission context."

Fix in Cursor Fix in Web

}

await updatePermissionRevocationStatusWithPermission(
existingPermission,
isRevoked,
);
} catch (error) {
logger.error('Error updating permission revocation status');
throw error;
}
}

/**
* Updates the revocation status of a granted permission when you already have the permission object.
* This is an optimized version that avoids re-fetching the permission.
*
* @param existingPermission - The existing permission object.
* @param isRevoked - The new revocation status.
*/
async function updatePermissionRevocationStatusWithPermission(
existingPermission: StoredGrantedPermission,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@V00D00-child V00D00-child Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

isRevoked: boolean,
): Promise<void> {
try {
logger.debug('Profile Sync: Updating permission revocation status:', {
existingPermission,
isRevoked,
});

await authenticate();

const updatedPermission: StoredGrantedPermission = {
...existingPermission,
isRevoked,
};

logger.debug(
'Profile Sync: Created updated permission object:',
updatedPermission,
);

await storeGrantedPermission(updatedPermission);
logger.debug('Profile Sync: Successfully stored updated permission');
} catch (error) {
logger.error(
'Error updating permission revocation status with existing permission:',
error,
);
throw error;
}
}

/**
* Checks if a delegation is disabled on-chain by calling the DelegationManager contract.
* @param delegationHash - The hash of the delegation to check.
* @param chainId - The chain ID in hex format.
* @param delegationManagerAddress - The address of the DelegationManager contract.
* @returns True if the delegation is disabled, false otherwise.
*/
async function checkDelegationDisabledOnChain(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

delegationHash: Hex,
chainId: Hex,
delegationManagerAddress: Hex,
): Promise<boolean> {
try {
logger.debug('Checking delegation disabled status on-chain', {
delegationHash,
chainId,
delegationManagerAddress,
});

// Encode the function call data for disabledDelegations(bytes32)
const functionSelector = '0x2d40d052'; // keccak256("disabledDelegations(bytes32)").slice(0, 10)
const encodedParams = delegationHash.slice(2).padStart(64, '0'); // Remove 0x and pad to 32 bytes
const callData = `${functionSelector}${encodedParams}`;

const result = await ethereumProvider.request<Hex>({
method: 'eth_call',
params: [
{
to: delegationManagerAddress,
data: callData,
},
'latest',
],
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Invalid eth_call Parameter Causes Failures

The checkDelegationDisabledOnChain function calls eth_call with chainId as the third parameter. The standard Ethereum JSON-RPC eth_call method expects [callObject, blockParameter] or an optional stateOverrideSet as its third parameter. Passing chainId here is non-standard and will likely cause the call to fail or behave unexpectedly with most providers.

Fix in Cursor Fix in Web

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (!result) {
logger.warn('No result from contract call');
return false;
}

// Parse the boolean result (32 bytes, last byte is the boolean value)
const isDisabled =
result !==
'0x0000000000000000000000000000000000000000000000000000000000000000';

logger.debug('Delegation disabled status result', { isDisabled });
return isDisabled;
} catch (error) {
logger.error('Error checking delegation disabled status on-chain', error);
// In case of error, assume not disabled to avoid blocking legitimate operations
return false;
}
}

This comment was marked as outdated.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Chain ID Ignored in Delegation Check

The checkDelegationDisabledOnChain function receives a chainId parameter but doesn't use it in the eth_call request. This causes the delegation status to be checked on the ethereumProvider's currently connected chain instead of the specified chain, potentially leading to incorrect validation results for permissions.

Fix in Cursor Fix in Web

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Chain ID Ignored in Delegation Check

The checkDelegationDisabledOnChain function receives a chainId but doesn't use it when making the eth_call. This means the contract call executes on the ethereumProvider's currently connected chain, which might not match the intended chainId. This can lead to incorrect delegation status checks and flawed revocation decisions.

Fix in Cursor Fix in Web

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web


/**
* Feature flag to disable profile sync feature until message-signing-snap v1.1.2 released in MM 12.18: https://github.com/MetaMask/metamask-extension/pull/32521.
*/
Expand All @@ -315,6 +476,9 @@ export function createProfileSyncManager(
getGrantedPermission,
storeGrantedPermission,
storeGrantedPermissionBatch,
updatePermissionRevocationStatus,
updatePermissionRevocationStatusWithPermission,
checkDelegationDisabledOnChain,
}
: unConfiguredProfileSyncManager;
}
10 changes: 8 additions & 2 deletions packages/gator-permissions-snap/src/rpc/permissions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ const allowedPermissionsByOrigin: { [origin: string]: string[] } = {
RpcMethod.PermissionsProviderGetPermissionOffers,
],
}),
metamask: [RpcMethod.PermissionsProviderGetGrantedPermissions],
metamask: [
RpcMethod.PermissionsProviderGetGrantedPermissions,
RpcMethod.PermissionsProviderSubmitRevocation,
],
};

/**
Expand All @@ -22,5 +25,8 @@ export const isMethodAllowedForOrigin = (
origin: string,
method: string,
): boolean => {
return allowedPermissionsByOrigin[origin]?.includes(method) ?? false;
const isAllowed =
allowedPermissionsByOrigin[origin]?.includes(method) ?? false;

return isAllowed;
};
Loading
Loading