Skip to content

Commit b46628f

Browse files
aaspinwallcagebot
aaspinwall
and
cagebot
authored
test(addon): add integration tests for stale-login-on-re-enable and the per-context init guard (#1044)
* test(addon): add integration specs for C2, D3 sync bugs Adds the final two remaining Medium-priority specs (per ADDON-BUG-REPORTS-2026-07-22.md) on top of the fake Thunderbird host harness: - C2: add-on re-enable trusts stale STORAGE_KEY_AUTH without backend revalidation. - D3: init.ts's single-flight guard only coordinates within a single execution context, not across background/popup/web. Together with the already-landed A-series, B-series, B5, and C1 specs, this completes the 13 confirmed sync/race bugs covered by Tier 1 of the integration-test plan. Each asserts CONFIRMED BUG behavior against the real production modules and is expected to start failing once the corresponding fix lands (see harness README's regression-test contract). * docs(addon): add integration harness README and wire test:integration script (#1045) - Adds src/test/integration/README.md: explains the Tier 1 fake-host harness (what it simulates, what it deliberately stubs out, the regression-test contract for CONFIRMED BUG specs, and current coverage status/gaps). - Adds packages/addon/package.json's test:integration script (VITE_TESTING=true VITE_SEND_CLIENT_URL=... VITE_SEND_SERVER_URL=... vitest run src/test/integration --silent) so the harness can be run scoped, without needing the full addon .env. - Adds the two source-of-truth e2e docs the harness/specs trace back to: ADDON-BUG-REPORTS-2026-07-22.md (concise, spec-file-referenced bug list) and ADDON-INTEGRATION-TEST-ENV-PLAN-2026-07-22.md (the Tier 1/2/3 environment plan this harness implements Tier 1 of). No CI changes in this PR -- see PR description / reviewer notes for why the existing addon-changes job in .github/workflows/validate.yml already covers this (it's path-filtered on packages/addon/** and already runs `lerna run test --scope=addon`, i.e. `pnpm test`, which picks up every spec under src/test/integration/ automatically since vitest's default include glob is **/*.test.ts with no directory exclusion). The new test:integration script is a convenience for local/targeted runs; it does not need to be wired into CI separately. Co-authored-by: cagebot <cagebot@cagebots-Virtual-Machine.local> * fix(addon): cross-context lock for init.ts default-folder recreate (#1046) init.ts's single-flight guard (added for #930) is a module-scope variable, so it only dedups concurrent calls within one JS context. Background, popup, and any web-app tab each load their own copy of the module, so none of them can see another context's in-flight delete+recreate of the same account's default folder -- reintroducing #930's race, just moved across contexts instead of within one. Add a short-TTL lock in browser.storage.local (the one thing every context actually shares) around just the delete+recreate branch, keyed by account id. A context that can't acquire the lock re-syncs and trusts whatever the lock holder leaves behind instead of racing it. No-ops in any context without browser.storage.local (e.g. a plain web-app tab with no sibling context to race against), so behavior there is unchanged. Updates the D3 integration test (packages/addon) to actually exercise the fake host's shared browser.storage.local via two real fake-host contexts, replacing the version that only proved the bug existed. Adds a second spec asserting a racing context defers instead of running its own delete+recreate while another context holds the lock -- confirmed this fails against the pre-fix init.ts and passes with the fix. Fixes #1032 Co-authored-by: cagebot <cagebot@cagebots-Virtual-Machine.local> --------- Co-authored-by: cagebot <cagebot@cagebots-Virtual-Machine.local>
1 parent 872c2e8 commit b46628f

8 files changed

Lines changed: 1047 additions & 13 deletions

File tree

‎packages/addon/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
"sync:builtin": "pnpm build:dev:system && ./scripts/sync-to-builtin.sh",
3030
"sync:builtin:local": "pnpm build:dev:system:local && ./scripts/sync-to-builtin.sh",
3131
"test": "VITE_TESTING=true vitest run --silent",
32+
"test:integration": "VITE_TESTING=true VITE_SEND_CLIENT_URL=https://send.tb.pro VITE_SEND_SERVER_URL=https://localhost:8088 vitest run src/test/integration --silent",
3233
"test:watch": "VITE_TESTING=true vitest --watch",
3334
"test-debug": "VITE_TESTING=true vitest --inspect-brk --single-thread",
3435
"typecheck": "tsc --noEmit",

‎packages/addon/src/test/integration/README.md‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
# Add-on Integration Tests (Tier 1: Fake-Host Harness)
2+
3+
This directory implements **Tier 1** of
4+
`packages/send/e2e/ADDON-INTEGRATION-TEST-ENV-PLAN-2026-07-22.md`: a fast,
5+
deterministic, CI-friendly harness that loads the **real** production
6+
modules (`background.ts`, `menu.ts`, `token-bridge.js`, `init.ts`,
7+
`UserMenu.vue`, `PopupView.vue`) against a fake multi-context Thunderbird
8+
host, and drives the exact race windows described in
9+
`packages/send/e2e/ADDON-BUG-REPORTS-2026-07-22.md` deterministically.
10+
11+
## Running
12+
13+
```sh
14+
pnpm --filter addon test:integration
15+
```
16+
17+
(equivalent to `VITE_TESTING=true VITE_SEND_CLIENT_URL=https://send.tb.pro
18+
VITE_SEND_SERVER_URL=https://localhost:8088 vitest run src/test/integration`)
19+
20+
## What this harness is
21+
22+
`fakeThunderbirdHost.ts` simulates the 3 JS-only WebExtension execution
23+
contexts this add-on has in a real running Thunderbird. `testHelpers.ts`
24+
holds the shared per-spec setup/teardown boilerplate (`setupHost()` /
25+
`teardownHost()` / `stubContext()` / `ADDON_ROOT` / `createDeferred()`) so
26+
individual spec files stay focused on the race they're proving instead of
27+
repeating harness wiring.
28+
29+
The three execution contexts:
30+
31+
- **background** — the non-persistent background page (`background.ts`)
32+
- **popup** — the upload popup window (`PopupView.vue`)
33+
- **web** — a plain browser tab running send.tb.pro, bridged via
34+
`token-bridge.js`
35+
36+
Each context gets its own independent `browser` mock object (mirroring the
37+
real platform, where each JS realm has its own `browser.*` binding), but all
38+
contexts share **one** fake `browser.storage.local` backing store with real
39+
`storage.onChanged` firing — this is the actual cross-context sync primitive
40+
in production (see `shared-pinia.ts`'s per-context-singleton comment), so a
41+
real shared store (not per-context stubs) is essential for these tests to be
42+
meaningful.
43+
44+
`browser.runtime.sendMessage` calls from any context fan out to every OTHER
45+
context's registered `onMessage` listeners, exactly like real WebExtension
46+
messaging. `browser.windows.create()` returns a deliberately controllable,
47+
unresolved Promise (see `resolveNextWindowCreate()`) so tests can force exact
48+
check-then-act interleavings (e.g. B3) that are effectively impossible to
49+
reproduce reliably against a real window manager.
50+
51+
## What this harness deliberately does NOT do
52+
53+
Per the integration-environment plan's key finding: the hamburger/TBPro menu
54+
(`menu.ts` + `public/api/TBProMenu/implementation.js`) is built with
55+
`ChromeUtils.importESModule` + `ExtensionCommon.ExtensionAPI` — privileged
56+
Thunderbird-internal APIs that only exist inside a real running Thunderbird
57+
process. This harness stubs `browser.TBProMenu`, `browser.CloudFileAccounts`,
58+
`browser.MailAccounts`, and `browser.AccountHub` as plain spies — it does
59+
**not** attempt to simulate real chrome UI behavior for them. Tests only
60+
assert that the right calls happened with the right arguments at the right
61+
time, which is what the confirmed sync-race findings actually depend on.
62+
63+
Anything requiring real platform timing/window-manager behavior (B3's
64+
magnitude-of-likelihood question, B5's background-page-recycle trigger
65+
condition, C1's window auto-close-on-disable behavior) is explicitly a
66+
**Tier 2/3** concern — see the environment plan doc — and is called out with
67+
a "Needs live test" comment in the relevant spec here.
68+
69+
## Regression-test contract
70+
71+
Every spec in this directory is named after and directly traces to a finding
72+
ID in `ADDON-BUG-REPORTS-2026-07-22.md` (e.g. `b1-*.test.ts` → finding B1).
73+
Each spec:
74+
75+
1. Imports the **real** production module(s) under test (not
76+
reimplementations).
77+
2. Drives the exact interleaving/race described in the finding.
78+
3. Asserts the **current (buggy) behavior**, with a `CONFIRMED BUG:` prefix
79+
in the test name.
80+
81+
**These specs are expected to FAIL once the underlying bug is fixed.** That
82+
is the point — a failing spec here is the CI signal that a fix landed. When
83+
that happens:
84+
85+
- Do not just delete the spec.
86+
- Update it to assert the new, correct behavior instead (turn the
87+
`CONFIRMED BUG:` assertion into a `FIXED:` assertion of the desired
88+
outcome), so the fix gets permanent regression coverage.
89+
- Remove the `Needs live test` caveat from the doc comment only if the fix
90+
addresses the client-side structural gap; live-test-only sub-questions
91+
(server-side outcomes, platform timing) still need Tier 2/3 verification
92+
even after a client-side fix lands.
93+
94+
## Coverage status
95+
96+
| ID | Spec file | Priority |
97+
|----|-----------|----------|
98+
| A1 | `a1-logout-does-not-abort-popup-upload.test.ts` | Medium |
99+
| A2 | `a2-origin-mismatch-logout-not-delivered.test.ts` | Medium |
100+
| A5 | `a5-logout-clears-unrelated-storage.test.ts` | Medium |
101+
| A6 | `a6-menu-stale-login-no-push-refresh.test.ts` | Medium |
102+
| A7 | `a7-accounthub-login-races-web-login.test.ts` | Medium |
103+
| B1 | `b1-rapid-attach-before-popup-ready.test.ts` | Medium |
104+
| B2 | `b2-popup-close-mid-upload.test.ts` | Medium |
105+
| B3 | `b3-popup-check-then-act-race.test.ts` | Medium |
106+
| B4 | `b4-attach-while-popup-open.test.ts` | Medium |
107+
| B5 | `b5-background-restart-loses-bookkeeping.test.ts` | **High** |
108+
| C1 | `c1-disable-mid-upload-orphans-popup.test.ts` | **High** |
109+
| C2 | `c2-reenable-trusts-stale-auth.test.ts` | Medium |
110+
| D3 | `d3-init-single-flight-per-context-only.test.ts` | Medium |
111+
112+
**Not yet covered** (deferred — see reasoning below):
113+
114+
- **A3** (forced-logout races concurrent multipart parts) and **A4**
115+
(cross-context refresh-token race) — both live in `auth-store.ts`'s OIDC
116+
client internals (`signinSilent()`, `MAX_CONCURRENT_PARTS` fan-out),
117+
which need deeper mocking of `oidc-client-ts`/`UserManager` than this
118+
harness currently provides. Left for a follow-up harness extension.
119+
- **D2** (bridged-passphrase one-shot consume race) — needs the real
120+
`Keychain`/`bridgePassphrase.ts` wired into two contexts sharing storage;
121+
doable with this harness's shared-storage primitive, just not yet written.
122+
123+
Extend this harness's mocks (particularly around `oidc-client-ts` and
124+
`Keychain`) to close the remaining three before considering Tier 1 complete
125+
per the original environment plan's effort estimate.

‎packages/addon/src/test/integration/c2-reenable-trusts-stale-auth.test.ts‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
/**
2+
* C2 — Add-on re-enable trusts stale `STORAGE_KEY_AUTH` without
3+
* re-validating against the backend.
4+
*
5+
* See ADDON-BUG-REPORTS-2026-07-22.md #C2 and
6+
* ADDON-SYNC-VERIFIED-FINDINGS-2026-07-21.md §C2.
7+
*
8+
* Mechanism under test:
9+
* - Re-enabling the add-on re-runs background.ts's top-level `main()` IIFE
10+
* from scratch, identically to a cold start.
11+
* - `main()` calls `getLoginState()` (menu.ts), which is a PURE LOCAL
12+
* STORAGE READ -- it checks only for the presence of `refresh_token` in
13+
* `browser.storage.local[STORAGE_KEY_AUTH]`, with zero network call to
14+
* validate that token against the backend.
15+
* - `shouldInitCloudFileOnStartup(isLoggedIn)` is a pure boolean
16+
* passthrough of whatever `getLoginState()` produced.
17+
* - If `isLoggedIn` is (falsely) true, `initCloudFile()` runs, which
18+
* re-registers the cloud file provider and creates/reactivates a cloud
19+
* file account -- fully re-activating cloud-file features for a session
20+
* that may have been revoked entirely server-side.
21+
*
22+
* This test seeds a stale (but locally well-formed) auth object into shared
23+
* storage, imports background.ts fresh (simulating a re-enable / cold
24+
* start), and confirms initCloudFile()'s effects run (CloudFileAccounts
25+
* re-registration + account creation) with ZERO network/API validation call
26+
* having occurred anywhere in the process.
27+
*/
28+
import { STORAGE_KEY_AUTH } from '@send-frontend/lib/const';
29+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
30+
import { type FakeHost } from './fakeThunderbirdHost';
31+
import { setupHost, stubContext, teardownHost } from './testHelpers';
32+
33+
describe('C2: add-on re-enable trusts stale STORAGE_KEY_AUTH without backend revalidation', () => {
34+
let host: FakeHost;
35+
36+
beforeEach(() => {
37+
host = setupHost();
38+
});
39+
40+
afterEach(() => {
41+
teardownHost();
42+
});
43+
44+
it('CONFIRMED BUG: re-enable (fresh module load) re-activates cloud file with no backend validation call', async () => {
45+
const ctx = stubContext(host);
46+
47+
// Seed a previously-stored session -- this is exactly the shape
48+
// getLoginState() checks: refresh_token + a resolvable username. In
49+
// reality this token may have been revoked/expired server-side at any
50+
// point after it was stored; nothing here re-checks that.
51+
ctx.browser.storage.local.get = vi.fn(async () => ({
52+
[STORAGE_KEY_AUTH]: {
53+
refresh_token: 'potentially-revoked-refresh-token',
54+
expires_at: Math.floor(Date.now() / 1000) - 999999, // long expired access token
55+
profile: { preferred_username: 'user@example.com' },
56+
},
57+
}));
58+
59+
// Simulate re-enable: this is functionally identical to a cold start --
60+
// the add-on's background page re-runs main() from scratch.
61+
await import('../../background');
62+
63+
// Let main()'s async IIFE (checkAndUninstallIfDeprecated -> initMenu ->
64+
// getLoginState -> initCloudFile chain) fully settle.
65+
await Promise.resolve();
66+
await Promise.resolve();
67+
await Promise.resolve();
68+
await Promise.resolve();
69+
70+
// THE BUG: getLoginState() trusted the stale refresh_token's mere
71+
// presence and returned isLoggedIn: true purely from local storage, so
72+
// shouldInitCloudFileOnStartup() green-lit initCloudFile(), which:
73+
// 1. re-registered the cloud file provider,
74+
expect(ctx.browser.CloudFileAccounts.registerProvider).toHaveBeenCalled();
75+
// 2. created/reactivated a cloud file account,
76+
expect(ctx.browser.CloudFileAccounts.createAccount).toHaveBeenCalled();
77+
78+
// ...all without a SINGLE call that could have validated the refresh
79+
// token against the backend (e.g. no auth/oidc/me equivalent, no
80+
// api.call at all during this startup path). If a validation call
81+
// existed, it would have to go through browser.storage.local (there is
82+
// no other backend interface mocked here) or an explicit fetch --
83+
// neither happened.
84+
//
85+
// (unregisterProvider is the "signed out" branch and must NOT have run,
86+
// confirming the code took the "trust local storage" happy path.)
87+
expect(ctx.browser.CloudFileAccounts.unregisterProvider).not.toHaveBeenCalled();
88+
});
89+
});

0 commit comments

Comments
 (0)