[D3SOX]

The server ignored nested environment variables such as OIDC__PROVIDER_URL. The environment source had no separator, so OIDC could only be configured in config.toml.

• Nested options now use __ as the separator. OIDC__CLIENT_ID sets client_id in [oidc]. Flat variables such as SECRET_KEY and TRUSTED_PROXY_HOPS work as before.
• allowed_redirect_urls also accepts a comma-separated string, so OIDC__ALLOWED_REDIRECT_URLS=myapp://oidc,https://app.example.com/oidc works. Without it, an environment-only OIDC setup can't log anyone in, because the server refuses OIDC login while that list is empty.
• The README explains the separator and the list format.
• RateLimiter::check now notes that failed attempts count toward the limit on purpose. If they didn't, password guessing would be unlimited.

Proof: a new unit test sets OIDC__* variables. It failed before the change ("OIDC__* variables should set oidc") and passes now. A second test checks that an environment variable overrides one option of a config.toml section. cargo fmt, cargo clippy -D warnings and cargo test pass.

Reviewed with sol-loop: 1 round with gpt-6.1-sol on high.
Sol verdict: approved

Changes made by Claude Opus 5.5 in Claude Code (T3 Code).

[sourcery-ai]

Sorry @D3SOX, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 2 days and 3 hours by commenting @sourcery-ai review. Upgrade to get a review now.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

Configuration loading now supports nested environment variables and comma-separated OIDC redirect URLs. The README documents these formats and their Compose setup. Rate-limit documentation clarifies that failed requests count toward the limit.

Changes: Configuration inputs

Layer / File(s)	Summary
Environment and OIDC configuration src/config.rs, README.md	Config::build loads nested environment keys using __. OIDC redirect URLs accept lists or comma-separated strings. Tests cover environment settings and merging environment values with TOML values. The README documents the environment variable format and Compose requirements.

Rate-limit documentation

Layer / File(s)	Summary
Rate-limit check clarification src/rate_limit.rs	The RateLimiter::check documentation states that failed requests, including failed login attempts, count toward the rate limit.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related PRs

• OpenTubeX/sync-server#33: Moves configuration loading to Config::build(), whose environment variable contract this change updates.
• OpenTubeX/sync-server#32: Adds the allowed_redirect_urls field whose deserialization this change updates.
• OpenTubeX/sync-server#31: Moves configuration loading to Config::build(), whose environment variable contract this change updates.

Suggested reviewers: unixource, bnyro

Merge Risk: Minimal · up to 5b008

The documented configuration approach has no established outstanding issue in the supplied evidence. Normal validation remains appropriate before merging.

Security architecture risk: Low · up to 849b0

The new configuration inputs retain startup validation and exact-match redirect restrictions. No introduced authentication bypass was identified. Risk is low, with remaining uncertainty around deployed environment access and mixed file/environment allowlist behavior. Rolling back an environment-only setup also requires restoring the previous configuration format.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — Nested environment values can now determine the provider and token-receiving client destinations for the configured server instance. This makes process-environment configuration security-sensitive, but no request-controlled route to modifying those settings was identified. Actual deployment access to that environment remains unverified.

Trust Boundaries and Controls

• observed — Environment-derived settings pass through the same startup validation as file settings. Provider discovery disables HTTP redirect following, and request paths reject an uninitialized provider client rather than falling back to an unverified login path.
• observed — OIDC callback state is removed under a mutex before token exchange, making it single-use across concurrent callbacks. Exchange uses the stored PKCE verifier and redirect URI, and token claims are checked with the stored nonce. Callback kinds are separated, and deletion additionally requires the provider subject to match the initiating account. These controls predate this PR.

Resilience and Maintainability Implications

• observed — Provider initialization failure leaves OIDC requests failing closed. Once callback processing consumes a challenge, exchange failure cannot reuse that challenge; a new flow is required. Challenges are process-local, with no expiry or restart recovery visible in the inspected implementation. These lifecycle characteristics are unchanged by the PR.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title names the main change: nested options such as OIDC__CLIENT_ID are now read from environment variables. The raw summary confirms config.rs now uses __ as the nested-key separator, and the des…
Description check		The description states the problem (nested environment variables such as OIDC__PROVIDER_URL were ignored) and lists the changes: __ as separator, comma-separated allowed_redirect_urls, README updates,…
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names the main change: nested options such as OIDC__CLIENT_ID are now read from environment variables. The raw summary confirms config.rs now uses __ as the nested-key separator, and the description and objectives describe the same change.

Full details: Description check

Explanation

The description states the problem (nested environment variables such as OIDC__PROVIDER_URL were ignored) and lists the changes: __ as separator, comma-separated allowed_redirect_urls, README updates, and the RateLimiter::check doc note. These match the raw summary for README.md, src/config.rs, and src/rate_limit.rs. It also describes the tests.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)

Finishing Touches

Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 95b3c9b9-1c15-40f7-8cd2-9ea93b27a93f

Commits

Reviewing files that changed from the base of the PR and between 849b047 and 5b008a8.

Files selected for processing: 1

• README.md

Files skipped from review as they are similar to previous changes: 1

• README.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.

[coderabbitai]

• Fix comments: Fix CodeRabbit comments on this PR