Read the expiry form value where the body is bounded (#43)

The pinned CI linter's gosec G120 flagged r.FormValue in
buildDatabaseTargetConfig because the MaxBytesReader guard lives
one function up in processTargetCreate, out of static-analysis
sight. Read the expiry alongside the other form values in
processTargetCreate and pass it down as a string, matching how
the http and slack config builders receive their URL.

internal/handlers/export_test.go

@@ -28,7 +28,7 @@ func (s *Handlers) BuildSlackTargetConfigForTest(
 // package.
 func (s *Handlers) BuildDatabaseTargetConfigForTest(
 	w http.ResponseWriter,
-	r *http.Request,
+	expiry string,
 ) (string, error) {
-	return s.buildDatabaseTargetConfig(w, r)
+	return s.buildDatabaseTargetConfig(w, expiry)
 }

internal/handlers/handlers_test.go

@@ -4,8 +4,6 @@ import (
 	"context"
 	"net/http"
 	"net/http/httptest"
-	"net/url"
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -189,25 +187,6 @@ func TestRenderTemplate(t *testing.T) {
 	)
 }
 
-// databaseConfigRequest builds a POST request carrying the
-// given expiry as a form value, as the add-target form does.
-func databaseConfigRequest(expiry string) *http.Request {
-	form := url.Values{}
-	if expiry != "" {
-		form.Set("expiry", expiry)
-	}
-
-	req := httptest.NewRequestWithContext(
-		context.Background(), http.MethodPost, "/",
-		strings.NewReader(form.Encode()),
-	)
-	req.Header.Set(
-		"Content-Type", "application/x-www-form-urlencoded",
-	)
-
-	return req
-}
-
 func TestBuildDatabaseTargetConfig_Valid(t *testing.T) {
 	t.Parallel()
 
@@ -220,25 +199,19 @@ func TestBuildDatabaseTargetConfig_Valid(t *testing.T) {
 
 	// Empty expiry: the keep-forever default, empty config.
 	w := httptest.NewRecorder()
-	cfg, err := h.BuildDatabaseTargetConfigForTest(
+	cfg, err := h.BuildDatabaseTargetConfigForTest(w, "")
-		w, databaseConfigRequest(""),
-	)
 	require.NoError(t, err)
 	assert.Empty(t, cfg)
 
 	// Explicit never is stored as config.
 	w = httptest.NewRecorder()
-	cfg, err = h.BuildDatabaseTargetConfigForTest(
+	cfg, err = h.BuildDatabaseTargetConfigForTest(w, "never")
-		w, databaseConfigRequest("never"),
-	)
 	require.NoError(t, err)
 	assert.JSONEq(t, `{"expiry":"never"}`, cfg)
 
 	// A positive duration is stored as config.
 	w = httptest.NewRecorder()
-	cfg, err = h.BuildDatabaseTargetConfigForTest(
+	cfg, err = h.BuildDatabaseTargetConfigForTest(w, "720h")
-		w, databaseConfigRequest("720h"),
-	)
 	require.NoError(t, err)
 	assert.JSONEq(t, `{"expiry":"720h"}`, cfg)
 }
@@ -257,9 +230,7 @@ func TestBuildDatabaseTargetConfig_RejectsBadExpiry(
 
 	for _, bad := range []string{"nonsense", "7d", "-5h"} {
 		w := httptest.NewRecorder()
-		cfg, err := h.BuildDatabaseTargetConfigForTest(
+		cfg, err := h.BuildDatabaseTargetConfigForTest(w, bad)
-			w, databaseConfigRequest(bad),
-		)
 
 		require.Error(t, err, "expiry %q", bad)
 		assert.Empty(t, cfg)

internal/handlers/source_management.go

@@ -816,6 +816,7 @@ func (h *Handlers) processTargetCreate(
 	targetType := database.TargetType(r.FormValue("type"))
 	targetURL := r.FormValue("url")
 	maxRetriesStr := r.FormValue("max_retries")
+	expiry := r.FormValue("expiry")
 
 	if name == "" {
 		http.Error(
@@ -835,7 +836,7 @@ func (h *Handlers) processTargetCreate(
 	}
 
 	configJSON, err := h.buildTargetConfig(
-		w, r, targetType, targetURL,
+		w, r, targetType, targetURL, expiry,
 	)
 	if err != nil {
 		return
@@ -893,11 +894,13 @@ func parseNonNegativeInt(s string) int {
 }
 
 // buildTargetConfig builds the JSON config string for a target.
+// The expiry form value is read by the caller (which bounds the
+// request body) and applies to database targets only.
 func (h *Handlers) buildTargetConfig(
 	w http.ResponseWriter,
 	r *http.Request,
 	targetType database.TargetType,
-	targetURL string,
+	targetURL, expiry string,
 ) (string, error) {
 	switch targetType {
 	case database.TargetTypeHTTP:
@@ -905,7 +908,7 @@ func (h *Handlers) buildTargetConfig(
 	case database.TargetTypeSlack:
 		return h.buildSlackTargetConfig(w, r, targetURL)
 	case database.TargetTypeDatabase:
-		return h.buildDatabaseTargetConfig(w, r)
+		return h.buildDatabaseTargetConfig(w, expiry)
 	case database.TargetTypeLog:
 		return "", nil
 	default:
@@ -1017,16 +1020,16 @@ func (h *Handlers) buildSlackTargetConfig(
 }
 
 // buildDatabaseTargetConfig builds config JSON for a database
-// (archive) target. The optional expiry form value is validated
+// (archive) target. The optional expiry (a form value read by
-// here, at creation time, so an unparseable value is rejected
+// the caller, which bounds the request body) is validated here,
-// with a 400 instead of failing every subsequent delivery. An
+// at creation time, so an unparseable value is rejected with a
-// empty expiry yields an empty config (the keep-forever
+// 400 instead of failing every subsequent delivery. An empty
-// default).
+// expiry yields an empty config (the keep-forever default).
 func (h *Handlers) buildDatabaseTargetConfig(
 	w http.ResponseWriter,
-	r *http.Request,
+	expiry string,
 ) (string, error) {
-	expiry := strings.TrimSpace(r.FormValue("expiry"))
+	expiry = strings.TrimSpace(expiry)
 	if expiry == "" {
 		return "", nil
 	}