diff --git a/README.md b/README.md index 5d474bc..e45d674 100644 --- a/README.md +++ b/README.md @@ -126,5 +126,16 @@ nobody notices, but it is still the UI thread doing it, and the file only grows. the officer is the one who decides a session was the guild's raid. An "always send finished nights for this guild" setting is reasonable once the matching has earned trust. -**Linux `basic_text` detection.** `safeStorage` can silently fall back to plaintext on -Linux; `canPersist()` reports availability but not which backend answered. +## Where the sign-in is kept + +`safeStorage` — DPAPI on Windows, Keychain on macOS, libsecret or KWallet on Linux. When +no real store is available the token is not written at all; the officer signs in each +launch instead. + +The Linux case is the one worth stating. With no keyring daemon running, Electron falls +back to a backend named `basic_text` that scrambles with a key hardcoded in Chromium — +recoverable by anyone who can read the file — and `isEncryptionAvailable()` still answers +**true** for it. So availability alone is not the check: on Linux the selected backend is +read as well, `basic_text` counts as no store, and a credentials file left by an earlier +build that trusted the flag is deleted on the next launch rather than left lying around. +The setup panel says which daemon to start. diff --git a/src/main/index.ts b/src/main/index.ts index 48c4f52..b5e4e13 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -3,7 +3,7 @@ import { app, BrowserWindow, dialog, ipcMain, shell } from 'electron'; import { ApiClient } from './api'; import { buildAuthorizeUrl, createPkcePair, LoopbackReceiver } from './auth'; import { autoDetect, findSavedVariables } from './discovery'; -import { canPersist, clearToken, loadToken, saveToken } from './secrets'; +import { canPersist, clearToken, loadToken, persistenceBlocker, saveToken } from './secrets'; import { readNights } from './savedVariables'; import { loadSettings, rememberUpload, saveSettings } from './settings'; import { nightKey } from '../shared/nightKey'; @@ -208,6 +208,9 @@ async function runSmokeCheck(target: BrowserWindow): Promise { ipcMain.handle('app:info', () => ({ version: CLIENT_VERSION, canRememberSignIn: canPersist(), + // Why not, when not. "No secure credential store" is accurate and unhelpful on a Linux + // desktop whose only missing piece is a keyring daemon nobody has had to think about. + signInMemoryBlocker: persistenceBlocker(), platform: process.platform, })); diff --git a/src/main/secrets.test.ts b/src/main/secrets.test.ts new file mode 100644 index 0000000..06fe237 --- /dev/null +++ b/src/main/secrets.test.ts @@ -0,0 +1,164 @@ +import { existsSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +/** + * Token storage, and specifically the Linux case that cannot be checked by hand from a + * Windows or macOS machine. + * + * `safeStorage.isEncryptionAvailable()` answers **true** on a Linux desktop with no + * keyring, where Electron falls back to a backend called `basic_text` that scrambles with + * a key hardcoded in Chromium. Trusting that answer alone meant writing a working API + * token to disk in effectively plain text while telling the officer it was stored + * securely — the exact downgrade the module's own comment says it refuses to perform. + * + * Nothing here talks to a real credential store; the point is the decision made from what + * one reports. + */ + +const userData = mkdtempSync(join(tmpdir(), 'raidify-secrets-')); +const FILE = join(userData, 'credentials.bin'); + +const state = { + available: true, + backend: 'gnome_libsecret' as string | undefined, + platform: 'linux' as string, +}; + +vi.mock('electron', () => ({ + app: { getPath: () => userData }, + safeStorage: { + isEncryptionAvailable: () => state.available, + getSelectedStorageBackend: () => { + if (state.backend === undefined) throw new Error('not supported on this platform'); + return state.backend; + }, + // Stand-ins. Reversing the string is enough to prove which bytes were written and + // that a round trip happened; the real implementation is the OS's problem. + encryptString: (s: string) => Buffer.from(`enc:${s}`), + decryptString: (b: Buffer) => b.toString().replace(/^enc:/, ''), + }, +})); + +const { canPersist, persistenceBlocker, saveToken, loadToken, clearToken } = await import( + './secrets' +); + +const realPlatform = process.platform; + +function setPlatform(value: string) { + Object.defineProperty(process, 'platform', { value, configurable: true }); +} + +beforeEach(() => { + state.available = true; + state.backend = 'gnome_libsecret'; + setPlatform('linux'); + clearToken(); +}); + +afterEach(() => { + Object.defineProperty(process, 'platform', { value: realPlatform, configurable: true }); +}); + +describe('a machine with a real credential store', () => { + it('keeps the sign-in and gives it back', () => { + saveToken('tok-abc'); + + expect(loadToken()).toBe('tok-abc'); + expect(canPersist()).toBe(true); + expect(persistenceBlocker()).toBeNull(); + }); + + it('writes something other than the bare token', () => { + saveToken('tok-abc'); + + expect(readFileSync(FILE).toString()).not.toBe('tok-abc'); + }); +}); + +describe('a Linux desktop with no keyring running', () => { + beforeEach(() => { + // What Electron actually reports there: encryption "available", backend that isn't. + state.available = true; + state.backend = 'basic_text'; + }); + + it('refuses to keep the sign-in', () => { + expect(canPersist()).toBe(false); + }); + + /** + * The refusal has to be the write refusing, not just the flag reading false. A caller + * that skipped the flag would otherwise put the token on disk anyway. + */ + it('refuses to write the token at all', () => { + expect(() => saveToken('tok-abc')).toThrow(); + expect(existsSync(FILE)).toBe(false); + }); + + /** + * A file left by a build that trusted isEncryptionAvailable() alone. Declining to read + * it while leaving it there is the worst outcome: the officer signs in again and the + * recoverable copy stays on disk indefinitely. + */ + it('deletes a token an earlier build stored unprotected', () => { + writeFileSync(FILE, 'enc:tok-from-old-build'); + + expect(loadToken()).toBeNull(); + expect(existsSync(FILE)).toBe(false); + }); + + /** + * "No secure credential store" is true and useless here — the desktop looks perfectly + * normal and the missing piece is a daemon. The message has to name it. + */ + it('says what to start', () => { + const blocker = persistenceBlocker(); + + expect(blocker).toBeTruthy(); + expect(blocker).toMatch(/keyring|KWallet/i); + }); +}); + +describe('a machine with no encryption at all', () => { + beforeEach(() => { + state.available = false; + state.backend = 'basic_text'; + }); + + it('refuses, and says so without inventing a Linux fix', () => { + expect(canPersist()).toBe(false); + expect(persistenceBlocker()).toMatch(/no secure credential store/i); + }); +}); + +describe('Windows and macOS', () => { + /** + * Neither has a backend to choose, and `getSelectedStorageBackend` is Linux-only — + * older Electron does not define it at all. Calling it anywhere else must not be able + * to break sign-in. + */ + it('never consults the Linux backend, even when asking would throw', () => { + setPlatform('win32'); + state.backend = undefined; + + expect(canPersist()).toBe(true); + expect(persistenceBlocker()).toBeNull(); + + saveToken('tok-win'); + expect(loadToken()).toBe('tok-win'); + }); + + /** + * `basic_text` is a Linux backend name. If the platform check were dropped, a stray + * answer on Windows would start refusing DPAPI — which works fine. + */ + it('is not refused by a Linux backend name', () => { + setPlatform('darwin'); + state.backend = 'basic_text'; + + expect(canPersist()).toBe(true); + }); +}); diff --git a/src/main/secrets.ts b/src/main/secrets.ts index 779d8f3..5fb7208 100644 --- a/src/main/secrets.ts +++ b/src/main/secrets.ts @@ -16,17 +16,67 @@ import { app, safeStorage } from 'electron'; const FILE = () => join(app.getPath('userData'), 'credentials.bin'); +/** + * Linux desktops without a running keyring. + * + * Electron falls back to a backend it calls `basic_text`, which is not encryption: it + * scrambles with a key hardcoded in Chromium's source, so anything written with it is + * recoverable by anyone who can read the file. `isEncryptionAvailable()` still answers + * **true** for it. + * + * That answer was the whole check, so on a machine with no gnome-keyring or KWallet this + * module did exactly what the comment above says it refuses to do — write a working API + * token to disk in effectively plain text, while telling the officer their sign-in was + * stored securely. The token uploads attendance for their guild. + */ +const INSECURE_BACKEND = 'basic_text'; + +/** + * Which OS credential store is actually behind `safeStorage`, on the one platform where + * the answer varies. Windows (DPAPI) and macOS (Keychain) have nothing to choose. + */ +function selectedBackend(): string | null { + if (process.platform !== 'linux') return null; + // Guarded: the method is Linux-only and absent on older Electron, and a crash here + // would take out sign-in entirely. + if (typeof safeStorage.getSelectedStorageBackend !== 'function') return null; + + try { + return safeStorage.getSelectedStorageBackend(); + } catch { + return null; + } +} + export function canPersist(): boolean { - return safeStorage.isEncryptionAvailable(); + if (!safeStorage.isEncryptionAvailable()) return false; + return selectedBackend() !== INSECURE_BACKEND; } -export function saveToken(token: string): void { - if (!canPersist()) { - throw new Error( - 'This system has no secure credential store available, so the sign-in cannot be remembered.', - ); +/** + * Why the sign-in cannot be kept, in words that name the fix. + * + * "No secure credential store" is true and useless on Linux: the officer has a desktop + * that looks perfectly normal, and the missing piece is a keyring daemon they have never + * had to think about. Null when persistence works. + */ +export function persistenceBlocker(): string | null { + if (canPersist()) return null; + + // Availability first. When there is no encryption at all, the backend can still report + // `basic_text` — and telling someone to start a keyring daemon when the real problem is + // that nothing is available sends them off fixing the wrong thing. + if (safeStorage.isEncryptionAvailable() && selectedBackend() === INSECURE_BACKEND) { + return 'No keyring is running, so the only store available would keep your sign-in in plain text. Start gnome-keyring or KWallet and sign in again to stay signed in.'; } + return 'This system has no secure credential store, so the sign-in cannot be remembered between launches.'; +} + +export function saveToken(token: string): void { + const blocker = persistenceBlocker(); + if (blocker) throw new Error(blocker); + const path = FILE(); mkdirSync(dirname(path), { recursive: true }); writeFileSync(path, safeStorage.encryptString(token), { mode: 0o600 }); @@ -34,7 +84,16 @@ export function saveToken(token: string): void { export function loadToken(): string | null { const path = FILE(); - if (!existsSync(path) || !canPersist()) return null; + if (!existsSync(path)) return null; + + if (!canPersist()) { + // A file written by a build that trusted `isEncryptionAvailable()` alone, on a machine + // where that answer was wrong. Refusing to read it while leaving it on disk would be + // the worst of both: the officer signs in again, and the recoverable copy stays there + // forever. Delete it and make them sign in. + clearToken(); + return null; + } try { return safeStorage.decryptString(readFileSync(path)); diff --git a/src/preload/index.ts b/src/preload/index.ts index 039ebf6..8bc27ee 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -14,7 +14,12 @@ import type { * describing something the officer asked for. */ const bridge = { - appInfo: (): Promise<{ version: string; canRememberSignIn: boolean; platform: string }> => + appInfo: (): Promise<{ + version: string; + canRememberSignIn: boolean; + signInMemoryBlocker: string | null; + platform: string; + }> => ipcRenderer.invoke('app:info'), checkCompat: (): Promise => ipcRenderer.invoke('compat:check'), diff --git a/src/renderer/App.tsx b/src/renderer/App.tsx index 33fc1e2..7d21967 100644 --- a/src/renderer/App.tsx +++ b/src/renderer/App.tsx @@ -9,6 +9,7 @@ import { StatusBanner } from './components/StatusBanner'; interface AppInfo { version: string; canRememberSignIn: boolean; + signInMemoryBlocker: string | null; platform: string; } @@ -116,6 +117,7 @@ export function App() { settings={settings} signedIn={signedIn} canRememberSignIn={info?.canRememberSignIn ?? true} + signInMemoryBlocker={info?.signInMemoryBlocker ?? null} guilds={guilds} installs={installs} watchingPath={watchingPath} diff --git a/src/renderer/components/SetupPanel.tsx b/src/renderer/components/SetupPanel.tsx index 89752f2..f556e32 100644 --- a/src/renderer/components/SetupPanel.tsx +++ b/src/renderer/components/SetupPanel.tsx @@ -16,6 +16,7 @@ export function SetupPanel({ settings, signedIn, canRememberSignIn, + signInMemoryBlocker, guilds, installs, watchingPath, @@ -29,6 +30,7 @@ export function SetupPanel({ settings: Settings | null; signedIn: boolean; canRememberSignIn: boolean; + signInMemoryBlocker: string | null; guilds: CompanionGuild[] | null; installs: SavedVariablesCandidate[] | null; watchingPath: string | null; @@ -120,8 +122,8 @@ export function SetupPanel({

{!canRememberSignIn && (

- This system has no secure credential store, so the sign-in cannot be - remembered between launches. + {signInMemoryBlocker ?? + 'This system has no secure credential store, so the sign-in cannot be remembered between launches.'}

)}