fix(admin): keep account photos out of the media library

CHANGELOG.md

@@ -7,6 +7,19 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## [development]
 
+### Fixed
+
+- An installation running the newest release was offered an update to the
+  same version: the recorded version and the release tag compared unequal
+  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
 
 -

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

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}}

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

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) {

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
+// serves it. The name is either a flat file of the media directory or
-// is opened through the store's root, so a name that would escape the
+// "avatars/<file>", the one namespace below it the route serves; anything
-// content directory is refused rather than checked for.
+// 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) {
+	cleaned := path.Clean(name)
-		return "", fmt.Errorf("%q is not an allowed image name", 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.

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 {

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 {
+	if _, err := s.MediaPath("../" + name); err == nil {
-		t.Fatalf("a base name should still resolve: %v", err)
+		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/<uuid>.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) {

internal/updater/updater.go

@@ -122,8 +122,19 @@ func fetchBounded(url string, limit int64) ([]byte, error) {
 	return body, nil
 }
 
+// trimVersionPrefix removes the "v" the toolchain records with, so a
+// recorded version ("v1.2.3", or a pseudo-version "v0.0.0-…") compares
+// against release tags, which CheckLatest already reports without it.
+func trimVersionPrefix(v string) string {
+	return strings.TrimPrefix(v, "v")
+}
+
 // CompareVersions compares two dotted versions numerically per part.
+// Either side may carry the leading "v" of a recorded version or of a
+// tag; the shapes compare equal, which is what keeps an installation
+// running v1.0.0 from being offered an update to 1.0.0.
 func CompareVersions(a, b string) int {
+	a, b = trimVersionPrefix(a), trimVersionPrefix(b)
 	as, bs := strings.Split(a, "."), strings.Split(b, ".")
 	for i := 0; i < len(as) || i < len(bs); i++ {
 		av, bv := part(as, i), part(bs, i)

internal/updater/updater_test.go

@@ -30,6 +30,13 @@ func TestCompareVersions(t *testing.T) {
 		{"0.6", "0.6.0", 0},
 		{"0.10.0", "0.9.0", 1},
 		{"1.2.3-rc1", "1.2.3", 0}, // suffix ignored per part
+		{"v1.0.0", "1.0.0", 0},    // the recorded version carries the v a tag does
+		{"1.0.0", "v1.0.0", 0},
+		{"v1.1.0", "v1.0.0", 1},
+		{"1.28.0", "1.9.0", 1}, // parts compare numerically, not by character
+		{"1.9.0", "1.28.0", -1},
+		{"1.28.0", "1.128.0", -1},
+		{"v1.28.0", "1.28.0", 0},
 	}
 	for _, tc := range cases {
 		if got := CompareVersions(tc.a, tc.b); got != tc.want {
@@ -60,6 +67,39 @@ func TestCheckLatestParsesTag(t *testing.T) {
 	if got := UpdateAvailable("2.0.0"); got != "" {
 		t.Fatalf("UpdateAvailable(newer local) = %q", got)
 	}
+	// The recorded version carries the "v" the release tag does: an
+	// installation running v1.2.3 must not be offered 1.2.3.
+	if got := UpdateAvailable("v1.2.3"); got != "" {
+		t.Fatalf("UpdateAvailable(v-prefixed current) = %q", got)
+	}
+	if got := UpdateAvailable("v1.2.2"); got != "1.2.3" {
+		t.Fatalf("UpdateAvailable(v-prefixed older) = %q", got)
+	}
+}
+
+// The production bug: an installation running v1.0.0 was offered an
+// update to 1.0.0, because the recorded version and the tag compared
+// unequal through the prefix. SelfUpdate must refuse in place, and it
+// must not download anything on the way.
+func TestSelfUpdateRefusesTheSameVersionThroughThePrefix(t *testing.T) {
+	var downloads int
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if strings.Contains(r.URL.Path, "/releases/download/") {
+			downloads++
+		}
+		_ = json.NewEncoder(w).Encode(map[string]any{"tag_name": "v9.9.9"})
+	}))
+	defer srv.Close()
+	old := ReleaseBase
+	ReleaseBase = srv.URL
+	defer func() { ReleaseBase = old }()
+
+	if _, err := SelfUpdate("v9.9.9"); err == nil || !strings.Contains(err.Error(), "already running") {
+		t.Fatalf("SelfUpdate(v-prefixed current) = %v, want the latest-release refusal", err)
+	}
+	if downloads != 0 {
+		t.Fatalf("%d download requests fired for a same-version update", downloads)
+	}
 }
 
 func TestCheckLatestErrors(t *testing.T) {