diff --git a/app/jobs/agent_bots/webhook_job.rb b/app/jobs/agent_bots/webhook_job.rb index 4ba67c9bf..7e0041c20 100644 --- a/app/jobs/agent_bots/webhook_job.rb +++ b/app/jobs/agent_bots/webhook_job.rb @@ -1,6 +1,6 @@ class AgentBots::WebhookJob < WebhookJob queue_as :high - retry_on RestClient::TooManyRequests, RestClient::InternalServerError, wait: 3.seconds, attempts: 3 do |job, error| + retry_on Webhooks::Trigger::RetryableError, wait: 3.seconds, attempts: 3 do |job, error| url, payload, webhook_type = job.arguments kwargs = job.arguments.last.is_a?(Hash) ? job.arguments.last : {} Webhooks::Trigger.new(url, payload, webhook_type || :agent_bot_webhook, secret: kwargs[:secret], @@ -9,7 +9,7 @@ class AgentBots::WebhookJob < WebhookJob def perform(url, payload, webhook_type = :agent_bot_webhook, secret: nil, delivery_id: nil) super(url, payload, webhook_type, secret: secret, delivery_id: delivery_id) - rescue RestClient::TooManyRequests, RestClient::InternalServerError => e + rescue Webhooks::Trigger::RetryableError => e Rails.logger.warn("[AgentBots::WebhookJob] attempt #{executions} failed #{e.class.name} payload=#{payload.to_json}") raise end diff --git a/lib/safe_fetch.rb b/lib/safe_fetch.rb index 2264b2850..d664dcf6a 100644 --- a/lib/safe_fetch.rb +++ b/lib/safe_fetch.rb @@ -2,6 +2,8 @@ require 'ssrf_filter' module SafeFetch DEFAULT_ALLOWED_CONTENT_TYPE_PREFIXES = %w[image/ video/].freeze + DEFAULT_ALLOWED_CONTENT_TYPES = [].freeze + DEFAULT_SENSITIVE_HEADERS = %w[authorization cookie proxy-authorization].freeze DEFAULT_OPEN_TIMEOUT = 2 DEFAULT_READ_TIMEOUT = 20 DEFAULT_MAX_BYTES_FALLBACK_MB = 40 @@ -19,106 +21,22 @@ module SafeFetch class HttpError < Error; end class FileTooLargeError < Error; end class UnsupportedContentTypeError < Error; end + class UnsupportedMethodError < Error; end +end - def self.fetch(url, - max_bytes: nil, - allowed_content_type_prefixes: DEFAULT_ALLOWED_CONTENT_TYPE_PREFIXES, - allowed_content_types: []) +require_relative 'safe_fetch/request_options' +require_relative 'safe_fetch/fetcher' + +module SafeFetch + def self.fetch(url, **, &) raise ArgumentError, 'block required' unless block_given? - effective_max_bytes = max_bytes || default_max_bytes - filename = filename_for(parse_and_validate_url!(url)) - tempfile = Tempfile.new('chatwoot-safe-fetch', binmode: true) - response = fetch_response(url, tempfile, effective_max_bytes, allowed_content_type_prefixes, allowed_content_types) - yield build_result(tempfile, filename, response) + Fetcher.new(RequestOptions.new(url: url, **)).fetch(&) rescue SsrfFilter::InvalidUriScheme, URI::InvalidURIError => e raise InvalidUrlError, e.message rescue SsrfFilter::Error, Resolv::ResolvError => e raise UnsafeUrlError, e.message rescue Net::OpenTimeout, Net::ReadTimeout, SocketError, OpenSSL::SSL::SSLError => e raise FetchError, e.message - ensure - tempfile&.close! - end - - class << self - private - - def fetch_response(url, tempfile, max_bytes, allowed_content_type_prefixes, allowed_content_types) - stream_to_tempfile(url, tempfile, max_bytes, allowed_content_type_prefixes, allowed_content_types) - end - - def stream_to_tempfile(url, tempfile, max_bytes, allowed_content_type_prefixes, allowed_content_types) - response = nil - bytes_written = 0 - - SsrfFilter.get( - url, - request_proc: ->(request) { apply_url_basic_auth(request) }, - http_options: { open_timeout: DEFAULT_OPEN_TIMEOUT, read_timeout: DEFAULT_READ_TIMEOUT } - ) do |res| - response = res - next unless res.is_a?(Net::HTTPSuccess) - - unless allowed_content_type?(res['content-type'], allowed_content_type_prefixes, allowed_content_types) - raise UnsupportedContentTypeError, "content-type not allowed: #{res['content-type']}" - end - - res.read_body do |chunk| - bytes_written += chunk.bytesize - raise FileTooLargeError, "exceeded #{max_bytes} bytes" if bytes_written > max_bytes - - tempfile.write(chunk) - end - end - - response - end - - def filename_for(uri) - File.basename(uri.path).presence || "download-#{Time.current.to_i}-#{SecureRandom.hex(4)}" - end - - def build_result(tempfile, filename, response) - raise HttpError, "#{response.code} #{response.message}" unless response.is_a?(Net::HTTPSuccess) - - tempfile.rewind - content_type = normalized_content_type(response['content-type']) - Result.new(tempfile: tempfile, filename: filename, content_type: content_type) - end - - def default_max_bytes - limit_mb = GlobalConfigService.load('MAXIMUM_FILE_UPLOAD_SIZE', DEFAULT_MAX_BYTES_FALLBACK_MB).to_i - limit_mb = DEFAULT_MAX_BYTES_FALLBACK_MB if limit_mb <= 0 - limit_mb.megabytes - end - - def parse_and_validate_url!(url) - uri = URI.parse(url) - raise InvalidUrlError, 'scheme must be http or https' unless uri.is_a?(URI::HTTP) || uri.is_a?(URI::HTTPS) - raise InvalidUrlError, 'missing host' if uri.host.blank? - - uri - end - - def allowed_content_type?(value, prefixes, content_types) - mime = normalized_content_type(value) - return false if mime.blank? - - prefixes.any? { |prefix| mime.start_with?(prefix) } || content_types.include?(mime) - end - - def normalized_content_type(value) - value.to_s.split(';').first&.strip&.downcase - end - - def apply_url_basic_auth(request) - uri = request.uri - return if uri.user.blank? - - username = URI.decode_uri_component(uri.user) - password = URI.decode_uri_component(uri.password.to_s) - request.basic_auth(username, password) - end end end diff --git a/lib/safe_fetch/fetcher.rb b/lib/safe_fetch/fetcher.rb new file mode 100644 index 000000000..fa3c01f55 --- /dev/null +++ b/lib/safe_fetch/fetcher.rb @@ -0,0 +1,75 @@ +class SafeFetch::Fetcher + def initialize(options) + @options = options + end + + def fetch + with_tempfile do |tempfile| + response = stream_response(tempfile) + raise SafeFetch::HttpError, "#{response.code} #{response.message}" unless response.is_a?(Net::HTTPSuccess) + + tempfile.rewind + yield SafeFetch::Result.new( + tempfile: tempfile, + filename: options.filename, + content_type: normalized_content_type(response['content-type']) + ) + end + end + + private + + attr_reader :options + + def with_tempfile + tempfile = Tempfile.new('chatwoot-safe-fetch', binmode: true) + yield tempfile + ensure + tempfile&.close! + end + + def stream_response(tempfile) + response = nil + bytes_written = 0 + + SsrfFilter.public_send(options.method, options.url, **options.request_options) do |res| + response = res + next unless res.is_a?(Net::HTTPSuccess) + + validate_content_type!(res['content-type']) + bytes_written = write_response_body(res, tempfile, bytes_written) + end + + response + end + + def validate_content_type!(content_type) + return unless options.validate_content_type? + return if allowed_content_type?(content_type) + + raise SafeFetch::UnsupportedContentTypeError, "content-type not allowed: #{content_type}" + end + + def write_response_body(response, tempfile, bytes_written) + response.read_body do |chunk| + bytes_written += chunk.bytesize + raise SafeFetch::FileTooLargeError, "exceeded #{options.effective_max_bytes} bytes" if bytes_written > options.effective_max_bytes + + tempfile.write(chunk) + end + + bytes_written + end + + def allowed_content_type?(value) + mime = normalized_content_type(value) + return false if mime.blank? + + options.allowed_content_type_prefixes.any? { |prefix| mime.start_with?(prefix) } || + options.allowed_content_types.include?(mime) + end + + def normalized_content_type(value) + value.to_s.split(';').first&.strip&.downcase + end +end diff --git a/lib/safe_fetch/request_options.rb b/lib/safe_fetch/request_options.rb new file mode 100644 index 000000000..72969a76d --- /dev/null +++ b/lib/safe_fetch/request_options.rb @@ -0,0 +1,116 @@ +class SafeFetch::RequestOptions + DEFAULTS = { + method: :get, + body: nil, + max_bytes: nil, + open_timeout: SafeFetch::DEFAULT_OPEN_TIMEOUT, + read_timeout: SafeFetch::DEFAULT_READ_TIMEOUT, + headers: nil, + http_basic_authentication: nil, + allowed_content_type_prefixes: SafeFetch::DEFAULT_ALLOWED_CONTENT_TYPE_PREFIXES, + allowed_content_types: SafeFetch::DEFAULT_ALLOWED_CONTENT_TYPES, + validate_content_type: true + }.freeze + + attr_reader :allowed_content_type_prefixes, :allowed_content_types, :body, :headers, + :http_basic_authentication, :method, :open_timeout, :read_timeout, :uri, :url + + def initialize(url:, **options) + config = DEFAULTS.merge(options) + @url = url + @uri = parse_and_validate_url!(url) + @method = normalize_method(config[:method]) + @body = config[:body] + @max_bytes = config[:max_bytes] + @open_timeout = config[:open_timeout] + @read_timeout = config[:read_timeout] + @headers = normalize_headers(config[: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]) + @validate_content_type = config[:validate_content_type] + end + + def effective_max_bytes + @effective_max_bytes ||= @max_bytes || default_max_bytes + end + + def filename + @filename ||= File.basename(uri.path).presence || "download-#{Time.current.to_i}-#{SecureRandom.hex(4)}" + end + + def request_options + { + headers: headers, + body: body, + request_proc: request_proc, + sensitive_headers: sensitive_headers, + http_options: { open_timeout: open_timeout, read_timeout: read_timeout } + } + end + + def validate_content_type? + @validate_content_type + end + + private + + def default_max_bytes + limit_mb = GlobalConfigService.load('MAXIMUM_FILE_UPLOAD_SIZE', SafeFetch::DEFAULT_MAX_BYTES_FALLBACK_MB).to_i + limit_mb = SafeFetch::DEFAULT_MAX_BYTES_FALLBACK_MB if limit_mb <= 0 + limit_mb.megabytes + end + + def parse_and_validate_url!(value) + parsed_uri = URI.parse(value) + raise SafeFetch::InvalidUrlError, 'scheme must be http or https' unless parsed_uri.is_a?(URI::HTTP) || parsed_uri.is_a?(URI::HTTPS) + raise SafeFetch::InvalidUrlError, 'missing host' if parsed_uri.host.blank? + + parsed_uri + end + + def normalize_method(value) + http_method = value.to_s.downcase.to_sym + return http_method if SsrfFilter::VERB_MAP.key?(http_method) + + raise SafeFetch::UnsupportedMethodError, "unsupported method: #{value}" + end + + def normalize_headers(value) + value&.to_h + end + + def request_proc + proc do |request| + credentials = http_basic_authentication.presence || basic_authentication_for(request.uri) + request.basic_auth(*credentials) if credentials.present? + 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 + + def original_uri_basic_authentication(request_uri) + return unless same_origin?(request_uri, uri) + + uri_basic_authentication(uri) + end + + def same_origin?(request_uri, other_uri) + request_uri.scheme == other_uri.scheme && request_uri.hostname == other_uri.hostname && request_uri.port == other_uri.port + end + + def uri_basic_authentication(value) + return if value.user.blank? + + [ + URI.decode_uri_component(value.user), + URI.decode_uri_component(value.password.to_s) + ] + end +end diff --git a/lib/webhooks/trigger.rb b/lib/webhooks/trigger.rb index 7cb15c836..57d96f51d 100644 --- a/lib/webhooks/trigger.rb +++ b/lib/webhooks/trigger.rb @@ -1,5 +1,15 @@ class Webhooks::Trigger SUPPORTED_ERROR_HANDLE_EVENTS = %w[message_created message_updated].freeze + RETRYABLE_AGENT_BOT_STATUSES = [429, 500].freeze + + class RetryableError < StandardError + attr_reader :status + + def initialize(status:, message:) + @status = status + super(message) + end + end def initialize(url, payload, webhook_type, secret: nil, delivery_id: nil) @url = url @@ -15,11 +25,9 @@ class Webhooks::Trigger def execute perform_request - rescue RestClient::TooManyRequests, RestClient::InternalServerError => e - raise if @webhook_type == :agent_bot_webhook - - handle_failure(e) rescue StandardError => e + raise RetryableError.new(status: http_status(e), message: e.message) if retryable_agent_bot_error?(e) + handle_failure(e) end @@ -32,17 +40,19 @@ class Webhooks::Trigger def perform_request body = @payload.to_json - RestClient::Request.execute( + SafeFetch.fetch( + @url, method: :post, - url: @url, - payload: body, + body: body, headers: request_headers(body), - timeout: webhook_timeout - ) + open_timeout: webhook_timeout, + read_timeout: webhook_timeout, + validate_content_type: false + ) { |_response| nil } end def request_headers(body) - headers = { content_type: :json, accept: :json } + headers = { 'Content-Type' => 'application/json', 'Accept' => 'application/json' } headers['X-Chatwoot-Delivery'] = @delivery_id if @delivery_id.present? if @secret.present? ts = Time.now.to_i.to_s @@ -111,4 +121,14 @@ class Webhooks::Trigger timeout&.positive? ? timeout : 5 end + + def retryable_agent_bot_error?(error) + @webhook_type == :agent_bot_webhook && RETRYABLE_AGENT_BOT_STATUSES.include?(http_status(error)) + end + + def http_status(error) + return unless error.is_a?(SafeFetch::HttpError) + + error.message.to_s[/\A(\d{3})\b/, 1]&.to_i + end end diff --git a/spec/jobs/agent_bots/webhook_job_spec.rb b/spec/jobs/agent_bots/webhook_job_spec.rb index c14c46cb3..a8d026cd3 100644 --- a/spec/jobs/agent_bots/webhook_job_spec.rb +++ b/spec/jobs/agent_bots/webhook_job_spec.rb @@ -8,7 +8,7 @@ RSpec.describe AgentBots::WebhookJob do let(:url) { 'https://test.com' } let(:payload) { { name: 'test' } } let(:webhook_type) { :agent_bot_webhook } - let(:retryable_error) { RestClient::InternalServerError.new(nil, 500) } + let(:retryable_error) { Webhooks::Trigger::RetryableError.new(status: 500, message: '500 Internal Server Error') } before do ActiveJob::Base.queue_adapter = :test @@ -33,7 +33,7 @@ RSpec.describe AgentBots::WebhookJob do it 'configures retry handlers for 429 and 500 errors' do handlers = described_class.rescue_handlers.map(&:first) - expect(handlers).to include('RestClient::TooManyRequests', 'RestClient::InternalServerError') + expect(handlers).to include('Webhooks::Trigger::RetryableError') end it 'retries 3 times and handles failure after retries are exhausted' do @@ -43,7 +43,7 @@ RSpec.describe AgentBots::WebhookJob do allow(Rails.logger).to receive(:warn) expect(Webhooks::Trigger).to receive(:execute).exactly(3).times - expect(trigger_instance).to receive(:handle_failure).with(instance_of(RestClient::InternalServerError)).once + expect(trigger_instance).to receive(:handle_failure).with(instance_of(Webhooks::Trigger::RetryableError)).once expect(Rails.logger).to receive(:warn).with(/AgentBots::WebhookJob/).exactly(3).times perform_enqueued_jobs { job } diff --git a/spec/lib/safe_fetch_spec.rb b/spec/lib/safe_fetch_spec.rb index e2c513587..83f2bdde5 100644 --- a/spec/lib/safe_fetch_spec.rb +++ b/spec/lib/safe_fetch_spec.rb @@ -80,6 +80,66 @@ RSpec.describe SafeFetch do expect(result.content_type).to eq('image/png') end end + + it 'preserves embedded credentials after a same-origin redirect removes userinfo' do + authenticated_url = 'http://user:pass@example.com/protected.png' + initial_url = 'http://example.com/protected.png' + redirect_url = 'http://example.com/public.png' + redirected_headers = nil + + stub_request(:get, initial_url) + .with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNz' }) + .to_return( + status: 302, + headers: { 'Location' => '/public.png' } + ) + stub_request(:get, redirect_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' } + ) + + described_class.fetch(authenticated_url) do |result| + expect(result.content_type).to eq('image/png') + end + + expect(redirected_headers).to include('authorization' => 'Basic dXNlcjpwYXNz') + end + + it 'strips embedded credentials on cross-origin redirects' do + authenticated_url = 'http://user:pass@example.com/protected.png' + initial_url = 'http://example.com/protected.png' + redirect_url = 'https://example.com/public.png' + redirected_headers = nil + + stub_request(:get, initial_url) + .with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNz' }) + .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: File.new(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'image/png' } + ) + + described_class.fetch(authenticated_url) do |result| + expect(result.content_type).to eq('image/png') + end + + expect(redirected_headers).not_to include('authorization') + end end context 'with URL validation' do @@ -223,6 +283,75 @@ RSpec.describe SafeFetch do end end + context 'with custom request options' do + let(:post_body) { { hello: 'world' }.to_json } + let(:headers) do + { + 'Authorization' => 'Bearer test-token', + 'Content-Type' => 'application/json' + } + end + + it 'supports POST requests with custom headers when content-type validation is disabled' do + stub_request(:post, url) + .with(body: post_body, headers: headers) + .to_return(status: 200, body: '', headers: {}) + + expect do + described_class.fetch( + url, + method: :post, + body: post_body, + headers: headers, + validate_content_type: false + ) { nil } + end.not_to raise_error + end + + it 'preserves non-credential headers on cross-origin redirects' do + redirect_url = 'https://example.com/image.png' + redirected_headers = nil + headers = { + 'Authorization' => 'Bearer test-token', + 'Cookie' => 'session=test', + 'Content-Type' => 'application/json', + 'X-Chatwoot-Delivery' => 'test-uuid', + 'X-Chatwoot-Signature' => 'sha256=test-signature' + } + + stub_request(:post, url).to_return( + status: 307, + headers: { 'Location' => redirect_url } + ) + stub_request(:post, redirect_url) + .with do |request| + redirected_headers = request.headers.transform_keys(&:downcase) + true + end + .to_return(status: 200, body: '', headers: {}) + + described_class.fetch( + url, + method: :post, + body: post_body, + headers: headers, + validate_content_type: false + ) { nil } + + expect(redirected_headers).to include( + 'content-type' => 'application/json', + 'x-chatwoot-delivery' => 'test-uuid', + 'x-chatwoot-signature' => 'sha256=test-signature' + ) + expect(redirected_headers).not_to include('authorization', 'cookie') + end + + it 'raises UnsupportedMethodError for unsupported HTTP methods' do + expect { described_class.fetch(url, method: :options) { nil } } + .to raise_error(described_class::UnsupportedMethodError) + end + end + context 'with body size cap' do it 'honours a custom max_bytes argument' do stub_request(:get, url).to_return( diff --git a/spec/lib/webhooks/trigger_spec.rb b/spec/lib/webhooks/trigger_spec.rb index 90d1ce7f8..1c5ebc16a 100644 --- a/spec/lib/webhooks/trigger_spec.rb +++ b/spec/lib/webhooks/trigger_spec.rb @@ -11,10 +11,13 @@ describe Webhooks::Trigger do let!(:message) { create(:message, account: account, inbox: inbox, conversation: conversation) } let(:webhook_type) { :api_inbox_webhook } - let!(:url) { 'https://test.com' } + let(:url) { 'https://test.com' } + let(:payload) { { hello: :hello } } + let(:fetch_result) { instance_double(SafeFetch::Result) } let(:agent_bot_error_content) { I18n.t('conversations.activity.agent_bot.error_moved_to_open') } let(:default_timeout) { 5 } let(:webhook_timeout) { default_timeout } + let(:base_headers) { { 'Content-Type' => 'application/json', 'Accept' => 'application/json' } } before do ActiveJob::Base.queue_adapter = :test @@ -30,30 +33,23 @@ describe Webhooks::Trigger do describe '#execute' do it 'triggers webhook' do - payload = { hello: :hello } + expect(SafeFetch).to receive(:fetch).with( + url, + method: :post, + body: payload.to_json, + headers: base_headers, + open_timeout: webhook_timeout, + read_timeout: webhook_timeout, + validate_content_type: false + ).and_yield(fetch_result) - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).once trigger.execute(url, payload, webhook_type) end it 'updates message status if webhook fails for message-created event' do payload = { event: 'message_created', conversation: { id: conversation.id }, id: message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('500 Internal Server Error')) expect { trigger.execute(url, payload, webhook_type) }.to change { message.reload.status }.from('sent').to('failed') end @@ -61,17 +57,18 @@ describe Webhooks::Trigger do it 'updates message status if webhook fails for message-updated event' do payload = { event: 'message_updated', conversation: { id: conversation.id }, id: message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('500 Internal Server Error')) + expect { trigger.execute(url, payload, webhook_type) }.to change { message.reload.status }.from('sent').to('failed') end + it 'treats blocked private webhook URLs as failures' do + payload = { event: 'message_created', conversation: { id: conversation.id }, id: message.id } + + expect { trigger.execute('http://127.0.0.1/webhook', payload, webhook_type) } + .to change { message.reload.status }.from('sent').to('failed') + end + context 'when webhook type is agent bot' do let(:webhook_type) { :agent_bot_webhook } let!(:pending_conversation) { create(:conversation, inbox: inbox, status: :pending, account: account) } @@ -80,16 +77,13 @@ describe Webhooks::Trigger do it 'raises 500 errors for retry and does not reopen conversation immediately' do payload = { event: 'message_created', id: pending_message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::InternalServerError.new(nil, 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('500 Internal Server Error')) - expect { trigger.execute(url, payload, webhook_type) }.to raise_error(RestClient::InternalServerError) + expect { trigger.execute(url, payload, webhook_type) } + .to(raise_error do |error| + expect(error.class.name).to eq('Webhooks::Trigger::RetryableError') + expect(error.status).to eq(500) + end) expect(pending_conversation.reload.status).to eq('pending') expect(Conversations::ActivityMessageJob).not_to have_been_enqueued end @@ -97,16 +91,13 @@ describe Webhooks::Trigger do it 'raises 429 errors for retry and does not reopen conversation immediately' do payload = { event: 'message_created', id: pending_message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::TooManyRequests.new(nil, 429)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('429 Too Many Requests')) - expect { trigger.execute(url, payload, webhook_type) }.to raise_error(RestClient::TooManyRequests) + expect { trigger.execute(url, payload, webhook_type) } + .to(raise_error do |error| + expect(error.class.name).to eq('Webhooks::Trigger::RetryableError') + expect(error.status).to eq(429) + end) expect(pending_conversation.reload.status).to eq('pending') expect(Conversations::ActivityMessageJob).not_to have_been_enqueued end @@ -114,14 +105,7 @@ describe Webhooks::Trigger do it 'reopens conversation and enqueues activity message if pending' do payload = { event: 'message_created', id: pending_message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('404 Not Found')) expect do perform_enqueued_jobs do @@ -139,14 +123,7 @@ describe Webhooks::Trigger do it 'does not change message status or enqueue activity when conversation is not pending' do payload = { event: 'message_created', conversation: { id: conversation.id }, id: message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('404 Not Found')) expect do trigger.execute(url, payload, webhook_type) @@ -160,14 +137,7 @@ describe Webhooks::Trigger do account.update(keep_pending_on_bot_failure: true) payload = { event: 'message_created', id: pending_message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('404 Not Found')) trigger.execute(url, payload, webhook_type) @@ -179,14 +149,8 @@ describe Webhooks::Trigger do account.update(keep_pending_on_bot_failure: false) payload = { event: 'message_created', id: pending_message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('404 Not Found')) + expect do perform_enqueued_jobs do trigger.execute(url, payload, webhook_type) @@ -204,14 +168,7 @@ describe Webhooks::Trigger do it 'handles 500 without raising for non-agent webhooks' do payload = { event: 'message_created', conversation: { id: conversation.id }, id: message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::InternalServerError.new(nil, 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('500 Internal Server Error')) expect { trigger.execute(url, payload, webhook_type) }.not_to raise_error expect(message.reload.status).to eq('failed') @@ -224,20 +181,30 @@ describe Webhooks::Trigger do context 'without secret or delivery_id' do it 'sends only content-type and accept headers' do - expect(RestClient::Request).to receive(:execute).with( - hash_including(headers: { content_type: :json, accept: :json }) - ) + expect(SafeFetch).to receive(:fetch).with( + url, + method: :post, + body: body, + headers: base_headers, + open_timeout: webhook_timeout, + read_timeout: webhook_timeout, + validate_content_type: false + ).and_yield(fetch_result) + trigger.execute(url, payload, webhook_type) end end context 'with delivery_id' do it 'adds X-Chatwoot-Delivery header' do - expect(RestClient::Request).to receive(:execute) do |args| - expect(args[:headers]['X-Chatwoot-Delivery']).to eq('test-uuid') - expect(args[:headers]).not_to have_key('X-Chatwoot-Signature') - expect(args[:headers]).not_to have_key('X-Chatwoot-Timestamp') + expect(SafeFetch).to receive(:fetch) do |received_url, **options, &block| + expect(received_url).to eq(url) + expect(options[:headers]['X-Chatwoot-Delivery']).to eq('test-uuid') + expect(options[:headers]).not_to have_key('X-Chatwoot-Signature') + expect(options[:headers]).not_to have_key('X-Chatwoot-Timestamp') + block.call(fetch_result) end + trigger.execute(url, payload, webhook_type, delivery_id: 'test-uuid') end end @@ -246,38 +213,45 @@ describe Webhooks::Trigger do let(:secret) { 'test-secret' } it 'adds X-Chatwoot-Timestamp header' do - expect(RestClient::Request).to receive(:execute) do |args| - expect(args[:headers]['X-Chatwoot-Timestamp']).to match(/\A\d+\z/) + expect(SafeFetch).to receive(:fetch) do |_received_url, **options, &block| + expect(options[:headers]['X-Chatwoot-Timestamp']).to match(/\A\d+\z/) + block.call(fetch_result) end + trigger.execute(url, payload, webhook_type, secret: secret) end it 'adds X-Chatwoot-Signature header with correct HMAC' do - expect(RestClient::Request).to receive(:execute) do |args| - ts = args[:headers]['X-Chatwoot-Timestamp'] + expect(SafeFetch).to receive(:fetch) do |_received_url, **options, &block| + ts = options[:headers]['X-Chatwoot-Timestamp'] expected_sig = "sha256=#{OpenSSL::HMAC.hexdigest('SHA256', secret, "#{ts}.#{body}")}" - expect(args[:headers]['X-Chatwoot-Signature']).to eq(expected_sig) + expect(options[:headers]['X-Chatwoot-Signature']).to eq(expected_sig) + block.call(fetch_result) end + trigger.execute(url, payload, webhook_type, secret: secret) end it 'signs timestamp.body not just body' do - expect(RestClient::Request).to receive(:execute) do |args| - args[:headers]['X-Chatwoot-Timestamp'] + expect(SafeFetch).to receive(:fetch) do |_received_url, **options, &block| wrong_sig = "sha256=#{OpenSSL::HMAC.hexdigest('SHA256', secret, body)}" - expect(args[:headers]['X-Chatwoot-Signature']).not_to eq(wrong_sig) + expect(options[:headers]['X-Chatwoot-Signature']).not_to eq(wrong_sig) + block.call(fetch_result) end + trigger.execute(url, payload, webhook_type, secret: secret) end end context 'with both secret and delivery_id' do it 'includes all three security headers' do - expect(RestClient::Request).to receive(:execute) do |args| - expect(args[:headers]['X-Chatwoot-Delivery']).to eq('abc-123') - expect(args[:headers]['X-Chatwoot-Timestamp']).to be_present - expect(args[:headers]['X-Chatwoot-Signature']).to start_with('sha256=') + expect(SafeFetch).to receive(:fetch) do |_received_url, **options, &block| + expect(options[:headers]['X-Chatwoot-Delivery']).to eq('abc-123') + expect(options[:headers]['X-Chatwoot-Timestamp']).to be_present + expect(options[:headers]['X-Chatwoot-Signature']).to start_with('sha256=') + block.call(fetch_result) end + trigger.execute(url, payload, webhook_type, secret: 'mysecret', delivery_id: 'abc-123') end end @@ -286,14 +260,7 @@ describe Webhooks::Trigger do it 'does not update message status if webhook fails for other events' do payload = { event: 'conversation_created', conversation: { id: conversation.id }, id: message.id } - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: webhook_timeout - ).and_raise(RestClient::ExceptionWithResponse.new('error', 500)).once + expect(SafeFetch).to receive(:fetch).and_raise(SafeFetch::HttpError.new('500 Internal Server Error')) expect { trigger.execute(url, payload, webhook_type) }.not_to(change { message.reload.status }) end @@ -302,16 +269,15 @@ describe Webhooks::Trigger do let(:webhook_timeout) { nil } it 'falls back to default timeout' do - payload = { hello: :hello } - - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: default_timeout - ).once + expect(SafeFetch).to receive(:fetch).with( + url, + method: :post, + body: payload.to_json, + headers: base_headers, + open_timeout: default_timeout, + read_timeout: default_timeout, + validate_content_type: false + ).and_yield(fetch_result) trigger.execute(url, payload, webhook_type) end @@ -321,16 +287,15 @@ describe Webhooks::Trigger do let(:webhook_timeout) { -1 } it 'falls back to default timeout' do - payload = { hello: :hello } - - expect(RestClient::Request).to receive(:execute) - .with( - method: :post, - url: url, - payload: payload.to_json, - headers: { content_type: :json, accept: :json }, - timeout: default_timeout - ).once + expect(SafeFetch).to receive(:fetch).with( + url, + method: :post, + body: payload.to_json, + headers: base_headers, + open_timeout: default_timeout, + read_timeout: default_timeout, + validate_content_type: false + ).and_yield(fetch_result) trigger.execute(url, payload, webhook_type) end