diff --git a/enterprise/app/models/concerns/toolable.rb b/enterprise/app/models/concerns/toolable.rb index 828cd50c5..66bdf5d66 100644 --- a/enterprise/app/models/concerns/toolable.rb +++ b/enterprise/app/models/concerns/toolable.rb @@ -55,11 +55,7 @@ module Concerns::Toolable when 'bearer' { 'Authorization' => "Bearer #{auth_config['token']}" } when 'api_key' - if auth_config['location'] == 'header' - { auth_config['name'] => auth_config['key'] } - else - {} - end + { auth_config['name'] => auth_config['key'] } else {} end diff --git a/enterprise/lib/captain/tools/http_tool.rb b/enterprise/lib/captain/tools/http_tool.rb index 18ebcf53a..70576fb21 100644 --- a/enterprise/lib/captain/tools/http_tool.rb +++ b/enterprise/lib/captain/tools/http_tool.rb @@ -32,13 +32,15 @@ class Captain::Tools::HttpTool < Agents::Tool # fetching (resolution, timeouts, response size limits, and redirect handling). def execute_http_request(url, body, tool_context) json_body = body if @custom_tool.http_method == 'POST' + auth_headers = @custom_tool.build_auth_headers response_body = +'' SafeFetch.fetch( url, method: @custom_tool.http_method == 'POST' ? :post : :get, body: json_body, - headers: request_headers(tool_context, json_body), + headers: request_headers(tool_context, json_body, auth_headers), + sensitive_headers: auth_headers.keys, http_basic_authentication: @custom_tool.build_basic_auth_credentials, max_bytes: MAX_RESPONSE_SIZE, validate_content_type: false @@ -46,8 +48,8 @@ class Captain::Tools::HttpTool < Agents::Tool response_body end - def request_headers(tool_context, json_body) - headers = @custom_tool.build_auth_headers + def request_headers(tool_context, json_body, auth_headers) + headers = auth_headers.dup headers.merge!(@custom_tool.build_metadata_headers(tool_context&.state || {})) headers['Content-Type'] = 'application/json' if json_body.present? headers diff --git a/lib/safe_fetch/request_options.rb b/lib/safe_fetch/request_options.rb index 6d11ebd19..54f5a559c 100644 --- a/lib/safe_fetch/request_options.rb +++ b/lib/safe_fetch/request_options.rb @@ -6,6 +6,7 @@ class SafeFetch::RequestOptions open_timeout: SafeFetch::DEFAULT_OPEN_TIMEOUT, read_timeout: SafeFetch::DEFAULT_READ_TIMEOUT, headers: nil, + sensitive_headers: [], http_basic_authentication: nil, allowed_content_type_prefixes: SafeFetch::DEFAULT_ALLOWED_CONTENT_TYPE_PREFIXES, allowed_content_types: SafeFetch::DEFAULT_ALLOWED_CONTENT_TYPES, @@ -13,7 +14,7 @@ class SafeFetch::RequestOptions }.freeze attr_reader :allowed_content_type_prefixes, :allowed_content_types, :body, :headers, - :http_basic_authentication, :method, :open_timeout, :read_timeout, :uri, :url + :http_basic_authentication, :method, :open_timeout, :read_timeout, :sensitive_headers, :uri, :url def initialize(url:, **options) config = DEFAULTS.merge(options) @@ -25,6 +26,7 @@ class SafeFetch::RequestOptions @open_timeout = config[:open_timeout] @read_timeout = config[:read_timeout] @headers = normalize_headers(config[:headers]) + @sensitive_headers = normalize_sensitive_headers(config[:sensitive_headers]) @http_basic_authentication = config[:http_basic_authentication] @allowed_content_type_prefixes = Array(config[:allowed_content_type_prefixes]) @allowed_content_types = Array(config[:allowed_content_types]) @@ -84,6 +86,10 @@ class SafeFetch::RequestOptions value&.to_h end + def normalize_sensitive_headers(value) + (SafeFetch::DEFAULT_SENSITIVE_HEADERS + Array(value)).map { |header| header.to_s.downcase }.uniq + end + def request_proc proc do |request| credentials = http_basic_authentication.presence || basic_authentication_for(request.uri) @@ -91,10 +97,6 @@ class SafeFetch::RequestOptions end end - def sensitive_headers - SafeFetch::DEFAULT_SENSITIVE_HEADERS - end - def basic_authentication_for(request_uri) uri_basic_authentication(request_uri) || original_uri_basic_authentication(request_uri) end diff --git a/spec/enterprise/lib/captain/tools/http_tool_spec.rb b/spec/enterprise/lib/captain/tools/http_tool_spec.rb index e05308a7d..3ee19b530 100644 --- a/spec/enterprise/lib/captain/tools/http_tool_spec.rb +++ b/spec/enterprise/lib/captain/tools/http_tool_spec.rb @@ -129,7 +129,7 @@ RSpec.describe Captain::Tools::HttpTool, type: :model do before do custom_tool.update!( auth_type: 'api_key', - auth_config: { 'key' => 'api_key_123', 'location' => 'header', 'name' => 'X-API-Key' }, + auth_config: { 'key' => 'api_key_123', 'name' => 'X-API-Key' }, endpoint_url: 'https://example.com/data', response_template: nil ) @@ -145,6 +145,22 @@ RSpec.describe Captain::Tools::HttpTool, type: :model do expect(WebMock).to have_requested(:get, 'https://example.com/data') .with(headers: { 'X-API-Key' => 'api_key_123' }) end + + it 'strips the API key header on cross-origin redirects' do + redirect_url = 'http://example.com/data' + redirected_headers = nil + stub_request(:get, 'https://example.com/data').to_return(status: 302, headers: { 'Location' => redirect_url }) + stub_request(:get, redirect_url) + .with do |request| + redirected_headers = request.headers.transform_keys(&:downcase) + true + end + .to_return(status: 200, body: '{"authenticated": false}') + + tool.perform(tool_context) + + expect(redirected_headers).not_to include('x-api-key') + end end context 'with response template' do diff --git a/spec/enterprise/models/captain/custom_tool_spec.rb b/spec/enterprise/models/captain/custom_tool_spec.rb index 60b66778f..ab23b8aa4 100644 --- a/spec/enterprise/models/captain/custom_tool_spec.rb +++ b/spec/enterprise/models/captain/custom_tool_spec.rb @@ -201,7 +201,7 @@ RSpec.describe Captain::CustomTool, type: :model do expect(tool.auth_type).to eq('api_key') expect(tool.auth_config['key']).to eq('test_api_key') - expect(tool.auth_config['location']).to eq('header') + expect(tool.auth_config['name']).to eq('X-API-Key') end end @@ -259,19 +259,12 @@ RSpec.describe Captain::CustomTool, type: :model do expect(tool.build_auth_headers).to eq({ 'Authorization' => 'Bearer test_bearer_token_123' }) end - it 'returns API key header when location is header' do + it 'returns API key header' do tool = create(:captain_custom_tool, :with_api_key, account: account) expect(tool.build_auth_headers).to eq({ 'X-API-Key' => 'test_api_key' }) end - it 'returns empty hash for API key when location is not header' do - tool = create(:captain_custom_tool, account: account, auth_type: 'api_key', - auth_config: { key: 'test_key', location: 'query', name: 'api_key' }) - - expect(tool.build_auth_headers).to eq({}) - end - it 'returns empty hash for basic auth' do tool = create(:captain_custom_tool, :with_basic_auth, account: account) diff --git a/spec/factories/captain/custom_tool.rb b/spec/factories/captain/custom_tool.rb index 2bfcbf360..d001755b9 100644 --- a/spec/factories/captain/custom_tool.rb +++ b/spec/factories/captain/custom_tool.rb @@ -27,7 +27,7 @@ FactoryBot.define do trait :with_api_key do auth_type { 'api_key' } - auth_config { { key: 'test_api_key', location: 'header', name: 'X-API-Key' } } + auth_config { { key: 'test_api_key', name: 'X-API-Key' } } end trait :with_templates do diff --git a/spec/lib/safe_fetch_spec.rb b/spec/lib/safe_fetch_spec.rb index a124be774..1593a7c52 100644 --- a/spec/lib/safe_fetch_spec.rb +++ b/spec/lib/safe_fetch_spec.rb @@ -249,6 +249,34 @@ RSpec.describe SafeFetch do expect { described_class.fetch(redirect_url) { nil } }.not_to raise_error end end + + it 'strips caller-provided sensitive headers on private network cross-origin redirects' do + redirect_url = 'http://example.com/redirect.png' + private_url = 'http://private.example.com/image.png' + redirected_headers = nil + allow(Resolv).to receive(:getaddresses).with('private.example.com').and_return(['10.0.0.5']) + stub_request(:get, redirect_url).to_return(status: 302, headers: { 'Location' => private_url }) + stub_request(:get, private_url) + .with do |request| + redirected_headers = request.headers.transform_keys(&:downcase) + true + end + .to_return( + status: 200, + body: File.new(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'image/png' } + ) + + with_modified_env('SAFE_FETCH_ALLOW_PRIVATE_NETWORK' => 'true') do + described_class.fetch( + redirect_url, + headers: { 'X-API-Key' => 'secret-key' }, + sensitive_headers: ['X-API-Key'] + ) { nil } + end + + expect(redirected_headers).not_to include('x-api-key') + end end context 'with content-type allowlist' do @@ -400,6 +428,47 @@ RSpec.describe SafeFetch do expect(redirected_headers).not_to include('authorization', 'cookie') end + it 'strips caller-provided sensitive headers on cross-origin redirects' do + redirect_url = 'https://example.com/image.png' + redirected_headers = nil + headers = { 'X-API-Key' => 'secret-key' } + + stub_request(:get, url).to_return(status: 302, headers: { 'Location' => redirect_url }) + stub_request(:get, redirect_url) + .with do |request| + redirected_headers = request.headers.transform_keys(&:downcase) + true + end + .to_return(status: 200, body: '', headers: {}) + + described_class.fetch( + url, + headers: headers, + sensitive_headers: ['X-API-Key'], + validate_content_type: false + ) { nil } + + expect(redirected_headers).not_to include('x-api-key') + end + + it 'preserves caller-provided sensitive headers on same-origin redirects' do + redirect_url = 'http://example.com/redirected.png' + + stub_request(:get, url).to_return(status: 302, headers: { 'Location' => '/redirected.png' }) + stub_request(:get, redirect_url) + .with(headers: { 'X-API-Key' => 'secret-key' }) + .to_return(status: 200, body: '', headers: {}) + + described_class.fetch( + url, + headers: { 'X-API-Key' => 'secret-key' }, + sensitive_headers: ['X-API-Key'], + validate_content_type: false + ) { nil } + + expect(WebMock).to have_requested(:get, redirect_url).with(headers: { 'X-API-Key' => 'secret-key' }) + end + it 'raises UnsupportedMethodError for unsupported HTTP methods' do expect { described_class.fetch(url, method: :options) { nil } }.to raise_error do |error| expect(error.class.name).to eq('SafeFetch::UnsupportedMethodError')