From 56b20992e0e895d428067f44edaf8280fccc48af Mon Sep 17 00:00:00 2001 From: friskyydev Date: Tue, 4 Aug 2026 06:27:03 -0400 Subject: [PATCH] fix: refuse to keep a sign-in Linux would store in plain text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit secrets.ts opens by promising that when the OS declines to provide encryption we refuse to store the token rather than writing it in the clear, "because a companion that silently downgrades to a plaintext credential on disk is worse than one that asks the officer to sign in again." On a Linux desktop with no keyring running, that is exactly what it did. Electron falls back to a backend it calls `basic_text`, which scrambles with a key hardcoded in Chromium's source and is recoverable by anyone who can read the file. `safeStorage.isEncryptionAvailable()` answers **true** for it, and that answer was the entire check. So the app wrote a working API token — one that uploads attendance for the officer's guild — to disk in effectively plain text, and told them their sign-in had been stored securely. Availability is no longer the whole question. On Linux the selected backend is read too, `basic_text` counts as no store, and the write refuses rather than merely reporting a flag the caller might not consult. A credentials file left by a build that trusted the flag is deleted on the next launch: declining to read it while leaving it there would be the worst of both, since the officer signs in again and the recoverable copy stays forever. The setup panel already had a slot for "sign-in cannot be remembered" and it simply never fired here. It now carries the reason, because "no secure credential store" is true and useless to somebody whose desktop looks perfectly normal and whose missing piece is a daemon they have never had to think about — it names gnome-keyring and KWallet. Windows and macOS are untouched: neither has a backend to choose, and getSelectedStorageBackend is Linux-only and absent on older Electron, so it is guarded by platform and by existence. A test covers both, because dropping either guard would start refusing DPAPI, which works fine. 9 tests. They found an ordering bug on the way: with no encryption available at all, the backend can still report basic_text, and the message was sending people off to start a keyring when that was not the problem. Mutation-checked — skipping the stale-file deletion fails the test that demands it. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 15 ++- src/main/index.ts | 5 +- src/main/secrets.test.ts | 164 +++++++++++++++++++++++++ src/main/secrets.ts | 73 +++++++++-- src/preload/index.ts | 7 +- src/renderer/App.tsx | 2 + src/renderer/components/SetupPanel.tsx | 6 +- 7 files changed, 259 insertions(+), 13 deletions(-) create mode 100644 src/main/secrets.test.ts 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.'}

)}