From 871f2f4d56516d4fc07b6a242dab60872673995b Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Wed, 8 Apr 2026 10:47:54 +0530 Subject: [PATCH] fix: harden fetching on upload endpoint (#14012) --- Gemfile | 2 + Gemfile.lock | 2 + .../api/v1/accounts/upload_controller.rb | 42 +-- config/locales/en.yml | 8 + lib/safe_fetch.rb | 98 +++++++ .../api/v1/upload_controller_spec.rb | 113 +++++++- spec/lib/safe_fetch_spec.rb | 258 ++++++++++++++++++ 7 files changed, 494 insertions(+), 29 deletions(-) create mode 100644 lib/safe_fetch.rb create mode 100644 spec/lib/safe_fetch_spec.rb diff --git a/Gemfile b/Gemfile index 01c7a9f83..a5068e765 100644 --- a/Gemfile +++ b/Gemfile @@ -40,6 +40,8 @@ gem 'json_refs' gem 'rack-attack', '>= 6.7.0' # a utility tool for streaming, flexible and safe downloading of remote files gem 'down' +# SSRF-safe URL fetching +gem 'ssrf_filter', '~> 1.5' # authentication type to fetch and send mail over oauth2.0 gem 'gmail_xoauth' # Lock net-smtp to 0.3.4 to avoid issues with gmail_xoauth2 diff --git a/Gemfile.lock b/Gemfile.lock index 74ea4d82d..b77e5880f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -942,6 +942,7 @@ GEM activesupport (>= 5.2) sprockets (>= 3.0.0) squasher (0.7.2) + ssrf_filter (1.5.0) stackprof (0.2.25) statsd-ruby (1.5.0) stripe (18.0.1) @@ -1158,6 +1159,7 @@ DEPENDENCIES spring spring-watcher-listen squasher + ssrf_filter (~> 1.5) stackprof stripe (~> 18.0) telephone_number diff --git a/app/controllers/api/v1/accounts/upload_controller.rb b/app/controllers/api/v1/accounts/upload_controller.rb index 479d8ae1b..bf20bc6ff 100644 --- a/app/controllers/api/v1/accounts/upload_controller.rb +++ b/app/controllers/api/v1/accounts/upload_controller.rb @@ -5,7 +5,7 @@ class Api::V1::Accounts::UploadController < Api::V1::Accounts::BaseController elsif params[:external_url].present? create_from_url else - render_error('No file or URL provided', :unprocessable_entity) + render_error(I18n.t('errors.upload.missing_input'), :unprocessable_entity) end render_success(result) if result.is_a?(ActiveStorage::Blob) @@ -19,35 +19,21 @@ class Api::V1::Accounts::UploadController < Api::V1::Accounts::BaseController end def create_from_url - uri = parse_uri(params[:external_url]) - return if performed? - - fetch_and_process_file_from_uri(uri) - end - - def parse_uri(url) - uri = URI.parse(url) - validate_uri(uri) - uri - rescue URI::InvalidURIError, SocketError - render_error('Invalid URL provided', :unprocessable_entity) - nil - end - - def validate_uri(uri) - raise URI::InvalidURIError unless uri.is_a?(URI::HTTP) || uri.is_a?(URI::HTTPS) - end - - def fetch_and_process_file_from_uri(uri) - uri.open do |file| - create_and_save_blob(file, File.basename(uri.path), file.content_type) + SafeFetch.fetch(params[:external_url].to_s) do |result| + create_and_save_blob(result.tempfile, result.filename, result.content_type) end - rescue OpenURI::HTTPError => e - render_error("Failed to fetch file from URL: #{e.message}", :unprocessable_entity) - rescue SocketError - render_error('Invalid URL provided', :unprocessable_entity) + rescue SafeFetch::HttpError => e + render_error(I18n.t('errors.upload.fetch_failed_with_message', message: e.message), :unprocessable_entity) + rescue SafeFetch::FetchError + render_error(I18n.t('errors.upload.fetch_failed'), :unprocessable_entity) + rescue SafeFetch::FileTooLargeError + render_error(I18n.t('errors.upload.file_too_large'), :unprocessable_entity) + rescue SafeFetch::UnsupportedContentTypeError + render_error(I18n.t('errors.upload.unsupported_content_type'), :unprocessable_entity) + rescue SafeFetch::Error + render_error(I18n.t('errors.upload.invalid_url'), :unprocessable_entity) rescue StandardError - render_error('An unexpected error occurred', :internal_server_error) + render_error(I18n.t('errors.upload.unexpected'), :internal_server_error) end def create_and_save_blob(io, filename, content_type) diff --git a/config/locales/en.yml b/config/locales/en.yml index e6308c43c..1cb3c4d12 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -66,6 +66,14 @@ en: not_found: Assignment policy not found attachments: invalid: Invalid attachment + upload: + missing_input: 'No file or URL provided' + invalid_url: 'Invalid URL provided' + fetch_failed: 'Failed to fetch file from URL' + fetch_failed_with_message: 'Failed to fetch file from URL: %{message}' + file_too_large: 'File exceeds the maximum allowed size' + unsupported_content_type: 'File type not supported (only images and videos are allowed)' + unexpected: 'An unexpected error occurred' saml: feature_not_enabled: SAML feature not enabled for this account sso_not_enabled: SAML SSO is not enabled for this installation diff --git a/lib/safe_fetch.rb b/lib/safe_fetch.rb new file mode 100644 index 000000000..e6635c9c3 --- /dev/null +++ b/lib/safe_fetch.rb @@ -0,0 +1,98 @@ +require 'ssrf_filter' + +module SafeFetch + DEFAULT_ALLOWED_CONTENT_TYPE_PREFIXES = %w[image/ video/].freeze + DEFAULT_OPEN_TIMEOUT = 2 + DEFAULT_READ_TIMEOUT = 20 + DEFAULT_MAX_BYTES_FALLBACK_MB = 40 + + Result = Data.define(:tempfile, :filename, :content_type) + + class Error < StandardError; end + class InvalidUrlError < Error; end + class UnsafeUrlError < Error; end + class FetchError < Error; end + class HttpError < Error; end + class FileTooLargeError < Error; end + class UnsupportedContentTypeError < Error; end + + def self.fetch(url, + max_bytes: nil, + allowed_content_type_prefixes: DEFAULT_ALLOWED_CONTENT_TYPE_PREFIXES) + 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) + 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']) + 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 stream_to_tempfile(url, tempfile, max_bytes, allowed_content_type_prefixes) + response = nil + bytes_written = 0 + + SsrfFilter.get( + url, + 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) + 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 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) + mime = value.to_s.split(';').first&.strip&.downcase + return false if mime.blank? + + prefixes.any? { |prefix| mime.start_with?(prefix) } + end + end +end diff --git a/spec/controllers/api/v1/upload_controller_spec.rb b/spec/controllers/api/v1/upload_controller_spec.rb index 93ef28dd8..2878c2a5e 100644 --- a/spec/controllers/api/v1/upload_controller_spec.rb +++ b/spec/controllers/api/v1/upload_controller_spec.rb @@ -39,6 +39,11 @@ RSpec.describe 'Api::V1::Accounts::UploadController', type: :request do let(:valid_external_url) { 'http://example.com/image.jpg' } before do + allow(Resolv).to receive(:getaddresses).and_call_original + allow(Resolv).to receive(:getaddresses).with('example.com').and_return(['93.184.216.34']) + allow(Resolv).to receive(:getaddresses).with('error.example.com').and_return(['93.184.216.34']) + allow(Resolv).to receive(:getaddresses).with('nonexistent.example.com').and_return(['93.184.216.34']) + stub_request(:get, valid_external_url) .to_return(status: 200, body: File.new(Rails.root.join('spec/assets/avatar.png')), headers: { 'Content-Type' => 'image/png' }) end @@ -82,7 +87,7 @@ RSpec.describe 'Api::V1::Accounts::UploadController', type: :request do params: { external_url: 'http://nonexistent.example.com' } expect(response).to have_http_status(:unprocessable_entity) - expect(response.parsed_body['error']).to eq('Invalid URL provided') + expect(response.parsed_body['error']).to eq('Failed to fetch file from URL') end it 'handles HTTP errors' do @@ -96,6 +101,112 @@ RSpec.describe 'Api::V1::Accounts::UploadController', type: :request do expect(response).to have_http_status(:unprocessable_entity) expect(response.parsed_body['error']).to start_with('Failed to fetch file from URL') end + + it 'rejects oversized responses with a file-size message' do + stub_request(:get, valid_external_url) + .to_return(status: 200, + body: 'x' * (41 * 1024 * 1024), + headers: { 'Content-Type' => 'image/png' }) + + post upload_url, + headers: user.create_new_auth_token, + params: { external_url: valid_external_url } + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq('File exceeds the maximum allowed size') + end + + it 'rejects unsupported content types with a file-type message' do + stub_request(:get, valid_external_url) + .to_return(status: 200, + body: '', + headers: { 'Content-Type' => 'text/html' }) + + post upload_url, + headers: user.create_new_auth_token, + params: { external_url: valid_external_url } + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq('File type not supported (only images and videos are allowed)') + end + + context 'with SSRF attack vectors' do + it 'blocks requests to private IP ranges (10.x.x.x)' do + post upload_url, + headers: user.create_new_auth_token, + params: { external_url: 'http://10.0.0.1/secret' } + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq('Invalid URL provided') + end + + it 'blocks requests to private IP ranges (172.16.x.x)' do + post upload_url, + headers: user.create_new_auth_token, + params: { external_url: 'http://172.16.0.1/secret' } + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq('Invalid URL provided') + end + + it 'blocks requests to private IP ranges (192.168.x.x)' do + post upload_url, + headers: user.create_new_auth_token, + params: { external_url: 'http://192.168.1.1/secret' } + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq('Invalid URL provided') + end + + it 'blocks requests to loopback addresses' do + post upload_url, + headers: user.create_new_auth_token, + params: { external_url: 'http://127.0.0.1/secret' } + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq('Invalid URL provided') + end + + it 'blocks requests to AWS metadata service (169.254.169.254)' do + post upload_url, + headers: user.create_new_auth_token, + params: { external_url: 'http://169.254.169.254/latest/meta-data/iam/security-credentials/' } + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq('Invalid URL provided') + end + + it 'blocks requests to localhost' do + post upload_url, + headers: user.create_new_auth_token, + params: { external_url: 'http://localhost/secret' } + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq('Invalid URL provided') + end + + it 'blocks requests to .local domains' do + allow(Resolv).to receive(:getaddresses).with('server.local').and_return(['192.168.1.100']) + + post upload_url, + headers: user.create_new_auth_token, + params: { external_url: 'http://server.local/secret' } + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq('Invalid URL provided') + end + + it 'blocks DNS rebinding attacks (hostname resolving to private IP)' do + allow(Resolv).to receive(:getaddresses).with('evil.attacker.com').and_return(['10.0.0.1']) + + post upload_url, + headers: user.create_new_auth_token, + params: { external_url: 'http://evil.attacker.com/secret' } + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.parsed_body['error']).to eq('Invalid URL provided') + end + end end it 'returns an error when no file or URL is provided' do diff --git a/spec/lib/safe_fetch_spec.rb b/spec/lib/safe_fetch_spec.rb new file mode 100644 index 000000000..70f9b05de --- /dev/null +++ b/spec/lib/safe_fetch_spec.rb @@ -0,0 +1,258 @@ +require 'rails_helper' + +# `SafeFetch.fetch` is a custom method that requires a block (it yields a Result); +# it is NOT `Hash#fetch`, so RuboCop's autocorrect to `fetch(url, nil)` would break the API. +# rubocop:disable Style/RedundantFetchBlock +RSpec.describe SafeFetch do + let(:url) { 'http://example.com/image.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 + + describe '.fetch' do + context 'with a valid public URL serving an image' do + before do + stub_request(:get, url).to_return( + status: 200, + body: File.new(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'image/png' } + ) + end + + it 'yields a Result with tempfile, filename, and content_type' do + described_class.fetch(url) do |result| + expect(result.tempfile).to be_a(Tempfile) + expect(result.filename).to eq('image.png') + expect(result.content_type).to eq('image/png') + expect(result.tempfile.size).to be > 0 + end + end + + it 'closes the tempfile after the block returns' do + captured = nil + described_class.fetch(url) { |result| captured = result.tempfile } + expect(captured.closed?).to be true + end + + it 'closes the tempfile even when the block raises' do + captured = nil + expect do + described_class.fetch(url) do |result| + captured = result.tempfile + raise 'boom' + end + end.to raise_error('boom') + expect(captured.closed?).to be true + end + + it 'defaults the filename to a unique "download--" when the URL has no path' do + bare_url = 'http://example.com' + stub_request(:get, bare_url).to_return( + status: 200, + body: File.new(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'image/png' } + ) + + described_class.fetch(bare_url) do |result| + expect(result.filename).to match(/\Adownload-\d+-[a-f0-9]{8}\z/) + end + end + + it 'requires a block' do + expect { described_class.fetch(url) }.to raise_error(ArgumentError, /block required/) + end + end + + context 'with URL validation' do + it 'raises InvalidUrlError for javascript: URLs' do + expect { described_class.fetch('javascript:alert(1)') { nil } } + .to raise_error(SafeFetch::InvalidUrlError) + end + + it 'raises InvalidUrlError for mailto: URLs' do + expect { described_class.fetch('mailto:test@example.com') { nil } } + .to raise_error(SafeFetch::InvalidUrlError) + end + + it 'raises InvalidUrlError for data: URLs' do + expect { described_class.fetch('data:text/html,') { nil } } + .to raise_error(SafeFetch::InvalidUrlError) + end + + it 'raises InvalidUrlError for ftp: URLs' do + expect { described_class.fetch('ftp://example.com/file') { nil } } + .to raise_error(SafeFetch::InvalidUrlError) + end + + it 'raises InvalidUrlError for malformed URLs' do + expect { described_class.fetch('not_a_url') { nil } } + .to raise_error(SafeFetch::InvalidUrlError) + end + + it 'raises InvalidUrlError when host is missing' do + expect { described_class.fetch('http:///path') { nil } } + .to raise_error(SafeFetch::InvalidUrlError, /missing host/) + end + end + + context 'with SSRF protection (integration with ssrf_filter)' do + it 'raises UnsafeUrlError for private IP literals (10.x.x.x)' do + expect { described_class.fetch('http://10.0.0.1/secret') { nil } } + .to raise_error(SafeFetch::UnsafeUrlError) + end + + it 'raises UnsafeUrlError for loopback addresses' do + expect { described_class.fetch('http://127.0.0.1/secret') { nil } } + .to raise_error(SafeFetch::UnsafeUrlError) + end + + it 'raises UnsafeUrlError for AWS metadata IP (169.254.169.254)' do + expect { described_class.fetch('http://169.254.169.254/latest/meta-data/') { nil } } + .to raise_error(SafeFetch::UnsafeUrlError) + end + + it 'raises UnsafeUrlError when hostname resolves to a private IP (DNS rebinding)' do + allow(Resolv).to receive(:getaddresses).with('evil.example.com').and_return(['10.0.0.1']) + expect { described_class.fetch('http://evil.example.com/secret') { nil } } + .to raise_error(SafeFetch::UnsafeUrlError) + end + end + + context 'with content-type allowlist' do + it 'rejects text/html responses' do + stub_request(:get, url).to_return( + status: 200, + body: '', + headers: { 'Content-Type' => 'text/html' } + ) + + expect { described_class.fetch(url) { nil } } + .to raise_error(SafeFetch::UnsupportedContentTypeError) + end + + it 'rejects application/octet-stream responses' do + stub_request(:get, url).to_return( + status: 200, + body: 'x', + headers: { 'Content-Type' => 'application/octet-stream' } + ) + + expect { described_class.fetch(url) { nil } } + .to raise_error(SafeFetch::UnsupportedContentTypeError) + end + + it 'allows video/mp4 responses' do + stub_request(:get, url).to_return( + status: 200, + body: File.new(Rails.root.join('spec/assets/avatar.png')), + headers: { 'Content-Type' => 'video/mp4' } + ) + + expect { described_class.fetch(url) { nil } }.not_to raise_error + end + + it 'strips charset/boundary parameters before comparing' do + stub_request(:get, url).to_return( + status: 200, + body: 'x', + headers: { 'Content-Type' => 'image/png; charset=binary' } + ) + + expect { described_class.fetch(url) { nil } }.not_to raise_error + end + + it 'rejects when the content-type header is missing' do + stub_request(:get, url).to_return(status: 200, body: 'x', headers: {}) + + expect { described_class.fetch(url) { nil } } + .to raise_error(SafeFetch::UnsupportedContentTypeError) + end + end + + context 'with body size cap' do + it 'honours a custom max_bytes argument' do + stub_request(:get, url).to_return( + status: 200, + body: 'xxxxx', + headers: { 'Content-Type' => 'image/png' } + ) + + expect { described_class.fetch(url, max_bytes: 2) { nil } } + .to raise_error(SafeFetch::FileTooLargeError) + end + + it 'reads the default cap from GlobalConfigService MAXIMUM_FILE_UPLOAD_SIZE (matching Attachment#validate_file_size)' do + allow(GlobalConfigService).to receive(:load).and_call_original + allow(GlobalConfigService).to receive(:load).with('MAXIMUM_FILE_UPLOAD_SIZE', 40).and_return('1') + + oversize = 'x' * (1.megabyte + 1) + stub_request(:get, url).to_return( + status: 200, + body: oversize, + headers: { 'Content-Type' => 'image/png' } + ) + + expect { described_class.fetch(url) { nil } } + .to raise_error(SafeFetch::FileTooLargeError) + end + + it 'falls back to 40 MB when GlobalConfigService returns a non-positive value' do + allow(GlobalConfigService).to receive(:load).and_call_original + allow(GlobalConfigService).to receive(:load).with('MAXIMUM_FILE_UPLOAD_SIZE', 40).and_return('-10') + + # 1 MB body should pass under the 40 MB fallback + stub_request(:get, url).to_return( + status: 200, + body: 'x' * 1.megabyte, + headers: { 'Content-Type' => 'image/png' } + ) + + expect { described_class.fetch(url) { nil } }.not_to raise_error + end + + it 'allows uploads between the old hardcoded 10 MB and the configured limit (regression check)' do + # Default config is 40 MB; a 15 MB upload must succeed. + # This is the exact regression scenario: with the old hardcoded 10 MB cap, + # this would have failed even though direct file uploads of the same size succeed. + allow(GlobalConfigService).to receive(:load).and_call_original + allow(GlobalConfigService).to receive(:load).with('MAXIMUM_FILE_UPLOAD_SIZE', 40).and_return('40') + + stub_request(:get, url).to_return( + status: 200, + body: 'x' * (15 * 1024 * 1024), + headers: { 'Content-Type' => 'image/png' } + ) + + expect { described_class.fetch(url) { nil } }.not_to raise_error + end + end + + context 'with network failures' do + it 'maps Net::ReadTimeout to FetchError' do + stub_request(:get, url).to_raise(Net::ReadTimeout) + + expect { described_class.fetch(url) { nil } } + .to raise_error(SafeFetch::FetchError) + end + + it 'maps SocketError to FetchError' do + stub_request(:get, url).to_raise(SocketError.new('connection refused')) + + expect { described_class.fetch(url) { nil } } + .to raise_error(SafeFetch::FetchError) + end + end + + context 'with non-2xx upstream responses' do + it 'raises HttpError with the status code in the message' do + stub_request(:get, url).to_return(status: 404, body: '', headers: {}) + + expect { described_class.fetch(url) { nil } } + .to raise_error(SafeFetch::HttpError, /404/) + end + end + end +end +# rubocop:enable Style/RedundantFetchBlock