Harden extraction, attestation, and install orchestration#2
Merged
Conversation
Sustainability code review follow-through. Security, CI, and maintainability fixes across the extension. Security (archive extraction): - Reject tar symlinks whose target escapes the destination dir. - Replace the weak HasPrefix path guard with a separator-aware SafeJoin (new internal/fsutil package, shared with tool). - Enforce extraction resource limits (max total bytes, max entries). - Fix .txz/.tlz decompressed-name mapping. - Add adversarial extraction tests (traversal, symlink escape, oversized/too-many-entry archives). Attestation: - Distinguish "no attestation published" (warn, continue) from a genuine verification failure (an attestation exists but does not verify). Add --require-attestation to make real failures abort. - Add --no-verify and --require-attestation to upgrade for parity. Install orchestration: - Resolve each tool's target tag once and thread it into Install, eliminating the double `gh release view` per tool in both install-reconcile and upgrade. Resolve reconcile tags in parallel. - Split isUpToDate into a pure upToDate comparison plus a resolveTargetTag helper; preserve failure semantics. Maintainability / cleanup: - Introduce an overridable ghExec seam so the latest-tag and attestation paths are unit-testable; add tests for both. - Unify command registration in each command's init(). - Remove the unused, undocumented config.Settings struct. - Quote interpolated paths in emitted zsh/fish shell integration. - Replace reinvented helpers and dead code; Walk -> WalkDir. CI: - Add an OS matrix (linux/macos/windows), go vet, -race, a gofmt gate, and staticcheck to the test workflow. Docs: - Document the new attestation flags in docs/commands.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
gh-tool is a Unix-targeted CLI: installs are symlink-based, paths follow the XDG layout, and shell integration covers only bash/zsh/fish. The windows-latest matrix entry surfaced pre-existing Unix-only test assumptions (forward-slash paths, exec bits, root-path handling) for a runtime the tool does not actually support. Test on Linux and macOS, the platforms gh-tool runs on, both with -race. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CodeQL's go/zipslip query flagged the tar and zip extractors because it does not recognize the cross-package fsutil.SafeJoin helper as a sanitizing barrier, so the archive entry name appeared to flow unchecked into filesystem operations. The code was already safe, but the guard was not legible to the scanner (or to a reader of the extractor). Co-locate the canonical strings.HasPrefix-against-cleaned-destination check directly with the file operations it protects, so the barrier dominates the sink. Drop the now-unused SafeJoin and keep WithinDir as the shared boundary primitive (still used by the symlink-escape check and tool.pathWithin); add a WithinDir unit test. Behavior is unchanged: the adversarial extraction tests (traversal and symlink escape rejection) still pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CodeQL's go/zipslip query recognizes strings.HasPrefix(path, ...) as a path-traversal sanitizer only on the branch where it returns true. The previous guard folded the root-equality case into the condition (target != cleanDest && !HasPrefix(...)), which left a path to the file operations where HasPrefix was false, so CodeQL could not prove the barrier dominated the sinks. Skip the archive-root entry with a separate continue and rely solely on the HasPrefix containment check, so reaching any extraction sink strictly requires the check to pass. Behavior is unchanged; adversarial tests still pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sustainability code review of
gh-toolwith the resulting fixes implemented. Security, CI, and maintainability improvements across the extension.All checks green locally:
go build,go vet,staticcheck,gofmt, andgo test -race ./.... Coverage improved (archive 75.9%→82.4%, tool 56.3%→60.1%).Security (archive extraction — highest priority)
strings.HasPrefixcheck with a separator-awareSafeJoinin a new sharedinternal/fsutilpackage (also used bytool)..txz/.tlzbug: fixed the decompressed-name mapping.Attestation
--require-attestationto make real failures abort, plus--no-verify/--require-attestationparity onupgrade.Install orchestration
gh release viewper tool: the target tag is resolved once (in parallel for reconcile) and threaded intoInstallin both install-reconcile and upgrade.isUpToDateinto a pure, unit-testedupToDatecomparison plus aresolveTargetTaghelper, preserving failure semantics.Maintainability
ghExecseam ininternal/tool, making the latest-tag and attestation-verification paths unit-testable (new tests). The broader injectable gh-client interface was intentionally deferred to avoid an awkward Manager-only half-measure.init().config.Settingsstruct.filepath.Walk→filepath.WalkDir.CI
test.yml: OS matrix (linux/macos/windows),go vet,-race, agofmtgate, andstaticcheck.Docs
docs/commands.md.🤖 Generated with Copilot CLI