Clean up wrong vanity rules

.gitignore

@@ -1,2 +1,6 @@
 /uncensored-send
 /data/
+
+# Optional branding, supplied per deployment and embedded at build time.
+/web/static/favicon.png
+/web/static/favicon.ico

internal/server/server.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"html/template"
 	"io"
+	"io/fs"
 	"log/slog"
 	"net/http"
 	"strings"
@@ -32,6 +33,10 @@ type Server struct {
 	authLimiter *limiter
 	slots       chan struct{} // bounds uploads in flight
 
+	// favicon is the name of the embedded icon, or "" when this build has
+	// none. Resolved once: the assets cannot change while the process runs.
+	favicon string
+
 	now func() time.Time // swappable in tests
 }
 
@@ -50,6 +55,7 @@ func New(cfg *config.Config, st *store.Store, tokens *auth.File, log *slog.Logge
 		authLimiter: newLimiter(120, 20),
 		slots:       make(chan struct{}, cfg.MaxConcurrent),
 		now:         time.Now,
+		favicon:     faviconFor(web.Static()),
 	}
 	s.handler = s.routes()
 	return s, nil
@@ -70,6 +76,9 @@ func (s *Server) routes() http.Handler {
 	mux.HandleFunc("POST /login", s.handleLogin)
 	mux.HandleFunc("POST /logout", s.handleLogout)
 	mux.Handle("GET /static/", http.StripPrefix("/static/", s.staticHandler()))
+	if s.favicon != "" {
+		mux.HandleFunc("GET /favicon.ico", s.handleFavicon)
+	}
 	mux.HandleFunc("/", s.handleNotFound)
 
 	var h http.Handler = mux
@@ -92,6 +101,29 @@ func (s *Server) routes() http.Handler {
 
 // staticHandler serves the embedded assets with a long, immutable-ish cache
 // window kept short enough that an edit shows up without a cache-buster.
+// faviconFor names the icon to serve, or "" when the build has none. The file
+// is optional on purpose: none is committed, and dropping one into web/static
+// before building is the whole of the installation procedure.
+func faviconFor(fsys fs.FS) string {
+	// A .png is preferred where both exist; every browser in service reads it,
+	// and .ico survives only as the name the root request asks for.
+	for _, name := range []string{"favicon.png", "favicon.ico"} {
+		if f, err := fsys.Open(name); err == nil {
+			f.Close()
+			return name
+		}
+	}
+	return ""
+}
+
+// handleFavicon answers the root request browsers make on their own, whichever
+// of the two names the build supplied: a .png served here is still a .png, and
+// the Content-Type says so.
+func (s *Server) handleFavicon(w http.ResponseWriter, r *http.Request) {
+	w.Header().Set("Cache-Control", "public, max-age=300")
+	http.ServeFileFS(w, r, web.Static(), s.favicon)
+}
+
 func (s *Server) staticHandler() http.Handler {
 	fileServer := http.FileServerFS(web.Static())
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -196,6 +228,10 @@ type page struct {
 	User  string
 	Admin bool
 
+	// Favicon is the icon's URL, empty when the build shipped none, in which
+	// case the markup carries no link rather than one that 404s.
+	Favicon string
+
 	// Wide widens the page for content that is a table rather than a form.
 	// The reading measure that suits the upload page is far too narrow for a
 	// listing, which otherwise ends up behind a horizontal scrollbar.
@@ -206,6 +242,9 @@ type page struct {
 // who is logged in and offer only the links they can use.
 func (s *Server) page(r *http.Request, title string, script bool) page {
 	p := page{Base: s.cfg.BasePath, Title: title, Script: script}
+	if s.favicon != "" {
+		p.Favicon = s.cfg.BasePath + "static/" + s.favicon
+	}
 	if lim, err := s.limitsFor(r, cookieCredential(r)); err == nil {
 		p.User, p.Admin = lim.Name, lim.Admin
 	}

internal/server/server_test.go

@@ -17,6 +17,7 @@ import (
 	"slices"
 	"strings"
 	"testing"
+	"testing/fstest"
 	"time"
 
 	"uncensored-send/internal/auth"
@@ -414,17 +415,46 @@ func TestPathTraversalIsRejected(t *testing.T) {
 	}
 }
 
-func TestReservedNamesAreRejected(t *testing.T) {
+// A vanity name has only the spelling rules to satisfy. Names that look like
+// routes or like the site's own assets are ordinary names, because an id is
+// reachable only under /d/ and /i/ and never collides with anything of ours.
+func TestVanityNamesThatLookLikeRoutesAreOrdinary(t *testing.T) {
 	h := newHarness(t, nil)
-	for _, name := range []string{"api", "static", "d", "i", "upload", "robots.txt"} {
+
+	for _, name := range []string{"favicon.png", "admin", "login", "upload", "static", "robots.txt", "tokens.json"} {
+		resp := h.formUpload(t, map[string]string{"vanity": name, "token": h.token}, "f.txt", "body of "+name)
+		body, _ := io.ReadAll(resp.Body)
+		resp.Body.Close()
+		if resp.StatusCode != http.StatusCreated {
+			t.Errorf("vanity %q => %s: %s", name, resp.Status, strings.TrimSpace(string(body)))
+			continue
+		}
+		// And it is genuinely reachable at the name that was asked for.
+		got := h.get(t, "/d/"+name, "")
+		content, _ := io.ReadAll(got.Body)
+		got.Body.Close()
+		if got.StatusCode != http.StatusOK {
+			t.Errorf("GET /d/%s => %s, want 200", name, got.Status)
+		}
+		if string(content) != "body of "+name {
+			t.Errorf("GET /d/%s served %q, not the file that was uploaded", name, content)
+		}
+	}
+}
+
+// The spelling rules themselves still stand: they are what keeps a name from
+// becoming a path element it should not be.
+func TestMalformedVanityNamesAreRefused(t *testing.T) {
+	h := newHarness(t, nil)
+	for _, name := range []string{"d", "i", "no spaces allowed", "-leading-dash", "..", "trailing.", "a/b"} {
 		resp := h.upload(t, []byte("x"), map[string]string{
 			"Vanity":        name,
 			"Authorization": "Bearer " + h.token,
 		})
+		resp.Body.Close()
 		if resp.StatusCode != http.StatusBadRequest {
 			t.Errorf("vanity %q => %s, want 400", name, resp.Status)
 		}
-		resp.Body.Close()
 	}
 }
 
@@ -942,6 +972,54 @@ func (h *harness) formUploadWith(t *testing.T, cookie *http.Cookie, fields map[s
 	return resp
 }
 
+// --- favicon --------------------------------------------------------------
+
+// No icon is committed, so the selection logic is exercised against stand-in
+// filesystems: a build that has one is a build nobody can write a test for.
+func TestFaviconSelection(t *testing.T) {
+	for _, c := range []struct {
+		name  string
+		files []string
+		want  string
+	}{
+		{"nothing shipped", nil, ""},
+		{"a png", []string{"favicon.png"}, "favicon.png"},
+		{"an ico", []string{"favicon.ico"}, "favicon.ico"},
+		{"both, png wins", []string{"favicon.ico", "favicon.png"}, "favicon.png"},
+		{"something else entirely", []string{"logo.png"}, ""},
+	} {
+		fsys := fstest.MapFS{}
+		for _, f := range c.files {
+			fsys[f] = &fstest.MapFile{Data: []byte("x")}
+		}
+		if got := faviconFor(fsys); got != c.want {
+			t.Errorf("%s: faviconFor = %q, want %q", c.name, got, c.want)
+		}
+	}
+}
+
+// With no icon in the build, the markup must not promise one: a link to a
+// missing file costs every visitor a 404 on every page.
+func TestNoFaviconMeansNoLink(t *testing.T) {
+	h := newHarness(t, nil)
+	if h.favicon != "" {
+		t.Skipf("this build embeds %q, so the empty case cannot be checked here", h.favicon)
+	}
+
+	resp := h.get(t, "/", "")
+	page, _ := io.ReadAll(resp.Body)
+	resp.Body.Close()
+	if strings.Contains(string(page), `rel="icon"`) {
+		t.Error("the page links an icon that this build does not carry")
+	}
+
+	resp = h.get(t, "/favicon.ico", "")
+	resp.Body.Close()
+	if resp.StatusCode != http.StatusNotFound {
+		t.Errorf("GET /favicon.ico = %s, want 404 when no icon is embedded", resp.Status)
+	}
+}
+
 // --- content security policy ---------------------------------------------
 
 // The page's own behaviour and its CSP have to agree, and nothing in a Go test

internal/store/id.go

@@ -13,15 +13,6 @@ import (
 // could be mistaken for a path element, a dotfile or a traversal is excluded.
 var vanityRe = regexp.MustCompile(`^[a-z0-9][a-z0-9._-]{1,63}$`)
 
-// reserved names would shadow a route or a well-known file if they were ever
-// allowed into the object namespace.
-var reserved = map[string]bool{
-	"d": true, "i": true, "api": true, "static": true, "admin": true,
-	"login": true, "logout": true, "upload": true,
-	"favicon.ico": true, "robots.txt": true, "index.html": true,
-	"sitemap.xml": true, "tokens.json": true, "objects": true,
-}
-
 var ErrBadID = errors.New("invalid name")
 
 // CleanID validates an id arriving from a URL or from a vanity request and
@@ -30,6 +21,12 @@ var ErrBadID = errors.New("invalid name")
 //
 // This is the *only* function permitted to turn caller input into a path
 // element; every filesystem path in this package is built from its output.
+//
+// There is deliberately no list of reserved words. An id appears only under
+// /d/ and /i/ in a URL, and only as a directory of its own inside the objects
+// directory on disk, so no spelling of it can shadow a route or a file of
+// ours: "favicon.png" and "admin" are ordinary names and refusing them would
+// be theatre.
 func CleanID(s string) (string, error) {
 	s = strings.ToLower(strings.TrimSpace(s))
 	if !vanityRe.MatchString(s) {
@@ -40,9 +37,6 @@ func CleanID(s string) (string, error) {
 	if strings.Contains(s, "..") || strings.HasSuffix(s, ".") {
 		return "", ErrBadID
 	}
-	if reserved[s] {
-		return "", ErrBadID
-	}
 	return s, nil
 }
 

internal/store/store_test.go

@@ -23,12 +23,11 @@ func TestCleanID(t *testing.T) {
 		}
 	}
 
-	// Anything that could escape the objects directory, shadow a route, or
+	// Anything that could escape the objects directory or collide on a
-	// collide on a case-insensitive filesystem must be refused.
+	// case-insensitive filesystem must be refused.
 	invalid := []string{
 		"", "a", ".", "..", "...", "../etc/passwd", "a/b", `a\b`, "/abs",
-		".hidden", "a..b", "trailing.", "api", "static", "d", "i",
+		".hidden", "a..b", "trailing.", "d", "i", "with space", "emoji-🙂",
-		"robots.txt", "tokens.json", "with space", "emoji-🙂",
 		strings.Repeat("x", 65), "a\x00b", "a\nb",
 	}
 	for _, in := range invalid {
@@ -36,6 +35,16 @@ func TestCleanID(t *testing.T) {
 			t.Errorf("CleanID(%q) = %q, want an error", in, got)
 		}
 	}
+
+	// Names that merely look like something of ours are ordinary names: an id
+	// lives under /d/ and /i/ and in a directory of its own, so it shadows
+	// nothing. Refusing these would take names from people for no benefit.
+	for _, in := range []string{"api", "static", "admin", "upload", "login",
+		"robots.txt", "tokens.json", "favicon.png", "index.html"} {
+		if got, err := CleanID(in); err != nil || got != in {
+			t.Errorf("CleanID(%q) = %q, %v; want it accepted unchanged", in, got, err)
+		}
+	}
 }
 
 func TestCleanIDAcceptsGeneratedUUIDs(t *testing.T) {

web/templates/layout.html

@@ -3,8 +3,9 @@
 <head>
 <meta charset="utf-8">
 <meta name="viewport" content="width=device-width, initial-scale=1">
-<title>{{.Title}} · Uncensored Send</title>
+<title>{{.Title}} - Uncensored Send</title>
 <link rel="stylesheet" href="{{.Base}}static/style.css">
+{{if .Favicon}}<link rel="icon" href="{{.Favicon}}">{{end}}
 </head>
 <body data-base="{{.Base}}"{{if .Wide}} class="wide"{{end}}>
 <header>