From acc9178662104ec0f995fe316d1369355e5b46f6 Mon Sep 17 00:00:00 2001 From: Ismael G Marin C Date: Tue, 30 Jun 2026 14:22:22 -0600 Subject: [PATCH] security: validate provider base_url (require https for non-loopback, reject cleartext bearer) (#24) --- .reek.yml | 1 + lib/skill_bench/clients/README.md | 2 + lib/skill_bench/clients/all.rb | 1 + lib/skill_bench/clients/base_url_validator.rb | 105 ++++++++++++++++++ lib/skill_bench/clients/provider_config.rb | 33 +++++- .../clients/base_url_validator_test.rb | 80 +++++++++++++ .../evaluator/clients/provider_config_test.rb | 60 ++++++++++ 7 files changed, 281 insertions(+), 1 deletion(-) create mode 100644 lib/skill_bench/clients/base_url_validator.rb create mode 100644 test/evaluator/clients/base_url_validator_test.rb diff --git a/.reek.yml b/.reek.yml index cfc6a45..7d3605c 100644 --- a/.reek.yml +++ b/.reek.yml @@ -174,6 +174,7 @@ detectors: BooleanParameter: exclude: - "SkillBench::Commands::Init#self.run" + - "SkillBench::Clients::BaseUrlValidator#self.call" # Named transport flags (has_credential/allow_insecure) — security policy inputs UtilityFunction: enabled: false NilCheck: diff --git a/lib/skill_bench/clients/README.md b/lib/skill_bench/clients/README.md index 5709a3a..e707791 100644 --- a/lib/skill_bench/clients/README.md +++ b/lib/skill_bench/clients/README.md @@ -125,3 +125,5 @@ Every client, regardless of its internal complexity, guarantees a standard respo - **Silent Errors**: We prioritize "Fail Fast, Fail Clean". Errors are caught, logged with 5-line backtraces, and returned as `{ success: false }`. - **JSON Safety**: Robust parsing prevents malformed LLM responses from crashing the system. - **URL Sanitization**: All provider URL parameters are CGI-escaped to prevent injection attacks. +- **Base URL Validation**: `ProviderConfig` runs every provider's transport URL (`base_url`, and Azure's `endpoint`) through `BaseUrlValidator` at config-load time. A URL must be an absolute `http(s)` URL with a host; blank/relative/garbage values are rejected. When a credential (API key / bearer token) is attached, **non-loopback hosts must use `https`** so the token is never sent in cleartext (mitigating SSRF + token exfiltration). Loopback hosts (`localhost`, `127.0.0.1`, `::1`) may use `http` — the legitimate self-hosted/Ollama case — and `allow_insecure_base_url: true` is an explicit opt-in for cleartext to a non-loopback host. Error messages describe only the transport and never include the credential. + diff --git a/lib/skill_bench/clients/all.rb b/lib/skill_bench/clients/all.rb index df6250f..d52bc11 100644 --- a/lib/skill_bench/clients/all.rb +++ b/lib/skill_bench/clients/all.rb @@ -5,6 +5,7 @@ require_relative 'response_builder' require_relative 'request_builder' require_relative 'retry_handler' +require_relative 'base_url_validator' require_relative 'base_client' require_relative 'provider_config' require_relative 'provider_registry' diff --git a/lib/skill_bench/clients/base_url_validator.rb b/lib/skill_bench/clients/base_url_validator.rb new file mode 100644 index 0000000..59bcdbc --- /dev/null +++ b/lib/skill_bench/clients/base_url_validator.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +require 'uri' + +module SkillBench + module Clients + # Validates a provider `base_url` before it is used to build an HTTP + # connection that may carry an API key / bearer token. + # + # Security rationale: `base_url` is taken verbatim from config/env input and + # the authenticated request attaches a credential to whatever host it names. + # Left unchecked this is an SSRF surface, and an `http://` URL would transmit + # the credential in cleartext. This service enforces: + # + # - the URL must be an absolute `http`/`https` URL with a host (empty/relative + # /garbage values are rejected); + # - when a credential 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 — and an explicit opt-in + # (`allow_insecure_base_url`) can permit cleartext for non-loopback hosts. + # + # A blank (`nil`/empty) `base_url` is allowed so providers may supply their + # own (https) default downstream. Error messages describe only the transport + # and never include the credential. + class BaseUrlValidator + # Hosts permitted to use cleartext `http` even with a credential attached. + LOOPBACK_HOSTS = %w[localhost 127.0.0.1 ::1].freeze + + # Raised when a base URL is structurally invalid or would leak a credential + # over cleartext transport. The message never contains the credential. + class InvalidBaseURLError < StandardError; end + + # Validates a base URL and returns it unchanged when valid. + # + # @param base_url [String, nil] the URL to validate; blank values are + # returned as-is so a provider default can be applied later. + # @param has_credential [Boolean] whether a credential (api key/bearer + # token) will be attached to requests sent to this URL. + # @param allow_insecure [Boolean] explicit opt-in that permits cleartext + # `http` to a non-loopback host even when a credential is attached. + # @raise [InvalidBaseURLError] when the URL is invalid or insecure. + # @return [String, nil] the validated URL (blank input returned unchanged). + def self.call(base_url:, has_credential: false, allow_insecure: false) + new(base_url, has_credential, allow_insecure).call + end + + # @param base_url [String, nil] the URL to validate. + # @param has_credential [Boolean] whether a credential will be attached. + # @param allow_insecure [Boolean] opt-in permitting cleartext non-loopback. + def initialize(base_url, has_credential, allow_insecure) + @base_url = base_url + @has_credential = has_credential + @allow_insecure = allow_insecure + end + + # Runs the validation. + # + # @raise [InvalidBaseURLError] when the URL is invalid or insecure. + # @return [String, nil] the validated URL. + def call + return @base_url if blank?(@base_url) + + validate_absolute_http_url! + validate_secure_transport! + @base_url + end + + private + + def blank?(value) + value.to_s.strip.empty? + end + + def uri + @uri ||= URI.parse(@base_url.to_s) + rescue URI::InvalidURIError + nil + end + + def validate_absolute_http_url! + return if uri.is_a?(URI::HTTP) && !blank?(uri.hostname) + + raise InvalidBaseURLError, + "Invalid provider base_url #{@base_url.inspect}: " \ + 'must be an absolute http(s) URL with a host.' + end + + def validate_secure_transport! + return unless @has_credential + return if uri.scheme == 'https' + return if loopback? + return if @allow_insecure + + raise InvalidBaseURLError, + 'Insecure provider base_url: refusing to send a credential over cleartext http ' \ + "to non-loopback host #{uri.hostname.inspect}. Use https, target a loopback host, " \ + 'or set allow_insecure_base_url: true to override.' + end + + def loopback? + LOOPBACK_HOSTS.include?(uri.hostname) + end + end + end +end diff --git a/lib/skill_bench/clients/provider_config.rb b/lib/skill_bench/clients/provider_config.rb index c55d740..8ca45e2 100644 --- a/lib/skill_bench/clients/provider_config.rb +++ b/lib/skill_bench/clients/provider_config.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require_relative '../config' +require_relative 'base_url_validator' module SkillBench module Clients @@ -23,8 +24,21 @@ def initialize(provider, options) # Loads and returns standardized provider configuration. # + # The resolved transport URLs (`base_url` and, for Azure, `endpoint`) are + # validated before being returned: they must be absolute http(s) URLs, and + # a credential is never sent over cleartext http to a non-loopback host. + # + # @raise [BaseUrlValidator::InvalidBaseURLError] when a transport URL is + # structurally invalid or would leak the credential over cleartext http. # @return [Hash] Standardized configuration with api_key, model, base_url, etc. def call + validate_transport_urls! + standardized_config + end + + private + + def standardized_config { api_key: fetch_config(:api_key), model: fetch_config(:model), @@ -39,7 +53,24 @@ def call } end - private + # Validates every transport URL that could carry the credential. Both + # `base_url` and Azure's `endpoint` are user-supplied URLs that the + # authenticated request targets, so both are checked with one helper. + # + # @raise [BaseUrlValidator::InvalidBaseURLError] on an invalid/insecure URL. + # @return [void] + def validate_transport_urls! + has_credential = !fetch_config(:api_key).to_s.empty? + allow_insecure = truthy?(fetch_config(:allow_insecure_base_url)) + + [fetch_config(:base_url), fetch_config(:endpoint)].each do |url| + BaseUrlValidator.call(base_url: url, has_credential: has_credential, allow_insecure: allow_insecure) + end + end + + def truthy?(value) + value == true || value.to_s.strip.casecmp?('true') + end def fetch_config(key) @options[key] || @config[key] diff --git a/test/evaluator/clients/base_url_validator_test.rb b/test/evaluator/clients/base_url_validator_test.rb new file mode 100644 index 0000000..75fbbec --- /dev/null +++ b/test/evaluator/clients/base_url_validator_test.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require 'test_helper' + +module SkillBench + module Clients + class BaseUrlValidatorTest < Minitest::Test + Error = SkillBench::Clients::BaseUrlValidator::InvalidBaseURLError + + def test_accepts_https_non_loopback_with_credential + url = 'https://api.example.com' + + assert_equal url, BaseUrlValidator.call(base_url: url, has_credential: true) + end + + def test_accepts_http_loopback_hosts_with_credential + %w[ + http://localhost:11434 + http://127.0.0.1:11434 + http://[::1]:11434 + ].each do |url| + assert_equal url, BaseUrlValidator.call(base_url: url, has_credential: true), + "expected loopback URL to be accepted: #{url}" + end + end + + def test_rejects_http_non_loopback_with_credential + error = assert_raises(Error) do + BaseUrlValidator.call(base_url: 'http://evil.example.com', has_credential: true) + end + + assert_match(/cleartext http/i, error.message) + assert_match(/evil.example.com/, error.message) + end + + def test_error_message_never_leaks_credential + error = assert_raises(Error) do + BaseUrlValidator.call(base_url: 'http://evil.example.com', has_credential: true) + end + + # The validator is never given the secret, but assert defensively that the + # message only describes the transport, not any credential material. + refute_match(/bearer\s+\S/i, error.message) + end + + def test_accepts_http_non_loopback_without_credential + url = 'http://internal-proxy.example.com' + + assert_equal url, BaseUrlValidator.call(base_url: url, has_credential: false) + end + + def test_opt_in_permits_http_non_loopback_with_credential + url = 'http://internal-proxy.example.com' + + assert_equal url, BaseUrlValidator.call(base_url: url, has_credential: true, allow_insecure: true) + end + + def test_rejects_relative_url + ['/v1/chat/completions', 'api.example.com/v1'].each do |url| + assert_raises(Error, "expected relative URL to be rejected: #{url}") do + BaseUrlValidator.call(base_url: url, has_credential: true) + end + end + end + + def test_rejects_garbage_and_non_http_schemes + ['not a url', 'ftp://example.com', 'http://', 'https://'].each do |url| + assert_raises(Error, "expected invalid URL to be rejected: #{url}") do + BaseUrlValidator.call(base_url: url, has_credential: false) + end + end + end + + def test_allows_blank_url_so_provider_can_supply_default + assert_nil BaseUrlValidator.call(base_url: nil, has_credential: true) + assert_equal '', BaseUrlValidator.call(base_url: '', has_credential: true) + end + end + end +end diff --git a/test/evaluator/clients/provider_config_test.rb b/test/evaluator/clients/provider_config_test.rb index a7b9de4..5d3f7da 100644 --- a/test/evaluator/clients/provider_config_test.rb +++ b/test/evaluator/clients/provider_config_test.rb @@ -60,6 +60,66 @@ def test_call_includes_provider_specific_extras assert_equal 'test-project', result[:project_id] assert_equal 'us-central1', result[:location] end + + def test_call_accepts_https_base_url_with_key + result = ProviderConfig.call( + provider: :openai, + options: { api_key: 'key', base_url: 'https://api.example.com' } + ) + + assert_equal 'https://api.example.com', result[:base_url] + end + + def test_call_rejects_cleartext_base_url_with_key + error = assert_raises(BaseUrlValidator::InvalidBaseURLError) do + ProviderConfig.call( + provider: :openai, + options: { api_key: 'key', base_url: 'http://evil.example.com' } + ) + end + + assert_match(/cleartext http/i, error.message) + end + + def test_call_accepts_loopback_http_base_url_with_key + result = ProviderConfig.call( + provider: :ollama, + options: { api_key: 'key', base_url: 'http://localhost:11434' } + ) + + assert_equal 'http://localhost:11434', result[:base_url] + end + + def test_call_rejects_relative_base_url + assert_raises(BaseUrlValidator::InvalidBaseURLError) do + ProviderConfig.call( + provider: :openai, + options: { api_key: 'key', base_url: '/v1/chat/completions' } + ) + end + end + + def test_call_opt_in_flag_permits_cleartext_base_url + result = ProviderConfig.call( + provider: :openai, + options: { + api_key: 'key', + base_url: 'http://internal-proxy.example.com', + allow_insecure_base_url: true + } + ) + + assert_equal 'http://internal-proxy.example.com', result[:base_url] + end + + def test_call_validates_azure_endpoint_as_transport_url + assert_raises(BaseUrlValidator::InvalidBaseURLError) do + ProviderConfig.call( + provider: :azure, + options: { api_key: 'key', endpoint: 'http://evil.example.com' } + ) + end + end end end end