METRICS_USERNAME alone mounted /metrics behind a credential map whose only password was the empty string, so curl -u 'metrics:' returned 200 while the startup log reported hasMetricsAuth:false. The route mount tested the username (internal/server/routes.go), the log tested both variables (internal/config/config.go), so the two could disagree about whether the endpoint existed.
Approach
One value behind both.Config.MetricsAuthEnabled() is now the only answer to "is /metrics served": the route mount, the Prometheus recording middleware and the startup log's hasMetricsAuth field all read it. It requires both credentials, not the username alone, so a Config assembled in code (which bypasses environment validation) still cannot publish an endpoint that accepts an empty password.
Fail loudly on half-set.loadFromEnv rejects a half-set pair via a new ErrIncompleteMetricsAuth, naming both variables in either direction. A set-but-invalid configuration aborts startup rather than degrading into something the operator did not ask for. Neither fallback was acceptable: mounting on the username alone publishes an endpoint with an empty password, and quietly leaving it unmounted withholds an endpoint that was asked for. The error carries neither credential value — the password is a secret.
Both unset remains valid and leaves /metrics unmounted.
Definition of done
Requirement
Where
METRICS_USERNAME set + password empty/unset is a startup error naming both variables, exit non-zero
resolveMetricsAuth
Converse (METRICS_PASSWORD set, username empty) is the same error
same, direction-independent
Both unset stays valid, /metrics unmounted
MetricsAuthEnabled() false
hasMetricsAuth cannot disagree with the mount
both read MetricsAuthEnabled()
Tests cover all four combinations
TestMetricsAuthConfig
TestMetricsAuthConfig treats "set to the empty string" and "not set at all" as separate inputs on each side (7 rows), since the reported bug arrived through the first. TestMetricsRouteUnmountedWithoutCredentials, TestMetricsRouteRequiresCredentials and TestMetricsRouteUnmountedOnHalfSetConfig pin the same states against the real route tree; the middle one asserts an empty password gets 401.
newTestEnv in internal/server/routes_test.go gained a newTestEnvWithConfig variant so a test can drive the production route tree with a chosen Config; the same pointer reaches the router and every middleware.
Lint ran in Docker (Dockerfile.lint, golangci-lint v2.12.2 pinned by digest):
#12 [lint 3/3] RUN --network=none golangci-lint run --config .golangci.yml ./...
#12 88.21 0 issues.
#12 DONE 92.8s
EXIT=0
Cache-defeated container build — exit 0
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain ., run on the rebased tree. The CACHED layers are only the two base-image FROM resolutions and the dependency-download steps (go.mod/go.sum unchanged); the lint and test layers executed:
#22 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#22 75.18 0 issues.
#35 [builder 9/11] RUN make test
All 15 packages ran with real durations, no (cached) lines:
#35 93.16 ok sneak.berlin/go/webhooker/internal/config 1.472s
#35 139.1 ok sneak.berlin/go/webhooker/internal/server 6.802s
#35 139.1 ok sneak.berlin/go/webhooker/internal/middleware 5.300s
#35 139.1 ok sneak.berlin/go/webhooker/internal/handlers 52.040s
Build images and containers were removed afterwards; docker ps -a and docker images show nothing of this run. No prune was used.
Live verification against the issue's own reproduction
Half-set, both directions — the process refuses to start, exit 1:
$ METRICS_USERNAME=metrics ./bin/webhooker # METRICS_PASSWORD unset
incomplete metrics credentials: METRICS_USERNAME is set but METRICS_PASSWORD is empty;
METRICS_USERNAME and METRICS_PASSWORD must both be set to serve /metrics, or both be
empty to leave it unmounted
exit=1
$ METRICS_PASSWORD=s3cret ./bin/webhooker # METRICS_USERNAME unset
incomplete metrics credentials: METRICS_PASSWORD is set but METRICS_USERNAME is empty;
...
exit=1
Both set — hasMetricsAuth:true, and the issue's curl -u 'metrics:' now gets 401 where it previously got 200:
README: both env-var rows note the pairing requirement, a new "Metrics credentials" subsection states the three outcomes, and the endpoint table plus the middleware list no longer describe the mount as keyed on METRICS_USERNAME alone. make fmt run and included.
Closes https://git.eeqj.de/sneak/webhooker/issues/205.
## The defect
`METRICS_USERNAME` alone mounted `/metrics` behind a credential map whose only password was the empty string, so `curl -u 'metrics:'` returned 200 while the startup log reported `hasMetricsAuth:false`. The route mount tested the username (`internal/server/routes.go`), the log tested both variables (`internal/config/config.go`), so the two could disagree about whether the endpoint existed.
## Approach
**One value behind both.** `Config.MetricsAuthEnabled()` is now the only answer to "is `/metrics` served": the route mount, the Prometheus recording middleware and the startup log's `hasMetricsAuth` field all read it. It requires *both* credentials, not the username alone, so a `Config` assembled in code (which bypasses environment validation) still cannot publish an endpoint that accepts an empty password.
**Fail loudly on half-set.** `loadFromEnv` rejects a half-set pair via a new `ErrIncompleteMetricsAuth`, naming both variables in either direction. A set-but-invalid configuration aborts startup rather than degrading into something the operator did not ask for. Neither fallback was acceptable: mounting on the username alone publishes an endpoint with an empty password, and quietly leaving it unmounted withholds an endpoint that was asked for. The error carries neither credential value — the password is a secret.
Both unset remains valid and leaves `/metrics` unmounted.
## Definition of done
| Requirement | Where |
| --- | --- |
| `METRICS_USERNAME` set + password empty/unset is a startup error naming both variables, exit non-zero | `resolveMetricsAuth` |
| Converse (`METRICS_PASSWORD` set, username empty) is the same error | same, direction-independent |
| Both unset stays valid, `/metrics` unmounted | `MetricsAuthEnabled()` false |
| `hasMetricsAuth` cannot disagree with the mount | both read `MetricsAuthEnabled()` |
| Tests cover all four combinations | `TestMetricsAuthConfig` |
`TestMetricsAuthConfig` treats "set to the empty string" and "not set at all" as separate inputs on each side (7 rows), since the reported bug arrived through the first. `TestMetricsRouteUnmountedWithoutCredentials`, `TestMetricsRouteRequiresCredentials` and `TestMetricsRouteUnmountedOnHalfSetConfig` pin the same states against the real route tree; the middle one asserts an empty password gets 401.
`newTestEnv` in `internal/server/routes_test.go` gained a `newTestEnvWithConfig` variant so a test can drive the production route tree with a chosen `Config`; the same pointer reaches the router and every middleware.
`TODO.md` deliberately untouched (https://git.eeqj.de/sneak/webhooker/issues/112).
## Gate evidence
### `make check` — exit 0
Lint ran in Docker (`Dockerfile.lint`, golangci-lint v2.12.2 pinned by digest):
```
#12 [lint 3/3] RUN --network=none golangci-lint run --config .golangci.yml ./...
#12 88.21 0 issues.
#12 DONE 92.8s
EXIT=0
```
### Cache-defeated container build — exit 0
`docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .`, run on the rebased tree. The `CACHED` layers are only the two base-image `FROM` resolutions and the dependency-download steps (`go.mod`/`go.sum` unchanged); the lint and test layers executed:
```
#22 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#22 75.18 0 issues.
#35 [builder 9/11] RUN make test
```
All 15 packages ran with real durations, no `(cached)` lines:
```
#35 93.16 ok sneak.berlin/go/webhooker/internal/config 1.472s
#35 139.1 ok sneak.berlin/go/webhooker/internal/server 6.802s
#35 139.1 ok sneak.berlin/go/webhooker/internal/middleware 5.300s
#35 139.1 ok sneak.berlin/go/webhooker/internal/handlers 52.040s
```
New tests, in-container:
```
#35 93.16 --- PASS: TestMetricsAuthConfig (0.16s)
#35 93.16 --- PASS: TestMetricsAuthConfig/both_unset_leaves_metrics_unmounted
#35 93.16 --- PASS: TestMetricsAuthConfig/both_empty_leaves_metrics_unmounted
#35 93.16 --- PASS: TestMetricsAuthConfig/both_set_enables_metrics_auth
#35 93.16 --- PASS: TestMetricsAuthConfig/username_with_unset_password_fails
#35 93.16 --- PASS: TestMetricsAuthConfig/username_with_empty_password_fails
#35 93.16 --- PASS: TestMetricsAuthConfig/password_with_unset_username_fails
#35 93.16 --- PASS: TestMetricsAuthConfig/password_with_empty_username_fails
#35 139.1 --- PASS: TestMetricsRouteRequiresCredentials (2.13s)
#35 139.1 --- PASS: TestMetricsRouteUnmountedWithoutCredentials (2.35s)
#35 139.1 --- PASS: TestMetricsRouteUnmountedOnHalfSetConfig (0.08s)
```
Build images and containers were removed afterwards; `docker ps -a` and `docker images` show nothing of this run. No prune was used.
### Live verification against the issue's own reproduction
Half-set, both directions — the process refuses to start, exit 1:
```
$ METRICS_USERNAME=metrics ./bin/webhooker # METRICS_PASSWORD unset
incomplete metrics credentials: METRICS_USERNAME is set but METRICS_PASSWORD is empty;
METRICS_USERNAME and METRICS_PASSWORD must both be set to serve /metrics, or both be
empty to leave it unmounted
exit=1
$ METRICS_PASSWORD=s3cret ./bin/webhooker # METRICS_USERNAME unset
incomplete metrics credentials: METRICS_PASSWORD is set but METRICS_USERNAME is empty;
...
exit=1
```
Both set — `hasMetricsAuth:true`, and the issue's `curl -u 'metrics:'` now gets 401 where it previously got 200:
```
"hasMetricsAuth":true
curl -u 'metrics:' /metrics --> 401
curl -u 'metrics:s3cret' /metrics --> 200
curl (no auth) /metrics --> 401
```
Both unset — `hasMetricsAuth:false` and the route does not exist:
```
"hasMetricsAuth":false
curl -u 'metrics:' /metrics --> 404
```
## Docs
README: both env-var rows note the pairing requirement, a new "Metrics credentials" subsection states the three outcomes, and the endpoint table plus the middleware list no longer describe the mount as keyed on `METRICS_USERNAME` alone. `make fmt` run and included.
METRICS_USERNAME alone mounted /metrics behind a credential map
whose only password was the empty string, so `curl -u 'metrics:'`
returned 200 while the startup log reported hasMetricsAuth:false.
The route mount tested the username and the log tested both, so the
two could disagree about whether the endpoint existed.
Config.MetricsAuthEnabled is now the single value behind both: the
/metrics mount, the Prometheus recording middleware and the startup
log's hasMetricsAuth field all read it, and it requires both
credentials. loadFromEnv rejects a half-set pair outright with an
error naming both variables in either direction, so a set-but-invalid
configuration aborts startup rather than degrading into an endpoint
the operator did not ask for. Both unset stays valid and leaves
/metrics unmounted.
Tests cover all four combinations at the config layer, counting
"set to the empty string" and "not set at all" as separate inputs,
plus the route tree's behaviour in each state.
PASS — satisfies every DoD item in #205; independently re-ran docker build --no-cache-filter=lint --no-cache-filter=builder on c172dee: lint 78.6s / 0 issues, make test 126.4s, all 15 packages with real durations and zero (cached) lines, exit 0.
Anomalies that pass anyway, for the record:
metrics.NewRecorder(metrics.Config{}) in internal/middleware/middleware.go:444 registers on prometheus.DefaultRegisterer via MustRegister, which panics on a duplicate. TestMetricsRouteRequiresCredentials is the first test in internal/server to construct a metrics-enabled router, so exactly one registration happens today and it passes. A second metrics-enabled router in that package (or go test -count=2) would panic. Relevant to #209: no conflict with registering new collectors — /metrics serves promhttp.Handler() over the default gatherer, so collectors registered elsewhere are picked up — but #209 must not add a second metrics-enabled router test without giving the recorder its own registry.
Middleware.MetricsAuth() is unchanged and still builds map[string][]string{username: {password}} unconditionally, so it would accept an empty password if ever called outside the MetricsAuthEnabled() guard. Unreachable today (single call site, inside the guard) and the password compare is constant-time (subtle.ConstantTimeCompare, with the length-mismatch dummy), so -u 'metrics:' gets 401. Noted as hardening, not a defect.
Disclosure — judgement call waived: Config.MetricsAuthEnabled() returns false for a half-set Config built in code rather than panicking, i.e. auth-off without noise. That is not a fail-loudly violation because every production Config comes from config.New/loadFromEnv, which now aborts, and it is documented at the method. Verified there is no config-reload path and no second production Config provider, so hasMetricsAuth and the mount read the same fields on the same never-mutated pointer.
Also checked: base next, fast-forward on 10c8dd2; CI green (4m49s, real); title and commit both end (closes #205); TODO.md untouched; no attribution trailers or vendor references; make fmt-check clean in-container; the two half-set config rows assert ErrIncompleteMetricsAuth and the absence of the password from the message, and TestMetricsRouteUnmountedOnHalfSetConfig is the assertion that actually regresses against pre-fix behaviour.
PASS — satisfies every DoD item in https://git.eeqj.de/sneak/webhooker/issues/205; independently re-ran `docker build --no-cache-filter=lint --no-cache-filter=builder` on `c172dee`: lint 78.6s / 0 issues, `make test` 126.4s, all 15 packages with real durations and zero `(cached)` lines, exit 0.
Anomalies that pass anyway, for the record:
- `metrics.NewRecorder(metrics.Config{})` in `internal/middleware/middleware.go:444` registers on `prometheus.DefaultRegisterer` via `MustRegister`, which panics on a duplicate. `TestMetricsRouteRequiresCredentials` is the first test in `internal/server` to construct a metrics-enabled router, so exactly one registration happens today and it passes. A second metrics-enabled router in that package (or `go test -count=2`) would panic. Relevant to https://git.eeqj.de/sneak/webhooker/issues/209: no conflict with registering new collectors — `/metrics` serves `promhttp.Handler()` over the default gatherer, so collectors registered elsewhere are picked up — but #209 must not add a second metrics-enabled router test without giving the recorder its own registry.
- `Middleware.MetricsAuth()` is unchanged and still builds `map[string][]string{username: {password}}` unconditionally, so it would accept an empty password if ever called outside the `MetricsAuthEnabled()` guard. Unreachable today (single call site, inside the guard) and the password compare is constant-time (`subtle.ConstantTimeCompare`, with the length-mismatch dummy), so `-u 'metrics:'` gets 401. Noted as hardening, not a defect.
Disclosure — judgement call waived: `Config.MetricsAuthEnabled()` returns false for a half-set `Config` built in code rather than panicking, i.e. auth-off without noise. That is not a fail-loudly violation because every production `Config` comes from `config.New`/`loadFromEnv`, which now aborts, and it is documented at the method. Verified there is no config-reload path and no second production `Config` provider, so `hasMetricsAuth` and the mount read the same fields on the same never-mutated pointer.
Also checked: base `next`, fast-forward on `10c8dd2`; CI green (4m49s, real); title and commit both end `(closes #205)`; `TODO.md` untouched; no attribution trailers or vendor references; `make fmt-check` clean in-container; the two half-set config rows assert `ErrIncompleteMetricsAuth` and the absence of the password from the message, and `TestMetricsRouteUnmountedOnHalfSetConfig` is the assertion that actually regresses against pre-fix behaviour.
clawbot
merged commit bb30b3ad64 into next2026-08-20 06:30:25 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #205.
The defect
METRICS_USERNAMEalone mounted/metricsbehind a credential map whose only password was the empty string, socurl -u 'metrics:'returned 200 while the startup log reportedhasMetricsAuth:false. The route mount tested the username (internal/server/routes.go), the log tested both variables (internal/config/config.go), so the two could disagree about whether the endpoint existed.Approach
One value behind both.
Config.MetricsAuthEnabled()is now the only answer to "is/metricsserved": the route mount, the Prometheus recording middleware and the startup log'shasMetricsAuthfield all read it. It requires both credentials, not the username alone, so aConfigassembled in code (which bypasses environment validation) still cannot publish an endpoint that accepts an empty password.Fail loudly on half-set.
loadFromEnvrejects a half-set pair via a newErrIncompleteMetricsAuth, naming both variables in either direction. A set-but-invalid configuration aborts startup rather than degrading into something the operator did not ask for. Neither fallback was acceptable: mounting on the username alone publishes an endpoint with an empty password, and quietly leaving it unmounted withholds an endpoint that was asked for. The error carries neither credential value — the password is a secret.Both unset remains valid and leaves
/metricsunmounted.Definition of done
METRICS_USERNAMEset + password empty/unset is a startup error naming both variables, exit non-zeroresolveMetricsAuthMETRICS_PASSWORDset, username empty) is the same error/metricsunmountedMetricsAuthEnabled()falsehasMetricsAuthcannot disagree with the mountMetricsAuthEnabled()TestMetricsAuthConfigTestMetricsAuthConfigtreats "set to the empty string" and "not set at all" as separate inputs on each side (7 rows), since the reported bug arrived through the first.TestMetricsRouteUnmountedWithoutCredentials,TestMetricsRouteRequiresCredentialsandTestMetricsRouteUnmountedOnHalfSetConfigpin the same states against the real route tree; the middle one asserts an empty password gets 401.newTestEnvininternal/server/routes_test.gogained anewTestEnvWithConfigvariant so a test can drive the production route tree with a chosenConfig; the same pointer reaches the router and every middleware.TODO.mddeliberately untouched (#112).Gate evidence
make check— exit 0Lint ran in Docker (
Dockerfile.lint, golangci-lint v2.12.2 pinned by digest):Cache-defeated container build — exit 0
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain ., run on the rebased tree. TheCACHEDlayers are only the two base-imageFROMresolutions and the dependency-download steps (go.mod/go.sumunchanged); the lint and test layers executed:All 15 packages ran with real durations, no
(cached)lines:New tests, in-container:
Build images and containers were removed afterwards;
docker ps -aanddocker imagesshow nothing of this run. No prune was used.Live verification against the issue's own reproduction
Half-set, both directions — the process refuses to start, exit 1:
Both set —
hasMetricsAuth:true, and the issue'scurl -u 'metrics:'now gets 401 where it previously got 200:Both unset —
hasMetricsAuth:falseand the route does not exist:Docs
README: both env-var rows note the pairing requirement, a new "Metrics credentials" subsection states the three outcomes, and the endpoint table plus the middleware list no longer describe the mount as keyed on
METRICS_USERNAMEalone.make fmtrun and included.PASS — satisfies every DoD item in #205; independently re-ran
docker build --no-cache-filter=lint --no-cache-filter=builderonc172dee: lint 78.6s / 0 issues,make test126.4s, all 15 packages with real durations and zero(cached)lines, exit 0.Anomalies that pass anyway, for the record:
metrics.NewRecorder(metrics.Config{})ininternal/middleware/middleware.go:444registers onprometheus.DefaultRegistererviaMustRegister, which panics on a duplicate.TestMetricsRouteRequiresCredentialsis the first test ininternal/serverto construct a metrics-enabled router, so exactly one registration happens today and it passes. A second metrics-enabled router in that package (orgo test -count=2) would panic. Relevant to #209: no conflict with registering new collectors —/metricsservespromhttp.Handler()over the default gatherer, so collectors registered elsewhere are picked up — but #209 must not add a second metrics-enabled router test without giving the recorder its own registry.Middleware.MetricsAuth()is unchanged and still buildsmap[string][]string{username: {password}}unconditionally, so it would accept an empty password if ever called outside theMetricsAuthEnabled()guard. Unreachable today (single call site, inside the guard) and the password compare is constant-time (subtle.ConstantTimeCompare, with the length-mismatch dummy), so-u 'metrics:'gets 401. Noted as hardening, not a defect.Disclosure — judgement call waived:
Config.MetricsAuthEnabled()returns false for a half-setConfigbuilt in code rather than panicking, i.e. auth-off without noise. That is not a fail-loudly violation because every productionConfigcomes fromconfig.New/loadFromEnv, which now aborts, and it is documented at the method. Verified there is no config-reload path and no second productionConfigprovider, sohasMetricsAuthand the mount read the same fields on the same never-mutated pointer.Also checked: base
next, fast-forward on10c8dd2; CI green (4m49s, real); title and commit both end(closes #205);TODO.mduntouched; no attribution trailers or vendor references;make fmt-checkclean in-container; the two half-set config rows assertErrIncompleteMetricsAuthand the absence of the password from the message, andTestMetricsRouteUnmountedOnHalfSetConfigis the assertion that actually regresses against pre-fix behaviour.