From 81d22b24a79c124d3b83a309da01d692287c641b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Balv=C3=ADn?= Date: Tue, 29 Sep 2026 23:47:27 +0200 Subject: [PATCH] fix(admin): keep account photos out of the media library --- CHANGELOG.md | 5 ++ internal/admin/admin.go | 1 + internal/admin/media_routes.go | 19 ++++++ internal/admin/settings_account.go | 7 ++- internal/admin/settings_routes_test.go | 50 ++++++++++++++- internal/store/media.go | 85 +++++++++++++++++++++++--- internal/store/store.go | 7 +++ internal/store/store_test.go | 74 +++++++++++++++++++++- 8 files changed, 233 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d0e87d0..8874ea9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,11 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm through their leading `v`. The comparison now accepts both shapes, so v1.0.0 against 1.0.0 is no update at all, and the update button can no longer reinstall the version that is already running. +- A profile photo upload landed in the media library as a tile. Photos now + live under `avatars/` inside the media directory and the library never + lists them; photos of installations that predate the directory stay out of + the tiles as well, and a media delete can no longer remove a whole + directory through a crafted URL. ### Added diff --git a/internal/admin/admin.go b/internal/admin/admin.go index 916e749..bce3c97 100644 --- a/internal/admin/admin.go +++ b/internal/admin/admin.go @@ -43,6 +43,7 @@ type Content interface { RestoreRevision(p *post.Post, name string) *post.Post InvalidateCache() StoreUpload(originalName string, data []byte) (string, error) + StoreAvatar(data []byte) (string, error) MediaPath(name string) (string, error) DeleteMedia(url string) bool ListMedia() []store.Media diff --git a/internal/admin/media_routes.go b/internal/admin/media_routes.go index af9126a..b1e9be5 100644 --- a/internal/admin/media_routes.go +++ b/internal/admin/media_routes.go @@ -19,6 +19,25 @@ func (a *Admin) registerMediaRoutes(mux *http.ServeMux) { func (a *Admin) handleMediaLibrary(w http.ResponseWriter, r *http.Request) { data := a.pageData(r) items := a.deps.Store.ListMedia() + // A profile photo of any account is not library content. Avatars + // stored since the avatar directory existed never reach this list + // at all; the filter keeps the flat photos of installations that + // predate it out of the tiles the same way. + photos := make(map[string]bool) + for _, user := range a.deps.Users.All() { + if user.Photo != "" { + photos[user.Photo] = true + } + } + if len(photos) > 0 { + filtered := items[:0] + for _, item := range items { + if !photos[item.URL] { + filtered = append(filtered, item) + } + } + items = filtered + } data.MediaItems = mediaRows(items) data.MediaTotal = humanSize(totalSize(items)) data.Crumbs = []Crumb{{Label: "Media", IsLast: true, UI: true}} diff --git a/internal/admin/settings_account.go b/internal/admin/settings_account.go index 982f331..681db75 100644 --- a/internal/admin/settings_account.go +++ b/internal/admin/settings_account.go @@ -149,7 +149,7 @@ func (a *Admin) handleSettingsPhoto(w http.ResponseWriter, r *http.Request) { if !a.requireCSRF(w, r) { return } - file, header, err := r.FormFile("photo") + file, _, err := r.FormFile("photo") if err != nil { a.renderSettings(w, r, a.tr(r, "No file selected."), "", http.StatusUnprocessableEntity) return @@ -166,7 +166,10 @@ func (a *Admin) handleSettingsPhoto(w http.ResponseWriter, r *http.Request) { return } username := a.currentUser(r) - url, err := a.deps.Store.StoreUpload(header.Filename, raw) + // The photo is account state, not library content: it is stored + // under avatars/ inside the media directory, which the media + // library never lists as a tile. + url, err := a.deps.Store.StoreAvatar(raw) if err != nil { a.renderSettings(w, r, a.tr(r, "The photo could not be stored."), "", http.StatusInternalServerError) return diff --git a/internal/admin/settings_routes_test.go b/internal/admin/settings_routes_test.go index 141890a..88fd76e 100644 --- a/internal/admin/settings_routes_test.go +++ b/internal/admin/settings_routes_test.go @@ -11,6 +11,7 @@ import ( "net/http/httptest" "net/url" "os" + "path" "path/filepath" "strings" "sync/atomic" @@ -505,9 +506,18 @@ func TestPhotoUploadAndRemove(t *testing.T) { if !strings.Contains(rec.Body.String(), "Profile photo updated.") { t.Fatalf("body = %s", rec.Body.String()) } - if f.users.Find("admin").Photo == "" { + photo := f.users.Find("admin").Photo + if photo == "" { t.Fatal("photo not stored on the user") } + // The photo is account state, not a library tile: it lives under + // avatars/ and the media library does not list it. + if !strings.HasPrefix(photo, "/media/avatars/") { + t.Fatalf("photo url = %q, want the avatar namespace", photo) + } + if media := f.storeObj.ListMedia(); len(media) != 0 { + t.Fatalf("the avatar leaked into the library: %v", media) + } rec = postForm(t, f, "/admin/settings/photo/remove", url.Values{"_csrf": {csrf}}, cookie) if !strings.Contains(rec.Body.String(), "Profile photo removed.") { @@ -516,6 +526,44 @@ func TestPhotoUploadAndRemove(t *testing.T) { if f.users.Find("admin").Photo != "" { t.Fatal("photo not cleared") } + if _, err := f.storeObj.MediaPath(strings.TrimPrefix(photo, "/media/")); err == nil { + t.Fatal("the avatar file survived the removal") + } +} + +// Installations that predate the avatar directory kept their photos flat +// in the media directory; those files still serve, but the library hides +// every URL an account photo references, so the tiles stay content-only. +func TestMediaLibraryHidesAccountPhotos(t *testing.T) { + f := newFixture(t) + cookie := login(t, f, "admin", "correct-horse-9") + + webpData := append([]byte("RIFF"), 0, 0, 0, 0) + webpData = append(webpData, []byte("WEBPVP8 ")...) + legacy, err := f.storeObj.StoreUpload("legacy.png", webpData) + if err != nil { + t.Fatalf("StoreUpload: %v", err) + } + if _, err := f.users.UpdatePhoto("admin", legacy); err != nil { + t.Fatalf("UpdatePhoto: %v", err) + } + plain, err := f.storeObj.StoreUpload("plain.png", webpData) + if err != nil { + t.Fatalf("StoreUpload: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/admin/media", nil) + req.AddCookie(cookie) + body := f.do(t, req).Body.String() + // The tile is identified by its data-name attribute: the signed-in + // user's topbar avatar legitimately carries the photo URL too, so a + // bare name search would match the chrome, not the library. + if strings.Contains(body, `data-name="`+path.Base(legacy)+`"`) { + t.Fatal("the account photo appears as a library tile") + } + if !strings.Contains(body, `data-name="`+path.Base(plain)+`"`) { + t.Fatal("an ordinary media file is missing from the library") + } } func TestWebhookTestDelivery(t *testing.T) { diff --git a/internal/store/media.go b/internal/store/media.go index ee0b2a6..83ba7ca 100644 --- a/internal/store/media.go +++ b/internal/store/media.go @@ -17,19 +17,36 @@ import ( ) // MediaPath returns the absolute path of a media file, for a caller that -// serves it. The name must carry an allowed image extension, and the file -// is opened through the store's root, so a name that would escape the -// content directory is refused rather than checked for. +// serves it. The name is either a flat file of the media directory or +// "avatars/", the one namespace below it the route serves; anything +// else, and a name that carries no allowed image extension, is refused +// rather than checked for. The file is opened through the store's root, so +// a path that would escape it cannot be named. func (s *Store) MediaPath(name string) (string, error) { - if !imagefile.Allowed(name) { - return "", fmt.Errorf("%q is not an allowed image name", name) + cleaned := path.Clean(name) + if cleaned == "." || cleaned == ".." || strings.HasPrefix(cleaned, "..") { + return "", fmt.Errorf("%q is not a media name", name) + } + dir, base := path.Split(cleaned) + dir = path.Clean(dir) // "." for a flat name, "avatars" for an avatar + if base == "." || base == ".." || base == "" { + return "", fmt.Errorf("%q is not a media name", name) + } + if !imagefile.Allowed(base) { + return "", fmt.Errorf("%q is not an allowed image name", base) + } + rel := base + if dir != "." { + if dir != AvatarDirName { + return "", fmt.Errorf("%q is not a served media path", name) + } + rel = path.Join(AvatarDirName, base) } - base := filepath.Base(name) root, err := s.openRoot() if err != nil { return "", err } - handle, err := root.Open(path.Join(MediaDirName, base)) + handle, err := root.Open(path.Join(MediaDirName, rel)) if err != nil { return "", err } @@ -41,7 +58,7 @@ func (s *Store) MediaPath(name string) (string, error) { if !info.Mode().IsRegular() { return "", fmt.Errorf("%q is not a regular file", base) } - return filepath.Join(s.ContentDir, MediaDirName, base), nil + return filepath.Join(s.ContentDir, MediaDirName, rel), nil } // StoreUpload persists an uploaded image and returns its public URL. The @@ -67,17 +84,65 @@ func (s *Store) StoreUpload(originalName string, data []byte) (string, error) { return "/media/" + name, nil } +// StoreAvatar persists an account profile photo and returns its public +// URL, under avatars/ inside the media directory: the photo is account +// state and not library content, so it never appears as a media tile. +// The extension comes from the byte signature as StoreUpload does. +func (s *Store) StoreAvatar(data []byte) (string, error) { + ext := imagefile.Detect(data) + if ext == "" { + return "", fmt.Errorf("unsupported image format") + } + root, err := s.openRoot() + if err != nil { + return "", err + } + dir := path.Join(MediaDirName, AvatarDirName) + if err := root.MkdirAll(dir, 0o755); err != nil { + return "", fmt.Errorf("create avatar directory: %w", err) + } + name := uuid.NewV4().String() + ext + if err := atomicWriteIn(root, path.Join(dir, name), data); err != nil { + return "", err + } + return "/media/" + AvatarDirName + "/" + name, nil +} + // DeleteMedia removes a media file by its public URL and reports whether a -// file was removed. +// file was removed. The URL names either a flat file of the media +// directory or an avatar below avatars/; nothing else is accepted, and a +// URL that names no file at all removes nothing. func (s *Store) DeleteMedia(url string) bool { if url == "" || !strings.HasPrefix(url, "/media/") { return false } + name := path.Clean(strings.TrimPrefix(url, "/media/")) + if name == "." || name == ".." || strings.HasPrefix(name, "..") { + return false + } + dir, base := path.Split(name) + dir = path.Clean(dir) + var rel string + switch { + case dir == "." && base != "" && base != "." && base != "..": + rel = base + case dir == AvatarDirName && base != "" && base != "." && base != "..": + rel = path.Join(AvatarDirName, base) + default: + return false + } root, err := s.openRoot() if err != nil { return false } - return root.Remove(path.Join(MediaDirName, filepath.Base(url))) == nil + target := path.Join(MediaDirName, rel) + // Only a regular file is removed: a URL that resolves to the media + // directory, the avatar directory or any other directory is refused, + // so a delete can never empty a namespace. + if info, err := root.Stat(target); err != nil || !info.Mode().IsRegular() { + return false + } + return root.Remove(target) == nil } // Media is one file in the media library. diff --git a/internal/store/store.go b/internal/store/store.go index 19c3fd3..b65e185 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -36,6 +36,13 @@ var safeSlugRe = regexp.MustCompile(`[^a-zA-Z0-9._-]`) // MediaDirName is the uploads directory inside the content directory. const MediaDirName = "media" +// AvatarDirName is the directory inside the media directory that carries +// the account profile photos. The media library lists only the files of +// the media directory itself, so an avatar there never appears as a tile; +// the URL keeps the /media/ prefix, so the public route, the delete path +// and the backups treat it like any other image. +const AvatarDirName = "avatars" + // Store manages posts on disk in a flat or language-subdivided // directory. type Store struct { diff --git a/internal/store/store_test.go b/internal/store/store_test.go index dfda51c..9cc294a 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -5,6 +5,7 @@ package store import ( "os" + "path" "path/filepath" "strings" "sync" @@ -364,8 +365,8 @@ func TestStoreUploadAndListMedia(t *testing.T) { if _, err := s.MediaPath(name); err != nil { t.Fatalf("MediaPath cannot find the upload: %v", err) } - if _, err := s.MediaPath("../" + name); err != nil { - t.Fatalf("a base name should still resolve: %v", err) + if _, err := s.MediaPath("../" + name); err == nil { + t.Fatal("MediaPath accepted a name that climbs out of the media directory") } if _, err := s.MediaPath("notes.txt"); err == nil { t.Fatal("MediaPath accepted a name that is not an image") @@ -383,6 +384,75 @@ func TestStoreUploadAndListMedia(t *testing.T) { if s.DeleteMedia("/etc/passwd") { t.Fatal("DeleteMedia accepted non-media URL") } + // A URL that resolves to a directory removes nothing: the media and + // avatar directories are namespaces, not deletable content. + if s.DeleteMedia("/media/") { + t.Fatal("DeleteMedia accepted the media directory itself") + } +} + +// A profile photo is account state, not library content: it lives under +// avatars/ inside the media directory, which the listing skips, while +// the public route serves it and the delete path removes it. +func TestStoreAvatarLivesOutsideTheLibrary(t *testing.T) { + s, _ := newStore(t) + webpData := append([]byte("RIFF"), 0, 0, 0, 0) + webpData = append(webpData, []byte("WEBPVP8 ")...) + url, err := s.StoreAvatar(webpData) + if err != nil { + t.Fatalf("StoreAvatar: %v", err) + } + if !strings.HasPrefix(url, "/media/avatars/") || !strings.HasSuffix(url, ".webp") { + t.Fatalf("url = %q", url) + } + name := strings.TrimPrefix(url, "/media/") // avatars/.webp + if got, err := s.MediaPath(name); err != nil { + t.Fatalf("MediaPath cannot find the avatar: %v", err) + } else if !strings.Contains(filepath.ToSlash(got), "/media/avatars/") { + t.Fatalf("avatar path = %q", got) + } + if _, err := s.MediaPath("avatars/" + path.Base(name)); err != nil { + t.Fatalf("MediaPath cannot find the avatar by its route shape: %v", err) + } + if media := s.ListMedia(); len(media) != 0 { + t.Fatalf("the avatar leaked into the library: %v", media) + } + if !s.DeleteMedia(url) { + t.Fatal("DeleteMedia failed for the avatar") + } + if s.DeleteMedia(url) { + t.Fatal("DeleteMedia succeeded twice") + } + if _, err := s.MediaPath(name); err == nil { + t.Fatal("the avatar survived its deletion") + } +} + +// The avatar namespace is exactly one level below the media directory: +// anything deeper, anything outside it and anything without an image +// extension is refused, so the public route gains no reach. +func TestMediaPathRefusesNonMediaNames(t *testing.T) { + s, _ := newStore(t) + for _, name := range []string{ + "avatars/../x.webp", + "avatars/a/b.webp", + "sub/x.webp", + "avatarss/x.webp", + "avatars/x.txt", + "avatars/", + "avatars", + "..", + "a/../b.webp", + } { + if got, err := s.MediaPath(name); err == nil { + t.Fatalf("MediaPath(%q) = %q, want a refusal", name, got) + } + } + // The avatar directory itself is not deletable content, with or + // without a trailing slash. + if s.DeleteMedia("/media/avatars") || s.DeleteMedia("/media/avatars/") { + t.Fatal("DeleteMedia removed the avatar directory") + } } func TestStoreUploadSVGAndDimensions(t *testing.T) {