Skip to content

security: validate provider base_url (require https for non-loopback, reject cleartext bearer) (#24)#78

Merged
igmarin merged 1 commit into
mainfrom
security/validate-base-url-24
Jun 30, 2026
Merged

security: validate provider base_url (require https for non-loopback, reject cleartext bearer) (#24)#78
igmarin merged 1 commit into
mainfrom
security/validate-base-url-24

Conversation

@igmarin

@igmarin igmarin commented Jun 30, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #24.

Provider base_url was taken verbatim from config/env (fetch_config(:base_url), OLLAMA_BASE_URL, SKILL_BENCH_OPENCODE_BASE_URL, …) and flowed straight into Faraday.new(url: base_url) with no scheme/host validation. base_client#request_headers then attaches Authorization: Bearer <api_key> (or Azure's api-key) to whatever host that URL named. The consequences:

  • Cleartext token exposure — an http:// base URL transmits the API key in cleartext.
  • SSRF / token exfiltration — a misconfigured or attacker-influenced base_url points 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::BaseUrlValidator enforces:

  1. Structure — the URL must be an absolute http/https URL 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.
  2. Secure transport — when a credential (API key / bearer token) will be attached, non-loopback hosts must use https. Loopback hosts (localhost, 127.0.0.1, ::1) may use http — the legitimate self-hosted/Ollama case. An explicit opt-in allow_insecure_base_url: true permits 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 validates base_url and Azure's endpoint (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.rb was intentionally not modified, to stay disjoint from the parallel determinism PR (#41) that touches base_client#request_body. The credential and the URL are both available at config-load time, so the cleartext check needs nothing from base_client.

Tests

  • New 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.
  • Extended test/evaluator/clients/provider_config_test.rb: https accepted; cleartext-with-key rejected; loopback http accepted; relative rejected; allow_insecure_base_url opt-in; Azure endpoint validated 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 used https.

Gates (all green from repo root)

  • bundle exec rubocop — 274 files, no offenses
  • bundle exec reek — 0 warnings
  • bundle exec rake test — 775 runs, 0 failures, 0 errors, 4 pre-existing skips
  • bundle exec rake yard:coverage — 0 undocumented public objects

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@igmarin, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 45 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 6eaac914-1595-4b24-9a23-57b157ab4664

📥 Commits

Reviewing files that changed from the base of the PR and between 8011f03 and acc9178.

📒 Files selected for processing (7)
  • .reek.yml
  • lib/skill_bench/clients/README.md
  • lib/skill_bench/clients/all.rb
  • lib/skill_bench/clients/base_url_validator.rb
  • lib/skill_bench/clients/provider_config.rb
  • test/evaluator/clients/base_url_validator_test.rb
  • test/evaluator/clients/provider_config_test.rb

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@igmarin
igmarin merged commit 29c9a66 into main Jun 30, 2026
5 checks passed
@igmarin
igmarin deleted the security/validate-base-url-24 branch June 30, 2026 20:50
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.

Validate provider base_url (require http(s), reject cleartext bearer token, SSRF guard)

1 participant