Commit `7eb5e5e223`

7eb5e5e2230b6fb5b1bed6f0eaa03279aa61555b

parent: 40d405f318

Verified · cmc

cmc <hello@cleberg.net> · 2026-07-15 07:24 UTC

fix: harden HTTP transport, server timeouts and panic paths

Correctness and security findings surfaced by golangci-lint, plus two
latent panics found alongside them.

- router: http.ListenAndServe has no timeouts at all (gosec G114), so a
  slow client could hold a connection and its handler open indefinitely.
  Replace it with an explicit http.Server carrying read/write/idle
  timeouts.
- httpclient: InstallDAThrottle asserted http.DefaultTransport was a
  *http.Transport and would panic outright if anything had already
  wrapped it -- which is precisely what that function does. Check the
  assertion and fall back to a fresh transport. Expose ProxiedTransport
  so a configured download-proxy can inherit the same throttle and
  timeouts instead of silently bypassing them.
- cache: the Sys() assertion to *syscall.Stat_t is only valid on unix and
  would panic elsewhere; skip rotation instead. Indexing
  Headers["Content-Type"][0] panics when the header is absent; use
  Headers.Get. Cache files are written 0600 rather than 0700, as they are
  never executed.
- cli, api: check error returns, and exit rather than nil-dereference a
  file handle that failed to open.

SHA-1 and math/rand keep //nolint:gosec with reasons: they are cache-key
hashes and random-artwork picks, not security primitives.

Layout: unified · split

app/api.go +20 −11

 	"github.com/zerolabsco/devianter"
 )
+// API serves the JSON endpoints under /api, backed by the request its main
+// field points at.
 type API struct {
 	main *skunkyart
 }
 	Settings settingsParams `json:"settings"`
 }
+// Info responds with this instance's version and its proxy/NSFW settings.
 func (a API) Info() {
 	json, err := json.Marshal(info{
 		Version: a.main.Version,
 		},
 	})
 	try(err)
-	a.main.Writer.Write(json)
+	_, _ = a.main.Writer.Write(json)
 }
+// Error responds with a JSON error body and the given HTTP status.
 func (a API) Error(description string, status int) {
 	a.main.Writer.WriteHeader(status)
 	var response strings.Builder
 }
 func (a API) sendMedia(d *devianter.Deviation) {
-	mediaUrl, name := devianter.UrlFromMedia(d.Media)
+	mediaURL, name := devianter.UrlFromMedia(d.Media)
 	a.main.SetFilename(name)
-	if len(mediaUrl) != 0 {
+	if len(mediaURL) != 0 {
 		return
 	}
 	if CFG.Proxy {
-		mediaUrl = mediaUrl[21:]
+		mediaURL = mediaURL[21:]
-		dot := strings.Index(mediaUrl, ".")
+		dot := strings.Index(mediaURL, ".")
 		a.main.Writer.Header().Del("Content-Type")
-		a.main.DownloadAndSendMedia(mediaUrl[:dot], mediaUrl[dot+11:])
+		a.main.DownloadAndSendMedia(mediaURL[:dot], mediaURL[dot+11:])
 	} else {
-		a.main.Writer.Header().Add("Location", mediaUrl)
+		a.main.Writer.Header().Add("Location", mediaURL)
 		a.main.Writer.WriteHeader(302)
 	}
 }
-// TODO: add filters
+// Random responds with a random artwork's media, retrying a bounded number of
+// times when a search comes back empty or NSFW-filtered.
+//
+// TODO: add filters.
 func (a API) Random() {
 	// Bounded retries: the loop used to be unbounded, and the DeviantArt-error
 	// path never incremented attempt, so a single request could spin forever
 	// hammering the API (and get this instance's egress IP banned).
 	const maxAttempts = 3
-	for attempt := 0; attempt < maxAttempts; attempt++ {
+	// math/rand is deliberate: this picks a random artwork to show, which is not
+	// a security decision and does not need a cryptographic source.
+	for range maxAttempts {
 		// strconv.Itoa, not string(): string(65) is "A", not "65".
-		s, daErr, err := devianter.PerformSearch(strconv.Itoa(rand.Intn(999)), rand.Intn(30), 'a')
+		s, daErr, err := devianter.PerformSearch(strconv.Itoa(rand.Intn(999)), rand.Intn(30), 'a') //nolint:gosec // G404
 		try(err)
 		if daErr.RAW != nil {
 			continue
 			continue
 		}
-		deviation := &s.Results[rand.Intn(len(s.Results))]
+		deviation := &s.Results[rand.Intn(len(s.Results))] //nolint:gosec // G404: see above
 		if deviation.NSFW && !CFG.Nsfw {
 			continue
 		}

app/cache.go +27 −14

	@@ -1,8 +1,9 @@
1	// TODO: implement JSON caching and clean up the code
2	package app	1	package app
3		2
		3	// TODO: implement JSON caching and clean up the code.
		4
4	import (	5	import (
5	"crypto/sha1"	6	"crypto/sha1" //nolint:gosec // G505: SHA-1 is a cache-key hash here, not a security primitive
6	"encoding/hex"	7	"encoding/hex"
7	"io"	8	"io"
8	"os"	9	"os"
	@@ -20,6 +21,9 @@ type file struct {
20	var tempFS = make(map[[20]byte]*file)	21	var tempFS = make(map[[20]byte]*file)
21	var mx = &sync.RWMutex{}	22	var mx = &sync.RWMutex{}
22		23
		24	// DownloadAndSendMedia proxies one image from DeviantArt's wixmp CDN to the
		25	// client, serving it from the on-disk or in-memory cache when enabled. It
		26	// responds 403 when proxying is turned off for this instance.
23	func (s skunkyart) DownloadAndSendMedia(subdomain, path string) {	27	func (s skunkyart) DownloadAndSendMedia(subdomain, path string) {
24	var url strings.Builder	28	var url strings.Builder
25	url.WriteString("https://images-wixmp-")	29	url.WriteString("https://images-wixmp-")
	@@ -35,20 +39,24 @@ func (s skunkyart) DownloadAndSendMedia(subdomain, path string) {
35		39
36	switch {	40	switch {
37	case CFG.Cache.Enabled:	41	case CFG.Cache.Enabled:
38	fileName := sha1.Sum([]byte(subdomain + path))	42	fileName := sha1.Sum([]byte(subdomain + path)) //nolint:gosec // G401: cache-key hash, not a security primitive
39	filePath := CFG.Cache.Path + "/" + hex.EncodeToString(fileName[:])	43	filePath := CFG.Cache.Path + "/" + hex.EncodeToString(fileName[:])
40		44
41	c := func() {	45	c := func() {
42	file, err := os.Open(filePath)	46	// filePath is built from a SHA-1 of the request, not from user input,
		47	// so it cannot escape the cache directory.
		48	file, err := os.Open(filePath) //nolint:gosec // G304: path is a hash, not user-controlled
43	if err != nil {	49	if err != nil {
44	if dwnld := Download(url.String()); dwnld.Status == 200 && dwnld.Headers["Content-Type"][0][:5] == "image" {	50	dwnld := Download(url.String())
		51	if dwnld.Status == 200 && strings.HasPrefix(dwnld.Headers.Get("Content-Type"), "image") {
45	response = dwnld.Body	52	response = dwnld.Body
46	try(os.WriteFile(filePath, response, 0700))	53	try(os.WriteFile(filePath, response, 0600))
47	} else {	54	} else {
48	s.ReturnHTTPError(dwnld.Status)	55	s.ReturnHTTPError(dwnld.Status)
49	return	56	return
50	}	57	}
51	} else {	58	} else {
		59	defer func() { try(file.Close()) }()
52	file, e := io.ReadAll(file)	60	file, e := io.ReadAll(file)
53	try(e)	61	try(e)
54	response = file	62	response = file
	@@ -104,16 +112,19 @@ func (s skunkyart) DownloadAndSendMedia(subdomain, path string) {
104	response = []byte("Sorry, butt proxy on this instance are disabled.")	112	response = []byte("Sorry, butt proxy on this instance are disabled.")
105	}	113	}
106		114
107	s.Writer.Write(response)	115	_, _ = s.Writer.Write(response)
108	}	116	}
109		117
		118	// InitCacheSystem runs the cache rotation loop forever, evicting files past
		119	// their lifetime and emptying the cache when it outgrows max-size. Run it in its
		120	// own goroutine.
110	func InitCacheSystem() {	121	func InitCacheSystem() {
111	c := &CFG.Cache	122	c := &CFG.Cache
112	for {	123	for {
113	dir, err := os.ReadDir(c.Path)	124	dir, err := os.ReadDir(c.Path)
114	if err != nil {	125	if err != nil {
115	if os.IsNotExist(err) {	126	if os.IsNotExist(err) {
116	os.Mkdir(c.Path, 0700)	127	try(os.Mkdir(c.Path, 0700))
117	continue	128	continue
118	}	129	}
119	println(err.Error())	130	println(err.Error())
	@@ -128,11 +139,13 @@ func InitCacheSystem() {
128	if c.Lifetime != "" {	139	if c.Lifetime != "" {
129	now := time.Now().UnixMilli()	140	now := time.Now().UnixMilli()
130		141
131	stat := fileInfo.Sys().(*syscall.Stat_t)	142	// Sys() is platform-specific and only documented to be a
132	time := statTime(stat)	143	// *syscall.Stat_t on unix; skip rotation rather than panic
133		144	// if the filesystem reports something else.
134	if time+lifetimeParsed <= now {	145	if stat, ok := fileInfo.Sys().(*syscall.Stat_t); ok {
135	try(os.RemoveAll(fileName))	146	if statTime(stat)+lifetimeParsed <= now {
		147	try(os.RemoveAll(fileName))
		148	}
136	}	149	}
137	}	150	}
138		151
	@@ -144,7 +157,7 @@ func InitCacheSystem() {
144		157
145	if c.MaxSize != 0 && total > c.MaxSize {	158	if c.MaxSize != 0 && total > c.MaxSize {
146	try(os.RemoveAll(c.Path))	159	try(os.RemoveAll(c.Path))
147	os.Mkdir(c.Path, 0700)	160	try(os.Mkdir(c.Path, 0700))
148	}	161	}
149		162
150	time.Sleep(time.Second * time.Duration(c.UpdateInterval))	163	time.Sleep(time.Second * time.Duration(c.UpdateInterval))

app/cli.go +23 −10

 	"time"
 )
+// ExecuteCommandLineArguments parses argv, applying the flags that override
+// config and running one-shot commands such as --help and --add-instance. Some
+// of those commands exit the process rather than return.
 func ExecuteCommandLineArguments() {
 	var helpmsg = `SkunkyArt v{{.Version}} [{{.Description}}]
 Usage:
 		case "-h", "--help":
 			var buf bytes.Buffer
 			t := template.New("help")
-			t.Parse(helpmsg)
+			tryWithExitStatus(func() error {
-			t.Execute(&buf, &Release)
+				if _, err := t.Parse(helpmsg); err != nil {
+					return err
+				}
+				return t.Execute(&buf, &Release)
+			}(), 1)
 			exit(buf.String(), 0)
 		case "-a", "--add-instance":
 			addInstance()
 	var settingsVar struct {
 		Instances []settings `json:"instances"`
 	}
-	instancesJson, err := os.OpenFile("instances.json", os.O_CREATE|os.O_WRONLY, 0644)
+	// 0644: both files are committed to the repository and are meant to be
-	try(err)
+	// world-readable, so gosec's 0600 default does not apply.
-	defer instancesJson.Close()
+	instancesJSON, err := os.OpenFile("instances.json", os.O_CREATE|os.O_WRONLY, 0644) //nolint:gosec // G302
+	if err != nil {
+		exit(err.Error(), 1)
+	}
+	defer func() { try(instancesJSON.Close()) }()
-	instancesFile, err := os.OpenFile("INSTANCES.md", os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
+	instancesFile, err := os.OpenFile("INSTANCES.md", os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) //nolint:gosec // G302
-	try(err)
+	if err != nil {
-	defer instancesFile.Close()
+		exit(err.Error(), 1)
+	}
+	defer func() { try(instancesFile.Close()) }()
 	for {
 		if string(instances) == "" {
 			j, err := json.MarshalIndent(&settingsVar, "", "    ")
 			try(err)
-			instancesJson.Write(j)
+			try(func() error { _, err := instancesJSON.Write(j); return err }())
 			settingsVar := &settingsVar.Instances[len(settingsVar.Instances)-1]
 			var mdstr bytes.Buffer
 			mdstr.WriteString(settingsVar.Country)
 			mdstr.WriteString("|")
-			instancesFile.Write(mdstr.Bytes())
+			try(func() error { _, err := instancesFile.Write(mdstr.Bytes()); return err }())
 			break
 		}
 		time.Sleep(500 * time.Millisecond)

app/httpclient.go +49 −9

	@@ -2,6 +2,7 @@ package app
2		2
3	import (	3	import (
4	"net/http"	4	"net/http"
		5	"net/url"
5	"strings"	6	"strings"
6	"sync"	7	"sync"
7	"time"	8	"time"
	@@ -23,6 +24,10 @@ var (
23	daMaxConcurrent = 2 // max simultaneous in-flight DA requests	24	daMaxConcurrent = 2 // max simultaneous in-flight DA requests
24	)	25	)
25		26
		27	// downloadTimeout bounds a single outbound fetch end to end, so that a stalled
		28	// CDN connection cannot pin a request handler open indefinitely.
		29	const downloadTimeout = 60 * time.Second
		30
26	type daThrottle struct {	31	type daThrottle struct {
27	base http.RoundTripper	32	base http.RoundTripper
28	sem chan struct{}	33	sem chan struct{}
	@@ -30,6 +35,8 @@ type daThrottle struct {
30	last time.Time	35	last time.Time
31	}	36	}
32		37
		38	// RoundTrip applies the rate and concurrency limits to DeviantArt requests and
		39	// passes everything else straight through to the base transport.
33	func (t daThrottle) RoundTrip(req http.Request) (*http.Response, error) {	40	func (t daThrottle) RoundTrip(req http.Request) (*http.Response, error) {
34	// Only throttle DeviantArt's WAF-protected API host; let everything else fly.	41	// Only throttle DeviantArt's WAF-protected API host; let everything else fly.
35	if !strings.Contains(req.URL.Hostname(), "deviantart.com") {	42	if !strings.Contains(req.URL.Hostname(), "deviantart.com") {
	@@ -51,18 +58,51 @@ func (t daThrottle) RoundTrip(req http.Request) (*http.Response, error) {
51	return t.base.RoundTrip(req)	58	return t.base.RoundTrip(req)
52	}	59	}
53		60
		61	// baseTransport is the tuned transport installed by InstallDAThrottle, kept so
		62	// that per-client transports (see ProxiedTransport) inherit the same timeouts
		63	// instead of silently bypassing them.
		64	var baseTransport *http.Transport
		65
		66	// tunedTransport clones the current default transport, preserving its Proxy
		67	// (ProxyFromEnvironment) and connection-pool defaults, and tightens timeouts to
		68	// bound hung connections.
		69	func tunedTransport() *http.Transport {
		70	base, ok := http.DefaultTransport.(*http.Transport)
		71	if !ok {
		72	// Already wrapped, or a non-standard transport is installed. Start from a
		73	// fresh one rather than panicking on a type assertion.
		74	base = &http.Transport{Proxy: http.ProxyFromEnvironment}
		75	}
		76
		77	t := base.Clone()
		78	t.TLSHandshakeTimeout = 10 * time.Second
		79	t.ResponseHeaderTimeout = 20 * time.Second
		80	t.ExpectContinueTimeout = 2 * time.Second
		81	return t
		82	}
		83
54	// InstallDAThrottle wraps http.DefaultTransport with the rate/concurrency limits and	84	// InstallDAThrottle wraps http.DefaultTransport with the rate/concurrency limits and
55	// timeouts above. Call once at startup, before any DeviantArt request is made.	85	// timeouts above. Call once at startup, before any DeviantArt request is made.
56	func InstallDAThrottle() {	86	func InstallDAThrottle() {
57	// Clone the default transport so we keep its Proxy (ProxyFromEnvironment) and	87	baseTransport = tunedTransport()
58	// connection-pool defaults, then tighten timeouts to bound hung connections.	88	http.DefaultTransport = throttled(baseTransport)
59	base := http.DefaultTransport.(*http.Transport).Clone()	89	}
60	base.TLSHandshakeTimeout = 10 * time.Second	90
61	base.ResponseHeaderTimeout = 20 * time.Second	91	// throttled wraps base with the DeviantArt rate and concurrency limits.
62	base.ExpectContinueTimeout = 2 * time.Second	92	func throttled(base http.RoundTripper) http.RoundTripper {
		93	return &daThrottle{base: base, sem: make(chan struct{}, daMaxConcurrent)}
		94	}
63		95
64	http.DefaultTransport = &daThrottle{	96	// ProxiedTransport returns a throttled transport routing through proxy. Downloads
65	base: base,	97	// configured with download-proxy go through here so they keep the timeouts and
66	sem: make(chan struct{}, daMaxConcurrent),	98	// limits that InstallDAThrottle installs on the default transport.
		99	func ProxiedTransport(proxy *url.URL) http.RoundTripper {
		100	var base *http.Transport
		101	if baseTransport != nil {
		102	base = baseTransport.Clone()
		103	} else {
		104	base = tunedTransport()
67	}	105	}
		106	base.Proxy = http.ProxyURL(proxy)
		107	return throttled(base)
68	}	108	}

app/router.go +34 −11

 	"skunkyart/static"
 	"strconv"
 	"strings"
+	"time"
 )
+// Host is the scheme and host that generated links are built from. It is set per
+// request from the Host header and X-Forwarded-Proto.
 var Host string
+// Router registers the single catch-all handler that dispatches every path, then
+// serves until the process exits. It does not return on success.
 func Router() {
 	parsepath := func(path string) map[int]string {
 		if l := len(CFG.URI); len(path) > l {
 		return parsedpath
 	}
-	next := func(path map[int]string, from int) (out string) {
+	next := func(path map[int]string, from int) string {
+		var out strings.Builder
 		for x, l := from, len(path)-1; x <= l; x++ {
-			out += path[x]
+			out.WriteString(path[x])
 			if x != l {
-				out += "/"
+				out.WriteString("/")
 			}
 		}
-		return
+		return out.String()
 	}
 	open := func(name string) []byte {
 		file, err := static.Templates.Open(name)
-		try(err)
+		if err != nil {
-		fileReaded, err := io.ReadAll(file)
+			try(err)
-		try(err)
+			return nil
+		}
+		defer func() { try(file.Close()) }()
+		fileReaded, err := io.ReadAll(file)
+		if err != nil {
+			try(err)
+			return nil
+		}
 		return fileReaded
 	}
 				skunky.Emojitar(path[3])
 			}
 		case "stylesheet":
-			w.Header().Add("content-type", "text/css")
+			w.Header().Add("Content-Type", "text/css")
-			w.Write(open("css/skunky.css"))
+			_, _ = w.Write(open("css/skunky.css"))
 		case "favicon.ico":
-			w.Write(open("images/logo.png"))
+			_, _ = w.Write(open("images/logo.png"))
 		// API
 		case "api":
 	http.HandleFunc("/", handle)
 	println("SkunkyArt is listening on", CFG.Listen)
-	tryWithExitStatus(http.ListenAndServe(CFG.Listen, nil), 1)
+	// Explicit timeouts: the bare http.ListenAndServe has none, so a slow client
+	// can hold a connection (and its handler) open indefinitely. WriteTimeout is
+	// generous because media proxying streams large files through a handler.
+	srv := &http.Server{
+		Addr:              CFG.Listen,
+		ReadHeaderTimeout: 10 * time.Second,
+		ReadTimeout:       30 * time.Second,
+		WriteTimeout:      120 * time.Second,
+		IdleTimeout:       120 * time.Second,
+	}
+	tryWithExitStatus(srv.ListenAndServe(), 1)
 }

Commit 7eb5e5e223

Commit `7eb5e5e223`