fix: improve error handling and create the json encoder once #331 (#332)

* fix: improve error handling for resource closing and JSON encoding in MakeChallenge

* chore: update CHANGELOG with recent changes and improvements

* refactor: simplify RenderIndex function and improve error handling

---------

Signed-off-by: Jason Cameron <git@jasoncameron.dev>

cmd/anubis/main.go

@@ -117,7 +117,10 @@ func setupListener(network string, address string) (net.Listener, string) {
 
 		err = os.Chmod(address, os.FileMode(mode))
 		if err != nil {
-			listener.Close()
+			err := listener.Close()
+			if err != nil {
+				log.Printf("failed to close listener: %v", err)
+			}
 			log.Fatal(fmt.Errorf("could not change socket mode: %w", err))
 		}
 	}
@@ -227,12 +230,12 @@ func main() {
 			log.Fatalf("failed to parse and validate ED25519_PRIVATE_KEY_HEX: %v", err)
 		}
 	} else if *ed25519PrivateKeyHexFile != "" {
-		hex, err := os.ReadFile(*ed25519PrivateKeyHexFile)
+		hexData, err := os.ReadFile(*ed25519PrivateKeyHexFile)
 		if err != nil {
 			log.Fatalf("failed to read ED25519_PRIVATE_KEY_HEX_FILE %s: %v", *ed25519PrivateKeyHexFile, err)
 		}
 
-		priv, err = keyFromHex(string(bytes.TrimSpace(hex)))
+		priv, err = keyFromHex(string(bytes.TrimSpace(hexData)))
 		if err != nil {
 			log.Fatalf("failed to parse and validate content of ED25519_PRIVATE_KEY_HEX_FILE: %v", err)
 		}

cmd/containerbuild/main.go

@@ -131,7 +131,7 @@ func parseImageList(imageList string) ([]image, error) {
 	}
 
 	if len(result) == 0 {
-		return nil, fmt.Errorf("no images provided, bad flags??")
+		return nil, fmt.Errorf("no images provided, bad flags")
 	}
 
 	return result, nil

docs/docs/CHANGELOG.md

@@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Added headers support to bot policy rules
 - Moved configuration file from JSON to YAML by default
 - Added documentation on how to use Anubis with Traefik in Docker
+- Improved error handling in some edge cases
 - Disable `generic-bot-catchall` rule because of its high false positive rate in real-world scenarios
 
 ## v1.16.0

internal/headers.go

@@ -73,7 +73,7 @@ func NoStoreCache(next http.Handler) http.Handler {
 	})
 }
 
-// Do not allow browsing directory listings in paths that end with /
+// NoBrowsing prevents directory browsing by returning a 404 for any request that ends with a "/".
 func NoBrowsing(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if strings.HasSuffix(r.URL.Path, "/") {

internal/ogtags/fetch.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"golang.org/x/net/html"
+	"io"
 	"log/slog"
 	"mime"
 	"net"
@@ -26,7 +27,12 @@ func (c *OGTagCache) fetchHTMLDocument(urlStr string) (*html.Node, error) {
 		return nil, fmt.Errorf("http get failed: %w", err)
 	}
 	// this defer will call MaxBytesReader's Close, which closes the original body.
-	defer resp.Body.Close()
+	defer func(Body io.ReadCloser) {
+		err := Body.Close()
+		if err != nil {
+			slog.Debug("og: error closing response body", "url", urlStr, "error", err)
+		}
+	}(resp.Body)
 
 	if resp.StatusCode != http.StatusOK {
 		slog.Debug("og: received non-OK status code", "url", urlStr, "status", resp.StatusCode)

internal/test/playwright_test.go

@@ -378,14 +378,14 @@ func pwFail(t *testing.T, page playwright.Page, format string, args ...any) erro
 }
 
 func pwTimeout(tc testCase, deadline time.Time) *float64 {
-	max := *playwrightMaxTime
+	maxTime := *playwrightMaxTime
 	if tc.isHard {
-		max = *playwrightMaxHardTime
+		maxTime = *playwrightMaxHardTime
 	}
 
 	d := time.Until(deadline)
-	if d <= 0 || d > max {
-		return playwright.Float(float64(max.Milliseconds()))
+	if d <= 0 || d > maxTime {
+		return playwright.Float(float64(maxTime.Milliseconds()))
 	}
 	return playwright.Float(float64(d.Milliseconds()))
 }

lib/anubis.go

@@ -96,7 +96,12 @@ func LoadPoliciesOrDefault(fname string, defaultDifficulty int) (*policy.ParsedC
 		}
 	}
 
-	defer fin.Close()
+	defer func(fin io.ReadCloser) {
+		err := fin.Close()
+		if err != nil {
+			slog.Error("failed to close policy file", "file", fname, "err", err)
+		}
+	}(fin)
 
 	anubisPolicy, err := policy.ParseConfig(fin, fname, defaultDifficulty)
 
@@ -201,7 +206,7 @@ func (s *Server) MaybeReverseProxy(w http.ResponseWriter, r *http.Request) {
 	r.Header.Add("X-Anubis-Rule", cr.Name)
 	r.Header.Add("X-Anubis-Action", string(cr.Rule))
 	lg = lg.With("check_result", cr)
-	policy.PolicyApplications.WithLabelValues(cr.Name, string(cr.Rule)).Add(1)
+	policy.Applications.WithLabelValues(cr.Name, string(cr.Rule)).Add(1)
 
 	ip := r.Header.Get("X-Real-Ip")
 
@@ -258,21 +263,21 @@ func (s *Server) MaybeReverseProxy(w http.ResponseWriter, r *http.Request) {
 	if err != nil {
 		lg.Debug("cookie not found", "path", r.URL.Path)
 		s.ClearCookie(w)
-		s.RenderIndex(w, r, cr, rule)
+		s.RenderIndex(w, r, rule)
 		return
 	}
 
 	if err := ckie.Valid(); err != nil {
 		lg.Debug("cookie is invalid", "err", err)
 		s.ClearCookie(w)
-		s.RenderIndex(w, r, cr, rule)
+		s.RenderIndex(w, r, rule)
 		return
 	}
 
 	if time.Now().After(ckie.Expires) && !ckie.Expires.IsZero() {
 		lg.Debug("cookie expired", "path", r.URL.Path)
 		s.ClearCookie(w)
-		s.RenderIndex(w, r, cr, rule)
+		s.RenderIndex(w, r, rule)
 		return
 	}
 
@@ -283,7 +288,7 @@ func (s *Server) MaybeReverseProxy(w http.ResponseWriter, r *http.Request) {
 	if err != nil || !token.Valid {
 		lg.Debug("invalid token", "path", r.URL.Path, "err", err)
 		s.ClearCookie(w)
-		s.RenderIndex(w, r, cr, rule)
+		s.RenderIndex(w, r, rule)
 		return
 	}
 
@@ -298,7 +303,7 @@ func (s *Server) MaybeReverseProxy(w http.ResponseWriter, r *http.Request) {
 	if !ok {
 		lg.Debug("invalid token claims type", "path", r.URL.Path)
 		s.ClearCookie(w)
-		s.RenderIndex(w, r, cr, rule)
+		s.RenderIndex(w, r, rule)
 		return
 	}
 	challenge := s.challengeFor(r, rule.Challenge.Difficulty)
@@ -306,7 +311,7 @@ func (s *Server) MaybeReverseProxy(w http.ResponseWriter, r *http.Request) {
 	if claims["challenge"] != challenge {
 		lg.Debug("invalid challenge", "path", r.URL.Path)
 		s.ClearCookie(w)
-		s.RenderIndex(w, r, cr, rule)
+		s.RenderIndex(w, r, rule)
 		return
 	}
 
@@ -323,7 +328,7 @@ func (s *Server) MaybeReverseProxy(w http.ResponseWriter, r *http.Request) {
 		lg.Debug("invalid response", "path", r.URL.Path)
 		failedValidations.Inc()
 		s.ClearCookie(w)
-		s.RenderIndex(w, r, cr, rule)
+		s.RenderIndex(w, r, rule)
 		return
 	}
 
@@ -332,7 +337,7 @@ func (s *Server) MaybeReverseProxy(w http.ResponseWriter, r *http.Request) {
 	s.next.ServeHTTP(w, r)
 }
 
-func (s *Server) RenderIndex(w http.ResponseWriter, r *http.Request, cr policy.CheckResult, rule *policy.Bot) {
+func (s *Server) RenderIndex(w http.ResponseWriter, r *http.Request, rule *policy.Bot) {
 	lg := slog.With(
 		"user_agent", r.UserAgent(),
 		"accept_language", r.Header.Get("Accept-Language"),
@@ -374,27 +379,37 @@ func (s *Server) RenderBench(w http.ResponseWriter, r *http.Request) {
 func (s *Server) MakeChallenge(w http.ResponseWriter, r *http.Request) {
 	lg := slog.With("user_agent", r.UserAgent(), "accept_language", r.Header.Get("Accept-Language"), "priority", r.Header.Get("Priority"), "x-forwarded-for", r.Header.Get("X-Forwarded-For"), "x-real-ip", r.Header.Get("X-Real-Ip"))
 
+	encoder := json.NewEncoder(w)
 	cr, rule, err := s.check(r)
 	if err != nil {
 		lg.Error("check failed", "err", err)
 		w.WriteHeader(http.StatusInternalServerError)
-		json.NewEncoder(w).Encode(struct {
+		err := encoder.Encode(struct {
 			Error string `json:"error"`
 		}{
 			Error: "Internal Server Error: administrator has misconfigured Anubis. Please contact the administrator and ask them to look for the logs around \"makeChallenge\"",
 		})
+		if err != nil {
+			lg.Error("failed to encode error response", "err", err)
+			w.WriteHeader(http.StatusInternalServerError)
+		}
 		return
 	}
 	lg = lg.With("check_result", cr)
 	challenge := s.challengeFor(r, rule.Challenge.Difficulty)
 
-	json.NewEncoder(w).Encode(struct {
+	err = encoder.Encode(struct {
 		Challenge string                 `json:"challenge"`
 		Rules     *config.ChallengeRules `json:"rules"`
 	}{
 		Challenge: challenge,
 		Rules:     rule.Challenge,
 	})
+	if err != nil {
+		lg.Error("failed to encode challenge", "err", err)
+		w.WriteHeader(http.StatusInternalServerError)
+		return
+	}
 	lg.Debug("made challenge", "challenge", challenge, "rules", rule.Challenge, "cr", cr)
 	challengesIssued.Inc()
 }

lib/policy/policy.go

@@ -13,7 +13,7 @@ import (
 )
 
 var (
-	PolicyApplications = promauto.NewCounterVec(prometheus.CounterOpts{
+	Applications = promauto.NewCounterVec(prometheus.CounterOpts{
 		Name: "anubis_policy_results",
 		Help: "The results of each policy rule",
 	}, []string{"rule", "action"})