Skip to content

Commit f20c40e

Browse files
aaspinwallclaude
aaspinwall
and
claude
authored
fix(addon): stop closing Send tabs when access token has expired (#949)
* fix(addon): stop closing Send tabs when access token has expired getLoginState() treated an expired OIDC access token (short-lived `expires_at`) as logged out and called closeAllTbProTabs(), which removed every send.tb.pro tab — including a Send dashboard tab just opened from the accounts dashboard inside Thunderbird. The session is still valid: the Send web app refreshes the access token transparently via signinSilent() using the refresh_token. Make getLoginState() a read-only probe: report logged-in based on a stored session with a refresh_token, regardless of access-token expiry, and drop the destructive closeAllTbProTabs()/storage-wipe/menuLogout() side effects (real logout already flows through the SIGN_OUT path). Remove the now-unused closeAllTbProTabs() helper. Add menu.test.ts covering the regression. Closes #948 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(send): dedupe OIDC silent refresh and preserve session on transient failure The web app's token-refresh design caused a storm of token/userinfo requests and spurious logouts inside Thunderbird. automaticSilentRenew plus on-demand signinSilent() with no deduplication fired concurrent refreshes that race under refresh-token rotation (losers fail with invalid_grant), and loadUserInfo added a post-refresh userinfo round-trip that can fail in Thunderbird and reject an otherwise-successful refresh; the library's iframe fallback also cannot run there. - automaticSilentRenew: false, loadUserInfo: false (profile claims come from the id_token) to remove uncontrolled renews and the fragile round-trip. - Shared in-flight refreshAccessToken() promise dedupes concurrent callers. - Distinguish genuine auth failures (invalid_grant/login_required/session_expired) — which clear login state and notify the add-on via SIGN_OUT — from transient failures, which preserve the session for a later retry. Adds auth-store.test.ts. Closes #951 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(send): stop uploads firing with an empty ownerId (400 storm) When the session had not populated user.id, the create-entry POST /api/uploads sent ownerId: undefined; JSON.stringify drops the key, the backend Zod schema rejects it with 400 ("ownerId Required"), and the upload retry loop re-fired the whole block repeatedly — the visible signed -> PUT(200) -> uploads(400) storm. Guard uploadItem: if user.id is empty, re-hydrate via populateFromBackend(), and if it is still empty, abort with a clear error instead of sending an ownerId-less body. The Uploader holds the same shared reactive user object, so re-populating also fixes this.user.id inside doUpload. Closes #950 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c3ae0a5 commit f20c40e

5 files changed

Lines changed: 380 additions & 79 deletions

File tree

‎packages/addon/src/menu.ts‎

Lines changed: 22 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -134,57 +134,37 @@ export async function menuLogout() {
134134
});
135135
}
136136

137-
async function closeAllTbProTabs() {
138-
const openWindows = await browser.windows.getAll({ populate: true });
139-
140-
// Close all the tabs that are still open to the BASE_URL, as they may be in a bad state due to the expired token
141-
for (const window of openWindows) {
142-
for (const tab of window.tabs) {
143-
if (tab.url?.includes(`${BASE_URL}`)) {
144-
try {
145-
await browser.tabs.remove(tab.id!);
146-
console.log(`Closed expired session tab with id ${tab.id}`);
147-
} catch {
148-
console.warn(`Could not close tab with id ${tab.id}`);
149-
}
150-
}
151-
}
152-
}
153-
}
154-
155-
/*
137+
/*
156138
Checks if the add-on is logged in, this is separate from the web context.
157-
It's useful for the web context to figure out if the add-on is logged in or not, and also for the add-on to check if the token is expired and log out if necessary.
139+
It's a read-only probe: it's called by the Send route guard (via GET_LOGIN_STATE)
140+
and by a 60s timer, so it MUST NOT have destructive side effects (closing tabs,
141+
wiping storage, forcing the menu to log out).
142+
143+
Note on `expires_at`: that field is the short-lived OIDC *access token* expiry. An
144+
expired access token does NOT mean the session is over — the Send web app refreshes
145+
it transparently via userManager.signinSilent() using the refresh_token (see
146+
auth-store getAccessToken/checkAuthStatus). So login state is driven by the presence
147+
of a refresh_token, not by access-token expiry. Genuine logout flows through the
148+
SIGN_OUT message path, which calls menuLogout().
158149
*/
159150
export async function getLoginState() {
160151
try {
161152
// Get the auth token from browser storage (this is a copy of the auth token stored in the web context, used to determine login state in the add-on context)
162153
const authStorageData = await browser.storage.local.get(STORAGE_KEY_AUTH);
163-
console.log(authStorageData);
164-
if (authStorageData[STORAGE_KEY_AUTH]) {
165-
// Check token expiration
166-
const expiration = authStorageData[STORAGE_KEY_AUTH]?.expires_at * 1000; // Convert to milliseconds
167-
const now = Date.now();
168-
console.log('Token expires in (s):', (expiration - now) / 1000);
169-
// If the token is expired, treat as logged out
170-
if (expiration && now >= expiration) {
171-
console.warn('Auth token is expired. Treating as logged out.');
172-
await closeAllTbProTabs();
173-
174-
await browser.storage.local.remove(STORAGE_KEY_AUTH);
175-
await menuLogout();
176-
return { isLoggedIn: false, username: null };
177-
}
154+
const auth = authStorageData[STORAGE_KEY_AUTH];
155+
if (!auth) {
156+
return { isLoggedIn: false, username: null };
157+
}
178158

179-
const username =
180-
authStorageData[STORAGE_KEY_AUTH]?.profile?.preferred_username ||
181-
authStorageData[STORAGE_KEY_AUTH]?.profile?.email;
159+
const username = auth?.profile?.preferred_username || auth?.profile?.email;
182160

183-
if (username) {
184-
await menuLoggedIn({ username });
185-
return { isLoggedIn: true, username };
186-
}
161+
// A stored session with a refresh_token is logged in, even if the access
162+
// token's expires_at has lapsed — the web app will silently refresh it.
163+
if (auth.refresh_token && username) {
164+
await menuLoggedIn({ username });
165+
return { isLoggedIn: true, username };
187166
}
167+
188168
return { isLoggedIn: false, username: null };
189169
} catch (error) {
190170
console.error('Error retrieving auth state from storage:', error);

‎packages/addon/src/test/menu.test.ts‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
import { STORAGE_KEY_AUTH } from '@send-frontend/lib/const';
2+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
3+
4+
import { getLoginState } from '../menu';
5+
6+
/**
7+
* Regression guard: getLoginState() is a read-only probe (called by the Send
8+
* route guard via GET_LOGIN_STATE and by a 60s timer). It must never close tabs
9+
* or wipe storage as a side effect. In particular, an *expired access token*
10+
* (short-lived `expires_at`) is NOT logged out — the web app refreshes it
11+
* silently with the refresh_token — so it must keep returning isLoggedIn: true
12+
* and leave open send.tb.pro tabs alone. Previously the expiry branch called
13+
* closeAllTbProTabs(), which closed the Send dashboard tab opened from the
14+
* accounts dashboard.
15+
*/
16+
17+
const USERNAME = 'user@example.com';
18+
19+
function setupBrowserMock(authValue: unknown) {
20+
vi.stubGlobal('browser', {
21+
storage: {
22+
local: {
23+
get: vi.fn().mockResolvedValue(
24+
authValue === undefined ? {} : { [STORAGE_KEY_AUTH]: authValue }
25+
),
26+
remove: vi.fn().mockResolvedValue(undefined),
27+
clear: vi.fn().mockResolvedValue(undefined),
28+
},
29+
},
30+
tabs: {
31+
remove: vi.fn().mockResolvedValue(undefined),
32+
create: vi.fn().mockResolvedValue({ id: 1 }),
33+
},
34+
windows: {
35+
getAll: vi.fn().mockResolvedValue([]),
36+
},
37+
TBProMenu: {
38+
update: vi.fn().mockResolvedValue(undefined),
39+
create: vi.fn().mockResolvedValue(undefined),
40+
clear: vi.fn().mockResolvedValue(undefined),
41+
},
42+
i18n: { getMessage: vi.fn((key: string) => key) },
43+
});
44+
}
45+
46+
// `expires_at` is a Unix timestamp in seconds (OIDC access-token expiry).
47+
const PAST = Math.floor(Date.now() / 1000) - 3600;
48+
const FUTURE = Math.floor(Date.now() / 1000) + 3600;
49+
50+
const authWith = (overrides: Record<string, unknown> = {}) => ({
51+
refresh_token: 'refresh-abc',
52+
expires_at: FUTURE,
53+
profile: { preferred_username: USERNAME },
54+
...overrides,
55+
});
56+
57+
afterEach(() => {
58+
vi.unstubAllGlobals();
59+
vi.restoreAllMocks();
60+
});
61+
62+
describe('getLoginState', () => {
63+
it('reports logged out and closes no tabs when no auth is stored', async () => {
64+
setupBrowserMock(undefined);
65+
66+
const state = await getLoginState();
67+
68+
expect(state).toEqual({ isLoggedIn: false, username: null });
69+
expect(browser.tabs.remove).not.toHaveBeenCalled();
70+
});
71+
72+
it('stays logged in WITHOUT closing tabs when the access token has expired', async () => {
73+
setupBrowserMock(authWith({ expires_at: PAST }));
74+
75+
const state = await getLoginState();
76+
77+
expect(state).toEqual({ isLoggedIn: true, username: USERNAME });
78+
// The core regression: an expired access token must not tear down Send tabs.
79+
expect(browser.tabs.remove).not.toHaveBeenCalled();
80+
expect(browser.storage.local.remove).not.toHaveBeenCalled();
81+
});
82+
83+
it('reports logged in for a stored session with a valid token', async () => {
84+
setupBrowserMock(authWith());
85+
86+
const state = await getLoginState();
87+
88+
expect(state).toEqual({ isLoggedIn: true, username: USERNAME });
89+
expect(browser.tabs.remove).not.toHaveBeenCalled();
90+
});
91+
92+
it('falls back to the profile email when preferred_username is absent', async () => {
93+
setupBrowserMock(
94+
authWith({ profile: { email: USERNAME }, expires_at: PAST })
95+
);
96+
97+
const state = await getLoginState();
98+
99+
expect(state).toEqual({ isLoggedIn: true, username: USERNAME });
100+
});
101+
102+
it('reports logged out when the stored session has no refresh_token', async () => {
103+
setupBrowserMock(authWith({ refresh_token: undefined }));
104+
105+
const state = await getLoginState();
106+
107+
expect(state).toEqual({ isLoggedIn: false, username: null });
108+
expect(browser.tabs.remove).not.toHaveBeenCalled();
109+
});
110+
});

‎packages/send/frontend/src/apps/send/stores/folder-store.ts‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ export interface FolderStore {
7777

7878
const useFolderStore = defineStore('folderManager', () => {
7979
const { api } = useApiStore();
80-
const { user } = useUserStore();
80+
const { user, populateFromBackend } = useUserStore();
8181
const { progress } = useStatusStore();
8282
const { metrics } = useMetricsStore();
8383
const { keychain } = useKeychainStore();
@@ -265,6 +265,23 @@ const useFolderStore = defineStore('folderManager', () => {
265265
): Promise<Item[]> {
266266
progress.error = '';
267267

268+
// Guard against uploading with an unpopulated user. The create-entry POST
269+
// (`POST /api/uploads`) sends `ownerId: user.id`; an undefined id is silently
270+
// dropped by JSON.stringify, so the backend rejects every attempt with a 400
271+
// ("Required" on ownerId) and the upload retry loop hammers it — which also
272+
// drives a storm of token/userinfo refreshes. Re-hydrate from the backend
273+
// session first, and fail with a clear message if it still can't provide one.
274+
if (!user.id) {
275+
console.warn('uploadItem: user.id missing; re-populating from backend');
276+
await populateFromBackend();
277+
}
278+
if (!user.id) {
279+
progress.error = 'You are not fully signed in. Please sign in again.';
280+
throw new Error(
281+
'Cannot upload: user session is missing a user id (ownerId would be empty).'
282+
);
283+
}
284+
268285
const canUpload = await checkBlobSize(fileBlob);
269286

270287
const uploadExceedsLimit = await canUserUpload(fileBlob.size);

0 commit comments

Comments
 (0)