From d4f0d6f8f340466b46509efdd1a98a51810c54dc Mon Sep 17 00:00:00 2001 From: Ross Cadogan Date: Fri, 2 Jan 2026 19:27:17 +0000 Subject: [PATCH 1/2] feat(auth): add tab-based PKCE for Firefox mobile support (Phase 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements OAuth 2.1 + PKCE authentication for Firefox (desktop and Android) using a tab-based flow that intercepts OAuth redirects via browser.tabs.onUpdated. Key changes: - Add TabBasedPKCEAuth.ts with tab-based PKCE flow for Firefox - Update OAuthService.ts to route Firefox to tab-based PKCE - Add isFirefoxAndroid() and isKiwiBrowser() platform detection - Update PKCE_AUTH_SPEC.md with Firefox implementation details The tab-based flow: 1. Opens OAuth authorization URL in a new browser tab 2. Monitors tab URL changes via browser.tabs.onUpdated 3. Detects redirect to extensions.allizom.org 4. Extracts authorization code and exchanges for tokens 5. Closes the auth tab automatically Server requirement: Register Firefox redirect URI to enable PKCE authentication. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- doc/PKCE_AUTH_SPEC.md | 16 +- src/UserAgentModule.ts | 19 +- src/auth/OAuthService.ts | 27 +- src/auth/TabBasedPKCEAuth.ts | 373 ++++++++++++++++++++++++++ test/UserAgentModule.spec.ts | 60 +++++ test/auth/TabBasedPKCEAuth.spec.ts | 416 +++++++++++++++++++++++++++++ 6 files changed, 895 insertions(+), 16 deletions(-) create mode 100644 src/auth/TabBasedPKCEAuth.ts create mode 100644 test/auth/TabBasedPKCEAuth.spec.ts diff --git a/doc/PKCE_AUTH_SPEC.md b/doc/PKCE_AUTH_SPEC.md index 9a285ddb64..84d55c049a 100644 --- a/doc/PKCE_AUTH_SPEC.md +++ b/doc/PKCE_AUTH_SPEC.md @@ -13,7 +13,7 @@ | Extension PKCE implementation | ✅ Done | `OAuthService.ts`, `PKCEManager.ts` | | Chrome redirect URI registered | ✅ Done | `chromiumapp.org` | | Sign-out race condition fix | ✅ Done | `JwtManager.ts` | -| Firefox PKCE support | ⚠️ Partial | Falls back to cookie-based auth | +| Firefox PKCE support | ✅ Done | Tab-based PKCE via `TabBasedPKCEAuth.ts` | | Refresh token rotation | ❓ Untested | Needs verification | --- @@ -210,12 +210,16 @@ The server must whitelist these redirect URI patterns for `client_id=saypi-exten |-------------|--------------|--------| | Chrome (published) | `https://pepnjahikiccmajdphdcgeemmedjdgij.chromiumapp.org/` | ✅ Registered | | Chrome (development) | May vary - extension ID changes in dev mode | ⚠️ Not registered | -| Firefox | `https://.extensions.allizom.org/` | ❌ Not registered | +| Firefox | `https://gecko@saypi.ai.extensions.allizom.org/` | ⚠️ Needs registration | -**Note**: Firefox supports `browser.identity.launchWebAuthFlow()` but the extension currently falls back to cookie-based authentication. Enabling Firefox PKCE would require: -1. Registering the Firefox redirect URI on the server -2. Testing that `hasIdentityAPI()` returns true on Firefox -3. Verifying the flow works with Google/GitHub OAuth (known issues with some providers) +**Firefox PKCE Implementation**: Firefox uses tab-based PKCE authentication via `TabBasedPKCEAuth.ts`: +1. Opens authorization URL in a new browser tab +2. Monitors tab URL changes via `browser.tabs.onUpdated` +3. Detects redirect to `extensions.allizom.org` +4. Extracts authorization code and exchanges for tokens +5. Closes the auth tab + +**Server requirement**: Register the Firefox redirect URI to enable PKCE authentication. ### Testing the Fix diff --git a/src/UserAgentModule.ts b/src/UserAgentModule.ts index 17f370e206..04476db4ca 100644 --- a/src/UserAgentModule.ts +++ b/src/UserAgentModule.ts @@ -6,6 +6,21 @@ export function isFirefox(): boolean { return /Firefox/.test(navigator.userAgent); } +/** + * Check if the browser is Firefox on Android + * Used for mobile-specific auth and UI handling + */ +export function isFirefoxAndroid(): boolean { + return isFirefox() && /Android/.test(navigator.userAgent); +} + +/** + * Check if the browser is Kiwi Browser (Android Chromium with extension support) + */ +export function isKiwiBrowser(): boolean { + return /Kiwi/.test(navigator.userAgent) && /Android/.test(navigator.userAgent); +} + export function isMobileDevice(): boolean { const userAgent = typeof navigator !== "undefined" && typeof navigator.userAgent === "string" @@ -125,11 +140,9 @@ export function getTTSCompatibilityIssue(chatbotType: string): { } export function addUserAgentFlags(): void { - const isFirefoxAndroid: boolean = - /Firefox/.test(navigator.userAgent) && /Android/.test(navigator.userAgent); const element: HTMLElement = document.documentElement; - if (isFirefoxAndroid) { + if (isFirefoxAndroid()) { element.classList.add("firefox-android"); } diff --git a/src/auth/OAuthService.ts b/src/auth/OAuthService.ts index 3d458d4a02..a9f8c6565f 100644 --- a/src/auth/OAuthService.ts +++ b/src/auth/OAuthService.ts @@ -1,10 +1,12 @@ /** * OAuthService - OAuth 2.1 + PKCE authentication flow for browser extensions * - * Handles the complete OAuth authorization flow using browser.identity.launchWebAuthFlow - * for Chrome and fallback to tab-based flow for Firefox. + * Handles the complete OAuth authorization flow: + * - Chrome/Kiwi: Uses browser.identity.launchWebAuthFlow with PKCE + * - Firefox: Uses tab-based PKCE flow with browser.tabs.onUpdated * * @see https://developer.chrome.com/docs/extensions/reference/identity/ + * @see ./TabBasedPKCEAuth.ts for Firefox implementation */ import { browser } from 'wxt/browser'; @@ -18,6 +20,10 @@ import { clearPKCEState, type PKCEState, } from './PKCEManager'; +import { + authenticateWithTabBasedPKCE, + shouldUseTabBasedPKCE, +} from './TabBasedPKCEAuth'; /** * OAuth client ID for the SayPi extension @@ -351,23 +357,30 @@ async function authenticateWithTabFlow(): Promise { /** * Start the OAuth authentication flow - * Uses identity API for Chrome, falls back to tab flow for Firefox + * Uses identity API for Chrome/Kiwi, tab-based PKCE for Firefox */ export async function authenticate(): Promise { if (hasIdentityAPI()) { logger.debug('[OAuthService] Using identity API for authentication'); return authenticateWithIdentityAPI(); - } else { - logger.debug('[OAuthService] Falling back to tab-based authentication'); - return authenticateWithTabFlow(); } + + if (shouldUseTabBasedPKCE()) { + logger.debug('[OAuthService] Using tab-based PKCE for authentication'); + return authenticateWithTabBasedPKCE(); + } + + // Ultimate fallback to cookie-based flow (should rarely be needed) + logger.debug('[OAuthService] Falling back to cookie-based authentication'); + return authenticateWithTabFlow(); } /** * Check if PKCE authentication is supported on this browser + * Returns true for both identity API (Chrome) and tab-based PKCE (Firefox) */ export function isPKCESupported(): boolean { - return hasIdentityAPI(); + return hasIdentityAPI() || shouldUseTabBasedPKCE(); } /** diff --git a/src/auth/TabBasedPKCEAuth.ts b/src/auth/TabBasedPKCEAuth.ts new file mode 100644 index 0000000000..4893c70518 --- /dev/null +++ b/src/auth/TabBasedPKCEAuth.ts @@ -0,0 +1,373 @@ +/** + * TabBasedPKCEAuth - Tab-based PKCE authentication for browsers without identity API + * + * This module implements OAuth 2.1 + PKCE using browser tabs instead of + * browser.identity.launchWebAuthFlow(). Used for Firefox (desktop and Android) + * where the identity API is not available or doesn't work reliably. + * + * Flow: + * 1. Generate PKCE pair (code_verifier, code_challenge) + * 2. Open authorization URL in a new tab + * 3. Listen for tab URL changes via browser.tabs.onUpdated + * 4. Detect redirect to extensions.allizom.org (Firefox redirect URI) + * 5. Extract authorization code from URL + * 6. Exchange code for tokens + * 7. Close the auth tab + */ + +import { browser } from 'wxt/browser'; +import { config } from '../ConfigModule'; +import { logger } from '../LoggingModule'; +import { + generatePKCEPair, + generateState, + storePKCEState, + retrievePKCEState, + clearPKCEState, +} from './PKCEManager'; +import type { OAuthResult, TokenResponse } from './OAuthService'; + +/** + * OAuth client ID for the SayPi extension + */ +const CLIENT_ID = 'saypi-extension'; + +/** + * Timeout for waiting for auth redirect (5 minutes) + */ +const AUTH_TIMEOUT_MS = 5 * 60 * 1000; + +/** + * Pattern to match Firefox extension redirect URIs + * Matches: https://.extensions.allizom.org/... + */ +const FIREFOX_REDIRECT_PATTERN = /\.extensions\.allizom\.org\//; + +/** + * Pattern to match Chrome extension redirect URIs (for Kiwi Browser fallback) + */ +const CHROME_REDIRECT_PATTERN = /\.chromiumapp\.org\//; + +/** + * Get the redirect URI for tab-based PKCE flow + * For Firefox, constructs the extensions.allizom.org URL + */ +export function getTabBasedRedirectUri(): string { + const extensionUrl = browser.runtime.getURL(''); + const extensionId = new URL(extensionUrl).host; + + if (extensionUrl.startsWith('moz-extension://')) { + return `https://${extensionId}.extensions.allizom.org/`; + } + + if (extensionUrl.startsWith('chrome-extension://')) { + return `https://${extensionId}.chromiumapp.org/`; + } + + // Fallback + return `${extensionUrl}oauth-callback`; +} + +/** + * Check if a URL matches a redirect URI pattern + */ +function isRedirectUrl(url: string): boolean { + return FIREFOX_REDIRECT_PATTERN.test(url) || CHROME_REDIRECT_PATTERN.test(url); +} + +/** + * Build the authorization URL with PKCE parameters + */ +function buildAuthorizationUrl( + codeChallenge: string, + state: string, + redirectUri: string +): string { + const authUrl = new URL('/api/oauth/authorize', config.authServerUrl); + + authUrl.searchParams.set('response_type', 'code'); + authUrl.searchParams.set('client_id', CLIENT_ID); + authUrl.searchParams.set('redirect_uri', redirectUri); + authUrl.searchParams.set('code_challenge', codeChallenge); + authUrl.searchParams.set('code_challenge_method', 'S256'); + authUrl.searchParams.set('state', state); + authUrl.searchParams.set('scope', 'openid profile'); + + return authUrl.toString(); +} + +/** + * Parse the authorization response from the redirect URL + */ +function parseAuthorizationResponse(redirectUrl: string): { + code?: string; + state?: string; + error?: string; + errorDescription?: string; +} { + const url = new URL(redirectUrl); + const params = url.searchParams; + + return { + code: params.get('code') || undefined, + state: params.get('state') || undefined, + error: params.get('error') || undefined, + errorDescription: params.get('error_description') || undefined, + }; +} + +/** + * Exchange authorization code for tokens + */ +async function exchangeCodeForTokens( + code: string, + codeVerifier: string, + redirectUri: string +): Promise { + const tokenUrl = `${config.authServerUrl}/api/oauth/token`; + + const response = await fetch(tokenUrl, { + method: 'POST', + headers: { + 'Content-Type': 'application/x-www-form-urlencoded', + }, + body: new URLSearchParams({ + grant_type: 'authorization_code', + code, + code_verifier: codeVerifier, + client_id: CLIENT_ID, + redirect_uri: redirectUri, + }), + }); + + if (!response.ok) { + const errorData = await response.json().catch(() => ({})); + throw new Error( + errorData.error_description || + errorData.error || + `Token exchange failed: ${response.status}` + ); + } + + return response.json(); +} + +/** + * Perform OAuth 2.1 + PKCE authentication using tab-based flow + * + * This is used for Firefox (desktop and Android) where browser.identity + * API is not available. + */ +export async function authenticateWithTabBasedPKCE(): Promise { + if (!config.authServerUrl) { + return { + success: false, + error: 'configuration_error', + errorDescription: 'Auth server URL not configured', + }; + } + + let authTabId: number | undefined; + let tabListener: (( + tabId: number, + changeInfo: browser.Tabs.OnUpdatedChangeInfoType, + tab: browser.Tabs.Tab + ) => void) | undefined; + let timeoutId: ReturnType | undefined; + + const cleanup = async () => { + if (tabListener) { + browser.tabs.onUpdated.removeListener(tabListener); + tabListener = undefined; + } + if (timeoutId) { + clearTimeout(timeoutId); + timeoutId = undefined; + } + if (authTabId !== undefined) { + try { + await browser.tabs.remove(authTabId); + } catch { + // Tab may already be closed + } + authTabId = undefined; + } + }; + + try { + // Generate PKCE pair + const { codeVerifier, codeChallenge } = await generatePKCEPair(); + const state = generateState(); + const redirectUri = getTabBasedRedirectUri(); + + logger.debug('[TabBasedPKCE] Starting tab-based PKCE auth flow', { + redirectUri, + }); + + // Store PKCE state for verification + await storePKCEState({ + codeVerifier, + state, + redirectUri, + createdAt: Date.now(), + }); + + // Build authorization URL + const authUrl = buildAuthorizationUrl(codeChallenge, state, redirectUri); + + // Create a promise that resolves when we detect the redirect + const authPromise = new Promise((resolve) => { + // Set up tab listener + tabListener = (tabId, changeInfo, tab) => { + // Only watch our auth tab + if (tabId !== authTabId) return; + + // Check for URL changes + if (changeInfo.url && isRedirectUrl(changeInfo.url)) { + logger.debug('[TabBasedPKCE] Detected redirect URL', { + url: changeInfo.url, + }); + + // Parse the response + const response = parseAuthorizationResponse(changeInfo.url); + + // Handle OAuth errors + if (response.error) { + cleanup(); + clearPKCEState(); + resolve({ + success: false, + error: response.error, + errorDescription: response.errorDescription, + }); + return; + } + + if (!response.code || !response.state) { + cleanup(); + clearPKCEState(); + resolve({ + success: false, + error: 'invalid_response', + errorDescription: 'Missing code or state in response', + }); + return; + } + + // Exchange code for tokens + retrievePKCEState(response.state) + .then(async (pkceState) => { + if (!pkceState) { + return { + success: false as const, + error: 'state_mismatch', + errorDescription: 'Invalid or expired state parameter', + }; + } + + const tokens = await exchangeCodeForTokens( + response.code!, + pkceState.codeVerifier, + pkceState.redirectUri + ); + + logger.info('[TabBasedPKCE] Authentication successful'); + return { success: true as const, tokens }; + }) + .then((result) => { + cleanup(); + resolve(result); + }) + .catch((error) => { + cleanup(); + resolve({ + success: false, + error: 'token_exchange_failed', + errorDescription: error.message || 'Failed to exchange code for tokens', + }); + }); + } + + // Check if tab was closed by user + if (changeInfo.status === 'complete' && tab.url === 'about:blank') { + // Tab was likely closed and replaced + cleanup(); + clearPKCEState(); + resolve({ + success: false, + error: 'auth_cancelled', + errorDescription: 'Authentication tab was closed', + }); + } + }; + + browser.tabs.onUpdated.addListener(tabListener); + + // Set up timeout + timeoutId = setTimeout(() => { + cleanup(); + clearPKCEState(); + resolve({ + success: false, + error: 'auth_timeout', + errorDescription: 'Authentication timed out', + }); + }, AUTH_TIMEOUT_MS); + }); + + // Also listen for tab removal (user closes tab) + const removeListener = (tabId: number) => { + if (tabId === authTabId) { + browser.tabs.onRemoved.removeListener(removeListener); + cleanup(); + clearPKCEState(); + } + }; + browser.tabs.onRemoved.addListener(removeListener); + + // Open the auth tab + logger.debug('[TabBasedPKCE] Opening auth tab'); + const tab = await browser.tabs.create({ url: authUrl, active: true }); + authTabId = tab.id; + + if (authTabId === undefined) { + await cleanup(); + await clearPKCEState(); + return { + success: false, + error: 'tab_creation_failed', + errorDescription: 'Failed to create authentication tab', + }; + } + + return authPromise; + } catch (error: any) { + await cleanup(); + await clearPKCEState(); + + logger.error('[TabBasedPKCE] Authentication failed:', error); + + return { + success: false, + error: 'auth_failed', + errorDescription: error.message || 'Authentication failed', + }; + } +} + +/** + * Check if tab-based PKCE should be used + * Returns true for Firefox browsers where identity API is not available + */ +export function shouldUseTabBasedPKCE(): boolean { + // Check if identity API is available + const hasIdentityAPI = typeof browser.identity?.launchWebAuthFlow === 'function'; + + if (hasIdentityAPI) { + // Identity API is available, prefer it + return false; + } + + // Identity API not available, use tab-based PKCE + return true; +} diff --git a/test/UserAgentModule.spec.ts b/test/UserAgentModule.spec.ts index 1002bf55ba..ed4b7aca88 100644 --- a/test/UserAgentModule.spec.ts +++ b/test/UserAgentModule.spec.ts @@ -2,6 +2,8 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { isSafari, isFirefox, + isFirefoxAndroid, + isKiwiBrowser, isMobileDevice, addUserAgentFlags, addDeviceFlags, @@ -64,6 +66,64 @@ describe("UserAgentModule", () => { }); }); + describe("isFirefoxAndroid", () => { + it("should return true for Firefox on Android", () => { + Object.defineProperty(navigator, "userAgent", { + value: + "Mozilla/5.0 (Android 12; Mobile; rv:68.0) Gecko/68.0 Firefox/89.0", + configurable: true, + }); + expect(isFirefoxAndroid()).toBe(true); + }); + + it("should return false for Firefox on desktop", () => { + Object.defineProperty(navigator, "userAgent", { + value: + "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:89.0) Gecko/20100101 Firefox/89.0", + configurable: true, + }); + expect(isFirefoxAndroid()).toBe(false); + }); + + it("should return false for Chrome on Android", () => { + Object.defineProperty(navigator, "userAgent", { + value: + "Mozilla/5.0 (Linux; Android 12; SM-G998B) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.120 Mobile Safari/537.36", + configurable: true, + }); + expect(isFirefoxAndroid()).toBe(false); + }); + }); + + describe("isKiwiBrowser", () => { + it("should return true for Kiwi Browser on Android", () => { + Object.defineProperty(navigator, "userAgent", { + value: + "Mozilla/5.0 (Linux; Android 12; SM-G998B) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.120 Mobile Safari/537.36 Kiwi", + configurable: true, + }); + expect(isKiwiBrowser()).toBe(true); + }); + + it("should return false for regular Chrome on Android", () => { + Object.defineProperty(navigator, "userAgent", { + value: + "Mozilla/5.0 (Linux; Android 12; SM-G998B) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.120 Mobile Safari/537.36", + configurable: true, + }); + expect(isKiwiBrowser()).toBe(false); + }); + + it("should return false for Chrome on desktop", () => { + Object.defineProperty(navigator, "userAgent", { + value: + "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.124 Safari/537.36", + configurable: true, + }); + expect(isKiwiBrowser()).toBe(false); + }); + }); + describe("isMobileDevice", () => { it("should return true for mobile user agent", () => { Object.defineProperty(navigator, "userAgent", { diff --git a/test/auth/TabBasedPKCEAuth.spec.ts b/test/auth/TabBasedPKCEAuth.spec.ts new file mode 100644 index 0000000000..57dddfbb61 --- /dev/null +++ b/test/auth/TabBasedPKCEAuth.spec.ts @@ -0,0 +1,416 @@ +/** + * Tests for TabBasedPKCEAuth - Tab-based PKCE authentication for Firefox + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +// Mock wxt/browser before importing +const mockStorage: Record = {}; +const mockTabs: Map = new Map(); +let tabIdCounter = 1; +let onUpdatedListeners: Array<(tabId: number, changeInfo: any, tab: any) => void> = []; +let onRemovedListeners: Array<(tabId: number) => void> = []; + +vi.mock('wxt/browser', () => ({ + browser: { + storage: { + local: { + get: vi.fn((key: string) => Promise.resolve({ [key]: mockStorage[key] })), + set: vi.fn((data: Record) => { + Object.assign(mockStorage, data); + return Promise.resolve(); + }), + remove: vi.fn((key: string) => { + delete mockStorage[key]; + return Promise.resolve(); + }), + }, + }, + runtime: { + getURL: vi.fn(() => 'moz-extension://test-addon-id/'), + }, + tabs: { + create: vi.fn(async (options: { url: string; active?: boolean }) => { + const id = tabIdCounter++; + const tab = { id, url: options.url }; + mockTabs.set(id, tab); + return tab; + }), + remove: vi.fn(async (tabId: number) => { + mockTabs.delete(tabId); + }), + onUpdated: { + addListener: vi.fn((listener: any) => { + onUpdatedListeners.push(listener); + }), + removeListener: vi.fn((listener: any) => { + onUpdatedListeners = onUpdatedListeners.filter(l => l !== listener); + }), + }, + onRemoved: { + addListener: vi.fn((listener: any) => { + onRemovedListeners.push(listener); + }), + removeListener: vi.fn((listener: any) => { + onRemovedListeners = onRemovedListeners.filter(l => l !== listener); + }), + }, + }, + identity: undefined, // Simulate Firefox without identity API + }, +})); + +// Mock config +vi.mock('../../src/ConfigModule', () => ({ + config: { + authServerUrl: 'https://test.saypi.ai', + }, +})); + +// Mock logger +vi.mock('../../src/LoggingModule', () => ({ + logger: { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }, +})); + +import { + getTabBasedRedirectUri, + authenticateWithTabBasedPKCE, + shouldUseTabBasedPKCE, +} from '../../src/auth/TabBasedPKCEAuth'; +import { browser } from 'wxt/browser'; + +describe('TabBasedPKCEAuth', () => { + beforeEach(() => { + // Clear mock storage + Object.keys(mockStorage).forEach(key => delete mockStorage[key]); + // Clear mock tabs + mockTabs.clear(); + tabIdCounter = 1; + // Clear listeners + onUpdatedListeners = []; + onRemovedListeners = []; + // Reset call counts but keep implementations + vi.resetAllMocks(); + // Ensure runtime.getURL returns Firefox URL by default + (browser.runtime.getURL as any).mockReturnValue('moz-extension://test-addon-id/'); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + describe('getTabBasedRedirectUri', () => { + it('returns Firefox extensions.allizom.org URI for moz-extension', () => { + (browser.runtime.getURL as any).mockReturnValue('moz-extension://test-addon-id/'); + + const uri = getTabBasedRedirectUri(); + + expect(uri).toBe('https://test-addon-id.extensions.allizom.org/'); + }); + + it('returns Chrome chromiumapp.org URI for chrome-extension', () => { + (browser.runtime.getURL as any).mockReturnValue('chrome-extension://test-extension-id/'); + + const uri = getTabBasedRedirectUri(); + + expect(uri).toBe('https://test-extension-id.chromiumapp.org/'); + }); + + it('returns fallback URI for unknown protocol', () => { + (browser.runtime.getURL as any).mockReturnValue('unknown://test-id/'); + + const uri = getTabBasedRedirectUri(); + + expect(uri).toBe('unknown://test-id/oauth-callback'); + }); + }); + + describe('shouldUseTabBasedPKCE', () => { + it('returns true when identity API is not available', () => { + // browser.identity is undefined in our mock + expect(shouldUseTabBasedPKCE()).toBe(true); + }); + + it('returns false when identity API is available', () => { + // Temporarily add identity API + const originalIdentity = (browser as any).identity; + (browser as any).identity = { + launchWebAuthFlow: vi.fn(), + }; + + expect(shouldUseTabBasedPKCE()).toBe(false); + + // Restore + (browser as any).identity = originalIdentity; + }); + }); + + describe('authenticateWithTabBasedPKCE', () => { + beforeEach(() => { + vi.useFakeTimers(); + // Mock fetch for token exchange + global.fetch = vi.fn(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('creates a tab with authorization URL', async () => { + // Start authentication (don't await, we'll simulate the redirect) + const authPromise = authenticateWithTabBasedPKCE(); + + // Wait for tab to be created + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalled()); + + // Verify tab was created with correct URL + expect(browser.tabs.create).toHaveBeenCalledWith( + expect.objectContaining({ + url: expect.stringContaining('/api/oauth/authorize'), + active: true, + }) + ); + + // Verify URL contains PKCE parameters + const createCall = (browser.tabs.create as any).mock.calls[0][0]; + expect(createCall.url).toContain('code_challenge='); + expect(createCall.url).toContain('code_challenge_method=S256'); + expect(createCall.url).toContain('state='); + expect(createCall.url).toContain('client_id=saypi-extension'); + + // Simulate timeout to complete the test + vi.advanceTimersByTime(5 * 60 * 1000 + 1000); + await vi.runAllTimersAsync(); + + const result = await authPromise; + expect(result.success).toBe(false); + expect(result.error).toBe('auth_timeout'); + }); + + it('stores PKCE state before opening tab', async () => { + const authPromise = authenticateWithTabBasedPKCE(); + + // Wait for tab to be created (which happens after PKCE state is stored) + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalled()); + + // Verify PKCE state was stored with Firefox redirect URI + expect(browser.storage.local.set).toHaveBeenCalledWith( + expect.objectContaining({ + 'saypi-pkce-state': expect.objectContaining({ + codeVerifier: expect.any(String), + state: expect.any(String), + redirectUri: expect.stringContaining('extensions.allizom.org'), + createdAt: expect.any(Number), + }), + }) + ); + + // Cleanup - advance past timeout + vi.advanceTimersByTime(5 * 60 * 1000 + 1000); + await vi.runAllTimersAsync(); + await authPromise; + }); + + it('detects redirect URL and extracts authorization code', async () => { + // Mock successful token exchange + (global.fetch as any).mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ + access_token: 'test-access-token', + token_type: 'Bearer', + expires_in: 3600, + refresh_token: 'test-refresh-token', + scope: 'openid profile', + }), + }); + + // Start authentication + const authPromise = authenticateWithTabBasedPKCE(); + + // Wait for tab to be created (happens after PKCE state is stored) + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalled()); + + // Get the stored state + const storedState = mockStorage['saypi-pkce-state']; + expect(storedState).toBeDefined(); + + // Simulate redirect by triggering onUpdated listener + const tabId = 1; + const redirectUrl = `https://test-addon-id.extensions.allizom.org/?code=test-auth-code&state=${storedState.state}`; + + // Trigger the onUpdated listener + for (const listener of onUpdatedListeners) { + listener(tabId, { url: redirectUrl }, { id: tabId, url: redirectUrl }); + } + + await vi.runAllTimersAsync(); + const result = await authPromise; + + expect(result.success).toBe(true); + if (result.success) { + expect(result.tokens.access_token).toBe('test-access-token'); + expect(result.tokens.refresh_token).toBe('test-refresh-token'); + } + }); + + it('handles OAuth error in redirect URL', async () => { + const authPromise = authenticateWithTabBasedPKCE(); + + // Wait for tab to be created + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalled()); + + // Simulate error redirect + const tabId = 1; + const errorUrl = 'https://test-addon-id.extensions.allizom.org/?error=access_denied&error_description=User%20denied%20access'; + + for (const listener of onUpdatedListeners) { + listener(tabId, { url: errorUrl }, { id: tabId, url: errorUrl }); + } + + await vi.runAllTimersAsync(); + const result = await authPromise; + + expect(result.success).toBe(false); + expect(result.error).toBe('access_denied'); + expect(result.errorDescription).toBe('User denied access'); + }); + + it('handles state mismatch (CSRF protection)', async () => { + const authPromise = authenticateWithTabBasedPKCE(); + + // Wait for tab to be created + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalled()); + + // Simulate redirect with wrong state + const tabId = 1; + const wrongStateUrl = 'https://test-addon-id.extensions.allizom.org/?code=test-code&state=wrong-state'; + + for (const listener of onUpdatedListeners) { + listener(tabId, { url: wrongStateUrl }, { id: tabId, url: wrongStateUrl }); + } + + await vi.runAllTimersAsync(); + const result = await authPromise; + + expect(result.success).toBe(false); + expect(result.error).toBe('state_mismatch'); + }); + + it('handles token exchange failure', async () => { + // Mock failed token exchange + (global.fetch as any).mockResolvedValue({ + ok: false, + status: 400, + json: () => Promise.resolve({ + error: 'invalid_grant', + error_description: 'Authorization code expired', + }), + }); + + const authPromise = authenticateWithTabBasedPKCE(); + + // Wait for tab to be created + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalled()); + + const storedState = mockStorage['saypi-pkce-state']; + const tabId = 1; + const redirectUrl = `https://test-addon-id.extensions.allizom.org/?code=test-code&state=${storedState.state}`; + + for (const listener of onUpdatedListeners) { + listener(tabId, { url: redirectUrl }, { id: tabId, url: redirectUrl }); + } + + await vi.runAllTimersAsync(); + const result = await authPromise; + + expect(result.success).toBe(false); + expect(result.error).toBe('token_exchange_failed'); + }); + + it('handles timeout', async () => { + const authPromise = authenticateWithTabBasedPKCE(); + + // Wait for tab to be created + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalled()); + + // Advance time past timeout + vi.advanceTimersByTime(5 * 60 * 1000 + 1000); + await vi.runAllTimersAsync(); + + const result = await authPromise; + + expect(result.success).toBe(false); + expect(result.error).toBe('auth_timeout'); + }); + + it('cleans up listeners and closes tab after success', async () => { + (global.fetch as any).mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ + access_token: 'test-token', + token_type: 'Bearer', + expires_in: 3600, + refresh_token: 'test-refresh', + scope: 'openid profile', + }), + }); + + const authPromise = authenticateWithTabBasedPKCE(); + + // Wait for tab to be created + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalled()); + + const storedState = mockStorage['saypi-pkce-state']; + const tabId = 1; + const redirectUrl = `https://test-addon-id.extensions.allizom.org/?code=test-code&state=${storedState.state}`; + + for (const listener of [...onUpdatedListeners]) { + listener(tabId, { url: redirectUrl }, { id: tabId, url: redirectUrl }); + } + + await vi.runAllTimersAsync(); + await authPromise; + + // Verify cleanup + expect(browser.tabs.onUpdated.removeListener).toHaveBeenCalled(); + expect(browser.tabs.remove).toHaveBeenCalledWith(tabId); + }); + + it('clears PKCE state after authentication', async () => { + (global.fetch as any).mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ + access_token: 'test-token', + token_type: 'Bearer', + expires_in: 3600, + refresh_token: 'test-refresh', + scope: 'openid profile', + }), + }); + + const authPromise = authenticateWithTabBasedPKCE(); + + // Wait for tab to be created + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalled()); + + const storedState = mockStorage['saypi-pkce-state']; + const tabId = 1; + const redirectUrl = `https://test-addon-id.extensions.allizom.org/?code=test-code&state=${storedState.state}`; + + for (const listener of [...onUpdatedListeners]) { + listener(tabId, { url: redirectUrl }, { id: tabId, url: redirectUrl }); + } + + await vi.runAllTimersAsync(); + await authPromise; + + // PKCE state should be cleared (retrievePKCEState clears it on retrieval) + expect(browser.storage.local.remove).toHaveBeenCalledWith('saypi-pkce-state'); + }); + }); +}); From cf1ad22d5a66d45fced04debf27b55008b30237e Mon Sep 17 00:00:00 2001 From: Ross Cadogan Date: Fri, 2 Jan 2026 21:08:19 +0000 Subject: [PATCH 2/2] fix(auth): address code review issues in TabBasedPKCEAuth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes 5 issues identified in code review: 1. Memory leak - `removeListener` now tracked and cleaned up - Added `removeListener` variable to cleanup scope - cleanup() now removes both onUpdated and onRemoved listeners 2. Race condition - listeners now registered AFTER tab creation - Moved browser.tabs.create() before listener registration - Added guard checks for undefined authTabId in listeners 3. Async cleanup - made cleanup synchronous where needed - Changed cleanup() to synchronous function - Listener removal is immediate; tab removal is fire-and-forget - Added isCleanedUp guard flag to prevent multiple cleanups 4. Concurrent auth vulnerability - added authInProgress guard - Module-level flag prevents overlapping auth attempts - Returns 'auth_in_progress' error if already authenticating 5. Promise hanging when user closes tab - removeListener now resolves - removeListener callback now resolves promise with 'auth_cancelled' - Logs when user closes auth tab directly Added 5 new tests: - Concurrent auth prevention - User closing auth tab directly - removeListener cleanup verification - Auth allowed after previous completes - Listeners registered after tab creation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- src/auth/TabBasedPKCEAuth.ts | 122 ++++++++++++++++++++--------- test/auth/TabBasedPKCEAuth.spec.ts | 121 ++++++++++++++++++++++++++++ 2 files changed, 207 insertions(+), 36 deletions(-) diff --git a/src/auth/TabBasedPKCEAuth.ts b/src/auth/TabBasedPKCEAuth.ts index 4893c70518..ff58aa75a4 100644 --- a/src/auth/TabBasedPKCEAuth.ts +++ b/src/auth/TabBasedPKCEAuth.ts @@ -48,6 +48,11 @@ const FIREFOX_REDIRECT_PATTERN = /\.extensions\.allizom\.org\//; */ const CHROME_REDIRECT_PATTERN = /\.chromiumapp\.org\//; +/** + * Guard flag to prevent concurrent authentication attempts + */ +let authInProgress = false; + /** * Get the redirect URI for tab-based PKCE flow * For Firefox, constructs the extensions.allizom.org URL @@ -159,6 +164,15 @@ async function exchangeCodeForTokens( * API is not available. */ export async function authenticateWithTabBasedPKCE(): Promise { + // Prevent concurrent authentication attempts + if (authInProgress) { + return { + success: false, + error: 'auth_in_progress', + errorDescription: 'Authentication already in progress', + }; + } + if (!config.authServerUrl) { return { success: false, @@ -167,31 +181,52 @@ export async function authenticateWithTabBasedPKCE(): Promise { }; } + authInProgress = true; + let authTabId: number | undefined; let tabListener: (( tabId: number, changeInfo: browser.Tabs.OnUpdatedChangeInfoType, tab: browser.Tabs.Tab ) => void) | undefined; + let removeListener: ((tabId: number) => void) | undefined; let timeoutId: ReturnType | undefined; - - const cleanup = async () => { + let isCleanedUp = false; + + /** + * Cleanup function that removes all listeners and closes the auth tab. + * Uses a guard flag to prevent multiple cleanups. + * Synchronous listener removal happens immediately; tab removal is fire-and-forget. + */ + const cleanup = () => { + if (isCleanedUp) return; + isCleanedUp = true; + + // Synchronous cleanup - remove listeners immediately if (tabListener) { browser.tabs.onUpdated.removeListener(tabListener); tabListener = undefined; } + if (removeListener) { + browser.tabs.onRemoved.removeListener(removeListener); + removeListener = undefined; + } if (timeoutId) { clearTimeout(timeoutId); timeoutId = undefined; } + + // Async cleanup - fire and forget (tab removal doesn't need to complete before we return) if (authTabId !== undefined) { - try { - await browser.tabs.remove(authTabId); - } catch { - // Tab may already be closed - } + const tabIdToClose = authTabId; authTabId = undefined; + browser.tabs.remove(tabIdToClose).catch(() => { + // Tab may already be closed - ignore errors + }); } + + // Clear auth in progress flag + authInProgress = false; }; try { @@ -215,12 +250,27 @@ export async function authenticateWithTabBasedPKCE(): Promise { // Build authorization URL const authUrl = buildAuthorizationUrl(codeChallenge, state, redirectUri); + // Open the auth tab FIRST so we have the tab ID before registering listeners + logger.debug('[TabBasedPKCE] Opening auth tab'); + const tab = await browser.tabs.create({ url: authUrl, active: true }); + authTabId = tab.id; + + if (authTabId === undefined) { + cleanup(); + await clearPKCEState(); + return { + success: false, + error: 'tab_creation_failed', + errorDescription: 'Failed to create authentication tab', + }; + } + // Create a promise that resolves when we detect the redirect const authPromise = new Promise((resolve) => { - // Set up tab listener + // Set up tab listener for URL changes tabListener = (tabId, changeInfo, tab) => { - // Only watch our auth tab - if (tabId !== authTabId) return; + // Only watch our auth tab - guard against undefined authTabId + if (authTabId === undefined || tabId !== authTabId) return; // Check for URL changes if (changeInfo.url && isRedirectUrl(changeInfo.url)) { @@ -301,7 +351,24 @@ export async function authenticateWithTabBasedPKCE(): Promise { } }; + // Set up listener for tab removal (user closes tab directly) + removeListener = (tabId: number) => { + // Guard against undefined authTabId or already cleaned up + if (authTabId === undefined || tabId !== authTabId) return; + + logger.debug('[TabBasedPKCE] Auth tab was closed by user'); + cleanup(); + clearPKCEState(); + resolve({ + success: false, + error: 'auth_cancelled', + errorDescription: 'Authentication tab was closed by user', + }); + }; + + // Register listeners AFTER we have authTabId browser.tabs.onUpdated.addListener(tabListener); + browser.tabs.onRemoved.addListener(removeListener); // Set up timeout timeoutId = setTimeout(() => { @@ -315,34 +382,9 @@ export async function authenticateWithTabBasedPKCE(): Promise { }, AUTH_TIMEOUT_MS); }); - // Also listen for tab removal (user closes tab) - const removeListener = (tabId: number) => { - if (tabId === authTabId) { - browser.tabs.onRemoved.removeListener(removeListener); - cleanup(); - clearPKCEState(); - } - }; - browser.tabs.onRemoved.addListener(removeListener); - - // Open the auth tab - logger.debug('[TabBasedPKCE] Opening auth tab'); - const tab = await browser.tabs.create({ url: authUrl, active: true }); - authTabId = tab.id; - - if (authTabId === undefined) { - await cleanup(); - await clearPKCEState(); - return { - success: false, - error: 'tab_creation_failed', - errorDescription: 'Failed to create authentication tab', - }; - } - return authPromise; } catch (error: any) { - await cleanup(); + cleanup(); await clearPKCEState(); logger.error('[TabBasedPKCE] Authentication failed:', error); @@ -371,3 +413,11 @@ export function shouldUseTabBasedPKCE(): boolean { // Identity API not available, use tab-based PKCE return true; } + +/** + * Reset auth state for testing purposes. + * @internal This is only exported for testing - do not use in production code. + */ +export function _resetAuthStateForTesting(): void { + authInProgress = false; +} diff --git a/test/auth/TabBasedPKCEAuth.spec.ts b/test/auth/TabBasedPKCEAuth.spec.ts index 57dddfbb61..aa73bccdee 100644 --- a/test/auth/TabBasedPKCEAuth.spec.ts +++ b/test/auth/TabBasedPKCEAuth.spec.ts @@ -81,6 +81,7 @@ import { getTabBasedRedirectUri, authenticateWithTabBasedPKCE, shouldUseTabBasedPKCE, + _resetAuthStateForTesting, } from '../../src/auth/TabBasedPKCEAuth'; import { browser } from 'wxt/browser'; @@ -98,6 +99,8 @@ describe('TabBasedPKCEAuth', () => { vi.resetAllMocks(); // Ensure runtime.getURL returns Firefox URL by default (browser.runtime.getURL as any).mockReturnValue('moz-extension://test-addon-id/'); + // Reset auth state between tests + _resetAuthStateForTesting(); }); afterEach(() => { @@ -412,5 +415,123 @@ describe('TabBasedPKCEAuth', () => { // PKCE state should be cleared (retrievePKCEState clears it on retrieval) expect(browser.storage.local.remove).toHaveBeenCalledWith('saypi-pkce-state'); }); + + it('prevents concurrent authentication attempts', async () => { + // Start first authentication + const authPromise1 = authenticateWithTabBasedPKCE(); + + // Wait for tab to be created + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalled()); + + // Try to start second authentication while first is in progress + const result2 = await authenticateWithTabBasedPKCE(); + + // Second call should fail immediately with auth_in_progress + expect(result2.success).toBe(false); + expect(result2.error).toBe('auth_in_progress'); + expect(result2.errorDescription).toBe('Authentication already in progress'); + + // Clean up first auth by advancing past timeout + vi.advanceTimersByTime(5 * 60 * 1000 + 1000); + await vi.runAllTimersAsync(); + await authPromise1; + }); + + it('handles user closing auth tab directly', async () => { + const authPromise = authenticateWithTabBasedPKCE(); + + // Wait for tab to be created and listeners registered + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalled()); + await vi.waitFor(() => expect(onRemovedListeners.length).toBeGreaterThan(0)); + + // Simulate user closing the auth tab + const tabId = 1; + for (const listener of [...onRemovedListeners]) { + listener(tabId); + } + + await vi.runAllTimersAsync(); + const result = await authPromise; + + // Should return auth_cancelled error + expect(result.success).toBe(false); + expect(result.error).toBe('auth_cancelled'); + expect(result.errorDescription).toContain('closed by user'); + }); + + it('cleans up removeListener on success', async () => { + (global.fetch as any).mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ + access_token: 'test-token', + token_type: 'Bearer', + expires_in: 3600, + refresh_token: 'test-refresh', + scope: 'openid profile', + }), + }); + + const authPromise = authenticateWithTabBasedPKCE(); + + // Wait for tab to be created + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalled()); + + const storedState = mockStorage['saypi-pkce-state']; + const tabId = 1; + const redirectUrl = `https://test-addon-id.extensions.allizom.org/?code=test-code&state=${storedState.state}`; + + for (const listener of [...onUpdatedListeners]) { + listener(tabId, { url: redirectUrl }, { id: tabId, url: redirectUrl }); + } + + await vi.runAllTimersAsync(); + await authPromise; + + // Verify both listeners are cleaned up + expect(browser.tabs.onUpdated.removeListener).toHaveBeenCalled(); + expect(browser.tabs.onRemoved.removeListener).toHaveBeenCalled(); + }); + + it('allows new auth after previous auth completes', async () => { + // First auth - timeout + const authPromise1 = authenticateWithTabBasedPKCE(); + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalledTimes(1)); + vi.advanceTimersByTime(5 * 60 * 1000 + 1000); + await vi.runAllTimersAsync(); + const result1 = await authPromise1; + expect(result1.error).toBe('auth_timeout'); + + // Second auth should be allowed after first completes + const authPromise2 = authenticateWithTabBasedPKCE(); + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalledTimes(2)); + + // Verify second auth started successfully (got past the guard) + vi.advanceTimersByTime(5 * 60 * 1000 + 1000); + await vi.runAllTimersAsync(); + const result2 = await authPromise2; + expect(result2.error).toBe('auth_timeout'); // Expected - we didn't simulate a redirect + }); + + it('registers listeners only after tab is created', async () => { + const authPromise = authenticateWithTabBasedPKCE(); + + // Wait for tab to be created + await vi.waitFor(() => expect(browser.tabs.create).toHaveBeenCalled()); + + // Now verify listeners were registered AFTER tab creation + // The tab should have ID 1, and listeners should be registered + expect(browser.tabs.onUpdated.addListener).toHaveBeenCalled(); + expect(browser.tabs.onRemoved.addListener).toHaveBeenCalled(); + + // Verify the order: create was called before addListener + // Check that onUpdated.addListener was called and that listeners are set up + expect(onUpdatedListeners.length).toBeGreaterThan(0); + expect(onRemovedListeners.length).toBeGreaterThan(0); + + // Cleanup + vi.advanceTimersByTime(5 * 60 * 1000 + 1000); + await vi.runAllTimersAsync(); + await authPromise; + }); }); });