fix(download): address Copilot review - security, tests, cleanup

commit: 08eefbd5d5ce88e0c44045d54bb5feb8281bbd22
author: bnema · 9mo ago · 2026-01-07 18:19 UTC
parent: d3e73b1
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

4 files · +258 -7

M internal/infrastructure/webkit/download_handler.go +46 -5


@@ -2,6 +2,8 @@ package webkit
2	2
3	3		import (
4	4		"context"
	5	+	"fmt"
	6	+	"os"
5	7		"path/filepath"
6	8		"strings"
7	9		"sync"

@@ -11,6 +13,11 @@ import (
11	13		"github.com/bnema/puregotk-webkit/webkit"
12	14		)
13	15
	16	+	const (
	17	+	// dirPerm is the permission mode for creating download directories.
	18	+	dirPerm = 0755
	19	+	)
	20	+
14	21		// DownloadHandler manages WebKit downloads and notifies the UI layer.
15	22		type DownloadHandler struct {
16	23		downloadPath string

@@ -40,22 +47,38 @@ func (h DownloadHandler) HandleDownload(ctx context.Context, download webkit.D
40	47		var failedMu sync.Mutex
41	48
42	49		// Handle decide-destination signal to set download path.
	50	+	//nolint:unparam // bool return is required by WebKit's signal signature (false = handled synchronously)
43	51		decideDestCb := func(d webkit.Download, suggestedFilename string) bool {
44		-	destPath := filepath.Join(downloadPath, suggestedFilename)
45		-	// WebKit expects a file:// URI for the destination.
46		-	d.SetDestination("file://" + destPath)
	52	+	// Ensure download directory exists.
	53	+	if err := os.MkdirAll(downloadPath, dirPerm); err != nil {
	54	+	log.Error().Err(err).Str("path", downloadPath).Msg("failed to create download directory")
	55	+	return false
	56	+	}
	57	+
	58	+	// Sanitize filename to prevent path traversal attacks.
	59	+	safeName := sanitizeFilename(suggestedFilename)
	60	+	destPath := filepath.Join(downloadPath, safeName)
	61	+
	62	+	log.Debug().
	63	+	Str("suggested", suggestedFilename).
	64	+	Str("sanitized", safeName).
	65	+	Str("destPath", destPath).
	66	+	Msg("setting download destination")
	67	+
	68	+	// WebKit expects an absolute file path for the destination.
	69	+	d.SetDestination(destPath)
47	70
48	71		// Notify: download started.
49	72		if eventHandler != nil {
50	73		eventHandler.OnDownloadEvent(ctx, port.DownloadEvent{
51	74		Type: port.DownloadEventStarted,
52		-	Filename: suggestedFilename,
	75	+	Filename: safeName,
53	76		Destination: destPath,
54	77		})
55	78		}
56	79
57	80		log.Info().
58		-	Str("filename", suggestedFilename).
	81	+	Str("filename", safeName).
59	82		Str("destination", destPath).
60	83		Msg("download started")
61	84

@@ -72,11 +95,15 @@ func (h DownloadHandler) HandleDownload(ctx context.Context, download webkit.D
72	95		dest := d.GetDestination()
73	96		filename := extractFilename(dest)
74	97
	98	+	// Create error for the failed download.
	99	+	downloadErr := fmt.Errorf("download failed: %s", filename)
	100	+
75	101		if eventHandler != nil {
76	102		eventHandler.OnDownloadEvent(ctx, port.DownloadEvent{
77	103		Type: port.DownloadEventFailed,
78	104		Filename: filename,
79	105		Destination: dest,
	106	+	Error: downloadErr,
80	107		})
81	108		}
82	109

@@ -124,3 +151,17 @@ func extractFilename(dest string) string {
124	151		path := strings.TrimPrefix(dest, "file://")
125	152		return filepath.Base(path)
126	153		}
	154	+
	155	+	// sanitizeFilename sanitizes a suggested filename to prevent path traversal attacks.
	156	+	// It extracts only the base name and handles edge cases like "." or "..".
	157	+	func sanitizeFilename(name string) string {
	158	+	// Get only the base name (removes any directory components).
	159	+	clean := filepath.Base(name)
	160	+
	161	+	// If Base returns "." or ".." (edge cases), use fallback.
	162	+	if clean == "." \|\| clean == ".." \|\| clean == "" {
	163	+	return "download"
	164	+	}
	165	+
	166	+	return clean
	167	+	}

 import (
 	"context"
+	"fmt"
+	"os"
 	"path/filepath"
 	"strings"
 	"sync"
 	"github.com/bnema/puregotk-webkit/webkit"
 )
+const (
+	// dirPerm is the permission mode for creating download directories.
+	dirPerm = 0755
+)
 // DownloadHandler manages WebKit downloads and notifies the UI layer.
 type DownloadHandler struct {
 	downloadPath string
 	var failedMu sync.Mutex
 	// Handle decide-destination signal to set download path.
+	//nolint:unparam // bool return is required by WebKit's signal signature (false = handled synchronously)
 	decideDestCb := func(d webkit.Download, suggestedFilename string) bool {
-		destPath := filepath.Join(downloadPath, suggestedFilename)
+		// Ensure download directory exists.
-		// WebKit expects a file:// URI for the destination.
+		if err := os.MkdirAll(downloadPath, dirPerm); err != nil {
-		d.SetDestination("file://" + destPath)
+			log.Error().Err(err).Str("path", downloadPath).Msg("failed to create download directory")
+			return false
+		}
+		// Sanitize filename to prevent path traversal attacks.
+		safeName := sanitizeFilename(suggestedFilename)
+		destPath := filepath.Join(downloadPath, safeName)
+		log.Debug().
+			Str("suggested", suggestedFilename).
+			Str("sanitized", safeName).
+			Str("destPath", destPath).
+			Msg("setting download destination")
+		// WebKit expects an absolute file path for the destination.
+		d.SetDestination(destPath)
 		// Notify: download started.
 		if eventHandler != nil {
 			eventHandler.OnDownloadEvent(ctx, port.DownloadEvent{
 				Type:        port.DownloadEventStarted,
-				Filename:    suggestedFilename,
+				Filename:    safeName,
 				Destination: destPath,
 			})
 		}
 		log.Info().
-			Str("filename", suggestedFilename).
+			Str("filename", safeName).
 			Str("destination", destPath).
 			Msg("download started")
 		dest := d.GetDestination()
 		filename := extractFilename(dest)
+		// Create error for the failed download.
+		downloadErr := fmt.Errorf("download failed: %s", filename)
 		if eventHandler != nil {
 			eventHandler.OnDownloadEvent(ctx, port.DownloadEvent{
 				Type:        port.DownloadEventFailed,
 				Filename:    filename,
 				Destination: dest,
+				Error:       downloadErr,
 			})
 		}
 	path := strings.TrimPrefix(dest, "file://")
 	return filepath.Base(path)
 }
+// sanitizeFilename sanitizes a suggested filename to prevent path traversal attacks.
+// It extracts only the base name and handles edge cases like "." or "..".
+func sanitizeFilename(name string) string {
+	// Get only the base name (removes any directory components).
+	clean := filepath.Base(name)
+	// If Base returns "." or ".." (edge cases), use fallback.
+	if clean == "." || clean == ".." || clean == "" {
+		return "download"
+	}
+	return clean
+}

A internal/infrastructure/webkit/download_handler_test.go +148 -0

@@ -0,0 +1,148 @@
	1	+	package webkit
	2	+
	3	+	import (
	4	+	"testing"
	5	+
	6	+	"github.com/stretchr/testify/assert"
	7	+	)
	8	+
	9	+	func TestSanitizeFilename(t *testing.T) {
	10	+	tests := []struct {
	11	+	name string
	12	+	input string
	13	+	expected string
	14	+	}{
	15	+	{
	16	+	name: "normal filename",
	17	+	input: "document.pdf",
	18	+	expected: "document.pdf",
	19	+	},
	20	+	{
	21	+	name: "filename with spaces",
	22	+	input: "my document.pdf",
	23	+	expected: "my document.pdf",
	24	+	},
	25	+	{
	26	+	name: "path traversal with parent dirs",
	27	+	input: "../../../etc/passwd",
	28	+	expected: "passwd",
	29	+	},
	30	+	{
	31	+	name: "path traversal hidden file",
	32	+	input: "../.ssh/id_rsa",
	33	+	expected: "id_rsa",
	34	+	},
	35	+	{
	36	+	name: "nested path",
	37	+	input: "foo/bar/baz.txt",
	38	+	expected: "baz.txt",
	39	+	},
	40	+	{
	41	+	name: "absolute path",
	42	+	input: "/etc/passwd",
	43	+	expected: "passwd",
	44	+	},
	45	+	{
	46	+	name: "dot only",
	47	+	input: ".",
	48	+	expected: "download",
	49	+	},
	50	+	{
	51	+	name: "double dot only",
	52	+	input: "..",
	53	+	expected: "download",
	54	+	},
	55	+	{
	56	+	name: "empty string",
	57	+	input: "",
	58	+	expected: "download",
	59	+	},
	60	+	{
	61	+	name: "hidden file",
	62	+	input: ".bashrc",
	63	+	expected: ".bashrc",
	64	+	},
	65	+	{
	66	+	name: "windows style path",
	67	+	input: "..\\..\\Windows\\System32\\config",
	68	+	expected: "..\\..\\Windows\\System32\\config",
	69	+	},
	70	+	}
	71	+
	72	+	for _, tt := range tests {
	73	+	t.Run(tt.name, func(t *testing.T) {
	74	+	result := sanitizeFilename(tt.input)
	75	+	assert.Equal(t, tt.expected, result)
	76	+	})
	77	+	}
	78	+	}
	79	+
	80	+	func TestExtractFilename(t *testing.T) {
	81	+	tests := []struct {
	82	+	name string
	83	+	input string
	84	+	expected string
	85	+	}{
	86	+	{
	87	+	name: "file URI with path",
	88	+	input: "file:///home/user/Downloads/document.pdf",
	89	+	expected: "document.pdf",
	90	+	},
	91	+	{
	92	+	name: "file URI root",
	93	+	input: "file:///document.pdf",
	94	+	expected: "document.pdf",
	95	+	},
	96	+	{
	97	+	name: "plain path",
	98	+	input: "/home/user/Downloads/image.png",
	99	+	expected: "image.png",
	100	+	},
	101	+	{
	102	+	name: "filename only",
	103	+	input: "video.mp4",
	104	+	expected: "video.mp4",
	105	+	},
	106	+	{
	107	+	name: "empty string",
	108	+	input: "",
	109	+	expected: ".",
	110	+	},
	111	+	{
	112	+	name: "file URI with spaces",
	113	+	input: "file:///home/user/My Documents/file.txt",
	114	+	expected: "file.txt",
	115	+	},
	116	+	}
	117	+
	118	+	for _, tt := range tests {
	119	+	t.Run(tt.name, func(t *testing.T) {
	120	+	result := extractFilename(tt.input)
	121	+	assert.Equal(t, tt.expected, result)
	122	+	})
	123	+	}
	124	+	}
	125	+
	126	+	func TestNewDownloadHandler(t *testing.T) {
	127	+	t.Run("creates handler with path and nil event handler", func(t *testing.T) {
	128	+	handler := NewDownloadHandler("/tmp/downloads", nil)
	129	+
	130	+	assert.NotNil(t, handler)
	131	+	assert.Equal(t, "/tmp/downloads", handler.downloadPath)
	132	+	assert.Nil(t, handler.eventHandler)
	133	+	})
	134	+
	135	+	t.Run("creates handler with custom path", func(t *testing.T) {
	136	+	handler := NewDownloadHandler("/custom/path", nil)
	137	+
	138	+	assert.Equal(t, "/custom/path", handler.downloadPath)
	139	+	})
	140	+	}
	141	+
	142	+	func TestDownloadHandler_SetDownloadPath(t *testing.T) {
	143	+	handler := NewDownloadHandler("/initial/path", nil)
	144	+
	145	+	handler.SetDownloadPath("/new/path")
	146	+
	147	+	assert.Equal(t, "/new/path", handler.downloadPath)
	148	+	}

@@ -0,0 +1,148 @@
	1	+	package webkit
	2	+
	3	+	import (
	4	+	"testing"
	5	+
	6	+	"github.com/stretchr/testify/assert"
	7	+	)
	8	+
	9	+	func TestSanitizeFilename(t *testing.T) {
	10	+	tests := []struct {
	11	+	name string
	12	+	input string
	13	+	expected string
	14	+	}{
	15	+	{
	16	+	name: "normal filename",
	17	+	input: "document.pdf",
	18	+	expected: "document.pdf",
	19	+	},
	20	+	{
	21	+	name: "filename with spaces",
	22	+	input: "my document.pdf",
	23	+	expected: "my document.pdf",
	24	+	},
	25	+	{
	26	+	name: "path traversal with parent dirs",
	27	+	input: "../../../etc/passwd",
	28	+	expected: "passwd",
	29	+	},
	30	+	{
	31	+	name: "path traversal hidden file",
	32	+	input: "../.ssh/id_rsa",
	33	+	expected: "id_rsa",
	34	+	},
	35	+	{
	36	+	name: "nested path",
	37	+	input: "foo/bar/baz.txt",
	38	+	expected: "baz.txt",
	39	+	},
	40	+	{
	41	+	name: "absolute path",
	42	+	input: "/etc/passwd",
	43	+	expected: "passwd",
	44	+	},
	45	+	{
	46	+	name: "dot only",
	47	+	input: ".",
	48	+	expected: "download",
	49	+	},
	50	+	{
	51	+	name: "double dot only",
	52	+	input: "..",
	53	+	expected: "download",
	54	+	},
	55	+	{
	56	+	name: "empty string",
	57	+	input: "",
	58	+	expected: "download",
	59	+	},
	60	+	{
	61	+	name: "hidden file",
	62	+	input: ".bashrc",
	63	+	expected: ".bashrc",
	64	+	},
	65	+	{
	66	+	name: "windows style path",
	67	+	input: "..\\..\\Windows\\System32\\config",
	68	+	expected: "..\\..\\Windows\\System32\\config",
	69	+	},
	70	+	}
	71	+
	72	+	for _, tt := range tests {
	73	+	t.Run(tt.name, func(t *testing.T) {
	74	+	result := sanitizeFilename(tt.input)
	75	+	assert.Equal(t, tt.expected, result)
	76	+	})
	77	+	}
	78	+	}
	79	+
	80	+	func TestExtractFilename(t *testing.T) {
	81	+	tests := []struct {
	82	+	name string
	83	+	input string
	84	+	expected string
	85	+	}{
	86	+	{
	87	+	name: "file URI with path",
	88	+	input: "file:///home/user/Downloads/document.pdf",
	89	+	expected: "document.pdf",
	90	+	},
	91	+	{
	92	+	name: "file URI root",
	93	+	input: "file:///document.pdf",
	94	+	expected: "document.pdf",
	95	+	},
	96	+	{
	97	+	name: "plain path",
	98	+	input: "/home/user/Downloads/image.png",
	99	+	expected: "image.png",
	100	+	},
	101	+	{
	102	+	name: "filename only",
	103	+	input: "video.mp4",
	104	+	expected: "video.mp4",
	105	+	},
	106	+	{
	107	+	name: "empty string",
	108	+	input: "",
	109	+	expected: ".",
	110	+	},
	111	+	{
	112	+	name: "file URI with spaces",
	113	+	input: "file:///home/user/My Documents/file.txt",
	114	+	expected: "file.txt",
	115	+	},
	116	+	}
	117	+
	118	+	for _, tt := range tests {
	119	+	t.Run(tt.name, func(t *testing.T) {
	120	+	result := extractFilename(tt.input)
	121	+	assert.Equal(t, tt.expected, result)
	122	+	})
	123	+	}
	124	+	}
	125	+
	126	+	func TestNewDownloadHandler(t *testing.T) {
	127	+	t.Run("creates handler with path and nil event handler", func(t *testing.T) {
	128	+	handler := NewDownloadHandler("/tmp/downloads", nil)
	129	+
	130	+	assert.NotNil(t, handler)
	131	+	assert.Equal(t, "/tmp/downloads", handler.downloadPath)
	132	+	assert.Nil(t, handler.eventHandler)
	133	+	})
	134	+
	135	+	t.Run("creates handler with custom path", func(t *testing.T) {
	136	+	handler := NewDownloadHandler("/custom/path", nil)
	137	+
	138	+	assert.Equal(t, "/custom/path", handler.downloadPath)
	139	+	})
	140	+	}
	141	+
	142	+	func TestDownloadHandler_SetDownloadPath(t *testing.T) {
	143	+	handler := NewDownloadHandler("/initial/path", nil)
	144	+
	145	+	handler.SetDownloadPath("/new/path")
	146	+
	147	+	assert.Equal(t, "/new/path", handler.downloadPath)
	148	+	}

A internal/infrastructure/xdg/adapter_test.go +63 -0

@@ -0,0 +1,63 @@
	1	+	package xdg
	2	+
	3	+	import (
	4	+	"os"
	5	+	"path/filepath"
	6	+	"testing"
	7	+
	8	+	"github.com/stretchr/testify/assert"
	9	+	"github.com/stretchr/testify/require"
	10	+	)
	11	+
	12	+	func TestAdapter_DownloadDir(t *testing.T) {
	13	+	adapter := New()
	14	+
	15	+	t.Run("returns XDG_DOWNLOAD_DIR when set", func(t *testing.T) {
	16	+	// Save and restore original env.
	17	+	original := os.Getenv("XDG_DOWNLOAD_DIR")
	18	+	defer func() {
	19	+	if original == "" {
	20	+	os.Unsetenv("XDG_DOWNLOAD_DIR")
	21	+	} else {
	22	+	os.Setenv("XDG_DOWNLOAD_DIR", original)
	23	+	}
	24	+	}()
	25	+
	26	+	os.Setenv("XDG_DOWNLOAD_DIR", "/custom/downloads")
	27	+
	28	+	dir, err := adapter.DownloadDir()
	29	+
	30	+	require.NoError(t, err)
	31	+	assert.Equal(t, "/custom/downloads", dir)
	32	+	})
	33	+
	34	+	t.Run("falls back to ~/Downloads when XDG_DOWNLOAD_DIR not set", func(t *testing.T) {
	35	+	// Save and restore original env.
	36	+	original := os.Getenv("XDG_DOWNLOAD_DIR")
	37	+	defer func() {
	38	+	if original == "" {
	39	+	os.Unsetenv("XDG_DOWNLOAD_DIR")
	40	+	} else {
	41	+	os.Setenv("XDG_DOWNLOAD_DIR", original)
	42	+	}
	43	+	}()
	44	+
	45	+	os.Unsetenv("XDG_DOWNLOAD_DIR")
	46	+
	47	+	dir, err := adapter.DownloadDir()
	48	+
	49	+	require.NoError(t, err)
	50	+
	51	+	home, err := os.UserHomeDir()
	52	+	require.NoError(t, err)
	53	+
	54	+	expected := filepath.Join(home, "Downloads")
	55	+	assert.Equal(t, expected, dir)
	56	+	})
	57	+	}
	58	+
	59	+	func TestNew(t *testing.T) {
	60	+	adapter := New()
	61	+
	62	+	assert.NotNil(t, adapter)
	63	+	}

@@ -0,0 +1,63 @@
	1	+	package xdg
	2	+
	3	+	import (
	4	+	"os"
	5	+	"path/filepath"
	6	+	"testing"
	7	+
	8	+	"github.com/stretchr/testify/assert"
	9	+	"github.com/stretchr/testify/require"
	10	+	)
	11	+
	12	+	func TestAdapter_DownloadDir(t *testing.T) {
	13	+	adapter := New()
	14	+
	15	+	t.Run("returns XDG_DOWNLOAD_DIR when set", func(t *testing.T) {
	16	+	// Save and restore original env.
	17	+	original := os.Getenv("XDG_DOWNLOAD_DIR")
	18	+	defer func() {
	19	+	if original == "" {
	20	+	os.Unsetenv("XDG_DOWNLOAD_DIR")
	21	+	} else {
	22	+	os.Setenv("XDG_DOWNLOAD_DIR", original)
	23	+	}
	24	+	}()
	25	+
	26	+	os.Setenv("XDG_DOWNLOAD_DIR", "/custom/downloads")
	27	+
	28	+	dir, err := adapter.DownloadDir()
	29	+
	30	+	require.NoError(t, err)
	31	+	assert.Equal(t, "/custom/downloads", dir)
	32	+	})
	33	+
	34	+	t.Run("falls back to ~/Downloads when XDG_DOWNLOAD_DIR not set", func(t *testing.T) {
	35	+	// Save and restore original env.
	36	+	original := os.Getenv("XDG_DOWNLOAD_DIR")
	37	+	defer func() {
	38	+	if original == "" {
	39	+	os.Unsetenv("XDG_DOWNLOAD_DIR")
	40	+	} else {
	41	+	os.Setenv("XDG_DOWNLOAD_DIR", original)
	42	+	}
	43	+	}()
	44	+
	45	+	os.Unsetenv("XDG_DOWNLOAD_DIR")
	46	+
	47	+	dir, err := adapter.DownloadDir()
	48	+
	49	+	require.NoError(t, err)
	50	+
	51	+	home, err := os.UserHomeDir()
	52	+	require.NoError(t, err)
	53	+
	54	+	expected := filepath.Join(home, "Downloads")
	55	+	assert.Equal(t, expected, dir)
	56	+	})
	57	+	}
	58	+
	59	+	func TestNew(t *testing.T) {
	60	+	adapter := New()
	61	+
	62	+	assert.NotNil(t, adapter)
	63	+	}

M internal/ui/app.go +1 -2


@@ -482,7 +482,7 @@ func (a *App) initDownloadHandler(ctx context.Context) {
482	482		}
483	483
484	484		// Create download event adapter to show toasts.
485		-	eventAdapter := &downloadEventAdapter{app: a, ctx: ctx}
	485	+	eventAdapter := &downloadEventAdapter{app: a}
486	486
487	487		// Create and wire the download handler.
488	488		handler := webkit.NewDownloadHandler(downloadPath, eventAdapter)

@@ -494,7 +494,6 @@ func (a *App) initDownloadHandler(ctx context.Context) {
494	494		// downloadEventAdapter implements port.DownloadEventHandler and shows toasts.
495	495		type downloadEventAdapter struct {
496	496		app *App
497		-	ctx context.Context
498	497		}
499	498
500	499		func (d *downloadEventAdapter) OnDownloadEvent(ctx context.Context, event port.DownloadEvent) {

 	}
 	// Create download event adapter to show toasts.
-	eventAdapter := &downloadEventAdapter{app: a, ctx: ctx}
+	eventAdapter := &downloadEventAdapter{app: a}
 	// Create and wire the download handler.
 	handler := webkit.NewDownloadHandler(downloadPath, eventAdapter)
 // downloadEventAdapter implements port.DownloadEventHandler and shows toasts.
 type downloadEventAdapter struct {
 	app *App
-	ctx context.Context
 }
 func (d *downloadEventAdapter) OnDownloadEvent(ctx context.Context, event port.DownloadEvent) {