Add in-dashboard update checker - #635
Conversation
tashfeenahmed
left a comment
There was a problem hiding this comment.
Reviewed this properly — read the whole diff, ran the suites locally, and captured the actual outbound request. The engineering is good: response validation is thorough, the error paths are stable and don't leak upstream detail, the 404/401/403/429 handling is right, and the test file genuinely covers the interesting cases (cache, coalescing, Atom fallback, malformed payloads, no-identity). Nothing touches licensing, tiers, catalog sync, or any other phone-home path — update.ts imports node:child_process and express and nothing else. Locales are mechanically sound and check:i18n passes at 60/677.
Two things need to change before this can go in.
1. There's no way to turn it off.
Self-hosted boxes need an off switch for anything that talks to the internet, and there isn't one here — no env var, no setting. Airgapped and policy-restricted installs currently get a Settings section that can only fail, and each click costs two outbound attempts plus a logger.error.
Please add an opt-out following the existing convention (# FREELLMAPI_COMPRESSION=off, # FREELLMAPI_CONTEXT_HANDOFF=...), e.g. FREELLMAPI_UPDATE_CHECK=off. When it's off I'd expect /check and /status to report disabled without fetching or shelling out to git, and the UI section to hide rather than render a dead button.
2. Drop the ambient GITHUB_TOKEN pickup.
resolveIdentity/performCheck read bare process.env.GITHUB_TOKEN and attach it as Authorization: Bearer. I confirmed it on the wire:
GET https://api.github.com/repos/tashfeenahmed/freellmapi/compare/<local-sha>...main
Accept: application/vnd.github+json
User-Agent: freellmapi-update-checker
X-GitHub-Api-Version: 2022-11-28
Authorization: Bearer <whatever GITHUB_TOKEN happens to be>
The URL and UA are fine — the local SHA is inherent to an ancestry check and I have no problem with it. The token is the issue. GITHUB_TOKEN is not currently read anywhere else in this repo, so this PR introduces a new implicit read of one of the most generically-named variables there is. Plenty of Compose users already have GITHUB_TOKEN in a shared root .env for unrelated tooling, and it's frequently a broad-scope PAT. Silently borrowing it turns an anonymous update ping into one authenticated as that user's GitHub account, which is more than the check needs and more than anyone opted into.
Same host so it isn't exfiltration, but it should be explicit: namespace it (FREELLMAPI_UPDATE_GITHUB_TOKEN or similar) so it's only used when someone deliberately sets it for this feature.
3. .env.example needs the new vars.
Related to the above and a concrete bug on its own: nothing was added to .env.example, so anyone who actually sets the token gets flagged by our own env-drift checker as an unknown key on startup. Document the token var and the kill switch there. FREELLMAPI_COMMIT_SHA / FREELLMAPI_INSTALL_METHOD are build-injected and read from the process env rather than the file, so those don't trip drift — but a commented note wouldn't hurt.
Non-blocking, take or leave:
- Failures aren't negatively cached. Every retry re-hits the API and then the Atom feed. A short negative TTL (30–60s) would stop a stuck box from re-dialing on every click.
execFileSync('git', ...)with a 5s timeout runs inside the request handler, so first open of Settings can block the event loop. Lazy is right; async would be better.- Self-built Docker images (
docker buildwith no--build-arg) end up as installationunknown, notdocker, because the install-method assignment is gated behind a valid SHA. Cosmetic, but those users get the Releases link instead of thedocker compose pullhint. MAX_ATOM_FEED_LENGTHis checked afterawait atomResponse.text(), so the cap doesn't actually bound what gets read into memory.ARG FREELLMAPI_COMMIT_SHAsits above the runtimeCOPYlayers, so a changing SHA invalidates cache for all of them. Moving it below the COPYs keeps the layer cache useful.- The updates section reuses
premium.checkForUpdatesandpremium.lastChecked. Fine today, fragile if those strings get restructured — worth their own keys undersettings..
Fix 1–3 and I'll merge. CI is green and the rest of this is solid work.
|
Addressed all three requested changes:
I also preserved Rebased/merged current |
Summary
mainReliability and privacy
Testing
npm test(1,503 server, 44 CLI, 15 client tests)npm run buildnpm run build:allindesktop/settings-dialog.tsxCloses #491