From 594d5d0150c531aa0e06f56003de9bbcef6cb71e Mon Sep 17 00:00:00 2001 From: Thayol Date: Sun, 13 Sep 2026 10:28:33 +0200 Subject: [PATCH] Clean up wrong vanity rules --- .gitignore | 4 ++ internal/server/server.go | 39 ++++++++++++++++ internal/server/server_test.go | 84 ++++++++++++++++++++++++++++++++-- internal/store/id.go | 18 +++----- internal/store/store_test.go | 17 +++++-- web/templates/layout.html | 3 +- 6 files changed, 145 insertions(+), 20 deletions(-) diff --git a/.gitignore b/.gitignore index e69e0ae..da3c025 100644 --- a/.gitignore +++ b/.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 diff --git a/internal/server/server.go b/internal/server/server.go index 6687d54..c151cb9 100644 --- a/internal/server/server.go +++ b/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 } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index a1d63d7..0b9c5b9 100644 --- a/internal/server/server_test.go +++ b/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 diff --git a/internal/store/id.go b/internal/store/id.go index 41e5f0d..2795b9d 100644 --- a/internal/store/id.go +++ b/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 } diff --git a/internal/store/store_test.go b/internal/store/store_test.go index d40a68a..9b01031 100644 --- a/internal/store/store_test.go +++ b/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 - // collide on a case-insensitive filesystem must be refused. + // Anything that could escape the objects directory or collide on a + // case-insensitive filesystem must be refused. invalid := []string{ "", "a", ".", "..", "...", "../etc/passwd", "a/b", `a\b`, "/abs", - ".hidden", "a..b", "trailing.", "api", "static", "d", "i", - "robots.txt", "tokens.json", "with space", "emoji-๐Ÿ™‚", + ".hidden", "a..b", "trailing.", "d", "i", "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) { diff --git a/web/templates/layout.html b/web/templates/layout.html index 7204735..9e819f5 100644 --- a/web/templates/layout.html +++ b/web/templates/layout.html @@ -3,8 +3,9 @@ -{{.Title}} ยท Uncensored Send +{{.Title}} - Uncensored Send +{{if .Favicon}}{{end}}