Repository navigation
fix(relay): parse env settings strictly and refuse invalid values - #9236
Draft
ThePumpingLemma wants to merge 1 commit into
Draft
ThePumpingLemma wants to merge 1 commit into
ThePumpingLemma wants to merge 1 commit into
Conversation
Problem: config.rs already had strict helpers (parse_bool and positive_u64_from_env), but most settings parsed by hand, each with its own rules: - BUZZ_REQUIRE_AUTH_TOKEN, BUZZ_PUBKEY_ALLOWLIST, BUZZ_REQUIRE_RELAY_MEMBERSHIP, BUZZ_ALLOW_NIP_OA_AUTH, BUZZ_MEDIA_UPLOAD_RECORDS and BUZZ_SERVE_GIT_WEB_GUI accepted only exact "true"/"1". TRUE, True, On or " 1 " silently meant false, which left REST token auth or relay membership enforcement off. - BUZZ_HUDDLE_AUDIO_AVAILABLE was inverted: anything except exact "false"/"0" meant true, so FALSE or Off left huddle audio on. - BUZZ_V1_ENABLED and the two BUZZ_MEMBERSHIP_*_ENABLED flags accepted only lowercase "true". BUZZ_MESH and BUZZ_MESH_DEMO_ECHO took any-case "on" but only lowercase "true". - Numeric settings fell back to the default on any parse failure, so a typo like 10k or a u8 overflow (BUZZ_SLOW_CLIENT_GRACE_LIMIT=300) quietly ran with the default. BUZZ_OPERATOR_LISTENER_TIMEOUT_MS logged an error and did the same. - BUZZ_SEND_BUFFER=0 was accepted and reaches mpsc::channel(0) for every WebSocket connection, which panics. BUZZ_MAX_CONNECTIONS, BUZZ_MAX_CONCURRENT_HANDLERS and BUZZ_GIT_MAX_CONCURRENT_OPS accepted 0, which sizes a semaphore that admits nothing. Change: every on/off setting now goes through parse_bool. Its accepted set is unchanged: true/1/on or false/0/off in any case, surrounding whitespace ignored, blank meaning off. "yes" stays unaccepted and now fails startup instead of silently meaning false. One generic parse_optional_number / parse_number pair replaces every hand-written numeric parse. It trims the value, treats unset or blank as the default and turns a parse or range failure into a ConfigError naming the variable. parse_positive, the thin wrapper for counts, sizes and rates, replaces positive_u64_from_env. Settings that already filtered `> 0` now reject 0 instead of falling back, and the four settings above join them. Settings where 0 has a meaning keep accepting it (BUZZ_DRAIN_JITTER_MS, BUZZ_REPLICA_READ_MAX_AGE_MS, BUZZ_GIT_PACK_CACHE_MAX_BYTES, BUZZ_GIT_MAX_REPOS_PER_PUBKEY, ports). Existing clamps (BUZZ_PARTITION_AUDIT_INTERVAL_SECS, BUZZ_DRAIN_JITTER_MS, BUZZ_MEDIA_MAX_CONCURRENT_UPLOADS_PER_PUBKEY) and every default are unchanged. parse_optional_bool was a duplicate of parse_bool(name, false) and is removed. Two stale comments now say that a malformed RELAY_OWNER_PUBKEY fails startup (config.rs and main.rs). Behaviour change for deployments: a value the relay used to ignore or misread now stops startup with an error naming the variable. TRUE, On and " 1 " now enable the security flags they were meant to enable. Blank numeric values still mean the default; for the rate limits, BUZZ_REPLICA_READ_MAX_AGE_MS, BUZZ_PUSH_GATEWAY_TIMEOUT_MS and BUZZ_V1_RETENTION_SECONDS that is a relaxation (blank used to fail). A blank BUZZ_HUDDLE_AUDIO_AVAILABLE now means off, as it does for every other parse_bool setting. Helm chart: Helm renders integers of a million or more as floats, so the chart passed BUZZ_GIT_MAX_PACK_BYTES="5.24288e+08" and BUZZ_GIT_PACK_CACHE_MAX_BYTES="5.36870912e+09". The relay used to ignore both and run with its own defaults; strict parsing would refuse to start every chart install. deployment.yaml now casts every integer setting with int64 before quoting (as the storage-accounting CronJob already does), and the chart version is bumped to 0.1.12. Why safe: defaults are unchanged and every value in the chart, compose files, CI and scripts now renders a canonical spelling. The new failures only fire on values that were already being ignored or misread. Tests: env_settings_parse_strictly_or_refuse_startup is a table test through Config::from_env. It fails on main with 69 mismatches (e.g. BUZZ_REQUIRE_AUTH_TOKEN="TRUE": resolved to the wrong value; BUZZ_SEND_BUFFER="0": accepted) and passes with this change. The DB pool size tests now assert refusal instead of fallback. The chart test "renders large integer settings as plain integers" fails without the int64 casts. Signed-off-by: David Grochowski <dgrochowski@squareup.com>
ThePumpingLemma
force-pushed
the
dgrochowski/strict-env-parsing
branch
from
November 7, 2026 04:44
71db066 to
23afef7
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The relay reads about 40 settings from the environment in
crates/buzz-relay/src/config.rs. Strict helpers already existed (parse_bool,positive_u64_from_env), but most settings parsed by hand, each with its own rules. Bad values were misread or silently replaced by the default.BUZZ_REQUIRE_AUTH_TOKEN,BUZZ_PUBKEY_ALLOWLIST,BUZZ_REQUIRE_RELAY_MEMBERSHIP,BUZZ_ALLOW_NIP_OA_AUTH,BUZZ_MEDIA_UPLOAD_RECORDSandBUZZ_SERVE_GIT_WEB_GUItook only exacttrue/1.TRUE,Onor1meant false, so REST token auth or relay membership enforcement stayed off.BUZZ_HUDDLE_AUDIO_AVAILABLEwas inverted: anything but exactfalse/0meant true, soFALSEleft huddle audio on in a multi-pod deployment.BUZZ_V1_ENABLEDand the twoBUZZ_MEMBERSHIP_*_ENABLEDflags took only lowercasetrue.BUZZ_MESHtookOnbut notTRUE.BUZZ_MAX_CONNECTIONS=10k) or overflow (BUZZ_SLOW_CLIENT_GRACE_LIMIT=300, au8) quietly ran with the default.BUZZ_OPERATOR_LISTENER_TIMEOUT_MSlogged and did the same.BUZZ_SEND_BUFFER=0was accepted. It reachesmpsc::channel(0)inconnection.rs, which panics on every WebSocket connection.BUZZ_MAX_CONNECTIONS,BUZZ_MAX_CONCURRENT_HANDLERSandBUZZ_GIT_MAX_CONCURRENT_OPStook0, which sizes a semaphore that admits nothing.Change
Every on/off setting now goes through
parse_bool. Its accepted set does not change:true/1/onorfalse/0/off, any case, whitespace trimmed, blank means off.yesis still not accepted; it now fails startup instead of meaning false.One generic parser,
parse_optional_number/parse_number, replaces every hand-written numeric parse. It trims the value. Unset or blank means the default. A parse or range failure is aConfigErrorthat names the variable.parse_positive(counts, sizes, rates) replacespositive_u64_from_env.> 0now reject0instead of falling back.BUZZ_SEND_BUFFER,BUZZ_MAX_CONNECTIONS,BUZZ_MAX_CONCURRENT_HANDLERSandBUZZ_GIT_MAX_CONCURRENT_OPSjoin them.0means something keep accepting it:BUZZ_DRAIN_JITTER_MS,BUZZ_REPLICA_READ_MAX_AGE_MS,BUZZ_GIT_PACK_CACHE_MAX_BYTES,BUZZ_GIT_MAX_REPOS_PER_PUBKEYand the ports.parse_optional_boolduplicatedparse_bool(name, false)and is gone.config.rsandmain.rs) said a malformedRELAY_OWNER_PUBKEYis warned and ignored. It fails startup, and the comments now say so..env.examplestates the accepted spellings. The changelog is generated from PR titles, so there is no hand-written entry.Behaviour change for deployments with invalid values. A value the relay used to ignore or misread now stops startup with an error that names the variable.
TRUE,Onand1now turn on the flag they name, including the auth and membership flags. Blank numeric values still mean the default. That relaxes the rate limits,BUZZ_REPLICA_READ_MAX_AGE_MS,BUZZ_PUSH_GATEWAY_TIMEOUT_MSandBUZZ_V1_RETENTION_SECONDS, which used to reject blank. A blankBUZZ_HUDDLE_AUDIO_AVAILABLEnow means off, like every otherparse_boolsetting. The chart, compose files, CI and scripts in this repo all use canonical values. The chart needed one fix: Helm rendered integers of a million or more as floats (BUZZ_GIT_MAX_PACK_BYTES="5.24288e+08"), which the relay used to ignore and would now refuse.deployment.yamlnow casts every integer setting withint64before quoting. Chart version 0.1.12.Testing
New table test
config::tests::env_settings_parse_strictly_or_refuse_startupruns each case throughConfig::from_env.Before the fix, on
origin/mainplus the test:That run had 69 failing rows (57 bool, 12 numeric). After the fix:
cargo test -p buzz-relay --lib config::: 96 passed, 0 failed.cargo test -p buzz-relay --lib -- --skip audio::: 1619 passed, 0 failed, 597 ignored (they need Postgres).cargo clippy -p buzz-relay --all-targets -- -D warnings: clean.cargo fmt --all: no changes.The DB pool size tests now expect
0and junk to refuse startup instead of falling back.Not covered
audio::lib tests were skipped locally. Twoaudio::handlershadow-auth tests hang in my environment without Postgres, and the hang is the same onorigin/main, so they are left to CI.Config::from_env, and the table test covers it.config.rs(for examplenip_fi_config) is unchanged. The open drafts feat(relay): BUZZ_DB_ACQUIRE_TIMEOUT_SECS overrides the Postgres acquire timeout #8893 and feat(import): consented conversation import with original timestamps (NIP-CI) #8981 add settings inconfig.rs; they can switch toparse_number/parse_positiveafter this lands.Review
Agent review found one blocker, now fixed in this commit:
BUZZ_GIT_MAX_PACK_BYTES="5.24288e+08"andBUZZ_GIT_PACK_CACHE_MAX_BYTES="5.36870912e+09". Strict parsing would have stopped every chart install at startup.deployment.yamlnow casts every integer env value with| int64 | quote, and the chart is bumped to 0.1.12. The new helm-unittest case "renders large integer settings as plain integers" fails without the casts.Also checked, no issues: blank huddle audio is off and the chart never renders it blank, blank numeric values fall back to the default, the zero rejection list is correct, every default and clamp is unchanged, every startup error names its variable, and tests that change env vars hold
env_guards().Open optional items:
parse_bool's error says "must be true or false" but also accepts1/0/on/off. Listing the accepted spellings would make the new startup failures easier to fix.BUZZ_MAX_COMMUNITIES_PER_OWNER(buzz-db) andnip_fi_configstill parse on their own. Already listed under "Not covered".Generated with Claude Code
Exercised on a local relay
Commit tested:
23afef733. Dev profile, isolated harness relay on :3030. Each setting was tested by restarting the relay with the variable changed.BUZZ_REQUIRE_AUTH_TOKEN=false(harness default)POST /querywith only the devX-Pubkeyheader returns 200 with eventsBUZZ_REQUIRE_AUTH_TOKEN=TRUEX-Pubkey-only/queryreturns 401{"error":"missing Nostr auth"}. NIP-98 requests and WS NIP-42 auth still workBUZZ_SEND_BUFFER=0Error: Configuration error: invalid config: BUZZ_SEND_BUFFER must be a positive integerBUZZ_REQUIRE_AUTH_TOKEN=yesError: Configuration error: invalid config: BUZZ_REQUIRE_AUTH_TOKEN must be true or falseChart:
helm templatewith the chart'sci/quickstart-values.yaml(rendered from a temp copy withhelm dependency build). Every integer env renders as a plain quoted integer, includingBUZZ_GIT_MAX_PACK_BYTES: "524288000"andBUZZ_GIT_PACK_CACHE_MAX_BYTES: "5368709120". No float ore+value appears anywhere in the output. The bool envs render as"true"/"false".Nit: the refusal says
must be true or false, but1/0/on/offare also accepted. Naming the accepted spellings would help operators.Postgres-gated tests: the PR adds no ignored tests, and
--ignored config::matches 0. WithBUZZ_TEST_DATABASE_URLset, the non-ignoredcargo test -p buzz-relay --lib -- config::gives 96 passed, 0 failed.Not exercised: