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

.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:

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.
+

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'

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

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]

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

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