fix: reject forged subdomains in the media proxy (#9)

* fix: make memcache concurrency-safe and stamp the version at link time

memcache was a crash waiting for traffic. Readers touched tempFS without
holding mx, while a per-entry goroutine deleted from it under the lock: a
concurrent map read and map write, which the runtime treats as a fatal error
that recover cannot catch. The option ships in config.example.json and was the
one cache key SETUP.md never documented, so it read like a free win to enable.

Put every map and field access behind the mutex, and age the whole map from one
janitor instead of a goroutine per cached file, each of which looped forever
holding its entry alive. mx is now a plain Mutex: every operation here mutates
something, and the old code took an RLock to write. Document the option, and
cover it with tests that run the readers, writers and janitor concurrently.

Split the disk/origin fetch out of DownloadAndSendMedia while there, so the
error path returns instead of falling through to write an empty body after the
error page.

Release.Version was hardcoded to 1.3.2, so images tagged v1.3.6 reported 1.3.2
from --help and /api/instance, and --help linked to the wrong release. Take it
from a main.version string the release workflow links in from the git tag.

* fix: reject forged subdomains in the media proxy

DownloadAndSendMedia built its upstream URL by concatenation, pasting the
subdomain segment of the request path straight into the host position. That
segment reaches the handler already percent-decoded, so it can carry "@", "#",
"?" and "/" — the characters that end a host. A request for

    /media/file/[email protected]:8080%2F/f/x.jpg

built a URL whose host parsed as 127.0.0.1:8080, with images-wixmp-x demoted to
userinfo, letting any caller aim the instance's fetcher at any address it could
reach, including services behind the firewall.

Validate the label against ^[a-zA-Z0-9-]+$ and refuse anything else with a 400.
Rejecting rather than escaping is what closes this: the label is the host, and
url.URL passes a host through verbatim, so building the URL structurally is not
sufficient on its own. DeviantArt's own media URLs use a hex-and-dash label, and
ParseMedia already splits on the first dot, so a legitimate label cannot contain
one.

Build the URL from url.URL fields as well, which escapes the path, and encode
the token argument, which reached the request unescaped.

Reported by CodeQL as go/request-forgery (CWE-918).

8 files changed, 379 insertions, 68 deletions

diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
index 632cfa7..28eadb8 100644
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -60,6 +60,10 @@ jobs:
           push: true
           tags: ${{ steps.meta.outputs.tags }}
           labels: ${{ steps.meta.outputs.labels }}
+          # Link the tag into the binary, so --help and /api/instance report the
+          # same version as the image tag.
+          build-args: |
+            VERSION=${{ steps.meta.outputs.version }}
           cache-from: type=gha
           cache-to: type=gha,mode=max
 
diff --git a/Dockerfile b/Dockerfile
index 719eab6..fec1cf5 100755
--- a/Dockerfile
+++ b/Dockerfile
@@ -3,10 +3,12 @@ ARG GO_VERSION=1.25
 FROM --platform=$BUILDPLATFORM golang:${GO_VERSION} AS build
 ARG TARGETOS
 ARG TARGETARCH
+# Set by the release workflow from the git tag; --help and /api/instance report it.
+ARG VERSION=dev
 
 WORKDIR /build
 COPY . .
-RUN CGO_ENABLED=0 GOARCH=${TARGETARCH} GOOS=${TARGETOS} go build -ldflags "-s -w -extldflags '-static'" && \
+RUN CGO_ENABLED=0 GOARCH=${TARGETARCH} GOOS=${TARGETOS} go build -ldflags "-s -w -extldflags '-static' -X main.version=${VERSION}" && \
   echo "skunkyart:x:10000:10000:SkunkyArt user:/:/sbin/nologin" > /etc/minimal-passwd && \
   echo "skunkyart:x:10000:" > /etc/minimal-group
 
diff --git a/README.md b/README.md
index 9dd9d17..4126803 100755
--- a/README.md
+++ b/README.md
@@ -20,6 +20,9 @@ can also add the `-ldflags "-w -s"` argument (GCCGO has a different name for it
 
 `go build -tags embed -ldflags "-w -s"`
 
+Such a build reports its version as `dev`. To stamp one in, as the release
+workflow does from the git tag, add `-X main.version=<version>` to the ldflags.
+
 ## Docker
 Prebuilt multi-arch images (`linux/amd64`, `linux/arm64`) are published to GHCR
 on every release tag:
diff --git a/SETUP.md b/SETUP.md
index 4e7dd93..987041a 100755
--- a/SETUP.md
+++ b/SETUP.md
@@ -16,6 +16,10 @@ Time units:
     runs as, and SkunkyArt refuses to start if it is not. The container image
     runs as uid 10000, so a bind-mounted cache needs
     `sudo chown -R 10000:10000 <dir>` on the host.
+  * `memcache` — Also keep served media in RAM, on top of the on-disk cache.
+    Entries are scored by how often they are requested and dropped once they go
+    a round unused. The cache is bounded only by that scoring, so leave it off
+    unless you have RAM to spare for your traffic.
   * `lifetime` — Cached file life time, requires numeric value, followed by multiplicative suffix (see Time Units for details)
   * `max-size` — Maximum file size in megabytes
   * `update-interval` — Automatic rotation interval
diff --git a/app/cache.go b/app/cache.go
index 91c9847..e4822db 100755
--- a/app/cache.go
+++ b/app/cache.go
@@ -6,7 +6,9 @@ import (
 	"crypto/sha1" //nolint:gosec // G505: SHA-1 is a cache-key hash here, not a security primitive
 	"encoding/hex"
 	"io"
+	"net/url"
 	"os"
+	"regexp"
 	"strings"
 	"sync"
 	"syscall"
@@ -18,90 +20,139 @@ type file struct {
 	Content []byte
 }
 
+// tempFS is the in-memory media cache, guarded by mx. A plain Mutex rather than
+// an RWMutex on purpose: every operation here mutates something (a read bumps
+// Score), and the previous code took an RLock to write, which is not exclusive.
 var tempFS = make(map[[20]byte]*file)
-var mx = &sync.RWMutex{}
+var mx sync.Mutex
+
+// memGet returns the cached body for key and raises its score so that popular
+// entries outlive the janitor, or nil when the entry is absent or still empty.
+func memGet(key [20]byte) []byte {
+	mx.Lock()
+	defer mx.Unlock()
+
+	f := tempFS[key]
+	if f == nil || f.Content == nil {
+		return nil
+	}
+	f.Score += 2
+	return f.Content
+}
+
+// memPut caches body under key. An empty body is not cached, so a failed fetch
+// cannot poison the cache with a zero-length image.
+func memPut(key [20]byte, body []byte) {
+	if len(body) == 0 {
+		return
+	}
+
+	mx.Lock()
+	defer mx.Unlock()
+	tempFS[key] = &file{Content: body}
+}
+
+// InitMemCacheJanitor ages the in-memory cache forever, dropping entries whose
+// score has run out. Run it in its own goroutine, once, and only when memcache
+// is enabled.
+//
+// One loop ages the whole map. The previous design started a goroutine per
+// cached file, each looping until its own entry was evicted, and each touching
+// the map without holding mx — a concurrent map read and write, which the Go
+// runtime treats as a fatal error that recover cannot catch.
+func InitMemCacheJanitor() {
+	for {
+		time.Sleep(1 * time.Minute)
+		ageMemCache()
+	}
+}
+
+// ageMemCache runs one round of aging: every entry loses a point, and entries
+// that are already out of points are dropped. An entry starts at zero, so a body
+// nothing asks for again is gone within a round.
+func ageMemCache() {
+	mx.Lock()
+	defer mx.Unlock()
+
+	for k, f := range tempFS {
+		if f.Score <= 0 {
+			delete(tempFS, k)
+			continue
+		}
+		f.Score--
+	}
+}
+
+// mediaSubdomain matches the one hostname label wixmp media URLs vary: a hex
+// string, sometimes with dashes. Anything outside that set is rejected rather
+// than escaped, because this label is what selects the host to fetch from.
+var mediaSubdomain = regexp.MustCompile(`^[a-zA-Z0-9-]+$`)
+
+// buildMediaURL returns the wixmp CDN URL for one media item, reporting false
+// when subdomain is not a bare hostname label.
+//
+// subdomain and path arrive already percent-decoded from the request path, so
+// they can carry the characters that end a host. Concatenated into a URL string,
+// a subdomain of "[email protected]#" reparses as host attacker.example, with
+// "images-wixmp-x" demoted to userinfo and the intended host to a fragment —
+// pointing the fetch at whatever the caller names, including addresses reachable
+// only from the instance itself.
+func buildMediaURL(subdomain, path, token string) (string, bool) {
+	if !mediaSubdomain.MatchString(subdomain) {
+		return "", false
+	}
+
+	// Fields rather than concatenation: String escapes the path, so a decoded
+	// "#" or "?" in it stays part of the path instead of ending it. The host is
+	// checked above rather than escaped, because url.URL passes it through
+	// verbatim.
+	u := url.URL{
+		Scheme: "https",
+		Host:   "images-wixmp-" + subdomain + ".wixmp.com",
+		Path:   "/" + path,
+	}
+	if token != "" {
+		u.RawQuery = url.Values{"token": {token}}.Encode()
+	}
+	return u.String(), true
+}
 
 // DownloadAndSendMedia proxies one image from DeviantArt's wixmp CDN to the
 // client, serving it from the on-disk or in-memory cache when enabled. It
 // responds 403 when proxying is turned off for this instance.
 func (s skunkyart) DownloadAndSendMedia(subdomain, path string) {
-	var url strings.Builder
-	url.WriteString("https://images-wixmp-")
-	url.WriteString(subdomain)
-	url.WriteString(".wixmp.com/")
-	url.WriteString(path)
-	if t := s.Args.Get("token"); t != "" {
-		url.WriteString("?token=")
-		url.WriteString(t)
+	mediaURL, ok := buildMediaURL(subdomain, path, s.Args.Get("token"))
+	if !ok {
+		s.ReturnHTTPError(400)
+		return
 	}
 
 	var response []byte
 
 	switch {
 	case CFG.Cache.Enabled:
-		fileName := sha1.Sum([]byte(subdomain + path)) //nolint:gosec // G401: cache-key hash, not a security primitive
-		filePath := CFG.Cache.Path + "/" + hex.EncodeToString(fileName[:])
-
-		c := func() {
-			// filePath is built from a SHA-1 of the request, not from user input,
-			// so it cannot escape the cache directory.
-			file, err := os.Open(filePath) //nolint:gosec // G304: path is a hash, not user-controlled
-			if err != nil {
-				dwnld := Download(url.String())
-				if dwnld.Status == 200 && strings.HasPrefix(dwnld.Headers.Get("Content-Type"), "image") {
-					response = dwnld.Body
-					try(os.WriteFile(filePath, response, 0600))
-				} else {
-					s.ReturnHTTPError(dwnld.Status)
-					return
-				}
-			} else {
-				defer func() { try(file.Close()) }()
-				file, e := io.ReadAll(file)
-				try(e)
-				response = file
-			}
-		}
+		key := sha1.Sum([]byte(subdomain + path)) //nolint:gosec // G401: cache-key hash, not a security primitive
+		filePath := CFG.Cache.Path + "/" + hex.EncodeToString(key[:])
 
 		if CFG.Cache.MemCache {
-			mx.Lock()
-			if tempFS[fileName] == nil {
-				tempFS[fileName] = &file{}
-			}
-			mx.Unlock()
-
-			if tempFS[fileName].Content != nil {
-				response = tempFS[fileName].Content
-				tempFS[fileName].Score += 2
+			if cached := memGet(key); cached != nil {
+				response = cached
 				break
-			} else {
-				c()
-				go func() {
-					defer restore()
-
-					mx.RLock()
-					tempFS[fileName].Content = response
-					mx.RUnlock()
-
-					for {
-						time.Sleep(1 * time.Minute)
-
-						mx.Lock()
-						if tempFS[fileName].Score <= 0 {
-							delete(tempFS, fileName)
-							mx.Unlock()
-							return
-						}
-						tempFS[fileName].Score--
-						mx.Unlock()
-					}
-				}()
 			}
-		} else {
-			c()
+		}
+
+		body, ok := s.loadOrFetchMedia(filePath, mediaURL)
+		if !ok {
+			// loadOrFetchMedia has already written the error response.
+			return
+		}
+		response = body
+
+		if CFG.Cache.MemCache {
+			memPut(key, response)
 		}
 	case CFG.Proxy:
-		dwnld := Download(url.String())
+		dwnld := Download(mediaURL)
 		if dwnld.Status != 200 {
 			s.ReturnHTTPError(dwnld.Status)
 			return
@@ -115,6 +166,34 @@ func (s skunkyart) DownloadAndSendMedia(subdomain, path string) {
 	_, _ = s.Writer.Write(response)
 }
 
+// loadOrFetchMedia returns the media body for filePath, preferring the on-disk
+// cache and falling back to fetching mediaURL, which it then writes back to the
+// cache. It reports false when it has already written an error response, so the
+// caller must not write anything further.
+func (s skunkyart) loadOrFetchMedia(filePath, mediaURL string) ([]byte, bool) {
+	// filePath is built from a SHA-1 of the request, not from user input, so it
+	// cannot escape the cache directory.
+	if f, err := os.Open(filePath); err == nil { //nolint:gosec // G304: path is a hash, not user-controlled
+		defer func() { try(f.Close()) }()
+
+		if body, err := io.ReadAll(f); err == nil {
+			return body, true
+		} else {
+			// An unreadable cache entry is not fatal; re-fetch it instead.
+			try(err)
+		}
+	}
+
+	dwnld := Download(mediaURL)
+	if dwnld.Status != 200 || !strings.HasPrefix(dwnld.Headers.Get("Content-Type"), "image") {
+		s.ReturnHTTPError(dwnld.Status)
+		return nil, false
+	}
+
+	try(os.WriteFile(filePath, dwnld.Body, 0600))
+	return dwnld.Body, true
+}
+
 // InitCacheSystem runs the cache rotation loop forever, evicting files past
 // their lifetime and emptying the cache when it outgrows max-size. Run it in its
 // own goroutine.
diff --git a/app/cache_test.go b/app/cache_test.go
new file mode 100644
index 0000000..eafd8b1
--- /dev/null
+++ b/app/cache_test.go
@@ -0,0 +1,207 @@
+package app
+
+import (
+	"bytes"
+	"net/http/httptest"
+	"net/url"
+	"sync"
+	"testing"
+)
+
+// resetMemCache empties the in-memory cache so each test starts clean.
+func resetMemCache() {
+	mx.Lock()
+	defer mx.Unlock()
+	tempFS = make(map[[20]byte]*file)
+}
+
+func key(b byte) [20]byte {
+	var k [20]byte
+	k[0] = b
+	return k
+}
+
+// TestBuildMediaURLRejectsForgedSubdomain is the regression test for the SSRF in
+// the media proxy: subdomain reaches us percent-decoded from the request path,
+// so it can carry "@", "#", "?" and "/" — every character that ends a host. When
+// the URL was built by concatenation, each of these reparsed as a host the
+// caller chose. The label is the host, so it has to be rejected, not escaped.
+func TestBuildMediaURLRejectsForgedSubdomain(t *testing.T) {
+	// The path a request for /media/file/<subdomain>/f.jpg would decode to.
+	for _, subdomain := range []string{
+		"[email protected]#",     // userinfo + fragment: host is attacker.example
+		"[email protected]/",     // userinfo, host terminated by the slash
+		"[email protected]:8080/",       // the same, aimed inside the instance's network
+		"x@[::1]:8080/",           // IPv6 loopback
+		"attacker.example#",       // fragment alone truncates to images-wixmp-attacker.example
+		"attacker.example?",       // query does the same
+		"a/../../secret",          // slashes escape the label entirely
+		"a\\attacker.example",     // backslash, which some parsers fold to "/"
+		"a.wixmp.com.attacker.eu", // dots: a label may not contain them
+		"",                        // empty label
+	} {
+		if got, ok := buildMediaURL(subdomain, "f/x.jpg", ""); ok {
+			t.Errorf("subdomain %q: accepted and built %q, want rejected", subdomain, got)
+		}
+	}
+}
+
+// TestBuildMediaURLKeepsHostOnWixmp is the property that actually matters: for
+// anything accepted, the host the client ends up talking to is the CDN.
+func TestBuildMediaURLKeepsHostOnWixmp(t *testing.T) {
+	got, ok := buildMediaURL("ed30a86b-8c4c-a887", "f/x.jpg", "abc")
+	if !ok {
+		t.Fatal("a plain hex-and-dash label was rejected, want accepted")
+	}
+
+	u, err := url.Parse(got)
+	if err != nil {
+		t.Fatalf("built an unparseable URL %q: %v", got, err)
+	}
+	if u.Host != "images-wixmp-ed30a86b-8c4c-a887.wixmp.com" {
+		t.Errorf("host is %q, want the wixmp CDN", u.Host)
+	}
+	if u.User != nil {
+		t.Errorf("URL carries userinfo %v, want none", u.User)
+	}
+	if u.Query().Get("token") != "abc" {
+		t.Errorf("token is %q, want abc", u.Query().Get("token"))
+	}
+}
+
+// TestBuildMediaURLEscapesPath checks that the path cannot end the URL early and
+// smuggle in a query or fragment of the caller's choosing.
+func TestBuildMediaURLEscapesPath(t *testing.T) {
+	got, ok := buildMediaURL("ed30a86b", "f/x.jpg#frag?q=1", "")
+	if !ok {
+		t.Fatal("a plain label was rejected, want accepted")
+	}
+
+	u, err := url.Parse(got)
+	if err != nil {
+		t.Fatalf("built an unparseable URL %q: %v", got, err)
+	}
+	if u.Fragment != "" {
+		t.Errorf("path opened a fragment %q, want it escaped into the path", u.Fragment)
+	}
+	if u.RawQuery != "" {
+		t.Errorf("path opened a query %q, want it escaped into the path", u.RawQuery)
+	}
+	if u.Path != "/f/x.jpg#frag?q=1" {
+		t.Errorf("path is %q, want it preserved verbatim", u.Path)
+	}
+}
+
+// TestDownloadAndSendMediaRejectsForgedSubdomain drives the handler itself, to
+// pin down that a forged label is refused before any fetch is attempted rather
+// than merely being rejected by the helper. Proxying is enabled here, so the
+// pre-fix handler would have reached the network on this input.
+func TestDownloadAndSendMediaRejectsForgedSubdomain(t *testing.T) {
+	proxy := CFG.Proxy
+	CFG.Proxy = true
+	defer func() { CFG.Proxy = proxy }()
+
+	w := httptest.NewRecorder()
+	s := skunkyart{Writer: w, Host: "http://localhost", Args: url.Values{}}
+	s.DownloadAndSendMedia("[email protected]:8080/", "f/x.jpg")
+
+	if w.Code != 400 {
+		t.Errorf("status is %d, want 400 for a forged subdomain", w.Code)
+	}
+}
+
+// TestMemCacheConcurrentAccess hammers the in-memory cache from many goroutines
+// while the janitor ages it, which is what a media flood does on an instance
+// with memcache enabled.
+//
+// This is the regression test for the readers that touched tempFS without
+// holding mx: concurrently with the janitor's delete that is a concurrent map
+// read and map write, which the runtime reports as a fatal error that no
+// recover can catch. Run under -race to also catch the unsynchronised field
+// access that does not happen to trip the map check.
+func TestMemCacheConcurrentAccess(t *testing.T) {
+	resetMemCache()
+	defer resetMemCache()
+
+	const workers, rounds = 24, 200
+	body := []byte("not-really-an-image")
+
+	var wg sync.WaitGroup
+	for w := range workers {
+		wg.Go(func() {
+			for i := range rounds {
+				// Overlapping keys, so goroutines contend for the same entries.
+				k := key(byte((w + i) % 8)) //nolint:gosec // G115: (w+i)%8 is 0-7
+				memPut(k, body)
+				memGet(k)
+			}
+		})
+	}
+
+	// Age the cache underneath the readers and writers: this is the delete that
+	// the old per-entry goroutines raced against.
+	wg.Go(func() {
+		for range rounds {
+			ageMemCache()
+		}
+	})
+
+	wg.Wait()
+}
+
+// TestMemGetReturnsStoredBody covers the plain hit and miss paths.
+func TestMemGetReturnsStoredBody(t *testing.T) {
+	resetMemCache()
+	defer resetMemCache()
+
+	k := key(1)
+	if got := memGet(k); got != nil {
+		t.Fatalf("empty cache: got %q, want nil", got)
+	}
+
+	want := []byte("body")
+	memPut(k, want)
+
+	got := memGet(k)
+	if !bytes.Equal(got, want) {
+		t.Fatalf("after put: got %q, want %q", got, want)
+	}
+}
+
+// TestMemPutIgnoresEmptyBody stops a failed fetch from caching a zero-length
+// image that would then be served to everyone until it aged out.
+func TestMemPutIgnoresEmptyBody(t *testing.T) {
+	resetMemCache()
+	defer resetMemCache()
+
+	k := key(2)
+	memPut(k, nil)
+	memPut(k, []byte{})
+
+	if got := memGet(k); got != nil {
+		t.Fatalf("empty body was cached: got %q, want nil", got)
+	}
+}
+
+// TestAgeMemCacheEvicts checks that a cold entry is dropped while a hot one
+// survives, since that scoring is the only bound on the cache's memory use.
+func TestAgeMemCacheEvicts(t *testing.T) {
+	resetMemCache()
+	defer resetMemCache()
+
+	cold, hot := key(3), key(4)
+	memPut(cold, []byte("cold"))
+	memPut(hot, []byte("hot"))
+
+	// A hit raises the hot entry's score above zero.
+	memGet(hot)
+
+	ageMemCache()
+
+	if got := memGet(cold); got != nil {
+		t.Errorf("cold entry survived aging: got %q, want nil", got)
+	}
+	if got := memGet(hot); got == nil {
+		t.Error("hot entry was evicted after a hit, want it kept")
+	}
+}
diff --git a/app/config.go b/app/config.go
index ad07aa3..74c01f7 100755
--- a/app/config.go
+++ b/app/config.go
@@ -127,6 +127,9 @@ func ExecuteConfig() {
 			// XOR (1026), not exponentiation — so the cap was ~1000x too small.
 			CFG.Cache.MaxSize *= 1024 * 1024
 			go InitCacheSystem()
+			if CFG.Cache.MemCache {
+				go InitMemCacheJanitor()
+			}
 		}
 
 		About = instanceAbout{
diff --git a/main.go b/main.go
index 7a5a63e..0d7f1f8 100755
--- a/main.go
+++ b/main.go
@@ -8,8 +8,17 @@ import (
 	"github.com/zerolabsco/devianter"
 )
 
+// version is the release this binary was built from. The release workflow links
+// it in from the git tag so that --help and /api/instance cannot drift from the
+// tag the image was built at:
+//
+//	go build -ldflags "-X main.version=1.3.7"
+//
+// A plain `go build` leaves it as "dev".
+var version = "dev"
+
 func main() {
-	app.Release.Version = "1.3.2"
+	app.Release.Version = version
 	app.Release.Description = "Two API endpoints and template embedding into binary"
 
 	app.ExecuteCommandLineArguments()