a696807 fix(update): address copilot review feedback · dumber

M internal/infrastructure/updater/github.go +37 -15


@@ -297,6 +297,27 @@ func verifyChecksum(filePath, expectedHash string) error {
297	297		return nil
298	298		}
299	299
	300	+	// getExpectedChecksum fetches checksums and returns the hash for the given URL.
	301	+	func (g *GitHubDownloader) getExpectedChecksum(ctx context.Context, downloadURL string) (string, error) {
	302	+	checksums, err := g.fetchChecksums(ctx)
	303	+	if err != nil {
	304	+	return "", fmt.Errorf("failed to fetch checksums: %w", err)
	305	+	}
	306	+
	307	+	parsedURL, err := url.Parse(downloadURL)
	308	+	if err != nil {
	309	+	return "", fmt.Errorf("failed to parse download URL: %w", err)
	310	+	}
	311	+
	312	+	archiveFilename := filepath.Base(parsedURL.Path)
	313	+	expectedHash, ok := checksums[archiveFilename]
	314	+	if !ok {
	315	+	return "", fmt.Errorf("no checksum found for %s", archiveFilename)
	316	+	}
	317	+
	318	+	return expectedHash, nil
	319	+	}
	320	+
300	321		// Download fetches the update archive from the given URL.
301	322		func (g *GitHubDownloader) Download(ctx context.Context, downloadURL, destDir string) (string, error) {
302	323		log := logging.FromContext(ctx)

@@ -311,18 +332,10 @@ func (g *GitHubDownloader) Download(ctx context.Context, downloadURL, destDir st
311	332		return "", fmt.Errorf("failed to create download directory: %w", err)
312	333		}
313	334
314		-	// Fetch checksums first for verification.
315		-	checksums, err := g.fetchChecksums(ctx)
	335	+	// Fetch and look up expected checksum for this archive.
	336	+	expectedHash, err := g.getExpectedChecksum(ctx, downloadURL)
316	337		if err != nil {
317		-	return "", fmt.Errorf("failed to fetch checksums: %w", err)
318		-	}
319		-
320		-	// Extract expected filename from URL to look up checksum.
321		-	parsedURL, _ := url.Parse(downloadURL)
322		-	archiveFilename := filepath.Base(parsedURL.Path)
323		-	expectedHash, ok := checksums[archiveFilename]
324		-	if !ok {
325		-	return "", fmt.Errorf("no checksum found for %s", archiveFilename)
	338	+	return "", err
326	339		}
327	340
328	341		// Create the request.

@@ -380,10 +393,16 @@ func (g *GitHubDownloader) Download(ctx context.Context, downloadURL, destDir st
380	393		return "", fmt.Errorf("checksum verification failed: %w", err)
381	394		}
382	395
	396	+	// Safely truncate hash for logging (SHA256 is 64 chars, but be defensive).
	397	+	displayHash := expectedHash
	398	+	if len(displayHash) > 16 {
	399	+	displayHash = displayHash[:16] + "..."
	400	+	}
	401	+
383	402		log.Debug().
384	403		Int64("bytes", written).
385	404		Str("path", archivePath).
386		-	Str("checksum", expectedHash[:16]+"...").
	405	+	Str("checksum", displayHash).
387	406		Msg("download completed and verified")
388	407
389	408		return archivePath, nil

@@ -399,9 +418,12 @@ func sanitizeTarPath(name, destDir string) (string, error) {
399	418		return "", fmt.Errorf("absolute path not allowed: %s", name)
400	419		}
401	420
402		-	// Reject paths that try to escape using ..
403		-	if strings.HasPrefix(cleaned, "..") \|\| strings.Contains(cleaned, string(filepath.Separator)+"..") {
404		-	return "", fmt.Errorf("path traversal detected: %s", name)
	421	+	// Reject paths that contain ".." as a path component (not as part of filename).
	422	+	// Split by separator and check each component.
	423	+	for _, part := range strings.Split(cleaned, string(filepath.Separator)) {
	424	+	if part == ".." {
	425	+	return "", fmt.Errorf("path traversal detected: %s", name)
	426	+	}
405	427		}
406	428
407	429		// Join with destination and verify result is within destDir.

 	return nil
 }
+// getExpectedChecksum fetches checksums and returns the hash for the given URL.
+func (g *GitHubDownloader) getExpectedChecksum(ctx context.Context, downloadURL string) (string, error) {
+	checksums, err := g.fetchChecksums(ctx)
+	if err != nil {
+		return "", fmt.Errorf("failed to fetch checksums: %w", err)
+	}
+	parsedURL, err := url.Parse(downloadURL)
+	if err != nil {
+		return "", fmt.Errorf("failed to parse download URL: %w", err)
+	}
+	archiveFilename := filepath.Base(parsedURL.Path)
+	expectedHash, ok := checksums[archiveFilename]
+	if !ok {
+		return "", fmt.Errorf("no checksum found for %s", archiveFilename)
+	}
+	return expectedHash, nil
+}
 // Download fetches the update archive from the given URL.
 func (g *GitHubDownloader) Download(ctx context.Context, downloadURL, destDir string) (string, error) {
 	log := logging.FromContext(ctx)
 		return "", fmt.Errorf("failed to create download directory: %w", err)
 	}
-	// Fetch checksums first for verification.
+	// Fetch and look up expected checksum for this archive.
-	checksums, err := g.fetchChecksums(ctx)
+	expectedHash, err := g.getExpectedChecksum(ctx, downloadURL)
 	if err != nil {
-		return "", fmt.Errorf("failed to fetch checksums: %w", err)
+		return "", err
-	}
-	// Extract expected filename from URL to look up checksum.
-	parsedURL, _ := url.Parse(downloadURL)
-	archiveFilename := filepath.Base(parsedURL.Path)
-	expectedHash, ok := checksums[archiveFilename]
-	if !ok {
-		return "", fmt.Errorf("no checksum found for %s", archiveFilename)
 	}
 	// Create the request.
 		return "", fmt.Errorf("checksum verification failed: %w", err)
 	}
+	// Safely truncate hash for logging (SHA256 is 64 chars, but be defensive).
+	displayHash := expectedHash
+	if len(displayHash) > 16 {
+		displayHash = displayHash[:16] + "..."
+	}
 	log.Debug().
 		Int64("bytes", written).
 		Str("path", archivePath).
-		Str("checksum", expectedHash[:16]+"...").
+		Str("checksum", displayHash).
 		Msg("download completed and verified")
 	return archivePath, nil
 		return "", fmt.Errorf("absolute path not allowed: %s", name)
 	}
-	// Reject paths that try to escape using ..
+	// Reject paths that contain ".." as a path component (not as part of filename).
-	if strings.HasPrefix(cleaned, "..") || strings.Contains(cleaned, string(filepath.Separator)+"..") {
+	// Split by separator and check each component.
-		return "", fmt.Errorf("path traversal detected: %s", name)
+	for _, part := range strings.Split(cleaned, string(filepath.Separator)) {
+		if part == ".." {
+			return "", fmt.Errorf("path traversal detected: %s", name)
+		}
 	}
 	// Join with destination and verify result is within destDir.

M internal/infrastructure/updater/github_test.go +6 -15


@@ -5,6 +5,7 @@ import (
5	5		"encoding/hex"
6	6		"os"
7	7		"path/filepath"
	8	+	"strings"
8	9		"testing"
9	10		)
10	11

@@ -208,10 +209,10 @@ func TestSanitizeTarPath(t *testing.T) {
208	209		errMsg: "path traversal",
209	210		},
210	211		{
211		-	name: "double dot at start",
212		-	path: "..dumber",
213		-	wantErr: true, // Starts with ".." so rejected as path traversal
214		-	errMsg: "path traversal",
	212	+	name: "double dot at start of filename",
	213	+	path: "..dumber",
	214	+	wantErr: false, // Valid filename, not a path traversal (.. is not a path component)
	215	+	wantPath: "/tmp/extract/..dumber",
215	216		},
216	217		{
217	218		name: "absolute path unix",

@@ -356,15 +357,5 @@ func TestSizeLimits(t *testing.T) {
356	357
357	358		// contains checks if substr is in s (helper for error message checks).
358	359		func contains(s, substr string) bool {
359		-	return len(s) >= len(substr) && (s == substr \|\| substr == "" \|\|
360		-	(s != "" && substr != "" && findSubstring(s, substr)))
361		-	}
362		-
363		-	func findSubstring(s, substr string) bool {
364		-	for i := 0; i <= len(s)-len(substr); i++ {
365		-	if s[i:i+len(substr)] == substr {
366		-	return true
367		-	}
368		-	}
369		-	return false
	360	+	return strings.Contains(s, substr)
370	361		}

 	"encoding/hex"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 )
 			errMsg:  "path traversal",
 		},
 		{
-			name:    "double dot at start",
+			name:     "double dot at start of filename",
-			path:    "..dumber",
+			path:     "..dumber",
-			wantErr: true, // Starts with ".." so rejected as path traversal
+			wantErr:  false, // Valid filename, not a path traversal (.. is not a path component)
-			errMsg:  "path traversal",
+			wantPath: "/tmp/extract/..dumber",
 		},
 		{
 			name:    "absolute path unix",
 // contains checks if substr is in s (helper for error message checks).
 func contains(s, substr string) bool {
-	return len(s) >= len(substr) && (s == substr || substr == "" ||
+	return strings.Contains(s, substr)
-		(s != "" && substr != "" && findSubstring(s, substr)))
-}
-func findSubstring(s, substr string) bool {
-	for i := 0; i <= len(s)-len(substr); i++ {
-		if s[i:i+len(substr)] == substr {
-			return true
-		}
-	}
-	return false
 }

M internal/ui/coordinator/update.go +1 -5


@@ -67,12 +67,10 @@ func (c *UpdateCoordinator) checkAsync(ctx context.Context) {
67	67		return
68	68		}
69	69
	70	+	// Update both lastInfo and status atomically to avoid inconsistent reads.
70	71		c.mu.Lock()
71	72		c.lastInfo = result
72		-	c.mu.Unlock()
73		-
74	73		if !result.UpdateAvailable {
75		-	c.mu.Lock()
76	74		c.status = entity.UpdateStatusUpToDate
77	75		c.mu.Unlock()
78	76		log.Debug().

@@ -80,8 +78,6 @@ func (c *UpdateCoordinator) checkAsync(ctx context.Context) {
80	78		Msg("already on latest version")
81	79		return
82	80		}
83		-
84		-	c.mu.Lock()
85	81		c.status = entity.UpdateStatusAvailable
86	82		c.mu.Unlock()
87	83		log.Info().

 		return
 	}
+	// Update both lastInfo and status atomically to avoid inconsistent reads.
 	c.mu.Lock()
 	c.lastInfo = result
-	c.mu.Unlock()
 	if !result.UpdateAvailable {
-		c.mu.Lock()
 		c.status = entity.UpdateStatusUpToDate
 		c.mu.Unlock()
 		log.Debug().
 			Msg("already on latest version")
 		return
 	}
-	c.mu.Lock()
 	c.status = entity.UpdateStatusAvailable
 	c.mu.Unlock()
 	log.Info().