Skip to content

fix: singleton race condition and plaintext auth token storage#58

Draft
Copilot wants to merge 2 commits intomainfrom
copilot/fix-singleton-race-condition
Draft

fix: singleton race condition and plaintext auth token storage#58
Copilot wants to merge 2 commits intomainfrom
copilot/fix-singleton-race-condition

Conversation

Copy link
Copy Markdown

Copilot AI commented Mar 19, 2026

Two high-severity security issues: concurrent callers to getNumbersApi() could each pass the if (!instance) guard during the async gap, creating divergent NumbersApiManager instances; and auth_token was persisted to disk via chrome.storage.local.

Singleton race condition (NumbersApiManager.ts)

Replace the racy lazy init with a promise-based lock — the promise is assigned synchronously so all concurrent callers await the same initialization:

// Before: async gap lets a second caller create a second instance
if (!instance) {
  instance = new NumbersApiManager();
  await instance.initialize(); // ← race window
}

// After: initPromise assigned before any await; concurrent callers share it
let initPromise: Promise<NumbersApiManager> | null = null;
export function getNumbersApi(): Promise<NumbersApiManager> {
  if (!initPromise) {
    initPromise = (async () => {
      const mgr = new NumbersApiManager();
      await mgr.initialize();
      return mgr;
    })();
  }
  return initPromise;
}

Auth token plaintext storage (StorageService.ts)

  • Move auth_token from chrome.storage.local (persisted to disk) to chrome.storage.session (MV3 in-memory only, cleared on browser close).
  • Non-sensitive fields (auth_email, auth_username) remain in local for UX continuity.
  • init() migrates any legacy auth_token from localsession on upgrade, wrapped in try/catch to avoid losing the token if session write fails.
Original prompt

This section details on the original issue you should resolve

<issue_title>[Security][High] Singleton race condition and plaintext auth token storage</issue_title>
<issue_description>## Summary

Two high-severity security findings discovered during deep audit of the main branch (commit cc3b576):

1. Race Condition in getNumbersApi() Singleton (NumbersApiManager.ts:151-159)

The lazy singleton initialization in getNumbersApi() is not guarded against concurrent calls. If two async callers invoke getNumbersApi() simultaneously before the first await instance.initialize() resolves, two separate NumbersApiManager instances will be created. This can cause:

  • Duplicate token restoration — two parallel getCurrentUser() calls to the API
  • State divergence — one caller holds a reference to a different instance than another
  • Auth inconsistency — if one initialize fails and the other succeeds, some components may see logged-in state while others see logged-out

File: src/services/NumbersApiManager.ts lines 150-159

let instance: NumbersApiManager | null = null;

export async function getNumbersApi(): Promise<NumbersApiManager> {
  if (!instance) {
    instance = new NumbersApiManager();  // Not guarded — second caller can enter here
    await instance.initialize();         // Async gap allows concurrent creation
  }
  return instance;
}

Suggested fix: Assign instance before awaiting, or use a mutex/promise-based lock:

let instance: NumbersApiManager | null = null;
let initPromise: Promise<NumbersApiManager> | null = null;

export function getNumbersApi(): Promise<NumbersApiManager> {
  if (!initPromise) {
    initPromise = (async () => {
      const mgr = new NumbersApiManager();
      await mgr.initialize();
      instance = mgr;
      return mgr;
    })();
  }
  return initPromise;
}

2. Auth Token Stored in Plaintext in chrome.storage.local (StorageService.ts:79-85)

The auth token is stored directly in chrome.storage.local without any encryption or obfuscation. While chrome.storage.local is scoped to the extension, any extension with storage permission that shares the same browser profile can potentially read it if Chrome's storage isolation is compromised. Additionally, local storage is written to disk unencrypted.

File: src/services/StorageService.ts lines 79-85

async setAuth(auth: StoredAuth): Promise<void> {
  await chrome.storage.local.set({
    auth_token: auth.token,   // Plaintext token
    auth_email: auth.email,
    auth_username: auth.username,
  });
}

Suggested fix: Use chrome.storage.session (MV3, in-memory only, cleared on browser close) for the auth token while keeping non-sensitive data in local. Alternatively, encrypt the token before storage using crypto.subtle.

Impact

  • Singleton race can cause intermittent auth failures and state corruption
  • Plaintext token storage increases risk if disk or profile is compromised</issue_description>

Comments on the Issue (you are @copilot in this section)


💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.

- NumbersApiManager: replace racy lazy singleton with a promise-based
  lock so concurrent callers all await the same initPromise rather than
  potentially creating multiple instances
- StorageService: move auth_token from chrome.storage.local (persisted
  to disk) to chrome.storage.session (in-memory, cleared on browser
  close); keep non-sensitive email/username in local storage; add
  migration step in init() to move legacy tokens on upgrade

Co-authored-by: numbers-official <181934381+numbers-official@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix singleton race condition in NumbersApiManager fix: singleton race condition and plaintext auth token storage Mar 19, 2026
Copilot AI requested a review from numbers-official March 19, 2026 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Security][High] Singleton race condition and plaintext auth token storage

2 participants