From 661608c0b1a393c93cb1225dfb32233461e609fc Mon Sep 17 00:00:00 2001 From: Sony Mathew Date: Fri, 24 Apr 2026 18:59:45 +0530 Subject: [PATCH] fix: [CW-6931] Harden external downloads against SSRF [avatar from url job] (#14153) This routes external downloads used by avatar sync through SafeFetch. It closes the SSRF exposure from raw Down.download paths, preserves provider-specific auth and header flows, and adds regression coverage for blocked internal URLs plus authenticated downloads. Fixes # (issue): [CW-6931](https://linear.app/chatwoot/issue/CW-6931/avatarwidget-url-ssrf-downdownload-unprotected-unauth) --- app/jobs/avatar/avatar_from_url_job.rb | 59 +++++-- app/models/concerns/avatarable.rb | 5 +- lib/safe_fetch.rb | 56 +++++-- spec/jobs/avatar/avatar_from_url_job_spec.rb | 154 +++++++++++++++---- spec/lib/safe_fetch_spec.rb | 58 ++++++- spec/models/contact_spec.rb | 7 + 6 files changed, 274 insertions(+), 65 deletions(-) diff --git a/app/jobs/avatar/avatar_from_url_job.rb b/app/jobs/avatar/avatar_from_url_job.rb index 929e76597..49bf25803 100644 --- a/app/jobs/avatar/avatar_from_url_job.rb +++ b/app/jobs/avatar/avatar_from_url_job.rb @@ -9,27 +9,17 @@ class Avatar::AvatarFromUrlJob < ApplicationJob include UrlHelper queue_as :purgable - MAX_DOWNLOAD_SIZE = 15 * 1024 * 1024 + ALLOWED_CONTENT_TYPES = Avatarable::ALLOWED_AVATAR_CONTENT_TYPES + MAX_DOWNLOAD_SIZE = 15.megabytes RATE_LIMIT_WINDOW = 1.minute def perform(avatarable, avatar_url) - return unless avatarable.respond_to?(:avatar) - return unless url_valid?(avatar_url) + return unless syncable_avatar?(avatarable, avatar_url) - return unless should_sync_avatar?(avatarable, avatar_url) - - avatar_file = Down.download(avatar_url, max_size: MAX_DOWNLOAD_SIZE) - raise Down::Error, 'Invalid file' unless valid_file?(avatar_file) - - avatarable.avatar.attach( - io: avatar_file, - filename: avatar_file.original_filename, - content_type: avatar_file.content_type - ) - - rescue Down::NotFound - Rails.logger.info "AvatarFromUrlJob: avatar not found at #{avatar_url}" - rescue Down::Error => e + fetch_and_attach_avatar(avatarable, avatar_url) + rescue SafeFetch::HttpError => e + log_http_error(avatar_url, e) + rescue SafeFetch::Error => e Rails.logger.error "AvatarFromUrlJob error for #{avatar_url}: #{e.class} - #{e.message}" ensure update_avatar_sync_attributes(avatarable, avatar_url) @@ -37,6 +27,41 @@ class Avatar::AvatarFromUrlJob < ApplicationJob private + def syncable_avatar?(avatarable, avatar_url) + avatarable.respond_to?(:avatar) && + url_valid?(avatar_url) && + should_sync_avatar?(avatarable, avatar_url) + end + + def fetch_and_attach_avatar(avatarable, avatar_url) + SafeFetch.fetch( + avatar_url, + max_bytes: MAX_DOWNLOAD_SIZE, + allowed_content_type_prefixes: [], + allowed_content_types: ALLOWED_CONTENT_TYPES + ) do |avatar_file| + attach_avatar(avatarable, avatar_file) + end + end + + def attach_avatar(avatarable, avatar_file) + raise SafeFetch::FetchError, 'Invalid file' unless valid_file?(avatar_file) + + avatarable.avatar.attach( + io: avatar_file.tempfile, + filename: avatar_file.original_filename, + content_type: avatar_file.content_type + ) + end + + def log_http_error(avatar_url, error) + if error.message.start_with?('404') + Rails.logger.info "AvatarFromUrlJob: avatar not found at #{avatar_url}" + else + Rails.logger.error "AvatarFromUrlJob error for #{avatar_url}: #{error.class} - #{error.message}" + end + end + def should_sync_avatar?(avatarable, avatar_url) # Only Contacts are rate-limited and hash-gated. return true unless avatarable.is_a?(Contact) diff --git a/app/models/concerns/avatarable.rb b/app/models/concerns/avatarable.rb index 94ca55037..3057f8d44 100644 --- a/app/models/concerns/avatarable.rb +++ b/app/models/concerns/avatarable.rb @@ -4,6 +4,8 @@ module Avatarable extend ActiveSupport::Concern include Rails.application.routes.url_helpers + ALLOWED_AVATAR_CONTENT_TYPES = %w[image/jpeg image/png image/gif image/webp].freeze + included do has_one_attached :avatar validate :acceptable_avatar, if: -> { avatar.changed? } @@ -30,7 +32,6 @@ module Avatarable errors.add(:avatar, 'is too big') if avatar.byte_size > 15.megabytes - acceptable_types = ['image/jpeg', 'image/png', 'image/gif'].freeze - errors.add(:avatar, 'filetype not supported') unless acceptable_types.include?(avatar.content_type) + errors.add(:avatar, 'filetype not supported') unless ALLOWED_AVATAR_CONTENT_TYPES.include?(avatar.content_type) end end diff --git a/lib/safe_fetch.rb b/lib/safe_fetch.rb index e6635c9c3..2264b2850 100644 --- a/lib/safe_fetch.rb +++ b/lib/safe_fetch.rb @@ -6,7 +6,11 @@ module SafeFetch DEFAULT_READ_TIMEOUT = 20 DEFAULT_MAX_BYTES_FALLBACK_MB = 40 - Result = Data.define(:tempfile, :filename, :content_type) + Result = Data.define(:tempfile, :filename, :content_type) do + def original_filename + filename + end + end class Error < StandardError; end class InvalidUrlError < Error; end @@ -18,19 +22,15 @@ module SafeFetch def self.fetch(url, max_bytes: nil, - allowed_content_type_prefixes: DEFAULT_ALLOWED_CONTENT_TYPE_PREFIXES) + allowed_content_type_prefixes: DEFAULT_ALLOWED_CONTENT_TYPE_PREFIXES, + allowed_content_types: []) raise ArgumentError, 'block required' unless block_given? effective_max_bytes = max_bytes || default_max_bytes - uri = parse_and_validate_url!(url) - filename = filename_for(uri) + filename = filename_for(parse_and_validate_url!(url)) tempfile = Tempfile.new('chatwoot-safe-fetch', binmode: true) - - response = stream_to_tempfile(url, tempfile, effective_max_bytes, allowed_content_type_prefixes) - raise HttpError, "#{response.code} #{response.message}" unless response.is_a?(Net::HTTPSuccess) - - tempfile.rewind - yield Result.new(tempfile: tempfile, filename: filename, content_type: response['content-type']) + response = fetch_response(url, tempfile, effective_max_bytes, allowed_content_type_prefixes, allowed_content_types) + yield build_result(tempfile, filename, response) rescue SsrfFilter::InvalidUriScheme, URI::InvalidURIError => e raise InvalidUrlError, e.message rescue SsrfFilter::Error, Resolv::ResolvError => e @@ -44,18 +44,23 @@ module SafeFetch class << self private - def stream_to_tempfile(url, tempfile, max_bytes, allowed_content_type_prefixes) + 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) + unless allowed_content_type?(res['content-type'], allowed_content_type_prefixes, allowed_content_types) raise UnsupportedContentTypeError, "content-type not allowed: #{res['content-type']}" end @@ -74,6 +79,14 @@ module SafeFetch 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 @@ -88,11 +101,24 @@ module SafeFetch uri end - def allowed_content_type?(value, prefixes) - mime = value.to_s.split(';').first&.strip&.downcase + 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) } + 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/spec/jobs/avatar/avatar_from_url_job_spec.rb b/spec/jobs/avatar/avatar_from_url_job_spec.rb index 8db3769ad..e85c46390 100644 --- a/spec/jobs/avatar/avatar_from_url_job_spec.rb +++ b/spec/jobs/avatar/avatar_from_url_job_spec.rb @@ -1,9 +1,13 @@ require 'rails_helper' RSpec.describe Avatar::AvatarFromUrlJob do - let(:file) { fixture_file_upload(Rails.root.join('spec/assets/avatar.png'), 'image/png') } let(:valid_url) { 'https://example.com/avatar.png' } + before do + allow(Resolv).to receive(:getaddresses).and_call_original + allow(Resolv).to receive(:getaddresses).with('example.com').and_return(['93.184.216.34']) + end + it 'enqueues the job' do contact = create(:contact) expect { described_class.perform_later(contact, 'https://example.com/avatar.png') } @@ -14,7 +18,13 @@ RSpec.describe Avatar::AvatarFromUrlJob do let(:avatarable) { create(:contact) } it 'attaches and updates sync attributes' do - expect(Down).to receive(:download).with(valid_url, max_size: Avatar::AvatarFromUrlJob::MAX_DOWNLOAD_SIZE).and_return(file) + stub_request(:get, valid_url) + .to_return( + status: 200, + body: File.read(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'image/png' } + ) + described_class.perform_now(avatarable, valid_url) avatarable.reload expect(avatarable.avatar).to be_attached @@ -22,10 +32,71 @@ RSpec.describe Avatar::AvatarFromUrlJob do expect(avatarable.additional_attributes['last_avatar_sync_at']).to be_present end + it 'attaches webp avatars and updates sync attributes' do + webp_url = 'https://example.com/avatar.webp' + + stub_request(:get, webp_url) + .to_return( + status: 200, + body: File.read(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'image/webp' } + ) + + described_class.perform_now(avatarable, webp_url) + avatarable.reload + + expect(avatarable.avatar).to be_attached + expect(avatarable.additional_attributes['avatar_url_hash']).to eq(Digest::SHA256.hexdigest(webp_url)) + expect(avatarable.additional_attributes['last_avatar_sync_at']).to be_present + end + + it 'attaches avatars with parameterized content type headers' do + parameterized_url = 'https://example.com/avatar-parameterized.png' + + stub_request(:get, parameterized_url) + .to_return( + status: 200, + body: File.read(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'IMAGE/PNG; charset=binary' } + ) + + described_class.perform_now(avatarable, parameterized_url) + avatarable.reload + + expect(avatarable.avatar).to be_attached + expect(avatarable.avatar.blob.content_type).to eq('image/png') + expect(avatarable.additional_attributes['avatar_url_hash']).to eq(Digest::SHA256.hexdigest(parameterized_url)) + end + + it 'attaches avatars from URLs with embedded basic auth credentials' do + authenticated_url = 'https://user:pass@example.com/avatar-authenticated.png' + + stub_request(:get, 'https://example.com/avatar-authenticated.png') + .with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNz' }) + .to_return( + status: 200, + body: File.read(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'image/png' } + ) + + described_class.perform_now(avatarable, authenticated_url) + avatarable.reload + + expect(avatarable.avatar).to be_attached + expect(avatarable.additional_attributes['avatar_url_hash']).to eq(Digest::SHA256.hexdigest(authenticated_url)) + end + it 'returns early when rate limited' do ts = 30.seconds.ago.iso8601 avatarable.update(additional_attributes: { 'last_avatar_sync_at' => ts }) - expect(Down).not_to receive(:download) + + stub_request(:get, valid_url) + .to_return( + status: 200, + body: File.read(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'image/png' } + ) + described_class.perform_now(avatarable, valid_url) avatarable.reload expect(avatarable.avatar).not_to be_attached @@ -33,21 +104,29 @@ RSpec.describe Avatar::AvatarFromUrlJob do expect(Time.zone.parse(avatarable.additional_attributes['last_avatar_sync_at'])) .to be > Time.zone.parse(ts) expect(avatarable.additional_attributes['avatar_url_hash']).to eq(Digest::SHA256.hexdigest(valid_url)) + expect(WebMock).not_to have_requested(:get, valid_url) end it 'returns early when hash unchanged' do avatarable.update(additional_attributes: { 'avatar_url_hash' => Digest::SHA256.hexdigest(valid_url) }) - expect(Down).not_to receive(:download) + + stub_request(:get, valid_url) + .to_return( + status: 200, + body: File.read(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'image/png' } + ) + described_class.perform_now(avatarable, valid_url) expect(avatarable.avatar).not_to be_attached avatarable.reload expect(avatarable.additional_attributes['last_avatar_sync_at']).to be_present expect(avatarable.additional_attributes['avatar_url_hash']).to eq(Digest::SHA256.hexdigest(valid_url)) + expect(WebMock).not_to have_requested(:get, valid_url) end it 'updates sync attributes even when URL is invalid' do invalid_url = 'invalid_url' - expect(Down).not_to receive(:download) described_class.perform_now(avatarable, invalid_url) avatarable.reload expect(avatarable.avatar).not_to be_attached @@ -56,17 +135,12 @@ RSpec.describe Avatar::AvatarFromUrlJob do end it 'updates sync attributes when file download is valid but content type is unsupported' do - temp_file = Tempfile.new(['invalid', '.xml']) - temp_file.write('content') - temp_file.rewind - - uploaded = ActionDispatch::Http::UploadedFile.new( - tempfile: temp_file, - filename: 'invalid.xml', - type: 'application/xml' - ) - - expect(Down).to receive(:download).with(valid_url, max_size: Avatar::AvatarFromUrlJob::MAX_DOWNLOAD_SIZE).and_return(uploaded) + stub_request(:get, valid_url) + .to_return( + status: 200, + body: 'content', + headers: { 'Content-Type' => 'application/xml' } + ) described_class.perform_now(avatarable, valid_url) avatarable.reload @@ -74,9 +148,19 @@ RSpec.describe Avatar::AvatarFromUrlJob do expect(avatarable.avatar).not_to be_attached expect(avatarable.additional_attributes['last_avatar_sync_at']).to be_present expect(avatarable.additional_attributes['avatar_url_hash']).to eq(Digest::SHA256.hexdigest(valid_url)) + end - temp_file.close - temp_file.unlink + it 'updates sync attributes when the avatar URL is blocked by SSRF protection' do + blocked_url = 'http://127.0.0.1/avatar.png' + + expect do + described_class.perform_now(avatarable, blocked_url) + end.not_to raise_error + + avatarable.reload + expect(avatarable.avatar).not_to be_attached + expect(avatarable.additional_attributes['last_avatar_sync_at']).to be_present + expect(avatarable.additional_attributes['avatar_url_hash']).to eq(Digest::SHA256.hexdigest(blocked_url)) end end @@ -84,7 +168,13 @@ RSpec.describe Avatar::AvatarFromUrlJob do let(:avatarable) { create(:agent_bot) } it 'downloads and attaches avatar' do - expect(Down).to receive(:download).with(valid_url, max_size: Avatar::AvatarFromUrlJob::MAX_DOWNLOAD_SIZE).and_return(file) + stub_request(:get, valid_url) + .to_return( + status: 200, + body: File.read(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'image/png' } + ) + described_class.perform_now(avatarable, valid_url) expect(avatarable.avatar).to be_attached end @@ -93,22 +183,30 @@ RSpec.describe Avatar::AvatarFromUrlJob do # ref: https://github.com/chatwoot/chatwoot/issues/10449 it 'does not raise error when downloaded file has no filename (invalid content)' do contact = create(:contact) - temp_file = Tempfile.new(['invalid', '.xml']) - temp_file.write('content') - temp_file.rewind + invalid_file = Tempfile.new('avatar-without-name') - expect(Down).to receive(:download).with(valid_url, max_size: Avatar::AvatarFromUrlJob::MAX_DOWNLOAD_SIZE) - .and_return(ActionDispatch::Http::UploadedFile.new(tempfile: temp_file, type: 'application/xml')) + allow(SafeFetch).to receive(:fetch) + .with( + valid_url, + max_bytes: Avatar::AvatarFromUrlJob::MAX_DOWNLOAD_SIZE, + allowed_content_type_prefixes: [], + allowed_content_types: Avatar::AvatarFromUrlJob::ALLOWED_CONTENT_TYPES + ).and_yield( + SafeFetch::Result.new( + tempfile: invalid_file, + filename: nil, + content_type: 'image/png' + ) + ) expect { described_class.perform_now(contact, valid_url) }.not_to raise_error - - temp_file.close - temp_file.unlink + expect(contact.reload.avatar).not_to be_attached + ensure + invalid_file.close! end it 'skips sync attribute updates when URL is nil' do contact = create(:contact) - expect(Down).not_to receive(:download) expect { described_class.perform_now(contact, nil) }.not_to raise_error diff --git a/spec/lib/safe_fetch_spec.rb b/spec/lib/safe_fetch_spec.rb index d8d37115a..e2c513587 100644 --- a/spec/lib/safe_fetch_spec.rb +++ b/spec/lib/safe_fetch_spec.rb @@ -65,6 +65,23 @@ RSpec.describe SafeFetch do end end + context 'with embedded basic auth credentials' do + it 'passes decoded credentials to the request' do + authenticated_url = 'http://user+avatar%40example.com:p%40ss+word%3A1@example.com/image.png' + stub_request(:get, url) + .with(headers: { 'Authorization' => 'Basic dXNlcithdmF0YXJAZXhhbXBsZS5jb206cEBzcyt3b3JkOjE=' }) + .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 + end + end + context 'with URL validation' do it 'raises InvalidUrlError for javascript: URLs' do expect { described_class.fetch('javascript:alert(1)') { nil } } @@ -153,14 +170,49 @@ RSpec.describe SafeFetch do expect { described_class.fetch(url) { nil } }.not_to raise_error end - it 'strips charset/boundary parameters before comparing' do + it 'normalizes parameters and casing before yielding content_type' do stub_request(:get, url).to_return( status: 200, body: 'x', - headers: { 'Content-Type' => 'image/png; charset=binary' } + headers: { 'Content-Type' => 'IMAGE/PNG; charset=binary' } ) - expect { described_class.fetch(url) { nil } }.not_to raise_error + described_class.fetch(url) do |result| + expect(result.content_type).to eq('image/png') + end + end + + it 'allows exact content-type matches when prefixes are empty' do + pdf_url = 'http://example.com/file.pdf' + stub_request(:get, pdf_url).to_return( + status: 200, + body: 'pdf-data', + headers: { 'Content-Type' => 'application/pdf' } + ) + + expect do + described_class.fetch( + pdf_url, + allowed_content_type_prefixes: [], + allowed_content_types: ['application/pdf'] + ) { nil } + end.not_to raise_error + end + + it 'rejects exact content-type mismatches when prefixes are empty' do + stub_request(:get, url).to_return( + status: 200, + body: 'x', + headers: { 'Content-Type' => 'image/webp' } + ) + + expect do + described_class.fetch( + url, + allowed_content_type_prefixes: [], + allowed_content_types: ['image/png'] + ) { nil } + end.to raise_error(described_class::UnsupportedContentTypeError) end it 'rejects when the content-type header is missing' do diff --git a/spec/models/contact_spec.rb b/spec/models/contact_spec.rb index f21a81978..159454eec 100644 --- a/spec/models/contact_spec.rb +++ b/spec/models/contact_spec.rb @@ -16,6 +16,13 @@ RSpec.describe Contact do describe 'concerns' do it_behaves_like 'avatarable' + + it 'accepts webp avatars' do + contact = build(:contact, account: create(:account)) + contact.avatar.attach(get_blob_for(Rails.root.join('spec/assets/avatar.png'), 'image/webp')) + + expect(contact).to be_valid + end end context 'when prepare contact attributes before validation' do