Skip to content
Merged
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
1 change: 1 addition & 0 deletions packages/multichain-api-middleware/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed

- Add partial permission revoke into `wallet_revokeSession` ([#6668](https://github.com/MetaMask/core/pull/6668))
- Bump `@metamask/chain-agnostic-permission` from `1.0.0` to `1.1.1` ([#6241](https://github.com/MetaMask/core/pull/6241), [#6345](https://github.com/MetaMask/core/pull/6241))
- Bump `@metamask/controller-utils` from `^11.10.0` to `^11.14.0` ([#6069](https://github.com/MetaMask/core/pull/6069), [#6303](https://github.com/MetaMask/core/pull/6303), [#6620](https://github.com/MetaMask/core/pull/6620), [#6629](https://github.com/MetaMask/core/pull/6629))
- Bump `@metamask/network-controller` from `^24.0.0` to `^24.2.0` ([#6148](https://github.com/MetaMask/core/pull/6148), [#6303](https://github.com/MetaMask/core/pull/6303), [#6678](https://github.com/MetaMask/core/pull/6678))
Expand Down
18 changes: 18 additions & 0 deletions packages/multichain-api-middleware/src/handlers/types.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import type {
Caip25CaveatType,
Caip25CaveatValue,
} from '@metamask/chain-agnostic-permission';
import type {
Caveat,
CaveatSpecificationConstraint,
PermissionController,
PermissionSpecificationConstraint,
Expand All @@ -19,3 +24,16 @@ type AbstractPermissionController = PermissionController<
export type GrantedPermissions = Awaited<
ReturnType<AbstractPermissionController['requestPermissions']>
>[0];

export type WalletRevokeSessionHooks = {
revokePermissionForOrigin: (permissionName: string) => void;
updateCaveat: (
target: string,
caveatType: string,
caveatValue: Caip25CaveatValue,
) => void;
getCaveatForOrigin: (
endowmentPermissionName: string,
caveatType: string,
) => Caveat<typeof Caip25CaveatType, Caip25CaveatValue>;
};
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { Caip25EndowmentPermissionName } from '@metamask/chain-agnostic-permission';
import {
Caip25CaveatType,
Caip25EndowmentPermissionName,
} from '@metamask/chain-agnostic-permission';
import {
PermissionDoesNotExistError,
UnrecognizedSubjectError,
Expand All @@ -8,7 +11,10 @@ import type { JsonRpcRequest } from '@metamask/utils';

import { walletRevokeSession } from './wallet-revokeSession';

const baseRequest: JsonRpcRequest & { origin: string } = {
const baseRequest: JsonRpcRequest & {
origin: string;
params: { scopes?: string[] };
} = {
origin: 'http://test.com',
params: {},
jsonrpc: '2.0' as const,
Expand All @@ -20,27 +26,38 @@ const createMockedHandler = () => {
const next = jest.fn();
const end = jest.fn();
const revokePermissionForOrigin = jest.fn();
const updateCaveat = jest.fn();
const getCaveatForOrigin = jest.fn();
const response = {
result: true,
id: 1,
jsonrpc: '2.0' as const,
};
const handler = (request: JsonRpcRequest & { origin: string }) =>
const handler = (
request: JsonRpcRequest & {
origin: string;
params: { scopes?: string[] };
},
) =>
walletRevokeSession.implementation(request, response, next, end, {
revokePermissionForOrigin,
updateCaveat,
getCaveatForOrigin,
});

return {
next,
response,
end,
revokePermissionForOrigin,
updateCaveat,
getCaveatForOrigin,
handler,
};
};

describe('wallet_revokeSession', () => {
it('revokes the the CAIP-25 endowment permission', async () => {
it('revokes the CAIP-25 endowment permission', async () => {
const { handler, revokePermissionForOrigin } = createMockedHandler();

await handler(baseRequest);
Expand All @@ -49,6 +66,87 @@ describe('wallet_revokeSession', () => {
);
});

it('partially revokes the CAIP-25 endowment permission if `scopes` param is passed in', async () => {
const { handler, getCaveatForOrigin, updateCaveat } = createMockedHandler();
getCaveatForOrigin.mockImplementation(() => ({
value: {
optionalScopes: {
'eip155:1': {
accounts: ['eip155:1:0xdeadbeef'],
},
'eip155:5': {
accounts: ['eip155:5:0xdeadbeef'],
},
'eip155:10': {
accounts: ['eip155:10:0xdeadbeef'],
},
},
requiredScopes: {},
},
}));

await handler({ ...baseRequest, params: { scopes: ['eip155:1'] } });
expect(updateCaveat).toHaveBeenCalledWith(
Caip25EndowmentPermissionName,
Caip25CaveatType,
{
optionalScopes: {
'eip155:5': { accounts: ['eip155:5:0xdeadbeef'] },
'eip155:10': { accounts: ['eip155:10:0xdeadbeef'] },
},
requiredScopes: {},
},
);
});

it('not call `updateCaveat` if `scopes` param is passed in with non existing permitted scope', async () => {
const { handler, getCaveatForOrigin, updateCaveat } = createMockedHandler();
getCaveatForOrigin.mockImplementation(() => ({
value: {
optionalScopes: {
'eip155:1': {
accounts: [],
},
},
requiredScopes: {},
},
}));

await handler({ ...baseRequest, params: { scopes: ['eip155:5'] } });
expect(updateCaveat).not.toHaveBeenCalled();
});

it('fully revokes permission when all accounts are removed after scope removal', async () => {
const {
handler,
getCaveatForOrigin,
updateCaveat,
revokePermissionForOrigin,
} = createMockedHandler();
getCaveatForOrigin.mockImplementation(() => ({
value: {
optionalScopes: {
'eip155:1': {
accounts: ['eip155:1:0xdeadbeef'],
},
'eip155:5': {
accounts: ['eip155:5:0xdeadbeef'],
},
},
requiredScopes: {},
},
}));

await handler({
...baseRequest,
params: { scopes: ['eip155:1', 'eip155:5'] },
});
expect(updateCaveat).not.toHaveBeenCalled();
expect(revokePermissionForOrigin).toHaveBeenCalledWith(
Caip25EndowmentPermissionName,
);
});

it('returns true if the CAIP-25 endowment permission does not exist', async () => {
const { handler, response, revokePermissionForOrigin } =
createMockedHandler();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,41 +1,113 @@
import { Caip25EndowmentPermissionName } from '@metamask/chain-agnostic-permission';
import {
Caip25CaveatMutators,
Caip25CaveatType,
Caip25EndowmentPermissionName,
getCaipAccountIdsFromCaip25CaveatValue,
} from '@metamask/chain-agnostic-permission';
import type {
JsonRpcEngineNextCallback,
JsonRpcEngineEndCallback,
} from '@metamask/json-rpc-engine';
import {
CaveatMutatorOperation,
PermissionDoesNotExistError,
UnrecognizedSubjectError,
} from '@metamask/permission-controller';
import { rpcErrors } from '@metamask/rpc-errors';
import type { JsonRpcSuccess, JsonRpcRequest } from '@metamask/utils';

import type { WalletRevokeSessionHooks } from './types';

/**
* Revokes specific session scopes from an existing caveat.
* Fully revokes permission if no accounts remain permitted after iterating through scopes.
*
* @param scopes - Array of scope strings to remove from the caveat.
* @param hooks - The hooks object.
* @param hooks.revokePermissionForOrigin - The hook for revoking a permission for an origin function.
* @param hooks.updateCaveat - The hook used to conditionally update the caveat rather than fully revoke the permission.
* @param hooks.getCaveatForOrigin - The hook to fetch an existing caveat for the origin of the request.
*/
function partialRevokePermissions(
scopes: string[],
hooks: WalletRevokeSessionHooks,
) {
let updatedCaveatValue = hooks.getCaveatForOrigin(
Caip25EndowmentPermissionName,
Caip25CaveatType,
).value;

for (const scopeString of scopes) {
const result = Caip25CaveatMutators[Caip25CaveatType].removeScope(
updatedCaveatValue,
scopeString,
);

// If operation is a Noop, it means a scope was passed that was not present in the permission, so we proceed with the loop
if (result.operation === CaveatMutatorOperation.Noop) {
continue;
}

updatedCaveatValue = result?.value ?? {
requiredScopes: {},
optionalScopes: {},
sessionProperties: {},
isMultichainOrigin: true,
};
}

const caipAccountIds =
getCaipAccountIdsFromCaip25CaveatValue(updatedCaveatValue);

// We fully revoke permission if no accounts are left after scope removal loop.
if (!caipAccountIds.length) {
hooks.revokePermissionForOrigin(Caip25EndowmentPermissionName);
} else {
hooks.updateCaveat(
Caip25EndowmentPermissionName,
Caip25CaveatType,
updatedCaveatValue,
);
}
}
Copy link

Choose a reason for hiding this comment

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

Bug: Permission Revocation Errors and Null Handling

The partialRevokePermissions function has a few issues. When removeScope returns null for a non-existent scope, the nullish coalescing operator ?? incorrectly replaces the entire caveat with an empty object, which can unintentionally revoke all permissions or undo previous successful scope removals. The function also updates the caveat or revokes permission even when no actual changes occur. Finally, accessing .value on the result of getCaveatForOrigin lacks a null check, potentially causing a TypeError if no caveat exists.

Fix in Cursor Fix in Web

Copy link
Member Author

Choose a reason for hiding this comment

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


/**
* Handler for the `wallet_revokeSession` RPC method as specified by [CAIP-285](https://chainagnostic.org/CAIPs/caip-285).
* The implementation below deviates from the linked spec in that it ignores the `sessionId` param
* and instead revokes the singular session for the origin if available. Additionally,
* the handler also does not return an error if there is currently no active session and instead
* returns true which is the same result returned if an active session was actually revoked.
*
* @param _request - The JSON-RPC request object. Unused.
* @param request - The JSON-RPC request object. Unused.
* @param response - The JSON-RPC response object.
* @param _next - The next middleware function. Unused.
* @param end - The end callback function.
* @param hooks - The hooks object.
* @param hooks.revokePermissionForOrigin - The hook for revoking a permission for an origin function.
* @param hooks.updateCaveat - The hook used to conditionally update the caveat rather than fully revoke the permission.
* @param hooks.getCaveatForOrigin - The hook to fetch an existing caveat for the origin of the request.
* @returns Nothing.
*/
async function walletRevokeSessionHandler(
_request: JsonRpcRequest & { origin: string },
request: JsonRpcRequest & {
origin: string;
params: { scopes?: string[] };
},
response: JsonRpcSuccess,
_next: JsonRpcEngineNextCallback,
end: JsonRpcEngineEndCallback,
hooks: {
revokePermissionForOrigin: (permissionName: string) => void;
},
hooks: WalletRevokeSessionHooks,
) {
const {
params: { scopes },
} = request;

try {
hooks.revokePermissionForOrigin(Caip25EndowmentPermissionName);
if (scopes?.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if we pass an empty array of scopes, what should happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

if (scopes?.length) evaluates to false, so we fully revoke the permission. Basically the same as passing an empty params object

Copy link
Contributor

Choose a reason for hiding this comment

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

right, that's what happens right now, but is that what we want? If i say "hey, please remove the following specific scopes from my permission, nothing", should it remove everything? Idk. anyway not worth blocking on and probably not important right now

partialRevokePermissions(scopes, hooks);
} else {
hooks.revokePermissionForOrigin(Caip25EndowmentPermissionName);
}
} catch (err) {
if (
!(err instanceof UnrecognizedSubjectError) &&
Expand All @@ -54,5 +126,7 @@ export const walletRevokeSession = {
implementation: walletRevokeSessionHandler,
hookNames: {
revokePermissionForOrigin: true,
updateCaveat: true,
getCaveatForOrigin: true,
},
};
Loading