fix: greprile review

Author: Ajit Kumar

src/githubAccount.js

@@ -168,27 +168,30 @@ export class GitHubAccountController {
     for (const listener of this.#listeners) listener(account);
   }
 
-  async #runLegacyOperation(operation, token) {
+  async #runLegacyOperation(operation, token, epoch) {
     let result;
     try {
       result = await operation(this.#createGitHub(token));
     } catch (error) {
       if (isAuthenticationError(error)) {
-        this.#credentialEpoch += 1;
-        this.#legacyToken = '';
-        this.#account = null;
-        this.#setAccount(null);
+        if (!this.#invalidateLegacyToken(token, epoch)) {
+          throw new GitHubAuthError('cancelled');
+        }
         throw new GitHubAuthError('invalid-token');
       }
       throw error;
     }
-    this.#finalizeLegacyToken(token);
+    this.#finalizeLegacyToken(token, epoch);
     return result;
   }
 
   #runOperation(operation, write) {
     if (this.#legacyToken) {
-      return this.#runLegacyOperation(operation, this.#legacyToken);
+      return this.#runLegacyOperation(
+        operation,
+        this.#legacyToken,
+        this.#credentialEpoch,
+      );
     }
     return this.#authManager.runWithToken(
       (token) => operation(this.#createGitHub(token)),
@@ -221,9 +224,14 @@ export class GitHubAccountController {
     }
   }
 
-  #finalizeLegacyToken(token) {
-    if (this.#legacyMigration || token !== this.#legacyToken) return;
-    const epoch = this.#credentialEpoch;
+  #finalizeLegacyToken(token, epoch) {
+    if (
+      this.#legacyMigration ||
+      epoch !== this.#credentialEpoch ||
+      token !== this.#legacyToken
+    ) {
+      return;
+    }
     const migration = this.#authManager
       .usePersonalAccessToken(token)
       .then((account) => {
@@ -248,10 +256,7 @@ export class GitHubAccountController {
         ) {
           return;
         }
-        this.#credentialEpoch += 1;
-        this.#legacyToken = '';
-        this.#account = null;
-        this.#setAccount(null);
+        this.#invalidateLegacyToken(token, epoch, false);
       })
       .finally(() => {
         if (this.#legacyMigration === migration) {
@@ -261,6 +266,25 @@ export class GitHubAccountController {
     this.#legacyMigration = migration;
   }
 
+  #invalidateLegacyToken(token, epoch, reportStorageError = true) {
+    if (epoch !== this.#credentialEpoch || token !== this.#legacyToken) {
+      return false;
+    }
+
+    this.#credentialEpoch += 1;
+    this.#legacyToken = '';
+    this.#account = null;
+    let cleanupError;
+    try {
+      this.#removeLegacyToken();
+    } catch (error) {
+      cleanupError = error;
+    }
+    this.#setAccount(null);
+    if (cleanupError && reportStorageError) throw cleanupError;
+    return true;
+  }
+
   #removeLegacyToken() {
     try {
       this.#legacyStore?.removeItem?.(LEGACY_TOKEN_KEY);

test/github-account.test.js

@@ -340,8 +340,121 @@ test('a confirmed invalid provisional PAT opens account recovery', async () => {
       );
 
       assert.equal(await controller.getAccount(), null);
-      assert.equal(values.get('github-token'), 'legacy-token');
+      assert.equal(values.has('github-token'), false);
       assert.equal(accounts.at(-1), null);
+
+      const restarted = new GitHubAccountController({
+        authManager,
+        createGitHub: () => ({}),
+        legacyStore: createStore(values),
+      });
+      assert.equal(await restarted.initialize(), null);
+    },
+  );
+});
+
+test('an invalid background migration removes the legacy PAT permanently', async () => {
+  const values = new Map([['github-token', 'legacy-token']]);
+  const authManager = createAuthManager([]);
+  authManager.usePersonalAccessToken = async () => {
+    throw Object.assign(new Error('invalid token'), { kind: 'invalid-token' });
+  };
+
+  await withSourceModule(
+    'githubAccount.js',
+    { localStorage: createStore(values) },
+    async ({ GitHubAccountController }) => {
+      const controller = new GitHubAccountController({
+        authManager,
+        createGitHub: () => ({}),
+        legacyStore: createStore(values),
+      });
+      await controller.initialize();
+
+      assert.equal(await controller.run(() => 'loaded'), 'loaded');
+      await settle();
+
+      assert.equal(await controller.getAccount(), null);
+      assert.equal(values.has('github-token'), false);
+
+      const restarted = new GitHubAccountController({
+        authManager,
+        createGitHub: () => ({}),
+        legacyStore: createStore(values),
+      });
+      assert.equal(await restarted.initialize(), null);
+    },
+  );
+});
+
+test('legacy PAT invalidation stays signed out when cleanup fails', async () => {
+  const values = new Map([['github-token', 'legacy-token']]);
+  const authManager = createAuthManager([]);
+  const legacyStore = {
+    ...createStore(values),
+    removeItem() {
+      throw new Error('storage unavailable');
+    },
+  };
+
+  await withSourceModule(
+    'githubAccount.js',
+    { localStorage: legacyStore },
+    async ({ GitHubAccountController }) => {
+      const controller = new GitHubAccountController({
+        authManager,
+        createGitHub: () => ({}),
+        legacyStore,
+      });
+      await controller.initialize();
+
+      await assert.rejects(
+        controller.run(() => {
+          throw Object.assign(new Error('unauthorized'), { status: 401 });
+        }),
+        hasKind('storage'),
+      );
+
+      assert.equal(await controller.getAccount(), null);
+      assert.equal(values.get('github-token'), 'legacy-token');
+    },
+  );
+});
+
+test('a stale legacy rejection cannot clear replacement credentials', async () => {
+  const values = new Map([['github-token', 'legacy-token']]);
+  const operationStarted = deferred();
+  const releaseOperation = deferred();
+  const authManager = createAuthManager([]);
+
+  await withSourceModule(
+    'githubAccount.js',
+    { localStorage: createStore(values) },
+    async ({ GitHubAccountController }) => {
+      const controller = new GitHubAccountController({
+        authManager,
+        createGitHub: () => ({}),
+        legacyStore: createStore(values),
+      });
+      const accounts = [];
+      controller.subscribe((account) => accounts.push(account));
+      await controller.initialize();
+
+      const stale = controller.run(async () => {
+        operationStarted.resolve();
+        await releaseOperation.promise;
+        throw Object.assign(new Error('unauthorized'), { status: 401 });
+      });
+      const staleRejection = assert.rejects(stale, hasKind('cancelled'));
+      await operationStarted.promise;
+
+      await controller.usePersonalAccessToken('replacement-token');
+      releaseOperation.resolve();
+      await staleRejection;
+
+      assert.equal((await controller.getAccount()).login, 'octocat');
+      assert.equal(values.has('github-token'), false);
+      assert.equal(accounts.at(-1).login, 'octocat');
     },
   );
 });