From 7f62763a2e9888eb629f21c5bf120b358dbdb855 Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Thu, 18 Jun 2026 13:57:22 -0400 Subject: [PATCH 1/3] fix(sidebar): refresh popup allowList after grantAccess succeeds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In sidebar mode, `openSigningWindow` reuses the existing sidebar window across consecutive signing flows (e.g. `signMessage` → `requestAccess` → `signMessage` continuation) via `SIDEBAR_NAVIGATE` over the long-lived port. The popup React tree and its redux store are preserved, so `useIsDomainListedAllowed` keeps reading the pre-grant `state.settings.allowList` even though the backend just wrote the new domain to `localStore`. Result: after granting access for the first time from a sidebar, SignMessage / SignAuthEntry views render "domain not in allowlist" even though the dApp is now connected. Have the `grantAccess` thunk refresh settings after the backend write so popup-side redux picks up the new allowList entry. Popup windows opened via `browser.windows.create` already mount fresh redux and refetch — this just makes the sidebar surface behave the same way. Best-effort: a failed `loadSettings` doesn't reject the grant. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/popup/ducks/__tests__/access.test.ts | 126 ++++++++++++++++++ extension/src/popup/ducks/access.ts | 26 +++- 2 files changed, 150 insertions(+), 2 deletions(-) create mode 100644 extension/src/popup/ducks/__tests__/access.test.ts diff --git a/extension/src/popup/ducks/__tests__/access.test.ts b/extension/src/popup/ducks/__tests__/access.test.ts new file mode 100644 index 0000000000..b7e91e7920 --- /dev/null +++ b/extension/src/popup/ducks/__tests__/access.test.ts @@ -0,0 +1,126 @@ +import { combineReducers, configureStore } from "@reduxjs/toolkit"; + +import { + grantAccess as internalGrantAccess, + loadSettings as internalLoadSettings, +} from "@shared/api/internal"; +import { DEFAULT_AUTO_LOCK_TIMEOUT_MINUTES } from "@shared/constants/autoLock"; +import { SettingsState } from "@shared/api/types"; +import { reducer as authReducer } from "../accountServices"; +import { reducer as settingsReducer } from "../settings"; +import { grantAccess } from "../access"; + +jest.mock("@shared/api/internal", () => ({ + ...jest.requireActual("@shared/api/internal"), + grantAccess: jest.fn(), + loadSettings: jest.fn(), +})); + +const TESTNET = { + network: "TESTNET", + networkName: "Testnet", + networkUrl: "https://horizon-testnet.stellar.org", + networkPassphrase: "Test SDF Network ; September 2015", +}; + +const makeStore = () => + configureStore({ + reducer: combineReducers({ auth: authReducer, settings: settingsReducer }), + preloadedState: { + auth: { + allAccounts: [], + migratedAccounts: [], + applicationState: "APPLICATION_STARTED", + hasPrivateKey: true, + publicKey: "GBTEST", + connectingWalletType: "NONE", + bipPath: "", + tokenIdList: [], + error: "", + accountStatus: "IDLE", + isAccountMismatch: false, + }, + } as any, + }); + +describe("grantAccess thunk", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + // Regression: in sidebar mode the popup React tree (and its redux + // store) is reused across consecutive signing flows because + // openSigningWindow's sidebar branch just `postMessage`s a + // SIDEBAR_NAVIGATE to the existing port. Without an explicit refresh + // here, the post-grant SignMessage / SignAuthEntry view reads a stale + // popup-side allowList and renders "not connected" even though the + // backend just wrote the new domain to localStore. + it("refreshes popup-side settings (and allowList) after a successful grantAccess", async () => { + (internalGrantAccess as jest.Mock).mockResolvedValueOnce(undefined); + (internalLoadSettings as jest.Mock).mockResolvedValueOnce({ + allowList: { Testnet: { GBTEST: ["dapp.example"] } }, + isDataSharingAllowed: true, + isMemoValidationEnabled: true, + isHideDustEnabled: true, + isOpenSidebarByDefault: false, + autoLockTimeoutMinutes: DEFAULT_AUTO_LOCK_TIMEOUT_MINUTES, + networkDetails: TESTNET, + networksList: [], + isRpcHealthy: true, + isSorobanPublicEnabled: false, + isNonSSLEnabled: false, + isExperimentalModeEnabled: false, + isHashSigningEnabled: false, + assetsLists: {}, + settingsState: SettingsState.SUCCESS, + userNotification: { enabled: false, message: "" }, + }); + + const store = makeStore(); + await (store.dispatch as any)( + grantAccess({ url: "https://dapp.example", uuid: "uuid-1" }), + ); + + expect(internalGrantAccess).toHaveBeenCalledWith({ + url: "https://dapp.example", + uuid: "uuid-1", + }); + expect(internalLoadSettings).toHaveBeenCalledTimes(1); + + const state = store.getState() as any; + expect(state.settings.allowList).toEqual({ + Testnet: { GBTEST: ["dapp.example"] }, + }); + // Order: backend grant runs before settings refresh, so the + // refreshed allowList in redux reflects the just-granted domain. + const grantCall = (internalGrantAccess as jest.Mock).mock + .invocationCallOrder[0]; + const loadCall = (internalLoadSettings as jest.Mock).mock + .invocationCallOrder[0]; + expect(grantCall).toBeLessThan(loadCall); + }); + + // The refresh is best-effort: if loadSettings throws (transient MV3 + // service-worker hiccup) we swallow it rather than rejecting the + // thunk, since the backend grant has already succeeded and a stale + // popup allowList is a strictly better state than failing the grant. + it("swallows a loadSettings error so the grant still fulfills", async () => { + (internalGrantAccess as jest.Mock).mockResolvedValueOnce(undefined); + (internalLoadSettings as jest.Mock).mockRejectedValueOnce( + new Error("transient SW restart"), + ); + const consoleSpy = jest + .spyOn(console, "error") + .mockImplementation(() => undefined); + + const store = makeStore(); + const action = await (store.dispatch as any)( + grantAccess({ url: "https://dapp.example", uuid: "uuid-2" }), + ); + + expect(action.type).toBe("grantAccess/fulfilled"); + expect(internalLoadSettings).toHaveBeenCalledTimes(1); + expect(consoleSpy).toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); +}); diff --git a/extension/src/popup/ducks/access.ts b/extension/src/popup/ducks/access.ts index 185df10e08..1cf3b83eaf 100644 --- a/extension/src/popup/ducks/access.ts +++ b/extension/src/popup/ducks/access.ts @@ -3,18 +3,40 @@ import { createAsyncThunk } from "@reduxjs/toolkit"; import { rejectAccess as internalRejectAccess, grantAccess as internalGrantAccess, + loadSettings as internalLoadSettings, addToken as internalAddToken, signTransaction as internalSignTransaction, signBlob as internalSignBlob, signAuthEntry as internalSignAuthEntry, } from "@shared/api/internal"; import { publicKeySelector } from "popup/ducks/accountServices"; +import { saveSettingsAction } from "popup/ducks/settings"; import { AppState } from "popup/App"; +// After granting access to a dApp, refresh popup-side settings so the +// updated allowList is reflected in redux. Without this, sidebar mode +// reuses the same popup React tree across consecutive signing flows +// (e.g. `signMessage` → `requestAccess` → `signMessage` continuation), +// and `useIsDomainListedAllowed` still reads the pre-grant allowList, +// showing the just-connected dApp as "not connected". Popup windows +// opened via `browser.windows.create` mount fresh redux and refetch on +// their own — the bug is sidebar-specific — but refreshing here makes +// the behaviour consistent across both surfaces. export const grantAccess = createAsyncThunk( "grantAccess", - ({ url, uuid }: { url: string; uuid: string }) => - internalGrantAccess({ url, uuid }), + async ({ url, uuid }: { url: string; uuid: string }, { dispatch }) => { + const result = await internalGrantAccess({ url, uuid }); + try { + const settings = await internalLoadSettings(); + dispatch(saveSettingsAction(settings)); + } catch (e) { + // Refresh is best-effort: a failed reload should not prevent the + // grant from resolving, since the backend write already succeeded. + // Next loadSettings call (e.g. on the next view mount) will sync. + console.error("grantAccess: failed to refresh settings", e); + } + return result; + }, ); export const rejectAccess = createAsyncThunk( From cbcab3003a05d3903f52f729b72358c221f04de8 Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Fri, 26 Jun 2026 10:59:59 -0400 Subject: [PATCH 2/3] fix(access): report grantAccess refresh failures to Sentry The best-effort settings refresh in grantAccess logged failures with console.error, which never reaches Sentry. Use captureException so these transient failures are observable, passing the error object to preserve the stack trace. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/popup/ducks/__tests__/access.test.ts | 19 +++++++++++-------- extension/src/popup/ducks/access.ts | 5 ++++- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/extension/src/popup/ducks/__tests__/access.test.ts b/extension/src/popup/ducks/__tests__/access.test.ts index b7e91e7920..871cae9b86 100644 --- a/extension/src/popup/ducks/__tests__/access.test.ts +++ b/extension/src/popup/ducks/__tests__/access.test.ts @@ -1,5 +1,7 @@ import { combineReducers, configureStore } from "@reduxjs/toolkit"; +import { captureException } from "@sentry/browser"; + import { grantAccess as internalGrantAccess, loadSettings as internalLoadSettings, @@ -16,6 +18,10 @@ jest.mock("@shared/api/internal", () => ({ loadSettings: jest.fn(), })); +jest.mock("@sentry/browser", () => ({ + captureException: jest.fn(), +})); + const TESTNET = { network: "TESTNET", networkName: "Testnet", @@ -106,12 +112,8 @@ describe("grantAccess thunk", () => { // popup allowList is a strictly better state than failing the grant. it("swallows a loadSettings error so the grant still fulfills", async () => { (internalGrantAccess as jest.Mock).mockResolvedValueOnce(undefined); - (internalLoadSettings as jest.Mock).mockRejectedValueOnce( - new Error("transient SW restart"), - ); - const consoleSpy = jest - .spyOn(console, "error") - .mockImplementation(() => undefined); + const loadError = new Error("transient SW restart"); + (internalLoadSettings as jest.Mock).mockRejectedValueOnce(loadError); const store = makeStore(); const action = await (store.dispatch as any)( @@ -120,7 +122,8 @@ describe("grantAccess thunk", () => { expect(action.type).toBe("grantAccess/fulfilled"); expect(internalLoadSettings).toHaveBeenCalledTimes(1); - expect(consoleSpy).toHaveBeenCalled(); - consoleSpy.mockRestore(); + expect(captureException).toHaveBeenCalledWith(loadError, { + extra: { context: "grantAccess: failed to refresh settings" }, + }); }); }); diff --git a/extension/src/popup/ducks/access.ts b/extension/src/popup/ducks/access.ts index 1cf3b83eaf..2782cf5112 100644 --- a/extension/src/popup/ducks/access.ts +++ b/extension/src/popup/ducks/access.ts @@ -1,4 +1,5 @@ import { createAsyncThunk } from "@reduxjs/toolkit"; +import { captureException } from "@sentry/browser"; import { rejectAccess as internalRejectAccess, @@ -33,7 +34,9 @@ export const grantAccess = createAsyncThunk( // Refresh is best-effort: a failed reload should not prevent the // grant from resolving, since the backend write already succeeded. // Next loadSettings call (e.g. on the next view mount) will sync. - console.error("grantAccess: failed to refresh settings", e); + captureException(e, { + extra: { context: "grantAccess: failed to refresh settings" }, + }); } return result; }, From 2b95bd0d2d64a48a8d3ca2146ea0acd592037dce Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Fri, 26 Jun 2026 12:25:14 -0400 Subject: [PATCH 3/3] fix(access): make post-grant settings refresh non-blocking Awaiting the settings reload before the grantAccess thunk resolved delayed GrantAccess's window.close()/route-close. In sidebar mode the reused popup tree then sat on /grant-access long enough for the dApp's follow-up signing request to be treated as interrupting an active signing route (interstitial) and clobbered by the deferred close. Run the refresh fire-and-forget instead: the thunk resolves right after the grant, and redux still updates when the reload lands, re-rendering the next view with the fresh allowList. Sentry reporting of reload failures is preserved. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/popup/ducks/__tests__/access.test.ts | 13 ++++++++++ extension/src/popup/ducks/access.ts | 26 ++++++++++++------- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/extension/src/popup/ducks/__tests__/access.test.ts b/extension/src/popup/ducks/__tests__/access.test.ts index 871cae9b86..5fb11637c9 100644 --- a/extension/src/popup/ducks/__tests__/access.test.ts +++ b/extension/src/popup/ducks/__tests__/access.test.ts @@ -49,6 +49,11 @@ const makeStore = () => } as any, }); +// The settings refresh is fire-and-forget (not awaited by the thunk), so it +// lands a tick after the thunk fulfills. Flush pending microtasks before +// asserting on its effects. +const flushPromises = () => new Promise((resolve) => setTimeout(resolve, 0)); + describe("grantAccess thunk", () => { beforeEach(() => { jest.clearAllMocks(); @@ -86,6 +91,9 @@ describe("grantAccess thunk", () => { await (store.dispatch as any)( grantAccess({ url: "https://dapp.example", uuid: "uuid-1" }), ); + // The refresh dispatches saveSettingsAction asynchronously after the + // thunk fulfills; wait for it to land before asserting on redux state. + await flushPromises(); expect(internalGrantAccess).toHaveBeenCalledWith({ url: "https://dapp.example", @@ -120,8 +128,13 @@ describe("grantAccess thunk", () => { grantAccess({ url: "https://dapp.example", uuid: "uuid-2" }), ); + // The grant fulfills regardless of the refresh outcome. expect(action.type).toBe("grantAccess/fulfilled"); expect(internalLoadSettings).toHaveBeenCalledTimes(1); + + // The failure is reported to Sentry asynchronously, after the thunk + // has already fulfilled. + await flushPromises(); expect(captureException).toHaveBeenCalledWith(loadError, { extra: { context: "grantAccess: failed to refresh settings" }, }); diff --git a/extension/src/popup/ducks/access.ts b/extension/src/popup/ducks/access.ts index 2782cf5112..81326a170f 100644 --- a/extension/src/popup/ducks/access.ts +++ b/extension/src/popup/ducks/access.ts @@ -27,17 +27,23 @@ export const grantAccess = createAsyncThunk( "grantAccess", async ({ url, uuid }: { url: string; uuid: string }, { dispatch }) => { const result = await internalGrantAccess({ url, uuid }); - try { - const settings = await internalLoadSettings(); - dispatch(saveSettingsAction(settings)); - } catch (e) { - // Refresh is best-effort: a failed reload should not prevent the - // grant from resolving, since the backend write already succeeded. - // Next loadSettings call (e.g. on the next view mount) will sync. - captureException(e, { - extra: { context: "grantAccess: failed to refresh settings" }, + // Fire-and-forget: do NOT await the refresh before resolving. Awaiting it + // delayed GrantAccess's window.close()/route-close, which in sidebar mode + // kept the reused popup tree on /grant-access long enough for the dApp's + // follow-up signing request to be treated as interrupting an active + // signing route (showing an interstitial) and then clobbered by the + // deferred close. Letting the refresh run async still updates redux when + // it resolves, re-rendering the next view with the fresh allowList. + internalLoadSettings() + .then((settings) => dispatch(saveSettingsAction(settings))) + .catch((e) => { + // Best-effort: a failed reload must not break the grant, since the + // backend write already succeeded. The next loadSettings call (e.g. + // on the next view mount) will sync. + captureException(e, { + extra: { context: "grantAccess: failed to refresh settings" }, + }); }); - } return result; }, );