sneak/webhooker
1
0
Fork 0
Code Issues 31 Pull Requests 5 Actions Packages Projects Releases Wiki Activity

Warn when production shares one rate-limit bucket (closes #149) #153

Merged
clawbot merged 1 commits from issue-149-trusted-proxies-warning into next 2026-08-12 13:49:39 +02:00
Conversation 1 Commits 1 Files Changed 4 +175 -13
clawbot commented 2026-08-12 13:42:25 +02:00
Collaborator
Copy Link

Closes #149.

Visibility only: the rate-limit keying, the limits and the TRUSTED_PROXIES default are unchanged.

What changed

  • internal/config/config.go: new warnSharedRateLimitBucket, called from New right after the "Configuration loaded" line. Fires only when IsProd() and len(TrustedProxies) == 0.
  • README.md env-var table + #### Trusted proxies: state the consequence and name TRUSTED_PROXIES as the remedy.
  • README.md ### Rate Limiting: the shared-bucket paragraph now splits the receiver case (costs throughput, safe direction) from the login/password-change case (costs availability of the only admin path, not safe). The verified aggregate-ceiling text is preserved verbatim inside the receiver bullet.
  • README.md security-features bullet: the "5 POST attempts per minute per IP" claim is now conditional on TRUSTED_PROXIES.
  • internal/config/config_test.go + export_test.go: TestSharedRateLimitBucketWarning, asserting against a test-controlled slog handler.

TODO.md deliberately untouched (owned by #151).

Warning text

Level WARN, attrs environment, trustedProxies:

> TRUSTED_PROXIES is empty: rate limits key on the connecting peer, so behind the reverse proxy a production deployment runs behind, every client shares one bucket per limit. Any remote client can then keep the login limit full and deny the admin login, the only administrative path, until restart. Set TRUSTED_PROXIES to your reverse proxy's address.

Mutation evidence

  • Warning removed (early return unconditional): --- FAIL: TestSharedRateLimitBucketWarning/prod_without_trusted_proxies_warns, four "" does not contain assertions.
  • Condition removed the other way (always warn): --- FAIL: .../prod_with_trusted_proxies_is_quiet and --- FAIL: .../dev_without_trusted_proxies_is_quiet.
  • Restored: all three subtests pass.

Gate evidence

  • make check exit 0, 0 issues.
  • docker build --no-cache-filter=lint,builder --progress=plain . exit 0. In-container lint ran (#16 53.57 0 issues.) and tests ran uncached (#23 ok sneak.berlin/go/webhooker/internal/config 1.113s, not (cached)), including --- PASS: TestSharedRateLimitBucketWarning and its three subtests.
Closes https://git.eeqj.de/sneak/webhooker/issues/149. Visibility only: the rate-limit keying, the limits and the `TRUSTED_PROXIES` default are unchanged. ## What changed - `internal/config/config.go`: new `warnSharedRateLimitBucket`, called from `New` right after the "Configuration loaded" line. Fires only when `IsProd()` and `len(TrustedProxies) == 0`. - `README.md` env-var table + `#### Trusted proxies`: state the consequence and name `TRUSTED_PROXIES` as the remedy. - `README.md` `### Rate Limiting`: the shared-bucket paragraph now splits the receiver case (costs throughput, safe direction) from the login/password-change case (costs availability of the only admin path, not safe). The verified aggregate-ceiling text is preserved verbatim inside the receiver bullet. - `README.md` security-features bullet: the "5 POST attempts per minute per IP" claim is now conditional on `TRUSTED_PROXIES`. - `internal/config/config_test.go` + `export_test.go`: `TestSharedRateLimitBucketWarning`, asserting against a test-controlled `slog` handler. `TODO.md` deliberately untouched (owned by https://git.eeqj.de/sneak/webhooker/issues/151). ## Warning text Level `WARN`, attrs `environment`, `trustedProxies`: > TRUSTED_PROXIES is empty: rate limits key on the connecting peer, so behind the reverse proxy a production deployment runs behind, every client shares one bucket per limit. Any remote client can then keep the login limit full and deny the admin login, the only administrative path, until restart. Set TRUSTED_PROXIES to your reverse proxy's address. ## Mutation evidence - Warning removed (early return unconditional): `--- FAIL: TestSharedRateLimitBucketWarning/prod_without_trusted_proxies_warns`, four `"" does not contain` assertions. - Condition removed the other way (always warn): `--- FAIL: .../prod_with_trusted_proxies_is_quiet` and `--- FAIL: .../dev_without_trusted_proxies_is_quiet`. - Restored: all three subtests pass. ## Gate evidence - `make check` exit 0, `0 issues.` - `docker build --no-cache-filter=lint,builder --progress=plain .` exit 0. In-container lint ran (`#16 53.57 0 issues.`) and tests ran uncached (`#23 ok sneak.berlin/go/webhooker/internal/config 1.113s`, not `(cached)`), including `--- PASS: TestSharedRateLimitBucketWarning` and its three subtests.
clawbot added 1 commit 2026-08-12 13:42:26 +02:00
Warn when production shares one rate-limit bucket (closes #149)
All checks were successful
check / check (push) Successful in 3m7s
Details
0beeddd475 
With TRUSTED_PROXIES empty, every rate limiter keys on the connecting
peer. Production runs behind a TLS-terminating reverse proxy, so the
peer is that proxy for every request and all clients share one bucket
per limit. For the login limiter that means any remote client sending
five POSTs a minute holds the only administrative login at HTTP 429.

The empty default is correct — trusting forwarded headers from
arbitrary peers lets any client choose its own bucket — so this makes
the consequence visible rather than changing the keying, the limits or
the default:

- config logs a WARN at startup when the environment is prod and
  TRUSTED_PROXIES is empty, naming the variable, the shared bucket and
  the deniable admin login.
- The security-feature bullet's "per IP" login claim is now conditional
  on TRUSTED_PROXIES, which is the only case where it holds.
- The rate-limiting section separates the receiver case (sharing costs
  throughput, the safe direction) from the login case (sharing costs
  availability of the only admin path, not safe).
- The trusted-proxies configuration section states the consequence and
  names TRUSTED_PROXIES as the remedy.
clawbot added the needs-review label 2026-08-12 13:42:31 +02:00
clawbot self-assigned this 2026-08-12 13:42:34 +02:00
clawbot commented 2026-08-12 13:48:42 +02:00
Author
Collaborator
Copy Link

PASS. Every clause of the warning and all three README rewrites verified against source; DoD for #149 met, scope held (keying, limits and the TRUSTED_PROXIES default untouched), single commit on next, mergeable, no attribution trailers.

Nit, non-blocking: README.md:326-327 still carries the uncorrected form of the very claim this PR fixes elsewhere — "go-chi/httprate for per-IP login rate limiting" — with no TRUSTED_PROXIES qualifier and no nearby text that supplies one, unlike README.md:864 and :871 which are qualified by the paragraph at :907. Same class of inaccuracy as the corrected :1148-1155, but outside the three passages the issue enumerated, so not held against this PR. Acceptable would be a trailing "(per bucket; per IP only with TRUSTED_PROXIES set)" or a link to Rate Limiting.

Disclosure: the warning is gated on IsProd(), and WEBHOOKER_ENVIRONMENT defaults to dev, so an internet-exposed deployment that never set it gets no warning. That matches the issue's DoD wording exactly ("in production mode") and is not treated as a defect here; flagging it as input to #150.

Gate evidence on 0beeddd: CI check / check (push) success in 3m7s. Local make check exit 0, zero (cached) test markers, 0 issues. docker build --no-cache-filter=lint,builder exit 0 in 3m0.8s; in-container lint executed (#16 49.42 0 issues., no parallel-linter message), tests uncached (ok sneak.berlin/go/webhooker/internal/config 1.112s) with all three warning subtests passing. Mutation reproduced independently in both directions in a throwaway copy: forcing the guard to always return fails prod_without_trusted_proxies_warns on all four assertions; forcing it never to return fails both quiet subtests.

PASS. Every clause of the warning and all three README rewrites verified against source; DoD for https://git.eeqj.de/sneak/webhooker/issues/149 met, scope held (keying, limits and the `TRUSTED_PROXIES` default untouched), single commit on `next`, mergeable, no attribution trailers. Nit, non-blocking: `README.md:326-327` still carries the uncorrected form of the very claim this PR fixes elsewhere — "**go-chi/httprate** for per-IP login rate limiting" — with no `TRUSTED_PROXIES` qualifier and no nearby text that supplies one, unlike `README.md:864` and `:871` which are qualified by the paragraph at `:907`. Same class of inaccuracy as the corrected `:1148-1155`, but outside the three passages the issue enumerated, so not held against this PR. Acceptable would be a trailing "(per bucket; per IP only with `TRUSTED_PROXIES` set)" or a link to [Rate Limiting](https://git.eeqj.de/sneak/webhooker/src/branch/next/README.md#rate-limiting). Disclosure: the warning is gated on `IsProd()`, and `WEBHOOKER_ENVIRONMENT` defaults to `dev`, so an internet-exposed deployment that never set it gets no warning. That matches the issue's DoD wording exactly ("in production mode") and is not treated as a defect here; flagging it as input to https://git.eeqj.de/sneak/webhooker/issues/150. Gate evidence on `0beeddd`: CI `check / check (push)` success in 3m7s. Local `make check` exit 0, zero `(cached)` test markers, `0 issues.` `docker build --no-cache-filter=lint,builder` exit 0 in 3m0.8s; in-container lint executed (`#16 49.42 0 issues.`, no parallel-linter message), tests uncached (`ok sneak.berlin/go/webhooker/internal/config 1.112s`) with all three warning subtests passing. Mutation reproduced independently in both directions in a throwaway copy: forcing the guard to always return fails `prod_without_trusted_proxies_warns` on all four assertions; forcing it never to return fails both quiet subtests.
clawbot merged commit d8f9d149b5 into next 2026-08-12 13:49:39 +02:00
clawbot deleted branch issue-149-trusted-proxies-warning 2026-08-12 13:49:39 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-review
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#153
Delete Branch "issue-149-trusted-proxies-warning"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?