feat: answer a repeated single-valued query parameter with 400 duplicate_query_parameter and a repeated Authorization, X-Warmbly-Token, Idempotency-Key or Origin header with 400 duplicate_header on the API and forms service, keep the documented list parameters repeatable, fold X-Forwarded-For lines and read the last line of the other forwarding headers there and in the tracking service

docs/content/docs/api/error-codes.mdx

@@ -96,6 +96,12 @@ Fields are named by their JSON key, with nested fields as a dotted path (`inner.
 
 Versioned API routes check every query parameter name and value before the endpoint runs, including endpoints that read filters directly. The legacy `/invitations/lookup` alias applies the same validation. NUL characters, invalid UTF-8 and malformed query encoding return `400` rather than sending unsupported text to the database. For example, `?q=%00` returns `Query parameter "q[0]" contains a NUL character, which is not supported.` Valid Unicode text and properly encoded punctuation remain unchanged.
 
+Every route takes each query parameter at most once. A parameter repeated in the query string returns `400` with `code: "duplicate_query_parameter"` and a `request_id`, whatever the values. The parameters documented as lists accept repeats: `status` on `GET /v1/advisor/recommendations`, and `campaign_ids` and `folder_ids` on `GET /v1/analytics/dashboard`. Send any other list as one comma-separated value where the endpoint documents one.
+
+A request carrying more than one `Authorization`, `X-Warmbly-Token`, `Idempotency-Key` or `Origin` header line returns `400` with `code: "duplicate_header"`. Send each of them once. Repeated `X-Forwarded-For` lines are read as one list in arrival order, and for `X-Forwarded-Proto`, `X-Forwarded-Host`, `X-Forwarded-Port` and `X-Real-IP` the last line is used, the one the nearest proxy wrote. Forwarded headers are believed only from [`TRUSTED_PROXIES`](/development/configuration/#network-and-proxy).
+
+The public forms service answers a repeated query parameter or `X-Warmbly-Render` header on its `/api/forms/` routes with `400` and `{"error": "duplicate_query_parameter"}` or `{"error": "duplicate_header"}`.
+
 Resource IDs on email, campaign and contact routes must be UUIDs. Email `tags` and campaign `email_tags` and `folders` arrays also require UUID values. Malformed values return `400` with `code: "bad_request"` and a `request_id` before a database write. Valid UUIDs are normalized and repeated relation IDs are deduplicated; workspace ownership checks still apply.
 
 **How to fix:**
@@ -108,6 +114,8 @@ Resource IDs on email, campaign and contact routes must be UUIDs. Email `tags` a
 
 | `code` | Meaning |
 |--------|---------|
+| `duplicate_query_parameter` | A query parameter appeared more than once on a route that reads it as a single value |
+| `duplicate_header` | `Authorization`, `X-Warmbly-Token`, `Idempotency-Key` or `Origin` was sent on more than one header line |
 | `invalid_lead_status` | `POST /contacts/search` or `POST /contacts/export` was given a `lead_status` that is not one of the documented values |
 | `invalid_sort_by` | `POST /contacts/search`, `POST /contacts/export` or a bulk action's `all` selection was given a `sort_by` of the form `custom:<key>` whose key could never be a custom-field name (letters, numbers, underscores, spaces or dashes) |
 | `invalid_column`, `duplicate_column`, `too_many_columns`, `invalid_sort` | `PUT /me/views/:view` was given a column id that view cannot render, the same column twice, more than 64 columns, or a sort that names neither a sortable contact column nor a well-formed `custom:<key>` (`unibox_rail` has no columns or sort at all) |

internal/api/middleware/request_params.go

@@ -0,0 +1,108 @@
+package middleware
+
+import (
+	"net/http"
+	"net/url"
+	"regexp"
+	"slices"
+	"sort"
+	"strings"
+
+	"github.com/gin-gonic/gin"
+	"github.com/warmbly/warmbly/internal/errx"
+)
+
+// RepeatableQuery lists, by route pattern, the query parameters a handler reads
+// as a list. "*" passes every parameter through (a proxied upstream decides).
+var RepeatableQuery = map[string][]string{
+	"/v1/advisor/recommendations": {"status"},
+	"/v1/analytics/dashboard":     {"campaign_ids", "folder_ids"},
+	"/ingest/*path":               {"*"},
+}
+
+// SingleValuedQuery refuses a query that names a parameter more than once,
+// except one its route reads as a list (RepeatableQuery).
+func SingleValuedQuery() gin.HandlerFunc {
+	return func(c *gin.Context) {
+		if name, dup := DuplicateQueryParam(c.Request.URL.RawQuery, RepeatableQuery[c.FullPath()]); dup {
+			c.Abort()
+			errx.JSON(c, errx.NewWithIdentifier(errx.BadRequest, "duplicate_query_parameter", duplicateMessage("query parameter", name)))
+			return
+		}
+		c.Next()
+	}
+}
+
+// DuplicateQueryParam returns the first parameter raw names more than once that allowed does not list.
+func DuplicateQueryParam(raw string, allowed []string) (string, bool) {
+	if raw == "" {
+		return "", false
+	}
+	if slices.Contains(allowed, "*") {
+		return "", false
+	}
+	values, _ := url.ParseQuery(raw)
+	keys := make([]string, 0, len(values))
+	for k, v := range values {
+		if len(v) > 1 && !slices.Contains(allowed, k) {
+			keys = append(keys, k)
+		}
+	}
+	if len(keys) == 0 {
+		return "", false
+	}
+	sort.Strings(keys)
+	return keys[0], true
+}
+
+// singleValuedHeaders authenticate, authorize or deduplicate a request; a second line is refused.
+var singleValuedHeaders = []string{"Authorization", "X-Warmbly-Token", "Idempotency-Key", "Origin"}
+
+// lastLineHeaders are single-valued forwarding headers; the line the nearest proxy wrote (the last) is kept.
+var lastLineHeaders = []string{"X-Forwarded-Proto", "X-Forwarded-Host", "X-Forwarded-Port", "X-Real-Ip"}
+
+// RequestHeaders refuses a repeated credential, idempotency or origin header,
+// folds repeated X-Forwarded-For lines into one list in arrival order, and keeps
+// only the last line of the other forwarding headers.
+func RequestHeaders() gin.HandlerFunc {
+	return func(c *gin.Context) {
+		h := c.Request.Header
+		for _, name := range singleValuedHeaders {
+			if len(h.Values(name)) > 1 {
+				c.Abort()
+				errx.JSON(c, errx.NewWithIdentifier(errx.BadRequest, "duplicate_header", duplicateMessage("header", name)))
+				return
+			}
+		}
+		normalizeForwardedHeaders(h)
+		c.Next()
+	}
+}
+
+// ForwardedHeaders is the forwarding-header half of RequestHeaders, for a service with its own error shape.
+func ForwardedHeaders() gin.HandlerFunc {
+	return func(c *gin.Context) {
+		normalizeForwardedHeaders(c.Request.Header)
+		c.Next()
+	}
+}
+
+func normalizeForwardedHeaders(h http.Header) {
+	if v := h.Values("X-Forwarded-For"); len(v) > 1 {
+		h.Set("X-Forwarded-For", strings.Join(v, ", "))
+	}
+	for _, name := range lastLineHeaders {
+		if v := h.Values(name); len(v) > 1 {
+			h.Set(name, v[len(v)-1])
+		}
+	}
+}
+
+var plainParamName = regexp.MustCompile(`^[A-Za-z0-9_.\[\]-]{1,64}$`)
+
+func duplicateMessage(kind, name string) string {
+	if plainParamName.MatchString(name) {
+		return kind + " " + name + " may appear only once"
+	}
+	return "a " + kind + " may appear only once"
+}

internal/api/middleware/request_params_test.go

@@ -0,0 +1,104 @@
+package middleware
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/gin-gonic/gin"
+)
+
+func singleValuedRouter(called *bool) *gin.Engine {
+	gin.SetMode(gin.TestMode)
+	r := gin.New()
+	r.Use(RequestIDMiddleware(), RequestHeaders(), SingleValuedQuery())
+	ok := func(c *gin.Context) {
+		*called = true
+		c.Status(http.StatusOK)
+	}
+	r.GET("/v1/campaigns", ok)
+	r.GET("/v1/advisor/recommendations", ok)
+	r.GET("/v1/analytics/dashboard", ok)
+	return r
+}
+
+func TestSingleValuedQueryRefusesRepeatedParameters(t *testing.T) {
+	for _, target := range []string{
+		"/v1/campaigns?cursor=a&cursor=b",
+		"/v1/campaigns?limit=5&q=x&limit=6",
+		"/v1/advisor/recommendations?status=open&limit=1&limit=2",
+		"/v1/analytics/dashboard?campaign_ids=a&from=1&from=2",
+	} {
+		t.Run(target, func(t *testing.T) {
+			called := false
+			w := httptest.NewRecorder()
+			singleValuedRouter(&called).ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil))
+			if called || w.Code != http.StatusBadRequest {
+				t.Fatalf("called = %t, status = %d; want false, 400", called, w.Code)
+			}
+			body := w.Body.String()
+			if !strings.Contains(body, `"duplicate_query_parameter"`) || !strings.Contains(body, `"request_id"`) {
+				t.Errorf("missing stable code or request id: %s", body)
+			}
+		})
+	}
+}
+
+func TestSingleValuedQueryKeepsListParameters(t *testing.T) {
+	for _, target := range []string{
+		"/v1/campaigns?q=one&limit=5",
+		"/v1/advisor/recommendations?status=open&status=snoozed",
+		"/v1/analytics/dashboard?campaign_ids=a&campaign_ids=b&folder_ids=c&folder_ids=d",
+	} {
+		t.Run(target, func(t *testing.T) {
+			called := false
+			w := httptest.NewRecorder()
+			singleValuedRouter(&called).ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil))
+			if !called || w.Code != http.StatusOK {
+				t.Fatalf("called = %t, status = %d; want true, 200", called, w.Code)
+			}
+		})
+	}
+}
+
+func TestRequestHeadersRefusesRepeatedCredentials(t *testing.T) {
+	for _, name := range []string{"Authorization", "X-Warmbly-Token", "Idempotency-Key", "Origin"} {
+		t.Run(name, func(t *testing.T) {
+			called := false
+			req := httptest.NewRequest(http.MethodGet, "/v1/campaigns", nil)
+			req.Header.Add(name, "one")
+			req.Header.Add(name, "two")
+			w := httptest.NewRecorder()
+			singleValuedRouter(&called).ServeHTTP(w, req)
+			if called || w.Code != http.StatusBadRequest || !strings.Contains(w.Body.String(), `"duplicate_header"`) {
+				t.Fatalf("called = %t, status = %d, body = %s", called, w.Code, w.Body.String())
+			}
+		})
+	}
+}
+
+func TestRequestHeadersResolvesForwardingHeaders(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	r := gin.New()
+	if err := r.SetTrustedProxies([]string{"192.0.2.0/24"}); err != nil {
+		t.Fatal(err)
+	}
+	r.Use(RequestIDMiddleware(), RequestHeaders())
+	var ip, proto string
+	r.GET("/", func(c *gin.Context) {
+		ip, proto = c.ClientIP(), c.GetHeader("X-Forwarded-Proto")
+		c.Status(http.StatusOK)
+	})
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req.RemoteAddr = "192.0.2.10:1234"
+	req.Header.Add("X-Forwarded-For", "203.0.113.66")
+	req.Header.Add("X-Forwarded-For", "198.51.100.7")
+	req.Header.Add("X-Forwarded-Proto", "http")
+	req.Header.Add("X-Forwarded-Proto", "https")
+	w := httptest.NewRecorder()
+	r.ServeHTTP(w, req)
+	if w.Code != http.StatusOK || ip != "198.51.100.7" || proto != "https" {
+		t.Fatalf("status = %d, ip = %q, proto = %q; want 200, 198.51.100.7, https", w.Code, ip, proto)
+	}
+}

internal/api/resource_scope_routes_test.go

@@ -39,3 +39,17 @@ func TestReadOnlyPostRoutesExist(t *testing.T) {
 		}
 	}
 }
+
+// A list parameter's route that is renamed would start refusing its repeated values; this keeps the allowlist honest.
+func TestRepeatableQueryRoutesExist(t *testing.T) {
+	r := Run(&handler.Handler{}, &middleware.Handler{}, &middleware.OidcHandler{}, "", gin.TestMode, nil)
+	registered := map[string]bool{}
+	for _, rt := range r.Routes() {
+		registered[rt.Path] = true
+	}
+	for path := range middleware.RepeatableQuery {
+		if !registered[path] {
+			t.Errorf("repeatable-query route %q is not registered", path)
+		}
+	}
+}

internal/api/routes.go

@@ -61,6 +61,7 @@ func Run(
 	r.Use(middleware.SecurityHeaders())
 
 	r.Use(middleware.RequestIDMiddleware())
+	r.Use(middleware.RequestHeaders(), middleware.SingleValuedQuery())
 	r.Use(middleware.APIVersionMiddleware(middleware.APIVersion))
 
 	r.GET("/health", func(c *gin.Context) {

internal/formserver/server.go

@@ -140,7 +140,7 @@ func (s *Server) Router(trustedProxies []string) (*gin.Engine, error) {
 		gin.SetMode(gin.ReleaseMode)
 	}
 	r := gin.New()
-	r.Use(middleware.RequestLogger(), gin.Recovery(), middleware.PageHeaders(formsHost()))
+	r.Use(middleware.ForwardedHeaders(), middleware.RequestLogger(), gin.Recovery(), middleware.PageHeaders(formsHost()))
 	// Same posture as the backend: trust no proxy unless the operator names
 	// it, so a forged X-Forwarded-For cannot dodge the submit limiter.
 	if len(trustedProxies) > 0 {
@@ -174,13 +174,26 @@ func (s *Server) Router(trustedProxies []string) (*gin.Engine, error) {
 
 	// Every JSON call needs the render token minted with the page shell, so
 	// scripted clients that never load the page get nothing.
-	api := r.Group("/api", s.requireRenderToken)
+	api := r.Group("/api", singleValued, s.requireRenderToken)
 	api.GET("/forms/:publicID", s.GetPublicForm)
 	api.POST("/forms/:publicID/submit", s.SubmitForm)
 	api.POST("/forms/:publicID/events", s.RecordFormEvent)
 	return r, nil
 }
 
+// singleValued refuses a repeated query parameter or render token; no forms route reads a list.
+func singleValued(c *gin.Context) {
+	if _, dup := middleware.DuplicateQueryParam(c.Request.URL.RawQuery, nil); dup {
+		c.AbortWithStatusJSON(http.StatusBadRequest, gin.H{"error": "duplicate_query_parameter"})
+		return
+	}
+	if len(c.Request.Header.Values("X-Warmbly-Render")) > 1 {
+		c.AbortWithStatusJSON(http.StatusBadRequest, gin.H{"error": "duplicate_header"})
+		return
+	}
+	c.Next()
+}
+
 // requireRenderToken rejects API calls whose X-Warmbly-Render token is
 // missing, forged, expired or minted for another form. The app treats the
 // stale_page error as "refresh this tab".

internal/formserver/server_test.go

@@ -481,3 +481,23 @@ func TestFormShellStampsBrowserReporting(t *testing.T) {
 		}
 	}
 }
+
+func TestFormServerRefusesRepeatedParametersAndRenderTokens(t *testing.T) {
+	_, srv, r := newFixture(t, 0)
+	token := mintFor(srv, "pubtest123")
+
+	if w := get(r, "/api/forms/pubtest123?ref=a&ref=b", token); w.Code != http.StatusBadRequest || !strings.Contains(w.Body.String(), "duplicate_query_parameter") {
+		t.Fatalf("repeated query status %d: %s", w.Code, w.Body.String())
+	}
+	req := httptest.NewRequest(http.MethodGet, "/api/forms/pubtest123", nil)
+	req.Header.Add("X-Warmbly-Render", token)
+	req.Header.Add("X-Warmbly-Render", mintFor(srv, "otherform"))
+	w := httptest.NewRecorder()
+	r.ServeHTTP(w, req)
+	if w.Code != http.StatusBadRequest || !strings.Contains(w.Body.String(), "duplicate_header") {
+		t.Fatalf("repeated render token status %d: %s", w.Code, w.Body.String())
+	}
+	if w := get(r, "/api/forms/pubtest123?ref=a", token); w.Code != http.StatusOK {
+		t.Fatalf("single query status %d: %s", w.Code, w.Body.String())
+	}
+}

tracking/src/handlers.rs

@@ -734,8 +734,11 @@ fn client_ip(
     if !peer_is_trusted(peer, trusted) {
         return peer_ip.to_string();
     }
+    // A repeated header is read from its last line, the one the nearest proxy wrote.
     let raw = headers
-        .get(header)
+        .get_all(header)
+        .iter()
+        .next_back()
         .and_then(|h| h.to_str().ok())
         .map(str::trim)
         .filter(|v| !v.is_empty());
@@ -987,6 +990,13 @@ mod tests {
             "198.51.100.7"
         );
         assert_eq!(client_ip(peer, &h, &trusted, "cf-connecting-ip"), "8.8.8.8");
+        // A repeated header line is read from the one the proxy added last.
+        let mut h = hdr(&[("x-forwarded-for", "1.1.1.1")]);
+        h.append("x-forwarded-for", "198.51.100.7".parse().unwrap());
+        assert_eq!(
+            client_ip(peer, &h, &trusted, "x-forwarded-for"),
+            "198.51.100.7"
+        );
         // Garbage or a missing header falls back to the proxy itself.
         let h = hdr(&[("x-forwarded-for", "not an ip")]);
         assert_eq!(client_ip(peer, &h, &trusted, "x-forwarded-for"), "10.1.2.3");