1 Commits

Author SHA1 Message Date
20a050b49d Root background loops at context.Background() (closes #97)
All checks were successful
check / check (push) Successful in 3m3s
The context fx hands an OnStart hook is derived with
context.WithTimeout(ctx, StartTimeout) — 15 seconds by default — and
is cancelled once the start phase ends. It is a start-phase context,
not an application-lifetime one. Two components derived their
long-lived loops from it and so stopped running roughly fifteen
seconds after boot.

Engine.start rooted the entire worker pool, restart recovery, and the
retry sweep in it. Every worker returned on ctx.Done() shortly after
startup, so the process kept receiving and persisting inbound events
while nothing at all forwarded them: deliveryCh filled up and started
logging "delivery channel full" with no consumer left. That is the
whole purpose of the application.

RetentionReaper.start had the same defect. With the default one-hour
RETENTION_SWEEP_INTERVAL the loop was cancelled forty-five minutes
before its first tick, so the reaper never ran a single sweep and
per-webhook event databases grew without bound.

Both now derive their loop context from context.Background(). Their
lifetime is bounded by OnStop, which already cancels and waits on the
WaitGroup, so shutdown is unchanged. Each hook registration moves into
a registerHooks method, the OnStart parameter is named _ so the trap
cannot be reintroduced by silencing an unused-parameter warning, and a
comment at each start explains why the hook context must not be used.
This matches the shape of the same fix applied to the archive sweeper.

The new lifecycle tests drive the genuine registered hooks with an
already-cancelled OnStart context and assert the loops still do work
afterwards — a task delivered, an expired event reaped. Reverting
either fix makes its pair of tests fail. Each component also gets a
shutdown test asserting OnStop cancels the loop and wg.Wait() returns
inside a bounded timeout, so the fix does not trade a startup bug for
a shutdown hang.

iWaitForStatus becomes iWaitForDelivered: every call site waits for
the delivered status, and the two added call sites pushed it past
unparam's threshold for reporting an always-identical argument.
2026-08-09 05:15:13 +00:00
17 changed files with 565 additions and 697 deletions

View File

@@ -867,17 +867,9 @@ Applied to all routes in this order:
8. **Sentry** — Error reporting to Sentry (if `SENTRY_DSN` is set;
configured with `Repanic: true` so panics still reach Recoverer)
Additionally, form endpoints (`/pages`, `/user/*`, `/sources`,
`/source/*`) apply a **MaxBodySize** middleware that limits
POST/PUT/PATCH request bodies to 1 MB. It is registered ahead of the
CSRF middleware in every one of those route groups, because
gorilla/csrf parses the form; if the cap were installed after it, form
parsing would run under net/http's 10 MB default and the 1 MB limit
would never apply. A request that declares a `Content-Length` over the
limit is answered with `413 Request Entity Too Large` before any other
middleware or handler runs; a chunked request, or one that lies about
its length, is hard-capped by `http.MaxBytesReader` and fails
downstream at form-parse time.
Additionally, form endpoints (`/pages`, `/sources`, `/source/*`) apply a
**MaxBodySize** middleware that limits POST/PUT/PATCH request bodies to
1 MB using `http.MaxBytesReader`, preventing oversized form submissions.
### Authentication
@@ -899,8 +891,7 @@ downstream at form-parse time.
- Production security headers on all responses: HSTS, X-Content-Type-Options
(`nosniff`), X-Frame-Options (`DENY`), Content-Security-Policy, Referrer-Policy,
and Permissions-Policy
- Request body size limits (1 MB) on all form POST endpoints, enforced
by middleware that runs before CSRF parses the form
- Request body size limits (1 MB) on all form POST endpoints
- **CSRF protection** via [gorilla/csrf](https://github.com/gorilla/csrf)
on all state-changing forms (cookie-based double-submit tokens with
HMAC authentication). Applied to `/pages`, `/sources`, `/source`, and

13
TODO.md
View File

@@ -28,13 +28,12 @@ databases currently grow without bound.
# Completed Steps
- 2026-08-09 Enforce the request body size limit before the CSRF
middleware parses the form (#90): `MaxBodySize` is now registered
ahead of `CSRF()` in every form route group, the `/user/{username}`
group gained the cap it never had (which is where `POST /password`
lives), the middleware rejects a declared-oversize body with a real
413 up front, and the redundant handler-local
`http.MaxBytesReader` calls were removed
- 2026-08-09 Root the delivery engine's worker pool and the retention
reaper's sweep loop at `context.Background()` rather than the fx
`OnStart` hook context (#97), which carries fx's 15s start timeout and
killed both roughly fifteen seconds after boot: the proxy silently
stopped delivering webhooks entirely, and the reaper never ran a
single sweep under its default one-hour interval
- 2026-08-07 Update golangci-lint to v2.12.2 (Docker image digest in
`Dockerfile`, release-archive sha256 pins in `script/bootstrap`),
adopt the canonical `.golangci.yml` (v2 `linters.settings` layout so

View File

@@ -5,6 +5,8 @@ import (
"log/slog"
"os"
"time"
"go.uber.org/fx"
)
// NewTestRetentionReaper builds a RetentionReaper backed by the given
@@ -29,3 +31,26 @@ func NewTestRetentionReaper(
func (r *RetentionReaper) ExportSweep(ctx context.Context) {
r.sweep(ctx)
}
// ExportRegisterHooks registers the reaper's real fx lifecycle hooks
// on a lifecycle supplied by a test, so a test can drive the exact
// OnStart/OnStop functions the application runs and hand OnStart the
// kind of context fx actually supplies.
func (r *RetentionReaper) ExportRegisterHooks(lc fx.Lifecycle) {
r.registerHooks(lc)
}
// ExportStart starts the reaper's background loop for tests.
func (r *RetentionReaper) ExportStart() {
r.start()
}
// ExportStop stops the reaper's background loop for tests.
func (r *RetentionReaper) ExportStop() {
r.stop()
}
// ExportSetInterval overrides the sweep interval for tests.
func (r *RetentionReaper) ExportSetInterval(d time.Duration) {
r.interval = d
}

View File

@@ -56,9 +56,20 @@ func NewRetentionReaper(
interval: params.Config.RetentionSweepInterval,
}
r.registerHooks(lc)
return r
}
// registerHooks wires the reaper's start and stop into the fx
// lifecycle. The start hook's context is deliberately ignored: see
// start for why the sweep loop must not inherit it.
func (r *RetentionReaper) registerHooks(lc fx.Lifecycle) {
lc.Append(fx.Hook{
OnStart: func(ctx context.Context) error {
r.start(ctx)
//nolint:contextcheck // Not inheriting the hook context is
// the point: see start.
OnStart: func(_ context.Context) error {
r.start()
return nil
},
@@ -68,12 +79,20 @@ func NewRetentionReaper(
return nil
},
})
return r
}
func (r *RetentionReaper) start(ctx context.Context) {
ctx, cancel := context.WithCancel(ctx)
// start launches the background sweep loop.
//
// The loop's context is derived from context.Background(), NOT from
// the fx OnStart hook context. The hook context carries fx's start
// timeout (15s by default) and is cancelled once the start phase
// completes, so a loop derived from it dies 45 minutes before its
// first tick under the default one-hour sweep interval, leaving a
// reaper that never reaps. A long-lived goroutine must outlive the
// startup phase, so its lifetime is bounded by OnStop instead: stop
// cancels this context and waits on the WaitGroup.
func (r *RetentionReaper) start() {
ctx, cancel := context.WithCancel(context.Background())
r.cancel = cancel
r.wg.Add(1)

View File

@@ -0,0 +1,209 @@
package database_test
import (
"context"
"testing"
"time"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/fx"
"gorm.io/gorm"
"sneak.berlin/go/webhooker/internal/database"
)
const (
// reaperTestInterval is the sweep interval a lifecycle test
// runs the reaper at, so a loop that survives startup produces
// an observable sweep quickly.
reaperTestInterval = 10 * time.Millisecond
// reaperStopTimeout bounds how long a lifecycle test waits for
// the reaper's OnStop hook to return before declaring the
// shutdown hung.
reaperStopTimeout = 10 * time.Second
// reaperTestRetentionDays is the retention policy the lifecycle
// tests give their webhook.
reaperTestRetentionDays = 30
)
// recordingLifecycle is a minimal fx.Lifecycle that records the
// hooks a component registers, so a test can invoke the real
// OnStart/OnStop functions with a context of its choosing.
type recordingLifecycle struct {
hooks []fx.Hook
}
func (l *recordingLifecycle) Append(h fx.Hook) {
l.hooks = append(l.hooks, h)
}
// startReaperViaHook drives the genuine fx hooks the application
// registers for the reaper, handing OnStart a context that is
// already done. It returns the recorded lifecycle so the caller
// can drive OnStop too.
func startReaperViaHook(
t *testing.T, r *database.RetentionReaper,
) *recordingLifecycle {
t.Helper()
lc := &recordingLifecycle{}
r.ExportRegisterHooks(lc)
require.Len(t, lc.hooks, 1)
// fx hands OnStart a context carrying the application start
// timeout, and cancels it when the start phase ends. An
// already-cancelled context is that same defect taken to its
// limit, and unlike a plain context.Background() it actually
// distinguishes a correctly rooted loop from a broken one.
hookCtx, cancel := context.WithCancel(context.Background())
cancel()
require.NoError(t, lc.hooks[0].OnStart(hookCtx))
return lc
}
// eventGone reports whether an event row has been removed. It
// takes no *testing.T because it is polled from an
// assert.Eventually condition, which runs off the test goroutine
// where testify assertions must not be used.
func eventGone(db *gorm.DB, eventID string) bool {
var n int64
err := db.Unscoped().Model(&database.Event{}).
Where("id = ?", eventID).Count(&n).Error
if err != nil {
return false
}
return n == 0
}
// seedExpiredWebhook creates a webhook with a finite retention
// policy plus one long-expired event chain, and returns the
// webhook's database and the chain's event ID.
func seedExpiredWebhook(
t *testing.T, env *retentionTestEnv,
) (*gorm.DB, string) {
t.Helper()
webhookID := createWebhook(
t, env.mainDB.DB(), reaperTestRetentionDays,
)
db, err := env.mgr.GetDB(webhookID)
require.NoError(t, err)
chain := seedEventChain(
t, db, webhookID,
time.Now().Add(-365*24*time.Hour),
)
return db, chain.eventID
}
// TestRetentionReaper_LoopOutlivesStartHookContext is the
// regression test for a reaper that never reaped. fx calls
// OnStart with a context carrying the application's start timeout
// (15s by default) and cancels it when the start phase ends, so a
// sweep loop rooted in it is dead three quarters of an hour
// before its first tick under the default one-hour interval, and
// per-webhook event databases grow without bound exactly as they
// did before retention existed.
//
// Driving OnStart with an already-cancelled context is that
// defect taken to its limit: a loop that inherits the hook
// context never ticks once, while a correctly rooted loop keeps
// sweeping for as long as the process lives.
func TestRetentionReaper_LoopOutlivesStartHookContext(
t *testing.T,
) {
t.Parallel()
env := setupRetentionTest(t)
db, eventID := seedExpiredWebhook(t, env)
env.reaper.ExportSetInterval(reaperTestInterval)
lc := startReaperViaHook(t, env.reaper)
t.Cleanup(func() {
_ = lc.hooks[0].OnStop(context.Background())
})
assert.Eventually(
t,
func() bool { return eventGone(db, eventID) },
5*time.Second,
reaperTestInterval,
"the sweep loop must keep running after the start "+
"hook's context is done; it reaped nothing, so it "+
"inherited the hook context and died",
)
}
// TestRetentionReaper_StopHookStopsLoop proves the fix did not
// trade a startup bug for a shutdown hang: now that the sweep
// loop no longer observes the start hook's cancellation, OnStop
// is the only thing that can stop it, and it must both return
// promptly and actually leave the loop stopped.
func TestRetentionReaper_StopHookStopsLoop(t *testing.T) {
t.Parallel()
env := setupRetentionTest(t)
db, eventID := seedExpiredWebhook(t, env)
env.reaper.ExportSetInterval(reaperTestInterval)
lc := startReaperViaHook(t, env.reaper)
// Let the loop prove it is running before stopping it, so a
// fast OnStop cannot pass by stopping something already dead.
require.Eventually(
t,
func() bool { return eventGone(db, eventID) },
5*time.Second,
reaperTestInterval,
)
var stopErr error
stopped := make(chan struct{})
go func() {
defer close(stopped)
// stop blocks on the loop's WaitGroup, so returning at all
// proves the goroutine observed the cancellation.
stopErr = lc.hooks[0].OnStop(context.Background())
}()
select {
case <-stopped:
case <-time.After(reaperStopTimeout):
t.Fatal(
"OnStop did not return: the retention reaper's " +
"WaitGroup is still waiting on a loop that never " +
"observed cancellation",
)
}
require.NoError(t, stopErr)
// With the loop gone, a newly expired chain must survive.
survivor := seedEventChain(
t, db, "stopped-webhook",
time.Now().Add(-365*24*time.Hour),
)
time.Sleep(20 * reaperTestInterval)
assert.False(
t,
eventGone(db, survivor.eventID),
"a stopped reaper must not sweep anything",
)
}

View File

@@ -149,18 +149,7 @@ func New(
Transport: NewSSRFSafeTransport(),
})
lc.Append(fx.Hook{
OnStart: func(ctx context.Context) error {
e.start(ctx)
return nil
},
OnStop: func(_ context.Context) error {
e.stop()
return nil
},
})
e.registerHooks(lc)
return e
}
@@ -210,8 +199,40 @@ func (e *Engine) ScheduleRetry(
})
}
func (e *Engine) start(ctx context.Context) {
ctx, cancel := context.WithCancel(ctx)
// registerHooks wires the engine's start and stop into the fx
// lifecycle. The start hook's context is deliberately ignored:
// see start for why the worker pool must not inherit it.
func (e *Engine) registerHooks(lc fx.Lifecycle) {
lc.Append(fx.Hook{
//nolint:contextcheck // Not inheriting the hook context
// is the point: see start.
OnStart: func(_ context.Context) error {
e.start()
return nil
},
OnStop: func(_ context.Context) error {
e.stop()
return nil
},
})
}
// start launches the worker pool, restart recovery, and the
// periodic retry sweep.
//
// Their context is derived from context.Background(), NOT from
// the fx OnStart hook context. The hook context carries fx's
// start timeout (15s by default) and is cancelled once the start
// phase completes, so goroutines derived from it stop a few
// seconds into the process: every worker would return and the
// engine would silently stop delivering webhooks entirely. A
// long-lived goroutine must outlive the startup phase, so its
// lifetime is bounded by OnStop instead: stop cancels this
// context and waits on the WaitGroup.
func (e *Engine) start() {
ctx, cancel := context.WithCancel(context.Background())
e.cancel = cancel
for range e.workers {

View File

@@ -476,7 +476,7 @@ func TestWorkerLifecycle_StartStop(t *testing.T) {
t.Parallel()
s := newISetup(t)
s.Engine.ExportStart(context.Background())
s.Engine.ExportStart()
event := iSeedEvent(
t, s.WebhookDB, s.WebhookID,
@@ -499,21 +499,17 @@ func TestWorkerLifecycle_StartStop(t *testing.T) {
s.Engine.Notify([]delivery.Task{task})
iWaitForStatus(
t, s.WebhookDB, d.ID,
database.DeliveryStatusDelivered,
)
iWaitForDelivered(t, s.WebhookDB, d.ID)
s.Engine.ExportStop()
}
// iWaitForStatus polls until the delivery reaches the
// expected status.
func iWaitForStatus(
// iWaitForDelivered polls until the delivery reaches the
// delivered status.
func iWaitForDelivered(
t *testing.T,
db *gorm.DB,
deliveryID string,
expected database.DeliveryStatus,
) {
t.Helper()
@@ -527,7 +523,7 @@ func iWaitForStatus(
return false
}
return d.Status == expected
return d.Status == database.DeliveryStatusDelivered
}, 5*time.Second, 50*time.Millisecond)
}
@@ -558,7 +554,7 @@ func TestWorkerLifecycle_ProcessesRetryChannel(
database.DeliveryStatusRetrying,
)
s.Engine.ExportStart(context.Background())
s.Engine.ExportStart()
bodyStr := event.Body
cfg := iHTTPConfig(ts.URL)
@@ -569,10 +565,7 @@ func TestWorkerLifecycle_ProcessesRetryChannel(
s.Engine.ExportRetryCh() <- task
iWaitForStatus(
t, s.WebhookDB, d.ID,
database.DeliveryStatusDelivered,
)
iWaitForDelivered(t, s.WebhookDB, d.ID)
s.Engine.ExportStop()
}

View File

@@ -0,0 +1,199 @@
package delivery_test
import (
"context"
"testing"
"time"
"github.com/google/uuid"
"github.com/stretchr/testify/require"
"go.uber.org/fx"
"sneak.berlin/go/webhooker/internal/database"
"sneak.berlin/go/webhooker/internal/delivery"
)
const (
// hookStopTimeout bounds how long a lifecycle test waits for
// the engine's OnStop hook to return before declaring the
// shutdown hung.
hookStopTimeout = 10 * time.Second
// hookSettleDelay is how long startEngineViaHook waits after
// OnStart before the caller may enqueue work. A worker pool
// wrongly rooted in the already-done hook context has nothing
// but ctx.Done() ready in its select, so it is deterministically
// gone by the end of this window. Without the wait, Notify would
// race the pool's very first select, in which a ready ctx.Done()
// and a ready deliveryCh are chosen between at random and a
// doomed pool still delivers.
hookSettleDelay = 250 * time.Millisecond
)
// recordingLifecycle is a minimal fx.Lifecycle that records the
// hooks a component registers, so a test can invoke the real
// OnStart/OnStop functions with a context of its choosing.
type recordingLifecycle struct {
hooks []fx.Hook
}
func (l *recordingLifecycle) Append(h fx.Hook) {
l.hooks = append(l.hooks, h)
}
// startEngineViaHook drives the genuine fx hooks the application
// registers for the engine, handing OnStart a context that is
// already done, and returns only once a pool that inherited that
// context would have exited. It returns the recorded lifecycle so
// the caller can drive OnStop too.
//
// Callers must not seed pending or retrying deliveries before
// calling this: restart recovery enqueues those during startup,
// which would put work in the queue while the pool is still
// racing its first select.
func startEngineViaHook(
t *testing.T, eng *delivery.Engine,
) *recordingLifecycle {
t.Helper()
lc := &recordingLifecycle{}
eng.ExportRegisterHooks(lc)
require.Len(t, lc.hooks, 1)
// fx hands OnStart a context carrying the application start
// timeout, and cancels it when the start phase ends. An
// already-cancelled context is that same defect taken to its
// limit, and unlike a plain context.Background() it actually
// distinguishes a correctly rooted loop from a broken one.
hookCtx, cancel := context.WithCancel(context.Background())
cancel()
require.NoError(t, lc.hooks[0].OnStart(hookCtx))
time.Sleep(hookSettleDelay)
return lc
}
// seedLogTask seeds a pending delivery for a log target and
// returns its ID together with the task that drives it. The log
// target needs no network, so a delivery completing proves only
// that a worker picked the task up.
func seedLogTask(
t *testing.T, s iSetup,
) (string, delivery.Task) {
t.Helper()
event := iSeedEvent(
t, s.WebhookDB, s.WebhookID,
`{"lifecycle":"hook-context"}`,
)
targetID := uuid.New().String()
d := iSeedDelivery(
t, s.WebhookDB, event.ID, targetID,
database.DeliveryStatusPending,
)
bodyStr := event.Body
task := iTask(
d, event, s.WebhookID, targetID,
"hook-context-test", "", 0, 1, &bodyStr,
)
task.TargetType = database.TargetTypeLog
return d.ID, task
}
// TestEngine_WorkersOutliveStartHookContext is the regression
// test for a delivery engine that stopped delivering roughly
// fifteen seconds after boot. fx calls OnStart with a context
// carrying the application's start timeout (15s by default) and
// cancels it when the start phase ends, so a worker pool rooted
// in it exits shortly after startup: the process keeps accepting
// and persisting events while nothing at all forwards them.
//
// Driving OnStart with an already-cancelled context is that
// defect taken to its limit. A pool that inherits the hook
// context is gone before the task is even enqueued; a correctly
// rooted pool keeps working for as long as the process lives.
func TestEngine_WorkersOutliveStartHookContext(t *testing.T) {
t.Parallel()
s := newISetup(t)
lc := startEngineViaHook(t, s.Engine)
t.Cleanup(func() {
_ = lc.hooks[0].OnStop(context.Background())
})
// Seeded only after the pool has settled, so restart recovery
// cannot enqueue it during startup.
deliveryID, task := seedLogTask(t, s)
s.Engine.Notify([]delivery.Task{task})
iWaitForDelivered(t, s.WebhookDB, deliveryID)
}
// TestEngine_StopHookStopsWorkers proves the fix did not trade a
// startup bug for a shutdown hang: now that the worker pool no
// longer observes the start hook's cancellation, OnStop is the
// only thing that can stop it, and it must both return promptly
// and actually leave the pool drained.
func TestEngine_StopHookStopsWorkers(t *testing.T) {
t.Parallel()
s := newISetup(t)
lc := startEngineViaHook(t, s.Engine)
// Let the pool prove it is running before stopping it, so a
// fast OnStop cannot pass by stopping something already dead.
firstID, firstTask := seedLogTask(t, s)
s.Engine.Notify([]delivery.Task{firstTask})
iWaitForDelivered(t, s.WebhookDB, firstID)
var stopErr error
stopped := make(chan struct{})
go func() {
defer close(stopped)
// stop blocks on the workers' WaitGroup, so returning at
// all proves every goroutine observed the cancellation.
stopErr = lc.hooks[0].OnStop(context.Background())
}()
select {
case <-stopped:
case <-time.After(hookStopTimeout):
t.Fatal(
"OnStop did not return: the delivery engine's " +
"WaitGroup is still waiting on a goroutine that " +
"never observed cancellation",
)
}
require.NoError(t, stopErr)
// With every worker gone, a freshly notified task must sit
// untouched in the queue rather than being delivered.
secondID, secondTask := seedLogTask(t, s)
s.Engine.Notify([]delivery.Task{secondTask})
time.Sleep(200 * time.Millisecond)
var after database.Delivery
require.NoError(
t,
s.WebhookDB.First(&after, "id = ?", secondID).Error,
)
require.Equal(
t,
database.DeliveryStatusPending,
after.Status,
"a stopped engine must not deliver anything",
)
}

View File

@@ -7,6 +7,7 @@ import (
"net/http"
"time"
"go.uber.org/fx"
"gorm.io/gorm"
"sneak.berlin/go/webhooker/internal/database"
)
@@ -189,8 +190,16 @@ func (e *Engine) ExportRecoverInFlight(
}
// ExportStart exposes start for testing.
func (e *Engine) ExportStart(ctx context.Context) {
e.start(ctx)
func (e *Engine) ExportStart() {
e.start()
}
// ExportRegisterHooks registers the engine's real fx lifecycle
// hooks on a lifecycle supplied by a test, so a test can drive
// the exact OnStart/OnStop functions the application runs and
// hand OnStart the kind of context fx actually supplies.
func (e *Engine) ExportRegisterHooks(lc fx.Lifecycle) {
e.registerHooks(lc)
}
// ExportStop exposes stop for testing.

View File

@@ -29,8 +29,10 @@ func (h *Handlers) HandleLoginPage() http.HandlerFunc {
// HandleLoginSubmit handles the login form submission (POST)
func (h *Handlers) HandleLoginSubmit() http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
// The body size cap is enforced by the MaxBodySize
// middleware, which runs before CSRF parses the form.
// Limit request body to prevent memory exhaustion
r.Body = http.MaxBytesReader(w, r.Body, 1<<maxBodyShift)
// Parse form data
err := r.ParseForm()
if err != nil {
h.log.Error("failed to parse form", "error", err)

View File

@@ -31,8 +31,9 @@ func (h *Handlers) HandlePasswordChange() http.HandlerFunc {
return
}
// The body size cap is enforced by the MaxBodySize
// middleware, which runs before CSRF parses the form.
// Limit request body to prevent memory exhaustion.
r.Body = http.MaxBytesReader(w, r.Body, 1<<maxBodyShift)
err := r.ParseForm()
if err != nil {
h.log.Error("failed to parse form", "error", err)

View File

@@ -127,8 +127,10 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc {
return
}
// The body size cap is enforced by the MaxBodySize
// middleware, which runs before CSRF parses the form.
r.Body = http.MaxBytesReader(
w, r.Body, 1<<maxBodyShift,
)
err := r.ParseForm()
if err != nil {
http.Error(
@@ -384,8 +386,10 @@ func (h *Handlers) HandleSourceEditSubmit() http.HandlerFunc {
return
}
// The body size cap is enforced by the MaxBodySize
// middleware, which runs before CSRF parses the form.
r.Body = http.MaxBytesReader(
w, r.Body, 1<<maxBodyShift,
)
err = r.ParseForm()
if err != nil {
http.Error(
@@ -405,8 +409,10 @@ func (h *Handlers) applyWebhookEdit(
r *http.Request,
webhook *database.Webhook,
) {
// The body size cap is enforced by the MaxBodySize middleware,
// which runs before CSRF parses the form.
r.Body = http.MaxBytesReader(
w, r.Body, 1<<maxBodyShift,
)
name := r.FormValue("name")
if name == "" {
data := map[string]any{
@@ -719,8 +725,10 @@ func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc {
return
}
// The body size cap is enforced by the MaxBodySize
// middleware, which runs before CSRF parses the form.
r.Body = http.MaxBytesReader(
w, r.Body, 1<<maxBodyShift,
)
err = r.ParseForm()
if err != nil {
http.Error(
@@ -777,8 +785,10 @@ func (h *Handlers) HandleTargetCreate() http.HandlerFunc {
return
}
// The body size cap is enforced by the MaxBodySize
// middleware, which runs before CSRF parses the form.
r.Body = http.MaxBytesReader(
w, r.Body, 1<<maxBodyShift,
)
err = r.ParseForm()
if err != nil {
http.Error(
@@ -798,8 +808,10 @@ func (h *Handlers) processTargetCreate(
r *http.Request,
webhook database.Webhook,
) {
// The body size cap is enforced by the MaxBodySize middleware,
// which runs before CSRF parses the form.
r.Body = http.MaxBytesReader(
w, r.Body, 1<<maxBodyShift,
)
name := r.FormValue("name")
targetType := database.TargetType(r.FormValue("type"))
targetURL := r.FormValue("url")

View File

@@ -285,36 +285,10 @@ func (s *Middleware) NoCache() func(http.Handler) http.Handler {
}
}
// bodyLimitedMethod reports whether the request method carries a
// body that the MaxBodySize middleware should cap.
func bodyLimitedMethod(method string) bool {
return method == http.MethodPost ||
method == http.MethodPut ||
method == http.MethodPatch
}
// MaxBodySize returns middleware that limits the size of
// POST/PUT/PATCH request bodies to maxBytes. It must be registered
// before any middleware that parses the body — notably CSRF, which
// calls r.PostFormValue — so that form parsing happens under this
// cap rather than net/http's 10 MB default.
//
// Two enforcement paths exist, because http.MaxBytesReader alone
// cannot produce a 413: it reports the overflow as an error from
// Read, by which point the body parser downstream has already
// converted that error into its own response.
//
// - Declared oversize: the request announces a Content-Length
// greater than maxBytes. The middleware answers 413 Request
// Entity Too Large immediately and does not call the next
// handler, so neither CSRF nor the endpoint handler runs.
// - Undeclared oversize: the request is chunked (Content-Length
// of -1) or lies about its Content-Length. There is nothing to
// check up front, so http.MaxBytesReader hard-caps the body at
// maxBytes and the request fails downstream — the form parse
// errors out and CSRF rejects it with 403. The response is less
// precise than a 413, but the body is still never buffered
// beyond the cap, which is the property that matters.
// MaxBodySize returns middleware that limits the request body size
// for POST requests. If the body exceeds the given limit in
// bytes, the server returns 413 Request Entity Too Large. This
// prevents clients from sending arbitrarily large form bodies.
func (s *Middleware) MaxBodySize(
maxBytes int64,
) func(http.Handler) http.Handler {
@@ -323,31 +297,14 @@ func (s *Middleware) MaxBodySize(
w http.ResponseWriter,
r *http.Request,
) {
if !bodyLimitedMethod(r.Method) {
next.ServeHTTP(w, r)
return
}
if r.ContentLength > maxBytes {
s.log.Warn(
"request body exceeds limit",
"method", r.Method,
"path", r.URL.Path,
"content_length", r.ContentLength,
"limit", maxBytes,
if r.Method == http.MethodPost ||
r.Method == http.MethodPut ||
r.Method == http.MethodPatch {
r.Body = http.MaxBytesReader(
w, r.Body, maxBytes,
)
http.Error(
w,
"Request Entity Too Large",
http.StatusRequestEntityTooLarge,
)
return
}
r.Body = http.MaxBytesReader(w, r.Body, maxBytes)
next.ServeHTTP(w, r)
})
}

View File

@@ -3,12 +3,10 @@ package middleware_test
import (
"context"
"encoding/base64"
"io"
"log/slog"
"net/http"
"net/http/httptest"
"os"
"strings"
"testing"
"github.com/gorilla/sessions"
@@ -428,153 +426,6 @@ func TestNoCache_SetsHeaders(t *testing.T) {
)
}
// --- MaxBodySize Middleware Tests ---
const testBodyLimit int64 = 64
// maxBodySizeHandler wraps a sentinel handler in MaxBodySize with
// testBodyLimit. The sentinel records whether it ran and how much of
// the body it managed to read, so tests can distinguish "never
// reached" from "reached but truncated".
type maxBodySizeResult struct {
called bool
read int
readErr error
response *httptest.ResponseRecorder
}
func runMaxBodySize(
t *testing.T,
req *http.Request,
) *maxBodySizeResult {
t.Helper()
m, _ := testMiddleware(t, config.EnvironmentDev)
res := &maxBodySizeResult{response: httptest.NewRecorder()}
handler := m.MaxBodySize(testBodyLimit)(http.HandlerFunc(
func(w http.ResponseWriter, r *http.Request) {
res.called = true
body, err := io.ReadAll(r.Body)
res.read = len(body)
res.readErr = err
w.WriteHeader(http.StatusOK)
},
))
handler.ServeHTTP(res.response, req)
return res
}
// postWithBody builds a POST request whose Content-Length is
// accurate for the given payload size.
func postWithBody(size int) *http.Request {
req := httptest.NewRequestWithContext(
context.Background(),
http.MethodPost, "/pages/login",
strings.NewReader(strings.Repeat("a", size)),
)
req.Header.Set(
"Content-Type", "application/x-www-form-urlencoded",
)
return req
}
func TestMaxBodySize_DeclaredOversize_413AndHandlerNotReached(
t *testing.T,
) {
t.Parallel()
res := runMaxBodySize(t, postWithBody(int(testBodyLimit)+1))
assert.False(
t, res.called,
"handler must not be reached for an oversized body",
)
assert.Equal(
t, http.StatusRequestEntityTooLarge, res.response.Code,
)
}
func TestMaxBodySize_AtLimit_PassesThrough(t *testing.T) {
t.Parallel()
res := runMaxBodySize(t, postWithBody(int(testBodyLimit)))
assert.True(
t, res.called,
"handler should be reached for a body at the limit",
)
require.NoError(t, res.readErr)
assert.Equal(t, int(testBodyLimit), res.read)
assert.Equal(t, http.StatusOK, res.response.Code)
}
func TestMaxBodySize_UnderLimit_PassesThrough(t *testing.T) {
t.Parallel()
res := runMaxBodySize(t, postWithBody(1))
assert.True(t, res.called)
require.NoError(t, res.readErr)
assert.Equal(t, 1, res.read)
assert.Equal(t, http.StatusOK, res.response.Code)
}
func TestMaxBodySize_GetWithOversizeBody_NotCapped(t *testing.T) {
t.Parallel()
req := httptest.NewRequestWithContext(
context.Background(),
http.MethodGet, "/pages/login",
strings.NewReader(
strings.Repeat("a", int(testBodyLimit)+1),
),
)
res := runMaxBodySize(t, req)
assert.True(
t, res.called,
"GET requests are not subject to the POST body cap",
)
require.NoError(t, res.readErr)
assert.Equal(t, int(testBodyLimit)+1, res.read)
}
// TestMaxBodySize_UndeclaredOversize_TruncatedAtCap covers the
// chunked / lying-Content-Length case: there is nothing to check up
// front, so the request reaches the handler but MaxBytesReader
// hard-caps the body and the read fails at the limit.
func TestMaxBodySize_UndeclaredOversize_TruncatedAtCap(
t *testing.T,
) {
t.Parallel()
req := postWithBody(int(testBodyLimit) + 1)
// Simulate a chunked request: no declared length.
req.ContentLength = -1
res := runMaxBodySize(t, req)
assert.True(
t, res.called,
"an undeclared oversize body cannot be rejected up front",
)
require.Error(
t, res.readErr,
"reading past the cap must fail",
)
assert.Equal(
t, int(testBodyLimit), res.read,
"the handler must not see more than the cap",
)
}
// --- Helper Tests ---
func TestIpFromHostPort(t *testing.T) {

View File

@@ -1,36 +0,0 @@
package server
import (
"log/slog"
"net/http"
"sneak.berlin/go/webhooker/internal/config"
"sneak.berlin/go/webhooker/internal/handlers"
"sneak.berlin/go/webhooker/internal/middleware"
)
// MaxFormBodySizeForTest exposes the form body cap so tests can
// build requests that sit exactly at, below, and above it.
const MaxFormBodySizeForTest = maxFormBodySize
// NewRouterForTest builds the real route tree via SetupRoutes with
// the supplied middleware and handlers, bypassing the fx lifecycle
// and the HTTP listener. Tests use it so that route-group middleware
// registration order is exercised exactly as it ships, rather than
// against a hand-rebuilt chain that could drift from routes.go.
func NewRouterForTest(
log *slog.Logger,
cfg *config.Config,
mw *middleware.Middleware,
h *handlers.Handlers,
) http.Handler {
s := &Server{
log: log,
mw: mw,
h: h,
params: ServerParams{Config: cfg},
}
s.SetupRoutes()
return s.router
}

View File

@@ -90,11 +90,9 @@ func (s *Server) setupRoutes() {
func (s *Server) setupPageRoutes() {
s.router.Route("/pages", func(r chi.Router) {
// MaxBodySize must precede CSRF: gorilla/csrf parses the
// form, so the cap has to be installed before it runs.
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Use(s.mw.CSRF())
r.Use(s.mw.NoCache())
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Group(func(r chi.Router) {
r.Use(s.mw.LoginRateLimit())
@@ -108,9 +106,6 @@ func (s *Server) setupPageRoutes() {
func (s *Server) setupUserRoutes() {
s.router.Route("/user/{username}", func(r chi.Router) {
// MaxBodySize must precede CSRF: gorilla/csrf parses the
// form, so the cap has to be installed before it runs.
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Use(s.mw.CSRF())
r.Use(s.mw.NoCache())
r.Use(s.mw.RequireAuth())
@@ -123,24 +118,20 @@ func (s *Server) setupUserRoutes() {
func (s *Server) setupSourceRoutes() {
s.router.Route("/sources", func(r chi.Router) {
// MaxBodySize must precede CSRF: gorilla/csrf parses the
// form, so the cap has to be installed before it runs.
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Use(s.mw.CSRF())
r.Use(s.mw.NoCache())
r.Use(s.mw.RequireAuth())
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Get("/", s.h.HandleSourceList())
r.Get("/new", s.h.HandleSourceCreate())
r.Post("/new", s.h.HandleSourceCreateSubmit())
})
s.router.Route("/source/{sourceID}", func(r chi.Router) {
// MaxBodySize must precede CSRF: gorilla/csrf parses the
// form, so the cap has to be installed before it runs.
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Use(s.mw.CSRF())
r.Use(s.mw.NoCache())
r.Use(s.mw.RequireAuth())
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Get("/", s.h.HandleSourceDetail())
r.Get("/edit", s.h.HandleSourceEdit())
r.Post("/edit", s.h.HandleSourceEditSubmit())

View File

@@ -1,375 +0,0 @@
package server_test
import (
"context"
"html"
"net/http"
"net/http/httptest"
"net/url"
"regexp"
"strings"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/fx"
"go.uber.org/fx/fxtest"
"sneak.berlin/go/webhooker/internal/config"
"sneak.berlin/go/webhooker/internal/database"
"sneak.berlin/go/webhooker/internal/delivery"
"sneak.berlin/go/webhooker/internal/globals"
"sneak.berlin/go/webhooker/internal/handlers"
"sneak.berlin/go/webhooker/internal/healthcheck"
"sneak.berlin/go/webhooker/internal/logger"
"sneak.berlin/go/webhooker/internal/middleware"
"sneak.berlin/go/webhooker/internal/server"
"sneak.berlin/go/webhooker/internal/session"
)
// csrfCookieName is the cookie gorilla/csrf issues when it runs. Its
// presence or absence on a response is how these tests tell whether
// the CSRF middleware executed.
const csrfCookieName = "_gorilla_csrf"
type noopNotifier struct{}
func (n *noopNotifier) Notify([]delivery.Task) {}
// testEnv is the real router from routes.go plus the collaborators
// tests need to seed users and forge sessions.
type testEnv struct {
router http.Handler
sess *session.Session
db *database.Database
}
// newTestEnv wires the dependency graph with fx and builds the
// production route tree, so middleware registration order is
// exercised exactly as it ships.
func newTestEnv(t *testing.T) *testEnv {
t.Helper()
var (
log *logger.Logger
cfg *config.Config
mw *middleware.Middleware
hnd *handlers.Handlers
sess *session.Session
db *database.Database
)
app := fxtest.New(
t,
fx.Provide(
globals.New,
logger.New,
func() *config.Config {
return &config.Config{
DataDir: t.TempDir(),
Environment: config.EnvironmentDev,
}
},
database.New,
database.NewWebhookDBManager,
healthcheck.New,
session.New,
func() delivery.Notifier { return &noopNotifier{} },
middleware.New,
handlers.New,
),
fx.Populate(&log, &cfg, &mw, &hnd, &sess, &db),
)
app.RequireStart()
t.Cleanup(app.RequireStop)
return &testEnv{
router: server.NewRouterForTest(log.Get(), cfg, mw, hnd),
sess: sess,
db: db,
}
}
// oversizeValue returns a form value one byte past the route-group
// body cap, so an encoded form containing it is guaranteed oversize.
func oversizeValue() string {
return strings.Repeat("a", int(server.MaxFormBodySizeForTest)+1)
}
// csrfCookieSet reports whether the response issued a gorilla/csrf
// cookie, which only happens if the CSRF middleware ran.
func csrfCookieSet(w *httptest.ResponseRecorder) bool {
for _, c := range w.Result().Cookies() {
if c.Name == csrfCookieName {
return true
}
}
return false
}
// get issues a GET through the router with the supplied cookies.
func (e *testEnv) get(
path string,
cookies []*http.Cookie,
) *httptest.ResponseRecorder {
req := httptest.NewRequestWithContext(
context.Background(), http.MethodGet, path, nil,
)
for _, c := range cookies {
req.AddCookie(c)
}
w := httptest.NewRecorder()
e.router.ServeHTTP(w, req)
return w
}
// post issues a urlencoded form POST through the router. The body is
// a strings.Reader, so the request carries an accurate
// Content-Length — the signal MaxBodySize checks up front.
func (e *testEnv) post(
path string,
form url.Values,
cookies []*http.Cookie,
) *httptest.ResponseRecorder {
req := httptest.NewRequestWithContext(
context.Background(), http.MethodPost, path,
strings.NewReader(form.Encode()),
)
req.Header.Set(
"Content-Type", "application/x-www-form-urlencoded",
)
for _, c := range cookies {
req.AddCookie(c)
}
w := httptest.NewRecorder()
e.router.ServeHTTP(w, req)
return w
}
// csrfFrom renders the page at path and returns the CSRF token from
// its form together with every cookie needed for the follow-up POST.
func (e *testEnv) csrfFrom(
t *testing.T,
path string,
cookies []*http.Cookie,
) (string, []*http.Cookie) {
t.Helper()
w := e.get(path, cookies)
require.Equal(t, http.StatusOK, w.Code)
pattern := regexp.MustCompile(
`name="csrf_token" value="([^"]+)"`,
)
match := pattern.FindStringSubmatch(w.Body.String())
require.Len(t, match, 2, "form must embed a CSRF token")
// html/template escapes "+" and "=" in attribute values, and
// gorilla/csrf tokens are standard base64, so the value read
// out of the markup has to be unescaped before it is submitted.
token := html.UnescapeString(match[1])
combined := make([]*http.Cookie, 0, len(cookies))
combined = append(combined, cookies...)
combined = append(combined, w.Result().Cookies()...)
return token, combined
}
// authCookies forges an authenticated session for the given user.
func (e *testEnv) authCookies(
t *testing.T,
userID, username string,
) []*http.Cookie {
t.Helper()
req := httptest.NewRequestWithContext(
context.Background(), http.MethodGet, "/setup", nil,
)
w := httptest.NewRecorder()
s, err := e.sess.Get(req)
require.NoError(t, err)
e.sess.SetUser(s, userID, username)
require.NoError(t, e.sess.Save(req, w, s))
cookies := w.Result().Cookies()
require.NotEmpty(t, cookies, "session cookie should be set")
return cookies
}
// seedUser creates a user with the given password and returns the
// stored hash so tests can assert whether it later changed.
func (e *testEnv) seedUser(
t *testing.T,
username, password string,
) (string, string) {
t.Helper()
hash, err := database.HashPassword(password)
require.NoError(t, err)
user := &database.User{Username: username, Password: hash}
require.NoError(t, e.db.DB().Create(user).Error)
return user.ID, hash
}
// storedHash reads the current password hash for a username.
func (e *testEnv) storedHash(t *testing.T, username string) string {
t.Helper()
var user database.User
require.NoError(t,
e.db.DB().Where("username = ?", username).
First(&user).Error,
)
return user.Password
}
// --- /pages group ---
// TestPagesLogin_OversizeBody_RejectedBeforeCSRF proves the cap runs
// ahead of gorilla/csrf: the response is a clean 413 and no CSRF
// cookie was issued, so neither the CSRF middleware nor the login
// handler ran.
func TestPagesLogin_OversizeBody_RejectedBeforeCSRF(t *testing.T) {
t.Parallel()
env := newTestEnv(t)
form := url.Values{}
form.Set("username", oversizeValue())
form.Set("password", "irrelevant")
w := env.post("/pages/login", form, nil)
assert.Equal(
t, http.StatusRequestEntityTooLarge, w.Code,
)
assert.False(
t, csrfCookieSet(w),
"CSRF middleware must not run for an oversized body",
)
}
// TestPagesLogin_UnderLimit_NoToken_CSRFRejects is the control for
// the test above: an identically shaped but under-limit POST does
// reach gorilla/csrf, which rejects it and issues its cookie. Without
// this, the missing-cookie assertion above would prove nothing.
func TestPagesLogin_UnderLimit_NoToken_CSRFRejects(t *testing.T) {
t.Parallel()
env := newTestEnv(t)
form := url.Values{}
form.Set("username", "someone")
form.Set("password", "irrelevant")
w := env.post("/pages/login", form, nil)
assert.Equal(t, http.StatusForbidden, w.Code)
assert.True(
t, csrfCookieSet(w),
"CSRF middleware should run for an under-limit body",
)
}
// TestPagesLogin_UnderLimit_ValidToken_ReachesHandler proves the
// reorder did not break CSRF token handling: a token harvested from
// the rendered login form is still accepted and the request lands in
// the handler.
func TestPagesLogin_UnderLimit_ValidToken_ReachesHandler(
t *testing.T,
) {
t.Parallel()
env := newTestEnv(t)
token, cookies := env.csrfFrom(t, "/pages/login", nil)
form := url.Values{}
form.Set("csrf_token", token)
form.Set("username", "nosuchuser")
form.Set("password", "wrongpassword")
w := env.post("/pages/login", form, cookies)
assert.Equal(t, http.StatusUnauthorized, w.Code)
assert.Contains(
t, w.Body.String(), "Invalid username or password",
"request should reach the login handler",
)
}
// --- /user/{username} group ---
// TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged
// covers the route that previously had no middleware body cap at
// all. The request carries a valid session and a valid CSRF token,
// so the only thing that can stop it is the size cap; the unchanged
// password hash is the observable proof the handler never ran.
func TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged(
t *testing.T,
) {
t.Parallel()
env := newTestEnv(t)
userID, originalHash := env.seedUser(t, "pwuser", "oldpassword")
cookies := env.authCookies(t, userID, "pwuser")
token, cookies := env.csrfFrom(t, "/user/pwuser/", cookies)
form := url.Values{}
form.Set("csrf_token", token)
form.Set("current_password", "oldpassword")
form.Set("new_password", oversizeValue())
form.Set("confirm_password", oversizeValue())
w := env.post("/user/pwuser/password", form, cookies)
assert.Equal(
t, http.StatusRequestEntityTooLarge, w.Code,
)
assert.Equal(
t, originalHash, env.storedHash(t, "pwuser"),
"handler must not run, so the password must be unchanged",
)
}
// TestPasswordChange_UnderLimit_Succeeds proves that adding the cap
// to the /user/{username} group did not break the route it guards.
func TestPasswordChange_UnderLimit_Succeeds(t *testing.T) {
t.Parallel()
env := newTestEnv(t)
userID, originalHash := env.seedUser(t, "okuser", "oldpassword")
cookies := env.authCookies(t, userID, "okuser")
token, cookies := env.csrfFrom(t, "/user/okuser/", cookies)
form := url.Values{}
form.Set("csrf_token", token)
form.Set("current_password", "oldpassword")
form.Set("new_password", "brandnewpassword")
form.Set("confirm_password", "brandnewpassword")
w := env.post("/user/okuser/password", form, cookies)
assert.Equal(t, http.StatusOK, w.Code)
assert.NotEqual(
t, originalHash, env.storedHash(t, "okuser"),
"an under-limit password change should still apply",
)
}