feat: merge resolved-peer address checks on every mail dial, strict bounded autoconfig XML, shell-free updater commands and refusal of repeated single-valued query parameters and credential headers

.env.example

@@ -68,7 +68,8 @@ DEPLOYMENT_MODE=self_hosted
 
 # true lets customer webhooks point at http:// and private addresses, which
 # lets any workspace member make the backend reach into your network.
-# Development only. Not forwarded by the shipped docker-compose.yml.
+# Honoured only on a self-hosted or development instance. Not forwarded by the
+# shipped docker-compose.yml.
 # WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS=false
 
 

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) |

docs/content/docs/development/configuration.mdx

@@ -43,7 +43,7 @@ Anything flagged on [Instance health](/development/instance-health/) links back
 | `WARMBLY_CLOUD_URL` | The Warmbly Cloud API a self-hosted instance links to for the hosted warmup pool ([guide](/guides/warmbly-cloud/)). Outbound HTTPS only | `https://api.warmbly.com` | no |
 | `INSTANCE_NAME` | The name shown on the Warmbly Cloud approval page when linking this instance | the hostname | no |
 | `WARMBLY_VERSION` | Version string reported to Warmbly Cloud on link requests | `dev` | no |
-| `WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS` | `true` lets customer webhooks point at `http://` and private addresses. Development only: it lets any workspace member make the backend reach into your internal network | `false` | yes |
+| `WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS` | `true` lets customer webhooks point at `http://` and private addresses. Honoured only on a self-hosted (`DEPLOYMENT_MODE=self_hosted`) or development (`APP_ENV=dev`) instance; a hosted production instance ignores it. It lets any workspace member make the backend reach into your internal network, so only set it where you control that network | `false` | yes |
 
 In Gin release mode, access logs omit successful health probes and internal GET lookups, including an expected missing email-message mapping. Authentication failures, server errors, other missing resources and all mutations remain logged. Gin debug mode keeps every access log. Query strings and credential path parameters remain redacted in both modes; organization audit events are unaffected.
 
@@ -507,6 +507,7 @@ Keep the copies private. GeoLite2 may not be redistributed, which is also why no
 | `ENCRYPTED_KEYS_BACKEND_URL` | Backend base the worker fetches organization keys from | unset | yes |
 | `ENCRYPTED_KEYS_WORKER_TOKEN` | The worker's copy of `INTERNAL_API_TOKEN` | unset | yes |
 | `MAIL_TLS_INSECURE` | Skips certificate verification on mailbox connections | `false` | yes |
+| `MAIL_ALLOW_PRIVATE_HOSTS` | Whether a mailbox's SMTP or IMAP server may resolve to a loopback, private, link-local, CGNAT, multicast or metadata address. Checked on every connection, against the address actually dialled. A hosted production instance always refuses them; a self-hosted or development instance allows them unless this is `false`. A joining node receives the value its control plane resolved | `true` on a self-host or in development, refused on hosted production | yes |
 
 <Callout type="warn" title="An unset key URL is silent">
 An empty `ENCRYPTED_KEYS_BACKEND_URL` or `ENCRYPTED_KEYS_WORKER_TOKEN` lets the worker start, subscribe and never register. There is no log line. The `no_worker_heartbeat` check on [Instance health](/development/instance-health/#no_worker_heartbeat) is what surfaces it.
@@ -516,10 +517,11 @@ Workers hold no database connection by design. Everything relational they need a
 
 ### What a joining node is given
 
-`warmbly join` renders `/etc/warmbly/node.env` from the backend's own environment, so most of this page reaches a node without being set twice. Three things are decided rather than copied:
+`warmbly join` renders `/etc/warmbly/node.env` from the backend's own environment, so most of this page reaches a node without being set twice. Four things are decided rather than copied:
 
 - **The crypto and blob providers are translated.** An instance on `KMS_PROVIDER=aws` hands its nodes `brokered`, and one on `BLOB_PROVIDER=s3` hands them `brokered` too, so no machine in the fleet carries a cloud credential. Providers that need no credential pass through unchanged.
 - **A worker never receives `PRIMARY_DB`.** A consumer does, because it opens Postgres itself. An instance that keeps its DSN in a secret manager rather than the environment has none to send, and the join script says so.
+- **`MAIL_ALLOW_PRIVATE_HOSTS` is resolved.** A node cannot tell a hosted instance from a self-host, so it receives the policy its control plane applies. A node that has not rejoined since an upgrade takes it from `node.local.env`; on a hosted instance, rejoin existing nodes or write `MAIL_ALLOW_PRIVATE_HOSTS=false` there.
 - **Addresses are inherited literally.** `NATS_URL`, `REDIS` and `ENCRYPTED_KEYS_BACKEND_URL` are used exactly as the backend has them, and a value that only resolves inside your container network produces a node that enrols cleanly and reaches nothing.
 
 ### Settings the control plane cannot know

docs/content/docs/development/instance-health.mdx

@@ -166,7 +166,7 @@ The request that loaded this page carried an `X-Forwarded-For` header while `TRU
 
 ### unsafe_webhook_urls
 
-`WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS=true` lets customer webhooks point at `http://` and private addresses, which means any workspace member can make the backend reach into your internal network. Development only. See [webhooks](/guides/webhooks/).
+`WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS=true` lets customer webhooks point at `http://` and private addresses, which means any workspace member can make the backend reach into your internal network. It is honoured only on a self-hosted or development instance and the check fires only where it takes effect. See [webhooks](/guides/webhooks/).
 
 ### tls_verification_off
 

docs/content/docs/development/operator-notifications.mdx

@@ -26,7 +26,7 @@ Use **Test** on a channel to deliver a sample alert immediately. A test ignores
 
 Webhook URLs must be HTTPS and resolve to a publicly routable host. Inline credentials are rejected. This is the same posture customer webhooks use, and it exists so a notification channel cannot be pointed at your internal network.
 
-To point a channel at a host on your own network, set `WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS=true`. That also permits plain HTTP. Only do this on a deployment where you control the network.
+To point a channel at a host on your own network, set `WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS=true`. That also permits plain HTTP. It is honoured only on a self-hosted or development instance; only set it where you control the network.
 
 ### Signing
 

docs/content/docs/development/updates.mdx

@@ -187,10 +187,10 @@ Updater:
 |---|---|---|
 | `UPDATER_TOKEN` | The token it accepts; falls back to `INTERNAL_API_TOKEN`. Refuses to start without one | unset |
 | `UPDATER_MODE` | `image` pins a release tag and pulls; `compose` rebuilds a checkout; `command` runs `UPDATER_COMMAND` | `compose` |
-| `UPDATER_COMMAND` | The build-and-restart command for `command` mode | unset |
+| `UPDATER_COMMAND` | The build-and-restart program for `command` mode: an absolute path, optionally followed by plain space-separated arguments (letters, digits and `_./:=,@%+-`). It runs in `UPDATER_REPO_DIR` without a shell, so put pipes, `&&` and variables in a script and name the script here. The updater refuses to start on any other value | unset |
 | `UPDATER_REPO_DIR` | The checkout, or in image mode the install directory holding the compose file and `.env` | working directory |
-| `UPDATER_REMOTE` | The git remote to fetch and pull from | `origin` |
-| `UPDATER_COMPOSE_PROJECT` | The `-p` the stack was started with | `warmbly` |
+| `UPDATER_REMOTE` | The git remote to fetch and pull from, by name | `origin` |
+| `UPDATER_COMPOSE_PROJECT` | The `-p` the stack was started with: lowercase letters, digits, `_` and `-` | `warmbly` |
 | `UPDATER_COMPOSE_PROFILES` | Extra profiles on every compose call, so its own image is rebuilt | `updater` |
 | `UPDATER_BACKEND_HEALTH_URL` | Polled after the restart until it answers | `http://backend:8080/health` |
 | `UPDATER_FETCH_INTERVAL` | How often it fetches for the commits-behind count | `30m` |

docs/content/docs/guides/mailboxes.mdx

@@ -211,6 +211,8 @@ Both are about the same thing: the credentials never leave the machine, so there
 
 Set the Bridge's own username and password, which it shows in its interface, not your Proton account password. The same applies to any relay on the machine: a Dovecot sharing a host with its worker, or a local sink used for testing.
 
+More generally, hosted Warmbly connects only to mail servers at public addresses, checked against the address each connection actually reaches. A self-hosted instance can reach servers on its own network, and an operator can refuse those too with [`MAIL_ALLOW_PRIVATE_HOSTS=false`](/development/configuration/#workers).
+
 </Fold>
 
 <Fold>

internal/api/handler/fleet_nodes.go

@@ -398,6 +398,8 @@ func renderNodeEnv(nodeID uuid.UUID, role models.NodeRole, region string) string
 	kmsProvider, blobProvider := nodeProviders()
 	fmt.Fprintf(&b, "KMS_PROVIDER=%s\n", kmsProvider)
 	fmt.Fprintf(&b, "BLOB_PROVIDER=%s\n", blobProvider)
+	// Resolved here: a node cannot tell a hosted instance from a self-host on its own.
+	fmt.Fprintf(&b, "MAIL_ALLOW_PRIVATE_HOSTS=%t\n", config.PrivateMailHostsAllowed())
 
 	keys := nodeEnvKeys
 	if role == models.NodeRoleConsumer {

internal/api/handler/fleet_nodes_test.go

@@ -274,6 +274,30 @@ func TestRenderNodeEnvBrokersCredentialProviders(t *testing.T) {
 	}
 }
 
+// A node cannot tell a hosted instance from a self-host, so it is handed the
+// mail-host policy the control plane resolved for itself.
+func TestRenderNodeEnvResolvesPrivateMailHostPolicy(t *testing.T) {
+	setInstanceEnv(t)
+	for _, tc := range []struct {
+		name, mode, appEnv, flag, want string
+	}{
+		{"hosted production refuses", "cloud", "prod", "true", "false"},
+		{"self-host allows", "self_hosted", "prod", "", "true"},
+		{"self-host opted out", "self_hosted", "prod", "false", "false"},
+		{"development allows", "cloud", "dev", "", "true"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Setenv("DEPLOYMENT_MODE", tc.mode)
+			t.Setenv("APP_ENV", tc.appEnv)
+			t.Setenv("MAIL_ALLOW_PRIVATE_HOSTS", tc.flag)
+			env := envLines(t, renderNodeEnv(uuid.New(), models.NodeRoleWorker, ""))
+			if env["MAIL_ALLOW_PRIVATE_HOSTS"] != tc.want {
+				t.Errorf("MAIL_ALLOW_PRIVATE_HOSTS = %q, want %q", env["MAIL_ALLOW_PRIVATE_HOSTS"], tc.want)
+			}
+		})
+	}
+}
+
 // A provider that needs no credential works on a node as it stands, and
 // translating it would break a local install for nothing.
 func TestRenderNodeEnvPassesThroughLocalProviders(t *testing.T) {

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/app/instancecheck/checks_security.go

@@ -157,7 +157,7 @@ func checkDevModePublic(ctx context.Context, d Deps, in Input) *Finding {
 }
 
 func checkUnsafeWebhookURLs(ctx context.Context, d Deps, in Input) *Finding {
-	if !truthy("WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS") {
+	if !config.UnsafeWebhookURLsAllowed() {
 		return nil
 	}
 	return result(CategorySecurity, SeverityWarning, "Unsafe webhook URLs are allowed",

internal/app/instanceconfig/entries.go

@@ -102,9 +102,9 @@ var table = []Entry{
 	},
 	{
 		Key: "WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS", Group: GroupDeployment, RuntimeChangeable: ChangePerRequest,
-		Effect:     "true lets customer webhooks point at http:// and private addresses. Development only.",
+		Effect:     "true lets customer webhooks point at http:// and private addresses. Honoured only on a self-hosted or development instance.",
 		DocsAnchor: docsDeployment,
-		Resolve:    boolOr("WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS", false),
+		Resolve:    func(*Runtime) string { return yesNo(config.UnsafeWebhookURLsAllowed()) },
 	},
 	{
 		Key: "AI_PROVIDER", Group: GroupDeployment, RuntimeChangeable: ChangeBootOnly,
@@ -813,6 +813,12 @@ var table = []Entry{
 		DocsAnchor: docsWorkers,
 		Resolve:    boolOr("MAIL_TLS_INSECURE", false),
 	},
+	{
+		Key: "MAIL_ALLOW_PRIVATE_HOSTS", Group: GroupWorkers, RuntimeChangeable: ChangeBootOnly,
+		Effect:     "false refuses customer mailbox connections to private, loopback and link-local addresses on a self-hosted or development instance. Hosted production always refuses them.",
+		DocsAnchor: docsWorkers,
+		Resolve:    func(*Runtime) string { return yesNo(config.PrivateMailHostsAllowed()) },
+	},
 	{
 		Key: "BOX_GOOGLE_CLIENT_ID", Group: GroupWorkers, RuntimeChangeable: ChangeBootOnly,
 		Effect:     "Your Google Cloud OAuth client for connecting Gmail mailboxes. Needed on the backend and on every worker.",

internal/app/instancesettings/notifications.go

@@ -3,10 +3,10 @@ package instancesettings
 import (
 	"net"
 	"net/url"
-	"os"
 	"strings"
 
 	"github.com/google/uuid"
+	"github.com/warmbly/warmbly/internal/config"
 	"github.com/warmbly/warmbly/internal/pkg/safehttp"
 )
 
@@ -201,7 +201,7 @@ func ValidateChannelURL(raw string) error {
 	if err != nil {
 		return errChannelURL("invalid url")
 	}
-	allowUnsafe := strings.EqualFold(os.Getenv("WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS"), "true")
+	allowUnsafe := config.UnsafeWebhookURLsAllowed()
 	if u.Scheme != "https" && !(allowUnsafe && u.Scheme == "http") {
 		return errChannelURL("url scheme must be https")
 	}

internal/app/webhook/service.go

@@ -17,13 +17,13 @@ import (
 	"net"
 	"net/http"
 	"net/url"
-	"os"
 	"strings"
 	"time"
 
 	"github.com/google/uuid"
 	"github.com/rs/zerolog/log"
 
+	"github.com/warmbly/warmbly/internal/config"
 	"github.com/warmbly/warmbly/internal/infrastructure/cache"
 	"github.com/warmbly/warmbly/internal/models"
 	"github.com/warmbly/warmbly/internal/pkg/safehttp"
@@ -551,16 +551,16 @@ func generateChallenge() (string, error) {
 // ValidateOutboundURL is the exported SSRF/HTTPS guard reused by any subsystem
 // that stores a user-supplied URL we will later POST to (e.g. third-party
 // integration actions such as Discord/generic webhooks). Same policy as the
-// customer-webhook endpoints: HTTPS + publicly-routable host, unless
-// WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS=true for local/self-hosted development.
+// customer-webhook endpoints: HTTPS + publicly-routable host, unless a
+// self-hosted or development instance sets WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS=true.
 func ValidateOutboundURL(raw string) error {
 	return validateURL(raw)
 }
 
 // validateURL keeps malformed entries and obvious SSRF targets out of the
 // table. Public webhook endpoints must use HTTPS and route to public hosts.
-// Local/self-hosted development can set WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS=true
-// to permit HTTP and private targets.
+// A self-hosted or development instance can set WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS=true
+// to permit HTTP and private targets; hosted production ignores it.
 func validateURL(raw string) error {
 	raw = strings.TrimSpace(raw)
 	if raw == "" {
@@ -570,7 +570,7 @@ func validateURL(raw string) error {
 	if err != nil {
 		return invalidInput("invalid url: %v", err)
 	}
-	allowUnsafe := strings.EqualFold(os.Getenv("WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS"), "true")
+	allowUnsafe := config.UnsafeWebhookURLsAllowed()
 	if u.Scheme != "https" && !(allowUnsafe && u.Scheme == "http") {
 		return invalidInput("url scheme must be https")
 	}

internal/client/netbind/netbind.go

@@ -9,11 +9,17 @@
 package netbind
 
 import (
+	"context"
 	"crypto/tls"
+	"errors"
 	"net"
 	"os"
 	"sync"
+	"syscall"
 	"time"
+
+	"github.com/warmbly/warmbly/internal/config"
+	"github.com/warmbly/warmbly/internal/pkg/safehttp"
 )
 
 const (
@@ -46,14 +52,34 @@ func FromEnv() *net.TCPAddr {
 
 // Dialer returns a *net.Dialer bound to the given local address. If local is
 // nil, the env var is consulted. If still unset, the OS default route is used.
+// Unless config.PrivateMailHostsAllowed, it refuses any peer that is not publicly routable.
 func Dialer(local *net.TCPAddr) *net.Dialer {
 	if local == nil {
 		local = FromEnv()
 	}
-	return &net.Dialer{
+	d := &net.Dialer{
 		Timeout:   defaultTimeout,
 		LocalAddr: local,
 	}
+	if !config.PrivateMailHostsAllowed() {
+		d.ControlContext = publicPeerOnly
+	}
+	return d
+}
+
+// ErrPrivateMailHost is returned when a mail connection targets a non-public address.
+var ErrPrivateMailHost = errors.New("mail server address is not publicly routable")
+
+// publicPeerOnly checks the resolved address each connection attempt is about to use.
+func publicPeerOnly(_ context.Context, _, address string, _ syscall.RawConn) error {
+	host, _, err := net.SplitHostPort(address)
+	if err != nil {
+		return err
+	}
+	if safehttp.IsBlockedIP(net.ParseIP(host)) {
+		return ErrPrivateMailHost
+	}
+	return nil
 }
 
 // TLSDialer mirrors Dialer for TLS connections (IMAP, HTTPS).

internal/client/netbind/netbind_test.go

@@ -1,7 +1,9 @@
 package netbind
 
 import (
+	"context"
 	"crypto/tls"
+	"errors"
 	"net"
 	"testing"
 	"time"
@@ -50,3 +52,61 @@ func TestDefaultTimeoutIsReasonable(t *testing.T) {
 		t.Fatalf("defaultTimeout = %v looks wrong", defaultTimeout)
 	}
 }
+
+// A loopback listener stands in for a private mail server.
+func TestDialer_RefusesPrivatePeerOnHostedProduction(t *testing.T) {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = ln.Close() }()
+	go func() {
+		for {
+			c, err := ln.Accept()
+			if err != nil {
+				return
+			}
+			_ = c.Close()
+		}
+	}()
+
+	t.Setenv("DEPLOYMENT_MODE", "cloud")
+	t.Setenv("APP_ENV", "prod")
+	t.Setenv("MAIL_ALLOW_PRIVATE_HOSTS", "true")
+	if _, err := Dialer(nil).Dial("tcp", ln.Addr().String()); !errors.Is(err, ErrPrivateMailHost) {
+		t.Fatalf("hosted dial err = %v, want ErrPrivateMailHost", err)
+	}
+
+	t.Setenv("DEPLOYMENT_MODE", "self_hosted")
+	t.Setenv("MAIL_ALLOW_PRIVATE_HOSTS", "")
+	conn, err := Dialer(nil).Dial("tcp", ln.Addr().String())
+	if err != nil {
+		t.Fatalf("self-host dial err = %v", err)
+	}
+	_ = conn.Close()
+
+	t.Setenv("MAIL_ALLOW_PRIVATE_HOSTS", "false")
+	if _, err := Dialer(nil).Dial("tcp", ln.Addr().String()); !errors.Is(err, ErrPrivateMailHost) {
+		t.Fatalf("self-host opted out dial err = %v, want ErrPrivateMailHost", err)
+	}
+}
+
+func TestPublicPeerOnly(t *testing.T) {
+	for addr, blocked := range map[string]bool{
+		"127.0.0.1:993":       true,
+		"10.0.0.5:587":        true,
+		"169.254.169.254:143": true,
+		"100.64.1.1:25":       true,
+		"[::1]:993":           true,
+		"[fd00::1]:993":       true,
+		"224.0.0.1:993":       true,
+		"0.0.0.0:993":         true,
+		"8.8.8.8:993":         false,
+		"[2606:4700::1]:465":  false,
+	} {
+		err := publicPeerOnly(context.Background(), "tcp", addr, nil)
+		if got := err != nil; got != blocked {
+			t.Errorf("%s: blocked = %v, want %v", addr, got, blocked)
+		}
+	}
+}

internal/config/providers.go

@@ -127,6 +127,34 @@ func MailhostISPDB() bool {
 	return !SelfHosted()
 }
 
+// DevEnv reports whether APP_ENV names a development instance; unset is dev.
+func DevEnv() bool {
+	switch strings.ToLower(strings.TrimSpace(os.Getenv("APP_ENV"))) {
+	case "", "dev", "development", "local":
+		return true
+	}
+	return false
+}
+
+// HostedProduction reports a hosted (not self-host) instance outside development.
+func HostedProduction() bool { return !SelfHosted() && !DevEnv() }
+
+// UnsafeWebhookURLsAllowed reports whether outbound HTTP to customer URLs may use
+// http:// and private addresses. Only a self-hosted or development instance may opt in.
+func UnsafeWebhookURLsAllowed() bool {
+	return isTrue(os.Getenv("WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS")) && !HostedProduction()
+}
+
+// PrivateMailHostsAllowed reports whether mailbox SMTP/IMAP connections may reach
+// non-public addresses. Never on hosted production; a self-hosted or development
+// instance allows them (on-premises mail servers) unless MAIL_ALLOW_PRIVATE_HOSTS=false.
+func PrivateMailHostsAllowed() bool {
+	if HostedProduction() {
+		return false
+	}
+	return !isFalse(os.Getenv("MAIL_ALLOW_PRIVATE_HOSTS"))
+}
+
 // MailvendorSandboxURL points every inbox vendor client at the sandbox's mock
 // vendor API (<url>/<vendor>). Honoured only in development (APP_ENV dev or
 // unset), so a stray value can never send real vendor keys elsewhere.

internal/config/providers_test.go

@@ -27,3 +27,30 @@ func TestMailvendorSandboxURLOnlyInDevelopment(t *testing.T) {
 		}
 	}
 }
+
+// Hosted production ignores both opt-outs; a self-hosted or development instance honours them.
+func TestPrivateAddressOptOutsFollowTheDeployment(t *testing.T) {
+	for _, tc := range []struct {
+		mode, appEnv, flag  string
+		webhooks, mailHosts bool
+	}{
+		{"cloud", "prod", "true", false, false},
+		{"cloud", "prod", "", false, false},
+		{"cloud", "dev", "true", true, true},
+		{"cloud", "dev", "", false, true},
+		{"self_hosted", "prod", "true", true, true},
+		{"self_hosted", "prod", "", false, true},
+		{"self_hosted", "prod", "false", false, false},
+	} {
+		t.Setenv("DEPLOYMENT_MODE", tc.mode)
+		t.Setenv("APP_ENV", tc.appEnv)
+		t.Setenv("WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS", tc.flag)
+		t.Setenv("MAIL_ALLOW_PRIVATE_HOSTS", tc.flag)
+		if got := UnsafeWebhookURLsAllowed(); got != tc.webhooks {
+			t.Errorf("%s/%s flag=%q: webhooks = %v, want %v", tc.mode, tc.appEnv, tc.flag, got, tc.webhooks)
+		}
+		if got := PrivateMailHostsAllowed(); got != tc.mailHosts {
+			t.Errorf("%s/%s flag=%q: mail hosts = %v, want %v", tc.mode, tc.appEnv, tc.flag, got, tc.mailHosts)
+		}
+	}
+}

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())
+	}
+}

internal/pkg/mailhost/autoconfig.go

@@ -79,9 +79,7 @@ func (d *Detector) fetchConfig(ctx context.Context, u, domain string) *Settings
 // document, or nil when it does not offer both over TLS.
 func parseClientConfig(body []byte, domain string) *Settings {
 	var cfg clientConfig
-	dec := xml.NewDecoder(bytes.NewReader(body))
-	dec.Strict = false
-	if err := dec.Decode(&cfg); err != nil {
+	if err := decodeUntrustedXML(body, &cfg); err != nil {
 		return nil
 	}
 	imap, okI := pick(cfg.Provider.Incoming, "imap", domain, imapRank)
@@ -92,6 +90,32 @@ func parseClientConfig(body []byte, domain string) *Settings {
 	return &Settings{SMTP: smtp, IMAP: imap}
 }
 
+var errXMLRefused = errors.New("xml document refused")
+
+// decodeUntrustedXML decodes a bounded, well-formed document that carries no DTD or other directive.
+func decodeUntrustedXML(body []byte, v any) error {
+	if len(body) > maxBody {
+		return errXMLRefused
+	}
+	scan := xml.NewDecoder(bytes.NewReader(body))
+	scan.Strict = true
+	for {
+		tok, err := scan.Token()
+		if errors.Is(err, io.EOF) {
+			break
+		}
+		if err != nil {
+			return err
+		}
+		if _, ok := tok.(xml.Directive); ok {
+			return errXMLRefused
+		}
+	}
+	dec := xml.NewDecoder(bytes.NewReader(body))
+	dec.Strict = true
+	return dec.Decode(v)
+}
+
 // imapRank prefers implicit TLS on 993.
 func imapRank(e Endpoint) int {
 	switch {

internal/pkg/mailhost/mailhost_test.go

@@ -304,6 +304,18 @@ func TestParseClientConfig(t *testing.T) {
 	if parseClientConfig([]byte(bad), "x.example") != nil {
 		t.Fatal("an invalid hostname must be refused")
 	}
+
+	withDTD := `<?xml version="1.0"?><!DOCTYPE clientConfig [<!ENTITY h "mail.host.example">]>` + tls465
+	if parseClientConfig([]byte(withDTD), "x.example") != nil {
+		t.Fatal("a document carrying a DTD must be refused")
+	}
+	entity := strings.ReplaceAll(tls465, "mail.host.example", "&h;")
+	if parseClientConfig([]byte(entity), "x.example") != nil {
+		t.Fatal("an undeclared entity must be refused")
+	}
+	if parseClientConfig([]byte(tls465+strings.Repeat(" ", maxBody)), "x.example") != nil {
+		t.Fatal("an oversized document must be refused")
+	}
 }
 
 func TestDetectAutoconfig(t *testing.T) {

internal/pkg/safehttp/safehttp.go

@@ -19,11 +19,11 @@ import (
 	"net"
 	"net/http"
 	"net/netip"
-	"os"
 	"strings"
 	"time"
 
 	"github.com/rs/zerolog/log"
+	"github.com/warmbly/warmbly/internal/config"
 )
 
 // ErrBlockedAddress is returned when a request targets a non-public address.
@@ -41,11 +41,8 @@ var blockedIPv6 = []netip.Prefix{
 	netip.MustParsePrefix("fec0::/10"),
 }
 
-// allowUnsafe mirrors the existing WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS escape hatch
-// so local/self-hosted development can reach private hosts.
-func allowUnsafe() bool {
-	return strings.EqualFold(os.Getenv("WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS"), "true")
-}
+// allowUnsafe is the WARMBLY_ALLOW_UNSAFE_WEBHOOK_URLS opt-out, honoured only off hosted production.
+func allowUnsafe() bool { return config.UnsafeWebhookURLsAllowed() }
 
 // blockedHostnames are denied before DNS resolution: cloud-metadata service
 // names that resolve to link-local IPs (covered by the IP check too, but denying

internal/updater/compose.go

@@ -119,6 +119,12 @@ func (r *Runner) selfImageID(ctx context.Context) string {
 	return strings.TrimSpace(string(out))
 }
 
+// selfRecreateArgs runs a constant script whose compose arguments arrive as positional parameters.
+func (r *Runner) selfRecreateArgs() []string {
+	args := r.composeArgs("run", "-d", "--rm", "--no-deps", "--entrypoint", "sh", selfService, "-c", `sleep 3; exec docker "$@"`, "sh")
+	return append(args, r.composeArgs("up", "-d", "--no-build", "--no-deps", selfService)...)
+}
+
 // recreateSelf moves the updater onto the image it just built or pulled. It
 // cannot run `compose up updater` in-process (that stops this container half
 // way), so a detached one-off container from the new image does it a few
@@ -137,9 +143,7 @@ func (r *Runner) recreateSelf(ctx context.Context, job *Job) {
 		return
 	}
 	r.logf(job, "updater image changed; recreating the updater itself")
-	inner := fmt.Sprintf("sleep 3; docker %s", strings.Join(r.composeArgs("up", "-d", "--no-build", "--no-deps", selfService), " "))
-	args := r.composeArgs("run", "-d", "--rm", "--no-deps", "--entrypoint", "sh", selfService, "-c", inner)
-	cmd := exec.Command("docker", args...)
+	cmd := exec.Command("docker", r.selfRecreateArgs()...)
 	cmd.Dir = r.cfg.RepoDir
 	if out, err := cmd.CombinedOutput(); err != nil {
 		r.logf(job, "could not schedule the updater's own recreate (ignored): %s", strings.TrimSpace(string(out)))

internal/updater/runner.go

@@ -12,6 +12,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"regexp"
 	"strings"
 	"sync"
 	"time"
@@ -24,8 +25,9 @@ type Config struct {
 	Mode    Mode
 	RepoDir string
 	Remote  string
-	// Command runs in ModeCommand after the checkout moved. It is passed to
-	// sh -c and has to build and restart everything itself.
+	// Command runs in ModeCommand after the checkout moved: an absolute
+	// executable and plain arguments, run without a shell. It has to build and
+	// restart everything itself.
 	Command string
 	// ComposeProject is the -p the stack was started with (the Makefile pins
 	// "warmbly"), ComposeProfiles the extra profiles to activate on top of the
@@ -49,8 +51,9 @@ const healthWait = 6 * time.Minute
 
 // Runner owns the checkout, the one job at a time, and the persisted history.
 type Runner struct {
-	cfg Config
-	git git
+	cfg  Config
+	argv []string
+	git  git
 
 	mu       sync.Mutex
 	checkout *Checkout
@@ -75,23 +78,63 @@ func NewRunner(cfg Config) (*Runner, error) {
 	} else if _, err := os.Stat(filepath.Join(cfg.RepoDir, ".git")); err != nil {
 		return nil, fmt.Errorf("%s is not a git checkout: %w", cfg.RepoDir, err)
 	}
-	if cfg.Mode == ModeCommand && strings.TrimSpace(cfg.Command) == "" {
-		return nil, errors.New("UPDATER_MODE=command needs UPDATER_COMMAND")
+	var argv []string
+	if cfg.Mode == ModeCommand {
+		var err error
+		if argv, err = commandArgv(cfg.Command); err != nil {
+			return nil, err
+		}
+	}
+	if cfg.ComposeProject != "" && !composeProjectRe.MatchString(cfg.ComposeProject) {
+		return nil, fmt.Errorf("UPDATER_COMPOSE_PROJECT %q is not a compose project name", cfg.ComposeProject)
+	}
+	for _, p := range strings.Split(cfg.ComposeProfiles, ",") {
+		if p = strings.TrimSpace(p); p != "" && !composeProfileRe.MatchString(p) {
+			return nil, fmt.Errorf("UPDATER_COMPOSE_PROFILES entry %q is not a compose profile name", p)
+		}
 	}
 	if cfg.Remote == "" {
 		cfg.Remote = "origin"
 	}
+	if !remoteNameRe.MatchString(cfg.Remote) {
+		return nil, fmt.Errorf("UPDATER_REMOTE %q is not a remote name", cfg.Remote)
+	}
 	if cfg.ComposeProject == "" {
 		cfg.ComposeProject = "warmbly"
 	}
 	if cfg.FetchInterval <= 0 {
 		cfg.FetchInterval = 30 * time.Minute
 	}
-	r := &Runner{cfg: cfg, git: git{dir: cfg.RepoDir, remote: cfg.Remote}}
+	r := &Runner{cfg: cfg, argv: argv, git: git{dir: cfg.RepoDir, remote: cfg.Remote}}
 	r.loadState()
 	return r, nil
 }
 
+var (
+	composeProjectRe = regexp.MustCompile(`^[a-z0-9][a-z0-9_-]*$`)
+	composeProfileRe = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_.-]*$`)
+	commandArgRe     = regexp.MustCompile(`^[A-Za-z0-9_./:=,@%+-]+$`)
+	remoteNameRe     = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_.-]*$`)
+)
+
+// commandArgv splits UPDATER_COMMAND into an absolute executable and plain
+// arguments; it is executed directly, never through a shell.
+func commandArgv(raw string) ([]string, error) {
+	argv := strings.Fields(raw)
+	if len(argv) == 0 {
+		return nil, errors.New("UPDATER_MODE=command needs UPDATER_COMMAND")
+	}
+	if !filepath.IsAbs(argv[0]) {
+		return nil, errors.New("UPDATER_COMMAND must start with an absolute path to an executable")
+	}
+	for _, a := range argv {
+		if !commandArgRe.MatchString(a) {
+			return nil, fmt.Errorf("UPDATER_COMMAND argument %q holds a character it cannot take; put shell logic in a script", a)
+		}
+	}
+	return argv, nil
+}
+
 // Start refreshes the checkout state now and then on the fetch interval.
 func (r *Runner) Start(ctx context.Context) {
 	go func() {
@@ -303,7 +346,7 @@ func (r *Runner) runSteps(ctx context.Context, job *Job, req UpdateRequest) erro
 	case ModeCommand:
 		r.step(job, "command")
 		r.logf(job, "running UPDATER_COMMAND")
-		if err := r.exec(ctx, job, r.cfg.RepoDir, nil, "sh", "-c", r.cfg.Command); err != nil {
+		if err := r.exec(ctx, job, r.cfg.RepoDir, nil, r.argv[0], r.argv[1:]...); err != nil {
 			return err
 		}
 	default:

internal/updater/runner_test.go

@@ -0,0 +1,81 @@
+package updater
+
+import (
+	"os"
+	"path/filepath"
+	"reflect"
+	"testing"
+)
+
+func TestCommandArgvTakesAnAbsoluteExecutableAndPlainArguments(t *testing.T) {
+	got, err := commandArgv("  /opt/warmbly/src/scripts/upgrade-bare-metal.sh --env=/etc/warmbly/.env ")
+	if err != nil {
+		t.Fatal(err)
+	}
+	want := []string{"/opt/warmbly/src/scripts/upgrade-bare-metal.sh", "--env=/etc/warmbly/.env"}
+	if !reflect.DeepEqual(got, want) {
+		t.Fatalf("argv = %q, want %q", got, want)
+	}
+
+	for _, raw := range []string{
+		"",
+		"upgrade.sh",
+		"./upgrade.sh",
+		"/bin/upgrade; rm -rf /",
+		"/bin/upgrade && reboot",
+		"/bin/upgrade $(id)",
+		"/bin/upgrade `id`",
+		"/bin/upgrade | tee x",
+		"/bin/upgrade > /tmp/x",
+		"/bin/upgrade 'quoted'",
+	} {
+		if _, err := commandArgv(raw); err == nil {
+			t.Errorf("%q was accepted", raw)
+		}
+	}
+}
+
+func TestNewRunnerValidatesOperatorSettings(t *testing.T) {
+	dir := t.TempDir()
+	if err := os.Mkdir(filepath.Join(dir, ".git"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	base := Config{Mode: ModeCommand, RepoDir: dir, StateDir: t.TempDir(), Command: "/usr/local/bin/upgrade"}
+	if _, err := NewRunner(base); err != nil {
+		t.Fatalf("valid config refused: %v", err)
+	}
+	for name, mutate := range map[string]func(*Config){
+		"shell command":   func(c *Config) { c.Command = "cd /src && make up" },
+		"compose project": func(c *Config) { c.ComposeProject = "warmbly;id" },
+		"compose profile": func(c *Config) { c.ComposeProfiles = "updater,--x" },
+		"remote":          func(c *Config) { c.Remote = "--upload-pack=x" },
+	} {
+		cfg := base
+		mutate(&cfg)
+		if _, err := NewRunner(cfg); err == nil {
+			t.Errorf("%s was accepted", name)
+		}
+	}
+}
+
+func TestComposeNamesAreValidated(t *testing.T) {
+	if !composeProjectRe.MatchString("warmbly") || composeProjectRe.MatchString("warmbly;id") || composeProjectRe.MatchString("-p") {
+		t.Fatal("compose project pattern")
+	}
+	if !composeProfileRe.MatchString("updater") || composeProfileRe.MatchString("--x") || composeProfileRe.MatchString("a b") {
+		t.Fatal("compose profile pattern")
+	}
+}
+
+func TestSelfRecreatePassesComposeArgumentsPositionally(t *testing.T) {
+	r := &Runner{cfg: Config{ComposeProject: "warmbly", ComposeProfiles: "updater"}}
+	got := r.selfRecreateArgs()
+	want := []string{
+		"compose", "-p", "warmbly", "--profile", "updater",
+		"run", "-d", "--rm", "--no-deps", "--entrypoint", "sh", selfService, "-c", `sleep 3; exec docker "$@"`, "sh",
+		"compose", "-p", "warmbly", "--profile", "updater", "up", "-d", "--no-build", "--no-deps", selfService,
+	}
+	if !reflect.DeepEqual(got, want) {
+		t.Fatalf("args = %q\nwant   %q", got, want)
+	}
+}

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");