Files
2cbf838158 feat(packages): GitHub Releases binary installer (#898)
* feat(packages): add GitHub Releases binary installer

New runtime source `github:owner/repo[@tag]` for installing Linux CLI
binaries from GitHub Releases. Admin-only, SHA256-verified, ELF-validated.

Backend:
- GitHub API client with 10-min cache + rate-limit mapping
- SSRF-guarded streaming downloader (HTTPS + host allowlist, re-validated
  on every redirect hop, literal-IP rejection)
- Checksums.txt / SHA256SUMS lookup with constant-time verify
- Archive extract (tar.gz / zip / raw) with path-traversal + zip-bomb
  guards, symlink skip
- ELF magic + 64-bit class + runtime-arch validation
- Atomic manifest persistence (temp + rename)

HTTP:
- POST /v1/packages/install accepts github: spec
- GET /v1/packages/github-releases for picker UI (viewer+, arch-filtered)
- Extended InstalledPackages response with github field
- github-bin runtime probe

Infra:
- Dockerfile creates /app/data/.runtime/bin (goclaw:goclaw 0755)
- docker-entrypoint.sh prepends bin dir to PATH
- Env-only config (never config.json): token, max size, org allowlist,
  bin dir, manifest path

UI:
- GitHub Binaries section + release picker modal
- Dismissable musl/glibc compatibility warning (localStorage)
- i18n keys across en/vi/zh

Docs: docs/packages-github.md user guide + 14-skills-runtime.md cross-ref.

Closes #741

* refactor(packages): revert validPkgName broadening + drop unused sentinel

Code review cleanup:
- validPkgName regex had `:` added defensively, but github: specs are
  validated separately via skills.ParseGitHubSpec before reaching this
  check — the broadening was dead attack surface.
- Drop unused ErrUnknownArchive sentinel + the `_ = ErrUnknownArchive`
  stub in extractRaw.

* feat(packages): per-user rate limit on /v1/packages/github-releases

Cap picker endpoint at 30 req/min/user (burst 10) to protect the shared
GitHub API quota. Key is userID (header X-GoClaw-User-Id) or remote IP
for anonymous callers. Returns 429 + Retry-After: 60 when tripped.

Standalone token-bucket limiter (stale-entry cleanup every 5 min) lives
in internal/http rather than importing internal/gateway, which would
create a package cycle.

* fix(ui): guard split()[0] for noUncheckedIndexedAccess strict TS

CI pnpm build failed on TS2345: `.split('@')[0]` returns
`string | undefined` under strict index access. Default to empty
string to satisfy the type checker; runtime behaviour unchanged
because the downstream regex rejects empty strings.

* fix(packages): address Claude review — medium + low findings

Medium
- rate limiter: atomic.Int64 lastSeen + amortized sweep replaces
  goroutine-based cleanup → fixes data race on lastSeen and the
  goroutine leak when tests swap the package-level limiter.
- checksum pipeline: slog.Warn on ReadFile and ParseChecksums failures
  (previously silent). "asset not listed" stays warn+proceed but is now
  documented as the publisher's choice — ELF validation remains the
  final gate.
- downloader: drop http.Client.Timeout (30s capped the whole request
  including body read, aborting large downloads on slow links). Context
  deadline from install timeout (5 min) is the correct bound.

Low / style / UI
- extractRaw honors maxUncompressed (ErrFileTooLarge on overflow) so
  the helper is safe outside the hot path.
- cmd/gateway_github_installer.go: drop the explicit cfg.Defaults()
  call — NewGitHubInstaller already invokes it.
- GitHubPackageEntry: remove unpopulated InstalledBy field + document
  why.
- owner regex tightened to 39-char GitHub limit (was 40).
- mu lock comment corrected: serializes only the disk-write phase.
- UI: shared stripPrefixAndTag helper + owner regex mirrors the backend
  39-char cap; destructure-with-default kills the split()[0] ?? ""
  awkwardness while still satisfying noUncheckedIndexedAccess.

Verified: go build (pg + sqliteonly) · go vet · go test -race
./internal/skills ./internal/http · pnpm build.

* fix(packages): address Claude review round 2

Medium
- validRepoPath now rejects trailing hyphens in the owner segment and
  caps at 39 chars, matching gitHubSpecRE exactly. Previously a subtle
  drift between the two validators could let `foo-/repo` slip to the
  GitHub API and surface as a 502 instead of a clean 400.
- handleGitHubReleases no longer forwards raw err.Error() from the
  upstream call. Maps sentinel errors:
    ErrGitHubRateLimited  → 429 + Retry-After
    ErrGitHubNotFound     → 404
    ErrGitHubUnauthorized → 502 "github authentication failed"
    default               → 502 "failed to fetch releases"
  Avoids leaking rate-limit reset timestamps / server internals to
  viewer-tier callers.

Low / UX
- Install response now returns the manifest entry for github: specs
  (new lookupGitHubEntry helper; nil-safe fallback to {ok:true}). Lets
  the UI display "installed: lazygit v0.42.0" without a list refresh.
- gitHubSpecRE tag segment capped at 1..255 chars (git ref-name bound).
  UI isValidFullSpec mirrors the same cap.

* fix(packages): address Claude review round 3

Medium
- github_api: URL-encode owner, repo, and tag via url.PathEscape when
  building API paths. Previously a tag containing '#' would be stripped
  as a URL fragment and '?' would inject a query parameter, silently
  hitting the wrong release.

Low / polish
- Uninstall via full "github:owner/repo[@tag]" spec now falls back to
  manifest lookup by owner/repo, handling packages whose binary name
  differs from the repo name (cli/cli → gh).
- GitHubClient.cache sweeps expired entries opportunistically when the
  map grows past 256 entries (prevents theoretical unbounded growth
  over long uptime).
- handleInstall for github: specs now calls GitHubInstaller.Install
  directly and returns the freshly-created manifest entry, eliminating
  the double manifest read via List() from the lookupGitHubEntry helper.
- pickBinaries comment corrected — actual behavior excludes paths
  matched by nonBinaryPathRE rather than enforcing a single-depth limit.

* fix(packages): address Claude review round 4 (style + ordering)

All 4 findings are Low severity:

- github_api.go: replace interface{} with any across the cache type,
  cacheGet return, cacheSet param, and doJSON out param.
- doJSON: rename local `url` to `apiURL` to avoid shadowing the
  "net/url" package import used by GetRelease/ListReleases.
- Uninstall: save the updated manifest BEFORE removing binaries on
  disk. If saveManifest fails we now bail out without leaving a
  manifest entry that still claims binaries which have been deleted
  (a retried Uninstall would otherwise hit ErrPackageNotInstalled
  after the first attempt wiped the files). Disk removal stays
  best-effort and warn-on-error, which matches the idempotent intent.
- pickBinaries: inline comment corrected to reflect actual behavior —
  depth is not enforced; nonBinaryPathRE filter + downstream ELF
  validation are the real gates.

* fix(packages): address Claude review round 5

All 3 findings are Low severity:

- handleInstall github fast-path now wraps the context with
  skills.InstallTimeout (5 min) before calling gh.Install and emits the
  same "skills: installing dep" / "dep installed" / "github install
  failed" log lines as the generic InstallSingleDep path, so
  operator-observability is identical between github: and pip:/npm:
  install flows.
- installTimeout promoted to exported InstallTimeout so the http layer
  shares the single source of truth rather than duplicating the
  5-minute constant.
- cacheMaxEntries comment clarifies it is a soft sweep trigger, not a
  hard cap — when every entry is still within TTL the map can briefly
  exceed the threshold by one insert.

* fix(packages): address Claude review round 6 (final Lows)

Both findings are Low severity (reviewer marked the PR "ready to merge"
already):

- github_installer: "no checksum asset available" downgraded from
  slog.Warn to slog.Info. Many popular upstream releases (jq, fzf,
  older ripgrep, etc.) ship no checksum file at all — that is publisher
  policy, not a problem with the install. The suspicious cases
  (checksum file unreadable, unparseable, or missing this asset) stay
  at Warn so they stand out.
- handleGitHubReleases response now uses a narrow assetPreview DTO
  (name + size_bytes) instead of embedding the full GitHubAsset type
  which also carried browser_download_url. The picker UI never rendered
  the URL; trimming the response keeps the viewer-tier surface minimal.
  UI AssetPreview interface realigned to match.

* fix(packages): address Claude review round 7

Narrow the GET /v1/packages GitHub entry to a viewer-safe projection
(repo/tag/binaries/name/installed_at), mirroring the assetPreview fix
from round 6. Strips asset_url, sha256, and asset_name from the list
response — viewer-level callers no longer see CDN download URLs or
checksum metadata for installed packages. UI types realigned; the
removed fields were never rendered.

Finding #2 (install writes binary before manifest save) left as noted —
reviewer confirmed informational only, self-heals on retry, no security
impact since binaries pass ELF validation before being written.

* fix(packages): address Claude review round 8

Map HTTP 429 (GitHub secondary rate limits — abuse detection,
unauthenticated bursts, search) to ErrGitHubRateLimited in the API
client so the picker endpoint renders 429 "rate limit reached" with
Retry-After: 60 instead of falling through to 502 "failed to fetch
releases". Primary rate limits (403 + X-RateLimit-Remaining: 0) were
already handled; this covers the secondary class documented at
https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#about-secondary-rate-limits

* fix(packages): address Claude review round 9

Two defensive hardenings flagged as Very Low:

- ParseChecksums: strip leading `./` from checksum filenames.
  `sha256sum ./file` emits `./file` in the name column; the caller
  looks up by bare asset basename so `./`-prefixed entries would
  silently miss. Real release checksums almost never use this form,
  but the guard is essentially free.

- doJSON: cap response body at 8 MiB via io.LimitReader before JSON
  decode. Current GitHub list/release payloads are well under this
  (~1 MiB at per_page=100). Guards against future call sites or a
  misbehaving upstream returning an oversized document.

* fix(cron): eliminate cross-test race on runLoopTickInterval

`Service.Stop()` closes stopChan but does not wait for the runLoop
goroutine to exit. In the test suite, test A's `defer cs.Stop()` can
return before the spawned runLoop has reached
`ticker := time.NewTicker(runLoopTickInterval)`. If test B then calls
`setFastTick()` to mutate the package-level var, the race detector
correctly flags it:

  Read at runLoopTickInterval by goroutine A (runLoop ticker init)
  Previous write by goroutine B (setFastTick in test B)

Fix: snapshot `runLoopTickInterval` inside `Start()` under the mutex
before spawning the goroutine, and pass the value as a parameter to
`runLoop`. The spawned goroutine no longer reads the package-level
var, so the cross-test window is closed. Production behavior
unchanged.

Verified: `go test -race -count=3 ./internal/cron/...` passes three
times in a row; the CI failure on PR #898 reproduced before the fix
and is gone after.

* fix(packages): address review P0/P1/P2 + new DoS vector

P0.1 — UI uninstall 400: parseAndValidatePackage now accepts
  github:<bare-name> (manifest Name form, no owner/repo) in addition
  to the full spec. UI sends github:${pkg.name} from the manifest;
  dispatcher already tolerated bare names — the HTTP validator was
  the only gate rejecting them. Install path re-validates strictly
  via ParseGitHubSpec and bare-name install returns 400 now (was 500).

P1.1 — ExtractArchive raw-ELF fallback name: add ExtractArchiveAs(
  path, fallbackName, max). Installer passes parsed.Repo so raw
  (non-archive) ELF assets no longer end up recorded as
  /tmp/goclaw-gh-asset-XXXX.bin — that basename would leak into the
  manifest Binaries entry and break PATH lookup.

P1.3 — Archive entry count cap: maxArchiveEntries = 10_000 +
  ErrTooManyEntries sentinel. Tar: count ALL headers seen (incl.
  symlinks/dirs we skip) to block the gzip-bomb-of-headers DoS —
  header bytes don't count against maxUncompressed for zero-size
  entries. Zip: pre-check via peekZipEntryCount reads the EOCD
  record manually and rejects oversized archives BEFORE
  zip.OpenReader allocates []*zip.File of declared capacity (this
  was a fresh red-team finding; stdlib would otherwise alloc ~1GB
  for a crafted 200MB zip claiming 4M entries).

P1.4 — Rate-limit install/uninstall: packagesWriteLimiter
  (10/min/user, burst 3). Admin-only mitigates but a compromised
  token could otherwise flood upstream (GitHub/pip/npm) or spam
  manifest mutations.

P1.6 — Non-Linux early reject: ErrUnsupportedOS guard at the top
  of Install(). Windows/macOS hosts no longer waste bandwidth
  fetching a Linux asset just to fail at the ELF machine check.

P1.7 — Manifest fsync: OpenFile → Write → Sync → Close → Rename →
  dir Sync, with tmp cleanup on every error path. POSIX doesn't
  guarantee durability via rename alone; XFS / ext4 with async
  journal can reorder.

P2.1 — Belt-and-suspenders zip runtime break when cumulative bytes
  reach the cap (pre-declared check already covers it but the
  streaming loop now bails immediately).

P2.6 — Binary-name collision warn: slog.Warn when a different repo
  already owns the basename we're about to overwrite. Last-writer-
  wins unchanged; operator now gets a signal instead of silence.

Hardening — rate-limit key: rateLimitKeyFromRequest prefers
  store.UserIDFromContext over the raw X-GoClaw-User-Id header so
  an admin can't rotate the header mid-session to dodge the bucket.
  Header/IP fallback retained for pre-auth / test callers.

Tests: 9 new cases on parseAndValidatePackage (github full/bare/
  empty/traversal/injection/space/leading-hyphen);
  TestExtractArchiveAs_RawELFUsesFallbackName;
  TestExtractTarGz_EntryCountCap + TestExtractZip_EntryCountCap;
  TestPeekZipEntryCount (DoS pre-check path).

Verified: go build ./... && go build -tags sqliteonly ./... &&
  go vet ./... && go test -race ./internal/skills/... ./internal/http/...

---------

Co-authored-by: viettranx <viettranx@gmail.com>
2026-04-16 15:09:48 +07:00

96 lines
3.2 KiB
Go

package skills
import (
"crypto/subtle"
"errors"
"fmt"
"regexp"
"strings"
)
// ErrChecksumMismatch is returned when a computed SHA256 doesn't match the expected value.
var ErrChecksumMismatch = errors.New("github: checksum mismatch")
// checksumLineRE parses `<sha256> <filename>` or `<sha256> *<filename>` lines.
var checksumLineRE = regexp.MustCompile(`(?m)^([a-fA-F0-9]{64})[ \t]+\*?(\S.*)$`)
// FindChecksumAsset scans a release's assets looking for a checksum file.
// Returns nil if none found.
// Lookup order:
// 1. <assetName>.sha256
// 2. checksums.txt
// 3. SHA256SUMS / SHA256SUMS.txt
// 4. *_checksums.txt (pattern match)
// 5. *.sha256sums
func FindChecksumAsset(release *GitHubRelease, assetName string) *GitHubAsset {
if release == nil {
return nil
}
targetPerAsset := strings.ToLower(assetName + ".sha256")
byLower := func(n string) string { return strings.ToLower(n) }
// Pass 1: exact per-asset .sha256 companion.
for i := range release.Assets {
if byLower(release.Assets[i].Name) == targetPerAsset {
return &release.Assets[i]
}
}
// Pass 2: common aggregate names.
priorities := []string{"checksums.txt", "sha256sums", "sha256sums.txt", "sha256sum.txt"}
for _, p := range priorities {
for i := range release.Assets {
if byLower(release.Assets[i].Name) == p {
return &release.Assets[i]
}
}
}
// Pass 3: fuzzier match — trailing `_checksums.txt` / `-checksums.txt` / `.sha256sums`.
for i := range release.Assets {
n := byLower(release.Assets[i].Name)
if strings.HasSuffix(n, "_checksums.txt") || strings.HasSuffix(n, "-checksums.txt") {
return &release.Assets[i]
}
if strings.HasSuffix(n, ".sha256sums") {
return &release.Assets[i]
}
}
return nil
}
// ParseChecksums parses a checksums.txt-style file into a map of filename → SHA256.
// Accepts both `<sha> <name>` (two spaces) and `<sha> *<name>` (binary prefix).
// Unrecognized lines are ignored (comments, empty lines).
func ParseChecksums(content []byte) (map[string]string, error) {
matches := checksumLineRE.FindAllSubmatch(content, -1)
out := make(map[string]string, len(matches))
for _, m := range matches {
sha := strings.ToLower(string(m[1]))
name := strings.TrimSpace(string(m[2]))
// `sha256sum ./file` emits `./file` in the name column. Strip the
// leading `./` so the lookup in the caller (keyed by bare asset
// basename) matches. Real release checksums almost never use this
// form, but the guard is defensive and essentially free.
name = strings.TrimPrefix(name, "./")
// In "checksums.txt" the name may be just a basename. Keep as-is;
// caller looks up by asset name which matches basename.
out[name] = sha
}
if len(out) == 0 {
return nil, fmt.Errorf("no valid checksum entries found")
}
return out, nil
}
// VerifyChecksum does a constant-time comparison between expected and actual hex strings.
func VerifyChecksum(expected, actual string) error {
e := strings.ToLower(strings.TrimSpace(expected))
a := strings.ToLower(strings.TrimSpace(actual))
if len(e) == 0 || len(a) == 0 {
return ErrChecksumMismatch
}
if subtle.ConstantTimeCompare([]byte(e), []byte(a)) != 1 {
return fmt.Errorf("%w (expected %s, got %s)", ErrChecksumMismatch, e, a)
}
return nil
}