Skip to content

Commit 98e65ea

Browse files
authored
fix: use pairingTopic for permission revocation in removeSession() (MetaMask#27945)
## Description `removeSession()` calls `permissionsController.revokeAllPermissions(session.topic)`, but permissions are stored under `session.pairingTopic` (used as `channelId` / `origin` when `requestPermissions` is called in `_handleSessionProposal` — lines 468, 581). Since `topic` and `pairingTopic` are always different values, the revocation was targeting a non-existent subject and silently failing (caught by the surrounding try/catch). Stale permission entries accumulated in the `PermissionController` for every disconnected WC session. This changes the revocation key to `session.pairingTopic` to match how permissions are created. ## Related issues Fixes: https://consensyssoftware.atlassian.net/browse/WAPI-1361 ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: Fixed - Fixed removeSession() using session.topic instead of session.pairingTopic when revoking WalletConnect permissions, which caused stale permission entries to persist in the PermissionController ([MetaMask#27945](MetaMask#27945)) ## Manual testing steps 1. Connect a dapp via WalletConnect 2. Inspect `PermissionController.state.subjects` — the dapp's entry should be keyed by `pairingTopic` 3. Disconnect the session (long-press in Settings > Experimental > WC Sessions) 4. Inspect `PermissionController.state.subjects` — the entry should now be removed ## Pre-merge author checklist - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes how WalletConnect v2 session teardown revokes permissions, which impacts permission state cleanup and could affect disconnect behavior if the wrong identifier is used elsewhere. Scope is small and covered by an updated unit test. > > **Overview** > Fixes `WC2Manager.removeSession()` to revoke permissions using `session.pairingTopic` (and logs that identifier) instead of `session.topic`, preventing stale `PermissionController` subjects from accumulating after disconnects. > > Updates the `WalletConnectV2` unit test to assert `revokeAllPermissions` is called with the pairing topic. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 8ba16c9. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent eb330c5 commit 98e65ea

2 files changed

Lines changed: 3 additions & 3 deletions

File tree

app/core/WalletConnect/WalletConnectV2.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -864,7 +864,7 @@ describe('WC2Manager', () => {
864864
},
865865
topic: 'test-topic',
866866
});
867-
expect(revokeAllPermissionsSpy).toHaveBeenCalledWith('test-topic');
867+
expect(revokeAllPermissionsSpy).toHaveBeenCalledWith('test-pairing');
868868
});
869869
});
870870

app/core/WalletConnect/WalletConnectV2.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -381,10 +381,10 @@ export class WC2Manager {
381381
}
382382
).PermissionController;
383383
DevLogger.log(
384-
`WC2::removeSession revokeAllPermissions for ${session.topic}`,
384+
`WC2::removeSession revokeAllPermissions for ${session.pairingTopic}`,
385385
permissionsController.state,
386386
);
387-
permissionsController.revokeAllPermissions(session.topic);
387+
permissionsController.revokeAllPermissions(session.pairingTopic);
388388
} catch (err) {
389389
DevLogger.log(`WC2::removeSession error while disconnecting`, err);
390390
}

0 commit comments

Comments
 (0)