Correct release-blocking README and startup-warning inaccuracies (closes #151)
All checks were successful
check / check (push) Successful in 3m0s
All checks were successful
check / check (push) Successful in 3m0s
Publishing this README would have shipped false statements about the product. Corrects the eight items on the issue plus everything a full sweep turned up: the Slack circuit-breaker scope, a nonexistent WAL, the wrong config key for slack targets, six undocumented routes, the conditional /metrics registration, wrong retention bands, wrong shutdown mechanism, and a Quick Start that led a new contributor into a red build. The lockout warning now fires whenever TRUSTED_PROXIES is empty rather than only in production, since the variable it was gated on defaults to dev. Rate-limit keying, the limits and the TRUSTED_PROXIES default are untouched — those belong to #150. What /s/* actually serves was settled empirically rather than by reading: all five of GET/HEAD/POST/PUT/DELETE return 200, pinned by TestStaticServesEveryMethod. Restricting it is filed separately. Independently reviewed after three prior rounds. The reviewer re-derived all fifteen claim-table rows against the code, including every row a previous revision had marked "correct, left alone" and got wrong, and found zero false; then verified every route method-by-method, all twelve environment variables, all nine entity tables, and the package tree against git ls-files. The Quick Start was confirmed by running it in a fresh clone.
This commit was merged in pull request #156.
This commit is contained in:
@@ -422,33 +422,43 @@ func loadFromEnv() (*Config, error) {
|
||||
}, nil
|
||||
}
|
||||
|
||||
// warnSharedRateLimitBucket logs a startup warning when a production
|
||||
// deployment leaves TRUSTED_PROXIES empty.
|
||||
// warnSharedRateLimitBucket logs a startup warning whenever
|
||||
// TRUSTED_PROXIES is empty, in any environment.
|
||||
//
|
||||
// With no trusted proxies every rate limiter keys on the connecting
|
||||
// peer's address. A production deployment is required to run behind a
|
||||
// TLS-terminating reverse proxy, and the peer is then that proxy for
|
||||
// every request, so all clients share one bucket per limiter. The
|
||||
// peer's address. Whether that is harmless or dangerous depends on
|
||||
// what is in front of the process, which this code cannot observe:
|
||||
// with nothing in front, the peer is the client and the limits are
|
||||
// per-client as intended; behind a reverse proxy the peer is the proxy
|
||||
// for every request, so all clients share one bucket per limiter. The
|
||||
// login limiter's bucket is the dangerous one: any remote client can
|
||||
// keep it full, which denies the only administrative login to
|
||||
// everyone until the process restarts.
|
||||
// keep it full, which denies the only administrative login to everyone
|
||||
// until the process restarts.
|
||||
//
|
||||
// The warning is deliberately not gated on WEBHOOKER_ENVIRONMENT. That
|
||||
// variable defaults to dev, so gating on it would silence the warning
|
||||
// for exactly the operator who forgot to configure the deployment —
|
||||
// the case it exists to catch.
|
||||
//
|
||||
// The default of trusting nobody is deliberate — trusting forwarded
|
||||
// headers from arbitrary peers lets any client choose its own bucket —
|
||||
// so this warns rather than failing startup or changing the key.
|
||||
func (c *Config) warnSharedRateLimitBucket(log *slog.Logger) {
|
||||
if !c.IsProd() || len(c.TrustedProxies) > 0 {
|
||||
if len(c.TrustedProxies) > 0 {
|
||||
return
|
||||
}
|
||||
|
||||
log.Warn(
|
||||
"TRUSTED_PROXIES is empty: rate limits key on the "+
|
||||
"connecting peer, so behind the reverse proxy a "+
|
||||
"production deployment runs behind, every client "+
|
||||
"shares one bucket per limit. Any remote client can "+
|
||||
"then keep the login limit full and deny the admin "+
|
||||
"login, the only administrative path, until restart. "+
|
||||
"Set TRUSTED_PROXIES to your reverse proxy's address.",
|
||||
"TRUSTED_PROXIES is empty: every rate limit keys on the "+
|
||||
"connecting peer's address. With nothing proxying to "+
|
||||
"this process that is the client itself and the limits "+
|
||||
"are per-client as intended. Behind a reverse proxy the "+
|
||||
"peer is the proxy on every request, so all clients "+
|
||||
"share one bucket per limit and any remote client can "+
|
||||
"keep the login limit full, denying the admin login — "+
|
||||
"the only administrative path — until restart. If "+
|
||||
"anything proxies to this process, set TRUSTED_PROXIES "+
|
||||
"to its address.",
|
||||
"environment", c.Environment,
|
||||
"trustedProxies", len(c.TrustedProxies),
|
||||
)
|
||||
@@ -491,6 +501,10 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
|
||||
"maintenanceMode", s.MaintenanceMode,
|
||||
"dataDir", s.DataDir,
|
||||
"retentionSweepInterval", s.RetentionSweepInterval.String(),
|
||||
// Logged because a perfectly valid non-positive value here
|
||||
// disables idle expiry entirely, and that is worth showing
|
||||
// back to the operator.
|
||||
"sessionIdleTimeout", s.SessionIdleTimeout.String(),
|
||||
"receiverRateLimit", s.ReceiverRateLimit,
|
||||
"trustedProxies", len(s.TrustedProxies),
|
||||
"hasSentryDSN", s.SentryDSN != "",
|
||||
|
||||
Reference in New Issue
Block a user