security: hardening url fetches, rate-limits & session fixation

app/controllers/api/v4/uploads_controller.rb

@@ -5,6 +5,10 @@ module V4
     class UploadsController < ApplicationController
       before_action :check_quota, only: [ :create, :create_from_url ]
 
+      rate_limit to: 10, within: 1.minute, only: :create_from_url,
+        by: -> { current_user&.id },
+        with: -> { render json: { error: "Too many requests" }, status: :too_many_requests }
+
       # POST /api/v4/upload
       def create
         file = params[:file]

app/controllers/sessions_controller.rb

@@ -6,6 +6,7 @@ class SessionsController < ApplicationController
   def create
     auth = request.env["omniauth.auth"]
     user = User.find_or_create_from_omniauth(auth)
+    reset_session
     session[:user_id] = user.id
 
     # Check and upgrade verification status if needed

app/models/upload.rb

@@ -1,8 +1,8 @@
 # frozen_string_literal: true
 
-require "open-uri"
-require "resolv"
 require "ipaddr"
+require "net/http"
+require "socket"
 
 class Upload < ApplicationRecord
   include PgSearch::Model
@@ -96,64 +96,80 @@ def rename!(new_filename)
     blob.update!(filename: sanitized)
   end
 
-  # Validate that a URL is safe to fetch (not targeting internal networks)
-  def self.assert_public_url!(url)
-    uri = URI.parse(url)
+  MAX_FETCH_REDIRECTS = 5
+  FETCH_OPEN_TIMEOUT = 5
+  FETCH_READ_TIMEOUT = 30
+  FETCH_DEADLINE = 60
+
+  BLOCKED_FETCH_RANGES = %w[
+    0.0.0.0/8 10.0.0.0/8 100.64.0.0/10 127.0.0.0/8 169.254.0.0/16 172.16.0.0/12
+    192.0.0.0/24 192.168.0.0/16 198.18.0.0/15 224.0.0.0/4 240.0.0.0/4
+    ::/128 ::1/128 64:ff9b::/96 fc00::/7 fe80::/10
+  ].map { |range| IPAddr.new(range) }.freeze
 
+  def self.resolve_fetchable_addresses!(uri)
     unless %w[http https].include?(uri.scheme&.downcase)
       raise ArgumentError, "URL scheme must be http or https."
     end
 
-    host = uri.host
-    raise ArgumentError, "Invalid host" if host.nil? || host.empty?
+    host = uri.hostname
+    raise ArgumentError, "Invalid host" if host.blank?
+
+    port = uri.port || uri.default_port
 
-    begin
-      addrs = Resolv.getaddresses(host)
-    rescue Resolv::ResolvError
+    addresses =
       begin
-        IPAddr.new(host)
-        addrs = [ host ]
-      rescue IPAddr::InvalidAddressError
-        raise ArgumentError, "Couldn't resolve host."
+        Addrinfo.getaddrinfo(host, port, nil, :STREAM).map(&:ip_address).uniq
+      rescue SocketError
+        []
       end
+    raise ArgumentError, "Couldn't resolve host." if addresses.empty?
+
+    addresses.each do |address|
+      raise ArgumentError, "IP address not allowed" if blocked_fetch_address?(address)
     end
-    raise ArgumentError, "Couldn't resolve host." if addrs.empty?
 
-    addrs.each do |addr_str|
-      ip = IPAddr.new(addr_str)
-      raise ArgumentError, "IP address not allowed" if ip.loopback? || ip.private? || ip.link_local?
+    addresses
+  end
+
+  def self.fetch_public_url!(url, max_bytes:, authorization: nil)
+    uri = URI.parse(url)
+    token = authorization
+    hops = 0
+    deadline = Process.clock_gettime(Process::CLOCK_MONOTONIC) + FETCH_DEADLINE
+
+    loop do
+      address = resolve_fetchable_addresses!(uri).first
+      result = perform_fetch(uri, address, token, max_bytes, deadline)
+      return result unless result.key?(:location)
+
+      hops += 1
+      raise "Failed to download: too many redirects" if hops > MAX_FETCH_REDIRECTS
+      raise "Failed to download: redirect without a location" if result[:location].blank?
+
+      following = uri.merge(result[:location])
+      token = nil unless same_fetch_origin?(uri, following)
+      uri = following
     end
   end
 
   # Create upload from URL (for API/rescue operations)
-  # Checks quota via HEAD request before downloading when possible
   def self.create_from_url(url, user:, provenance:, original_url: nil, authorization: nil, filename: nil)
-    assert_public_url!(url)
-
-    redirect_validator = proc do |_response_env, new_request_env|
-      assert_public_url!(new_request_env[:url].to_s)
-    end
-
-    conn = build_http_client(redirect_validator)
-
-    headers = {}
-    headers["Authorization"] = authorization if authorization.present?
-
-    # Pre-check file size via HEAD if possible
-    pre_check_quota_via_head(conn, url, headers, user)
-
-    # Download the file
-    response = conn.get(url, nil, headers)
-    if response.status.between?(300, 399)
-      location = response.headers["location"]
-      raise "Failed to download: #{response.status} redirect to #{location}"
-    end
-    raise "Failed to download: #{response.status}" unless response.success?
+    quota_service = QuotaService.new(user)
+    policy = quota_service.current_policy
+    remaining_storage = policy.max_total_storage - user.total_storage_bytes
+    raise "File would exceed storage quota" if remaining_storage <= 0
+
+    fetched = fetch_public_url!(
+      url,
+      max_bytes: [ policy.max_file_size, remaining_storage ].min,
+      authorization: authorization
+    )
 
     filename ||= extract_filename_from_url(url)
-    body = response.body
+    body = fetched[:body]
     content_type = Marcel::MimeType.for(StringIO.new(body), name: filename) ||
-                   response.headers["content-type"] ||
+                   fetched[:content_type] ||
                    "application/octet-stream"
     content_type = normalize_content_type(content_type)
 
@@ -182,37 +198,49 @@ def self.create_from_url(url, user:, provenance:, original_url: nil, authorizati
   class << self
     private
 
-    def build_http_client(redirect_validator = nil)
-      Faraday.new(ssl: { verify: true, verify_mode: OpenSSL::SSL::VERIFY_PEER }) do |f|
-        f.response :follow_redirects, limit: 5, callback: redirect_validator
-        f.adapter Faraday.default_adapter
-      end.tap do |conn|
-        conn.options.open_timeout = 30
-        conn.options.timeout = 120
-      end
+    def blocked_fetch_address?(address)
+      ip = IPAddr.new(address.split("%").first)
+      ip = ip.native if ip.ipv6? && ip.ipv4_mapped?
+      BLOCKED_FETCH_RANGES.any? { |range| range.family == ip.family && range.include?(ip) }
+    rescue IPAddr::InvalidAddressError
+      true
     end
 
-    def pre_check_quota_via_head(conn, url, headers, user)
-      head_response = conn.head(url, nil, headers)
-      return unless head_response.success?
-
-      content_length = head_response.headers["content-length"]&.to_i
-      return unless content_length && content_length > 0
-
-      quota_service = QuotaService.new(user)
-      policy = quota_service.current_policy
-
-      if content_length > policy.max_file_size
-        raise "File too large: #{ActiveSupport::NumberHelper.number_to_human_size(content_length)} " \
-              "exceeds limit of #{ActiveSupport::NumberHelper.number_to_human_size(policy.max_file_size)}"
+    def perform_fetch(uri, address, authorization, max_bytes, deadline)
+      http = Net::HTTP.new(uri.hostname, uri.port)
+      http.ipaddr = address
+      http.use_ssl = uri.scheme == "https"
+      http.verify_mode = OpenSSL::SSL::VERIFY_PEER
+      http.open_timeout = FETCH_OPEN_TIMEOUT
+      http.read_timeout = FETCH_READ_TIMEOUT
+
+      request = Net::HTTP::Get.new(uri.request_uri)
+      request["Authorization"] = authorization if authorization.present?
+
+      http.start do |connection|
+        connection.request(request) do |response|
+          return { location: response["location"] } if response.is_a?(Net::HTTPRedirection)
+          raise "Failed to download: #{response.code}" unless response.is_a?(Net::HTTPSuccess)
+
+          body = +""
+          response.read_body do |chunk|
+            body << chunk
+            if body.bytesize > max_bytes
+              raise "File too large: exceeds limit of " \
+                    "#{ActiveSupport::NumberHelper.number_to_human_size(max_bytes)}"
+            end
+            if Process.clock_gettime(Process::CLOCK_MONOTONIC) > deadline
+              raise "Failed to download: timed out"
+            end
+          end
+
+          return { body: body, content_type: response["content-type"] }
+        end
       end
+    end
 
-      return if quota_service.can_upload?(content_length)
-
-      raise "File would exceed storage quota"
-    rescue Faraday::Error
-      # HEAD request failed — proceed with GET and check after
-      nil
+    def same_fetch_origin?(from, to)
+      [ from.scheme, from.hostname, from.port ] == [ to.scheme, to.hostname, to.port ]
     end
 
     def extract_filename_from_url(url)

test/controllers/api/v4/uploads_controller_test.rb

@@ -46,20 +46,10 @@ class API::V4::UploadsControllerTest < ActionDispatch::IntegrationTest
   test "should upload from URL with valid token" do
     url = "https://example.com/test.jpg"
 
-    fake_response = Struct.new(:status, :body, :headers) { def success? = true }
-                          .new(200, "fake image data", { "content-type" => "image/jpeg" })
-    fake_opts = Object.new.tap { |o| o.define_singleton_method(:open_timeout=) { |_| }
-                                     o.define_singleton_method(:timeout=) { |_| } }
-    fake_head_response = Struct.new(:status, :body, :headers) { def success? = false }
-                               .new(405, "", {})
-    fake_conn = Object.new.tap { |c| c.define_singleton_method(:options) { fake_opts }
-                                     c.define_singleton_method(:get) { |*| fake_response }
-                                     c.define_singleton_method(:head) { |*| fake_head_response } }
-
-    original_faraday_new = Faraday.method(:new)
-    original_assert_public_url = Upload.method(:assert_public_url!)
-    Faraday.define_singleton_method(:new) { |*, **, &_block| fake_conn }
-    Upload.define_singleton_method(:assert_public_url!) { |_url| nil }
+    original_fetch = Upload.method(:fetch_public_url!)
+    Upload.define_singleton_method(:fetch_public_url!) do |_url, **_options|
+      { body: "fake image data", content_type: "image/jpeg" }
+    end
     begin
       assert_difference("Upload.count", 1) do
         post api_v4_upload_from_url_url,
@@ -70,8 +60,7 @@ class API::V4::UploadsControllerTest < ActionDispatch::IntegrationTest
           }
       end
     ensure
-      Faraday.define_singleton_method(:new, original_faraday_new)
-      Upload.define_singleton_method(:assert_public_url!, original_assert_public_url)
+      Upload.define_singleton_method(:fetch_public_url!, original_fetch)
     end
 
     assert_response :created
@@ -96,15 +85,10 @@ class API::V4::UploadsControllerTest < ActionDispatch::IntegrationTest
   test "should handle upload errors gracefully" do
     url = "https://example.com/broken.jpg"
 
-    fake_opts = Object.new.tap { |o| o.define_singleton_method(:open_timeout=) { |_| }
-                                     o.define_singleton_method(:timeout=) { |_| } }
-    fake_conn = Object.new.tap { |c| c.define_singleton_method(:options) { fake_opts }
-                                     c.define_singleton_method(:get) { |*| raise StandardError, "Network error" } }
-
-    original_faraday_new = Faraday.method(:new)
-    original_assert_public_url = Upload.method(:assert_public_url!)
-    Faraday.define_singleton_method(:new) { |*, **, &_block| fake_conn }
-    Upload.define_singleton_method(:assert_public_url!) { |_url| nil }
+    original_fetch = Upload.method(:fetch_public_url!)
+    Upload.define_singleton_method(:fetch_public_url!) do |_url, **_options|
+      raise StandardError, "Network error"
+    end
     begin
       post api_v4_upload_from_url_url,
         params: { url: url }.to_json,
@@ -113,15 +97,55 @@ class API::V4::UploadsControllerTest < ActionDispatch::IntegrationTest
           "Content-Type" => "application/json"
         }
     ensure
-      Faraday.define_singleton_method(:new, original_faraday_new)
-      Upload.define_singleton_method(:assert_public_url!, original_assert_public_url)
+      Upload.define_singleton_method(:fetch_public_url!, original_fetch)
     end
 
     assert_response :unprocessable_entity
     json = JSON.parse(response.body)
     assert json["error"].include?("Upload failed")
   end
 
+  test "should rate limit URL uploads per user across API keys" do
+    second_token = @user.api_keys.create!(name: "Second Test Key").token
+    upload = create_upload("rate-limit.jpg")
+    requests_by_key = Hash.new(0)
+    rate_limit_store = API::V4::UploadsController.cache_store
+    original_create_from_url = Upload.method(:create_from_url)
+    original_increment = rate_limit_store.method(:increment)
+
+    Upload.define_singleton_method(:create_from_url) { |_url, **_options| upload }
+    rate_limit_store.define_singleton_method(:increment) do |key, amount, **_options|
+      requests_by_key[key] += amount
+    end
+
+    begin
+      10.times do |index|
+        post api_v4_upload_from_url_url,
+          params: { url: "https://example.com/rate-limit.jpg" }.to_json,
+          headers: {
+            "Authorization" => "Bearer #{index.even? ? @token : second_token}",
+            "Content-Type" => "application/json"
+          }
+
+        assert_response :created
+      end
+
+      post api_v4_upload_from_url_url,
+        params: { url: "https://example.com/rate-limit.jpg" }.to_json,
+        headers: {
+          "Authorization" => "Bearer #{second_token}",
+          "Content-Type" => "application/json"
+        }
+    ensure
+      Upload.define_singleton_method(:create_from_url, original_create_from_url)
+      rate_limit_store.define_singleton_method(:increment, original_increment)
+    end
+
+    assert_response :too_many_requests
+    assert_equal({ "error" => "Too many requests" }, JSON.parse(response.body))
+    assert_equal [ "rate-limit:api/v4/uploads:#{@user.id}" ], requests_by_key.keys
+  end
+
   # --- batch upload ---
 
   test "should upload a batch of files" do