diff --git a/electron/main.cjs b/electron/main.cjs index 8d767692..f2482ce8 100644 --- a/electron/main.cjs +++ b/electron/main.cjs @@ -459,15 +459,27 @@ function armAuthHandoff() { function consumeAuthNonce(nonce) { const armed = armedAuthNonce const expiry = armedAuthExpiry - armedAuthNonce = null - armedAuthExpiry = 0 - if (!armed || Date.now() > expiry) return false + // Clear only on a MATCH (or a genuine expiry). Clearing first meant any + // inbound value disarmed the pending sign-in: click sign-in twice, finish the + // first tab, and its now-stale nonce took the second one's arming with it — + // so the obvious recovery, going back and finishing the other tab, failed too. + if (!armed || Date.now() > expiry) { + armedAuthNonce = null + armedAuthExpiry = 0 + return false + } if (typeof nonce !== 'string' || nonce.length !== armed.length) return false + let ok = false try { - return crypto.timingSafeEqual(Buffer.from(nonce), Buffer.from(armed)) + ok = crypto.timingSafeEqual(Buffer.from(nonce), Buffer.from(armed)) } catch { - return false + ok = false + } + if (ok) { + armedAuthNonce = null + armedAuthExpiry = 0 } + return ok } /** Pull token + companyId + nonce out of a `cumora://auth#token=…` URL. The OS @@ -495,7 +507,7 @@ function parseAuthDeepLink(rawUrl) { function dispatchAuthToken(token, companyId, nonce) { if (!consumeAuthNonce(nonce)) { console.warn('[auth] dropped inbound token: no matching armed nonce (possible drive-by deep link)') - return + return false } if (mainWindow && !mainWindow.isDestroyed()) { if (mainWindow.isMinimized()) mainWindow.restore() @@ -505,6 +517,7 @@ function dispatchAuthToken(token, companyId, nonce) { } else { pendingAuthToken = { token, companyId } } + return true } let pendingAuthToken = null @@ -545,8 +558,15 @@ function startAuthLoopback() { } const companyId = typeof parsed.companyId === 'string' ? parsed.companyId : null const nonce = typeof parsed.nonce === 'string' ? parsed.nonce : null - dispatchAuthToken(parsed.token, companyId, nonce) - res.statusCode = 204; res.end() + // Answer what actually happened. A 204 for a token the app dropped + // made the browser page report "Signed in — Cumora has your session" + // while nothing had been handed over; the page keys on `r.ok` and its + // catch branch is the one that tells the user to open Cumora itself. + if (dispatchAuthToken(parsed.token, companyId, nonce)) { + res.statusCode = 204; res.end() + } else { + res.statusCode = 409; res.end('no armed sign-in') + } } catch { res.statusCode = 400; res.end('bad json') } diff --git a/server/src/__tests__/electron-auth-handoff.test.ts b/server/src/__tests__/electron-auth-handoff.test.ts new file mode 100644 index 00000000..1d5cb3e8 --- /dev/null +++ b/server/src/__tests__/electron-auth-handoff.test.ts @@ -0,0 +1,122 @@ +/** + * Guard: the loopback must not report a sign-in it dropped. + * + * The desktop arms a nonce, opens the system browser, and the return page POSTs + * `{token, companyId, nonce}` back to the loopback. `dispatchAuthToken` drops a + * token whose nonce does not match — correctly, that is the drive-by-deep-link + * defence — but it returned `void`, and the handler answered 204 either way. + * + * The page keys on `r.ok`: + * + * if (!r.ok) throw new Error('handoff rejected: ' + r.status); + * h1.textContent = 'Signed in'; + * label.innerHTML = ' Signed in'; + * hint.textContent = 'You can close this tab.'; + * + * So the browser told the user they were signed in, with a checkmark, while the + * app had received nothing. Its catch branch — the one that says to open Cumora + * itself — was unreachable for this case. + * + * Reaching it is ordinary: `armAuthHandoff` "supersedes any previous unused + * nonce", and AuthScreen re-arms on window focus. Click sign-in, click back + * into Cumora, click sign-in again, then finish the FIRST tab. + * + * Second half: `consumeAuthNonce` cleared the armed nonce before validating, so + * that stale first tab also disarmed the second one — the obvious recovery, + * going back and finishing the other tab, failed too. + * + * electron/ has no runtime harness, so this reads the source the way + * electron-tray-unread-dot.test.ts does, and self-tests its matchers. + * + * Run: node --import tsx --test server/src/__tests__/electron-auth-handoff.test.ts + */ +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' +import { dirname, join } from 'node:path' + +const REPO_ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..', '..') +const MAIN_CJS = readFileSync(join(REPO_ROOT, 'electron', 'main.cjs'), 'utf8') + +function bodyOf(source: string, decl: string): string | null { + const at = source.indexOf(decl) + if (at < 0) return null + const end = source.indexOf('\n}', at) + return end < 0 ? null : source.slice(at, end) +} + +/** Does the handler decide its status from the dispatch result? */ +function answersTheRealStatus(handler: string): boolean { + return /if \(dispatchAuthToken\(/.test(handler) && /statusCode = 4\d\d/.test(handler) +} + +/** Does the nonce survive a value that does not match it? */ +function clearsOnlyOnMatch(fn: string): boolean { + const firstGuard = fn.search(/if \(!armed/) + const firstClear = fn.indexOf('armedAuthNonce = null') + if (firstGuard < 0 || firstClear < 0) return false + return firstClear > firstGuard +} + +test('the loopback answers what actually happened', () => { + const handler = bodyOf(MAIN_CJS, "if (typeof parsed?.token !== 'string'") + assert.ok(handler, 'the /auth/token handler moved — update this guard alongside the refactor') + assert.ok( + answersTheRealStatus(handler), + 'the handler answers 204 without checking whether the token was accepted', + ) +}) + +test('dispatchAuthToken reports acceptance on both paths', () => { + const fn = bodyOf(MAIN_CJS, 'function dispatchAuthToken(') + assert.ok(fn, 'dispatchAuthToken moved — update this guard alongside the refactor') + assert.match(fn, /return false/, 'the drop path must say so') + assert.match(fn, /return true/, 'the accept path must say so') +}) + +test('a mismatching nonce does not disarm the pending sign-in', () => { + const fn = bodyOf(MAIN_CJS, 'function consumeAuthNonce(') + assert.ok(fn, 'consumeAuthNonce moved — update this guard alongside the refactor') + assert.ok( + clearsOnlyOnMatch(fn), + 'the armed nonce is cleared before it is validated, so any inbound value disarms it', + ) +}) + +test('the timing-safe compare is still how the nonce is checked', () => { + // The change above must not have loosened the comparison it guards. + const fn = bodyOf(MAIN_CJS, 'function consumeAuthNonce(') ?? '' + assert.match(fn, /crypto\.timingSafeEqual\(/) + assert.match(fn, /nonce\.length !== armed\.length/) +}) + +// ── the matchers must be able to fail ────────────────────────────────────── + +test('the status guard rejects the unconditional 204', () => { + assert.equal( + answersTheRealStatus('dispatchAuthToken(parsed.token, companyId, nonce)\nres.statusCode = 204; res.end()'), + false, + ) +}) + +test('the status guard accepts a branched answer', () => { + assert.equal( + answersTheRealStatus('if (dispatchAuthToken(a, b, c)) { res.statusCode = 204 } else { res.statusCode = 409 }'), + true, + ) +}) + +test('the nonce guard rejects clearing before validating', () => { + assert.equal( + clearsOnlyOnMatch('const armed = armedAuthNonce\narmedAuthNonce = null\nif (!armed) return false'), + false, + ) +}) + +test('the nonce guard accepts clearing inside the guard', () => { + assert.equal( + clearsOnlyOnMatch('const armed = armedAuthNonce\nif (!armed) { armedAuthNonce = null; return false }'), + true, + ) +})