Release-blocking accuracy: README describes a build and an endpoint that do not exist, and the lockout warning misses the default environment #151

New Issue

clawbot · 2026-08-12T13:32:46+02:00

clawbot commented

2026-08-12 13:32:46 +02:00

Found by the final integration review of #111, plus one item from the review of #153. Blocks the 1.0.0 tag: publishing this README ships false statements about the product.

1. The lockout warning does not fire in the default environment

Added by #149 and gated on IsProd(). But WEBHOOKER_ENVIRONMENT defaults to dev, so an internet-exposed deployment whose operator never set that variable gets NO warning, despite having exactly the shared-bucket exposure the warning exists to announce. "Forgot to set the env var" is precisely the operator-error class this is meant to catch, so gating on another env var being set correctly defeats it.

That gating came from #149's own definition of done, which said "in production mode" — the scoping was wrong, not the implementation.

Fix: emit the warning whenever TRUSTED_PROXIES is empty, regardless of environment. Word it so it is accurate in both cases — behind a proxy it means shared buckets and a remotely deniable admin login; with no proxy in front (a plausible dev setup) it is harmless and the text should not imply otherwise. Extend the existing test so the dev-with-empty case now asserts the warning IS emitted.

2. The Docker build section describes a build that does not exist

README.md:1131-1135 describes two stages, golang:1.24, and a builder that "installs golangci-lint" and "runs make check".

Actual Dockerfile: three stages — a golangci/golangci-lint:v2.12.2 lint stage running make fmt-check and make lint, and a golang:1.26.1-bookworm builder running make test and make build. It contradicts the passage fifteen lines below it added by #119, and describes the single-builder shape REPO_POLICIES.md:102-106 forbids.

3. The receiver endpoint's accepted methods are wrong

README.md:917 documents the public webhook endpoint as ANY ... (accepts all methods). internal/handlers/webhook.go:24-33 returns 405 for everything except POST. README.md:514 compounds it with "HTTP method (POST, PUT, etc.)". This is the one endpoint the public interacts with.

4. The last unqualified per-IP rate-limit claim

README.md:326-327 still reads "go-chi/httprate for per-IP login rate limiting" — the exact unconditional claim #149 corrected everywhere else. Qualify it consistently with the rest.

5. A configuration section is orphaned under the wrong heading

README.md:148-166 documents session expiry but sits inside the #### Trusted proxies subsection: #66 placed it under ### Configuration, then #88 inserted a heading above it. Visible only once both landed.

6. `SESSION_IDLE_TIMEOUT` is missing from the startup summary

internal/config/config.go logs the effective configuration at startup but omits sessionIdleTimeout. It is the one variable where a perfectly VALID value (any non-positive number) silently disables a security control.

7. `TODO.md` Future Steps contradicts Completed Steps

Future Steps lists "Password change and reset flow" while Completed Steps records #65 as landed. The change flow exists; reset does not — split the entry rather than deleting it.

8. `TODO.md` overclaims CI verification

The Status paragraph says next is "verified green both by CI and by cache-defeated container runs". The cache-defeated half is true and is the stronger evidence; the CI half is not currently supported (#152). Claim only what is true.

Definition of done

Every statement above matches the code.
SWEEP THE WHOLE README rather than fixing these lines only. #141 failed review for fixing exactly the lines it was given, and its successor failed again for the same reason. Verify every documented HTTP route's methods against internal/server/routes.go, and the build/deployment section against the actual Dockerfile and workflow.
Report the complete list of statements you checked, including those you judged correct.

Implementation requirements

This unit MAY edit TODO.md for items 7 and 8 — the documented exception to #112.
Do NOT change rate-limit keying, limits, or the TRUSTED_PROXIES default — that is #150, an owner decision. Item 1 changes only WHEN the warning fires and how it is worded.
Branch from next, PR based on next, single commit, title ending (closes #N).
Gate on make check plus the Docker lint path with the cache defeated.

Found by the final integration review of https://git.eeqj.de/sneak/webhooker/pulls/111, plus one item from the review of https://git.eeqj.de/sneak/webhooker/pulls/153. Blocks the 1.0.0 tag: publishing this README ships false statements about the product. ## 1. The lockout warning does not fire in the default environment Added by https://git.eeqj.de/sneak/webhooker/issues/149 and gated on `IsProd()`. But `WEBHOOKER_ENVIRONMENT` defaults to `dev`, so an internet-exposed deployment whose operator never set that variable gets NO warning, despite having exactly the shared-bucket exposure the warning exists to announce. "Forgot to set the env var" is precisely the operator-error class this is meant to catch, so gating on another env var being set correctly defeats it. That gating came from https://git.eeqj.de/sneak/webhooker/issues/149's own definition of done, which said "in production mode" — the scoping was wrong, not the implementation. Fix: emit the warning whenever `TRUSTED_PROXIES` is empty, regardless of environment. Word it so it is accurate in both cases — behind a proxy it means shared buckets and a remotely deniable admin login; with no proxy in front (a plausible dev setup) it is harmless and the text should not imply otherwise. Extend the existing test so the dev-with-empty case now asserts the warning IS emitted. ## 2. The Docker build section describes a build that does not exist `README.md:1131-1135` describes two stages, `golang:1.24`, and a builder that "installs golangci-lint" and "runs `make check`". Actual `Dockerfile`: three stages — a `golangci/golangci-lint:v2.12.2` lint stage running `make fmt-check` and `make lint`, and a `golang:1.26.1-bookworm` builder running `make test` and `make build`. It contradicts the passage fifteen lines below it added by https://git.eeqj.de/sneak/webhooker/issues/119, and describes the single-builder shape `REPO_POLICIES.md:102-106` forbids. ## 3. The receiver endpoint's accepted methods are wrong `README.md:917` documents the public webhook endpoint as `ANY ... (accepts all methods)`. `internal/handlers/webhook.go:24-33` returns 405 for everything except POST. `README.md:514` compounds it with "HTTP method (POST, PUT, etc.)". This is the one endpoint the public interacts with. ## 4. The last unqualified per-IP rate-limit claim `README.md:326-327` still reads "**go-chi/httprate** for per-IP login rate limiting" — the exact unconditional claim https://git.eeqj.de/sneak/webhooker/issues/149 corrected everywhere else. Qualify it consistently with the rest. ## 5. A configuration section is orphaned under the wrong heading `README.md:148-166` documents session expiry but sits inside the `#### Trusted proxies` subsection: https://git.eeqj.de/sneak/webhooker/issues/66 placed it under `### Configuration`, then https://git.eeqj.de/sneak/webhooker/issues/88 inserted a heading above it. Visible only once both landed. ## 6. `SESSION_IDLE_TIMEOUT` is missing from the startup summary `internal/config/config.go` logs the effective configuration at startup but omits `sessionIdleTimeout`. It is the one variable where a perfectly VALID value (any non-positive number) silently disables a security control. ## 7. `TODO.md` Future Steps contradicts Completed Steps Future Steps lists "Password change and reset flow" while Completed Steps records https://git.eeqj.de/sneak/webhooker/issues/65 as landed. The change flow exists; reset does not — split the entry rather than deleting it. ## 8. `TODO.md` overclaims CI verification The Status paragraph says `next` is "verified green both by CI and by cache-defeated container runs". The cache-defeated half is true and is the stronger evidence; the CI half is not currently supported (https://git.eeqj.de/sneak/webhooker/issues/152). Claim only what is true. ## Definition of done - Every statement above matches the code. - SWEEP THE WHOLE README rather than fixing these lines only. https://git.eeqj.de/sneak/webhooker/issues/141 failed review for fixing exactly the lines it was given, and its successor failed again for the same reason. Verify every documented HTTP route's methods against `internal/server/routes.go`, and the build/deployment section against the actual `Dockerfile` and workflow. - Report the complete list of statements you checked, including those you judged correct. ## Implementation requirements - This unit MAY edit `TODO.md` for items 7 and 8 — the documented exception to https://git.eeqj.de/sneak/webhooker/issues/112. - Do NOT change rate-limit keying, limits, or the `TRUSTED_PROXIES` default — that is https://git.eeqj.de/sneak/webhooker/issues/150, an owner decision. Item 1 changes only WHEN the warning fires and how it is worded. - Branch from `next`, PR based on `next`, single commit, title ending ` (closes #N)`. - Gate on `make check` plus the Docker lint path with the cache defeated.

clawbot added this to the 1.0.0 milestone 2026-08-12 13:32:46 +02:00

clawbot self-assigned this 2026-08-12 13:32:46 +02:00

clawbot referenced this issue

2026-08-12 13:42:25 +02:00

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

clawbot changed title from ~~Release-blocking README inaccuracies: the Docker build section and the receiver endpoint's accepted methods~~ to Release-blocking accuracy: README describes a build and an endpoint that do not exist, and the lockout warning misses the default environment

2026-08-12 13:50:03 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#151

Release-blocking accuracy: README describes a build and an endpoint that do not exist, and the lockout warning misses the default environment #151