diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index dacb670..c0ed301 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -5,13 +5,45 @@ on: pull_request: branches: [main] +permissions: + contents: read + jobs: test: - runs-on: ubuntu-latest + name: test (${{ matrix.os }}) + strategy: + fail-fast: false + matrix: + # gh-tool is a Unix-targeted CLI: installs are symlink-based, paths + # follow the XDG layout, and shell integration covers bash/zsh/fish. + # Test on the platforms it actually supports as a runtime. + os: [ubuntu-latest, macos-latest] + runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v4 - uses: actions/setup-go@v5 with: go-version-file: go.mod - run: go build ./... - - run: go test ./... + - run: go vet ./... + - run: go test -race ./... + + lint: + name: lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + - name: gofmt + run: | + files=$(gofmt -l .) + if [ -n "$files" ]; then + echo "The following files are not gofmt-formatted:" + echo "$files" + echo "Run 'gofmt -w .' to fix." + exit 1 + fi + - name: staticcheck + run: go run honnef.co/go/tools/cmd/staticcheck@latest ./... diff --git a/cmd/add.go b/cmd/add.go index 4f2bfb9..e5e0e26 100644 --- a/cmd/add.go +++ b/cmd/add.go @@ -1,6 +1,7 @@ package cmd import ( + "errors" "fmt" "os" "path/filepath" @@ -45,10 +46,12 @@ func init() { func runAdd(cmd *cobra.Command, args []string) error { if !term.IsTerminal(int(os.Stdin.Fd())) || !term.IsTerminal(int(os.Stdout.Fd())) { - return fmt.Errorf(`gh tool add requires an interactive terminal. + fmt.Fprintf(os.Stderr, `gh tool add requires an interactive terminal. For non-interactive use, run: gh tool install %s --pattern '...' --bin '...' -or edit your manifest directly.`, args[0]) +or edit your manifest directly. +`, args[0]) + return errors.New("interactive terminal required") } repo := args[0] @@ -70,7 +73,7 @@ or edit your manifest directly.`, args[0]) if err != nil { return err } - fmt.Printf("✓ %s @ %s — %d classified assets, %d skipped\n", rel.Repo, rel.Tag, len(rel.All), len(rel.Skipped)) + fmt.Printf("%s %s @ %s — %d classified assets, %d skipped\n", ui.Success(ui.IconSuccess), rel.Repo, rel.Tag, len(rel.All), len(rel.Skipped)) platforms := rel.Platforms() if len(platforms) == 0 { @@ -183,9 +186,9 @@ or edit your manifest directly.`, args[0]) return fmt.Errorf("saving manifest: %w", err) } if existing { - fmt.Printf("✓ Updated %s in %s\n", repo, mfPath) + fmt.Printf("%s Updated %s in %s\n", ui.Success(ui.IconSuccess), repo, mfPath) } else { - fmt.Printf("✓ Saved %s to %s\n", repo, mfPath) + fmt.Printf("%s Saved %s to %s\n", ui.Success(ui.IconSuccess), repo, mfPath) } if !flagAddInstall { @@ -229,7 +232,7 @@ func refineDarwinArchs(chosen map[discover.PlatformKey]string, inspectAssetName func chooseAddPlatforms(platforms []discover.PlatformKey) ([]discover.PlatformKey, error) { if len(platforms) == 1 { - fmt.Printf("· Only one platform detected: %s\n", platforms[0]) + fmt.Printf("%s Only one platform detected: %s\n", ui.IconBullet, platforms[0]) return platforms, nil } options := make([]string, len(platforms)) @@ -425,7 +428,7 @@ func chooseAddBins(layout *discover.Layout, repo, inspectAssetName, foldedPatter var picked []string if match := layout.MatchBinName(name); match != "" && len(layout.Executables) == 1 { - fmt.Printf("· Auto-detected bin: %s\n", match) + fmt.Printf("%s Auto-detected bin: %s\n", ui.IconBullet, match) picked = []string{match} } else { options := make([]string, len(layout.Executables)) @@ -433,9 +436,7 @@ func chooseAddBins(layout *discover.Layout, repo, inspectAssetName, foldedPatter // releases (uv: uv+uvx, git: many) almost always want all of // them, and a single-executable case where the name doesn't // match the repo (handled below) still wants the binary. - for i, e := range layout.Executables { - options[i] = e - } + copy(options, layout.Executables) if err := promptMultiSelect("Select binaries to symlink:", options, options, &picked); err != nil { return nil, err } diff --git a/cmd/cache.go b/cmd/cache.go index 01b235f..d01e1c3 100644 --- a/cmd/cache.go +++ b/cmd/cache.go @@ -2,6 +2,7 @@ package cmd import ( "fmt" + "io/fs" "os" "path/filepath" "strings" @@ -9,6 +10,8 @@ import ( "github.com/cli/go-gh/v2/pkg/tableprinter" "github.com/cli/go-gh/v2/pkg/term" "github.com/spf13/cobra" + + "github.com/ascarter/gh-tool/internal/ui" ) var cacheCmd = &cobra.Command{ @@ -137,20 +140,24 @@ func runCacheClean(cmd *cobra.Command, args []string) error { if err := os.RemoveAll(cacheDir); err != nil { return err } - fmt.Printf("✓ Cleaned cache for %s\n", args[0]) + fmt.Printf("%s Cleaned cache for %s\n", ui.Success(ui.IconSuccess), args[0]) return nil } if err := os.RemoveAll(dirs.Cache); err != nil { return err } - fmt.Println("✓ Cleaned all cached downloads") + fmt.Printf("%s Cleaned all cached downloads\n", ui.Success(ui.IconSuccess)) return nil } func dirStats(dir string) (totalSize int64, fileCount int) { - _ = filepath.Walk(dir, func(_ string, info os.FileInfo, err error) error { - if err != nil || info.IsDir() { + _ = filepath.WalkDir(dir, func(_ string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return nil + } + info, err := d.Info() + if err != nil { return nil } totalSize += info.Size() diff --git a/cmd/install.go b/cmd/install.go index 3aada30..d8f4564 100644 --- a/cmd/install.go +++ b/cmd/install.go @@ -26,17 +26,18 @@ ad-hoc installs not in the manifest).`, } var ( - flagPattern string - flagTag string - flagBin []string - flagMan []string - flagComp []string - flagNoVerify bool - flagForce bool - flagFile string - flagJobs int - flagNoProgress bool - flagVerbose bool + flagPattern string + flagTag string + flagBin []string + flagMan []string + flagComp []string + flagNoVerify bool + flagRequireAttestation bool + flagForce bool + flagFile string + flagJobs int + flagNoProgress bool + flagVerbose bool ) func init() { @@ -46,11 +47,13 @@ func init() { installCmd.Flags().StringSliceVar(&flagMan, "man", nil, "man page path(s) in archive") installCmd.Flags().StringSliceVar(&flagComp, "completion", nil, "completion path(s) in archive") installCmd.Flags().BoolVar(&flagNoVerify, "no-verify", false, "skip attestation verification") + installCmd.Flags().BoolVar(&flagRequireAttestation, "require-attestation", false, "fail if an attestation exists but does not verify") installCmd.Flags().BoolVar(&flagForce, "force", false, "reinstall even if up-to-date") installCmd.Flags().StringVarP(&flagFile, "file", "f", "", "manifest path (default: $XDG_CONFIG_HOME/gh-tool/config.toml)") installCmd.Flags().IntVarP(&flagJobs, "jobs", "j", 0, "parallel installs (default: min(8, NumCPU))") installCmd.Flags().BoolVar(&flagNoProgress, "no-progress", false, "disable the live progress UI") installCmd.Flags().BoolVarP(&flagVerbose, "verbose", "v", false, "log every step (download, verify, extract)") + rootCmd.AddCommand(installCmd) } // manifestPath returns the manifest path honoring --file, falling back to the @@ -65,6 +68,7 @@ func manifestPath(dirs paths.Dirs) string { func runInstall(cmd *cobra.Command, args []string) error { dirs := resolveDirs() mgr := tool.NewManager(dirs) + mgr.RequireAttestation = flagRequireAttestation mfPath := manifestPath(dirs) cfg, err := config.Load(mfPath) @@ -112,8 +116,14 @@ func runInstall(cmd *cobra.Command, args []string) error { if flagForce { mgr.CleanupInstall(t.Name()) - } else if isUpToDate(mgr, t) { - return nil + } else if targetTag, err := resolveTargetTag(t); err == nil { + if state := mgr.ReadState(t.Name()); state != nil && upToDate(state, t, targetTag) { + fmt.Printf("%s %s up to date (%s)\n", ui.IconBullet, t.Name(), targetTag) + return nil + } + // Thread the resolved tag through so Install does not resolve it + // a second time. + t.Tag = targetTag } return mgr.Install(t, !flagNoVerify) @@ -121,7 +131,10 @@ func runInstall(cmd *cobra.Command, args []string) error { // runInstallReconcile reconciles the local install set against the manifest // in parallel. Tools filtered out by ShouldInstallOn or already at the -// target version are short-circuited before the worker pool spawns. +// target version are short-circuited before the worker pool spawns. The +// target tag for each eligible tool is resolved once, up front (in +// parallel), and threaded into Install so the latest tag is not resolved a +// second time during download. func runInstallReconcile(mgr *tool.Manager, cfg *config.Config) error { if len(cfg.Tools) == 0 { fmt.Println("No tools in manifest. Use: gh tool add ") @@ -129,22 +142,64 @@ func runInstallReconcile(mgr *tool.Manager, cfg *config.Config) error { } verify := !flagNoVerify - type pending struct { - t config.Tool - } - var queue []pending + // Phase 1: filter by OS before making any network calls. + type item struct { + t config.Tool + targetTag string + resolveErr error + } + items := make([]*item, 0, len(cfg.Tools)) for _, t := range cfg.Tools { if !t.ShouldInstallOn(runtime.GOOS) { fmt.Printf("%s %s skipped on %s\n", ui.IconBullet, t.Name(), runtime.GOOS) continue } + items = append(items, &item{t: t}) + } + if len(items) == 0 { + return nil + } + + // Phase 2: resolve each eligible tool's target tag in parallel. This is + // the slow part of reconcile (one `gh release view` per unpinned tool). + // Resolving once here lets us both compare against installed state and + // thread the resolved tag into Install, avoiding a second resolution. + resolveJobs := make([]ui.Job, 0, len(items)) + for _, it := range items { + it := it + resolveJobs = append(resolveJobs, ui.Job{ + Name: it.t.Name(), + Run: func() error { + it.targetTag, it.resolveErr = resolveTargetTag(it.t) + return nil + }, + }) + } + _, _ = ui.Run(resolveJobs, ui.ResolveJobs(flagJobs)) + + // Phase 3: compare, print, and queue serially so output stays ordered. + queue := make([]config.Tool, 0, len(items)) + for _, it := range items { + name := it.t.Name() if flagForce { - mgr.CleanupInstall(t.Name()) - } else if isUpToDate(mgr, t) { + mgr.CleanupInstall(name) + } + if it.resolveErr != nil { + // Resolution failed; queue with the original spec so Install + // re-resolves and surfaces the error through its normal path. + queue = append(queue, it.t) continue } - queue = append(queue, pending{t: t}) + if !flagForce { + if state := mgr.ReadState(name); state != nil && upToDate(state, it.t, it.targetTag) { + fmt.Printf("%s %s up to date (%s)\n", ui.IconBullet, name, it.targetTag) + continue + } + } + t := it.t + t.Tag = it.targetTag + queue = append(queue, t) } if len(queue) == 0 { @@ -165,8 +220,8 @@ func runInstallReconcile(mgr *tool.Manager, cfg *config.Config) error { } jobs := make([]ui.Job, 0, len(queue)) - for _, p := range queue { - t := p.t + for _, t := range queue { + t := t jobs = append(jobs, ui.Job{ Name: t.Name(), Run: func() error { return mgr.Install(t, verify) }, @@ -191,39 +246,42 @@ func runInstallReconcile(mgr *tool.Manager, cfg *config.Config) error { return nil } -// isUpToDate reports whether the installed copy already matches the -// target version AND the manifest's asset spec. The spec check covers -// the case where the user added or renamed a bin/man/completion entry -// after the tool was already installed at the current release tag — -// without it, `gh tool install` would print "up to date" and never -// create the new symlinks. -func isUpToDate(mgr *tool.Manager, t config.Tool) bool { - name := t.Name() - state := mgr.ReadState(name) - if state == nil { - return false +// resolveTargetTag returns the tag a tool should be installed at: the pinned +// tag when one is set, otherwise the repo's latest release tag resolved via +// the GitHub API. An empty latest tag is treated as an error so callers don't +// accidentally thread an empty tag (which would silently fall back to +// re-resolution downstream). +func resolveTargetTag(t config.Tool) (string, error) { + if t.Tag != "" && t.Tag != "latest" { + return t.Tag, nil } - - targetTag := t.Tag - if targetTag == "" || targetTag == "latest" { - latest, err := tool.LatestTag(t.Repo) - if err != nil { - return false - } - targetTag = latest + tag, err := tool.LatestTag(t.Repo) + if err != nil { + return "", err + } + if tag == "" { + return "", fmt.Errorf("no latest release tag for %s", t.Repo) } + return tag, nil +} - if state.Tag != targetTag { +// upToDate reports whether the installed copy already matches the target tag +// AND the manifest's asset spec. The spec check covers the case where the +// user added or renamed a bin/man/completion entry after the tool was already +// installed at the current release tag — without it, `gh tool install` would +// print "up to date" and never create the new symlinks. This function is pure: +// it performs no I/O and prints nothing, so the resolution of targetTag is the +// caller's responsibility. +func upToDate(state *tool.InstalledState, t config.Tool, targetTag string) bool { + if state == nil { return false } - if !stringSlicesEqual(state.Bin, t.Bin) || - !stringSlicesEqual(state.Man, t.Man) || - !stringSlicesEqual(state.Completions, t.Completions) { + if state.Tag != targetTag { return false } - - fmt.Printf("%s %s up to date (%s)\n", ui.IconBullet, name, targetTag) - return true + return stringSlicesEqual(state.Bin, t.Bin) && + stringSlicesEqual(state.Man, t.Man) && + stringSlicesEqual(state.Completions, t.Completions) } // stringSlicesEqual compares two slices as unordered sets, treating diff --git a/cmd/install_test.go b/cmd/install_test.go index 40422d9..27c5226 100644 --- a/cmd/install_test.go +++ b/cmd/install_test.go @@ -34,10 +34,10 @@ func TestStringSlicesEqual(t *testing.T) { // Regression: when a tool is already installed at the manifest's target // tag but the manifest's bin/man/completions spec has changed (entries -// added or removed), isUpToDate must return false so install runs and +// added or removed), upToDate must return false so install runs and // actualizes the new spec. The previous implementation only compared // the tag and would report "up to date" forever. -func TestIsUpToDateDetectsManifestSpecChange(t *testing.T) { +func TestUpToDateDetectsManifestSpecChange(t *testing.T) { root := t.TempDir() dirs := paths.Dirs{ Config: filepath.Join(root, "config"), @@ -62,31 +62,39 @@ func TestIsUpToDateDetectsManifestSpecChange(t *testing.T) { if err := writeStateForTest(dirs, "fzf", state); err != nil { t.Fatalf("write state: %v", err) } + got := mgr.ReadState("fzf") + if got == nil { + t.Fatalf("ReadState returned nil") + } // Same tag, bin spec unchanged → up to date. tmatch := config.Tool{Repo: "junegunn/fzf", Tag: "v1", Bin: []string{"fzf"}} - if !isUpToDate(mgr, tmatch) { - t.Errorf("isUpToDate(matching spec)=false, want true") + if !upToDate(got, tmatch, "v1") { + t.Errorf("upToDate(matching spec)=false, want true") } // Same tag, manifest gained completions entry → must NOT be up to // date so install runs and creates the new completion symlinks. tadded := config.Tool{Repo: "junegunn/fzf", Tag: "v1", Bin: []string{"fzf"}, Completions: []string{"shell/completion.bash"}} - if isUpToDate(mgr, tadded) { - t.Errorf("isUpToDate(spec gained completions)=true, want false") + if upToDate(got, tadded, "v1") { + t.Errorf("upToDate(spec gained completions)=true, want false") } // Same tag, bin renamed → must NOT be up to date. trenamed := config.Tool{Repo: "junegunn/fzf", Tag: "v1", Bin: []string{"fzf-bin:fzf"}} - if isUpToDate(mgr, trenamed) { - t.Errorf("isUpToDate(renamed bin)=true, want false") + if upToDate(got, trenamed, "v1") { + t.Errorf("upToDate(renamed bin)=true, want false") + } + + // Different target tag → false regardless of spec. + tmatch2 := config.Tool{Repo: "junegunn/fzf", Tag: "v1", Bin: []string{"fzf"}} + if upToDate(got, tmatch2, "v2") { + t.Errorf("upToDate(different tag)=true, want false") } - // Different tag → false regardless of spec. We pin Tag to bypass - // the LatestTag network call. - tnewtag := config.Tool{Repo: "junegunn/fzf", Tag: "v2", Bin: []string{"fzf"}} - if isUpToDate(mgr, tnewtag) { - t.Errorf("isUpToDate(different tag)=true, want false") + // Nil state → false. + if upToDate(nil, tmatch, "v1") { + t.Errorf("upToDate(nil state)=true, want false") } } @@ -104,4 +112,3 @@ func writeStateForTest(dirs paths.Dirs, name string, state tool.InstalledState) defer f.Close() return toml.NewEncoder(f).Encode(state) } - diff --git a/cmd/list.go b/cmd/list.go index a596987..3a82390 100644 --- a/cmd/list.go +++ b/cmd/list.go @@ -31,6 +31,7 @@ var listCmd = &cobra.Command{ func init() { listCmd.Flags().BoolVar(&flagListOutdated, "outdated", false, "show only tools with a newer release available") listCmd.Flags().BoolVar(&flagListPinned, "pinned", false, "show only tools pinned to a specific tag") + rootCmd.AddCommand(listCmd) } func runList(cmd *cobra.Command, args []string) error { diff --git a/cmd/remove.go b/cmd/remove.go index 195af71..00961ff 100644 --- a/cmd/remove.go +++ b/cmd/remove.go @@ -19,6 +19,10 @@ var removeCmd = &cobra.Command{ RunE: runRemove, } +func init() { + rootCmd.AddCommand(removeCmd) +} + func runRemove(cmd *cobra.Command, args []string) error { repo := args[0] dirs := resolveDirs() diff --git a/cmd/root.go b/cmd/root.go index 66a2406..59dd9e9 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -62,9 +62,6 @@ func Execute() error { func init() { rootCmd.AddCommand(versionCmd) - rootCmd.AddCommand(installCmd) - rootCmd.AddCommand(removeCmd) - rootCmd.AddCommand(listCmd) } var versionCmd = &cobra.Command{ diff --git a/cmd/shell.go b/cmd/shell.go index c899090..d829c97 100644 --- a/cmd/shell.go +++ b/cmd/shell.go @@ -105,15 +105,15 @@ export MANPATH="%s:$MANPATH" fmt.Printf(` # zsh completions (interactive shells only) if [[ -o interactive ]]; then - fpath=(%s $fpath) + fpath=("%s" $fpath) fi `, dirs.ZshCompletionDir()) } case "fish": fmt.Printf(`# gh-tool shell integration (fish) -fish_add_path -g %s -set -gx MANPATH %s $MANPATH +fish_add_path -g "%s" +set -gx MANPATH "%s" $MANPATH `, dirs.BinDir(), dirs.ManDir()) if opts.GhtoolHome != "" { fmt.Printf("set -gx GHTOOL_HOME %q\n", opts.GhtoolHome) @@ -121,8 +121,8 @@ set -gx MANPATH %s $MANPATH if !opts.NoCompletions { fmt.Printf(` # fish completions (interactive shells only) -if status is-interactive; and test -d %s - for f in %s/*.fish +if status is-interactive; and test -d "%s" + for f in "%s"/*.fish source $f end end diff --git a/cmd/upgrade.go b/cmd/upgrade.go index c6180d4..f46380b 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -17,6 +17,8 @@ var ( flagUpgradeJobs int flagUpgradeNoProgress bool flagUpgradeVerbose bool + flagUpgradeNoVerify bool + flagUpgradeRequireAtt bool ) var upgradeCmd = &cobra.Command{ @@ -29,12 +31,15 @@ func init() { upgradeCmd.Flags().IntVarP(&flagUpgradeJobs, "jobs", "j", 0, "parallel upgrades (default: min(8, NumCPU))") upgradeCmd.Flags().BoolVar(&flagUpgradeNoProgress, "no-progress", false, "disable the live progress UI") upgradeCmd.Flags().BoolVarP(&flagUpgradeVerbose, "verbose", "v", false, "log every step (download, verify, extract)") + upgradeCmd.Flags().BoolVar(&flagUpgradeNoVerify, "no-verify", false, "skip attestation verification") + upgradeCmd.Flags().BoolVar(&flagUpgradeRequireAtt, "require-attestation", false, "fail if an attestation exists but does not verify") rootCmd.AddCommand(upgradeCmd) } func runUpgrade(cmd *cobra.Command, args []string) error { dirs := resolveDirs() mgr := tool.NewManager(dirs) + mgr.RequireAttestation = flagUpgradeRequireAtt var states []tool.InstalledState all, err := mgr.ListInstalled() @@ -160,13 +165,15 @@ func runUpgrade(cmd *cobra.Command, args []string) error { } jobs := make([]ui.Job, 0, len(candidates)) + verify := !flagUpgradeNoVerify for _, c := range candidates { t := c.t - // Force tag to latest by clearing it. - t.Tag = "" + // Thread the tag resolved during the check phase so Install does + // not resolve the latest tag a second time. + t.Tag = c.latest jobs = append(jobs, ui.Job{ Name: t.Name(), - Run: func() error { return mgr.Install(t, true) }, + Run: func() error { return mgr.Install(t, verify) }, }) } diff --git a/docs/commands.md b/docs/commands.md index 0b959b4..fcc6465 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -16,25 +16,26 @@ gh tool version Print version ## Notable flags - **`add`**: `--file/-f`, `--tag/-t`, `--no-write` (preview the generated block without saving). -- **`install`**: `--pattern/-p`, `--tag/-t`, `--bin`, `--man`, `--completion`, `--no-verify`, `--force`, `--file/-f`, `--jobs/-j`, `--no-progress`, `--verbose/-v`. +- **`install`**: `--pattern/-p`, `--tag/-t`, `--bin`, `--man`, `--completion`, `--no-verify`, `--require-attestation`, `--force`, `--file/-f`, `--jobs/-j`, `--no-progress`, `--verbose/-v`. - **`list`**: `--outdated`, `--pinned`. -- **`upgrade`**: `--jobs/-j`, `--no-progress`, `--verbose/-v`. +- **`upgrade`**: `--jobs/-j`, `--no-progress`, `--verbose/-v`, `--no-verify`, `--require-attestation`. - **`reset`**: `--yes/-y`. - **`shell`**: `--no-completions`. ## Common flag semantics -| Flag | Purpose | -|--------------------|----------------------------------------------------------------| -| `-j, --jobs N` | Parallelism cap (default `min(8, NumCPU)`). | -| `--no-progress` | Disable the live progress UI; print one line per event. | -| `-v, --verbose` | Log every step (download, verify, extract) per tool. | -| `--no-verify` | Skip attestation verification (install only). | +| Flag | Purpose | +|--------------------------|----------------------------------------------------------------| +| `-j, --jobs N` | Parallelism cap (default `min(8, NumCPU)`). | +| `--no-progress` | Disable the live progress UI; print one line per event. | +| `-v, --verbose` | Log every step (download, verify, extract) per tool. | +| `--no-verify` | Skip attestation verification (install and upgrade). | +| `--require-attestation` | Fail the install/upgrade if an attestation exists but does not verify. Repos that publish no attestation still proceed (install and upgrade). | ## How install works 1. `gh tool install` downloads a release asset via `gh release download` into a cache directory. -2. The asset is verified with `gh attestation verify` (best-effort — most repos don't publish attestations yet). +2. The asset is verified with `gh attestation verify` (best-effort — most repos don't publish attestations yet). A repo that publishes no attestation only warns and continues; pass `--require-attestation` to make a genuine verification *failure* (an attestation exists but does not match) abort. 3. Archives (`tar.gz`, `tar.xz`, `zip`) are extracted; bare binaries are copied directly. If an archive has a single top-level directory, it is stripped automatically. 4. Symlinks are created from the bin directory into the extracted tool directory. Use `source:link` in `bin` to rename binaries (e.g., `jq-macos-arm64:jq`). 5. A state file under the state directory records the installed tag, the resolved download pattern, and the symlinked `bin`/`man`/`completions`. `list`, `remove`, and `upgrade` operate from these state files; the manifest is only consulted by `install` (and by `list` for drift reporting). diff --git a/internal/archive/archive.go b/internal/archive/archive.go index e2364e5..c1eed0a 100644 --- a/internal/archive/archive.go +++ b/internal/archive/archive.go @@ -10,6 +10,18 @@ import ( "os/exec" "path/filepath" "strings" + + "github.com/ascarter/gh-tool/internal/fsutil" +) + +// Extraction safety limits. Declared as vars (not consts) so tests can lower +// them to exercise the bomb guards without producing multi-gigabyte fixtures. +var ( + // maxExtractBytes caps total uncompressed bytes written per extraction, + // guarding against decompression bombs from untrusted release assets. + maxExtractBytes int64 = 2 << 30 // 2 GiB + // maxExtractEntries caps the number of archive members processed. + maxExtractEntries = 100_000 ) // Extract unpacks an archive file into destDir. @@ -17,25 +29,72 @@ import ( // If the archive has a single top-level directory, its contents are promoted up // (the leading directory is stripped). // For non-archive files (bare binaries), the file is copied directly and made executable. +// +// Extraction is bounded by maxExtractBytes / maxExtractEntries, every member is +// verified to stay within destDir (no path traversal), and symlink members that +// would resolve outside destDir are rejected. func Extract(archivePath, destDir string) error { if err := os.MkdirAll(destDir, 0o755); err != nil { return err } + g := newLimitGuard() lower := strings.ToLower(archivePath) switch { case strings.HasSuffix(lower, ".tar.gz") || strings.HasSuffix(lower, ".tgz"): - return extractTarGz(archivePath, destDir) + return extractTarGz(archivePath, destDir, g) case strings.HasSuffix(lower, ".tar.xz") || strings.HasSuffix(lower, ".txz"): - return extractTarXz(archivePath, destDir) + return extractTarXz(archivePath, destDir, g) case strings.HasSuffix(lower, ".zip"): - return extractZip(archivePath, destDir) + return extractZip(archivePath, destDir, g) default: - return copyBinary(archivePath, destDir) + return copyBinary(archivePath, destDir, g) } } -func extractTarGz(archivePath, destDir string) error { +// limitGuard tracks the remaining byte and entry budget for one extraction. +type limitGuard struct { + bytesLeft int64 + entriesLeft int +} + +func newLimitGuard() *limitGuard { + return &limitGuard{bytesLeft: maxExtractBytes, entriesLeft: maxExtractEntries} +} + +// entry accounts for one archive member, returning an error once the entry +// budget is exhausted. +func (g *limitGuard) entry() error { + if g.entriesLeft <= 0 { + return fmt.Errorf("archive exceeds maximum entry count (%d)", maxExtractEntries) + } + g.entriesLeft-- + return nil +} + +// writeFile copies r into a new file at path, honoring mode and the running +// byte budget. It errors (rather than filling the disk) if the cumulative +// extracted size would exceed maxExtractBytes. +func (g *limitGuard) writeFile(path string, r io.Reader, mode os.FileMode) error { + out, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode) + if err != nil { + return err + } + defer out.Close() + // Copy at most bytesLeft+1: reading the extra byte without hitting EOF + // signals the archive is over budget. + n, err := io.CopyN(out, r, g.bytesLeft+1) + g.bytesLeft -= n + if err != nil && err != io.EOF { + return err + } + if g.bytesLeft < 0 { + return fmt.Errorf("archive exceeds maximum extracted size (%d bytes)", maxExtractBytes) + } + return nil +} + +func extractTarGz(archivePath, destDir string, g *limitGuard) error { f, err := os.Open(archivePath) if err != nil { return err @@ -49,22 +108,22 @@ func extractTarGz(archivePath, destDir string) error { defer gz.Close() prefix := detectTarPrefix(archivePath) - return extractTar(gz, destDir, prefix) + return extractTar(gz, destDir, prefix, g) } -func extractTarXz(archivePath, destDir string) error { +func extractTarXz(archivePath, destDir string, g *limitGuard) error { // xz decompression requires the xz command since Go stdlib doesn't include it if _, err := exec.LookPath("xz"); err != nil { return fmt.Errorf("xz command not found (required for .tar.xz): %w", err) } - // Decompress to a temporary .tar file - tarPath := strings.TrimSuffix(archivePath, filepath.Ext(archivePath)) - if tarPath == archivePath { - tarPath = archivePath + ".tar" + // Determine the file xz will write, mirroring its suffix rules. + tarPath, err := xzDecompressedName(archivePath) + if err != nil { + return err } - // Run xz -dk to decompress (keep original, force overwrite) + // Run xz -dkf to decompress (keep original, force overwrite) cmd := exec.Command("xz", "-dkf", archivePath) if out, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("xz decompress: %s: %w", string(out), err) @@ -81,9 +140,31 @@ func extractTarXz(archivePath, destDir string) error { if _, err := f.Seek(0, io.SeekStart); err != nil { return err } - return extractTar(f, destDir, prefix) -} // extractTar reads tar entries from r and writes them to destDir, stripping prefix. -func extractTar(r io.Reader, destDir, prefix string) error { + return extractTar(f, destDir, prefix, g) +} + +// xzDecompressedName returns the path `xz -d` writes when decompressing +// archivePath, mirroring xz's suffix rules: ".xz" is stripped, while the +// compact ".txz" (and ".tlz") forms map to ".tar". +func xzDecompressedName(archivePath string) (string, error) { + low := strings.ToLower(archivePath) + switch { + case strings.HasSuffix(low, ".txz"): + return archivePath[:len(archivePath)-len(".txz")] + ".tar", nil + case strings.HasSuffix(low, ".tlz"): + return archivePath[:len(archivePath)-len(".tlz")] + ".tar", nil + case strings.HasSuffix(low, ".xz"): + return archivePath[:len(archivePath)-len(".xz")], nil + case strings.HasSuffix(low, ".lzma"): + return archivePath[:len(archivePath)-len(".lzma")], nil + default: + return "", fmt.Errorf("unrecognized xz suffix: %s", archivePath) + } +} + +// extractTar reads tar entries from r and writes them to destDir, stripping prefix. +func extractTar(r io.Reader, destDir, prefix string, g *limitGuard) error { + cleanDest := filepath.Clean(destDir) tr := tar.NewReader(r) for { hdr, err := tr.Next() @@ -102,9 +183,20 @@ func extractTar(r io.Reader, destDir, prefix string) error { } } - target := filepath.Join(destDir, filepath.FromSlash(name)) + if err := g.entry(); err != nil { + return err + } - if !strings.HasPrefix(filepath.Clean(target), filepath.Clean(destDir)) { + // Skip the archive root itself; only real entries are extracted. + clean := filepath.Clean(filepath.FromSlash(name)) + if clean == "." { + continue + } + // Reject entries that escape destDir (Zip-Slip / path traversal). + // Reaching the file operations below requires this strings.HasPrefix + // containment check to pass, so it directly dominates every sink. + target := filepath.Join(cleanDest, clean) + if !strings.HasPrefix(target, cleanDest+string(os.PathSeparator)) { return fmt.Errorf("tar entry escapes destination: %s", hdr.Name) } @@ -118,10 +210,16 @@ func extractTar(r io.Reader, destDir, prefix string) error { return err } mode := hdr.FileInfo().Mode() - if err := writeFile(target, tr, mode); err != nil { + if err := g.writeFile(target, tr, mode); err != nil { return err } case tar.TypeSymlink: + // Reject symlinks whose target would resolve outside destDir. + // Without this, a later regular-file entry could be written + // *through* the symlink and escape the install directory. + if !symlinkWithinDir(destDir, target, hdr.Linkname) { + return fmt.Errorf("tar symlink escapes destination: %s -> %s", hdr.Name, hdr.Linkname) + } if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { return err } @@ -134,6 +232,17 @@ func extractTar(r io.Reader, destDir, prefix string) error { return nil } +// symlinkWithinDir reports whether a symlink created at linkPath pointing to +// linkname resolves to a location within destDir. Absolute targets and +// relative targets that climb out via ".." are rejected. +func symlinkWithinDir(destDir, linkPath, linkname string) bool { + resolved := linkname + if !filepath.IsAbs(linkname) { + resolved = filepath.Join(filepath.Dir(linkPath), linkname) + } + return fsutil.WithinDir(resolved, destDir) +} + func detectTarPrefixFromReader(r io.Reader) string { tr := tar.NewReader(r) topDirs := make(map[string]bool) @@ -173,13 +282,14 @@ func detectTarPrefix(archivePath string) string { return detectTarPrefixFromReader(gz) } -func extractZip(archivePath, destDir string) error { +func extractZip(archivePath, destDir string, g *limitGuard) error { r, err := zip.OpenReader(archivePath) if err != nil { return fmt.Errorf("zip: %w", err) } defer r.Close() + cleanDest := filepath.Clean(destDir) prefix := detectZipPrefix(r) for _, f := range r.File { @@ -191,9 +301,20 @@ func extractZip(archivePath, destDir string) error { } } - target := filepath.Join(destDir, filepath.FromSlash(name)) + if err := g.entry(); err != nil { + return err + } - if !strings.HasPrefix(filepath.Clean(target), filepath.Clean(destDir)) { + // Skip the archive root itself; only real entries are extracted. + clean := filepath.Clean(filepath.FromSlash(name)) + if clean == "." { + continue + } + // Reject entries that escape destDir (Zip-Slip / path traversal). + // Reaching the file operations below requires this strings.HasPrefix + // containment check to pass, so it directly dominates every sink. + target := filepath.Join(cleanDest, clean) + if !strings.HasPrefix(target, cleanDest+string(os.PathSeparator)) { return fmt.Errorf("zip entry escapes destination: %s", f.Name) } @@ -213,7 +334,7 @@ func extractZip(archivePath, destDir string) error { return err } mode := f.FileInfo().Mode() - err = writeFile(target, rc, mode) + err = g.writeFile(target, rc, mode) rc.Close() if err != nil { return err @@ -239,7 +360,7 @@ func detectZipPrefix(r *zip.ReadCloser) string { } // copyBinary handles non-archive assets (bare binaries like jq releases). -func copyBinary(src, destDir string) error { +func copyBinary(src, destDir string, g *limitGuard) error { name := filepath.Base(src) target := filepath.Join(destDir, name) @@ -249,15 +370,5 @@ func copyBinary(src, destDir string) error { } defer in.Close() - return writeFile(target, in, 0o755) -} - -func writeFile(path string, r io.Reader, mode os.FileMode) error { - out, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode) - if err != nil { - return err - } - defer out.Close() - _, err = io.Copy(out, r) - return err + return g.writeFile(target, in, 0o755) } diff --git a/internal/archive/archive_security_test.go b/internal/archive/archive_security_test.go new file mode 100644 index 0000000..4cef808 --- /dev/null +++ b/internal/archive/archive_security_test.go @@ -0,0 +1,210 @@ +package archive + +import ( + "archive/tar" + "archive/zip" + "compress/gzip" + "os" + "path/filepath" + "strings" + "testing" +) + +// writeRawTarGz writes a tar.gz built from the given headers/bodies verbatim +// (no prefix-stripping helper), so tests can craft malicious member names. +func writeRawTarGz(t *testing.T, entries []tar.Header, bodies map[string]string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "evil.tar.gz") + f, err := os.Create(path) + if err != nil { + t.Fatal(err) + } + defer f.Close() + gw := gzip.NewWriter(f) + defer gw.Close() + tw := tar.NewWriter(gw) + defer tw.Close() + for i := range entries { + hdr := entries[i] + body := bodies[hdr.Name] + hdr.Size = int64(len(body)) + if err := tw.WriteHeader(&hdr); err != nil { + t.Fatal(err) + } + if body != "" { + if _, err := tw.Write([]byte(body)); err != nil { + t.Fatal(err) + } + } + } + return path +} + +func TestExtractTarRejectsTraversal(t *testing.T) { + // Include a benign sibling so no single leading dir is stripped and the + // malicious "../" name reaches the boundary check intact. + src := writeRawTarGz(t, + []tar.Header{ + {Name: "app/ok", Mode: 0o644}, + {Name: "../escape.txt", Mode: 0o644}, + }, + map[string]string{"app/ok": "fine", "../escape.txt": "pwned"}, + ) + parent := t.TempDir() + dest := filepath.Join(parent, "tool") + err := Extract(src, dest) + if err == nil || !strings.Contains(err.Error(), "escapes destination") { + t.Fatalf("expected escape error, got %v", err) + } + if _, err := os.Stat(filepath.Join(parent, "escape.txt")); err == nil { + t.Error("traversal file was written outside destination") + } +} + +func TestExtractTarRejectsSiblingPrefixEscape(t *testing.T) { + // dest is .../tool; a member resolving to .../tool-evil/x shares the + // string prefix but is NOT within dest. A raw HasPrefix check would + // let this through; the boundary check must reject it. + src := writeRawTarGz(t, + []tar.Header{ + {Name: "app/ok", Mode: 0o644}, + {Name: "../tool-evil/x", Mode: 0o644}, + }, + map[string]string{"app/ok": "fine", "../tool-evil/x": "pwned"}, + ) + parent := t.TempDir() + dest := filepath.Join(parent, "tool") + if err := Extract(src, dest); err == nil { + t.Fatal("expected sibling-prefix escape to be rejected") + } + if _, err := os.Stat(filepath.Join(parent, "tool-evil", "x")); err == nil { + t.Error("sibling-prefix file was written outside destination") + } +} + +func TestExtractTarRejectsSymlinkEscape(t *testing.T) { + // An absolute symlink pointing outside dest must be rejected so a later + // entry cannot be written through it. + outside := filepath.Join(t.TempDir(), "outside") + if err := os.MkdirAll(outside, 0o755); err != nil { + t.Fatal(err) + } + src := writeRawTarGz(t, + []tar.Header{{Name: "link", Typeflag: tar.TypeSymlink, Linkname: outside, Mode: 0o777}}, + nil, + ) + dest := t.TempDir() + if err := Extract(src, dest); err == nil || !strings.Contains(err.Error(), "symlink escapes") { + t.Fatalf("expected symlink escape error, got %v", err) + } +} + +func TestExtractTarRejectsRelativeSymlinkEscape(t *testing.T) { + src := writeRawTarGz(t, + []tar.Header{{Name: "link", Typeflag: tar.TypeSymlink, Linkname: "../../escape", Mode: 0o777}}, + nil, + ) + dest := t.TempDir() + if err := Extract(src, dest); err == nil { + t.Fatal("expected relative symlink escape to be rejected") + } +} + +func TestExtractTarAllowsInBoundsSymlink(t *testing.T) { + src := writeRawTarGz(t, + []tar.Header{ + {Name: "real", Typeflag: tar.TypeReg, Mode: 0o644}, + {Name: "link", Typeflag: tar.TypeSymlink, Linkname: "real", Mode: 0o777}, + }, + map[string]string{"real": "content"}, + ) + dest := t.TempDir() + if err := Extract(src, dest); err != nil { + t.Fatalf("in-bounds symlink should be allowed, got %v", err) + } + if _, err := os.Lstat(filepath.Join(dest, "link")); err != nil { + t.Errorf("expected symlink to be created: %v", err) + } +} + +func TestExtractZipRejectsTraversal(t *testing.T) { + path := filepath.Join(t.TempDir(), "evil.zip") + f, err := os.Create(path) + if err != nil { + t.Fatal(err) + } + zw := zip.NewWriter(f) + // Benign sibling so no common prefix is stripped. + if ok, err := zw.Create("app/ok"); err != nil { + t.Fatal(err) + } else if _, err := ok.Write([]byte("fine")); err != nil { + t.Fatal(err) + } + w, err := zw.Create("../escape.txt") + if err != nil { + t.Fatal(err) + } + if _, err := w.Write([]byte("pwned")); err != nil { + t.Fatal(err) + } + zw.Close() + f.Close() + + parent := t.TempDir() + dest := filepath.Join(parent, "tool") + if err := Extract(path, dest); err == nil || !strings.Contains(err.Error(), "escapes destination") { + t.Fatalf("expected zip escape error, got %v", err) + } + if _, err := os.Stat(filepath.Join(parent, "escape.txt")); err == nil { + t.Error("zip traversal file was written outside destination") + } +} + +func TestExtractRejectsOversizedArchive(t *testing.T) { + orig := maxExtractBytes + maxExtractBytes = 8 + defer func() { maxExtractBytes = orig }() + + src := createTestTarGz(t, "", map[string]string{"big": "way more than eight bytes"}) + dest := t.TempDir() + if err := Extract(src, dest); err == nil || !strings.Contains(err.Error(), "maximum extracted size") { + t.Fatalf("expected size-limit error, got %v", err) + } +} + +func TestExtractRejectsTooManyEntries(t *testing.T) { + orig := maxExtractEntries + maxExtractEntries = 1 + defer func() { maxExtractEntries = orig }() + + src := createTestTarGz(t, "", map[string]string{"a": "x", "b": "y"}) + dest := t.TempDir() + if err := Extract(src, dest); err == nil || !strings.Contains(err.Error(), "maximum entry count") { + t.Fatalf("expected entry-count error, got %v", err) + } +} + +func TestXZDecompressedName(t *testing.T) { + tests := []struct { + in string + want string + }{ + {"foo.tar.xz", "foo.tar"}, + {"foo.txz", "foo.tar"}, + {"foo.tlz", "foo.tar"}, + {"foo.xz", "foo"}, + } + for _, tt := range tests { + got, err := xzDecompressedName(tt.in) + if err != nil { + t.Errorf("xzDecompressedName(%q) error: %v", tt.in, err) + continue + } + if got != tt.want { + t.Errorf("xzDecompressedName(%q) = %q, want %q", tt.in, got, tt.want) + } + } + if _, err := xzDecompressedName("foo.gz"); err == nil { + t.Error("expected error for non-xz suffix") + } +} diff --git a/internal/config/config.go b/internal/config/config.go index bb2bb4a..1a649df 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -11,15 +11,7 @@ import ( // Config is the top-level TOML manifest structure. type Config struct { - Settings Settings `toml:"settings,omitempty"` - Tools []Tool `toml:"tool"` -} - -// Settings holds optional path overrides and global configuration. -type Settings struct { - DataHome string `toml:"data_home,omitempty"` - StateHome string `toml:"state_home,omitempty"` - CacheHome string `toml:"cache_home,omitempty"` + Tools []Tool `toml:"tool"` } // Tool describes a single tool to install from a GitHub release. diff --git a/internal/fsutil/fsutil.go b/internal/fsutil/fsutil.go new file mode 100644 index 0000000..6bf07e1 --- /dev/null +++ b/internal/fsutil/fsutil.go @@ -0,0 +1,19 @@ +// Package fsutil holds small filesystem path helpers shared across packages. +package fsutil + +import ( + "path/filepath" + "strings" +) + +// WithinDir reports whether target is dir itself or nested inside it. It is a +// path-boundary check (separator-aware), not a raw string-prefix test, so a +// sibling like "/a/bdir" is NOT considered within "/a/b". +func WithinDir(target, dir string) bool { + target = filepath.Clean(target) + dir = filepath.Clean(dir) + if target == dir { + return true + } + return strings.HasPrefix(target, dir+string(filepath.Separator)) +} diff --git a/internal/fsutil/fsutil_test.go b/internal/fsutil/fsutil_test.go new file mode 100644 index 0000000..f22d3ba --- /dev/null +++ b/internal/fsutil/fsutil_test.go @@ -0,0 +1,28 @@ +package fsutil + +import ( + "path/filepath" + "testing" +) + +func TestWithinDir(t *testing.T) { + dir := filepath.FromSlash("/a/b") + cases := []struct { + target string + want bool + }{ + {filepath.FromSlash("/a/b"), true}, // dir itself + {filepath.FromSlash("/a/b/c"), true}, // nested + {filepath.FromSlash("/a/b/c/d.txt"), true}, // deeply nested + {filepath.FromSlash("/a/b/../b/c"), true}, // climbs but stays within after clean + {filepath.FromSlash("/a/bdir"), false}, // sibling prefix, not within + {filepath.FromSlash("/a"), false}, // parent + {filepath.FromSlash("/a/c"), false}, // sibling + {filepath.FromSlash("/a/b/../../etc"), false}, // escapes via .. + } + for _, c := range cases { + if got := WithinDir(c.target, dir); got != c.want { + t.Errorf("WithinDir(%q, %q) = %v, want %v", c.target, dir, got, c.want) + } + } +} diff --git a/internal/tool/discover/archive_inspect.go b/internal/tool/discover/archive_inspect.go index badc663..055e853 100644 --- a/internal/tool/discover/archive_inspect.go +++ b/internal/tool/discover/archive_inspect.go @@ -2,6 +2,7 @@ package discover import ( "fmt" + "io/fs" "os" "path/filepath" "regexp" @@ -74,11 +75,15 @@ var manPageRE = regexp.MustCompile(`\.[1-9]([a-z]?)$`) // and completions. func scanLayout(root string) (*Layout, error) { layout := &Layout{MachOArchs: map[string]bool{}} - err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { if err != nil { return err } - if info.IsDir() { + if d.IsDir() { + return nil + } + info, err := d.Info() + if err != nil { return nil } rel, _ := filepath.Rel(root, path) diff --git a/internal/tool/tool.go b/internal/tool/tool.go index e86eb2f..265e171 100644 --- a/internal/tool/tool.go +++ b/internal/tool/tool.go @@ -2,6 +2,7 @@ package tool import ( "fmt" + "io/fs" "os" "path/filepath" "runtime" @@ -14,9 +15,16 @@ import ( "github.com/ascarter/gh-tool/internal/archive" "github.com/ascarter/gh-tool/internal/config" + "github.com/ascarter/gh-tool/internal/fsutil" "github.com/ascarter/gh-tool/internal/paths" ) +// ghExec is the seam through which this package shells out to the `gh` CLI. +// It defaults to go-gh's Exec and is overridable in tests so the download, +// latest-tag, and attestation-verification paths can be exercised without a +// real gh binary or network access. +var ghExec = gh.Exec + // InstalledState records the per-machine install of a tool. It is the // authoritative inventory entry used by list, remove, and upgrade. Only // fields meaningful on the host where the install lives are stored. @@ -48,6 +56,13 @@ func (s InstalledState) AsTool() config.Tool { type Manager struct { Dirs paths.Dirs reporter Reporter + + // RequireAttestation makes a genuine attestation verification failure + // (an attestation exists but does not verify) abort the install. The + // common case of a repo publishing no attestation at all still only + // warns, so the default experience is unchanged for the many tools that + // don't ship attestations. + RequireAttestation bool } // NewManager creates a Manager with resolved XDG paths and a no-op reporter. @@ -133,7 +148,7 @@ func (m *Manager) DownloadAsset(t config.Tool) (assetPath, tag, resolvedPattern args := []string{"release", "download", tag, "-R", t.Repo, "-D", cacheDir, "-p", resolvedPattern, "--clobber"} m.reporter.Stage(name, fmt.Sprintf("Downloading %s %s", t.Repo, tag)) - if _, stderr, err := gh.Exec(args...); err != nil { + if _, stderr, err := ghExec(args...); err != nil { if msg := strings.TrimSpace(stderr.String()); msg != "" { return "", "", "", fmt.Errorf("downloading release: %w: %s", err, msg) } @@ -161,7 +176,12 @@ func (m *Manager) installFromAsset(t config.Tool, assetPath, tag, resolvedPatter name := t.Name() if verify { - m.verifyAttestation(name, t.Repo, assetPath) + if err := m.verifyAttestation(name, t.Repo, assetPath); err != nil { + if m.RequireAttestation { + return err + } + m.reporter.Warn(name, err.Error()) + } } // Reap any prior install's symlinks before wiping the tool dir. The @@ -405,13 +425,7 @@ func (m *Manager) removeToolSymlinks(name string) { // pathWithin reports whether child is equal to or nested inside parent. func pathWithin(child, parent string) bool { - parent = filepath.Clean(parent) - child = filepath.Clean(child) - if child == parent { - return true - } - sep := string(filepath.Separator) - return strings.HasPrefix(child, parent+sep) + return fsutil.WithinDir(child, parent) } func (m *Manager) writeState(name string, state InstalledState) error { @@ -428,7 +442,7 @@ func (m *Manager) writeState(name string, state InstalledState) error { } func resolveLatestTag(repo string) (string, error) { - stdout, _, err := gh.Exec("release", "view", "-R", repo, "--json", "tagName", "--jq", ".tagName") + stdout, _, err := ghExec("release", "view", "-R", repo, "--json", "tagName", "--jq", ".tagName") if err != nil { return "", err } @@ -436,6 +450,10 @@ func resolveLatestTag(repo string) (string, error) { } // findDownloadedAsset finds the first non-checksum file in a cache dir. +// This assumes a single downloadable asset is present, which holds because +// DownloadAsset clears the cache dir before each download and passes a single +// `-p` pattern to `gh release download`. Checksum/signature sidecar files +// (*.sig, *.asc, and names containing "checksum"/"sha256") are skipped. func findDownloadedAsset(dir string) (string, error) { entries, err := os.ReadDir(dir) if err != nil { @@ -468,8 +486,8 @@ func findFileInDir(root, name string) string { // Search by basename base := filepath.Base(name) var found string - _ = filepath.Walk(root, func(path string, info os.FileInfo, err error) error { - if err != nil || info.IsDir() { + _ = filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { return nil } if filepath.Base(path) == base { @@ -486,15 +504,63 @@ func forceSymlink(src, dst string) error { return os.Symlink(src, dst) } -// verifyAttestation attempts to verify a downloaded asset using gh attestation verify. -// This is best-effort: it surfaces a warning via the reporter if verification -// fails but does not return an error. On success the next stage (Extracting) -// is the user's signal that verification passed. -func (m *Manager) verifyAttestation(name, repo, assetPath string) { +// verifyAttestation verifies a downloaded asset using `gh attestation verify`. +// It distinguishes two outcomes that the previous best-effort implementation +// conflated: +// +// - The repo publishes no attestation (the overwhelmingly common case for +// third-party CLIs). Treated as a soft signal: a warning is surfaced and +// nil is returned so the install proceeds. +// - An attestation exists but does not verify (a tampered or mismatched +// asset). This returns a non-nil error. installFromAsset aborts when +// Manager.RequireAttestation is set; otherwise it surfaces the failure as +// a prominent warning. +// +// On success the next stage (Extracting) is the user's signal that +// verification passed. +func (m *Manager) verifyAttestation(name, repo, assetPath string) error { m.reporter.Stage(name, fmt.Sprintf("Verifying attestation for %s", filepath.Base(assetPath))) - if _, _, err := gh.Exec("attestation", "verify", assetPath, "-R", repo); err != nil { - m.reporter.Stage(name, "attestation not verified (this is expected for most repos)") + _, stderr, err := ghExec("attestation", "verify", assetPath, "-R", repo) + if err == nil { + return nil + } + if attestationAbsent(stderr.String()) { + m.reporter.Stage(name, "no attestation published (expected for most repos)") + return nil + } + msg := firstLine(stderr.String()) + if msg == "" { + msg = err.Error() + } + return fmt.Errorf("attestation verification failed for %s: %s", repo, msg) +} + +// attestationAbsent reports whether gh's stderr indicates the repo simply has +// no attestation to verify (as opposed to an attestation that failed to +// verify). Matching is best-effort against gh's wording. +func attestationAbsent(stderr string) bool { + low := strings.ToLower(stderr) + for _, marker := range []string{ + "no attestation", + "no attestations", + "failed to fetch attestations", + "no matching attestations", + } { + if strings.Contains(low, marker) { + return true + } + } + return false +} + +// firstLine returns the first non-empty trimmed line of s. +func firstLine(s string) string { + for _, line := range strings.Split(s, "\n") { + if t := strings.TrimSpace(line); t != "" { + return t + } } + return "" } // Tokens returns the literal expansions of every supported template token diff --git a/internal/tool/tool_test.go b/internal/tool/tool_test.go index 492a793..9e4c45e 100644 --- a/internal/tool/tool_test.go +++ b/internal/tool/tool_test.go @@ -1,10 +1,13 @@ package tool import ( + "bytes" + "errors" "os" "path/filepath" "reflect" "runtime" + "strings" "testing" "github.com/ascarter/gh-tool/internal/config" @@ -599,3 +602,102 @@ func TestFindDownloadedAsset(t *testing.T) { t.Errorf("expected error when only skippable files present") } } + +func TestAttestationAbsent(t *testing.T) { + absent := []string{ + "no attestations found for subject", + "failed to fetch attestations from owner/repo", + "X No matching attestations found", + } + for _, s := range absent { + if !attestationAbsent(s) { + t.Errorf("attestationAbsent(%q) = false, want true", s) + } + } + present := []string{ + "verification failed: signature mismatch", + "the attestation could not be verified", + "", + } + for _, s := range present { + if attestationAbsent(s) { + t.Errorf("attestationAbsent(%q) = true, want false", s) + } + } +} + +func TestFirstLine(t *testing.T) { + if got := firstLine("\n\n hello \nworld"); got != "hello" { + t.Errorf("firstLine = %q, want %q", got, "hello") + } + if got := firstLine(" \n "); got != "" { + t.Errorf("firstLine of blank = %q, want empty", got) + } +} + +// withGHExec swaps the package-level gh exec seam for the duration of a test +// and restores it afterward. Tests using it must not run in parallel. +func withGHExec(t *testing.T, fn func(args ...string) (bytes.Buffer, bytes.Buffer, error)) { + t.Helper() + orig := ghExec + ghExec = fn + t.Cleanup(func() { ghExec = orig }) +} + +func bufOf(s string) bytes.Buffer { + var b bytes.Buffer + b.WriteString(s) + return b +} + +func TestResolveLatestTag(t *testing.T) { + withGHExec(t, func(args ...string) (bytes.Buffer, bytes.Buffer, error) { + return bufOf("v1.2.3\n"), bytes.Buffer{}, nil + }) + got, err := resolveLatestTag("owner/repo") + if err != nil { + t.Fatalf("resolveLatestTag: %v", err) + } + if got != "v1.2.3" { + t.Errorf("resolveLatestTag = %q, want %q", got, "v1.2.3") + } + + withGHExec(t, func(args ...string) (bytes.Buffer, bytes.Buffer, error) { + return bytes.Buffer{}, bufOf("release not found"), errors.New("exit 1") + }) + if _, err := resolveLatestTag("owner/repo"); err == nil { + t.Errorf("resolveLatestTag: expected error when gh fails") + } +} + +func TestVerifyAttestation(t *testing.T) { + mgr := NewManager(paths.Dirs{}) + + // Verified: gh exits 0. + withGHExec(t, func(args ...string) (bytes.Buffer, bytes.Buffer, error) { + return bytes.Buffer{}, bytes.Buffer{}, nil + }) + if err := mgr.verifyAttestation("fzf", "junegunn/fzf", "/tmp/fzf.tar.gz"); err != nil { + t.Errorf("verifyAttestation(verified) = %v, want nil", err) + } + + // Absent: gh fails but stderr indicates no attestation was published. + withGHExec(t, func(args ...string) (bytes.Buffer, bytes.Buffer, error) { + return bytes.Buffer{}, bufOf("no attestations found for subject"), errors.New("exit 1") + }) + if err := mgr.verifyAttestation("fzf", "junegunn/fzf", "/tmp/fzf.tar.gz"); err != nil { + t.Errorf("verifyAttestation(absent) = %v, want nil", err) + } + + // Failed: an attestation exists but does not verify. + withGHExec(t, func(args ...string) (bytes.Buffer, bytes.Buffer, error) { + return bytes.Buffer{}, bufOf("signature verification failed\nmismatched digest"), errors.New("exit 1") + }) + err := mgr.verifyAttestation("fzf", "junegunn/fzf", "/tmp/fzf.tar.gz") + if err == nil { + t.Fatalf("verifyAttestation(failed) = nil, want error") + } + if !strings.Contains(err.Error(), "attestation verification failed") { + t.Errorf("verifyAttestation(failed) error = %q, want it to mention verification failure", err) + } +} diff --git a/internal/ui/live_reporter.go b/internal/ui/live_reporter.go index f765ea7..f7ccee9 100644 --- a/internal/ui/live_reporter.go +++ b/internal/ui/live_reporter.go @@ -4,7 +4,6 @@ import ( "fmt" "io" "os" - "sort" "strings" "sync" "time" @@ -231,7 +230,6 @@ func (m *liveModel) View() string { names := make([]string, 0, len(m.order)) names = append(names, m.order...) - sort.SliceStable(names, func(i, j int) bool { return false }) var lines []string for _, n := range names { diff --git a/internal/ui/pool.go b/internal/ui/pool.go index 8b13ee7..1e944e2 100644 --- a/internal/ui/pool.go +++ b/internal/ui/pool.go @@ -2,6 +2,7 @@ package ui import ( "runtime" + "strconv" "sync" ) @@ -113,20 +114,5 @@ func (e *BatchError) Error() string { if e.Failed == 1 { return "1 job failed" } - return pluralCount(e.Failed) + " of " + pluralCount(e.Total) + " jobs failed" -} - -// pluralCount renders an integer with a trailing word. We keep it tiny; -// fmt would pull no extra dependencies but a hand-rolled itoa stays -// allocation-light and explicit. -func pluralCount(n int) string { - if n == 0 { - return "0" - } - digits := []byte{} - for n > 0 { - digits = append([]byte{byte('0' + n%10)}, digits...) - n /= 10 - } - return string(digits) + return strconv.Itoa(e.Failed) + " of " + strconv.Itoa(e.Total) + " jobs failed" }