aboutsummaryrefslogtreecommitdiffstats
path: root/internal/notify
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-09-15 18:01:49 +0200
committerDanilo M. <danix@danix.xyz>2026-09-15 18:01:49 +0200
commit7a6b1c1ada3ba6ca2ca4ed70cf51f2f5f30d7b1e (patch)
tree6b0e0ffd76f0de162093a56d1799847a8b81ade5 /internal/notify
parent3b8a0e0f4a5c28394ac578e8559bf57ae82d3fd2 (diff)
downloadnotifyd-7a6b1c1ada3ba6ca2ca4ed70cf51f2f5f30d7b1e.tar.gz
notifyd-7a6b1c1ada3ba6ca2ca4ed70cf51f2f5f30d7b1e.zip
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.
Diffstat (limited to 'internal/notify')
-rw-r--r--internal/notify/service.go7
-rw-r--r--internal/notify/service_test.go34
-rw-r--r--internal/notify/store_test.go12
3 files changed, 49 insertions, 4 deletions
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) })