Skip to content

fix: revoke permissions for each session in WC2Manager.removeAll()#27944

Merged
adonesky1 merged 4 commits into
mainfrom
ad/fix-removeall-revoke-permissions
Mar 26, 2026
Merged

fix: revoke permissions for each session in WC2Manager.removeAll()#27944
adonesky1 merged 4 commits into
mainfrom
ad/fix-removeall-revoke-permissions

Conversation

@adonesky1

@adonesky1 adonesky1 commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

Description

removeSession() revokes permissions via permissionsController.revokeAllPermissions() after cleanup, but removeAll() did not — leaving stale permission entries in the PermissionController for every removed session.

This adds the missing revokeAllPermissions(session.pairingTopic) call in removeAll().

Note on permission key inconsistency

This PR uses session.pairingTopic as the revocation key because that's the key permissions are created under (origin: channelId where channelId = pairingTopic — see _handleSessionProposal).

However, removeSession() uses session.topic instead (line 377), which is a different value. That call is likely a no-op (silently caught by the surrounding try/catch). This is a pre-existing bug in removeSession — it should probably also use session.pairingTopic. Left as-is to keep this PR scoped.

Related issues

Follow-up to #27932

Manual testing steps

  1. Connect multiple dapps via WalletConnect
  2. Go to Settings > Experimental > WalletConnect Sessions
  3. Verify sessions are listed
  4. Trigger removeAll() (e.g., via wallet reset flow)
  5. Verify permission entries for those sessions are no longer in PermissionController.state.subjects

Pre-merge author checklist


Note

Low Risk
Low risk: small, well-scoped change that only adds missing permission cleanup during WC2Manager.removeAll(), with a unit test covering the new behavior.

Overview
Fixes WC2Manager.removeAll() to also revoke WalletConnect v2 permissions for each active session (via PermissionController.revokeAllPermissions(session.pairingTopic)) before disconnecting, preventing stale permission entries after bulk session removal.

Adds a focused unit test asserting permission revocation is triggered during removeAll(), and logs (without failing) if revocation throws.

Written by Cursor Bugbot for commit e8b281e. This will update automatically on new commits. Configure here.

jiexi and others added 3 commits March 25, 2026 11:03
removeSession() revokes permissions via revokeAllPermissions() after
cleanup, but removeAll() did not — leaving stale permission entries
in the PermissionController for every removed session.
@adonesky1 adonesky1 requested a review from a team as a code owner March 25, 2026 19:26
@github-actions

Copy link
Copy Markdown
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot metamaskbot added the team-wallet-integrations Wallet Integrations team label Mar 25, 2026
Base automatically changed from jl/WAPI-1355/fix-redux-store-leak-wc to main March 25, 2026 21:39
jiexi
jiexi previously approved these changes Mar 26, 2026
…ke-permissions

# Conflicts:
#	app/core/WalletConnect/WalletConnectV2.test.ts
@github-actions github-actions Bot added the risk-medium Moderate testing recommended · Possible bug introduction risk label Mar 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeNetworkAbstractions, SmokeMultiChainAPI
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: medium
  • AI Confidence: 72%
click to see 🤖 AI reasoning details

E2E Test Selection:
The changes are in WalletConnectV2.ts (removeAll method) and its test file. The core change adds permissionsController.revokeAllPermissions(session.pairingTopic) before disconnecting each WalletConnect session during a full session removal. This is a correctness fix ensuring permissions are cleaned up when WC sessions are removed.

Key observations:

  1. No dedicated WalletConnect E2E smoke tests exist in the test suite (confirmed by searching tests/smoke/ and tests/regression/).
  2. The change touches the PermissionController interaction, which is relevant to dApp chain permissions and session management.
  3. SmokeNetworkAbstractions covers chain permission system for dApps (granting/revoking chain access) — directly relevant since this change affects permission revocation during WC session cleanup.
  4. SmokeMultiChainAPI covers CAIP-25 multi-chain session API and wallet_revokeSession — directly relevant since this change ensures permissions are properly revoked when sessions end.
  5. The change is wrapped in try/catch so it's non-breaking if PermissionController throws.
  6. No UI changes, no performance-sensitive paths affected.

The selected tags cover the dApp permission and session management flows most likely to be affected by this change.

Performance Test Selection:
The change only adds a permission revocation call (revokeAllPermissions) in the WalletConnect removeAll() method. This is a cleanup operation that runs when all WC sessions are removed — not in any hot path, rendering loop, or performance-sensitive flow. No performance tests are warranted.

View GitHub Actions results

@adonesky1 adonesky1 added the no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed label Mar 26, 2026
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Mar 26, 2026
@adonesky1 adonesky1 enabled auto-merge March 26, 2026 21:23
@github-actions

Copy link
Copy Markdown
Contributor

E2E Fixture Validation — Schema is up to date
17 value mismatches detected (expected — fixture represents an existing user).
View details

@sonarqubecloud

Copy link
Copy Markdown

@adonesky1 adonesky1 added this pull request to the merge queue Mar 26, 2026
Merged via the queue into main with commit 3e1b3d4 Mar 26, 2026
120 of 124 checks passed
@adonesky1 adonesky1 deleted the ad/fix-removeall-revoke-permissions branch March 26, 2026 22:24
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 26, 2026
@metamaskbot metamaskbot added the release-7.73.0 Issue or pull request that will be included in release 7.73.0 label Mar 26, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

INVALID-PR-TEMPLATE PR's body doesn't match template no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed release-7.73.0 Issue or pull request that will be included in release 7.73.0 risk-medium Moderate testing recommended · Possible bug introduction risk size-S team-wallet-integrations Wallet Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants