fix(security): close still-valid Copilot review concerns from merged PRs

Triage of Copilot comments on PRs #38–#43 (merged) identified seven
items where the concern is still valid in main. Bundled here rather
than split into separate per-finding PRs.

Dashboard (gearbox/):
- login.go: isSafeReturnURL now rejects ASCII control characters
  (< 0x20, 0x7F). Without this, %0d%0a-encoded paths and embedded tabs
  could survive the open-redirect allowlist and feed into downstream
  header-injection attempts (P0-4 follow-up from PR #39).
- security_headers.go: validCSPDirective now allows base64 chars
  (`+`, `=`) so nonce-... / sha256/384/512-... source expressions
  aren't silently dropped. Both sanitizers now use a shared
  cspContainsForbidden helper that rejects ALL ASCII control chars
  and any non-ASCII rune, catching form-feed and unicode whitespace
  that the previous hand-curated ContainsAny lists missed
  (P1-4 follow-up from PR #43).

Agent (gearbox-agent/):
- parser.go: sanitizeName output is now capped at 25 chars so the
  default backend name {app}_{service}_backend stays inside
  validBackendName's 63-char limit. A long app or service directory
  name would otherwise build a default that fails validation and
  silently drops the whole backend (P0-1 follow-up from PR #39).
- websocket.go: canonicalHost uses net.SplitHostPort instead of
  strings.Index(":"), which corrupted IPv6 host parsing
  ("[::1]:8405" was treated as host="[" / port=":1]:8405").
  (P1-5 follow-up from PR #42).
- backoff.go: NewBackoffTracker now clamps zero/negative window,
  baseDelay, threshold to safe minimums. cleanupLoop's
  time.NewTicker would otherwise panic on a zero window
  (P1-7 follow-up from PR #42).
- ratelimit.go: "map at capacity" warn log is throttled to one per
  minute. Without throttling, a distributed scan that fills the
  client map drove a log-write per denied IP, turning the
  structured-log pipeline into a secondary disk/ingest DoS surface
  (P1-6 follow-up from PR #42).
- fail2ban.go: comment said "unit argument" but the code handles
  fail2ban jails — wording drift left over from copy/paste of a
  systemctl-related comment (P1-3 follow-up from PR #41).

Skipped (not material in this deployment):
- APIKeyAuth using its own IP extractor rather than the rate limiter's
  (only diverges behind a trusted proxy; we're not).
- Retry-After hardcoded to 60 (UX nit, not security).
- Fork-PR handling in claude-security-review workflow (no fork PRs in
  this repo).
- Historical "recommended fix order" wording in 2026-05-findings.md
  (the section is explicitly retrospective).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: sarg3nt

gearbox-agent/internal/api/websocket.go

@@ -3,6 +3,7 @@ package api
 import (
 	"encoding/json"
 	"log/slog"
+	"net"
 	"net/http"
 	"net/url"
 	"os"
@@ -50,18 +51,24 @@ func canonicalOrigin(s string) (string, bool) {
 // scheme) for comparison against canonicalOrigin output. The scheme is
 // derived from r.TLS at the call site, since the Host header itself carries
 // no scheme. Same default-port stripping rules as canonicalOrigin.
+//
+// IPv6 hosts are bracketed in Host headers ("[::1]:8405") — net.SplitHostPort
+// strips the brackets correctly; a hand-rolled strings.Index(":") parser
+// would split on the first ':' inside the address and corrupt the comparison.
+// 2026-05 audit P1-5 follow-up (Copilot review on PR #42).
 func canonicalHost(scheme, hostPort string) string {
 	scheme = strings.ToLower(scheme)
-	host := strings.ToLower(hostPort)
-	if i := strings.Index(host, ":"); i >= 0 {
-		port := host[i+1:]
-		host = host[:i]
-		if (scheme == "https" && port == "443") || (scheme == "http" && port == "80") {
-			return scheme + "://" + host
-		}
-		return scheme + "://" + host + ":" + port
+	host, port, err := net.SplitHostPort(strings.ToLower(hostPort))
+	if err != nil {
+		// No port — the whole value is the host. Strip surrounding
+		// brackets in case it's a bare IPv6 literal.
+		host = strings.Trim(strings.ToLower(hostPort), "[]")
+		return scheme + "://" + host
+	}
+	if (scheme == "https" && port == "443") || (scheme == "http" && port == "80") {
+		return scheme + "://" + host
 	}
-	return scheme + "://" + host
+	return scheme + "://" + host + ":" + port
 }
 
 const (

gearbox-agent/internal/framework/middleware/backoff.go

@@ -47,7 +47,24 @@ type failureRecord struct {
 //	window    = 5 minutes
 //	baseDelay = 10 seconds
 //	maxDelay  = 30 minutes
+//
+// Invalid inputs (non-positive durations or threshold) are clamped to
+// safe defaults rather than panicking — cleanupLoop's time.NewTicker
+// would otherwise panic on a zero/negative window. 2026-05 audit P1-7
+// follow-up (Copilot review on PR #42).
 func NewBackoffTracker(threshold int, window, baseDelay, maxDelay time.Duration, logger *slog.Logger) *BackoffTracker {
+	if threshold < 1 {
+		threshold = 1
+	}
+	if window <= 0 {
+		window = 5 * time.Minute
+	}
+	if baseDelay <= 0 {
+		baseDelay = 10 * time.Second
+	}
+	if maxDelay < baseDelay {
+		maxDelay = baseDelay
+	}
 	bt := &BackoffTracker{
 		failures:    make(map[string]*failureRecord),
 		threshold:   threshold,

gearbox-agent/internal/framework/middleware/ratelimit.go

@@ -36,8 +36,19 @@ type RateLimiter struct {
 	logger      *slog.Logger
 	trustProxy  bool          // Whether to trust X-Forwarded-For headers
 	stopCleanup chan struct{} // Signal to stop cleanup goroutine
+
+	// capacityWarnInterval throttles the "map at capacity" warning so a
+	// distributed scanner can't turn the structured-log pipeline into a
+	// disk/ingest amplification vector. 2026-05 audit P1-6 follow-up
+	// (Copilot review on PR #42).
+	lastCapacityWarn time.Time
 }
 
+// capacityWarnInterval is the minimum time between "map at capacity"
+// warnings. One per minute is enough for ops visibility (the condition
+// is sticky once it starts) and slow enough to make log-DoS impractical.
+const capacityWarnInterval = time.Minute
+
 type clientBucket struct {
 	tokens     float64
 	lastUpdate time.Time
@@ -90,11 +101,19 @@ func (rl *RateLimiter) Allow(ip string) bool {
 				// Still full after premature cleanup. Deny rather than
 				// allocate. The denied IP gets back in next time someone
 				// else's bucket goes stale.
-				if rl.logger != nil {
+				//
+				// Throttle the warn log: under a distributed scan this
+				// fires on every new IP, so unbounded logging would let
+				// the attacker drive disk/ingest cost (a secondary DoS).
+				// One warning per capacityWarnInterval is enough — the
+				// condition is sticky once it starts.
+				if rl.logger != nil && now.Sub(rl.lastCapacityWarn) >= capacityWarnInterval {
 					rl.logger.Warn("Rate limiter map at capacity; denying new client",
 						"ip", ip,
 						"map_size", len(rl.clients),
+						"throttled_until", now.Add(capacityWarnInterval).Format(time.RFC3339),
 					)
+					rl.lastCapacityWarn = now
 				}
 				return false
 			}

gearbox-agent/internal/framework/services/compose/parser.go

@@ -384,15 +384,30 @@ func (p *Parser) parseDependsOn(dependsOn any) []string {
 }
 
 // sanitizeName sanitizes a name for use in HAProxy backend names.
+//
+// The output is capped at maxSanitizedNameLen so that the default backend
+// name built by callers (`{app}_{service}_backend`, ~8 chars of fixed
+// suffix + 1 separator) reliably fits inside validBackendName's 63-char
+// cap. Without this, a very long app or service directory name would
+// build a default that fails the validator and silently drops the whole
+// backend. 2026-05 audit P0-1 follow-up (Copilot review on PR #39).
 func sanitizeName(name string) string {
+	const maxSanitizedNameLen = 25 // 25 + 1 + 25 + 1 + "backend" (7) = 59 ≤ 63
 	// Replace invalid characters with underscore
 	re := regexp.MustCompile(`[^a-zA-Z0-9_.-]`)
 	sanitized := re.ReplaceAllString(name, "_")
 	// Replace multiple consecutive underscores
 	re = regexp.MustCompile(`_+`)
 	sanitized = re.ReplaceAllString(sanitized, "_")
 	// Remove leading/trailing underscores
-	return strings.Trim(sanitized, "_")
+	sanitized = strings.Trim(sanitized, "_")
+	if len(sanitized) > maxSanitizedNameLen {
+		sanitized = sanitized[:maxSanitizedNameLen]
+		// Trim a trailing separator that the truncation may have exposed
+		// (e.g. ".my-app." → ".my-app").
+		sanitized = strings.TrimRight(sanitized, "_.-")
+	}
+	return sanitized
 }
 
 // getOrDefault returns the value for a key or a default if not present.

gearbox-agent/internal/gears/security/fail2ban.go

@@ -158,7 +158,7 @@ func (c *Fail2BanCollector) getJails() ([]string, error) {
 // getJailStats returns statistics for a specific jail.
 //
 // Defense-in-depth: the jail name is verified to match validJailName before
-// being passed to fail2ban-client, and "--" separates flags from the unit
+// being passed to fail2ban-client, and "--" separates flags from the jail
 // argument. Today jailName always comes from getJails() (which parses
 // fail2ban-client's own output), but a malformed jail configuration on the
 // host could otherwise produce a value that fail2ban-client itself would

gearbox/internal/framework/handler/login.go

@@ -34,7 +34,15 @@ func isSafeReturnURL(s string) bool {
 		return false
 	}
 	for i := 0; i < len(s); i++ {
-		if s[i] == '\\' {
+		c := s[i]
+		if c == '\\' {
+			return false
+		}
+		// Reject ASCII control characters (including \r, \n, \t, NUL,
+		// DEL). Paths containing these are not valid URLs and have been
+		// used in header-injection / open-redirect sneak paths. 2026-05
+		// audit P0-4 follow-up (Copilot review on PR #39).
+		if c < 0x20 || c == 0x7f {
 			return false
 		}
 	}

gearbox/internal/framework/middleware/security_headers.go

@@ -19,8 +19,28 @@ import (
 // or newline. We keep the validator deliberately conservative: only
 // alphanumerics, a small punctuation set, and quoted keywords.
 //
+// `+` and `=` are included so nonce-... and sha256/sha384/sha512-...
+// expressions whose payload uses the standard base64 alphabet aren't
+// silently dropped. 2026-05 audit P1-4 follow-up (Copilot review on
+// PR #43).
+//
 // 2026-05 audit P1-4.
-var validCSPDirective = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9-]+(\s+[a-zA-Z0-9'_:/.\-*]+)+$`)
+var validCSPDirective = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9-]+(\s+[a-zA-Z0-9'_:/.\-*+=]+)+$`)
+
+// cspContainsForbidden returns true when s contains any byte that has no
+// business appearing in a CSP header value. A hand-curated whitespace
+// list misses codepoints like form-feed (\f) and the various unicode
+// space chars, so we reject ASCII control chars (< 0x20, 0x7F) and any
+// rune outside ASCII as a precaution against header smuggling via the
+// CSP env vars. 2026-05 audit P1-4 follow-up (Copilot review on PR #43).
+func cspContainsForbidden(s string) bool {
+	for _, r := range s {
+		if r < 0x20 || r == 0x7f || r > 0x7e {
+			return true
+		}
+	}
+	return false
+}
 
 // sanitizeCSPExtraSource accepts a single entry from the
 // CSP_EXTRA_SOURCES comma-separated env var and returns the trimmed
@@ -33,7 +53,7 @@ func sanitizeCSPExtraSource(s string) (string, bool) {
 	if trimmed == "" {
 		return "", false
 	}
-	if strings.ContainsAny(trimmed, ";\r\n\t") {
+	if strings.ContainsAny(trimmed, ";") || cspContainsForbidden(trimmed) {
 		return "", false
 	}
 	if !validCSPDirective.MatchString(trimmed) {
@@ -52,7 +72,7 @@ func sanitizeCSPReportURI(s string) (string, bool) {
 	if trimmed == "" {
 		return "", false
 	}
-	if strings.ContainsAny(trimmed, " \t\r\n;") {
+	if strings.ContainsAny(trimmed, " ;") || cspContainsForbidden(trimmed) {
 		return "", false
 	}
 	u, err := url.Parse(trimmed)