Compare commits
6 Commits
issue-102-
...
c4022c0834
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
c4022c0834 | ||
| be576096aa | |||
| 3941f0b0ff | |||
| 543005c0c2 | |||
| 9bfd033a29 | |||
| fd6397154a |
@@ -1,3 +1,6 @@
|
||||
# .ci-fingerprint is deliberately NOT excluded: it is the CI cache barrier
|
||||
# that keeps the check stages from replaying a cached pass. See the lint
|
||||
# stage of the Dockerfile.
|
||||
.git/
|
||||
bin/
|
||||
*.md
|
||||
|
||||
@@ -11,5 +11,53 @@ jobs:
|
||||
steps:
|
||||
- name: Checkout
|
||||
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 2024-10-23
|
||||
with:
|
||||
# The fingerprint step below needs history to find the last commit
|
||||
# that touched the Docker build context.
|
||||
fetch-depth: 0
|
||||
|
||||
- name: Neutralize superseded run statuses
|
||||
# Gitea cancels the in-flight run when another commit is pushed to the
|
||||
# same branch and records the cancellation as `failure`, so a commit
|
||||
# that was never tested reads red. The cancellation is unconditional
|
||||
# server-side for push events and cannot be disabled from a workflow
|
||||
# file, so the superseding run rewrites those statuses to `skipped`.
|
||||
# Only the exact cancellation status is touched; a real failure is
|
||||
# left alone.
|
||||
env:
|
||||
GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }}
|
||||
run: |
|
||||
set -eu
|
||||
api="${GITHUB_API_URL}/repos/${GITHUB_REPOSITORY}"
|
||||
ctx='check / check (push)'
|
||||
for sha in $(git rev-list --max-count=20 "${GITHUB_SHA}^" || true); do
|
||||
latest="$(curl -sf "${api}/commits/${sha}/status" | jq -r \
|
||||
--arg c "$ctx" \
|
||||
'[.statuses[] | select(.context == $c)][0] // empty
|
||||
| "\(.status)|\(.description)"')" || continue
|
||||
[ "$latest" = 'failure|Has been cancelled' ] || continue
|
||||
curl -sf -X POST "${api}/statuses/${sha}" \
|
||||
-H "Authorization: token ${GITEA_TOKEN}" \
|
||||
-H 'Content-Type: application/json' \
|
||||
-d "$(jq -nc --arg c "$ctx" '{
|
||||
context: $c,
|
||||
state: "skipped",
|
||||
description: "Superseded by a newer commit; never tested"
|
||||
}')" >/dev/null
|
||||
echo "neutralized superseded status on ${sha}"
|
||||
done
|
||||
|
||||
- name: Fingerprint the build context
|
||||
# `.dockerignore` keeps docs out of the build context, so a docs-only
|
||||
# commit legitimately replays the whole image from cache and stays
|
||||
# cheap. Every other commit writes a new fingerprint into the context,
|
||||
# which invalidates the `COPY . .` layer of both check stages: a
|
||||
# commit that was never linted, formatted-checked, tested and built
|
||||
# cannot report success from cache.
|
||||
run: |
|
||||
set -eu
|
||||
fp="$(git log -1 --format=%H -- . ':!*.md' ':!LICENSE' ':!.editorconfig')"
|
||||
printf '%s\n' "${fp:-$GITHUB_SHA}" > .ci-fingerprint
|
||||
|
||||
- name: Build Docker image (runs make check)
|
||||
run: script/cibuild
|
||||
|
||||
3
.gitignore
vendored
3
.gitignore
vendored
@@ -42,3 +42,6 @@ data/
|
||||
# Temporary files
|
||||
tmp/
|
||||
temp/
|
||||
|
||||
# CI cache barrier, written into the build context by the check workflow
|
||||
.ci-fingerprint
|
||||
@@ -12,7 +12,11 @@ WORKDIR /src
|
||||
COPY go.mod go.sum ./
|
||||
RUN go mod download
|
||||
|
||||
# Copy source code
|
||||
# Copy source code. In CI the context also carries .ci-fingerprint, whose
|
||||
# value changes with every commit that touches the build context (see
|
||||
# .gitea/workflows/check.yml). That invalidates this layer, so the checks
|
||||
# below cannot report success by replaying a cached pass. Do not add it to
|
||||
# .dockerignore.
|
||||
COPY . .
|
||||
|
||||
# Run formatting check and linter
|
||||
@@ -36,7 +40,8 @@ WORKDIR /build
|
||||
COPY go.mod go.sum ./
|
||||
RUN go mod download
|
||||
|
||||
# Copy source code
|
||||
# Copy source code, including the .ci-fingerprint cache barrier described in
|
||||
# the lint stage above.
|
||||
COPY . .
|
||||
|
||||
# Run tests and build
|
||||
|
||||
54
README.md
54
README.md
@@ -93,6 +93,7 @@ TTY detection, and security headers are always applied.
|
||||
| `METRICS_USERNAME` | Basic auth username for `/metrics` | `""` |
|
||||
| `METRICS_PASSWORD` | Basic auth password for `/metrics` | `""` |
|
||||
| `SENTRY_DSN` | Sentry error reporting DSN | `""` |
|
||||
| `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` |
|
||||
| `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` |
|
||||
| `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint | `120` |
|
||||
| `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted | `""` (none) |
|
||||
@@ -173,8 +174,12 @@ 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,
|
||||
`RECEIVER_RATE_LIMIT` must be at least 1, and every entry in
|
||||
`TRUSTED_PROXIES` must be a CIDR block or a bare IP address.
|
||||
`RECEIVER_RATE_LIMIT` must be at least 1,
|
||||
`RETENTION_SWEEP_INTERVAL` must be greater than zero (it is a ticker
|
||||
period, so `0s` or a negative value would crash the reaper after
|
||||
startup), and every entry in `TRUSTED_PROXIES` must be a CIDR block or
|
||||
a bare IP address. `SESSION_IDLE_TIMEOUT` is the exception: a
|
||||
non-positive value there means idle expiry is disabled, not invalid.
|
||||
|
||||
Boolean variables (`DEBUG`, `MAINTENANCE_MODE`) accept exactly the
|
||||
spellings Go's `strconv.ParseBool` accepts — `1`, `t`, `T`, `TRUE`,
|
||||
@@ -260,9 +265,10 @@ webhooker solves this by acting as a durable intermediary:
|
||||
targets simultaneously. This enables patterns like forwarding a
|
||||
GitHub webhook to both a deployment service and a Slack channel.
|
||||
|
||||
5. **Replay** — Stored events can be manually redelivered for debugging
|
||||
or testing, without requiring the original sender to fire the webhook
|
||||
again.
|
||||
5. **Replay** (not yet implemented) — Every received event is stored in
|
||||
full, which is what manual redelivery for debugging or testing will
|
||||
be built on. No redelivery exists today, in the web UI or the API;
|
||||
see [TODO.md](TODO.md).
|
||||
|
||||
### Use Cases
|
||||
|
||||
@@ -272,6 +278,7 @@ webhooker solves this by acting as a durable intermediary:
|
||||
size, and delivery performance
|
||||
- **Debugging** and introspection of webhook payloads in the web UI
|
||||
- **Replay** of webhook events for application testing and development
|
||||
(planned; not yet implemented)
|
||||
- **Fan-out** delivery of a single webhook to multiple downstream
|
||||
targets
|
||||
- **High-availability ingestion** for delivery to less reliable backend
|
||||
@@ -497,7 +504,7 @@ A programmatic access credential for API authentication.
|
||||
#### Event
|
||||
|
||||
A captured incoming webhook request. Stores the complete HTTP request
|
||||
data for replay and auditing.
|
||||
data for auditing and for the planned replay capability.
|
||||
|
||||
| Field | Type | Description |
|
||||
| -------------- | ------ | ----------- |
|
||||
@@ -779,9 +786,10 @@ unknown) one while one of its deliveries is still `retrying`, both
|
||||
recovery paths above terminally mark that delivery `failed` and record a
|
||||
`DeliveryResult` naming the current target type as the reason, logging it
|
||||
at warn level. The delivery is not re-dispatched under the new type — the
|
||||
operator never asked for that delivery — and the event itself remains
|
||||
stored in the per-webhook event database, so it can be redelivered
|
||||
manually.
|
||||
operator never asked for that delivery — and while the event itself
|
||||
remains stored in the per-webhook event database, there is no way to
|
||||
redeliver it: manual redelivery is planned, not implemented (see
|
||||
[TODO.md](TODO.md)).
|
||||
|
||||
### Circuit Breaker (HTTP Targets with Retries)
|
||||
|
||||
@@ -1104,6 +1112,34 @@ binary is statically linked and runs on Alpine.
|
||||
`docker build .` is the CI gate — if it passes, the code is formatted,
|
||||
linted, tested, and compiled.
|
||||
|
||||
#### CI gate honesty
|
||||
|
||||
A layer cache lets `docker build .` exit 0 in seconds with the lint and
|
||||
test stages replayed rather than executed, which would make a green
|
||||
check meaningless. The `check` workflow therefore writes
|
||||
`.ci-fingerprint` into the build context before building. Its value is
|
||||
the hash of the last commit that touched the build context, so:
|
||||
|
||||
- Any commit that changes code (including a squash merge whose tree
|
||||
matches an already-built branch) gets a new fingerprint, invalidates
|
||||
the `COPY . .` layer of both check stages, and really runs
|
||||
`make fmt-check`, `make lint`, `make test`, and `make build`. A run
|
||||
that reports success ran them.
|
||||
- A docs-only commit leaves the fingerprint unchanged — `.dockerignore`
|
||||
excludes `*.md` and `LICENSE` from the context anyway — so the image
|
||||
replays from cache and costs seconds.
|
||||
|
||||
The module download layer sits above `COPY . .` and stays cached either
|
||||
way.
|
||||
|
||||
The workflow's first step covers a second way the gate lied: Gitea
|
||||
cancels an in-flight run when a newer commit lands on the same branch
|
||||
and records that cancellation as a `failure` status, marking a commit
|
||||
red that was never tested. Cancellation is unconditional server-side for
|
||||
push events, so the superseding run rewrites the exact
|
||||
`Has been cancelled` status to `skipped`. Genuine failures are never
|
||||
touched.
|
||||
|
||||
## TODO
|
||||
|
||||
See [TODO.md](TODO.md).
|
||||
|
||||
90
TODO.md
90
TODO.md
@@ -1,35 +1,94 @@
|
||||
# Workflow
|
||||
|
||||
* branch (from `main`)
|
||||
* do the work in Next Step
|
||||
* move Next Step to the top of Completed Steps
|
||||
* move the top item of Future Steps into Next Step
|
||||
* commit (`TODO.md` changes in the same commit as the work)
|
||||
* merge to `main` if the branch is not protected, otherwise open a PR
|
||||
* push
|
||||
One issue per unit of work, one branch and one PR per issue:
|
||||
|
||||
* ensure a tracked issue exists with a definition of done
|
||||
* branch from `next` (never from `main`)
|
||||
* do the work; open a PR based on `next` (never on `main`)
|
||||
* pass an independent review, then the manager squash-merges into `next`
|
||||
* push; nothing stays local-only
|
||||
|
||||
`next` is the branch for the next milestone and must stay green and
|
||||
mergeable to `main` without notice. One `next` -> `main` PR accumulates
|
||||
the milestone; releases are cut from `main` separately.
|
||||
|
||||
Issue branches do NOT touch this file — the manager maintains it on
|
||||
`next`. Every branch editing `TODO.md` conflicts with every other
|
||||
(#112).
|
||||
|
||||
# Status
|
||||
|
||||
pre-1.0. No git tags exist. main (4f5ecb1) is a working webhook proxy
|
||||
pre-1.0. No git tags exist. `main` (4f5ecb1) is a working webhook proxy
|
||||
with auth, CSRF/SSRF protections, login rate limiting, Slack target,
|
||||
event retention (#63), the database archiving target (#43), the admin
|
||||
password change flow (#65), policy compliance (#6), pinned lint tooling
|
||||
(#55), and fail-loud configuration parsing (#80). Note: TODO.md was
|
||||
deliberately deleted from this repo in f9a9569 (2026-03-01, #6); its
|
||||
content was folded into the README TODO section, which this draft
|
||||
reconstructs as of 2026-07-06.
|
||||
(#55), and fail-loud configuration parsing (#80).
|
||||
|
||||
`next` (9bfd033) holds the completed 1.0.0 milestone: every issue in it
|
||||
is closed, and it is verified green by cache-defeated container runs
|
||||
rather than by the CI badge, which can pass without executing anything
|
||||
(#119). Note: TODO.md was deliberately deleted from this repo in f9a9569
|
||||
(2026-03-01, #6); its content was folded into the README TODO section,
|
||||
which this draft reconstructs as of 2026-07-06.
|
||||
|
||||
# Next Step
|
||||
|
||||
Manual event redelivery from the web UI (replay is a core promised
|
||||
capability in the README rationale).
|
||||
Tag 1.0.0 from `main` once the milestone PR merges, then repair the CI
|
||||
gate (#119) before the next cycle's work lands — a gate that can report
|
||||
success without running is the one thing every other guarantee here
|
||||
rests on.
|
||||
|
||||
# Completed Steps
|
||||
|
||||
- 2026-08-12 Bound the `X-Forwarded-For` scan's allocation to the hop
|
||||
cap: the reverse walk cuts entries with `strings.LastIndexByte`
|
||||
instead of joining and splitting, so a 1 MB header allocates 16 bytes
|
||||
rather than 1.6 MB per request on the unauthenticated receiver.
|
||||
Semantics proven unchanged by differential testing against the
|
||||
previous implementation (#133)
|
||||
- 2026-08-12 Cap the `X-Forwarded-For` hop walk at 64 entries, so an
|
||||
attacker-supplied chain cannot burn unbounded CPU in the rate-limit
|
||||
key function; running off the end falls back to the peer address
|
||||
(#124)
|
||||
- 2026-08-12 Gate forwarded-header trust behind a `TRUSTED_PROXIES` CIDR
|
||||
list: all three rate limiters key on the connection's own address
|
||||
unless the direct peer is a configured proxy, in which case
|
||||
`X-Forwarded-For` is walked right to left for the first non-proxy hop.
|
||||
Default trusts nothing, and a set-but-unparseable value aborts
|
||||
startup. Before this, any client could mint a fresh bucket or drain
|
||||
another's by rotating a spoofed header (#88)
|
||||
- 2026-08-11 Web UI cleanup: nav terminology unified on Webhooks, the
|
||||
Profile settings placeholder removed, a progressive-enhancement copy
|
||||
button for the entrypoint URL, and retention form copy that states the
|
||||
actual policy (deletion by the reaper, 0 retains forever) (#57)
|
||||
- 2026-08-11 Mask the webhook credential in delivery errors and logs:
|
||||
Go embeds the request URL in `*url.Error`, so every transport failure
|
||||
persisted the full Slack webhook URL into the per-webhook event
|
||||
database via `DeliveryResult.Error`, a field a future REST API would
|
||||
have served. `maskURLError` drops path, query and userinfo while
|
||||
preserving the wrapped cause, so `errors.Is`/`As` and `Timeout()`
|
||||
still work and DNS, TLS and timeout failures still read differently
|
||||
(#118)
|
||||
- 2026-08-11 Rate-limit the public webhook receiver endpoint
|
||||
(`RECEIVER_RATE_LIMIT`, default 120/min), keyed on client IP plus
|
||||
entrypoint path so one entrypoint cannot exhaust another's budget;
|
||||
over-limit requests get 429 with `Retry-After`. It was the one
|
||||
unauthenticated, internet-facing endpoint with no limit at all (#64)
|
||||
- 2026-08-11 Enforce the body size limit before CSRF parses the form:
|
||||
`MaxBodySize` is now first in all four form-parsing route groups, so
|
||||
an oversized request is rejected with 413 instead of being read in
|
||||
full by the CSRF middleware before any cap applied (#90)
|
||||
- 2026-08-11 Mask target config on the source detail page, which
|
||||
rendered the stored blob verbatim and so exposed the Slack
|
||||
incoming-webhook URL — a bearer credential that cannot be revoked
|
||||
per-holder. Config reaches the template only as a `TargetView` of
|
||||
labelled fields, and header values are rendered as a count (#113)
|
||||
- 2026-08-11 Allow `retention_days` of 0 to mean retain forever, via a
|
||||
sentinel written in `BeforeSave` so the GORM column default cannot
|
||||
win the race. Also bounds the reaper's cutoff arithmetic: day counts
|
||||
above 106751 overflowed `time.Duration` and wrapped the cutoff into
|
||||
the future, where every row matched and the sweep deleted everything
|
||||
(#79)
|
||||
- 2026-08-09 Inactivity-based session timeout: sliding idle expiry
|
||||
(`SESSION_IDLE_TIMEOUT`, default `24h`) refreshed on authenticated
|
||||
requests, with the 7-day absolute cap kept as an independent
|
||||
@@ -84,6 +143,9 @@ capability in the README rationale).
|
||||
|
||||
# Future Steps
|
||||
|
||||
- Manual event redelivery from the web UI — the "Replay" capability the
|
||||
README describes as planned. No redelivery code exists anywhere in the
|
||||
tree; events are stored in full, which is all it would be built on
|
||||
- Delivery status and retry management UI
|
||||
- Per-webhook rate limiting in the receiver handler (per-webhook config
|
||||
plus handler enforcement; global limits must not apply to receiver
|
||||
|
||||
@@ -92,6 +92,7 @@ type Config struct {
|
||||
SentryDSN string
|
||||
|
||||
// RetentionSweepInterval is how often the retention reaper runs.
|
||||
// Always positive: it becomes a time.NewTicker period.
|
||||
RetentionSweepInterval time.Duration
|
||||
|
||||
// SessionIdleTimeout is the sliding inactivity window after
|
||||
@@ -235,6 +236,34 @@ func envDuration(
|
||||
return d, nil
|
||||
}
|
||||
|
||||
// envPositiveDuration returns the value of the named environment
|
||||
// variable parsed as a Go duration that must be greater than zero.
|
||||
// Returns defaultValue if not set. A set value that is unparseable or
|
||||
// non-positive is a hard error naming the key and the bad value.
|
||||
//
|
||||
// This is for durations that reach time.NewTicker, which panics on a
|
||||
// non-positive period, in a goroutine started after startup has
|
||||
// already reported success. It is deliberately not used for durations
|
||||
// where non-positive means "disabled" (SESSION_IDLE_TIMEOUT).
|
||||
func envPositiveDuration(
|
||||
key string,
|
||||
defaultValue time.Duration,
|
||||
) (time.Duration, error) {
|
||||
d, err := envDuration(key, defaultValue)
|
||||
if err != nil {
|
||||
return 0, err
|
||||
}
|
||||
|
||||
if d <= 0 {
|
||||
return 0, fmt.Errorf(
|
||||
"%w: %s must be greater than zero, got %s",
|
||||
ErrNonPositiveValue, key, d,
|
||||
)
|
||||
}
|
||||
|
||||
return d, nil
|
||||
}
|
||||
|
||||
// parseCIDR parses one trusted-proxy list entry, which may be a
|
||||
// CIDR block ("10.0.0.0/8") or a bare address ("10.0.0.1", treated
|
||||
// as a single-host block).
|
||||
@@ -346,7 +375,7 @@ func loadFromEnv() (*Config, error) {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
retentionSweepInterval, err := envDuration(
|
||||
retentionSweepInterval, err := envPositiveDuration(
|
||||
"RETENTION_SWEEP_INTERVAL",
|
||||
defaultRetentionSweepInterval,
|
||||
)
|
||||
@@ -354,6 +383,8 @@ func loadFromEnv() (*Config, error) {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
// Non-positive is "disabled" here, not invalid, so this stays on
|
||||
// envDuration.
|
||||
sessionIdleTimeout, err := envDuration(
|
||||
"SESSION_IDLE_TIMEOUT",
|
||||
defaultSessionIdleTimeout,
|
||||
|
||||
@@ -139,7 +139,11 @@ func TestRetentionSweepInterval(t *testing.T) {
|
||||
set bool
|
||||
value string
|
||||
expectError bool
|
||||
expected time.Duration
|
||||
// sentinel, when set, must be wrapped by the startup
|
||||
// error; every error case must additionally name the
|
||||
// variable in its message.
|
||||
sentinel error
|
||||
expected time.Duration
|
||||
}{
|
||||
{
|
||||
name: caseUnsetUsesDefault,
|
||||
@@ -158,6 +162,24 @@ func TestRetentionSweepInterval(t *testing.T) {
|
||||
value: "not-a-duration",
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
// A non-positive period panics the ticker in the
|
||||
// reaper and archive-sweeper goroutines, long after
|
||||
// startup has reported success, so it has to fail
|
||||
// here instead.
|
||||
name: "zero fails startup",
|
||||
set: true,
|
||||
value: "0s",
|
||||
expectError: true,
|
||||
sentinel: config.ErrNonPositiveValue,
|
||||
},
|
||||
{
|
||||
name: "negative fails startup",
|
||||
set: true,
|
||||
value: "-1h",
|
||||
expectError: true,
|
||||
sentinel: config.ErrNonPositiveValue,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
@@ -175,7 +197,9 @@ func TestRetentionSweepInterval(t *testing.T) {
|
||||
}
|
||||
|
||||
if tt.expectError {
|
||||
expectStartupError(t)
|
||||
expectStartupErrorFor(
|
||||
t, "RETENTION_SWEEP_INTERVAL", tt.sentinel,
|
||||
)
|
||||
} else {
|
||||
testRetentionSweepIntervalSuccess(t, tt.expected)
|
||||
}
|
||||
@@ -281,6 +305,22 @@ func TestSessionIdleTimeout(t *testing.T) {
|
||||
value: "not-a-duration",
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
// Non-positive is "idle expiry disabled" for this
|
||||
// variable, not a configuration error: unlike
|
||||
// RETENTION_SWEEP_INTERVAL it never becomes a ticker
|
||||
// period.
|
||||
name: "zero disables idle expiry",
|
||||
set: true,
|
||||
value: "0s",
|
||||
expected: 0,
|
||||
},
|
||||
{
|
||||
name: "negative disables idle expiry",
|
||||
set: true,
|
||||
value: "-1h",
|
||||
expected: -time.Hour,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
|
||||
@@ -46,19 +46,8 @@ func (r *RetentionReaper) ExportStart() {
|
||||
}
|
||||
|
||||
// ExportStop stops the reaper's background loop for tests.
|
||||
func (r *RetentionReaper) ExportStop(ctx context.Context) error {
|
||||
return r.stop(ctx)
|
||||
}
|
||||
|
||||
// ExportWedgeLoop adds a goroutine to the reaper's WaitGroup that
|
||||
// never observes cancellation and returns only when release is
|
||||
// closed. It stands in for a sweep stuck on a locked database.
|
||||
func (r *RetentionReaper) ExportWedgeLoop(
|
||||
release <-chan struct{},
|
||||
) {
|
||||
r.wg.Go(func() {
|
||||
<-release
|
||||
})
|
||||
func (r *RetentionReaper) ExportStop() {
|
||||
r.stop()
|
||||
}
|
||||
|
||||
// ExportSetInterval overrides the sweep interval for tests.
|
||||
|
||||
@@ -10,7 +10,6 @@ import (
|
||||
"go.uber.org/fx"
|
||||
"gorm.io/gorm"
|
||||
"sneak.berlin/go/webhooker/internal/config"
|
||||
"sneak.berlin/go/webhooker/internal/lifecycle"
|
||||
"sneak.berlin/go/webhooker/internal/logger"
|
||||
)
|
||||
|
||||
@@ -63,9 +62,8 @@ func NewRetentionReaper(
|
||||
}
|
||||
|
||||
// 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); the stop hook's
|
||||
// context is honoured (see stop).
|
||||
// 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{
|
||||
//nolint:contextcheck // Not inheriting the hook context is
|
||||
@@ -75,8 +73,10 @@ func (r *RetentionReaper) registerHooks(lc fx.Lifecycle) {
|
||||
|
||||
return nil
|
||||
},
|
||||
OnStop: func(ctx context.Context) error {
|
||||
return r.stop(ctx)
|
||||
OnStop: func(_ context.Context) error {
|
||||
r.stop()
|
||||
|
||||
return nil
|
||||
},
|
||||
})
|
||||
}
|
||||
@@ -105,27 +105,15 @@ func (r *RetentionReaper) start() {
|
||||
)
|
||||
}
|
||||
|
||||
// stop cancels the sweep loop's context and waits for it to
|
||||
// exit, bounded by the stop hook's context: a sweep wedged on a
|
||||
// locked database must not hang the process past fx's stop
|
||||
// timeout.
|
||||
func (r *RetentionReaper) stop(ctx context.Context) error {
|
||||
func (r *RetentionReaper) stop() {
|
||||
r.log.Info("retention reaper stopping")
|
||||
|
||||
if r.cancel != nil {
|
||||
r.cancel()
|
||||
}
|
||||
|
||||
err := lifecycle.WaitForShutdown(
|
||||
ctx, r.log, "retention reaper", &r.wg,
|
||||
)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
r.wg.Wait()
|
||||
r.log.Info("retention reaper stopped")
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
func (r *RetentionReaper) run(ctx context.Context) {
|
||||
|
||||
@@ -26,13 +26,6 @@ const (
|
||||
// reaperTestRetentionDays is the retention policy the lifecycle
|
||||
// tests give their webhook.
|
||||
reaperTestRetentionDays = 30
|
||||
|
||||
// reaperWedgeStopTimeout is the stop timeout the wedged-shutdown
|
||||
// test hands OnStop, standing in for fx's StopTimeout. The test
|
||||
// asserts only that the hook returns at all, and allows it
|
||||
// reaperStopTimeout — forty times this budget — to do so, so no
|
||||
// assertion races the wall clock.
|
||||
reaperWedgeStopTimeout = 250 * time.Millisecond
|
||||
)
|
||||
|
||||
// recordingLifecycle is a minimal fx.Lifecycle that records the
|
||||
@@ -214,59 +207,3 @@ func TestRetentionReaper_StopHookStopsLoop(t *testing.T) {
|
||||
"a stopped reaper must not sweep anything",
|
||||
)
|
||||
}
|
||||
|
||||
// TestRetentionReaper_StopHookHonoursStopTimeout is the
|
||||
// regression test for a shutdown that could never complete. fx
|
||||
// hands OnStop a context carrying the application's stop timeout;
|
||||
// an OnStop that discards it and calls wg.Wait() bare hangs the
|
||||
// process forever on a sweep blocked on a locked SQLite database
|
||||
// — precisely when a bounded shutdown matters most.
|
||||
//
|
||||
// The wedged goroutine here never observes cancellation, so the
|
||||
// hook can only return by honouring its context, and it must say
|
||||
// so rather than reporting a clean stop.
|
||||
func TestRetentionReaper_StopHookHonoursStopTimeout(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupRetentionTest(t)
|
||||
|
||||
env.reaper.ExportSetInterval(reaperTestInterval)
|
||||
|
||||
lc := startReaperViaHook(t, env.reaper)
|
||||
|
||||
release := make(chan struct{})
|
||||
|
||||
t.Cleanup(func() { close(release) })
|
||||
|
||||
env.reaper.ExportWedgeLoop(release)
|
||||
|
||||
stopCtx, cancel := context.WithTimeout(
|
||||
context.Background(), reaperWedgeStopTimeout,
|
||||
)
|
||||
defer cancel()
|
||||
|
||||
var stopErr error
|
||||
|
||||
stopped := make(chan struct{})
|
||||
|
||||
go func() {
|
||||
defer close(stopped)
|
||||
|
||||
stopErr = lc.hooks[0].OnStop(stopCtx)
|
||||
}()
|
||||
|
||||
select {
|
||||
case <-stopped:
|
||||
case <-time.After(reaperStopTimeout):
|
||||
t.Fatal(
|
||||
"OnStop did not return: it discarded the stop " +
|
||||
"context and is waiting on a wedged goroutine " +
|
||||
"that will never observe cancellation",
|
||||
)
|
||||
}
|
||||
|
||||
require.ErrorIs(t, stopErr, context.DeadlineExceeded)
|
||||
require.ErrorContains(t, stopErr, "retention reaper")
|
||||
}
|
||||
|
||||
@@ -10,7 +10,6 @@ import (
|
||||
"go.uber.org/fx"
|
||||
"sneak.berlin/go/webhooker/internal/config"
|
||||
"sneak.berlin/go/webhooker/internal/database"
|
||||
"sneak.berlin/go/webhooker/internal/lifecycle"
|
||||
"sneak.berlin/go/webhooker/internal/logger"
|
||||
)
|
||||
|
||||
@@ -68,9 +67,10 @@ func NewArchiveSweeper(
|
||||
}
|
||||
|
||||
// registerHooks wires the sweeper's start and stop into the fx
|
||||
// lifecycle. The start hook's context is deliberately ignored
|
||||
// (see start for why the background loop must not inherit it);
|
||||
// the stop hook's context is honoured (see stop).
|
||||
// lifecycle. Both hook contexts are deliberately ignored: see
|
||||
// start for why the background loop must not inherit the start
|
||||
// hook's context, and stop for why shutdown blocks on the loop
|
||||
// rather than on the stop hook's deadline.
|
||||
func (s *ArchiveSweeper) registerHooks(lc fx.Lifecycle) {
|
||||
lc.Append(fx.Hook{
|
||||
//nolint:contextcheck // Not passing the hook context is
|
||||
@@ -80,8 +80,10 @@ func (s *ArchiveSweeper) registerHooks(lc fx.Lifecycle) {
|
||||
|
||||
return nil
|
||||
},
|
||||
OnStop: func(ctx context.Context) error {
|
||||
return s.stop(ctx)
|
||||
OnStop: func(_ context.Context) error {
|
||||
s.stop()
|
||||
|
||||
return nil
|
||||
},
|
||||
})
|
||||
}
|
||||
@@ -111,27 +113,15 @@ func (s *ArchiveSweeper) start() {
|
||||
)
|
||||
}
|
||||
|
||||
// stop cancels the sweep loop's context and waits for it to
|
||||
// exit, bounded by the stop hook's context: a prune wedged on a
|
||||
// locked archive must not hang the process past fx's stop
|
||||
// timeout.
|
||||
func (s *ArchiveSweeper) stop(ctx context.Context) error {
|
||||
func (s *ArchiveSweeper) stop() {
|
||||
s.log.Info("archive sweeper stopping")
|
||||
|
||||
if s.cancel != nil {
|
||||
s.cancel()
|
||||
}
|
||||
|
||||
err := lifecycle.WaitForShutdown(
|
||||
ctx, s.log, "archive sweeper", &s.wg,
|
||||
)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
s.wg.Wait()
|
||||
s.log.Info("archive sweeper stopped")
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
func (s *ArchiveSweeper) run(ctx context.Context) {
|
||||
|
||||
@@ -14,6 +14,7 @@ import (
|
||||
"github.com/google/uuid"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"go.uber.org/fx"
|
||||
"gorm.io/driver/sqlite"
|
||||
"gorm.io/gorm"
|
||||
"gorm.io/gorm/clause"
|
||||
@@ -225,6 +226,17 @@ func countArchivedRows(path string) (int64, error) {
|
||||
return count, nil
|
||||
}
|
||||
|
||||
// captureLifecycle 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 captureLifecycle struct {
|
||||
hooks []fx.Hook
|
||||
}
|
||||
|
||||
func (l *captureLifecycle) Append(h fx.Hook) {
|
||||
l.hooks = append(l.hooks, h)
|
||||
}
|
||||
|
||||
// TestArchiveSweeper_LoopOutlivesStartHookContext is the
|
||||
// regression test for a sweeper that never swept. fx calls
|
||||
// OnStart with a context carrying the application's start
|
||||
@@ -258,7 +270,7 @@ func TestArchiveSweeper_LoopOutlivesStartHookContext(
|
||||
|
||||
// Drive the genuine fx hooks the application registers,
|
||||
// rather than a test-only entry point.
|
||||
lc := &recordingLifecycle{}
|
||||
lc := &captureLifecycle{}
|
||||
env.sweeper.ExportRegisterHooks(lc)
|
||||
require.Len(t, lc.hooks, 1)
|
||||
|
||||
@@ -912,36 +924,7 @@ func TestArchiveSweeper_StopsCleanly(t *testing.T) {
|
||||
env.sweeper.ExportSetInterval(time.Millisecond)
|
||||
env.sweeper.ExportStart()
|
||||
|
||||
// stop blocks on the loop's WaitGroup, so returning without
|
||||
// error proves the loop observed the cancellation and exited
|
||||
// well inside the stop context.
|
||||
require.NoError(
|
||||
t, env.sweeper.ExportStop(context.Background()),
|
||||
)
|
||||
}
|
||||
|
||||
// TestArchiveSweeper_StopHookHonoursStopTimeout is the sweeper's
|
||||
// half of the same shutdown defect the engine and the retention
|
||||
// reaper carried: an OnStop that discards its context and waits
|
||||
// on the WaitGroup bare hangs the process forever on a prune
|
||||
// wedged inside a locked archive.
|
||||
func TestArchiveSweeper_StopHookHonoursStopTimeout(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupSweeperTest(t)
|
||||
|
||||
lc := &recordingLifecycle{}
|
||||
env.sweeper.ExportRegisterHooks(lc)
|
||||
require.Len(t, lc.hooks, 1)
|
||||
require.NoError(t, lc.hooks[0].OnStart(context.Background()))
|
||||
|
||||
release := make(chan struct{})
|
||||
|
||||
t.Cleanup(func() { close(release) })
|
||||
|
||||
env.sweeper.ExportWedgeLoop(release)
|
||||
|
||||
requireStopHookExpires(t, lc.hooks[0], "archive sweeper")
|
||||
// stop blocks on the loop's WaitGroup, so returning at all
|
||||
// proves the loop observed the cancellation and exited.
|
||||
env.sweeper.ExportStop()
|
||||
}
|
||||
|
||||
@@ -13,7 +13,6 @@ import (
|
||||
"go.uber.org/fx"
|
||||
"gorm.io/gorm"
|
||||
"sneak.berlin/go/webhooker/internal/database"
|
||||
"sneak.berlin/go/webhooker/internal/lifecycle"
|
||||
"sneak.berlin/go/webhooker/internal/logger"
|
||||
)
|
||||
|
||||
@@ -235,9 +234,8 @@ func (e *Engine) ScheduleRetry(
|
||||
}
|
||||
|
||||
// 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); the
|
||||
// stop hook's context is honoured (see stop).
|
||||
// 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
|
||||
@@ -247,8 +245,10 @@ func (e *Engine) registerHooks(lc fx.Lifecycle) {
|
||||
|
||||
return nil
|
||||
},
|
||||
OnStop: func(ctx context.Context) error {
|
||||
return e.stop(ctx)
|
||||
OnStop: func(_ context.Context) error {
|
||||
e.stop()
|
||||
|
||||
return nil
|
||||
},
|
||||
})
|
||||
}
|
||||
@@ -289,26 +289,11 @@ func (e *Engine) start() {
|
||||
)
|
||||
}
|
||||
|
||||
// stop cancels the worker pool's context and waits for the pool
|
||||
// to drain, bounded by the stop hook's context: a wedged worker
|
||||
// must not hang the process past fx's stop timeout.
|
||||
func (e *Engine) stop(ctx context.Context) error {
|
||||
func (e *Engine) stop() {
|
||||
e.log.Info("delivery engine stopping")
|
||||
|
||||
if e.cancel != nil {
|
||||
e.cancel()
|
||||
}
|
||||
|
||||
err := lifecycle.WaitForShutdown(
|
||||
ctx, e.log, "delivery engine", &e.wg,
|
||||
)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
e.cancel()
|
||||
e.wg.Wait()
|
||||
e.log.Info("delivery engine stopped")
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
func (e *Engine) worker(ctx context.Context) {
|
||||
@@ -800,9 +785,9 @@ func (e *Engine) sweepSingleRetry(
|
||||
// status retrying themselves. Re-dispatching under the new type
|
||||
// would be a delivery the operator never asked for, and leaving
|
||||
// the row retrying strands it forever, so the delivery is
|
||||
// failed with a recorded reason and can be redelivered
|
||||
// manually. Logged at warn, not error: this is operator-caused
|
||||
// state, not a system fault.
|
||||
// failed with a recorded reason. The event stays stored, but
|
||||
// nothing redelivers it today. Logged at warn, not error: this
|
||||
// is operator-caused state, not a system fault.
|
||||
func (e *Engine) failUnretryableRetry(
|
||||
webhookDB *gorm.DB,
|
||||
webhookID string,
|
||||
|
||||
@@ -501,7 +501,7 @@ func TestWorkerLifecycle_StartStop(t *testing.T) {
|
||||
|
||||
iWaitForDelivered(t, s.WebhookDB, d.ID)
|
||||
|
||||
require.NoError(t, s.Engine.ExportStop(context.Background()))
|
||||
s.Engine.ExportStop()
|
||||
}
|
||||
|
||||
// iWaitForDelivered polls until the delivery reaches the
|
||||
@@ -567,7 +567,7 @@ func TestWorkerLifecycle_ProcessesRetryChannel(
|
||||
|
||||
iWaitForDelivered(t, s.WebhookDB, d.ID)
|
||||
|
||||
require.NoError(t, s.Engine.ExportStop(context.Background()))
|
||||
s.Engine.ExportStop()
|
||||
}
|
||||
|
||||
// --- processDelivery: unknown target type ---
|
||||
|
||||
@@ -27,13 +27,6 @@ const (
|
||||
// and a ready deliveryCh are chosen between at random and a
|
||||
// doomed pool still delivers.
|
||||
hookSettleDelay = 250 * time.Millisecond
|
||||
|
||||
// wedgeStopTimeout is the stop timeout a wedged-shutdown test
|
||||
// hands OnStop, standing in for fx's StopTimeout. The test
|
||||
// asserts only that the hook returns at all, and allows it
|
||||
// hookStopTimeout — forty times this budget — to do so, so no
|
||||
// assertion here races the wall clock.
|
||||
wedgeStopTimeout = 250 * time.Millisecond
|
||||
)
|
||||
|
||||
// recordingLifecycle is a minimal fx.Lifecycle that records the
|
||||
@@ -47,44 +40,6 @@ func (l *recordingLifecycle) Append(h fx.Hook) {
|
||||
l.hooks = append(l.hooks, h)
|
||||
}
|
||||
|
||||
// requireStopHookExpires drives hook.OnStop with a stop context
|
||||
// that expires while a wedged goroutine is still running, and
|
||||
// requires the hook to return the deadline error naming
|
||||
// component instead of blocking on the WaitGroup forever.
|
||||
func requireStopHookExpires(
|
||||
t *testing.T, hook fx.Hook, component string,
|
||||
) {
|
||||
t.Helper()
|
||||
|
||||
stopCtx, cancel := context.WithTimeout(
|
||||
context.Background(), wedgeStopTimeout,
|
||||
)
|
||||
defer cancel()
|
||||
|
||||
var stopErr error
|
||||
|
||||
stopped := make(chan struct{})
|
||||
|
||||
go func() {
|
||||
defer close(stopped)
|
||||
|
||||
stopErr = hook.OnStop(stopCtx)
|
||||
}()
|
||||
|
||||
select {
|
||||
case <-stopped:
|
||||
case <-time.After(hookStopTimeout):
|
||||
t.Fatal(
|
||||
"OnStop did not return: it discarded the stop " +
|
||||
"context and is waiting on a wedged goroutine " +
|
||||
"that will never observe cancellation",
|
||||
)
|
||||
}
|
||||
|
||||
require.ErrorIs(t, stopErr, context.DeadlineExceeded)
|
||||
require.ErrorContains(t, stopErr, component)
|
||||
}
|
||||
|
||||
// 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
|
||||
@@ -242,30 +197,3 @@ func TestEngine_StopHookStopsWorkers(t *testing.T) {
|
||||
"a stopped engine must not deliver anything",
|
||||
)
|
||||
}
|
||||
|
||||
// TestEngine_StopHookHonoursStopTimeout is the regression test
|
||||
// for a shutdown that could never complete. fx hands OnStop a
|
||||
// context carrying the application's stop timeout; an OnStop
|
||||
// that discards it and calls wg.Wait() bare hangs the process
|
||||
// forever on a single worker stuck inside a delivery target that
|
||||
// never returns — precisely when a bounded shutdown matters
|
||||
// most.
|
||||
//
|
||||
// The wedged goroutine here never observes cancellation, so the
|
||||
// hook can only return by honouring its context, and it must say
|
||||
// so rather than reporting a clean stop.
|
||||
func TestEngine_StopHookHonoursStopTimeout(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
s := newISetup(t)
|
||||
|
||||
lc := startEngineViaHook(t, s.Engine)
|
||||
|
||||
release := make(chan struct{})
|
||||
|
||||
t.Cleanup(func() { close(release) })
|
||||
|
||||
s.Engine.ExportWedgeWorker(release)
|
||||
|
||||
requireStopHookExpires(t, lc.hooks[0], "delivery engine")
|
||||
}
|
||||
|
||||
@@ -216,19 +216,8 @@ func (e *Engine) ExportRegisterHooks(lc fx.Lifecycle) {
|
||||
}
|
||||
|
||||
// ExportStop exposes stop for testing.
|
||||
func (e *Engine) ExportStop(ctx context.Context) error {
|
||||
return e.stop(ctx)
|
||||
}
|
||||
|
||||
// ExportWedgeWorker adds a goroutine to the engine's WaitGroup
|
||||
// that never observes cancellation and returns only when release
|
||||
// is closed. It stands in for a worker stuck inside a delivery
|
||||
// target that never returns, which is the only way stop can be
|
||||
// made to outlast its context.
|
||||
func (e *Engine) ExportWedgeWorker(release <-chan struct{}) {
|
||||
e.wg.Go(func() {
|
||||
<-release
|
||||
})
|
||||
func (e *Engine) ExportStop() {
|
||||
e.stop()
|
||||
}
|
||||
|
||||
// ExportDeliveryCh returns the delivery channel.
|
||||
@@ -529,19 +518,8 @@ func (s *ArchiveSweeper) ExportRegisterHooks(lc fx.Lifecycle) {
|
||||
}
|
||||
|
||||
// ExportStop stops the sweeper's background loop for tests.
|
||||
func (s *ArchiveSweeper) ExportStop(ctx context.Context) error {
|
||||
return s.stop(ctx)
|
||||
}
|
||||
|
||||
// ExportWedgeLoop adds a goroutine to the sweeper's WaitGroup
|
||||
// that never observes cancellation and returns only when release
|
||||
// is closed. It stands in for a prune stuck on a locked archive.
|
||||
func (s *ArchiveSweeper) ExportWedgeLoop(
|
||||
release <-chan struct{},
|
||||
) {
|
||||
s.wg.Go(func() {
|
||||
<-release
|
||||
})
|
||||
func (s *ArchiveSweeper) ExportStop() {
|
||||
s.stop()
|
||||
}
|
||||
|
||||
// ExportSetInterval overrides the sweep interval for tests.
|
||||
|
||||
@@ -1,57 +0,0 @@
|
||||
// Package lifecycle holds helpers shared by the components that
|
||||
// register fx start and stop hooks.
|
||||
package lifecycle
|
||||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"sync"
|
||||
)
|
||||
|
||||
// WaitForShutdown waits for wg to drain, bounded by ctx.
|
||||
//
|
||||
// fx hands OnStop a context carrying the application's stop
|
||||
// timeout. A bare wg.Wait() discards that deadline, so a single
|
||||
// goroutine that never observes cancellation — a delivery target
|
||||
// that never returns, a SQLite operation blocked on a lock —
|
||||
// hangs the process forever instead of letting it exit when the
|
||||
// timeout expires, which is exactly when a clean shutdown matters
|
||||
// most.
|
||||
//
|
||||
// On timeout it logs at error naming component and returns an
|
||||
// error: the goroutines are still running, and reporting success
|
||||
// would hide an unclean shutdown from the operator. The waiting
|
||||
// goroutine outlives this call and exits when (if) wg drains; it
|
||||
// holds nothing but the channel it closes.
|
||||
func WaitForShutdown(
|
||||
ctx context.Context,
|
||||
log *slog.Logger,
|
||||
component string,
|
||||
wg *sync.WaitGroup,
|
||||
) error {
|
||||
done := make(chan struct{})
|
||||
|
||||
go func() {
|
||||
defer close(done)
|
||||
|
||||
wg.Wait()
|
||||
}()
|
||||
|
||||
select {
|
||||
case <-done:
|
||||
return nil
|
||||
case <-ctx.Done():
|
||||
log.Error(
|
||||
"shutdown timed out, goroutines still running",
|
||||
"component", component,
|
||||
"error", ctx.Err(),
|
||||
)
|
||||
|
||||
return fmt.Errorf(
|
||||
"%s: shutdown timed out, "+
|
||||
"goroutines still running: %w",
|
||||
component, ctx.Err(),
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -1,62 +0,0 @@
|
||||
package lifecycle_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"log/slog"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
"sneak.berlin/go/webhooker/internal/lifecycle"
|
||||
)
|
||||
|
||||
// waitTimeout is the stop budget the timeout case gives a
|
||||
// goroutine that never returns. The test's own patience is the
|
||||
// go test deadline, so the only thing this value affects is how
|
||||
// long the case takes.
|
||||
const waitTimeout = 100 * time.Millisecond
|
||||
|
||||
func discardLogger() *slog.Logger {
|
||||
return slog.New(slog.DiscardHandler)
|
||||
}
|
||||
|
||||
func TestWaitForShutdown_DrainedGroup(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
var wg sync.WaitGroup
|
||||
|
||||
wg.Go(func() {})
|
||||
|
||||
require.NoError(
|
||||
t,
|
||||
lifecycle.WaitForShutdown(
|
||||
context.Background(), discardLogger(),
|
||||
"test component", &wg,
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
func TestWaitForShutdown_ContextExpires(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
release := make(chan struct{})
|
||||
|
||||
t.Cleanup(func() { close(release) })
|
||||
|
||||
var wg sync.WaitGroup
|
||||
|
||||
wg.Go(func() { <-release })
|
||||
|
||||
ctx, cancel := context.WithTimeout(
|
||||
context.Background(), waitTimeout,
|
||||
)
|
||||
defer cancel()
|
||||
|
||||
err := lifecycle.WaitForShutdown(
|
||||
ctx, discardLogger(), "test component", &wg,
|
||||
)
|
||||
|
||||
require.ErrorIs(t, err, context.DeadlineExceeded)
|
||||
require.ErrorContains(t, err, "test component")
|
||||
}
|
||||
@@ -25,6 +25,11 @@ func IPFromHostPort(hp string) string {
|
||||
return ipFromHostPort(hp)
|
||||
}
|
||||
|
||||
// ClientKeyForTest exposes clientKey for testing.
|
||||
func ClientKeyForTest(m *Middleware, r *http.Request) string {
|
||||
return m.clientKey(r)
|
||||
}
|
||||
|
||||
// IsClientTLS exposes isClientTLS for testing.
|
||||
func IsClientTLS(r *http.Request) bool {
|
||||
return isClientTLS(r)
|
||||
|
||||
@@ -32,6 +32,13 @@ const (
|
||||
// receiver rate limit. The configured limit is expressed in
|
||||
// requests per minute.
|
||||
receiverRateInterval = 1 * time.Minute
|
||||
|
||||
// maxForwardedHops bounds how many X-Forwarded-For entries the
|
||||
// chain walk examines. Real chains are one to three hops, but a
|
||||
// client can pad the header up to MaxHeaderBytes, so without a
|
||||
// bound every request pays a walk proportional to whatever the
|
||||
// client sent.
|
||||
maxForwardedHops = 64
|
||||
)
|
||||
|
||||
// normalizeAddr strips the IPv4-in-IPv6 wrapper and any zone from
|
||||
@@ -70,26 +77,49 @@ func (m *Middleware) isTrustedProxy(addr netip.Addr) bool {
|
||||
// a trusted proxy is the client. A hop that cannot be read as a bare
|
||||
// address ends the walk: past it the chain is not the shape assumed
|
||||
// here, so the caller falls back to the peer address.
|
||||
//
|
||||
// Only the last maxForwardedHops entries are examined. A longer chain
|
||||
// is padding, and running out of hops falls back to the peer address
|
||||
// the same way an unreadable hop does.
|
||||
//
|
||||
// The entries are cut off the right end of each header value in place
|
||||
// rather than split out of it: the receiver is unauthenticated and a
|
||||
// client can pad the header up to MaxHeaderBytes, so splitting would
|
||||
// allocate in proportion to the padding (about 8 MB for a 1 MB
|
||||
// header) before the cap could discard any of it. Multiple header
|
||||
// values are walked in reverse for the same reason, since joining
|
||||
// them copies the whole chain.
|
||||
func (m *Middleware) forwardedClientAddr(
|
||||
r *http.Request,
|
||||
) (netip.Addr, bool) {
|
||||
hops := strings.Split(
|
||||
strings.Join(r.Header.Values("X-Forwarded-For"), ","), ",",
|
||||
)
|
||||
seen := 0
|
||||
|
||||
for _, hop := range slices.Backward(hops) {
|
||||
hop = strings.TrimSpace(hop)
|
||||
if hop == "" {
|
||||
continue
|
||||
}
|
||||
for _, value := range slices.Backward(
|
||||
r.Header.Values("X-Forwarded-For"),
|
||||
) {
|
||||
for last := false; !last && seen < maxForwardedHops; seen++ {
|
||||
hop := value
|
||||
|
||||
addr, err := netip.ParseAddr(hop)
|
||||
if err != nil {
|
||||
return netip.Addr{}, false
|
||||
}
|
||||
comma := strings.LastIndexByte(value, ',')
|
||||
if comma < 0 {
|
||||
last = true
|
||||
} else {
|
||||
hop, value = value[comma+1:], value[:comma]
|
||||
}
|
||||
|
||||
if addr = normalizeAddr(addr); !m.isTrustedProxy(addr) {
|
||||
return addr, true
|
||||
hop = strings.TrimSpace(hop)
|
||||
if hop == "" {
|
||||
continue
|
||||
}
|
||||
|
||||
addr, err := netip.ParseAddr(hop)
|
||||
if err != nil {
|
||||
return netip.Addr{}, false
|
||||
}
|
||||
|
||||
if addr = normalizeAddr(addr); !m.isTrustedProxy(addr) {
|
||||
return addr, true
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -113,8 +143,10 @@ func (m *Middleware) clientKey(r *http.Request) string {
|
||||
peer, err := netip.ParseAddr(ipFromHostPort(r.RemoteAddr))
|
||||
if err != nil {
|
||||
// Not an address we can reason about; key on the raw
|
||||
// value rather than collapsing such peers into one
|
||||
// shared bucket.
|
||||
// value, the most specific identity left. On a
|
||||
// Unix-socket listener every peer carries the same
|
||||
// RemoteAddr and so shares one bucket, which is the
|
||||
// fail-closed direction.
|
||||
return r.RemoteAddr
|
||||
}
|
||||
|
||||
|
||||
@@ -8,7 +8,10 @@ import (
|
||||
"net/http/httptest"
|
||||
"net/netip"
|
||||
"os"
|
||||
"runtime"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"sneak.berlin/go/webhooker/internal/config"
|
||||
@@ -568,6 +571,105 @@ func TestRateLimitKey_ChainWalkSkipsClientPrepended(t *testing.T) {
|
||||
)
|
||||
}
|
||||
|
||||
// TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer covers the
|
||||
// hop-walk cap. A client behind the trusted proxy can pad
|
||||
// X-Forwarded-For with tens of thousands of trusted-looking hops,
|
||||
// which costs a walk proportional to the padding and, once the walk
|
||||
// runs off the left end of the chain, reaches the entry the client
|
||||
// put there. Capping the walk stops both: the key falls back to the
|
||||
// peer address, so rotating the head of the chain mints no bucket,
|
||||
// and the run does not scale with the chain length.
|
||||
func TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
|
||||
// 50k hops is roughly 0.9 MB, within the default
|
||||
// MaxHeaderBytes.
|
||||
const hops = 50000
|
||||
|
||||
padding := strings.Repeat(", 10.0.0.2", hops-1)
|
||||
|
||||
start := time.Now()
|
||||
|
||||
assertSharedBucket(
|
||||
t, trustedProxies("10.0.0.0/8"), "10.0.0.1:44444",
|
||||
func(i int) map[string]string {
|
||||
return map[string]string{
|
||||
headerXFF: fmt.Sprintf("9.9.9.%d%s", i+1, padding),
|
||||
}
|
||||
},
|
||||
"a padded X-Forwarded-For chain must fall back to the "+
|
||||
"peer address, not reach the client-controlled entry "+
|
||||
"at the head of the chain",
|
||||
)
|
||||
|
||||
assert.Less(
|
||||
t, time.Since(start), 2*time.Second,
|
||||
"the capped walk must not scale with the chain length",
|
||||
)
|
||||
}
|
||||
|
||||
// TestRateLimitKey_LongChainAllocationIsBounded is the allocation
|
||||
// half of the hop cap. Capping the walk still left every request
|
||||
// paying for the whole header the client sent, because the chain was
|
||||
// split before it was capped: about 8 MB of []string for the 1 MB a
|
||||
// default MaxHeaderBytes allows, on the unauthenticated receiver.
|
||||
//
|
||||
// Bytes are the measurement, not allocation count: strings.Split of a
|
||||
// 1 MB chain is a single allocation, so testing.AllocsPerRun scores
|
||||
// it as cheap. The test is deliberately sequential — it reads
|
||||
// process-wide counters, and Go runs this package's parallel tests
|
||||
// only after the sequential ones finish.
|
||||
//
|
||||
//nolint:paralleltest // reads process-wide allocation counters
|
||||
func TestRateLimitKey_LongChainAllocationIsBounded(t *testing.T) {
|
||||
// 100k hops of ", 10.0.0.2" is roughly 1 MB.
|
||||
const (
|
||||
hops = 100000
|
||||
iterations = 50
|
||||
maxBytesPerCall = 4096
|
||||
)
|
||||
|
||||
m := rateLimitMiddleware(t, &config.Config{
|
||||
TrustedProxies: trustedProxies("10.0.0.0/8"),
|
||||
})
|
||||
|
||||
req := httptest.NewRequestWithContext(
|
||||
context.Background(), http.MethodPost, loginPath, nil,
|
||||
)
|
||||
req.RemoteAddr = "10.0.0.1:44444"
|
||||
req.Header.Set(
|
||||
headerXFF, "9.9.9.9"+strings.Repeat(", 10.0.0.2", hops),
|
||||
)
|
||||
|
||||
var before, after runtime.MemStats
|
||||
|
||||
var key string
|
||||
|
||||
runtime.ReadMemStats(&before)
|
||||
|
||||
for range iterations {
|
||||
key = middleware.ClientKeyForTest(m, req)
|
||||
}
|
||||
|
||||
runtime.ReadMemStats(&after)
|
||||
|
||||
perCall := (after.TotalAlloc - before.TotalAlloc) / iterations
|
||||
|
||||
assert.Less(
|
||||
t, perCall, uint64(maxBytesPerCall),
|
||||
"a %d-byte X-Forwarded-For must not allocate in proportion "+
|
||||
"to its length, but cost %d bytes per call",
|
||||
len(req.Header.Get(headerXFF)), perCall,
|
||||
)
|
||||
|
||||
assert.Equal(
|
||||
t, "10.0.0.1", key,
|
||||
"the padded chain must still fall back to the peer address",
|
||||
)
|
||||
}
|
||||
|
||||
// TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer proves
|
||||
// the receiver limiter uses the same gated key function as the
|
||||
// POST limiters.
|
||||
|
||||
@@ -1,5 +1,4 @@
|
||||
// Webhooker client-side JavaScript
|
||||
console.log("Webhooker loaded");
|
||||
|
||||
// Copy-to-clipboard, as progressive enhancement.
|
||||
//
|
||||
|
||||
Reference in New Issue
Block a user