Skip to content

feat: typed AppConfig with fail-fast validation and clippy deny lints - #350

Open
milah-247 wants to merge 1 commit into
Bonizozo:mainfrom
milah-247:feat/typed-app-config-339
Open

feat: typed AppConfig with fail-fast validation and clippy deny lints#350
milah-247 wants to merge 1 commit into
Bonizozo:mainfrom
milah-247:feat/typed-app-config-339

Conversation

@milah-247

Copy link
Copy Markdown
Contributor

Implements typed, validated AppConfig loaded once at startup with aggregated error reporting (issue #339).

Changes

Core: src/config/app_config.rs (new)

  • AppConfig::from_env() reads ALL env vars once at startup
  • Collects every validation error before returning (never stops at first)
  • ConfigError displays as a human-readable bullet list
  • JWT secret: minimum 32 bytes + Shannon entropy ≥ 3.5 bits/byte enforced
  • Covers: DATABASE_URL, REDIS_URL, JWT_SECRET, SMTP_, CORS_, RATE_LIMIT_, WEBHOOK_SECRET, STELLAR_, PORT, REQUEST_TIMEOUT_SECS, PAGINATION_, TIP_ (all documented in module-level table)

Threading AppConfig through AppState: src/db/connection.rs

  • AppState gains config: Arc
  • All callers use cfg.xxx — zero runtime std::env::var on request paths

main.rs

  • Loads AppConfig::from_env() fail-fast at startup
  • All middleware/services receive typed config fields — no env reads

Middleware refactored off env reads

  • src/middleware/cors.rs: new cors_layer(&CorsConfig) replaces per-call env reads
  • src/middleware/rate_limiter.rs: reads from RateLimitConfig
  • src/middleware/timeout.rs: reads from AppConfig.request_timeout

Services refactored off env reads

  • src/services/auth_service.rs: jwt_secret parameter (no env read)
  • src/middleware/auth.rs: reads state.config.jwt.secret

Documented startup-only env reads (explicitly justified)

  • src/logging/mod.rs: LOG_FORMAT read before AppConfig exists (boot order)
  • src/telemetry/tracer.rs: OTEL_* follow OpenTelemetry naming convention

Lint enforcement: src/lib.rs

  • #![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::expect_used))]
  • All non-test expect/unwrap sites annotated with invariant comments and #[allow(clippy::expect_used)] / #[allow(clippy::unwrap_used)]

Unwrap/expect classification (non-test sites): | Site | Classification | Resolution |

|------|---------------|------------|
| config/app_config.rs: required field Some-checks | startup-path invariant | #[allow] + comment | | config/app_config.rs: Decimal::from_str literals | startup-path invariant | #[allow] + comment | | metrics/collectors.rs: OnceLock accessors | programmer invariant | #[allow] + comment | | middleware/rate_limiter.rs: GovernorConfig | startup-path invariant | #[allow] + comment | | webhooks/signature.rs: HMAC key length | crypto invariant | #[allow] + comment | | services/retry.rs: last_err in loop | loop invariant | #[allow] + comment | | shutdown.rs: signal handler install | OS invariant | #[allow] + comment | | models/creator.rs: regex literal | compile-time constant | OnceLock fn + #[allow] |

Startup-failure integration tests: tests/config_startup_test.rs

  • boot_fails_cleanly_when_jwt_secret_missing
  • boot_fails_cleanly_when_database_url_missing
  • boot_error_is_aggregated_not_first_only
  • boot_fails_on_weak_jwt_secret
  • boot_succeeds_with_valid_config
  • error_display_is_human_readable

All 6 tests pass. Zero clippy errors on unwrap_used/expect_used lints.

Closes #339

Implements typed, validated AppConfig loaded once at startup with
aggregated error reporting (issue Bonizozo#339).

## Changes

### Core: src/config/app_config.rs (new)
- AppConfig::from_env() reads ALL env vars once at startup
- Collects every validation error before returning (never stops at first)
- ConfigError displays as a human-readable bullet list
- JWT secret: minimum 32 bytes + Shannon entropy ≥ 3.5 bits/byte enforced
- Covers: DATABASE_URL, REDIS_URL, JWT_SECRET, SMTP_*, CORS_*,
  RATE_LIMIT_*, WEBHOOK_SECRET, STELLAR_*, PORT, REQUEST_TIMEOUT_SECS,
  PAGINATION_*, TIP_* (all documented in module-level table)

### Threading AppConfig through AppState: src/db/connection.rs
- AppState gains config: Arc<AppConfig>
- All callers use cfg.xxx — zero runtime std::env::var on request paths

### main.rs
- Loads AppConfig::from_env() fail-fast at startup
- All middleware/services receive typed config fields — no env reads

### Middleware refactored off env reads
- src/middleware/cors.rs: new cors_layer(&CorsConfig) replaces per-call env reads
- src/middleware/rate_limiter.rs: reads from RateLimitConfig
- src/middleware/timeout.rs: reads from AppConfig.request_timeout

### Services refactored off env reads
- src/services/auth_service.rs: jwt_secret parameter (no env read)
- src/middleware/auth.rs: reads state.config.jwt.secret

### Documented startup-only env reads (explicitly justified)
- src/logging/mod.rs: LOG_FORMAT read before AppConfig exists (boot order)
- src/telemetry/tracer.rs: OTEL_* follow OpenTelemetry naming convention

### Lint enforcement: src/lib.rs
- #![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::expect_used))]
- All non-test expect/unwrap sites annotated with invariant comments
  and #[allow(clippy::expect_used)] / #[allow(clippy::unwrap_used)]

### Unwrap/expect classification (non-test sites):
| Site | Classification | Resolution |
|------|---------------|------------|
| config/app_config.rs: required field Some-checks | startup-path invariant | #[allow] + comment |
| config/app_config.rs: Decimal::from_str literals | startup-path invariant | #[allow] + comment |
| metrics/collectors.rs: OnceLock accessors | programmer invariant | #[allow] + comment |
| middleware/rate_limiter.rs: GovernorConfig | startup-path invariant | #[allow] + comment |
| webhooks/signature.rs: HMAC key length | crypto invariant | #[allow] + comment |
| services/retry.rs: last_err in loop | loop invariant | #[allow] + comment |
| shutdown.rs: signal handler install | OS invariant | #[allow] + comment |
| models/creator.rs: regex literal | compile-time constant | OnceLock fn + #[allow] |

### Startup-failure integration tests: tests/config_startup_test.rs
- boot_fails_cleanly_when_jwt_secret_missing
- boot_fails_cleanly_when_database_url_missing
- boot_error_is_aggregated_not_first_only
- boot_fails_on_weak_jwt_secret
- boot_succeeds_with_valid_config
- error_display_is_human_readable

All 6 tests pass. Zero clippy errors on unwrap_used/expect_used lints.

Closes Bonizozo#339

@Christopherdominic Christopherdominic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on this one — spot-checked config/app_config.rs, middleware/auth.rs, middleware/cors.rs, middleware/cache.rs, and webhooks/signature.rs. The aggregated-error design is solid (collects every issue instead of failing fast on the first), the JWT entropy check is a genuinely useful gate that the old code didn't have, the CORS builder correctly never combines wildcard-origin with allow_credentials(true), and every unwrap/expect I looked at is on a real startup-time invariant with a comment justifying it (not just silenced). The cache.rs change from .expect() to a graceful unwrap_or_else fallback on header construction is a genuine correctness improvement, not just noise.

Only blocker: this branch conflicts with main (mergeStateStatus: DIRTY) — please rebase. Given the size (40 files) I didn't do a line-by-line pass of every file, but nothing in the security-relevant paths I checked raised concerns. Ping me once it's rebased and I'll take another look before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Eliminate All 251 Request-Path Panics and Introduce Fail-Fast Typed Startup Configuration

2 participants