Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/main/ipc/githubIpc.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { ipcMain, app } from 'electron';
import { log } from '../lib/logger';
import { GitHubService } from '../services/GitHubService';
import { githubService } from '../services/GitHubService';
import { worktreeService } from '../services/WorktreeService';
import { githubCLIInstaller } from '../services/GitHubCLIInstaller';
import { databaseService } from '../services/DatabaseService';
Expand All @@ -14,7 +14,6 @@ import { quoteShellArg } from '../utils/shellEscape';
import { getAppSettings } from '../settings';

const execAsync = promisify(exec);
const githubService = new GitHubService();

const slugify = (name: string) =>
name
Expand Down
121 changes: 111 additions & 10 deletions src/main/services/GitHubService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,22 @@
export class GitHubService {
private readonly SERVICE_NAME = 'emdash-github';
private readonly ACCOUNT_NAME = 'github-token';
private readonly MIGRATION_BLOCK_ACCOUNT = 'github-migration-blocked';

// Polling state management
private isPolling = false;
private pollingInterval: NodeJS.Timeout | null = null;
private currentDeviceCode: string | null = null;
private currentInterval = 5;

// One-shot migration guard: try reading from `gh auth token` at most once
// per process when the Emdash keychain is empty.
private migrationAttempted = false;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
private migrationInFlight: Promise<string | null> | null = null;

// Serializes auth state changes (logout + legacy token migration persistence).
private authStateLock: Promise<void> = Promise.resolve();

/**
* Authenticate with GitHub using Device Flow
* Returns device code info for the UI to display to the user
Expand Down Expand Up @@ -1258,44 +1267,136 @@
}
}

private async withAuthStateLock<T>(operation: () => Promise<T>): Promise<T> {
const previousLock = this.authStateLock;
let releaseLock: (() => void) | null = null;

this.authStateLock = new Promise<void>((resolve) => {
releaseLock = resolve;
});

await previousLock;

try {
return await operation();
} finally {
releaseLock?.();

Check failure on line 1283 in src/main/services/GitHubService.ts

View workflow job for this annotation

GitHub Actions / format-check

This expression is not callable.
}
}

/**
* Logout and clear stored token
*/
async logout(): Promise<void> {
this.stopPolling();
try {
const keytar = await import('keytar');
await keytar.deletePassword(this.SERVICE_NAME, this.ACCOUNT_NAME);
} catch (error) {
console.error('Failed to clear keychain token:', error);
throw new Error('Failed to clear keychain token');
}
this.migrationAttempted = true;

await this.withAuthStateLock(async () => {
try {
const keytar = await import('keytar');
await keytar.deletePassword(this.SERVICE_NAME, this.ACCOUNT_NAME);
await keytar.setPassword(this.SERVICE_NAME, this.MIGRATION_BLOCK_ACCOUNT, '1');
} catch (error) {
console.error('Failed to clear keychain token:', error);
throw new Error('Failed to clear keychain token');
}
});
}

/**
* Store authentication token securely
*/
private async storeToken(token: string): Promise<void> {
private async storeToken(token: string, source: 'user' | 'migration' = 'user'): Promise<void> {
try {
const keytar = await import('keytar');
await keytar.setPassword(this.SERVICE_NAME, this.ACCOUNT_NAME, token);
if (source === 'user') {
await keytar.deletePassword(this.SERVICE_NAME, this.MIGRATION_BLOCK_ACCOUNT);
}
} catch (error) {
console.error('Failed to store token:', error);
throw error;
}
}

/**
* Retrieve stored authentication token
* Retrieve stored authentication token.
*
* Migration for users authenticated before the gh-CLI decouple: their token
* only lives in the global gh CLI state. If the Emdash keychain is empty we
* try `gh auth token` once and persist the result, so PR lists and other
* gh-backed features keep working without asking the user to re-auth.
*/
async getStoredToken(): Promise<string | null> {
if (this.migrationInFlight) {
return this.migrationInFlight;
}

try {
const keytar = await import('keytar');
return await keytar.getPassword(this.SERVICE_NAME, this.ACCOUNT_NAME);
const stored = await keytar.getPassword(this.SERVICE_NAME, this.ACCOUNT_NAME);
if (stored) return stored;

const migrationBlocked =
(await keytar.getPassword(this.SERVICE_NAME, this.MIGRATION_BLOCK_ACCOUNT)) === '1';
if (migrationBlocked) return null;

if (this.migrationInFlight) {
return this.migrationInFlight;
}

if (this.migrationAttempted) return null;
} catch (error) {
console.error('Failed to retrieve token:', error);
return null;
}

const inFlight = this.migrateTokenFromGHCLI();
this.migrationInFlight = inFlight;

try {
return await inFlight;
} finally {
if (this.migrationInFlight === inFlight) {
this.migrationInFlight = null;
}
}
}

private async migrateTokenFromGHCLI(): Promise<string | null> {
return this.withAuthStateLock(async () => {
if (this.migrationAttempted) return null;
this.migrationAttempted = true;

try {
const keytar = await import('keytar');

const migrationBlockedBeforeRead =
(await keytar.getPassword(this.SERVICE_NAME, this.MIGRATION_BLOCK_ACCOUNT)) === '1';
if (migrationBlockedBeforeRead) return null;

const { stdout } = await execAsync('gh auth token', { encoding: 'utf8' });
const token = String(stdout).trim();
if (!token) return null;

// Re-check auth state while still serialized with logout().
const stored = await keytar.getPassword(this.SERVICE_NAME, this.ACCOUNT_NAME);
if (stored) return stored;

const migrationBlockedAfterRead =
(await keytar.getPassword(this.SERVICE_NAME, this.MIGRATION_BLOCK_ACCOUNT)) === '1';
if (migrationBlockedAfterRead) return null;

try {
await this.storeToken(token, 'migration');
} catch (error) {
console.warn('Failed to persist migrated gh CLI token to keychain:', error);
}
return token;
} catch {
return null;
}
});
}
// -----------------------------------------------------------------------
// Repo events API — efficient polling with ETag caching
Expand Down
113 changes: 106 additions & 7 deletions src/test/main/GitHubService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ let issueSearchStdout = '[]';
let prListStdout = '[]';
let repoViewStdout = 'generalaction/emdash';
let prCountStdout = '0';
let blockTokenStore = false;
let releaseTokenStore: (() => void) | null = null;
let notifyTokenStoreStarted: (() => void) | null = null;

vi.mock('child_process', () => {
const execImpl = (command: string, options?: any, callback?: any) => {
Expand Down Expand Up @@ -70,9 +73,30 @@ vi.mock('child_process', () => {
};
});

const setPasswordMock = vi.fn().mockResolvedValue(undefined);
const getPasswordMock = vi.fn().mockResolvedValue(null);
const deletePasswordMock = vi.fn().mockResolvedValue(undefined);
const keychain = new Map<string, string>();

const keyFor = (serviceName: string, accountName: string) => `${serviceName}:${accountName}`;

const setPasswordMock = vi.fn(
async (serviceName: string, accountName: string, password: string) => {
if (accountName === 'github-token') {
notifyTokenStoreStarted?.();
if (blockTokenStore) {
await new Promise<void>((resolve) => {
releaseTokenStore = resolve;
});
}
}

keychain.set(keyFor(serviceName, accountName), password);
}
);
const getPasswordMock = vi.fn(async (serviceName: string, accountName: string) => {
return keychain.get(keyFor(serviceName, accountName)) ?? null;
});
const deletePasswordMock = vi.fn(async (serviceName: string, accountName: string) => {
keychain.delete(keyFor(serviceName, accountName));
});
const fetchMock = vi.fn();

vi.mock('keytar', () => {
Expand All @@ -98,10 +122,33 @@ describe('GitHubService.isAuthenticated', () => {
prListStdout = '[]';
repoViewStdout = 'generalaction/emdash';
prCountStdout = '0';
blockTokenStore = false;
releaseTokenStore = null;
notifyTokenStoreStarted = null;
keychain.clear();
setPasswordMock.mockClear();
getPasswordMock.mockClear();
deletePasswordMock.mockClear();
getPasswordMock.mockResolvedValue(null);
setPasswordMock.mockImplementation(
async (serviceName: string, accountName: string, password: string) => {
if (accountName === 'github-token') {
notifyTokenStoreStarted?.();
if (blockTokenStore) {
await new Promise<void>((resolve) => {
releaseTokenStore = resolve;
});
}
}

keychain.set(keyFor(serviceName, accountName), password);
}
);
getPasswordMock.mockImplementation(async (serviceName: string, accountName: string) => {
return keychain.get(keyFor(serviceName, accountName)) ?? null;
});
deletePasswordMock.mockImplementation(async (serviceName: string, accountName: string) => {
keychain.delete(keyFor(serviceName, accountName));
});
fetchMock.mockReset();
fetchMock.mockResolvedValue({
ok: true,
Expand All @@ -121,14 +168,65 @@ describe('GitHubService.isAuthenticated', () => {
vi.unstubAllGlobals();
});

it('does not treat a global gh login as authenticated without an Emdash token', async () => {
it('migrates a token from the gh CLI into the keychain when Emdash has none', async () => {
const service = new GitHubService();

const result = await service.isAuthenticated();

expect(result).toBe(true);
expect(execCalls.find((cmd) => cmd.startsWith('gh auth token'))).toBeDefined();
expect(setPasswordMock).toHaveBeenCalledWith('emdash-github', 'github-token', 'gho_mocktoken');
expect(fetchMock).toHaveBeenCalledWith(
'https://api.github.com/user',
expect.objectContaining({
headers: expect.objectContaining({
Authorization: 'Bearer gho_mocktoken',
}),
})
);
});

it('does not persist a migrated token after logout races with migration', async () => {
const service = new GitHubService();

blockTokenStore = true;
const tokenStoreStarted = new Promise<void>((resolve) => {
notifyTokenStoreStarted = () => {
notifyTokenStoreStarted = null;
resolve();
};
});

const migration = service.getStoredToken();
await tokenStoreStarted;

const logout = service.logout();

expect(deletePasswordMock).not.toHaveBeenCalled();
expect(keychain.get('emdash-github:github-migration-blocked')).toBeUndefined();

releaseTokenStore?.();

expect(await migration).toBe('gho_mocktoken');
await logout;

expect(keychain.get('emdash-github:github-token') ?? null).toBeNull();
expect(keychain.get('emdash-github:github-migration-blocked')).toBe('1');
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});

it('does not auto-migrate from the gh CLI after the user has logged out', async () => {
const service = new GitHubService();
await service.logout();

execCalls.length = 0;
setPasswordMock.mockClear();

// New service instance simulates an app restart.
const serviceAfterRestart = new GitHubService();
const result = await serviceAfterRestart.isAuthenticated();

expect(result).toBe(false);
expect(fetchMock).not.toHaveBeenCalled();
expect(execCalls.find((cmd) => cmd.startsWith('gh auth status'))).toBeUndefined();
expect(execCalls.find((cmd) => cmd.startsWith('gh auth token'))).toBeUndefined();
expect(setPasswordMock).not.toHaveBeenCalled();
});

Expand All @@ -155,6 +253,7 @@ describe('GitHubService.isAuthenticated', () => {
await service.logout();

expect(deletePasswordMock).toHaveBeenCalledWith('emdash-github', 'github-token');
expect(setPasswordMock).toHaveBeenCalledWith('emdash-github', 'github-migration-blocked', '1');
expect(execCalls).toEqual([]);
});

Expand Down
Loading