From ced6c8d4302bd17e51007e9089bc3dc8ec45831b Mon Sep 17 00:00:00 2001
From: Xialie Zhuang <62231346+Lieisyourlie@users.noreply.github.com>
Date: Sat, 5 Sep 2026 10:52:48 +0800
Subject: [PATCH] fix(electron): the loopback must not report a sign-in it
 dropped
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The return page POSTs the token back to the desktop loopback and reports
success from the HTTP status:

  if (!r.ok) throw new Error('handoff rejected: ' + r.status);
  h1.textContent = 'Signed in';
  label.innerHTML = '<span class="ok">✓</span> Signed in';

dispatchAuthToken drops a token whose nonce does not match — that is the
drive-by-deep-link defence and it is right — but it returned void, and
the handler answered 204 regardless. So the browser told the user they
were signed in, with a checkmark and the button greyed out, while the app
had received nothing. The page's catch branch, the one that says to open
Cumora itself, was unreachable for this case.

Reaching it takes no attacker. armAuthHandoff "supersedes any previous
unused nonce", and AuthScreen re-arms on window focus: click sign-in, the
browser opens behind, click back into Cumora, click sign-in again, then
finish the FIRST tab. Its nonce is now stale.

Second half: consumeAuthNonce cleared the armed nonce before validating,
so that stale tab also disarmed the live one — the obvious recovery,
going back to finish the other tab, failed too. Clear only on a match, or
on a real expiry. The timing-safe compare and the length check are
unchanged, and a test pins them so this cannot quietly loosen the
comparison it guards.

The deep-link handler ignores the new return value on purpose: there is
nobody to answer there.

Three of the eight cases go red against the shipped file, and the
matchers self-test both ways — a source-scraping guard's failure mode is
becoming a silent no-op.
---
 electron/main.cjs                             |  36 ++++--
 .../__tests__/electron-auth-handoff.test.ts   | 122 ++++++++++++++++++
 2 files changed, 150 insertions(+), 8 deletions(-)
 create mode 100644 server/src/__tests__/electron-auth-handoff.test.ts

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 = '<span class="ok">✓</span> 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,
+  )
+})