From 7a6b1c1ada3ba6ca2ca4ed70cf51f2f5f30d7b1e Mon Sep 17 00:00:00 2001
From: "Danilo M." <danix@danix.xyz>
Date: Tue, 15 Sep 2026 18:01:49 +0200
Subject: fix(notify): cover expiry cleanup and harden the image-removal guard

Split the dismiss test so expiry is exercised on its own: Expire removes
the image without deleting the inert entry, and nothing asserted it.

removeImage now requires filepath.Dir(filepath.Clean(path)) to equal the
image directory, so a path carrying .. cannot reach a sibling daemon file
such as queue.json. The previous prefix test accepted it. Not exploitable
until Task 4 populates Popup.Image from a client hint, which is exactly
why the guard is fixed now.
---
 internal/notify/service.go      |  7 ++++---
 internal/notify/service_test.go | 34 ++++++++++++++++++++++++++++++++++
 internal/notify/store_test.go   | 12 +++++++++++-
 3 files changed, 49 insertions(+), 4 deletions(-)

(limited to 'internal/notify')

diff --git a/internal/notify/service.go b/internal/notify/service.go
index d46293f..2442127 100644
--- a/internal/notify/service.go
+++ b/internal/notify/service.go
@@ -14,7 +14,7 @@ package notify
 import (
 	"log"
 	"os"
-	"strings"
+	"path/filepath"
 	"sync"
 	"time"
 
@@ -52,12 +52,13 @@ func NewService(conn *dbus.Conn, dir string) *Service {
 }
 
 // removeImage unlinks only what the daemon wrote, so a client's own image-path
-// is never touched.
+// is never touched. A cleaned path must sit directly inside the image
+// directory, which also refuses a `..` traversal to a sibling daemon file.
 func (s *Service) removeImage(path string) {
 	if path == "" {
 		return
 	}
-	if !strings.HasPrefix(path, ImagesDir(s.dir)+string(os.PathSeparator)) {
+	if filepath.Dir(filepath.Clean(path)) != ImagesDir(s.dir) {
 		return
 	}
 	os.Remove(path)
diff --git a/internal/notify/service_test.go b/internal/notify/service_test.go
index fb47b98..871a0c7 100644
--- a/internal/notify/service_test.go
+++ b/internal/notify/service_test.go
@@ -144,6 +144,40 @@ func TestCloseNotificationEmitsReasonThree(t *testing.T) {
 	}
 }
 
+func TestRemoveImageRefusesPathsOutsideTheImageDir(t *testing.T) {
+	dir := t.TempDir()
+	imgDir := ImagesDir(dir)
+	if err := os.MkdirAll(imgDir, 0o700); err != nil {
+		t.Fatal(err)
+	}
+	sibling := filepath.Join(dir, "queue.json")
+	if err := os.WriteFile(sibling, []byte("x"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	owned := filepath.Join(imgDir, "1.png")
+	if err := os.WriteFile(owned, []byte("x"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	s := &Service{dir: dir}
+
+	s.removeImage(filepath.Join(imgDir, "..", "queue.json"))
+	if _, err := os.Stat(sibling); err != nil {
+		t.Fatalf("traversal removed a sibling: %v", err)
+	}
+	s.removeImage(sibling)
+	if _, err := os.Stat(sibling); err != nil {
+		t.Fatalf("unrelated path removed a sibling: %v", err)
+	}
+	s.removeImage("")
+	if _, err := os.Stat(sibling); err != nil {
+		t.Fatalf("empty path removed a sibling: %v", err)
+	}
+	s.removeImage(owned)
+	if _, err := os.Stat(owned); !os.IsNotExist(err) {
+		t.Fatalf("owned image not removed: %v", err)
+	}
+}
+
 // A natural expiry must drop the timer entry, or the map grows by one timer per
 // id for the whole session. No bus is needed: the store's emit is a no-op here,
 // so the test exercises arm's closure directly.
diff --git a/internal/notify/store_test.go b/internal/notify/store_test.go
index a0ab7be..13f78dc 100644
--- a/internal/notify/store_test.go
+++ b/internal/notify/store_test.go
@@ -133,7 +133,7 @@ func TestHistoryRingCapsAtTwenty(t *testing.T) {
 	}
 }
 
-func TestStoreRemovesImageOnDismissAndExpire(t *testing.T) {
+func TestStoreRemovesImageOnDismiss(t *testing.T) {
 	var removed []string
 	s := NewStore(func(uint32, uint32) {}, func(_, _ []Popup) {}, func(p string) { removed = append(removed, p) })
 	id, _ := s.Add(&Popup{Image: "/run/img/1.png"}, "", 0)
@@ -144,6 +144,16 @@ func TestStoreRemovesImageOnDismissAndExpire(t *testing.T) {
 	}
 }
 
+func TestStoreRemovesImageOnExpire(t *testing.T) {
+	var removed []string
+	s := NewStore(func(uint32, uint32) {}, func(_, _ []Popup) {}, func(p string) { removed = append(removed, p) })
+	id, _ := s.Add(&Popup{Image: "/run/img/expire.png"}, "", 0)
+	s.Expire(id)
+	if len(removed) != 1 || removed[0] != "/run/img/expire.png" {
+		t.Fatalf("expire removed %v", removed)
+	}
+}
+
 func TestStoreRemovesImageOnReplace(t *testing.T) {
 	var removed []string
 	s := NewStore(func(uint32, uint32) {}, func(_, _ []Popup) {}, func(p string) { removed = append(removed, p) })
-- 
cgit v1.2.3