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

feat: validate configuration on startup, fail fast on bad config (closes #52) #53

Merged
sneak merged 11 commits from feature/config-validation into main 2026-08-07 22:39:40 +02:00
Conversation 6 Commits 11 Files Changed 4 +981 -40
clawbot commented 2026-08-07 18:37:50 +02:00
Collaborator
Copy Link

closes #52

Implements startup configuration validation per the plan on #52. Two commits, TDD: the first commit adds the enforcement tests (red — six test functions fail against the lenient behavior) plus a mechanical extraction of newFromSmartConfig from config.New so construction is testable without fx; the second commit makes them green and carries the TODO.md bookkeeping.

Behavior

  • No silent fallbacks: a config value that is SET but unparseable or invalid aborts startup with an error naming the key and value. Defaults apply only to OMITTED keys. The old getString/getInt/getBool helpers swallowed every conversion error and returned the default; they are now strict. Fractional ports are rejected, not truncated (smartconfig's GetInt would have turned 8080.5 into 8080).
  • Unknown keys abort: unknown top-level keys and unknown metrics subkeys are fatal, each named in the error (unknown config keys: whitelist_hosts). The env section stays permitted because smartconfig consumes it for environment injection.
  • Malformed config file aborts: a config file that exists at a standard location but fails to parse was previously logged as a warning and skipped (the server would start on defaults); it is now fatal.
  • Range/sanity checks: port in 1-65535; upstream_connections_per_host at least 1; signing_key required, at least 32 characters (keyless mode was never implemented; the stale "leave empty" comment in config.example.yml is corrected); allowlist_hosts entries must be bare hostnames (leading-dot suffix patterns still allowed; schemes, paths, whitespace, non-string and empty entries rejected); state_dir non-empty and verified creatable+writable with a probe file before the listener binds; sentry_dsn must be a URL with scheme and host when set; metrics.username/metrics.password must be set together.

Verification

  • make check green on the branch head (all tests, golangci-lint 0 issues, fmt-check clean).
  • End-to-end: ./bin/pixad with port: banana exits 1 printing config key "port": value "banana" is not an integer; with whitelist_hosts: it exits 1 printing unknown config keys: whitelist_hosts.

Notes for review

  • getStringSlice keeps its lenient signature because the existing tests in config_test.go exercise it and modifying existing tests requires explicit approval. Strictness for allowlist_hosts is instead enforced up front on the raw value by validateAllowlistHostsValue, so nothing is silently skipped; extraction then reuses the existing parser. If you prefer the helper folded into a single strict function, that requires retargeting those three tests — happy to do that as a follow-up with approval.
  • TODO.md here is edited against current main; PR #50 (merge-ready) edits adjacent lines, so whichever merges second will need a trivial rebase of TODO.md only.
  • The README Configuration section lists keys that have never existed in the code (access_control_allow_origin, upstream_fetch_timeout, upstream_max_response_size, downstream_timeout). Under this change a config using them now fails fast instead of silently doing nothing — that is the intended behavior. Implementing them is already tracked as the P2 "add all configuration options from README" item in TODO.md.
closes #52 Implements startup configuration validation per the plan on #52. Two commits, TDD: the first commit adds the enforcement tests (red — six test functions fail against the lenient behavior) plus a mechanical extraction of `newFromSmartConfig` from `config.New` so construction is testable without fx; the second commit makes them green and carries the `TODO.md` bookkeeping. ## Behavior - **No silent fallbacks**: a config value that is SET but unparseable or invalid aborts startup with an error naming the key and value. Defaults apply only to OMITTED keys. The old `getString`/`getInt`/`getBool` helpers swallowed every conversion error and returned the default; they are now strict. Fractional ports are rejected, not truncated (smartconfig's `GetInt` would have turned `8080.5` into `8080`). - **Unknown keys abort**: unknown top-level keys and unknown `metrics` subkeys are fatal, each named in the error (`unknown config keys: whitelist_hosts`). The `env` section stays permitted because smartconfig consumes it for environment injection. - **Malformed config file aborts**: a config file that exists at a standard location but fails to parse was previously logged as a warning and skipped (the server would start on defaults); it is now fatal. - **Range/sanity checks**: `port` in 1-65535; `upstream_connections_per_host` at least 1; `signing_key` required, at least 32 characters (keyless mode was never implemented; the stale "leave empty" comment in `config.example.yml` is corrected); `allowlist_hosts` entries must be bare hostnames (leading-dot suffix patterns still allowed; schemes, paths, whitespace, non-string and empty entries rejected); `state_dir` non-empty and verified creatable+writable with a probe file before the listener binds; `sentry_dsn` must be a URL with scheme and host when set; `metrics.username`/`metrics.password` must be set together. ## Verification - `make check` green on the branch head (all tests, golangci-lint 0 issues, fmt-check clean). - End-to-end: `./bin/pixad` with `port: banana` exits 1 printing `config key "port": value "banana" is not an integer`; with `whitelist_hosts:` it exits 1 printing `unknown config keys: whitelist_hosts`. ## Notes for review - `getStringSlice` keeps its lenient signature because the existing tests in `config_test.go` exercise it and modifying existing tests requires explicit approval. Strictness for `allowlist_hosts` is instead enforced up front on the raw value by `validateAllowlistHostsValue`, so nothing is silently skipped; extraction then reuses the existing parser. If you prefer the helper folded into a single strict function, that requires retargeting those three tests — happy to do that as a follow-up with approval. - `TODO.md` here is edited against current `main`; PR #50 (merge-ready) edits adjacent lines, so whichever merges second will need a trivial rebase of `TODO.md` only. - The README Configuration section lists keys that have never existed in the code (`access_control_allow_origin`, `upstream_fetch_timeout`, `upstream_max_response_size`, `downstream_timeout`). Under this change a config using them now fails fast instead of silently doing nothing — that is the intended behavior. Implementing them is already tracked as the P2 "add all configuration options from README" item in `TODO.md`.
clawbot added 2 commits 2026-08-07 18:37:50 +02:00
test: add failing startup config validation tests (#52) f19da2c02c 
Encode the required fail-fast behavior as tests ahead of the
implementation: a config value that is SET but unparseable or invalid
must abort startup (defaults apply only to OMITTED keys), unknown
top-level keys and unknown metrics subkeys must abort naming the key,
a malformed config file at a standard location must abort instead of
being skipped with a warning, and state_dir must be creatable and
writable at startup. Mechanically extracts newFromSmartConfig from
config.New so the construction path is testable without fx; current
lenient behavior is unchanged, so the new enforcement tests fail.
feat: validate configuration on startup, fail fast on bad config (closes #52)
Some checks failed
check / check (push) Failing after 42s
Details
2fb0801dc6 
A config value that is set but unparseable or invalid now aborts
startup with an error naming the offending key and value; defaults
apply only to omitted keys. Unknown top-level config keys and unknown
metrics subkeys abort startup naming each unknown key, so typos like
whitelist_hosts fail immediately instead of being silently ignored. A
config file that exists at a standard location but fails to parse is
now a fatal error instead of being skipped with a warning. state_dir
is verified creatable and writable with a probe file before the
listener binds. Port must be in 1-65535 (fractional values are
rejected, not truncated), upstream_connections_per_host must be at
least 1, allowlist_hosts entries must be bare hostnames, sentry_dsn
must be a valid URL when set, and metrics credentials must be set
together. The stale signing_key comment in config.example.yml (keyless
mode was never implemented) now states the actual requirement.
TODO.md records the completed step per its Workflow section.
clawbot added the needs-review label 2026-08-07 18:37:54 +02:00
clawbot self-assigned this 2026-08-07 18:37:55 +02:00
clawbot commented 2026-08-07 18:49:44 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework

Adversarial review of head 2fb0801 against the definition of done in #52 (issue body plus the plan comment) and the repo's no-silent-fallback rule.

Blocking

  1. CI is red on the head commit, caused by this PR's code. check / check (push) fails on 2fb0801 ("Failing after 42s", actions run 61): the Dockerfile lint stage (golangci-lint v2.10.1-alpine, hash-pinned) reports internal/config/config.go:209:21: G703: Path traversal via taint analysis (gosec) on os.Remove(probePath) in the new ensureStateDirWritable, and make lint exits nonzero, failing the docker build. The PR body's "make check green" claim reproduces locally only because a newer golangci-lint (v2.12.2 via nix-shell) does not emit this finding; the pinned CI image is the authoritative gate and per repo rules the linter output is to be treated as legitimate. Acceptable: satisfy the taint check in code (the file already has precedent at internal/config/config.go:307, where os.Stat on config paths carries a justified //nolint:gosec // G703 comment; probePath comes from os.CreateTemp inside the just-validated directory, so an equivalent justified suppression or a filepath.Clean/containment check would be defensible). Do not touch .golangci.yml.

  2. Explicitly-null config values silently fall back to defaults, violating the PR's own core rule. The strict getters treat present-but-null identically to absent: internal/config/config.go:380 (getString), :402 (getInt), :439 (getBool) all do if !ok || raw == nil { return defaultVal }, and validateAllowlistHostsValue (:468) / getStringSlice (:514) do the same. Verified end-to-end with the built binary: a config containing port: null (equivalently a bare port: line — a classic truncated/typo'd entry) starts the server on port 8080; debug: ~, state_dir: null, allowlist_hosts: null, db_url: null likewise boot on defaults. These keys are SET; null is not a valid port/bool/path, and issue #52 DoD items 1 and 3 say a set-but-invalid value must abort naming the key. The behavior is also internally inconsistent, which shows it is an accident rather than a decided semantic: metrics: null aborts (config.go:148-153, "value <nil> is not a map of metrics settings") and state_dir: "" aborts, while state_dir: null silently defaults. Fix is feasible: smartconfig's Get returns (nil, true) for a top-level key present with a null value, so absent vs null is distinguishable. Acceptable: error on present-with-null naming the key, with tests; or, if the owner explicitly decides null-means-omitted, document it and apply it consistently (including metrics) with tests — that decision needs owner sign-off, not a reviewer's.

Non-blocking

  1. db_url: "" explicitly set silently derives file:&lt;state_dir&gt;/state.sqlite3?... (internal/config/config.go:117-120). Same family as finding 2 (an explicitly empty set value taking a computed default), and inconsistent with state_dir: "" which aborts. Pre-existing derivation design, so not held blocking on its own, but it should either error or be explicitly documented as the "derive" sentinel.

  2. validateAllowlistHost (internal/config/config.go:271-279) accepts the entry ".". allowlist.New files it as a suffix pattern and IsAllowed matches via strings.HasSuffix(host, "."), so any upstream URL written in FQDN trailing-dot form (http://evil.com./x, Hostname() = evil.com.) is allowed without a signature — a single-character config entry that effectively disables signing. Requires the operator to configure "." themselves, and the matcher is pre-existing, so non-blocking — but the new validator is the right place to reject "." (and arguably any entry lacking a label).

  3. Error-message consistency: the signing_key errors (config.go:221, :227) use bare phrasing ("signing_key is required") instead of the config key %q convention used everywhere else. They do name the key, and correctly avoid echoing the secret value; cosmetic only.

  4. The metrics: null error prints value &lt;nil&gt; is not a map of metrics settings — the rendered &lt;nil&gt; is unhelpful to an operator; cosmetic.

Notes

  • Pre-flagged getStringSlice retention: claim verified. Its only non-test caller is newFromSmartConfig (config.go:110), and whenever a config file exists, validateAllowlistHostsValue runs first (config.go:94-96). Empirically, allowlist_hosts: "a,,b", non-string list entries, and non-list values all abort naming allowlist_hosts and the value; nothing is silently skipped on the strict path. Retention is an owner-decision note, not a defect. (The null case is covered under blocking finding 2, not this helper.)
  • TDD verified. At f19da2c (tests-first commit) all six new test functions fail (25 failing subtests observed); at head all pass. The only inter-commit change to the new test file is gofmt column alignment (whitespace-only). Existing tests (internal/config/config_test.go) are byte-identical to main.
  • Observed make check result: green locally at head — tests pass, golangci-lint v2.12.2 reports 0 issues, gofmt clean (run via the Makefile's own nix-shell dependency mechanism; this environment lacks vips and main fails identically without it, so that part is environmental). The divergence from CI's pinned v2.10.1 is exactly blocking finding 1.
  • Hostile-value sweep otherwise clean. All of the following abort with exit 1 before binding, naming key and value: port: banana/0/99999/8080.5/[8080], duplicate port: keys (yaml.v3 rejects duplicates, now fatal), debug: 1, allow_http: 2/yes (YAML 1.2 string, ParseBool rejects — strict, correct), numeric signing_key, short/missing signing_key, metrics username-only/password-only/non-map, sentry_dsn without scheme/host, unknown top-level key and unknown metrics subkey (each named), malformed YAML at a standard location, uncreatable state_dir (/dev/null/subdir fails on the probe pre-listen).
  • Commit hygiene good: landing commit title ends with (closes #52); no attribution trailers; branch mergeable against current main (base 6573b9d is the main tip). TODO.md bookkeeping matches the Workflow section, and the config.example.yml stale-comment fix is in scope. Godoc present on the new identifiers (all unexported); naming is descriptive and consistent; no linter-config, vendor, or binary changes.

Verdict: FAIL. Label needs-rework (the CI redness is caused by in-PR code, so a rerun will not go green without changes). Findings 1 and 2 must be fixed; 3-6 at author/owner discretion.

FAIL — needs-rework Adversarial review of head `2fb0801` against the definition of done in #52 (issue body plus the plan comment) and the repo's no-silent-fallback rule. ## Blocking 1. **CI is red on the head commit, caused by this PR's code.** `check / check (push)` fails on `2fb0801` ("Failing after 42s", actions run 61): the Dockerfile lint stage (golangci-lint v2.10.1-alpine, hash-pinned) reports `internal/config/config.go:209:21: G703: Path traversal via taint analysis (gosec)` on `os.Remove(probePath)` in the new `ensureStateDirWritable`, and `make lint` exits nonzero, failing the docker build. The PR body's "make check green" claim reproduces locally only because a newer golangci-lint (v2.12.2 via nix-shell) does not emit this finding; the pinned CI image is the authoritative gate and per repo rules the linter output is to be treated as legitimate. Acceptable: satisfy the taint check in code (the file already has precedent at `internal/config/config.go:307`, where `os.Stat` on config paths carries a justified `//nolint:gosec // G703` comment; `probePath` comes from `os.CreateTemp` inside the just-validated directory, so an equivalent justified suppression or a `filepath.Clean`/containment check would be defensible). Do not touch `.golangci.yml`. 2. **Explicitly-null config values silently fall back to defaults, violating the PR's own core rule.** The strict getters treat present-but-null identically to absent: `internal/config/config.go:380` (`getString`), `:402` (`getInt`), `:439` (`getBool`) all do `if !ok || raw == nil { return defaultVal }`, and `validateAllowlistHostsValue` (`:468`) / `getStringSlice` (`:514`) do the same. Verified end-to-end with the built binary: a config containing `port: null` (equivalently a bare `port:` line — a classic truncated/typo'd entry) starts the server on port 8080; `debug: ~`, `state_dir: null`, `allowlist_hosts: null`, `db_url: null` likewise boot on defaults. These keys are SET; null is not a valid port/bool/path, and issue #52 DoD items 1 and 3 say a set-but-invalid value must abort naming the key. The behavior is also internally inconsistent, which shows it is an accident rather than a decided semantic: `metrics: null` aborts (`config.go:148-153`, "value &lt;nil&gt; is not a map of metrics settings") and `state_dir: ""` aborts, while `state_dir: null` silently defaults. Fix is feasible: smartconfig's `Get` returns `(nil, true)` for a top-level key present with a null value, so absent vs null is distinguishable. Acceptable: error on present-with-null naming the key, with tests; or, if the owner explicitly decides null-means-omitted, document it and apply it consistently (including `metrics`) with tests — that decision needs owner sign-off, not a reviewer's. ## Non-blocking 3. `db_url: ""` explicitly set silently derives `file:&lt;state_dir&gt;/state.sqlite3?...` (`internal/config/config.go:117-120`). Same family as finding 2 (an explicitly empty set value taking a computed default), and inconsistent with `state_dir: ""` which aborts. Pre-existing derivation design, so not held blocking on its own, but it should either error or be explicitly documented as the "derive" sentinel. 4. `validateAllowlistHost` (`internal/config/config.go:271-279`) accepts the entry `"."`. `allowlist.New` files it as a suffix pattern and `IsAllowed` matches via `strings.HasSuffix(host, ".")`, so any upstream URL written in FQDN trailing-dot form (`http://evil.com./x`, `Hostname()` = `evil.com.`) is allowed without a signature — a single-character config entry that effectively disables signing. Requires the operator to configure `"."` themselves, and the matcher is pre-existing, so non-blocking — but the new validator is the right place to reject `"."` (and arguably any entry lacking a label). 5. Error-message consistency: the `signing_key` errors (`config.go:221`, `:227`) use bare phrasing ("signing_key is required") instead of the `config key %q` convention used everywhere else. They do name the key, and correctly avoid echoing the secret value; cosmetic only. 6. The `metrics: null` error prints `value &lt;nil&gt; is not a map of metrics settings` — the rendered `&lt;nil&gt;` is unhelpful to an operator; cosmetic. ## Notes - **Pre-flagged `getStringSlice` retention: claim verified.** Its only non-test caller is `newFromSmartConfig` (`config.go:110`), and whenever a config file exists, `validateAllowlistHostsValue` runs first (`config.go:94-96`). Empirically, `allowlist_hosts: "a,,b"`, non-string list entries, and non-list values all abort naming `allowlist_hosts` and the value; nothing is silently skipped on the strict path. Retention is an owner-decision note, not a defect. (The null case is covered under blocking finding 2, not this helper.) - **TDD verified.** At `f19da2c` (tests-first commit) all six new test functions fail (25 failing subtests observed); at head all pass. The only inter-commit change to the new test file is gofmt column alignment (whitespace-only). Existing tests (`internal/config/config_test.go`) are byte-identical to `main`. - **Observed `make check` result:** green locally at head — tests pass, golangci-lint v2.12.2 reports 0 issues, gofmt clean (run via the Makefile's own nix-shell dependency mechanism; this environment lacks vips and `main` fails identically without it, so that part is environmental). The divergence from CI's pinned v2.10.1 is exactly blocking finding 1. - **Hostile-value sweep otherwise clean.** All of the following abort with exit 1 before binding, naming key and value: `port: banana`/`0`/`99999`/`8080.5`/`[8080]`, duplicate `port:` keys (yaml.v3 rejects duplicates, now fatal), `debug: 1`, `allow_http: 2`/`yes` (YAML 1.2 string, ParseBool rejects — strict, correct), numeric `signing_key`, short/missing `signing_key`, `metrics` username-only/password-only/non-map, `sentry_dsn` without scheme/host, unknown top-level key and unknown `metrics` subkey (each named), malformed YAML at a standard location, uncreatable `state_dir` (`/dev/null/subdir` fails on the probe pre-listen). - Commit hygiene good: landing commit title ends with ` (closes #52)`; no attribution trailers; branch mergeable against current `main` (base `6573b9d` is the `main` tip). `TODO.md` bookkeeping matches the Workflow section, and the `config.example.yml` stale-comment fix is in scope. Godoc present on the new identifiers (all unexported); naming is descriptive and consistent; no linter-config, vendor, or binary changes. Verdict: FAIL. Label `needs-rework` (the CI redness is caused by in-PR code, so a rerun will not go green without changes). Findings 1 and 2 must be fixed; 3-6 at author/owner discretion.
clawbot added needs-rework and removed needs-review labels 2026-08-07 18:50:12 +02:00
clawbot commented 2026-08-07 18:50:20 +02:00
Author
Collaborator
Copy Link

Manager note: independent adversarial review returned FAIL (see review comment above). Relabeling needs-rework and dispatching rework limited to the two blocking findings:

  1. Fix the CI-authoritative gosec G703 finding at internal/config/config.go:209 (os.Remove(probePath) in ensureStateDirWritable) in code — a justified //nolint:gosec matching the existing precedent at config.go:307, or a containment check. The linter config must not be touched.
  2. Make explicitly-null config values (port: null, bare port:, debug: ~, etc.) abort startup naming the key, instead of silently defaulting. An explicit null is a SET value under the repo's no-silent-fallback policy, and current behavior is internally inconsistent (metrics: null aborts while port: null defaults), so this is a defect fix, not a semantics decision.

Non-blocking review notes (db_url: "" silent derivation, allowlist "." trailing-dot-FQDN bypass, error-message style, &lt;nil&gt; in the metrics-null message) may be fixed in the same pass if trivial and in scope; otherwise they remain recorded here.

The retained lenient getStringSlice helper was verified by the reviewer to be fully guarded by validateAllowlistHostsValue and is an owner decision, not a defect; it does not block this PR. Question for sneak, with recommendation: keep the helper as-is (recommended — the strictness guarantee holds and folding it into a single strict function would require retargeting three existing tests, which needs explicit approval), or approve a follow-up that retargets those tests and removes the lenient signature. No action needed to merge this PR either way.

After rework, a fresh independent reviewer will re-review before any merge-ready labeling.

Manager note: independent adversarial review returned FAIL (see review comment above). Relabeling `needs-rework` and dispatching rework limited to the two blocking findings: 1. Fix the CI-authoritative gosec G703 finding at `internal/config/config.go:209` (`os.Remove(probePath)` in `ensureStateDirWritable`) in code — a justified `//nolint:gosec` matching the existing precedent at config.go:307, or a containment check. The linter config must not be touched. 2. Make explicitly-null config values (`port: null`, bare `port:`, `debug: ~`, etc.) abort startup naming the key, instead of silently defaulting. An explicit null is a SET value under the repo's no-silent-fallback policy, and current behavior is internally inconsistent (`metrics: null` aborts while `port: null` defaults), so this is a defect fix, not a semantics decision. Non-blocking review notes (`db_url: ""` silent derivation, allowlist `"."` trailing-dot-FQDN bypass, error-message style, `&lt;nil&gt;` in the metrics-null message) may be fixed in the same pass if trivial and in scope; otherwise they remain recorded here. The retained lenient `getStringSlice` helper was verified by the reviewer to be fully guarded by `validateAllowlistHostsValue` and is an owner decision, not a defect; it does not block this PR. Question for sneak, with recommendation: keep the helper as-is (recommended — the strictness guarantee holds and folding it into a single strict function would require retargeting three existing tests, which needs explicit approval), or approve a follow-up that retargets those tests and removes the lenient signature. No action needed to merge this PR either way. After rework, a fresh independent reviewer will re-review before any `merge-ready` labeling.
clawbot added 9 commits 2026-08-07 19:05:55 +02:00
Merge origin/main into feature/config-validation (TODO.md bookkeeping) 745a461688
fix: suppress gosec G703 false positive on state_dir probe removal 11e9206c21 
The pinned CI linter (golangci-lint v2.10.1) flags os.Remove(probePath)
in ensureStateDirWritable as G703 path traversal via taint analysis.
probePath comes from os.CreateTemp inside the StateDir that the probe
just validated, so the taint finding is a false positive; suppress it
with a justified nolint comment matching the existing precedent in
loadConfigFile. Verified against the pinned linter version via the
Dockerfile lint stage (0 issues).
test: explicit null config values must abort startup (PR #53 rework) 370545997f 
An explicitly-null key (port: null, bare port:, debug: ~, and every
other config key including metrics subkeys) is a SET value under the
no-silent-fallback rule and must abort startup naming the key, instead
of silently taking the default as it does today. All 13 subtests fail
against the current behavior; the fix follows.
fix: abort startup when a config key is explicitly set to null c0d325156f 
An explicitly-null key (port: null, bare port:, debug: ~, metrics
subkeys, and every other known key) previously fell through the ok/nil
check in the strict getters and silently took the default, violating
the no-silent-fallback rule and contradicting metrics: null which
already aborted. validateKnownKeys now collects null-valued keys (top
level and metrics subkeys) and aborts naming each one, and the strict
getters and validateAllowlistHostsValue error on null instead of
defaulting as defense in depth. This also replaces the unhelpful
'value <nil> is not a map of metrics settings' rendering for
metrics: null with the null-specific message.
test: explicitly empty db_url must abort startup (PR #53 rework) 813ff63153 
db_url: "" currently silently derives the state_dir-based sqlite URL,
which is a default applied to a SET value; state_dir: "" already
aborts. Failing test first, fix follows.
fix: abort startup on explicitly empty db_url instead of deriving 5297e6033a 
The derived file:...state.sqlite3 URL is a default and defaults apply
only to omitted keys: db_url set to an empty string now aborts naming
the key, matching the existing behavior of state_dir: "".
test: allowlist_hosts must reject dot-only entries (PR #53 rework) 808356f142 
A bare "." entry becomes a HasSuffix suffix pattern that matches any
upstream written in FQDN trailing-dot form (evil.com.), effectively
disabling URL signing with one character. Failing test first, fix
follows.
fix: reject allowlist_hosts entries containing no hostname labels 83fa22871e 
Entries consisting only of dots (".", "..") are now a startup error
naming the key and entry. Previously a bare "." passed validation and
became a suffix pattern matching every trailing-dot FQDN upstream,
bypassing URL signing.
style: use the config key error-message convention for signing_key
All checks were successful
check / check (push) Successful in 1m48s
Details
22f19c849b 
The signing_key errors used bare phrasing while every other validation
error follows the 'config key %q' convention; align them. The secret
value itself is still never echoed.
clawbot commented 2026-08-07 19:06:17 +02:00
Author
Collaborator
Copy Link

Rework complete against the FAIL review (issuecomment-43803) and the manager disposition (issuecomment-43807). New head 22f19c8.

Blocking finding 1: CI red on gosec G703 (pinned golangci-lint v2.10.1)

Fixed in code only, in 11e9206: os.Remove(probePath) in ensureStateDirWritable now carries //nolint:gosec // G703: probePath comes from os.CreateTemp inside the just-validated StateDir, matching the existing justified-suppression precedent in loadConfigFile. .golangci.yml is untouched. Verified against the authoritative pinned linter by building the Dockerfile lint stage (docker build --target lint ., golangci-lint v2.10.1-alpine): RUN make lint reports 0 issues on the final head. The local newer linter also reports 0 issues (the directive is not flagged as unused).

Blocking finding 2: explicitly-null values silently defaulted

Fixed TDD in 3705459 (13 failing subtests committed first: port: null, bare port:, debug: ~, maintenance_mode, allow_http, state_dir, db_url, sentry_dsn, upstream_connections_per_host, allowlist_hosts, signing_key, metrics: null, and null metrics subkeys) and c0d3251 (fix). An explicit null is a SET value and now aborts startup naming the key: validateKnownKeys collects null-valued keys (top level plus metrics subkeys) and errors on each, sorted and named (config key "port": value is null; omit the key entirely to use the default); the strict getters (getString/getInt/getBool) and validateAllowlistHostsValue also error on null instead of defaulting, as defense in depth. Behavior is now consistent across every key. End-to-end with the built binary: port: null exits 1 printing config key "port": value is null; omit the key entirely to use the default; a valid config still boots.

Non-blocking notes

  • Note 3 (db_url: "" silent derivation): fixed, TDD, in 813ff63 (failing test) and 5297e60 (fix). The derived sqlite URL is now applied only when db_url is omitted; an explicitly empty value aborts (config key "db_url": value must not be empty; omit the key to derive it from state_dir), matching state_dir: "". Verified end-to-end.
  • Note 4 (allowlist "." trailing-dot-FQDN bypass): fixed, TDD, in 808356f (failing tests for . and ..) and 83fa228 (fix). validateAllowlistHost rejects entries containing no hostname labels: config key "allowlist_hosts": entry "." contains no hostname labels. Verified end-to-end.
  • Note 5 (signing_key message style): fixed in 22f19c8 — both errors now use the config key %q convention; the secret value is still never echoed.
  • Note 6 (&lt;nil&gt; rendering for metrics: null): fixed as a side effect of finding 2 — metrics: null now takes the null-specific message instead of value &lt;nil&gt; is not a map of metrics settings.

Also in this push

745a461 merges current main into the branch: PR #50 merged after this PR opened and both edit TODO.md, so the PR had become unmergeable. Conflict resolution is TODO.md-only, exactly the trivial rebase the PR body predicted: both Completed entries kept, and the conflicted Future Steps lines dropped (cache-size moved to Next Step by main; config validation completed by this PR).

Verification

make check green on head 22f19c8 (all tests pass including the 13 new null subtests and the 2 new TDD pairs; golangci-lint 0 issues; fmt-check clean), and the pinned-CI-version Dockerfile lint stage passes with 0 issues. No existing tests were modified; all additions are new test functions. Labels/assignees left unchanged for the manager to dispatch re-review.

Rework complete against the FAIL review (issuecomment-43803) and the manager disposition (issuecomment-43807). New head `22f19c8`. ## Blocking finding 1: CI red on gosec G703 (pinned golangci-lint v2.10.1) Fixed in code only, in `11e9206`: `os.Remove(probePath)` in `ensureStateDirWritable` now carries `//nolint:gosec // G703: probePath comes from os.CreateTemp inside the just-validated StateDir`, matching the existing justified-suppression precedent in `loadConfigFile`. `.golangci.yml` is untouched. Verified against the authoritative pinned linter by building the Dockerfile lint stage (`docker build --target lint .`, golangci-lint v2.10.1-alpine): `RUN make lint` reports `0 issues` on the final head. The local newer linter also reports 0 issues (the directive is not flagged as unused). ## Blocking finding 2: explicitly-null values silently defaulted Fixed TDD in `3705459` (13 failing subtests committed first: `port: null`, bare `port:`, `debug: ~`, `maintenance_mode`, `allow_http`, `state_dir`, `db_url`, `sentry_dsn`, `upstream_connections_per_host`, `allowlist_hosts`, `signing_key`, `metrics: null`, and null `metrics` subkeys) and `c0d3251` (fix). An explicit null is a SET value and now aborts startup naming the key: `validateKnownKeys` collects null-valued keys (top level plus `metrics` subkeys) and errors on each, sorted and named (`config key "port": value is null; omit the key entirely to use the default`); the strict getters (`getString`/`getInt`/`getBool`) and `validateAllowlistHostsValue` also error on null instead of defaulting, as defense in depth. Behavior is now consistent across every key. End-to-end with the built binary: `port: null` exits 1 printing `config key "port": value is null; omit the key entirely to use the default`; a valid config still boots. ## Non-blocking notes - Note 3 (`db_url: ""` silent derivation): fixed, TDD, in `813ff63` (failing test) and `5297e60` (fix). The derived sqlite URL is now applied only when `db_url` is omitted; an explicitly empty value aborts (`config key "db_url": value must not be empty; omit the key to derive it from state_dir`), matching `state_dir: ""`. Verified end-to-end. - Note 4 (allowlist `"."` trailing-dot-FQDN bypass): fixed, TDD, in `808356f` (failing tests for `.` and `..`) and `83fa228` (fix). `validateAllowlistHost` rejects entries containing no hostname labels: `config key "allowlist_hosts": entry "." contains no hostname labels`. Verified end-to-end. - Note 5 (signing_key message style): fixed in `22f19c8` — both errors now use the `config key %q` convention; the secret value is still never echoed. - Note 6 (`&lt;nil&gt;` rendering for `metrics: null`): fixed as a side effect of finding 2 — `metrics: null` now takes the null-specific message instead of `value &lt;nil&gt; is not a map of metrics settings`. ## Also in this push `745a461` merges current `main` into the branch: PR #50 merged after this PR opened and both edit `TODO.md`, so the PR had become unmergeable. Conflict resolution is `TODO.md`-only, exactly the trivial rebase the PR body predicted: both Completed entries kept, and the conflicted Future Steps lines dropped (cache-size moved to Next Step by `main`; config validation completed by this PR). ## Verification `make check` green on head `22f19c8` (all tests pass including the 13 new null subtests and the 2 new TDD pairs; golangci-lint 0 issues; fmt-check clean), and the pinned-CI-version Dockerfile lint stage passes with 0 issues. No existing tests were modified; all additions are new test functions. Labels/assignees left unchanged for the manager to dispatch re-review.
clawbot added needs-review and removed needs-rework labels 2026-08-07 19:06:55 +02:00
clawbot commented 2026-08-07 19:13:26 +02:00
Author
Collaborator
Copy Link

PASS

Fresh independent adversarial re-review of head 22f19c8 against the definition of done in #52 (issue body plus plan comment), the first review (issuecomment-43803), the manager disposition (issuecomment-43807), and the rework summary (issuecomment-43887). All claims re-verified from scratch in a temporary worktree; nothing taken on prior reviewers' or the author's word.

Prior blocking findings — confirmation status

  1. G703 / CI red: CONFIRMED FIXED. internal/config/config.go:253 now carries //nolint:gosec // G703: probePath comes from os.CreateTemp inside the just-validated StateDir, matching the existing justified-suppression precedent at config.go:363. Verified against the authoritative pinned linter with an UNCACHED docker build --target lint . (golangci-lint v2.10.1-alpine, hash-pinned): make fmt-check passes and make lint reports 0 issues, exit 0. .golangci.yml, Makefile, Dockerfile, and script/ are byte-identical to main (diff empty). CI is green on the head commit: check / check (push) success (actions run 63, 1m48s).

  2. Explicit-null silent fallback: CONFIRMED FIXED. Verified end-to-end with a freshly built binary: port: null, bare port:, debug: ~, state_dir: null, allowlist_hosts: null, db_url: null, sentry_dsn: null, metrics: null, and null metrics subkeys (both, and mixed username: bob + password: null) each exit 1 before binding, naming the key: config key "port": value is null; omit the key entirely to use the default; multiple nulls are collected, sorted, and all named (config keys metrics.password, metrics.username: ...). Defense in depth is present in the strict getters (config.go:441, :467, :509) and validateAllowlistHostsValue (config.go:542) in addition to validateKnownKeys (config.go:159-183). A valid config still boots and serves HTTP 200. No &lt;nil&gt; artifacts appear in any observed error message.

Blocking

None.

Non-blocking

None new. The four non-blocking notes from the first review are all verified fixed at this head:

  • db_url: "" exits 1 with config key "db_url": value must not be empty; omit the key to derive it from state_dir; omitted db_url still derives file:&lt;state_dir&gt;/state.sqlite3?_journal_mode=WAL (unit-tested in TestOmittedValuesUseDefaults, passing).
  • Allowlist entries . and .. exit 1 with config key "allowlist_hosts": entry "." contains no hostname labels; a legitimate leading-dot suffix entry (.example.org) still loads and the server boots.
  • Both signing_key errors now use the config key %q convention (config.go:267, :273); the secret string value is still never echoed.
  • metrics: null takes the null-specific message instead of the old value &lt;nil&gt; rendering.

Notes

  • Observed make check: green at head. All packages pass (including the new validation tests), golangci-lint 0 issues, fmt-check clean, exit 0. (One environmental artifact worth recording for other reviewers: a stale shared golangci-lint analysis cache from a deleted sibling worktree of this module replays phantom issues against ../pixa-rework/... paths; a fresh GOLANGCI_LINT_CACHE reproduces the true result. None of the phantom findings involve this PR's files.)
  • Observed docker lint (pinned CI gate): green. Uncached docker build --target lint . — 0 issues, exit 0, as stated above.
  • No existing tests modified. internal/config/config_test.go is byte-identical to main; the PR's only test changes are the new internal/config/config_validation_test.go. Verified across the whole PR range.
  • TDD ordering verified for all rework pairs. At 3705459 the 13 null subtests fail (observed red) and at c0d3251 the full suite passes (observed green); 813ff63 fails on TestExplicitlyEmptyDBURLAbortsStartup (red) and 808356f fails on both dot-entry subtests (red), with head green. The fix commits change zero test lines in each pair.
  • Merge commit 745a461 clean. Relative to its branch parent it touches only TODO.md; relative to main it is exactly the PR's intended bookkeeping (Completed entry added, the Future Steps P0 line removed). No code smuggled in. PR is mergeable; base 5d0b5f8 is the current main tip.
  • Hostile-config sweep at head: all abort exit 1 pre-listen naming key and value. port: banana/0/99999/8080.5, duplicate port: keys (yaml duplicate-key parse error, fatal), debug: 1, upstream_connections_per_host: 0/-3, numeric signing_key, short/missing signing_key, metrics username-only, unknown top-level key (whitelist_hosts), unknown metrics subkey (metrics.port), unknown-key + null-key mix (unknown reported first — reasonable precedence), malformed YAML file, state_dir: "", uncreatable state_dir (/dev/null/sub), sentry_dsn: "not a url", allowlist entry with scheme, comma-list with empty entry ("a,,b"), and env: null (fatal via smartconfig: env section must be a map). metrics: {} boots (empty section, both credentials omitted — consistent with the set-together rule).
  • allowlist_hosts: "" (explicitly empty string) loads as an empty allowlist and boots. This is a defensible explicit value (empty list, same as [] — every upstream requires a signature), not a silent fallback to a different value; recorded for completeness only.
  • getStringSlice retention guard re-verified at this head: validateAllowlistHostsValue runs first whenever a config file exists (config.go:94-96) and now also rejects null; nothing reaches the lenient extraction unvalidated. Owner-decision item, not a defect, per the manager disposition.
  • Commit hygiene: landing commit/PR title ends with (closes #52); no attribution trailers or vendor/binary changes anywhere in the range; godoc present on new identifiers; naming descriptive and consistent; TODO.md matches the Workflow section.

Verdict: PASS. Eligible for merge-ready and assignment to sneak; labels/assignees left to the manager per dispatch.

PASS Fresh independent adversarial re-review of head `22f19c8` against the definition of done in #52 (issue body plus plan comment), the first review (issuecomment-43803), the manager disposition (issuecomment-43807), and the rework summary (issuecomment-43887). All claims re-verified from scratch in a temporary worktree; nothing taken on prior reviewers' or the author's word. ## Prior blocking findings — confirmation status 1. **G703 / CI red: CONFIRMED FIXED.** `internal/config/config.go:253` now carries `//nolint:gosec // G703: probePath comes from os.CreateTemp inside the just-validated StateDir`, matching the existing justified-suppression precedent at `config.go:363`. Verified against the authoritative pinned linter with an UNCACHED `docker build --target lint .` (golangci-lint v2.10.1-alpine, hash-pinned): `make fmt-check` passes and `make lint` reports `0 issues`, exit 0. `.golangci.yml`, `Makefile`, `Dockerfile`, and `script/` are byte-identical to `main` (diff empty). CI is green on the head commit: `check / check (push)` success (actions run 63, 1m48s). 2. **Explicit-null silent fallback: CONFIRMED FIXED.** Verified end-to-end with a freshly built binary: `port: null`, bare `port:`, `debug: ~`, `state_dir: null`, `allowlist_hosts: null`, `db_url: null`, `sentry_dsn: null`, `metrics: null`, and null `metrics` subkeys (both, and mixed `username: bob` + `password: null`) each exit 1 before binding, naming the key: `config key "port": value is null; omit the key entirely to use the default`; multiple nulls are collected, sorted, and all named (`config keys metrics.password, metrics.username: ...`). Defense in depth is present in the strict getters (`config.go:441`, `:467`, `:509`) and `validateAllowlistHostsValue` (`config.go:542`) in addition to `validateKnownKeys` (`config.go:159-183`). A valid config still boots and serves HTTP 200. No `&lt;nil&gt;` artifacts appear in any observed error message. ## Blocking None. ## Non-blocking None new. The four non-blocking notes from the first review are all verified fixed at this head: - `db_url: ""` exits 1 with `config key "db_url": value must not be empty; omit the key to derive it from state_dir`; omitted `db_url` still derives `file:&lt;state_dir&gt;/state.sqlite3?_journal_mode=WAL` (unit-tested in `TestOmittedValuesUseDefaults`, passing). - Allowlist entries `.` and `..` exit 1 with `config key "allowlist_hosts": entry "." contains no hostname labels`; a legitimate leading-dot suffix entry (`.example.org`) still loads and the server boots. - Both `signing_key` errors now use the `config key %q` convention (`config.go:267`, `:273`); the secret string value is still never echoed. - `metrics: null` takes the null-specific message instead of the old `value &lt;nil&gt;` rendering. ## Notes - **Observed `make check`: green at head.** All packages pass (including the new validation tests), golangci-lint 0 issues, fmt-check clean, exit 0. (One environmental artifact worth recording for other reviewers: a stale shared golangci-lint analysis cache from a deleted sibling worktree of this module replays phantom issues against `../pixa-rework/...` paths; a fresh `GOLANGCI_LINT_CACHE` reproduces the true result. None of the phantom findings involve this PR's files.) - **Observed docker lint (pinned CI gate): green.** Uncached `docker build --target lint .` — `0 issues`, exit 0, as stated above. - **No existing tests modified.** `internal/config/config_test.go` is byte-identical to `main`; the PR's only test changes are the new `internal/config/config_validation_test.go`. Verified across the whole PR range. - **TDD ordering verified for all rework pairs.** At `3705459` the 13 null subtests fail (observed red) and at `c0d3251` the full suite passes (observed green); `813ff63` fails on `TestExplicitlyEmptyDBURLAbortsStartup` (red) and `808356f` fails on both dot-entry subtests (red), with head green. The fix commits change zero test lines in each pair. - **Merge commit `745a461` clean.** Relative to its branch parent it touches only `TODO.md`; relative to `main` it is exactly the PR's intended bookkeeping (Completed entry added, the Future Steps P0 line removed). No code smuggled in. PR is mergeable; base `5d0b5f8` is the current `main` tip. - **Hostile-config sweep at head: all abort exit 1 pre-listen naming key and value.** `port: banana`/`0`/`99999`/`8080.5`, duplicate `port:` keys (yaml duplicate-key parse error, fatal), `debug: 1`, `upstream_connections_per_host: 0`/`-3`, numeric `signing_key`, short/missing `signing_key`, `metrics` username-only, unknown top-level key (`whitelist_hosts`), unknown `metrics` subkey (`metrics.port`), unknown-key + null-key mix (unknown reported first — reasonable precedence), malformed YAML file, `state_dir: ""`, uncreatable `state_dir` (`/dev/null/sub`), `sentry_dsn: "not a url"`, allowlist entry with scheme, comma-list with empty entry (`"a,,b"`), and `env: null` (fatal via smartconfig: `env section must be a map`). `metrics: {}` boots (empty section, both credentials omitted — consistent with the set-together rule). - `allowlist_hosts: ""` (explicitly empty string) loads as an empty allowlist and boots. This is a defensible explicit value (empty list, same as `[]` — every upstream requires a signature), not a silent fallback to a different value; recorded for completeness only. - `getStringSlice` retention guard re-verified at this head: `validateAllowlistHostsValue` runs first whenever a config file exists (`config.go:94-96`) and now also rejects null; nothing reaches the lenient extraction unvalidated. Owner-decision item, not a defect, per the manager disposition. - Commit hygiene: landing commit/PR title ends with ` (closes #52)`; no attribution trailers or vendor/binary changes anywhere in the range; godoc present on new identifiers; naming descriptive and consistent; `TODO.md` matches the Workflow section. Verdict: PASS. Eligible for `merge-ready` and assignment to sneak; labels/assignees left to the manager per dispatch.
clawbot added merge-ready and removed needs-review labels 2026-08-07 19:13:46 +02:00
clawbot removed their assignment 2026-08-07 19:13:49 +02:00
sneak was assigned by clawbot 2026-08-07 19:13:49 +02:00
clawbot commented 2026-08-07 19:13:54 +02:00
Author
Collaborator
Copy Link

Manager note: fresh independent re-review at head 22f19c8 returned PASS (see review comment above) — both prior blocking findings confirmed fixed, pinned CI lint gate at 0 issues, CI green (actions run 63), full hostile-config sweep clean, no existing tests modified, mergeable against current main (the PR #50 merge conflict was resolved in 745a461, TODO.md-only). Labeling merge-ready and assigning to sneak for merge (protected main).

One open owner question, non-blocking (does not affect merging this PR): keep the lenient getStringSlice helper as-is (recommended — its guard via validateAllowlistHostsValue was re-verified at this head), or approve a follow-up that folds it into a single strict function, which requires retargeting three existing tests in internal/config/config_test.go.

Manager note: fresh independent re-review at head `22f19c8` returned PASS (see review comment above) — both prior blocking findings confirmed fixed, pinned CI lint gate at 0 issues, CI green (actions run 63), full hostile-config sweep clean, no existing tests modified, mergeable against current `main` (the PR #50 merge conflict was resolved in `745a461`, `TODO.md`-only). Labeling `merge-ready` and assigning to sneak for merge (protected `main`). One open owner question, non-blocking (does not affect merging this PR): keep the lenient `getStringSlice` helper as-is (recommended — its guard via `validateAllowlistHostsValue` was re-verified at this head), or approve a follow-up that folds it into a single strict function, which requires retargeting three existing tests in `internal/config/config_test.go`.
sneak merged commit 61f42e6602 into main 2026-08-07 22:39:40 +02:00
sneak deleted branch feature/config-validation 2026-08-07 22:39:40 +02:00
clawbot commented 2026-08-07 22:42:42 +02:00
Author
Collaborator
Copy Link

Decision on the open getStringSlice question (owner delegated the call): keep the helper as-is. Rationale: the no-silent-fallback guarantee is enforced up front by validateAllowlistHostsValue before the helper ever runs, and that guard was independently verified by two separate adversarial reviews (issuecomment-43803 and issuecomment-43938), including at the merged head. Folding the helper into a single strict function would change zero observable behavior while churning three existing tests in internal/config/config_test.go. No follow-up issue; question closed.

Decision on the open `getStringSlice` question (owner delegated the call): **keep the helper as-is**. Rationale: the no-silent-fallback guarantee is enforced up front by `validateAllowlistHostsValue` before the helper ever runs, and that guard was independently verified by two separate adversarial reviews (issuecomment-43803 and issuecomment-43938), including at the merged head. Folding the helper into a single strict function would change zero observable behavior while churning three existing tests in `internal/config/config_test.go`. No follow-up issue; question closed.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/pixa#53
Delete Branch "feature/config-validation"

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?