fix: config UX & security validation (PR2 of bug-audit-v2)#109
Merged
Conversation
ab30f94 to
108b59d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Per-task TDD steps covering UX-1, UX-2, UX-3, SEC-1, SEC-2, SEC-3 (plus folded TEST-NEW-1 and TEST-NEW-6) from the 2026-06-05 audit. Task ordering accounts for one cross-task interaction: Task 5 (SEC-2) adds super().__post_init__() to FastAPIConfig.__post_init__ which Task 1 (UX-1) restructures first. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move the application.title/.debug/.version assignments inside the UnsetType branch so they only apply when lite-bootstrap built the FastAPI() instance. Pre-configured user apps now keep their construction-time values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…UX-2) Add prometheus_collector_registry: CollectorRegistry | None config field on FastStreamConfig. When non-None, FastStreamPrometheusInstrument uses the injected registry; otherwise the existing per-instance default is preserved. Lets users expose metrics that were registered on a shared registry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
FastAPIConfig and LitestarConfig already expose opentelemetry_excluded_urls; FastStream relied on getattr fallback with no discoverable field. Add the field to FastStreamConfig matching the FastAPI/Litestar pattern. _build_excluded_urls behavior is unchanged — the getattr access still works the same way. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…C-1) The swagger/redoc handlers reflected request.scope["root_path"] verbatim into HTML script/link tags. In default ASGI deployments root_path comes from server config (uvicorn --root-path), but if a user enables ProxyHeadersMiddleware with trusted X-Forwarded-Prefix, a malicious proxy could inject script content. Validate via the project's existing is_valid_path allowlist and fall back to empty (with a warning) on invalid input. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add OpenTelemetryConfig.__post_init__ that emits a warning when opentelemetry_endpoint points to a non-local host AND opentelemetry_insecure is True (the unfortunate default). Localhost, 127.0.0.1, ::1, and unix:// endpoints are silent. FastAPIConfig.__post_init__ now calls super().__post_init__() so FastAPI users see the warning too; other framework configs (FreeConfig, FastStreamConfig, LitestarConfig) don't have their own __post_init__ and inherit the new behavior automatically. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(SEC-3) cors_allowed_credentials=True with cors_allowed_origins=["*"] (or a permissive regex like ".*"/".+") is the canonical CORS misconfiguration. FastAPI's CORSMiddleware refuses to set the credentials header in that combo but Litestar's behavior differs and may silently accept it. Add a CorsConfig __post_init__ that raises ConfigurationError on the unsafe combination at config-construction time, before any framework sees the values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CorsConfig.__post_init__ (SEC-3) and OpenTelemetryConfig.__post_init__ (SEC-2) now call super().__post_init__() so the MRO chain propagates. Before this, CorsConfig.__post_init__ blocked the cascade for FastAPIConfig users (Cors precedes OpenTelemetry in MRO), so SEC-2's warning didn't fire. BaseConfig gains a no-op __post_init__ as the chain terminator. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
108b59d to
895f2b1
Compare
4 tasks
lesnik512
added a commit
that referenced
this pull request
Jun 5, 2026
Three PRs (#108, #109, #110), 26 findings closed, 153 → 187 tests, 100% coverage throughout. Captures what went well (per-PR sequencing, two-stage review catching three real bugs), what didn't (three implementer-hallucination incidents, cross-task __post_init__ cascade missed in PR2 plan, OTel API misread in audit, SEC-2 host parser untested against canonical localhost:4317 form), and six action items to harden the pipeline for the next audit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
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.
Summary
PR2 of the 2026-06-05 bug-audit-v2 plan. Closes six config-layer findings — three UX rough edges (FastAPI app stomping, missing FastStream config fields) and three security validators (root_path escape, insecure OTel exporter, unsafe CORS). All validation happens at
__post_init__time on frozen dataclass configs so the entire instrument lifecycle is unreached on misconfiguration. Sequenced perplanning/specs/2026-06-05-bug-audit-v2-sequencing.md; per-task TDD plan atplanning/plans/2026-06-05-pr2-config-security.md.Findings closed
FastAPIConfig.__post_init__no longer stompsapplication.title/debug/versionwhen the user supplies a pre-configuredFastAPI()instance. The three assignments moved inside theUnsetTypebranch so they only apply when lite-bootstrap built the app.prometheus_collector_registry: CollectorRegistry | None = NonetoFastStreamConfig. When non-None,FastStreamPrometheusInstrumentuses the injected registry (via__post_init__); otherwise the existing per-instance default is preserved. Backward-compatible.opentelemetry_excluded_urls: list[str]toFastStreamConfig, matching the existing fields onFastAPIConfig/LitestarConfig._build_excluded_urlsalready read viagetattrfallback so behavior is unchanged; the field is now discoverable.enable_offline_docsvalidatesrequest.scope["root_path"]through the project's existingis_valid_pathallowlist before interpolating into Swagger/Redoc HTML. Invalid paths fall back to empty with a warning. Covers the X-Forwarded-Prefix proxy-header injection vector.OpenTelemetryConfig.__post_init__that emits a warning whenopentelemetry_endpointresolves to a non-local host ANDopentelemetry_insecure=True(the unfortunate default).localhost,127.0.0.1,::1, andunix://endpoints are silent. Host parsing handles bothhost:port(canonical OTLP gRPC) andscheme://host:port, including IPv6 brackets.CorsConfig.__post_init__that raisesConfigurationErrorwhencors_allowed_credentials=Trueis combined with a wildcard origin ("*"in the list) or a permissive regex (.*,.+). FastAPI's CORSMiddleware silently refuses the combo; Litestar's behavior differs. Validation at config-construction is consistent across frameworks.Implementation notes
Two reviewer-driven course corrections during execution:
urllib.parse.urlparse+path.split(":")[0]fallback. Verified that this misparses the canonicallocalhost:4317OTLP form (urlparse treatslocalhostas the scheme and yieldspath='4317'). Fixed by prepending//to schemeless inputs so urlparse always returns a correcthostname. Added parametrized regression tests covering five local forms (localhost:4317,127.0.0.1:4317,[::1]:4317,http://localhost:4317,https://127.0.0.1:4317) and three remote forms (collector.example.com:4317,http://collector.example.com:4317,grpc://collector.example.com:4317).CorsConfigprecedesOpenTelemetryConfiginFastAPIConfig's MRO; the newCorsConfig.__post_init__returned early on the default (no credentials) without callingsuper().__post_init__(), so the OTel warning never fired for FastAPIConfig. Fixed by establishing the cascade invariant:BaseConfiggains a no-op__post_init__as the terminator; every config-class__post_init__callssuper().__post_init__()after its own logic.One subtle Python quirk surfaced:
super(FastAPIConfig, self).__post_init__()is required inFastAPIConfig.__post_init__rather than baresuper().@dataclass(slots=True)replaces the class object after the class body compiles, breaking the compile-time__class__cell that baresuper()relies on. Comment in the code documents the constraint.Non-blocking follow-ups flagged by the final reviewer
LitestarConfig,FreeConfig,FastStreamConfigfor explicit regression coverage of the other framework configs (currently covered transitively via the sharedOpenTelemetryConfig.__post_init__, but not directly).stacklevel=2now lands on lite-bootstrap internals (due to the cascade) rather than user code. Acceptable — the warning text is actionable enough that users can find the offending config.Test plan
just test— 175 passed (164 PR1 baseline ↔ unrelated to PR2; PR2 adds 11 new tests on its own pre-PR1 baseline), 100% coverage.just lint-ci— clean (ruff, eof-fixer, ty)."otl"test-fixture warning via alocalhost:4317swap intest_config.py).Sequencing note
PR2 branches off
main(NOT off PR1's branch). When PR1 (#108) merges first, PR2 rebases cleanly — they touch overlapping files (opentelemetry_instrument.py,fastapi_bootstrapper.py) but at non-overlapping line ranges. If PR2 merges first, PR1's diff narrows similarly.🤖 Generated with Claude Code