Skip to content

fix(desktop): renew the local bearer token before its session expires - #18384

Closed
saphid wants to merge 3 commits into
pingdotgg:mainfrom
saphid:t3code/sweep/desktop-bearer-renewal
Closed

saphid wants to merge 3 commits into
pingdotgg:mainfrom
saphid:t3code/sweep/desktop-bearer-renewal

Conversation

@saphid

@saphid saphid commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

A desktop left running beyond its bearer session lifetime kept returning the expired token from both the main-process and renderer caches. Renewing alone also failed for legacy backends: their fixed bootstrap grant expired after 24 hours, before a 30-day bearer needed renewal.

Main now retains the token with its deadline and exchanges again when 60 seconds or less remain, sharing exchanges under the existing semaphore. Renderer reads share only an in-flight request. Rotating bootstrap tokens are derived at exchange time; a configured legacy credential remains exchangeable for that backend’s lifetime. The server validates that credential directly and issues a fresh short-lived desktop grant, removing the legacy-only seeded-grant cache. A configured rotating secret still takes precedence; fixed launch tokens are rejected in that mode. Persisted pairing-link expiration, proof binding, consumption, revocation, and dev-startup expiration keep their existing path.

Credit to @saphid for identifying the two stale caches and the original renewal fix in #18384.

Verification: 36 focused cases across the four existing desktop/renderer/auth test files pass, consolidated from 39 by combining overlapping cache and renewal fixtures. Exact main fails the three intended regressions (33 pass / 3 fail); the original PR passes 35 / 36 and fails real legacy renewal; the rebuilt implementation passes all 36. The legacy service case exchanges and authenticates a 30-day bearer, advances TestClock 31 days, rejects the expired bearer, then exchanges the same configured credential and authenticates a new session with the same scopes. Desktop tests cover concurrent reads, near-expiry renewal, and a 31-day suspend using the current rotating window. Renderer tests cover shared pending reads and the next renewed token. Another 108 related session, HTTP, linking, authorization, and startup tests pass. Changed-file lint/format and desktop, web, and server typechecks pass.

This is headless service/unit evidence with real auth, session, signing, and in-memory SQLite services; no live Electron reconnect or OS suspend was exercised. Screenshots do not establish token renewal. macOS, Windows/WSL, Electron, iOS, and Android were not run here. A human should verify long-lived Electron HTTP/reconnect behavior and review the authentication policy change. Macroscope’s existing authentication human-review requirement remains outstanding; these checks do not replace it.

Implementation: Codex gpt-6.1-sol (high). Tests: Codex gpt-6-astra (high). Independent review: Claude claude-opus-5-5 and Codex gpt-6-sol (high), via T3 Code.

The desktop main process and the renderer each cached the primary
environment's bearer token forever. Once the 30-day session expired, the
backend closed the socket and every reconnect and HTTP request kept
presenting the dead token until the app was restarted.

The main process now records when its token expires and exchanges the
current bootstrap credential again a minute before then. The renderer only
shares an in-flight IPC request, so it always gets the main process's
current token.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 11, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — The PR fixes desktop bearer renewal across expiry and suspend scenarios, but it also changes server bootstrap-credential validation and credential lifetime in authentication code. The targeted tests are useful, yet the authentication policy and session-lifecycle changes require human review.

You can add or adjust custom eligibility rules. Learn more.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 11, 2026 — with ChatGPT Codex Connector
@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 247926de-82f2-40fd-bd53-a414602cd93b
📥 Commits

Reviewing files that changed from the base of the PR and between c1d528a and 35f7afe.

📒 Files selected for processing (4)
  • apps/desktop/src/backend/DesktopLocalEnvironmentAuth.test.ts
  • apps/desktop/src/backend/DesktopLocalEnvironmentAuth.ts
  • apps/web/src/environments/primary/desktopAuth.test.ts
  • apps/web/src/environments/primary/desktopAuth.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

📝 Walkthrough

Desktop auth now tracks bearer-token expiration and renews tokens before they expire. Renderer auth shares concurrent token requests but does not retain successful results for later reads.

Changes: Bearer token flow

Layer / File(s) Summary
Desktop token renewal
apps/desktop/src/backend/DesktopLocalEnvironmentAuth.ts, apps/desktop/src/backend/DesktopLocalEnvironmentAuth.test.ts
The desktop cache stores each token with its expiration and renews it when 60 seconds or less remain. The test checks cached reuse and a shared renewal exchange for concurrent reads.
Renderer request sharing
apps/web/src/environments/primary/desktopAuth.ts, apps/web/src/environments/primary/desktopAuth.test.ts
Concurrent renderer reads share one in-flight request. The shared promise clears after the request settles, so later sequential reads make new requests.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 35f7a

Desktop token renewal retains a margin before expiry, and no issue requiring a fix before merge is established.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 35f7a

The change refreshes an existing desktop credential without adding permissions or a new destination. No introduced security vulnerability was established, but access from other renderer contexts and recovery from interrupted renewal remain incompletely verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected authority remains the desktop primary environment and its existing bootstrap-granted scopes. The token IPC payload cannot select another environment or credential. Renewal changes credential availability over time, not the destination or privilege scope; complete renderer invocation reachability remains uncertain.

Security Findings and Attack Paths

  • inferred — Code executing in a bridge-enabled renderer can request the primary bearer through the existing bridge. This capability predates the PR; the comparison establishes no new caller route or token sink. Reachability from untrusted guests or frames was not established, so this is a boundary fact rather than a verified introduced attack path.

Trust Boundaries and Controls

  • observed — The bearer handler does not inspect sender identity, and generic IPC dispatch forwards the event without sender/window authorization. This enforcement limitation is unchanged by the PR. Main-window sandbox and navigation controls provide containment evidence, but not a complete proof that every invoker is trusted.

Resilience and Maintainability Implications

  • inferred — Replacement invalidates old sessions, so requests already holding an old bearer can race with renewal. Socket disconnects are classified as transient, and the supervisor retries connection preparation, which reads the current bearer again. This supports recovery without weakening revocation; individual HTTP replay and interruption after remote issuance remain unverified.

Hardening Proposals

  • proposed — Consider explicitly authorizing the expected main-window sender and frame before returning the bearer, reducing reliance on preload and renderer-topology assumptions. This would harden a pre-existing boundary, not remediate an established PR-introduced vulnerability.
Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description gives detailed problem, implementation, verification, limitations, and review context. However, it does not follow the required section structure and does not provide the required tria… Add explicit Problem, Change, Scope and approval, and Verification sections. Link the triaged issue or maintainer approval with the approval comment, or explain why this focused fix qualifies for an exemption. Complete the required human re…
Passed Passed checks: 3 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the main change: renewing the desktop local bearer token before session expiration.
Full details: Description check

Explanation

The description gives detailed problem, implementation, verification, limitations, and review context. However, it does not follow the required section structure and does not provide the required triaged issue or explicit maintainer approval for the authentication policy change. It also states that human review remains outstanding.

Resolution

Add explicit Problem, Change, Scope and approval, and Verification sections. Link the triaged issue or maintainer approval with the approval comment, or explain why this focused fix qualifies for an exemption. Complete the required human review before merging.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge juliusmarminge self-assigned this Oct 19, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one as maintainer for usage-billing-03. I will rebuild the renewal path against current main and cover recovery with the expiring legacy bootstrap grant as well as rotating bootstrap secrets. Credit to @saphid for identifying both token caches and the original renewal fix.

@github-actions github-actions Bot added size:L 100-499 changed lines (additions + deletions). and removed size:S 10-29 changed lines (additions + deletions). labels Oct 19, 2026

Copy link
Copy Markdown
Member

Closing in favour of #25024, which makes the same change. Thanks for the work here!

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

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants