mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-07-13 15:07:37 +00:00
Duy /zuey/
2cbf838158
* 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>