fix(captain): send custom tool API key headers (#15008)
Captain custom tools configured with API key authentication now send the configured key in the requested HTTP header. Existing tools begin working without needing to be recreated or reconfigured, while credentials remain protected across redirects. ## How to reproduce 1. Create a Captain custom tool using API Key authentication. 2. Configure `X-API-Key` as the header name and save the tool. 3. Invoke the tool and inspect the incoming request. 4. Before this change, the API key header is absent; after this change, the configured endpoint receives it. ## What changed The UI persists API key authentication as `name` and `key`, but the request builder also required an unused `location: header` property. The request builder now treats API key authentication as header-based, matching the only mode exposed by the UI. Custom authentication headers are also registered as sensitive with `SafeFetch`. They are retained for the configured endpoint and same-origin redirects, but stripped when a redirect crosses origins to prevent credential leakage. Factory, request, and redirect specs cover the real UI payload and both public and private-network fetch paths.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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')
|
||||
|
||||
Reference in New Issue
Block a user