Skip to content

fix(electron): remove the unreachable web-cookie login path (#14705) - #14689

Merged
diegosouzapw merged 3 commits into
cheaper-inference:release/v3.8.51from
gonisulaimann:fix/electron-web-cookie-login-config
Sep 24, 2026
Merged

diegosouzapw merged 3 commits into
cheaper-inference:release/v3.8.51from
gonisulaimann:fix/electron-web-cookie-login-config

Conversation

@gonisulaimann

@gonisulaimann gonisulaimann commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Re-scoped per the decision in #14705 (comment): remove the dead Electron web-cookie login path instead of shipping a CJS mirror for it. The original scope of this PR (an esbuild-generated tokenExtractionConfig mirror to fix the loginManager.js require) is superseded — with the path removed, that mirror has no consumer.

What was wrong

Two halves, both verified in #14705:

  • electron/main.js required ../src/lib/db/secrets from plain CJS. That module only exists as TypeScript, so the require always threw MODULE_NOT_FOUND and a successful credential extraction was reported as a failure.
  • Even if the require had resolved, persistSecret writes a blob into the secrets namespace that only JWT/API-key secret readers consume — no provider connection would ever be created from it.

And the path was unreachable anyway: the preload exposes startLogin/onLoginStatus, but no renderer code ever calls them. The dashboard's cookie-provider login goes through the server-side inAppLoginService (HTTP), not Electron IPC.

Together with #14660 (the tokenExtractionConfig.ts require in loginManager.js, fixed server-side by #14689's original scope being unnecessary), the entire Electron web-cookie login path was dead code that could never work.

What changed

  • Removed the login:start / login:cancel / login:status IPC handlers and the loginManager status forwarder from electron/main.js.
  • Removed the preload exposure (startLogin, cancelLogin, getLoginStatus, onLoginStatus) and the three login:* channel allowlist entries.
  • Deleted electron/loginManager.js and electron/lib/loginHeaderCapture.js (its only consumer) and their two build.files entries (including the pre-existing duplicate lib/loginHeaderCapture.js listing).
  • Deleted tests/unit/electron-login-header-capture.test.ts (tested the removed module).
  • Updated the stale comment in inAppLoginService.ts that pointed to electron/loginManager.js as the "Electron-native" alternative.
  • Added a regression guard in tests/unit/electron-main.test.ts (fix(startup): Electron login:start IPC is unreachable and its credential persist can never succeed (requires a .ts path from CJS) #14705): the removed modules, IPC channels, preload API, and build.files entries must not silently reappear.
  • changelog.d/maintenance/14689-electron-login-removal.md fragment per the changelog convention.

Verification

  • tests/unit/electron-main.test.ts — 39 pass (incl. 4 new guards).
  • All 16 electron unit suites green (lazy-window, preload, packaging, remote-server, smoke-script, …).
  • node --check on the edited CJS entry points; Prettier clean; my touched files contribute 0 new tsc/eslint violations (suppression counts unchanged).
  • Full-repo sweep: no remaining references to loginManager / loginHeaderCapture / login:* outside historical i18n CHANGELOG mirrors.

Base-red note: ⚠️ base-red inherited: #14547 (known typecheck/test failures being fixed in #14693/#14683 — unrelated to this PR).

Closes #14705.

Maintainer rework (merge-batch 2026-09-24)

I checked this head against the removal checklist agreed on #14705. Everything in the list is done except two leftover references, which I fixed in one commit on top (after a real merge of the current release/v3.8.51 tip; your commit is untouched):

  • knip.json: removed the electron/loginManager.js entry point, which no longer exists.
  • open-sse/services/tokenExtractionConfig.ts: the comment on TOKEN_EXTRACTION_CONFIGS no longer lists LoginManager as a consumer.

Verification:

  • node --check passes on electron/main.js and electron/preload.js.
  • git grep finds no remaining loginManager / loginHeaderCapture / login:* channel references outside historical changelog mirrors.
  • Electron suites (tests/unit/electron*.test.ts): 166/167. The one failure, electron-server-readiness, is a load flake: this PR doesn't touch that file, and it passes 4/4 when run alone.
  • Red → green: tests/unit/electron-main.test.ts run against the tip's electron/ fails the 4 new removal guards, and passes 39/39 on this head.

Also closes #14660: the broken tokenExtractionConfig require it reports lived in loginManager.js, which this PR deletes.

Closes #14660

…inference#14705)

The login:* IPC handlers could never work: their only persist step
required ../src/lib/db/secrets, a TypeScript path that plain CJS can
never resolve, so every successful extraction reported failure. No
renderer code ever called the preload API either, and even a resolved
require would have written a secrets-namespace blob nothing reads
instead of creating a provider connection.

Remove the whole path rather than repair it: the login:* handlers in
main.js, the preload exposure and channel allowlist entries,
loginManager.js, its only consumer lib/loginHeaderCapture.js, and the
matching build.files entries. Cookie-provider login stays on the
server-side inAppLoginService, which is the surface the dashboard
actually uses. Adds a regression guard so the path cannot silently
resurrect.
@gonisulaimann
gonisulaimann force-pushed the fix/electron-web-cookie-login-config branch from c0b9a6d to 8ad4218 Compare September 24, 2026 02:06
@gonisulaimann gonisulaimann changed the title fix(electron): ship a CJS mirror of the web-cookie login config (#14660) fix(electron): remove the unreachable web-cookie login path (#14705) Sep 24, 2026
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Thanks @gonisulaimann, and your diagnosis is spot on. I reproduced it on the tip: both require paths in loginManager.js throw MODULE_NOT_FOUND, and so does the ../src/lib/db/secrets require in main.js that you reported in #14705.

As I mentioned on #14705, we've decided to remove the Electron web-cookie login path instead of repairing it. Nothing in the renderer calls startLogin, persistSecret writes to a namespace no provider reads, and the server-side inAppLoginService already covers this flow. That leaves the CJS mirror, the generator, the npm script and the two CI workflow steps with nothing to guard, so we'd rather not merge them.

Would you like to re-scope this PR into the removal? Here's the exact list, checked against the current release/v3.8.51 tip:

  • delete electron/loginManager.js, electron/lib/loginHeaderCapture.js and tests/unit/electron-login-header-capture.test.ts
  • electron/main.js: remove line 36 (require("./loginManager")) and the whole // ── Web-Cookie Login IPC Handlers block (lines 1059–1100, stopping before // Autostart management handlers)
  • electron/preload.js: remove the three login:* invoke channels (106–108), "login:status" from receive (118), and the Web-Cookie Login block (177–181)
  • electron/package.json build.files: remove "loginManager.js" and both "lib/loginHeaderCapture.js" entries (lines 61, 62, 70)
  • knip.json: remove the "electron/loginManager.js" entry
  • update the stale comments at open-sse/services/inAppLoginService.ts:8 and open-sse/services/tokenExtractionConfig.ts:397, and add a changelog.d/ fragment
  • revert the mirror, the generator, the package.json script and the ci.yml/quality.yml steps
  • use Closes #14660 and Closes #14705 in the body

I dry-ran this removal on the tip. node -c is clean on both entry points, and the electron suites stay green. The one "server readiness" flake also fails on the unmodified tip, so it's unrelated.

If you'd rather not, no problem: we'll do the removal ourselves and credit you as co-author in the commit, since you found both halves of this.

…-cookie login removal

- knip.json: remove the deleted electron/loginManager.js entry point.
- tokenExtractionConfig.ts: the shared config map is now only consumed by
  InAppLoginService.
@gonisulaimann

Copy link
Copy Markdown
Contributor Author

Thanks for finishing the sweep — fc193c1 covers the knip entry and the stale comment that were left, and the body now closes both #14660 and #14705. Ready when you are.

@diegosouzapw
diegosouzapw merged commit 93b14b8 into cheaper-inference:release/v3.8.51 Sep 24, 2026
9 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants