mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-09-09 12:17:31 +00:00
* 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>
96 lines
3.2 KiB
Go
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
|
|
}
|