Skip to content

fix(desktop): authenticate profile session slices against oauth gateways - #71503

Closed
zakhounet wants to merge 1 commit into
NousResearch:mainfrom
zakhounet:patch/67600
Closed

fix(desktop): authenticate profile session slices against oauth gateways#71503
zakhounet wants to merge 1 commit into
NousResearch:mainfrom
zakhounet:patch/67600

Conversation

@zakhounet

Copy link
Copy Markdown
Contributor

What does this PR do?

The Desktop session sidebar renders empty for the root (default) profile on an oauth-gated remote gateway, while named profiles list normally and the sessions are intact server-side.

The two batched /api/profiles/sessions dispatches in main.ts resolved the primary connection and then passed conn.token straight to fetchJson:

const primary = await ensureBackend(null)

return fetchJson(`${primary.baseUrl}/api/profiles/sessions?${searchParams}`, primary.token, {})

That skips the authMode branch requestJsonForProfile applies to every other per-profile path. Against an oauth gateway the connection token is not a valid credential — the request 401s. Both call sites collapse a rejected fetch into { sessions: [], total: 0, profile_totals: {} }, so the failure surfaces nowhere: no error, no toast, just an empty list that reads as data loss.

This routes both dispatches through the existing fetchJsonForProfile helper, and extracts the branch they skipped into a pure, unit-tested decision — resolveProfileRestAuth — which reuses resolveOauthRestAuth rather than duplicating it. It states the contract those call sites broke: only a non-oauth gateway may authenticate with the connection token.

This follows the precedent set by native-auth-decisions.ts, whose own header says the value of these seams is "the test that pins the contract so the god-file call sites can't drift back to the buggy shape". These two call sites are exactly such a drift, so the fix is added as a fourth seam in that same module.

Related Issue

Fixes #67600

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • apps/desktop/electron/native-auth-decisions.ts — add resolveProfileRestAuth(authMode, nativeAccessToken, connectionToken) and the ProfileRestAuth type; delegates to resolveOauthRestAuth for the oauth case. Header docblock extended with the fourth seam.
  • apps/desktop/electron/main.tsrequestJsonForProfile now delegates its credential choice to resolveProfileRestAuth (no behaviour change); fetchProfilesSessionSlice and mergeRemoteProfileSessions route through fetchJsonForProfile instead of a raw fetchJson(…, primary.token).
  • apps/desktop/electron/native-auth-decisions.test.ts — 5 tests covering non-oauth, missing authMode, oauth-with-bearer, oauth-without-bearer, and the regression itself: an oauth gateway must never resolve to the connection token.

How to Test

Reproduce (before this change) — remote Desktop against an oauth-gated gateway, multi-profile install:

  1. Open Desktop on the root (default) profile. The sidebar shows SESSIONS 0 — or only the session created in the current run.
  2. Switch to a named profile (local, minimax): sessions list normally, so the sidebar component itself works.
  3. Switch back to default: SESSIONS 0 again, while the profile DB holds 800+ non-archived sessions.
  4. Replay the endpoint server-side (/api/profiles/sessions?profile=default): rows are returned. Request correct, data available, screen empty.
  5. tcpdump on the fan-out shows the dispatches returning 401, swallowed by the .catch().

Verify the fix:

cd apps/desktop
npx vitest run electron/native-auth-decisions.test.ts   # 13 passed
npx vitest run --project electron                       # 730 passed, 1 skipped
npm run check:lint                                      # typecheck + eslint, 0 errors

The regression test was validated by mutation: removing the authMode branch from resolveProfileRestAuth (i.e. reintroducing the bug) fails 3 tests, including resolveProfileRestAuth never authenticates an oauth gateway with the connection token. Restoring it turns them green again — the tests fail for the right reason, rather than passing by construction.

Runtime check: a Desktop built from this branch lists the root profile's sessions immediately on an oauth-gated remote gateway; the patched build has been in daily use for three days.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the suites this change can affect — vitest --project electron (730 passed), npm run check:lint (0 errors). The change is TypeScript-only, so pytest tests/ is untouched by it.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS on Apple Silicon (Darwin 25.5.0, M4), remote gateway over SSH tunnel

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — the new seam is documented in the native-auth-decisions.ts header alongside the existing three
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) — the decision is pure and platform-independent; no syscalls, paths, or encoding involved
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Notes on scope

resolveProfileRestAuth deliberately stops at the decision. requestJsonForProfile keeps ownership of ensureNativeAccessToken and of performing the request, so this PR changes no network behaviour for any path that was already correct — the oauth and non-oauth branches resolve exactly as before. The only behavioural change is that the two session-slice dispatches now take the same branch as every other per-profile call.

Related but distinct: #70944 / #42467 describe the same symptom (an empty Desktop sidebar) with a different cause — a profile state.db whose schema lags, raising inside the read-only aggregation endpoint. That path is server-side and is addressed by #70964. This PR is the client-side auth path: the DB here is migrated and the endpoint returns rows when replayed directly. The two do not overlap, and neither fix subsumes the other.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) area/auth Authentication, OAuth, credential pools area/sessions Session lifecycle, resume, persistence, history needs-decision Awaiting maintainer decision before any implementation sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jul 25, 2026
The two batched `/api/profiles/sessions` dispatches resolved the primary
connection and then passed `conn.token` straight to `fetchJson`, skipping the
`authMode` branch that `requestJsonForProfile` applies to every other
per-profile path. Against an oauth-gated gateway that token is not a valid
credential, so both requests 401. Each call site collapses a rejected fetch
into `{ sessions: [], total: 0 }`, so the failure surfaces nowhere: the
Desktop session sidebar renders empty for the root profile while the sessions
are intact server-side and the named profiles list normally.

Route both dispatches through `fetchJsonForProfile`, and name the branch they
skipped as `resolveProfileRestAuth`. It delegates to `resolveReadinessProbeAuth`
and keeps a single deliberate difference: a REST call retains the connection
token when `authMode` is not yet known, where the readiness probe treats an
unknown gateway as public — probing anonymously is safe, dropping the
credential from a real request is not. Unit tests pin both the oauth contract
and that divergence.

Fixes NousResearch#67600
@zakhounet

Copy link
Copy Markdown
Contributor Author

Closing this — #67600 is already fixed on main by 704a321 ("fix(desktop): preserve OAuth sessions in sidebar"). It routes both batched /api/profiles/sessions dispatches through the oauth-aware fetchJsonForProfile (via the extracted fetchPrimaryProfileSessions helper) — the same fix this PR carried, which is why the two now conflict on the same lines of main.ts.

Verified before closing: a Desktop client built from current main (7de4fbd4), with 704a321 and without this patch, renders the default profile sidebar correctly against an oauth-gated remote with per-profile overrides — the exact #67600 repro. The upstream fix resolves it on its own.

The only piece here not already in 704a321 was the resolveProfileRestAuth extraction in requestJsonForProfile, but its auth-mode mapping is behaviorally identical to the current code, so nothing is lost by closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Desktop session sidebar is empty for the default profile only — named profiles unaffected, backend verified serving the rows

2 participants