fix(addon): stop closing Send tabs when access token has expired - #949
Merged
Conversation
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>
…nt 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>
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>
radishmouse
approved these changes
Jul 3, 2026
This was referenced Jul 14, 2026
4 tasks
aaspinwall
added a commit
that referenced
this pull request
Jul 29, 2026
Revert the scoped single-key remove() approach and restore a full browser.storage.local.clear() on genuine logout, returning the add-on to a clean, logged-out state (auth token, staged passphrase, pending OIDC token set, folder-lock records, cloud-file configs all cleared). storage.local is per-extension isolated (keyed to the add-on's gecko id), NOT shared with Thunderbird core or other add-ons, so a blanket clear() only ever touches TB-Send's own data. A scoped remove() is fail-open: it leaks the passphrase and a live refresh token past logout. A concurrent in-flight login (PENDING_ADDON_TOKEN) is intentionally cancelled by logout. The blanket clear() is only reachable from a genuine logout (menuLogout via LOGOUT action / SIGN_OUT); the read-only getLoginState() probe never wipes (see #948/#949). Tests updated to assert the full-wipe behavior. Closes #1054
aaspinwall
added a commit
that referenced
this pull request
Aug 10, 2026
* fix(addon): scope menuLogout() storage clear to STORAGE_KEY_AUTH only menuLogout() previously called browser.storage.local.clear(), wiping unrelated in-flight data in the same namespace -- specifically PENDING_ADDON_TOKEN (an in-progress AccountHub login) and SEND_MESSAGE_TO_BRIDGE (a passphrase staged for the bridge handoff). Replace the blanket clear with a scoped remove() targeting only the auth session, leaving the in-flight data intact. Refs #1023 Closes #1023 * test(addon): update logout tests for scoped storage remove (fix CI) menuLogout() now scoped-removes STORAGE_KEY_AUTH instead of a blanket storage.local.clear() (this PR's fix), but two pre-existing tests still asserted clear() was called and failed CI (addon-changes / Shared package tests and lint): - menu.test.ts 'opens the logout page and clears storage after closing tabs' - happy-clean-logout-no-inflight-work.test.ts 'closes only Send tabs...' Both now assert storage.local.remove(STORAGE_KEY_AUTH) and that clear() is not called. Added the STORAGE_KEY_AUTH import to the integration spec. * fix(addon): fully wipe add-on storage on logout (#1054) Revert the scoped single-key remove() approach and restore a full browser.storage.local.clear() on genuine logout, returning the add-on to a clean, logged-out state (auth token, staged passphrase, pending OIDC token set, folder-lock records, cloud-file configs all cleared). storage.local is per-extension isolated (keyed to the add-on's gecko id), NOT shared with Thunderbird core or other add-ons, so a blanket clear() only ever touches TB-Send's own data. A scoped remove() is fail-open: it leaks the passphrase and a live refresh token past logout. A concurrent in-flight login (PENDING_ADDON_TOKEN) is intentionally cancelled by logout. The blanket clear() is only reachable from a genuine logout (menuLogout via LOGOUT action / SIGN_OUT); the read-only getLoginState() probe never wipes (see #948/#949). Tests updated to assert the full-wipe behavior. Closes #1054 --------- Co-authored-by: aaspinwall <aaspinwall@users.noreply.github.com> Co-authored-by: Alejandro Aspinwall <aaspinwall@thunderbird.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed?
This branch fixes three related add-on session/upload bugs.
1. Add-on menu no longer closes Send tabs on access-token expiry (#948).
Made the add-on's
getLoginState()(packages/addon/src/menu.ts) a read-only probe. It now reports logged-in based on a stored session that has arefresh_token, regardless of the OIDC access-tokenexpires_at, and no longer performs destructive side effects. The previously coupledcloseAllTbProTabs()/storage.remove/menuLogout()calls were removed from the probe, and the now-unusedcloseAllTbProTabs()helper was deleted. Addedpackages/addon/src/test/menu.test.ts.2. OIDC silent-refresh churn / spurious logouts (#951).
Reworked
packages/send/frontend/src/stores/auth-store.ts:automaticSilentRenew: falseandloadUserInfo: false— removes uncontrolled background renews and a fragile post-refreshuserinforound-trip that can fail inside Thunderbird and reject an otherwise-successful refresh (profile claims already come from the id_token).refreshAccessToken()promise dedupes concurrent refreshes, so refresh-token rotation no longer races callers intoinvalid_grant.invalid_grant,login_required,session_expired) — which clear login state and notify the add-on viaSIGN_OUT— from transient failures, which preserve the session for a later retry. This implements theSIGN_OUT-on-silent-refresh-failure follow-up noted in the original scope of this PR. Addedpackages/send/frontend/src/test/stores/auth-store.test.ts.3. File upload fails with an empty
ownerId→ 400 storm (#950).Added a self-healing guard in
uploadItem(packages/send/frontend/src/apps/send/stores/folder-store.ts): before uploading, ifuser.idis empty it callspopulateFromBackend(), and if the id is still empty it aborts with a clear error instead of sending anownerId-less body that the backend rejects with400and the retry loop hammers. Because theUploaderholds the same shared reactiveuserobject, re-populating also fixesthis.user.idinsidedoUpload.AI disclosure: These changes were implemented by Claude (agent-written) under close human direction. The maintainer reproduced each bug manually in Thunderbird, drove the investigation (including capturing the network traffic and the exact 400 response body), reviewed the root-cause analysis, and approved the plans and final diffs.
Why?
Inside Thunderbird, clicking a
send.tb.prolink fromaccounts.tb.pro/dashboardclosed the newly-opened Send tab once the add-on's stored access token had passed its short-livedexpires_at, becausegetLoginState()treated an expired access token as logged-out.expires_atis the access-token expiry, not the session lifetime — the Send web app refreshes transparently viasigninSilent()(#948).That refresh path was itself fragile:
automaticSilentRenewplus on-demandsigninSilent()with no deduplication produced concurrent refreshes that race under refresh-token rotation, andloadUserInfoadded auserinforound-trip that can fail in Thunderbird. The result was a storm oftoken/userinforequests and spurious logouts (#951).One downstream effect of that churn:
GET users/mecould fail at load, leaving the reactive user store unpopulated. An upload would then sendownerId: undefined;JSON.stringifydrops the key, the backend's Zod schema rejects it with400 "ownerId Required", and the upload retry loop re-fires the whole block repeatedly — the visiblesigned → PUT(200) → uploads(400)storm (#950).Limitations and Notes
userManager, so it cannot refresh tokens itself — refresh remains owned by the web app, which the menu change defers to.ProfileViewself-close), which is working correctly.ownerId) from one side; the auth-store rework fixes the churn that can leaveuser.idunpopulated from the other side.Applicable Issues
Closes #948
Closes #950
Closes #951
Screenshots
N/A — behavioral fixes, no UI change. The #950 network-tab repro (repeating
token/userinfo/signed/uploads400 cycles) is described in that issue.