fix(security): only waive TLS verification for loopback health checks - #7
Conversation
check_service_health is a #[tauri::command] taking a URL straight from the frontend, and built its reqwest client with danger_accept_invalid_certs(true) unconditionally. The comment said "For localhost self-signed certs", but nothing enforced that — certificate validation was disabled for every URL it was given, including remote https endpoints. The waiver is now gated on the target actually being loopback: - is_loopback() decides from the parsed host alone, so a name that merely resolves to loopback cannot talk deck into skipping validation. Matching on url::Host also covers the alternate IPv4 spellings the parser normalises (127.1, 0x7f.0.0.1) and bracketed IPv6. - The waiver is per-client rather than per-request, so it would otherwise follow a redirect to whatever host came next. Loopback probes get a redirect policy that refuses to leave loopback; a probe redirecting off-host reports unhealthy instead of trusting an unvalidated cert. - Non-http(s) schemes and malformed URLs are now caller errors (Err) rather than being reported as an unhealthy service (Ok(false)). Remote endpoints keep full certificate validation, which is what the ops-cockpit work needs: docs/specs/deck-ops-cockpit.md §1.3 notes deck calls out via native reqwest, so capabilities/default.json does not constrain outbound requests and allowlisting has to happen in Rust. 6 tests cover loopback spellings, remote hosts, lookalike hosts (localhost.example.com, localhost@evil.example.com), scheme rejection and malformed input. cargo fmt clean; clippy warning count unchanged at the 6 pre-existing ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe service health check now validates HTTP(S) URLs, applies a two-second timeout, limits redirects, and waives TLS validation only for loopback hosts. Tests cover URL and host validation behavior. ChangesService health-check security controls
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Closes the
danger_accept_invalid_certsissue tracked as baton8e072344.The problem
check_service_health(status.rs:40) is a registered#[tauri::command]takingurl: Stringstraight from the frontend, and built its reqwest client withdanger_accept_invalid_certs(true)unconditionally. The comment said "For localhost self-signed certs" — but nothing enforced that. Certificate validation was disabled for every URL it was handed, including remote https endpoints.The fix
The waiver is now gated on the target actually being loopback:
is_loopback()decides from the parsed host alone, so a name that merely resolves to loopback cannot talk deck into skipping validation. Matching onurl::Hostrather than the host string also covers the alternate IPv4 spellings the parser normalises (http://127.1,http://0x7f.0.0.1) and bracketed IPv6 (http://[::1]).danger_accept_invalid_certsis per-client, not per-request, so it would otherwise cover whatever host reqwest followed a redirect to. Loopback probes get a redirect policy that refuses to leave loopback; a probe that redirects off-host reports unhealthy rather than trusting an unvalidated certificate.Err) rather than being reported as an unhealthy service (Ok(false)).Remote endpoints keep full certificate validation.
Why now
docs/specs/deck-ops-cockpit.md§1.3 (landed in #6) plans new outbound HTTP from Rust to sluice, docket and story-gen, and notes that because deck uses nativereqwestrather than the Tauri HTTP plugin,capabilities/default.jsondoes not constrain outbound requests at all — allowlisting has to be enforced in Rust. This gives that work a clean base instead of a pattern worth copying.Verification
6 tests covering loopback spellings, remote hosts, lookalike hosts (
localhost.example.com,localhost@evil.example.com), scheme rejection and malformed input:cargo fmt --all --checkclean. Clippy warning count unchanged at the 6 pre-existing ones documented in.github/workflows/ci.yml; this change adds none.Provenance
The implementation was written by an earlier background agent whose session died before it could commit — the work survived only as an uncommitted diff in an abandoned worktree, and was recovered rather than rewritten.
🤖 Generated with Claude Code
Summary by CodeRabbit