XS✔ ◾ fix: don't require or send a GitHub PAT for the public stable release channel#983
XS✔ ◾ fix: don't require or send a GitHub PAT for the public stable release channel#983tomek-i wants to merge 1 commit into
Conversation
… channel
The stable ("latest") release channel is served from this repo's public
GitHub releases and never needs authentication, but configureAutoUpdater()
unconditionally attached any saved token as a Bearer header before
branching on channel type — so a saved-but-invalid/expired token (which
this channel never health-gates, unlike PR channels) was still sent on
every request to the public provider. The "Check for Updates" button was
also disabled on the stable channel whenever no token was saved at all.
Move the token attachment into the PR-channel branch, only after the
token is confirmed healthy, and explicitly clear any leftover
Authorization header when configuring the stable channel. Stop gating
the "Check for Updates" button's enabled state on hasGitHubToken for the
stable channel.
Closes #600
PR Metrics✔ Thanks for keeping your pull request small.
Metrics computed by PR Metrics. Add it to your Azure DevOps and GitHub PRs! |
github-actions
Bot
changed the title
There was a problem hiding this comment.
Pull request overview
Fixes update-check behavior so the public Latest Stable release channel never requires or sends a GitHub PAT, while keeping PR channels gated behind token health.
Changes:
- Backend:
configureAutoUpdater()no longer unconditionally attaches a saved GitHub token; stable channel explicitly clears anyAuthorizationheader, PR channel attaches the token only after a healthy-token gate. - UI: “Check for Updates” is no longer disabled for the stable channel when no token is saved or the saved token is invalid/expired.
- Tests: added backend + UI regression coverage for the stable-channel no-token/invalid-token scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/backend/ipc/release-channel-handlers.ts | Clears auth headers for stable channel; attaches token only after PR token health check passes. |
| src/backend/ipc/release-channel-handlers.test.ts | Adds regression test ensuring stable channel doesn’t end up with an auth header. |
| src/ui/src/components/settings/release-channels/ReleaseChannelSetting.tsx | Updates button disabled logic so stable updates aren’t token-gated. |
| src/ui/src/components/settings/release-channels/ReleaseChannelSetting.test.tsx | Adds UI tests ensuring stable “Check for Updates” stays enabled without/with invalid token. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { autoUpdater } = (await import("electron-updater")) as unknown as { | ||
| autoUpdater: { requestHeaders?: Record<string, string> }; | ||
| }; | ||
| autoUpdater.requestHeaders = {}; |
|
🚀 Pre-release build is available for this PR: |
tomek-i
added
armada:reviewing
|
🔭 crows-nest: picked up by ARMADA — dispatching to muster for review. |
| // as a Bearer header on every request to the public provider, risking spurious 401s instead of | ||
| // the clean unauthenticated request GitHub expects for public repos. Explicitly clear any | ||
| // Authorization header a previous PR-channel configuration may have left behind. | ||
| const { Authorization: _unused, ...headersWithoutAuth } = autoUpdater.requestHeaders ?? {}; |
There was a problem hiding this comment.
[major] Shared autoUpdater.requestHeaders singleton creates cross-channel header staleness/races
Two related gaps in how the fix manages the shared, module-level autoUpdater.requestHeaders object (flagged independently by both review lenses):
- Concurrent clear can wipe a PR-channel token mid-flight.
configureAutoUpdater({type:"latest"})now unconditionally clearsAuthorizationfrom the shared singleton.checkForUpdates()'s manual button and the 10-minute periodic timer both call into this asynchronously and independently. If a periodic-timer PR-channel check is in flight (relying on theAuthorizationheader a previousconfigureAutoUpdater(pr)call attached) at the same moment a user manually checks updates on the "latest" channel, the "latest" call's header-clear can strip the token out from under the concurrent PR request. This interaction didn't exist before this diff, since the header used to be reattached unconditionally at the top of every call regardless of channel. - Token rotation while already on a PR channel doesn't refresh the header.
checkForUpdates()'s PR-channel branch (~line 458, same file) re-verifies token health viaisGitHubTokenHealthy()but never itself re-attachesrequestHeaders.Authorization— it purely inherits whatever value was set by the lastconfigureAutoUpdater()call. If a user revokes and replaces their token via Settings without switching channels afterward, the health check reports "healthy" for the new token, but the actual outbound request still carries the old, stale token value. This undermines the PR's own stated goal ("only attach the token once confirmed healthy") on the very code path it was trying to make safer.
Suggested fix: give request-header attachment a single, per-request source of truth (e.g. compute/attach the current header value at the point of each autoUpdater.checkForUpdates() call, or centralize all header mutation behind one method both configureAutoUpdater and checkForUpdates call), rather than mutating a long-lived shared object from multiple independent call sites.
flagged by: code-review + codex-rescue (independently, from different angles)
| const { autoUpdater } = (await import("electron-updater")) as unknown as { | ||
| autoUpdater: { requestHeaders?: Record<string, string> }; | ||
| }; | ||
| autoUpdater.requestHeaders = {}; |
There was a problem hiding this comment.
[minor] New regression test doesn't simulate the actual leftover-header regression scenario
The new test resets autoUpdater.requestHeaders = {} immediately before calling configureAutoUpdater({type:"latest"}), so the destructure-and-clear logic this PR adds (const { Authorization: _unused, ...headersWithoutAuth } = ...) is a no-op in this test — verified empirically by removing the two clearing lines from the source and re-running just this test, which still passed.
The test as written only proves a token present in the token store isn't newly read/attached for the latest branch (the weaker claim — already guaranteed just by deleting the old unconditional getToken() call at the top of the method). It doesn't prove the actual regression scenario described in the PR's own commit message: a leftover Authorization header from a prior PR-channel configureAutoUpdater() call actually gets stripped when switching to latest.
Suggested strengthening: seed autoUpdater.requestHeaders = { Authorization: "Bearer stale-token" } before calling configureAutoUpdater({type:"latest"}), then assert it's cleared afterward.
flagged by: codex-rescue
| // as a Bearer header on every request to the public provider, risking spurious 401s instead of | ||
| // the clean unauthenticated request GitHub expects for public repos. Explicitly clear any | ||
| // Authorization header a previous PR-channel configuration may have left behind. | ||
| const { Authorization: _unused, ...headersWithoutAuth } = autoUpdater.requestHeaders ?? {}; |
There was a problem hiding this comment.
[nit] Unused-var alias in destructuring-omit is inconsistent with the codebase's style
const { Authorization: _unused, ...headersWithoutAuth } = autoUpdater.requestHeaders ?? {} uses an explicit _unused alias for the omitted key. Other rest-destructure-to-omit patterns elsewhere in this codebase (e.g. const { hotkeys, ...restOfPatch } = ... in user-settings-handlers.ts, const { execute, ...rest } = ... in language-model-provider.ts) don't use an _-prefixed alias, since Biome's ruleset here doesn't flag rest-destructured bindings as unused. Not a lint failure, just a minor style inconsistency — could drop the alias to const { Authorization, ...headersWithoutAuth } = ... (with an eslint-disable comment if a rule does complain) to match convention.
flagged by: code-review
Muster review — PR #983Two independent lenses reviewed this diff in parallel (code-review: conventions/correctness; codex-rescue: root-cause/second-opinion). Findings below are consolidated and deduped. Bottom line: 0 blocking, 1 major, 2 minor, 1 nit — not blocking, but the major finding is worth a look before merge. Summary by severity
Findings
Lens agreementThe two lenses converged independently on the same underlying architectural concern (shared mutable Otherwise cleanBoth lenses confirmed: the Posted by armada:muster — 2 lenses (code-review + codex-rescue), not degraded. |
tomek-i
added
armada
|
🔭 crows-nest: picked up by ARMADA — dispatching to muster for review. |
| // the clean unauthenticated request GitHub expects for public repos. Explicitly clear any | ||
| // Authorization header a previous PR-channel configuration may have left behind. | ||
| const { Authorization: _unused, ...headersWithoutAuth } = autoUpdater.requestHeaders ?? {}; | ||
| autoUpdater.requestHeaders = headersWithoutAuth; |
There was a problem hiding this comment.
[major] Stale Authorization header can survive when channel.type === "pr" but channel.channel is falsy
configureAutoUpdater() only clears/sets the Authorization header inside the channel.type === "latest" branch (here) and inside the channel.type === "pr" && channel.channel branch further down (~line 627). ReleaseChannel.channel is typed as optional (channel?: string), so a value of { type: "pr" } with a missing/empty channel satisfies neither branch — autoUpdater.requestHeaders is left completely untouched, so any Authorization header attached by a previous, successful PR-channel configuration would persist.
This is exactly the class of bug #600 is about (a stale/invalid token being sent when it shouldn't be). Today the UI always supplies a non-empty channel string when setting a pr-type channel (setChannel({ type: "pr", channel: \beta.${value}` })), so it isn't reachable through the current UI flow — but configureAutoUpdater`'s own contract doesn't defend against it (persisted storage from a migration, manual edit, or a future caller could hit it). Consider hoisting the header-clearing so it runs on any branch that doesn't end up explicitly attaching a fresh token, rather than only inside the two known-good branches.
flagged by: code-review
| // the clean unauthenticated request GitHub expects for public repos. Explicitly clear any | ||
| // Authorization header a previous PR-channel configuration may have left behind. | ||
| const { Authorization: _unused, ...headersWithoutAuth } = autoUpdater.requestHeaders ?? {}; | ||
| autoUpdater.requestHeaders = headersWithoutAuth; |
There was a problem hiding this comment.
[major] No synchronization around configureAutoUpdater lets concurrent channel-switch/periodic-check calls interleave and leak headers either direction
autoUpdater (electron-updater) and its requestHeaders are process-wide singleton state, but configureAutoUpdater() is invoked from multiple independent, uncoordinated call sites: app startup (src/backend/index.ts:506, fire-and-forget, not awaited), setChannel() (line ~197), and checkForUpdates()'s latest arm (line ~546) — plus the 10-minute periodic timer that re-invokes checkForUpdates() on its own schedule.
The pr branch does two awaits (isGitHubTokenHealthy() → network call, then tokenStore.getToken()) before attaching Authorization; the latest branch strips it synchronously. If a PR-channel configuration is in-flight (e.g. triggered by the periodic timer) while the user switches to latest (or vice versa), the two calls' effects can interleave on the shared autoUpdater object — e.g. latest strips the header, then the still-in-flight pr call's header-attach completes afterward and re-attaches a token onto an autoUpdater now configured with the public 'github' feed for latest. This isn't addressed by the PR and isn't covered by any test. A fix would need per-invocation isolation (e.g. a single in-flight promise/queue guarding configureAutoUpdater, or a version/generation check before mutating shared state).
flagged by: codex-rescue
| const handlers = new ReleaseChannelIPCHandlers(); | ||
| await handlers.configureAutoUpdater({ type: "latest" }); | ||
|
|
||
| expect(autoUpdater.requestHeaders?.Authorization).toBeUndefined(); |
There was a problem hiding this comment.
[major] New #600 regression test doesn't actually seed a stale Authorization header, so it can't prove the claimed fix
Both review lenses independently flagged this. The test sets autoUpdater.requestHeaders = {} (empty) immediately before calling configureAutoUpdater({ type: "latest" }), then asserts Authorization is undefined afterward. Since there was never an Authorization key present to begin with, this test would pass identically even if the destructure-and-strip logic in configureAutoUpdater were deleted outright — it never exercises the 'leftover header from a previous PR-channel configuration' scenario that the PR description and the inline code comment both call out as the actual bug being fixed. It also doesn't add coverage beyond the pre-existing 'no token attached at all for stable' test.
Suggested fix: seed autoUpdater.requestHeaders = { Authorization: 'Bearer stale-pr-token' } before calling configureAutoUpdater({ type: "latest" }), then assert the header is gone afterward — that actually exercises (and would fail without) the strip logic.
flagged by: code-review + codex-rescue
| // the clean unauthenticated request GitHub expects for public repos. Explicitly clear any | ||
| // Authorization header a previous PR-channel configuration may have left behind. | ||
| const { Authorization: _unused, ...headersWithoutAuth } = autoUpdater.requestHeaders ?? {}; | ||
| autoUpdater.requestHeaders = headersWithoutAuth; |
There was a problem hiding this comment.
[minor] Header-clearing only handles the exact key "Authorization"; no defense against a differently-cased or additional leftover header
The destructure const { Authorization: _unused, ...headersWithoutAuth } = autoUpdater.requestHeaders ?? {} is correct today only because this codebase is the sole writer of autoUpdater.requestHeaders and always uses the literal key "Authorization" (confirmed by grep — no other casing is ever written). But this is an implicit assumption with no guard: if electron-updater or a future contributor ever sets a differently-cased key (e.g. lowercase "authorization") or another auth-adjacent header, it would silently survive this strip and still be sent to the public unauthenticated "latest" endpoint — exactly the class of bug this PR is trying to eliminate. Consider clearing requestHeaders wholesale for the "latest" branch (autoUpdater.requestHeaders = {}) rather than selectively deleting one known key, since "latest" never needs any custom headers at all.
flagged by: codex-rescue
| // the clean unauthenticated request GitHub expects for public repos. Explicitly clear any | ||
| // Authorization header a previous PR-channel configuration may have left behind. | ||
| const { Authorization: _unused, ...headersWithoutAuth } = autoUpdater.requestHeaders ?? {}; | ||
| autoUpdater.requestHeaders = headersWithoutAuth; |
There was a problem hiding this comment.
[nit] Destructure-to-omit pattern is a one-off compared to existing idioms in this file
const { Authorization: _unused, ...headersWithoutAuth } = autoUpdater.requestHeaders ?? {}; autoUpdater.requestHeaders = headersWithoutAuth; works correctly, but the rest of this file always builds requestHeaders additively via spread ({ ...autoUpdater.requestHeaders, Authorization: ... }) rather than via destructure-omit. Not a correctness issue, just a minor stylistic inconsistency — a small shared helper (e.g. clearAuthHeader()/buildRequestHeaders(token?)) would read more consistently with the surrounding code and reduce duplication between the two places headers are now built.
flagged by: code-review
muster review — PR #983Two independent lenses reviewed this diff in parallel: a conventions/correctness code-review pass and a codex-rescue root-cause second opinion. Findings below are consolidated and deduped; both lenses had full context on the diff and the surrounding Verdict: 0 blocking, 3 major, 2 minor, 1 nit — no blocking issues, but the regression test needs strengthening before this is airtight. Summary by severity
Major findings (posted inline)
Minor / nit
Lens agreementThe two lenses converged independently on the same weak spot (finding #2 — the test doesn't prove the "clear leftover header" claim), which is the strongest signal in this review. They diverged in focus otherwise: code-review concentrated on branch-coverage/type-safety gaps in the diff itself, codex-rescue went wider into concurrency/singleton-state risk across the whole Bottom lineNo blocking issues — the core fix (don't attach a token for the stable channel; only attach for Findings posted as inline review comments above. This review does not approve/request-changes or merge — that's for the fleet's gating step. |
tomek-i
added
armada
|
🔭 crows-nest: review complete — 0 blocking findings (3 major, 2 minor, 1 nit; see inline comments). |
tomek-i
added
the
armada:reviewing
|
🔭 crows-nest: ready-PR pipeline started — review → address → re-validate → gated merge. |
| expect(result).toEqual({ available: false, currentVersion: "1.2.3" }); | ||
| }); | ||
|
|
||
| it("does not attach a saved (possibly invalid/expired) token as an auth header when switching to the latest stable (public) channel (#600)", async () => { |
There was a problem hiding this comment.
[minor] New regression test doesn't verify clearing of a pre-existing stale Authorization header
The new test sets autoUpdater.requestHeaders = {} (empty) before calling configureAutoUpdater({ type: "latest" }), then asserts no Authorization header appears. This correctly pins the original bug (unconditional token attach before branching), but it would still pass even if the header-stripping destructure (const { Authorization: _unused, ...headersWithoutAuth } = autoUpdater.requestHeaders ?? {}) were deleted entirely — starting from an empty object there's nothing to strip. It doesn't exercise the scenario the added code comment explicitly claims to guard: "Explicitly clear any Authorization header a previous PR-channel configuration may have left behind."
Suggested fix: add/extend a test that first sets autoUpdater.requestHeaders = { Authorization: 'Bearer stale-pr-token', 'X-Some-Other-Header': 'keep-me' } (simulating state left over from a prior PR-channel configuration) before calling configureAutoUpdater({ type: 'latest' }), then assert Authorization is gone and the unrelated header survives.
flagged by: code-review + codex-rescue (independent agreement)
| // Only attach the token once it's confirmed healthy above (#600) — PR release assets are | ||
| // downloaded via a "generic" feed pointed at this repo's own GitHub releases, which needs | ||
| // authentication headers to avoid anonymous rate limits during the download. | ||
| const githubToken = await this.tokenStore.getToken(); |
There was a problem hiding this comment.
[minor] Redundant token fetch in PR branch of configureAutoUpdater
isGitHubTokenHealthy() (called just above, gating this branch) already reads the token from this.tokenStore.getToken() internally to perform its health check, and then this line reads it again from the same store just to attach the Authorization header. Not a correctness bug — the store is a simple local read with no intervening async work that could change the value — but it's an avoidable double read. Consider having isGitHubTokenHealthy() return the token it already fetched (or threading it through) to avoid the duplicate call.
flagged by: codex-rescue
Muster review — PR #983Two independent lenses reviewed this diff in parallel: code-review (conventions/correctness) and codex-rescue (root-cause second opinion). Both lenses independently traced the full Summary
No blocking findings. Both lenses converged on the same conclusion: the core fix correctly stops attaching a saved token before branching on channel type, correctly strips any leftover Findings (both minor, non-blocking)
Also noted (not filed as a PR finding — pre-existing/out of scope)codex-rescue additionally observed that Bottom line0 blocking, 0 major, 2 minor — no blocking findings, ready pending optional test-coverage strengthening. The two lenses agreed throughout; no unresolved disagreement between them. Posted by |
tomek-i
removed
the
armada:reviewing
|
✅ crows-nest: reviewed — 2 findings (0 blocking, 0 major, 2 minor). Advisory only — awaiting human merge (auto-merge off). |
tomek-i
added
the
armada:reviewing
|
🔭 crows-nest: ready-PR pipeline started — review → address → re-validate → gated merge. |
| const { autoUpdater } = (await import("electron-updater")) as unknown as { | ||
| autoUpdater: { requestHeaders?: Record<string, string> }; | ||
| }; | ||
| autoUpdater.requestHeaders = {}; |
There was a problem hiding this comment.
[minor] New #600 regression test doesn't seed a leftover Authorization header, so it doesn't prove the strip
The test sets autoUpdater.requestHeaders = {} (already empty) immediately before calling configureAutoUpdater({ type: "latest" }), then asserts Authorization is undefined. Since there was never an Authorization key present to begin with, this only proves the latest branch doesn't add a header — it doesn't exercise the actual regression the PR's own comment describes ("clear any Authorization header a previous PR-channel configuration may have left behind"). getTokenMock.mockResolvedValue(...) is also a red herring here since the latest" branch never calls tokenStore.getToken()`.
Suggested fix: seed autoUpdater.requestHeaders = { Authorization: "Bearer stale-token" } (simulating a prior PR-channel config) before calling configureAutoUpdater for latest, then assert it's stripped — ideally also with a sibling header present to prove only Authorization is removed.
flagged independently by: code-review + codex-rescue
|
[minor] checkForUpdates()'s PR-channel download path never sets Authorization itself — relies on state left by a prior configureAutoUpdater() call ( This branch calls flagged by: codex-rescue |
|
[nit] Unused destructured binding pattern is new to this file (
flagged by: code-review |
|
[nit]
flagged by: codex-rescue |
🔭 muster review — PR #983Bottom line: no blocking findings — review advisory only. Two independent lenses reviewed this diff in parallel (code-review + codex-rescue), never seeing each other's notes.
Findings:
Lens agreement: both lenses independently converged on the same core gap (finding #1) — that's the strongest thing in this review. No disagreement between the lenses otherwise. Fix itself is sound: the header-stripping logic was verified against undefined/empty/populated This review is advisory only — muster does not approve/request-changes/merge. autoMerge is currently off for this repo, so this PR proceeds to "awaiting human merge" once re-validated. |
tomek-i
removed
the
armada:reviewing
|
✅ crows-nest: reviewed, no blocking findings, CI green — awaiting human merge (auto-merge off). |
Summary
The "Latest Stable" release channel is served from this repo's public GitHub releases and never needs authentication, but
configureAutoUpdater()unconditionally attached any saved GitHub token as aBearerheader before branching on channel type — so a saved-but-invalid/expired token was still sent on every stable-channel request (risking spurious 401s on a request that should be anonymous). The "Check for Updates" button was also disabled on the stable channel whenever no token was saved at all, even though the stable channel doesn't need one.Closes #600
What changed
src/backend/ipc/release-channel-handlers.tsconfigureAutoUpdater(): stop unconditionally attaching a saved token before branching on channel type. For thelatest(stable, public) channel, explicitly clear any leftoverAuthorizationheader instead. For theprchannel, attach the token only after its health has already been confirmed further down in that branch.src/backend/ipc/release-channel-handlers.test.tssrc/ui/src/components/settings/release-channels/ReleaseChannelSetting.tsxhasGitHubTokento be enabled for the stable channel — only PR channels stay gated onisTokenHealthy(per #919).src/ui/src/components/settings/release-channels/ReleaseChannelSetting.test.tsxDecisions
isGitHubTokenHealthy()passes, rather than attaching it unconditionally up front.Acceptance criteria
Testing
Ran the project's full configured suite from a clean worktree:
npm run build— passesnpm rebuild better-sqlite3 --build-from-source && npx vitest run --exclude 'src/ui/**'— 692/692 backend tests passnpm --prefix src/ui install && npm --prefix src/ui test— 261/261 UI tests passnpm run lint— clean (one pre-existinginfo-level suggestion in an unrelated file,Cloud360LiveView.tsx, not touched by this change)Bug repro evidence
Authorization: Bearerheader on requests to the public "Latest Stable" release channel, and the "Check for Updates" button is disabled for that channel whenever no token is saved — even though the stable channel is a public repo and never needs one.release-channel-handlers.test.ts) that configures the stable channel with a saved token and asserts noAuthorizationheader is attached to theautoUpdater, run against the unpatched code first.expected 'Bearer saved-but-possibly-invalid-tok…' to be undefined, confirming the token was attached unconditionally.Authorizationisundefinedfor the stable channel regardless of any saved token. The full suite (13/13 in this test file) is green.Follow-up items