Update golangci-lint to v2.12.2 with canonical config (closes #60)
All checks were successful
check / check (push) Successful in 35s

- Add canonical .golangci.yml (v2 schema, default: all, project
  thresholds for lll/funlen/cyclop/dupl)
- Bump golangci-lint pins from v2.0.2 to v2.12.2 in Makefile
  (go install, new /v2 module path) and Dockerfile (tagged+digest
  Debian image pin)
- Fix all lint findings surfaced by the new linter set across
  cmd/mfer, internal/bork, internal/cli, internal/log, and mfer:
  static sentinel errors (err113), context-aware HTTP and exec
  (noctx), guarded integer conversions and stricter permissions
  (gosec), named constants (mnd, goconst), function decomposition
  (funlen, cyclop, gocognit, nestif), declaration ordering
  (funcorder), t.Parallel/t.TempDir/t.Setenv adoption in tests
  (paralleltest, usetesting), protobuf getters (protogetter), plus
  formatting and style cleanups (wsl_v5, nlreturn, lll, revive,
  testifylint, and others)
- Serialize CLI runs in tests behind a mutex so parallel tests do
  not cross-wire the process-global logger's captured output

The decompositions are behavior-preserving. In particular:

- REPO_POLICIES.md is untouched and stays byte-identical to the
  authoritative copy in the prompts repo
- the mfer.manifest type stays unexported; whether to export it is an
  open owner design question (README question 13)
- directories created by fetch keep mode 0755, because fetched trees
  are content meant to be readable by other uids
- an absent MFFilePath.Mtime is handled explicitly and identically in
  freshen, list, and export rather than being read as the Unix epoch,
  which would classify every entry as changed and rewrite the manifest
  on every freshen
- every user-visible error message renders byte-identically to what it
  did before, with the err113 sentinels wrapped mid-sentence where
  needed; the rendered strings are now pinned by tests

Also fixes an argument-injection defect the lint pass surfaced: key IDs
reach gpg as bare positional arguments, so a key ID beginning with "-"
was parsed by gpg as an option. All positional arguments now follow an
explicit "--" end-of-options marker.

The symlink-escape gap in fetch's path handling, which sanitizePath
does not and cannot address, is filed separately as #86.
This commit is contained in:
2026-08-09 02:16:37 +00:00
parent 6d19de74e7
commit 3bfbb3fbe2
40 changed files with 3999 additions and 1799 deletions

View File

@@ -2,7 +2,9 @@ package cli
import (
"bytes"
"context"
"crypto/sha256"
"errors"
"fmt"
"io"
"net/http"
@@ -20,6 +22,45 @@ import (
"sneak.berlin/go/mfer/mfer"
)
const (
// progressChanBuffer is the buffer size of the download progress
// channel.
progressChanBuffer = 10
// bitsPerByte converts a bytes-per-second rate to bits per second.
bitsPerByte = 8
// dirPerms is the permission mode for directories created for
// downloaded files. Fetched trees are content that is normally
// published (served by a web server, read by another uid), so the
// traversal bit for group and other must stay set.
dirPerms os.FileMode = 0o755
// Bitrate unit thresholds in bits per second.
bpsPerGbps = 1e9
bpsPerMbps = 1e6
bpsPerKbps = 1e3
)
var (
// errURLRequired indicates the fetch command was run without a URL
// argument.
errURLRequired = errors.New("URL argument required")
// errEmptyPath indicates an empty file path in the manifest.
errEmptyPath = errors.New("empty path")
// errAbsolutePath indicates an absolute file path in the manifest.
errAbsolutePath = errors.New("absolute path not allowed")
// errPathTraversal indicates a manifest path escaping the target
// directory.
errPathTraversal = errors.New("path traversal not allowed")
// errSizeMismatch indicates a downloaded file with an unexpected
// size.
errSizeMismatch = errors.New("size mismatch")
// errHashMismatch indicates a downloaded file whose hash matches no
// manifest hash.
errHashMismatch = errors.New("hash mismatch")
)
// DownloadProgress reports the progress of a single file download.
type DownloadProgress struct {
Path string // File path being downloaded
@@ -29,14 +70,98 @@ type DownloadProgress struct {
ETA time.Duration // Estimated time to completion
}
// httpGet issues a GET request for the given URL using the provided
// context and returns the response. The caller must close the body.
//
// Errors are returned unwrapped: this helper replaced direct http.Get
// calls, and each caller already supplies its own context string, so
// adding one here would change user-visible messages.
func httpGet(ctx context.Context, fileURL string) (*http.Response, error) {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, fileURL, nil)
if err != nil {
return nil, err
}
resp, err := http.DefaultClient.Do(req)
if err != nil {
return nil, err
}
return resp, nil
}
// reportDownloadProgress renders download progress until the channel
// closes, then closes done.
func reportDownloadProgress(progress <-chan DownloadProgress, done chan<- struct{}) {
defer close(done)
for p := range progress {
rate := formatBitrate(p.BytesPerSec * bitsPerByte)
if p.ETA > 0 {
log.Infof("%s: %s/%s, %s, ETA %s",
p.Path, humanize.IBytes(safeUint64(p.BytesRead)),
humanize.IBytes(safeUint64(p.TotalBytes)),
rate, p.ETA.Round(time.Second))
} else {
log.Infof("%s: %s/%s, %s",
p.Path, humanize.IBytes(safeUint64(p.BytesRead)),
humanize.IBytes(safeUint64(p.TotalBytes)), rate)
}
}
}
// manifestBaseURL returns the URL of the directory containing the
// manifest, with a trailing slash.
func manifestBaseURL(manifestURL string) (*url.URL, error) {
baseURL, err := url.Parse(manifestURL)
if err != nil {
return nil, fmt.Errorf("fetch: invalid manifest URL: %w", err)
}
baseURL.Path = path.Dir(baseURL.Path)
if !strings.HasSuffix(baseURL.Path, "/") {
baseURL.Path += "/"
}
return baseURL, nil
}
// downloadManifestFiles downloads every file in the manifest, reporting
// progress on the progress channel.
func downloadManifestFiles(
ctx context.Context,
baseURL *url.URL,
files []*mfer.MFFilePath,
progress chan<- DownloadProgress,
) error {
for _, f := range files {
// Sanitize the path to prevent path traversal attacks
localPath, err := sanitizePath(f.GetPath())
if err != nil {
return fmt.Errorf("invalid path in manifest: %w", err)
}
fileURL := baseURL.String() + encodeFilePath(f.GetPath())
log.Infof("fetching %s", f.GetPath())
err = downloadFile(ctx, fileURL, localPath, f, progress)
if err != nil {
return fmt.Errorf("failed to download %s: %w", f.GetPath(), err)
}
}
return nil
}
func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error {
log.Debug("fetchManifestOperation()")
if ctx.Args().Len() == 0 {
return fmt.Errorf("URL argument required")
return errURLRequired
}
inputURL := ctx.Args().Get(0)
manifestURL, err := resolveManifestURL(inputURL)
if err != nil {
return fmt.Errorf("invalid URL: %w", err)
@@ -45,14 +170,16 @@ func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error {
log.Infof("fetching manifest from %s", manifestURL)
// Fetch manifest
resp, err := http.Get(manifestURL)
resp, err := httpGet(ctx.Context, manifestURL)
if err != nil {
return fmt.Errorf("failed to fetch manifest: %w", err)
}
defer func() { _ = resp.Body.Close() }()
if resp.StatusCode != http.StatusOK {
return fmt.Errorf("failed to fetch manifest: HTTP %d", resp.StatusCode)
return fmt.Errorf("failed to fetch manifest: %w %d",
errHTTPStatus, resp.StatusCode)
}
// Parse manifest
@@ -65,74 +192,43 @@ func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error {
log.Infof("manifest contains %d files", len(files))
// Compute base URL (directory containing manifest)
baseURL, err := url.Parse(manifestURL)
baseURL, err := manifestBaseURL(manifestURL)
if err != nil {
return fmt.Errorf("fetch: invalid manifest URL: %w", err)
}
baseURL.Path = path.Dir(baseURL.Path)
if !strings.HasSuffix(baseURL.Path, "/") {
baseURL.Path += "/"
return err
}
// Calculate total bytes to download
var totalBytes int64
for _, f := range files {
totalBytes += f.Size
totalBytes += f.GetSize()
}
// Create progress channel
progress := make(chan DownloadProgress, 10)
// Start progress reporter goroutine
// Create progress channel and start progress reporter goroutine
progress := make(chan DownloadProgress, progressChanBuffer)
done := make(chan struct{})
go func() {
defer close(done)
for p := range progress {
rate := formatBitrate(p.BytesPerSec * 8)
if p.ETA > 0 {
log.Infof("%s: %s/%s, %s, ETA %s",
p.Path, humanize.IBytes(uint64(p.BytesRead)), humanize.IBytes(uint64(p.TotalBytes)),
rate, p.ETA.Round(time.Second))
} else {
log.Infof("%s: %s/%s, %s",
p.Path, humanize.IBytes(uint64(p.BytesRead)), humanize.IBytes(uint64(p.TotalBytes)), rate)
}
}
}()
go reportDownloadProgress(progress, done)
// Track download start time
startTime := time.Now()
// Download each file
for _, f := range files {
// Sanitize the path to prevent path traversal attacks
localPath, err := sanitizePath(f.Path)
if err != nil {
close(progress)
<-done
return fmt.Errorf("invalid path in manifest: %w", err)
}
fileURL := baseURL.String() + encodeFilePath(f.Path)
log.Infof("fetching %s", f.Path)
if err := downloadFile(fileURL, localPath, f, progress); err != nil {
close(progress)
<-done
return fmt.Errorf("failed to download %s: %w", f.Path, err)
}
}
dlErr := downloadManifestFiles(ctx.Context, baseURL, files, progress)
close(progress)
<-done
if dlErr != nil {
return dlErr
}
// Print summary
elapsed := time.Since(startTime)
avgBytesPerSec := float64(totalBytes) / elapsed.Seconds()
avgRate := formatBitrate(avgBytesPerSec * 8)
avgRate := formatBitrate(avgBytesPerSec * bitsPerByte)
log.Infof("downloaded %d files (%s) in %.1fs (%s avg)",
len(files),
humanize.IBytes(uint64(totalBytes)),
humanize.IBytes(safeUint64(totalBytes)),
elapsed.Seconds(),
avgRate)
@@ -145,6 +241,7 @@ func encodeFilePath(p string) string {
for i, seg := range segments {
segments[i] = url.PathEscape(seg)
}
return strings.Join(segments, "/")
}
@@ -153,12 +250,12 @@ func encodeFilePath(p string) string {
func sanitizePath(p string) (string, error) {
// Reject empty paths
if p == "" {
return "", fmt.Errorf("empty path")
return "", errEmptyPath
}
// Reject absolute paths
if filepath.IsAbs(p) {
return "", fmt.Errorf("absolute path not allowed: %s", p)
return "", fmt.Errorf("%w: %s", errAbsolutePath, p)
}
// Clean the path to resolve . and ..
@@ -166,12 +263,12 @@ func sanitizePath(p string) (string, error) {
// Reject paths that escape the current directory
if strings.HasPrefix(cleaned, ".."+string(filepath.Separator)) || cleaned == ".." {
return "", fmt.Errorf("path traversal not allowed: %s", p)
return "", fmt.Errorf("%w: %s", errPathTraversal, p)
}
// Also check for absolute paths after cleaning (handles edge cases)
if filepath.IsAbs(cleaned) {
return "", fmt.Errorf("absolute path not allowed: %s", p)
return "", fmt.Errorf("%w: %s", errAbsolutePath, p)
}
return cleaned, nil
@@ -183,7 +280,7 @@ func sanitizePath(p string) (string, error) {
func resolveManifestURL(inputURL string) (string, error) {
parsed, err := url.Parse(inputURL)
if err != nil {
return "", err
return "", fmt.Errorf("failed to parse URL: %w", err)
}
// Check if URL already ends with .mf
@@ -214,10 +311,14 @@ type progressWriter struct {
func (pw *progressWriter) Write(p []byte) (int, error) {
n, err := pw.w.Write(p)
pw.written += int64(n)
if pw.progress != nil {
var bytesPerSec float64
var eta time.Duration
var (
bytesPerSec float64
eta time.Duration
)
elapsed := time.Since(pw.startTime)
if elapsed > 0 && pw.written > 0 {
bytesPerSec = float64(pw.written) / elapsed.Seconds()
@@ -226,6 +327,7 @@ func (pw *progressWriter) Write(p []byte) (int, error) {
eta = time.Duration(float64(remainingBytes)/bytesPerSec) * time.Second
}
}
sendProgress(pw.progress, DownloadProgress{
Path: pw.path,
BytesRead: pw.written,
@@ -234,18 +336,19 @@ func (pw *progressWriter) Write(p []byte) (int, error) {
ETA: eta,
})
}
return n, err
}
// formatBitrate formats a bits-per-second value with appropriate unit prefix.
func formatBitrate(bps float64) string {
switch {
case bps >= 1e9:
return fmt.Sprintf("%.1f Gbps", bps/1e9)
case bps >= 1e6:
return fmt.Sprintf("%.1f Mbps", bps/1e6)
case bps >= 1e3:
return fmt.Sprintf("%.1f Kbps", bps/1e3)
case bps >= bpsPerGbps:
return fmt.Sprintf("%.1f Gbps", bps/bpsPerGbps)
case bps >= bpsPerMbps:
return fmt.Sprintf("%.1f Mbps", bps/bpsPerMbps)
case bps >= bpsPerKbps:
return fmt.Sprintf("%.1f Kbps", bps/bpsPerKbps)
default:
return fmt.Sprintf("%.0f bps", bps)
}
@@ -259,53 +362,100 @@ func sendProgress(ch chan<- DownloadProgress, p DownloadProgress) {
}
}
// downloadFile downloads a URL to a local file path with hash verification.
// It downloads to a temporary file, verifies the hash, then renames to the final path.
// Progress is reported via the progress channel.
func downloadFile(fileURL, localPath string, entry *mfer.MFFilePath, progress chan<- DownloadProgress) error {
// Create parent directories if needed
// tempPathFor computes the temporary download path for a local file.
// For dotfiles, just append .tmp (they're already hidden); for regular
// files, prefix with . and append .tmp.
func tempPathFor(localPath string) string {
dir := filepath.Dir(localPath)
if dir != "" && dir != "." {
if err := os.MkdirAll(dir, 0o755); err != nil {
return fmt.Errorf("failed to create directory %s: %w", dir, err)
}
}
// Compute temp file path in the same directory
// For dotfiles, just append .tmp (they're already hidden)
// For regular files, prefix with . and append .tmp
base := filepath.Base(localPath)
var tmpName string
if strings.HasPrefix(base, ".") {
tmpName = base + ".tmp"
} else {
tmpName = "." + base + ".tmp"
}
tmpPath := filepath.Join(dir, tmpName)
if dir == "" || dir == "." {
tmpPath = tmpName
return tmpName
}
return filepath.Join(dir, tmpName)
}
// verifyDownloadedHash checks the computed sha256 digest against the
// manifest entry's hashes; at least one must match.
func verifyDownloadedHash(digest []byte, entry *mfer.MFFilePath) error {
computed, err := multihash.Encode(digest, multihash.SHA2_256)
if err != nil {
return fmt.Errorf("failed to encode hash: %w", err)
}
for _, hash := range entry.GetHashes() {
if bytes.Equal(computed, hash.GetMultiHash()) {
return nil
}
}
return errHashMismatch
}
// downloadFile downloads a URL to a local file path with hash verification.
// It downloads to a temporary file, verifies the hash, then renames to the final path.
// Progress is reported via the progress channel.
func downloadFile(
ctx context.Context,
fileURL, localPath string,
entry *mfer.MFFilePath,
progress chan<- DownloadProgress,
) error {
// Enforce the path invariant here rather than relying on the caller,
// so every entry point to downloadFile gets the same treatment.
localPath, err := sanitizePath(localPath)
if err != nil {
return fmt.Errorf("invalid path: %w", err)
}
// Create parent directories if needed
dir := filepath.Dir(localPath)
if dir != "" && dir != "." {
err := os.MkdirAll(dir, dirPerms)
if err != nil {
return fmt.Errorf("failed to create directory %s: %w", dir, err)
}
}
tmpPath := tempPathFor(localPath)
// Fetch file
resp, err := http.Get(fileURL) //nolint:gosec // URL constructed from manifest base
resp, err := httpGet(ctx, fileURL)
if err != nil {
return fmt.Errorf("HTTP request failed: %w", err)
}
defer func() { _ = resp.Body.Close() }()
if resp.StatusCode != http.StatusOK {
return fmt.Errorf("HTTP %d", resp.StatusCode)
return fmt.Errorf("%w %d", errHTTPStatus, resp.StatusCode)
}
// Determine expected size
expectedSize := entry.Size
expectedSize := entry.GetSize()
totalBytes := resp.ContentLength
if totalBytes < 0 {
totalBytes = expectedSize
}
// Create temp file
out, err := os.Create(tmpPath)
// Create temp file.
//
// G304: tmpPath is derived from localPath, which sanitizePath above
// constrains lexically to a relative path that does not escape the
// destination directory. That is a purely lexical guarantee: it does
// not resolve symlinks, so a pre-existing symlink inside the
// destination tree can still redirect this write outside of it
// (tracked in issue #86).
out, err := os.Create(tmpPath) //nolint:gosec // G304: see comment above
if err != nil {
return fmt.Errorf("failed to create temp file: %w", err)
}
@@ -328,45 +478,50 @@ func downloadFile(fileURL, localPath string, entry *mfer.MFFilePath, progress ch
// Close file before checking errors (to flush writes)
closeErr := out.Close()
// If copy failed, clean up temp file and return error
if copyErr != nil {
err = finishDownload(
tmpPath, localPath, written, expectedSize, h.Sum(nil), entry,
copyErr, closeErr)
if err != nil {
_ = os.Remove(tmpPath)
return err
}
return nil
}
// finishDownload validates the copy result, verifies size and hash, and
// moves the temp file into place. On error the caller removes tmpPath.
func finishDownload(
tmpPath, localPath string,
written, expectedSize int64,
digest []byte,
entry *mfer.MFFilePath,
copyErr, closeErr error,
) error {
if copyErr != nil {
return copyErr
}
if closeErr != nil {
_ = os.Remove(tmpPath)
return closeErr
}
// Verify size
if written != expectedSize {
_ = os.Remove(tmpPath)
return fmt.Errorf("size mismatch: expected %d bytes, got %d", expectedSize, written)
}
// Encode computed hash as multihash
computed, err := multihash.Encode(h.Sum(nil), multihash.SHA2_256)
if err != nil {
_ = os.Remove(tmpPath)
return fmt.Errorf("failed to encode hash: %w", err)
return fmt.Errorf("%w: expected %d bytes, got %d",
errSizeMismatch, expectedSize, written)
}
// Verify hash against manifest (at least one must match)
hashMatch := false
for _, hash := range entry.Hashes {
if bytes.Equal(computed, hash.MultiHash) {
hashMatch = true
break
}
}
if !hashMatch {
_ = os.Remove(tmpPath)
return fmt.Errorf("hash mismatch")
err := verifyDownloadedHash(digest, entry)
if err != nil {
return err
}
// Rename temp file to final path
if err := os.Rename(tmpPath, localPath); err != nil {
_ = os.Remove(tmpPath)
err = os.Rename(tmpPath, localPath)
if err != nil {
return fmt.Errorf("failed to rename temp file: %w", err)
}