Rate-limit the public webhook receiver endpoint (closes #64)
All checks were successful
check / check (push) Successful in 5s
All checks were successful
check / check (push) Successful in 5s
The receiver was the one unauthenticated, internet-facing endpoint with no rate limit, so a misbehaving or hostile sender could flood a webhook without bound. RECEIVER_RATE_LIMIT (default 120/min) now caps it, keyed on client IP plus entrypoint path so one entrypoint cannot exhaust another's budget. Over-limit requests get 429 with Retry-After. The limiter deliberately does not reuse postRateLimit: that helper is POST-only and keys on IP alone, whereas the receiver must count every method. A test locks that property in. Config parsing follows the fail-loudly idiom: a set-but-unparseable or non-positive value aborts startup rather than falling back to the default. Known limitation, tracked in #88: the key still trusts forwarded headers unconditionally, so the limit is evadable by rotating X-Forwarded-For until trusted-proxy gating lands.
This commit was merged in pull request #87.
This commit is contained in:
31
README.md
31
README.md
@@ -94,6 +94,7 @@ TTY detection, and security headers are always applied.
|
||||
| `METRICS_PASSWORD` | Basic auth password for `/metrics` | `""` |
|
||||
| `SENTRY_DSN` | Sentry error reporting DSN | `""` |
|
||||
| `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` |
|
||||
| `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint | `120` |
|
||||
|
||||
Sessions are bounded by two independent clocks, and end at whichever
|
||||
one runs out first:
|
||||
@@ -123,7 +124,8 @@ fatal configuration error: webhooker logs the offending variable and
|
||||
its value and refuses to start, rather than silently running with a
|
||||
substituted default. `PORT=eighty`, `DEBUG=ture`, and
|
||||
`RETENTION_SWEEP_INTERVAL=1 hour` all abort startup. `PORT` must
|
||||
additionally be a number in the range 1–65535.
|
||||
additionally be a number in the range 1–65535, and
|
||||
`RECEIVER_RATE_LIMIT` must be at least 1.
|
||||
|
||||
Boolean variables (`DEBUG`, `MAINTENANCE_MODE`) accept exactly the
|
||||
spellings Go's `strconv.ParseBool` accepts — `1`, `t`, `T`, `TRUE`,
|
||||
@@ -785,17 +787,24 @@ just delayed until the target is healthy again.
|
||||
|
||||
### Rate Limiting
|
||||
|
||||
Global rate limiting middleware (e.g., per-IP throttling applied at the
|
||||
router level) **must not** apply to webhook receiver endpoints. Webhook
|
||||
endpoints receive automated traffic from external services at
|
||||
unpredictable rates, and blanket rate limits would cause legitimate
|
||||
deliveries to be dropped.
|
||||
Global blanket rate limiting middleware (e.g., a per-IP throttle shared
|
||||
with the web UI) **must not** apply to webhook receiver endpoints.
|
||||
Webhook endpoints receive automated traffic from external services at
|
||||
unpredictable rates, and blanket limits shared with other routes would
|
||||
cause legitimate deliveries to be dropped.
|
||||
|
||||
Instead, each webhook has its own individually configurable rate limit,
|
||||
applied within the webhook handler itself. By default, no rate limit is
|
||||
applied — webhook endpoints accept traffic as fast as it arrives. Rate
|
||||
limits can be configured per-webhook when needed (e.g., to protect
|
||||
against a misbehaving sender).
|
||||
The receiver instead has its own dedicated abuse limit, scoped to the
|
||||
`/webhook/{uuid}` route only and keyed per client IP per entrypoint: one
|
||||
misbehaving sender is throttled without affecting other senders of the
|
||||
same entrypoint or the same sender's other entrypoints. The limit is
|
||||
`RECEIVER_RATE_LIMIT` requests per minute (default 120, generous for
|
||||
legitimate webhook senders). Requests over the limit receive HTTP 429
|
||||
with a `Retry-After` header. A set-but-invalid `RECEIVER_RATE_LIMIT`
|
||||
value aborts startup rather than silently falling back to the default.
|
||||
|
||||
Finer-grained per-webhook rate limits (configured in the web UI and
|
||||
enforced in the webhook handler) can layer on top of this env-level
|
||||
abuse limit later; they are tracked as future work.
|
||||
|
||||
### API Endpoints
|
||||
|
||||
|
||||
@@ -35,6 +35,13 @@ const (
|
||||
// authenticated activity before it expires.
|
||||
defaultSessionIdleTimeout = 24 * time.Hour
|
||||
|
||||
// defaultReceiverRateLimit is the default number of requests
|
||||
// per minute each client IP may send to a single webhook
|
||||
// receiver entrypoint. Generous for legitimate webhook
|
||||
// senders while bounding abuse of the one unauthenticated,
|
||||
// internet-exposed endpoint.
|
||||
defaultReceiverRateLimit = 120
|
||||
|
||||
// maxPort is the highest valid TCP port number. The lower
|
||||
// bound (at least 1) is enforced by envPositiveInt.
|
||||
maxPort = 65535
|
||||
@@ -79,6 +86,10 @@ type Config struct {
|
||||
// which a session expires. Non-positive disables idle expiry.
|
||||
SessionIdleTimeout time.Duration
|
||||
|
||||
// ReceiverRateLimit is the number of requests per minute each
|
||||
// client IP may send to a single webhook receiver entrypoint.
|
||||
ReceiverRateLimit int
|
||||
|
||||
params *ConfigParams
|
||||
log *slog.Logger
|
||||
}
|
||||
@@ -263,6 +274,14 @@ func loadFromEnv() (*Config, error) {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
receiverRateLimit, err := envPositiveInt(
|
||||
"RECEIVER_RATE_LIMIT",
|
||||
defaultReceiverRateLimit,
|
||||
)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
return &Config{
|
||||
DataDir: envString("DATA_DIR"),
|
||||
Debug: debug,
|
||||
@@ -274,6 +293,7 @@ func loadFromEnv() (*Config, error) {
|
||||
SentryDSN: envString("SENTRY_DSN"),
|
||||
RetentionSweepInterval: retentionSweepInterval,
|
||||
SessionIdleTimeout: sessionIdleTimeout,
|
||||
ReceiverRateLimit: receiverRateLimit,
|
||||
}, nil
|
||||
}
|
||||
|
||||
@@ -314,6 +334,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
|
||||
"maintenanceMode", s.MaintenanceMode,
|
||||
"dataDir", s.DataDir,
|
||||
"retentionSweepInterval", s.RetentionSweepInterval.String(),
|
||||
"receiverRateLimit", s.ReceiverRateLimit,
|
||||
"hasSentryDSN", s.SentryDSN != "",
|
||||
"hasMetricsAuth",
|
||||
s.MetricsUsername != "" && s.MetricsPassword != "",
|
||||
|
||||
@@ -14,6 +14,14 @@ import (
|
||||
"sneak.berlin/go/webhooker/internal/logger"
|
||||
)
|
||||
|
||||
// Shared subtest names for the env-parsing tables below, which all
|
||||
// exercise the same three cases against different variables.
|
||||
const (
|
||||
caseUnsetUsesDefault = "unset uses default"
|
||||
caseValidValueParsed = "valid value is parsed"
|
||||
caseUnparseableFails = "unparseable value fails startup"
|
||||
)
|
||||
|
||||
func TestEnvironmentConfig(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
@@ -130,18 +138,18 @@ func TestRetentionSweepInterval(t *testing.T) {
|
||||
expected time.Duration
|
||||
}{
|
||||
{
|
||||
name: "unset uses default",
|
||||
name: caseUnsetUsesDefault,
|
||||
set: false,
|
||||
expected: time.Hour,
|
||||
},
|
||||
{
|
||||
name: "valid value is parsed",
|
||||
name: caseValidValueParsed,
|
||||
set: true,
|
||||
value: "15m",
|
||||
expected: 15 * time.Minute,
|
||||
},
|
||||
{
|
||||
name: "unparseable value fails startup",
|
||||
name: caseUnparseableFails,
|
||||
set: true,
|
||||
value: "not-a-duration",
|
||||
expectError: true,
|
||||
@@ -172,7 +180,7 @@ func TestRetentionSweepInterval(t *testing.T) {
|
||||
}
|
||||
|
||||
// expectStartupError asserts that fx refuses to build the app,
|
||||
// which is what a set-but-unparseable duration must cause.
|
||||
// which is what a set-but-invalid environment value must cause.
|
||||
func expectStartupError(t *testing.T) {
|
||||
t.Helper()
|
||||
|
||||
@@ -226,18 +234,18 @@ func TestSessionIdleTimeout(t *testing.T) {
|
||||
expected time.Duration
|
||||
}{
|
||||
{
|
||||
name: "unset uses default",
|
||||
name: caseUnsetUsesDefault,
|
||||
set: false,
|
||||
expected: 24 * time.Hour,
|
||||
},
|
||||
{
|
||||
name: "valid value is parsed",
|
||||
name: caseValidValueParsed,
|
||||
set: true,
|
||||
value: "30m",
|
||||
expected: 30 * time.Minute,
|
||||
},
|
||||
{
|
||||
name: "unparseable value fails startup",
|
||||
name: caseUnparseableFails,
|
||||
set: true,
|
||||
value: "not-a-duration",
|
||||
expectError: true,
|
||||
@@ -336,3 +344,91 @@ func TestDefaultDataDir(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestReceiverRateLimit(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
set bool
|
||||
value string
|
||||
expectError bool
|
||||
expected int
|
||||
}{
|
||||
{
|
||||
name: caseUnsetUsesDefault,
|
||||
set: false,
|
||||
expected: 120,
|
||||
},
|
||||
{
|
||||
name: caseValidValueParsed,
|
||||
set: true,
|
||||
value: "30",
|
||||
expected: 30,
|
||||
},
|
||||
{
|
||||
name: caseUnparseableFails,
|
||||
set: true,
|
||||
value: "not-a-number",
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
name: "zero fails startup",
|
||||
set: true,
|
||||
value: "0",
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
name: "negative fails startup",
|
||||
set: true,
|
||||
value: "-5",
|
||||
expectError: true,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
// Cannot use t.Parallel() here because t.Setenv
|
||||
// is incompatible with parallel subtests.
|
||||
t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
|
||||
|
||||
if tt.set {
|
||||
t.Setenv("RECEIVER_RATE_LIMIT", tt.value)
|
||||
} else {
|
||||
require.NoError(t, os.Unsetenv(
|
||||
"RECEIVER_RATE_LIMIT",
|
||||
))
|
||||
}
|
||||
|
||||
if tt.expectError {
|
||||
expectStartupError(t)
|
||||
} else {
|
||||
testReceiverRateLimitSuccess(t, tt.expected)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func testReceiverRateLimitSuccess(
|
||||
t *testing.T,
|
||||
expected int,
|
||||
) {
|
||||
t.Helper()
|
||||
|
||||
var cfg *config.Config
|
||||
|
||||
app := fxtest.New(
|
||||
t,
|
||||
fx.Provide(
|
||||
globals.New,
|
||||
logger.New,
|
||||
config.New,
|
||||
),
|
||||
fx.Populate(&cfg),
|
||||
)
|
||||
require.NoError(t, app.Err())
|
||||
|
||||
app.RequireStart()
|
||||
|
||||
defer app.RequireStop()
|
||||
|
||||
assert.Equal(t, expected, cfg.ReceiverRateLimit)
|
||||
}
|
||||
|
||||
@@ -24,6 +24,11 @@ const (
|
||||
// passwordChangeRateInterval is the time window for the
|
||||
// password change rate limit.
|
||||
passwordChangeRateInterval = 1 * time.Minute
|
||||
|
||||
// receiverRateInterval is the time window for the webhook
|
||||
// receiver rate limit. The configured limit is expressed in
|
||||
// requests per minute.
|
||||
receiverRateInterval = 1 * time.Minute
|
||||
)
|
||||
|
||||
// LoginRateLimit returns middleware that enforces per-IP rate
|
||||
@@ -105,3 +110,37 @@ func (m *Middleware) postRateLimit(
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// ReceiverRateLimit returns middleware that rate-limits the
|
||||
// public webhook receiver endpoint per client IP per request
|
||||
// path (the path contains the entrypoint UUID, so each sender
|
||||
// is limited per entrypoint without affecting other senders or
|
||||
// other entrypoints). The limit is Config.ReceiverRateLimit
|
||||
// requests per minute. Requests over the limit receive a 429;
|
||||
// httprate adds the Retry-After header (RFC 6585). IP
|
||||
// extraction honours X-Forwarded-For, X-Real-IP, and
|
||||
// True-Client-IP headers for reverse-proxy setups.
|
||||
func (m *Middleware) ReceiverRateLimit() func(http.Handler) http.Handler {
|
||||
return httprate.Limit(
|
||||
m.params.Config.ReceiverRateLimit,
|
||||
receiverRateInterval,
|
||||
httprate.WithKeyFuncs(
|
||||
httprate.KeyByRealIP,
|
||||
httprate.KeyByEndpoint,
|
||||
),
|
||||
httprate.WithLimitHandler(http.HandlerFunc(
|
||||
func(w http.ResponseWriter, r *http.Request) {
|
||||
m.log.Warn(
|
||||
"webhook receiver rate limit exceeded",
|
||||
"path", r.URL.Path,
|
||||
)
|
||||
http.Error(
|
||||
w,
|
||||
"Too many requests. "+
|
||||
"Please slow down.",
|
||||
http.StatusTooManyRequests,
|
||||
)
|
||||
},
|
||||
)),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -2,8 +2,10 @@ package middleware_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"log/slog"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"os"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
@@ -179,3 +181,133 @@ func TestLoginRateLimit_IndependentPerIP(t *testing.T) {
|
||||
"different IP should not be affected",
|
||||
)
|
||||
}
|
||||
|
||||
// receiverLimitedHandler builds a ReceiverRateLimit-wrapped
|
||||
// handler with the given per-minute limit.
|
||||
func receiverLimitedHandler(
|
||||
t *testing.T, limit int,
|
||||
) http.Handler {
|
||||
t.Helper()
|
||||
|
||||
log := slog.New(slog.NewTextHandler(
|
||||
os.Stderr,
|
||||
&slog.HandlerOptions{Level: slog.LevelDebug},
|
||||
))
|
||||
|
||||
m := middleware.NewForTest(
|
||||
log,
|
||||
&config.Config{ReceiverRateLimit: limit},
|
||||
nil,
|
||||
)
|
||||
|
||||
return m.ReceiverRateLimit()(http.HandlerFunc(
|
||||
func(w http.ResponseWriter, _ *http.Request) {
|
||||
w.WriteHeader(http.StatusOK)
|
||||
},
|
||||
))
|
||||
}
|
||||
|
||||
// receiverPost sends one POST to the handler from the given IP
|
||||
// and path and returns the recorder.
|
||||
func receiverPost(
|
||||
handler http.Handler, ip, path string,
|
||||
) *httptest.ResponseRecorder {
|
||||
req := httptest.NewRequestWithContext(
|
||||
context.Background(),
|
||||
http.MethodPost, path, nil,
|
||||
)
|
||||
req.RemoteAddr = ip
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
handler.ServeHTTP(w, req)
|
||||
|
||||
return w
|
||||
}
|
||||
|
||||
func TestReceiverRateLimit_LimitsPerIPAndPath(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
const limit = 3
|
||||
|
||||
handler := receiverLimitedHandler(t, limit)
|
||||
|
||||
// The first limit requests from one IP to one entrypoint
|
||||
// pass.
|
||||
for i := range limit {
|
||||
w := receiverPost(
|
||||
handler, "9.9.9.9:1234", "/webhook/uuid-a",
|
||||
)
|
||||
assert.Equal(
|
||||
t, http.StatusOK, w.Code,
|
||||
"request %d should pass", i,
|
||||
)
|
||||
}
|
||||
|
||||
// The next request over the limit is rejected with a 429
|
||||
// carrying a Retry-After header.
|
||||
w := receiverPost(
|
||||
handler, "9.9.9.9:1234", "/webhook/uuid-a",
|
||||
)
|
||||
assert.Equal(t, http.StatusTooManyRequests, w.Code)
|
||||
assert.NotEmpty(
|
||||
t, w.Header().Get("Retry-After"),
|
||||
"429 must carry a Retry-After header",
|
||||
)
|
||||
|
||||
// The same IP is not limited on a different entrypoint.
|
||||
w = receiverPost(
|
||||
handler, "9.9.9.9:1234", "/webhook/uuid-b",
|
||||
)
|
||||
assert.Equal(
|
||||
t, http.StatusOK, w.Code,
|
||||
"a different entrypoint must not be affected",
|
||||
)
|
||||
|
||||
// A different IP is not limited on the same entrypoint.
|
||||
w = receiverPost(
|
||||
handler, "8.8.8.8:1234", "/webhook/uuid-a",
|
||||
)
|
||||
assert.Equal(
|
||||
t, http.StatusOK, w.Code,
|
||||
"a different client IP must not be affected",
|
||||
)
|
||||
}
|
||||
|
||||
// TestReceiverRateLimit_CountsEveryMethod proves the receiver
|
||||
// limit counts non-POST requests too: a GET shares the bucket
|
||||
// with a POST and is itself rejected once over the limit.
|
||||
func TestReceiverRateLimit_CountsEveryMethod(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
const (
|
||||
limit = 2
|
||||
ip = "7.7.7.7:1234"
|
||||
path = "/webhook/uuid-c"
|
||||
)
|
||||
|
||||
handler := receiverLimitedHandler(t, limit)
|
||||
|
||||
get := func() *httptest.ResponseRecorder {
|
||||
req := httptest.NewRequestWithContext(
|
||||
context.Background(), http.MethodGet, path, nil,
|
||||
)
|
||||
req.RemoteAddr = ip
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
handler.ServeHTTP(w, req)
|
||||
|
||||
return w
|
||||
}
|
||||
|
||||
// One POST plus one GET fill the bucket, so the GET must
|
||||
// have been counted.
|
||||
assert.Equal(
|
||||
t, http.StatusOK, receiverPost(handler, ip, path).Code,
|
||||
)
|
||||
assert.Equal(t, http.StatusOK, get().Code)
|
||||
|
||||
assert.Equal(
|
||||
t, http.StatusTooManyRequests, get().Code,
|
||||
"a GET over the limit must be rate-limited",
|
||||
)
|
||||
}
|
||||
|
||||
@@ -171,7 +171,7 @@ func (s *Server) setupSourceRoutes() {
|
||||
}
|
||||
|
||||
func (s *Server) setupWebhookRoutes() {
|
||||
s.router.HandleFunc(
|
||||
s.router.With(s.mw.ReceiverRateLimit()).HandleFunc(
|
||||
"/webhook/{uuid}",
|
||||
s.h.HandleWebhook(),
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user