diff --git a/docs/content/docs/api/error-codes.mdx b/docs/content/docs/api/error-codes.mdx index a9f1e7928..371f580c0 100644 --- a/docs/content/docs/api/error-codes.mdx +++ b/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:` 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:` (`unibox_rail` has no columns or sort at all) | diff --git a/internal/api/middleware/request_params.go b/internal/api/middleware/request_params.go new file mode 100644 index 000000000..1ff2100c4 --- /dev/null +++ b/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" +} diff --git a/internal/api/middleware/request_params_test.go b/internal/api/middleware/request_params_test.go new file mode 100644 index 000000000..7b5f7f4e8 --- /dev/null +++ b/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) + } +} diff --git a/internal/api/resource_scope_routes_test.go b/internal/api/resource_scope_routes_test.go index 93534b06a..dbf511c6a 100644 --- a/internal/api/resource_scope_routes_test.go +++ b/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) + } + } +} diff --git a/internal/api/routes.go b/internal/api/routes.go index bde647420..5f16c5b43 100644 --- a/internal/api/routes.go +++ b/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) { diff --git a/internal/formserver/server.go b/internal/formserver/server.go index fdda618c9..077a546e8 100644 --- a/internal/formserver/server.go +++ b/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". diff --git a/internal/formserver/server_test.go b/internal/formserver/server_test.go index b3736b4d7..f41a4c6c1 100644 --- a/internal/formserver/server_test.go +++ b/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()) + } +} diff --git a/tracking/src/handlers.rs b/tracking/src/handlers.rs index 83aeebeca..25ede25e7 100644 --- a/tracking/src/handlers.rs +++ b/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");