security: validate provider base_url (require https for non-loopback, reject cleartext bearer) (#24)#78
Conversation
… reject cleartext bearer) (#24)
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
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 |
Summary
Closes #24.
Provider
base_urlwas taken verbatim from config/env (fetch_config(:base_url),OLLAMA_BASE_URL,SKILL_BENCH_OPENCODE_BASE_URL, …) and flowed straight intoFaraday.new(url: base_url)with no scheme/host validation.base_client#request_headersthen attachesAuthorization: Bearer <api_key>(or Azure'sapi-key) to whatever host that URL named. The consequences:http://base URL transmits the API key in cleartext.base_urlpoints the authenticated request at an arbitrary host.Ollama legitimately defaults to
http://localhost:11434, so a blanket "https only" rule would regress it.Validation rule
A new service object
SkillBench::Clients::BaseUrlValidatorenforces:http/httpsURL with a host. Blank (nil/empty) is allowed so a provider can apply its own (https) default downstream; relative/garbage/non-http values are rejected.https. Loopback hosts (localhost,127.0.0.1,::1) may usehttp— the legitimate self-hosted/Ollama case. An explicit opt-inallow_insecure_base_url: truepermits cleartext to a non-loopback host.Error messages describe only the transport (scheme/host) and never include the credential.
Where it's enforced
Enforcement is at config-load time in
ProviderConfig#call(a single helper, called once), which already resolves both the credential and the transport URLs. It validatesbase_urland Azure'sendpoint(both are user-supplied URLs the authenticated request targets), so the rule is applied consistently to OpenAI-compatible / OpenCode / Azure / Ollama providers without copy-paste per provider.base_client left untouched
base_client.rbwas intentionally not modified, to stay disjoint from the parallel determinism PR (#41) that touchesbase_client#request_body. The credential and the URL are both available at config-load time, so the cleartext check needs nothing frombase_client.Tests
test/evaluator/clients/base_url_validator_test.rb: https accepted; loopback http (localhost/127.0.0.1/[::1]) accepted with a credential; non-loopback http + credential rejected with a clear, key-free message; http without a credential allowed; opt-in flag permits http; relative/garbage/non-http rejected; blank passes through.test/evaluator/clients/provider_config_test.rb: https accepted; cleartext-with-key rejected; loopback http accepted; relative rejected;allow_insecure_base_urlopt-in; Azureendpointvalidated as a transport URL.No existing tests needed weakening. All
http://URLs already in the suite are the keyless Ollama cases (ollama_test.rb), which remain valid under the rule (no credential ⇒ structure-only check); every other provider test already usedhttps.Gates (all green from repo root)
bundle exec rubocop— 274 files, no offensesbundle exec reek— 0 warningsbundle exec rake test— 775 runs, 0 failures, 0 errors, 4 pre-existing skipsbundle exec rake yard:coverage— 0 undocumented public objects