fix(config): validate env values and clamp unsafe settings

Invalid numeric environment values parsed to zero, an invalid prefixed
value fell through to a stale unprefixed value, and the prefixed boolean
parser rejected on and yes that the unprefixed parser accepted. A
non-positive cache TTL or retry-budget rate could crash or disable a
subsystem.

Parsing now warns and uses the coded default for an invalid value,
prefixed and unprefixed booleans accept the same tokens, and the cache
TTL and retry rate are clamped. A dead per-endpoint circuit config was
removed and a BIND_ADDRESS option was added, defaulting to the current
all-interfaces bind. The pprof server now stops on shutdown.
This commit is contained in:
2026-08-14 11:10:39 +01:00
parent 10e51daab3
commit 044fa41744
3 changed files with 358 additions and 31 deletions
+149 -22
View File
@@ -45,6 +45,11 @@ var (
once sync.Once
tracer *libpack_tracing.TracingSetup
shutdownManager *ShutdownManager
// pprofServer is the optional debug pprof HTTP server started by
// parseConfig when PPROF_PORT is set. It is stored here (rather than
// discarded) so main can register it with shutdownManager and Shutdown
// it cleanly instead of leaking the goroutine (see L2).
pprofServer *http.Server
)
// Shutdown phases for components registered on shutdownManager.
@@ -83,32 +88,78 @@ func getDetailsFromEnv[T any](key string, defaultValue T) T {
}
return any(envutil.Getenv(key, v)).(T)
case int:
if val, ok := os.LookupEnv(prefixedKey); ok {
if intVal, err := strconv.Atoi(val); err == nil {
// An explicitly-empty value (VAR="") is treated as unset -- fall
// through to the next source instead of warning, since there is
// nothing invalid to parse, only a deliberately blank override.
if val, ok := os.LookupEnv(prefixedKey); ok && val != "" {
if intVal, err := strconv.Atoi(strings.TrimSpace(val)); err == nil {
return any(intVal).(T)
}
// C3: the prefixed var is set but unparsable - use the coded
// default. Do NOT fall through to the unprefixed name, or a
// stale/unrelated unprefixed value could silently win.
warnInvalidEnv(prefixedKey, val, v)
return any(v).(T)
}
return any(envutil.GetInt(key, v)).(T)
if val, ok := os.LookupEnv(key); ok && val != "" {
if intVal, err := strconv.Atoi(strings.TrimSpace(val)); err == nil {
return any(intVal).(T)
}
// C2: envutil.GetInt silently returns 0 (not the default) when
// the value is present but non-numeric, because it discards the
// parse error. Parse it ourselves so we can warn and keep the
// coded default instead of silently accepting 0.
warnInvalidEnv(key, val, v)
return any(v).(T)
}
return any(v).(T)
case bool:
if val, ok := os.LookupEnv(prefixedKey); ok {
boolVal := strings.ToLower(val) == "true" || val == "1"
return any(boolVal).(T)
// An explicitly-empty value (VAR="") is treated as unset -- fall
// through to the next source instead of warning, since there is
// nothing invalid to parse, only a deliberately blank override.
if val, ok := os.LookupEnv(prefixedKey); ok && val != "" {
if b, valid := parseBoolEnv(val); valid {
return any(b).(T)
}
// C13: the prefixed and unprefixed bool parsers must agree on
// the same truthy/falsy token sets (true/1/on/yes,
// false/0/off/no). On anything else, warn and use the default
// rather than silently disagreeing with the unprefixed path.
warnInvalidEnv(prefixedKey, val, v)
return any(v).(T)
}
return any(envutil.GetBool(key, v)).(T)
if val, ok := os.LookupEnv(key); ok && val != "" {
if b, valid := parseBoolEnv(val); valid {
return any(b).(T)
}
warnInvalidEnv(key, val, v)
return any(v).(T)
}
return any(v).(T)
case float64:
// envutil has no GetFloat; handle the GMP_ prefixed and plain keys
// directly. Previously float64 env values (CIRCUIT_FAILURE_RATIO,
// CIRCUIT_BACKOFF_MULTIPLIER, RETRY_BUDGET_TOKENS_PER_SEC) fell into
// default and were silently impossible to configure.
if val, ok := os.LookupEnv(prefixedKey); ok {
if f, err := strconv.ParseFloat(val, 64); err == nil {
// An explicitly-empty value (VAR="") is treated as unset -- fall
// through to the next source instead of warning, since there is
// nothing invalid to parse, only a deliberately blank override.
if val, ok := os.LookupEnv(prefixedKey); ok && val != "" {
if f, err := strconv.ParseFloat(strings.TrimSpace(val), 64); err == nil {
return any(f).(T)
}
// C3: prefixed var set but unparsable - use the default, do not
// fall through to the unprefixed name.
warnInvalidEnv(prefixedKey, val, v)
return any(v).(T)
}
if val, ok := os.LookupEnv(key); ok {
if f, err := strconv.ParseFloat(val, 64); err == nil {
if val, ok := os.LookupEnv(key); ok && val != "" {
if f, err := strconv.ParseFloat(strings.TrimSpace(val), 64); err == nil {
return any(f).(T)
}
// C2: unprefixed var set but unparsable - warn and use default.
warnInvalidEnv(key, val, v)
return any(v).(T)
}
return defaultValue
default:
@@ -116,6 +167,46 @@ func getDetailsFromEnv[T any](key string, defaultValue T) T {
}
}
// warnInvalidEnv logs that an environment variable's value could not be
// parsed as its expected type and that the coded default is used instead.
// getDetailsFromEnv runs for many fields before cfg (and its Logger) exists,
// so this prefers cfg.Logger once available and falls back to a throwaway
// libpack_logging.Logger otherwise.
func warnInvalidEnv(varName, rawValue string, defaultValue any) {
msg := &libpack_logging.LogMessage{
Message: "Invalid value for environment variable, using default",
Pairs: map[string]any{"var": varName, "value": rawValue, "default": defaultValue},
}
cfgMutex.RLock()
var logger *libpack_logging.Logger
if cfg != nil {
logger = cfg.Logger
}
cfgMutex.RUnlock()
if logger == nil {
logger = libpack_logging.New()
}
logger.Warning(msg)
}
// parseBoolEnv parses val using the same truthy/falsy token set gookit's
// envutil.GetBool accepts (true/1/on/yes and false/0/off/no), so the
// GMP_-prefixed and unprefixed bool paths in getDetailsFromEnv agree on the
// same input instead of the prefixed path only accepting "true"/"1" (C13).
// ok is false when val matches neither set.
func parseBoolEnv(val string) (parsed bool, ok bool) {
switch strings.ToLower(strings.TrimSpace(val)) {
case "true", "1", "on", "yes":
return true, true
case "false", "0", "off", "no":
return false, true
default:
return false, false
}
}
// validateJWTClaimPath validates JWT claim paths to prevent injection attacks
func validateJWTClaimPath(path string) error {
if path == "" {
@@ -162,6 +253,10 @@ func parseConfig() {
// Server configurations
c.Server.PortGraphQL = getDetailsFromEnv("PORT_GRAPHQL", 8080)
c.Server.PortMonitoring = getDetailsFromEnv("MONITORING_PORT", 9393)
// S6: bind address for the GraphQL proxy / admin API / monitoring
// listeners. Empty default preserves today's "bind all interfaces"
// behavior (":port"); consumers format as "<BindAddress>:<port>".
c.Server.BindAddress = getDetailsFromEnv("BIND_ADDRESS", "")
c.Server.HostGraphQL = getDetailsFromEnv("HOST_GRAPHQL", "http://localhost/")
c.Server.HostGraphQLReadOnly = getDetailsFromEnv("HOST_GRAPHQL_READONLY", "")
// Client configurations
@@ -228,6 +323,17 @@ func parseConfig() {
// Logger setup
c.Logger = libpack_logging.New().SetMinLogLevel(libpack_logging.GetLogLevel(c.LogLevel)).
SetFieldName("timestamp", "ts").SetFieldName("message", "msg").SetShowCaller(false)
// C1: a non-positive CACHE_TTL crashes the cache cleanup ticker
// (time.NewTicker panics on d <= 0). Clamp to the coded default before
// this value ever reaches cache init.
if c.Cache.CacheTTL <= 0 {
c.Logger.Warning(&libpack_logging.LogMessage{
Message: "Invalid CACHE_TTL (must be positive), using default",
Pairs: map[string]any{"requested": c.Cache.CacheTTL, "default": 60},
})
c.Cache.CacheTTL = 60
}
// Health check
c.Server.HealthcheckGraphQL = getDetailsFromEnv("HEALTHCHECK_GRAPHQL_URL", "")
c.Client.GQLClient = graphql.NewConnection()
@@ -345,12 +451,21 @@ func parseConfig() {
c.CircuitBreaker.TripOn4xx = getDetailsFromEnv("CIRCUIT_TRIP_ON_4XX", false) // 4xx are usually client errors
c.CircuitBreaker.BackoffMultiplier = getDetailsFromEnv("CIRCUIT_BACKOFF_MULTIPLIER", 1.0) // No backoff by default
c.CircuitBreaker.MaxBackoffTimeout = getDetailsFromEnv("CIRCUIT_MAX_BACKOFF_TIMEOUT", 300) // 5 minutes max
// Initialize endpoint configs map
c.CircuitBreaker.EndpointConfigs = make(map[string]*EndpointCBConfig)
// Retry budget configuration
c.RetryBudget.Enable = getDetailsFromEnv("RETRY_BUDGET_ENABLE", true)
c.RetryBudget.TokensPerSecond = getDetailsFromEnv("RETRY_BUDGET_TOKENS_PER_SEC", 10.0)
// C14: a non-positive refill rate means the token bucket never refills,
// permanently locking out retries once the initial burst is spent.
// Clamp to the coded default (10.0/s, consistent with the default above
// and with RETRY_BUDGET_MAX_TOKENS's coded default of 100 below).
if c.RetryBudget.TokensPerSecond <= 0 {
c.Logger.Warning(&libpack_logging.LogMessage{
Message: "Invalid RETRY_BUDGET_TOKENS_PER_SEC (must be positive), using default",
Pairs: map[string]any{"requested": c.RetryBudget.TokensPerSecond, "default": 10.0},
})
c.RetryBudget.TokensPerSecond = 10.0
}
c.RetryBudget.MaxTokens = getDetailsFromEnv("RETRY_BUDGET_MAX_TOKENS", 100)
// Request coalescing configuration
@@ -374,16 +489,20 @@ func parseConfig() {
c.Logger.Info(&libpack_logging.LogMessage{
Message: "pprof endpoint listening on " + addr,
})
// L2: keep the server so it can be registered with
// shutdownManager and Shutdown cleanly instead of leaking the
// goroutine for the life of the process.
srv := &http.Server{
Addr: addr,
Handler: nil,
ReadHeaderTimeout: 5 * time.Second,
ReadTimeout: 30 * time.Second,
WriteTimeout: 120 * time.Second,
IdleTimeout: 120 * time.Second,
}
pprofServer = srv
go func(listenAddr string) {
srv := &http.Server{
Addr: listenAddr,
Handler: nil,
ReadHeaderTimeout: 5 * time.Second,
ReadTimeout: 30 * time.Second,
WriteTimeout: 120 * time.Second,
IdleTimeout: 120 * time.Second,
}
if err := srv.ListenAndServe(); err != nil {
if err := srv.ListenAndServe(); err != nil && err != http.ErrServerClosed {
c.Logger.Error(&libpack_logging.LogMessage{
Message: "pprof endpoint failed",
Pairs: map[string]any{"error": err.Error(), "addr": listenAddr},
@@ -663,6 +782,14 @@ func main() {
// traffic-drain phase, before any of the backends above are closed.
shutdownManager.RegisterComponentWithPhase("http-proxy", shutdownPhaseDrainTraffic, shutdownHTTPProxy)
// L2: register the optional pprof debug server (if PPROF_PORT was set)
// so it stops cleanly instead of leaking its listener goroutine forever.
if pprofServer != nil {
shutdownManager.RegisterComponent("pprof-server", func(ctx context.Context) error {
return pprofServer.Shutdown(ctx)
})
}
// Close the Redis cache client's connection pool at shutdown (in-memory
// caches are no-ops). Runs after the traffic drain so in-flight
// CacheLookup/CacheStore calls do not hit a closed Redis client.
+203
View File
@@ -291,3 +291,206 @@ func (suite *Tests) TestIntrospectionEnvironmentConfig() {
})
}
}
// TestGetDetailsFromEnv_InvalidValues covers C2 (an unparsable env value must
// fall back to the coded default, never a silent 0/false) and C13 (the
// GMP_-prefixed and unprefixed bool parsers must accept the same
// true/1/on/yes and false/0/off/no tokens, and agree on the same input).
// Uses t.Setenv so each subtest's environment is reset automatically.
func TestGetDetailsFromEnv_InvalidValues(t *testing.T) {
tests := []struct {
defaultValue any
expected any
name string
key string
envKey string
envValue string
}{
// C2: unprefixed int/float, garbage value falls back to the coded
// default instead of envutil.GetInt's silent 0.
{name: "unprefixed int garbage falls back to default, not 0", key: "TEST_C2_INT", envKey: "TEST_C2_INT", envValue: "not-a-number", defaultValue: 42, expected: 42},
{name: "unprefixed float garbage falls back to default", key: "TEST_C2_FLOAT", envKey: "TEST_C2_FLOAT", envValue: "not-a-number", defaultValue: 2.5, expected: 2.5},
// Sanity: a valid unprefixed value still parses (default-preserving).
{name: "unprefixed int valid value still parses", key: "TEST_C2_INT_OK", envKey: "TEST_C2_INT_OK", envValue: "7", defaultValue: 42, expected: 7},
// C13: unprefixed bool accepts on/yes/off/no (baseline gookit
// behavior, pinned here so a future change cannot silently regress
// the parity the prefixed path is fixed to match).
{name: "unprefixed bool on", key: "TEST_C13_BOOL_ON", envKey: "TEST_C13_BOOL_ON", envValue: "on", defaultValue: false, expected: true},
{name: "unprefixed bool off", key: "TEST_C13_BOOL_OFF", envKey: "TEST_C13_BOOL_OFF", envValue: "off", defaultValue: true, expected: false},
{name: "unprefixed bool yes", key: "TEST_C13_BOOL_YES", envKey: "TEST_C13_BOOL_YES", envValue: "yes", defaultValue: false, expected: true},
{name: "unprefixed bool no", key: "TEST_C13_BOOL_NO", envKey: "TEST_C13_BOOL_NO", envValue: "no", defaultValue: true, expected: false},
// C13: the prefixed path now accepts the same tokens (previously
// only "true"/"1" were accepted, diverging from the unprefixed
// path - security-relevant for e.g. GMP_BLOCK_SCHEMA_INTROSPECTION).
{name: "prefixed bool on", key: "TEST_C13_PFX_ON", envKey: "GMP_TEST_C13_PFX_ON", envValue: "on", defaultValue: false, expected: true},
{name: "prefixed bool off", key: "TEST_C13_PFX_OFF", envKey: "GMP_TEST_C13_PFX_OFF", envValue: "off", defaultValue: true, expected: false},
{name: "prefixed bool yes", key: "TEST_C13_PFX_YES", envKey: "GMP_TEST_C13_PFX_YES", envValue: "yes", defaultValue: false, expected: true},
{name: "prefixed bool no", key: "TEST_C13_PFX_NO", envKey: "GMP_TEST_C13_PFX_NO", envValue: "no", defaultValue: true, expected: false},
// C13/C2: a garbage bool value on either side falls back to the
// coded default instead of silently becoming false.
{name: "prefixed bool garbage falls back to default", key: "TEST_C13_PFX_BAD", envKey: "GMP_TEST_C13_PFX_BAD", envValue: "maybe", defaultValue: true, expected: true},
{name: "unprefixed bool garbage falls back to default", key: "TEST_C13_BAD", envKey: "TEST_C13_BAD", envValue: "maybe", defaultValue: true, expected: true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Setenv(tt.envKey, tt.envValue)
var result any
switch def := tt.defaultValue.(type) {
case int:
result = getDetailsFromEnv(tt.key, def)
case float64:
result = getDetailsFromEnv(tt.key, def)
case bool:
result = getDetailsFromEnv(tt.key, def)
default:
t.Fatalf("unsupported default value type %T", tt.defaultValue)
}
assert.Equal(t, tt.expected, result)
})
}
}
// TestGetDetailsFromEnv_PrefixedInvalidDoesNotFallThrough covers C3: when
// the GMP_-prefixed var is set but fails to parse, getDetailsFromEnv must
// use the coded default and must NOT fall through to a stale/unrelated
// unprefixed value of the same base key.
func TestGetDetailsFromEnv_PrefixedInvalidDoesNotFallThrough(t *testing.T) {
t.Run("int: prefixed invalid ignores stale unprefixed value", func(t *testing.T) {
t.Setenv("TEST_C3_INT", "999") // stale unprefixed value, must not win
t.Setenv("GMP_TEST_C3_INT", "not-a-number") // prefixed set but unparsable
result := getDetailsFromEnv("TEST_C3_INT", 42)
assert.Equal(t, 42, result)
})
t.Run("float64: prefixed invalid ignores stale unprefixed value", func(t *testing.T) {
t.Setenv("TEST_C3_FLOAT", "999.0") // stale unprefixed value, must not win
t.Setenv("GMP_TEST_C3_FLOAT", "not-a-number") // prefixed set but unparsable
result := getDetailsFromEnv("TEST_C3_FLOAT", 4.2)
assert.Equal(t, 4.2, result)
})
}
// TestGetDetailsFromEnv_EmptyValueTreatedAsUnset covers the polish fix: an
// explicitly-empty env var (VAR="") must fall back to the coded default,
// the same as an unset var, without emitting an invalid-value warning --
// there is nothing invalid to parse, only a deliberately blank override.
// Uses t.Setenv so each subtest's environment is reset automatically.
func TestGetDetailsFromEnv_EmptyValueTreatedAsUnset(t *testing.T) {
t.Run("int: empty prefixed value falls back to default", func(t *testing.T) {
t.Setenv("GMP_TEST_EMPTY_INT", "")
result := getDetailsFromEnv("TEST_EMPTY_INT", 42)
assert.Equal(t, 42, result)
})
t.Run("int: empty unprefixed value falls back to default", func(t *testing.T) {
t.Setenv("TEST_EMPTY_INT_UNPFX", "")
result := getDetailsFromEnv("TEST_EMPTY_INT_UNPFX", 42)
assert.Equal(t, 42, result)
})
t.Run("float64: empty prefixed value falls back to default", func(t *testing.T) {
t.Setenv("GMP_TEST_EMPTY_FLOAT", "")
result := getDetailsFromEnv("TEST_EMPTY_FLOAT", 2.5)
assert.Equal(t, 2.5, result)
})
t.Run("bool: empty prefixed value falls back to default", func(t *testing.T) {
t.Setenv("GMP_TEST_EMPTY_BOOL", "")
result := getDetailsFromEnv("TEST_EMPTY_BOOL", true)
assert.Equal(t, true, result)
})
t.Run("int: empty prefixed value still falls through to a valid unprefixed value", func(t *testing.T) {
t.Setenv("GMP_TEST_EMPTY_FALLTHROUGH", "")
t.Setenv("TEST_EMPTY_FALLTHROUGH", "7")
result := getDetailsFromEnv("TEST_EMPTY_FALLTHROUGH", 42)
assert.Equal(t, 7, result)
})
}
// TestGetDetailsFromEnv_FloatTrimSpace covers the polish fix: the float64
// path was missing the strings.TrimSpace that the int and bool paths
// already had, so a value like " 1.5 " failed to parse and silently fell
// back to the coded default instead of parsing to 1.5.
func TestGetDetailsFromEnv_FloatTrimSpace(t *testing.T) {
t.Run("prefixed float with surrounding whitespace parses", func(t *testing.T) {
t.Setenv("GMP_TEST_FLOAT_TRIM", " 1.5 ")
result := getDetailsFromEnv("TEST_FLOAT_TRIM", 2.5)
assert.Equal(t, 1.5, result)
})
t.Run("unprefixed float with surrounding whitespace parses", func(t *testing.T) {
t.Setenv("TEST_FLOAT_TRIM_UNPFX", " 1.5 ")
result := getDetailsFromEnv("TEST_FLOAT_TRIM_UNPFX", 2.5)
assert.Equal(t, 1.5, result)
})
}
// TestParseConfig_ClampsInvalidNumericEnv covers C1 (CACHE_TTL) and C14
// (RETRY_BUDGET_TOKENS_PER_SEC): a non-positive value must clamp to the
// coded default instead of crashing the cache cleanup ticker (C1) or
// permanently locking out retry-budget token refills (C14).
func TestParseConfig_ClampsInvalidNumericEnv(t *testing.T) {
tests := []struct {
checkResult func(t *testing.T, c *config)
name string
envKey string
envValue string
}{
{
name: "CACHE_TTL zero clamps to 60s default",
envKey: "CACHE_TTL",
envValue: "0",
checkResult: func(t *testing.T, c *config) {
assert.Equal(t, 60, c.Cache.CacheTTL)
},
},
{
name: "CACHE_TTL negative clamps to 60s default",
envKey: "CACHE_TTL",
envValue: "-5",
checkResult: func(t *testing.T, c *config) {
assert.Equal(t, 60, c.Cache.CacheTTL)
},
},
{
name: "RETRY_BUDGET_TOKENS_PER_SEC zero clamps to 10/s default",
envKey: "RETRY_BUDGET_TOKENS_PER_SEC",
envValue: "0",
checkResult: func(t *testing.T, c *config) {
assert.Equal(t, 10.0, c.RetryBudget.TokensPerSecond)
},
},
{
name: "RETRY_BUDGET_TOKENS_PER_SEC negative clamps to 10/s default",
envKey: "RETRY_BUDGET_TOKENS_PER_SEC",
envValue: "-1.5",
checkResult: func(t *testing.T, c *config) {
assert.Equal(t, 10.0, c.RetryBudget.TokensPerSecond)
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Setenv(tt.envKey, tt.envValue)
cfgMutex.Lock()
cfg = nil
cfgMutex.Unlock()
parseConfig()
cfgMutex.RLock()
c := cfg
cfgMutex.RUnlock()
tt.checkResult(t, c)
})
}
}
+6 -9
View File
@@ -7,14 +7,6 @@ import (
"github.com/valyala/fasthttp"
)
// EndpointCBConfig holds per-endpoint circuit breaker configuration
type EndpointCBConfig struct {
MaxFailures int // Override max failures for this endpoint
FailureRatio float64 // Override failure ratio for this endpoint
Timeout int // Override timeout for this endpoint
Disabled bool // Disable circuit breaker for this endpoint
}
// config is a struct that holds the configuration of the application.
// It includes settings for logging, monitoring, client connections, security, and server behavior.
type config struct {
@@ -69,6 +61,12 @@ type config struct {
HostGraphQLReadOnly string
HealthcheckGraphQL string
AllowURLs []string // List of allowed URL paths for access control
// BindAddress is the host portion of every listener's bind address
// (GraphQL proxy, admin API, monitoring). Empty (the default)
// preserves the existing ":port" behavior of binding all
// interfaces; set it (e.g. "127.0.0.1") to restrict listeners to a
// single interface. See S6.
BindAddress string
PortGraphQL int
PortMonitoring int
@@ -80,7 +78,6 @@ type config struct {
PurgeOnCrawl bool
}
CircuitBreaker struct {
EndpointConfigs map[string]*EndpointCBConfig // Per-endpoint circuit breaker configurations
ExcludedStatusCodes []int
MaxFailures int
FailureRatio float64