Merge pull request #210 from loftwah/dl/enhance-image-validation

Enhance Image Validation and Handling #155

app/models/user.rb

@@ -18,8 +18,13 @@ class User < ApplicationRecord
   validates :username, presence: true, uniqueness: true, format: { with: VALID_USERNAME_REGEX, message: 'can only contain letters, numbers, and underscores' }
   validates :full_name, presence: true
   validates :avatar_border, inclusion: { in: ['white', 'black', 'none', 'rainbow', 'rainbow-overlay'] }
-  validates :avatar, format: { with: /\A(https?:\/\/).*\z/i, message: "must be a valid URL" }, allow_blank: true
-  validates :banner, format: { with: /\A(https?:\/\/).*\z/i, message: "must be a valid URL" }, allow_blank: true
+
+  # Remove these two lines as we're replacing them with custom validation
+  # validates :avatar, format: { with: /\A(https?:\/\/).*\z/i, message: "must be a valid URL" }, allow_blank: true
+  # validates :banner, format: { with: /\A(https?:\/\/).*\z/i, message: "must be a valid URL" }, allow_blank: true
+
+  # Add this line to use our new custom validation
+  validate :validate_image_urls
 
   before_validation :ensure_username_presence
   before_create :set_default_images
@@ -62,6 +67,46 @@ def valid_url?(url)
 
   private
 
+  def validate_image_urls
+    validate_image_url(:avatar, "Avatar")
+    validate_image_url(:banner, "Banner")
+  end
+
+  def validate_image_url(attribute, attribute_name)
+    url = self[attribute]
+    return if url.blank?
+
+    # Allow DigitalOcean Spaces URLs without further validation
+    return if url.start_with?('https://linkarooie.syd1.digitaloceanspaces.com/')
+
+    unless url.match?(/\A(https?:\/\/).*\z/i)
+      errors.add(attribute, "#{attribute_name} URL must start with http:// or https://")
+      return
+    end
+
+    # Optionally validate the URL, but don't prevent saving if it fails
+    unless valid_image_url?(url)
+      logger.warn "Warning: #{attribute_name} URL may not be a valid image: #{url}"
+    end
+  end
+
+  def valid_image_url?(url)
+    begin
+      response = fetch_head(url)
+      return response.is_a?(Net::HTTPSuccess) && response['Content-Type'].to_s.start_with?('image/')
+    rescue SocketError, URI::InvalidURIError, Net::OpenTimeout, OpenSSL::SSL::SSLError => e
+      logger.error "Error validating image URL: #{url}. Error: #{e.message}"
+      false
+    end
+  end
+
+  def fetch_head(url)
+    uri = URI.parse(url)
+    Net::HTTP.start(uri.host, uri.port, use_ssl: uri.scheme == 'https', open_timeout: 5, read_timeout: 5) do |http|
+      http.request_head(uri.path)
+    end
+  end
+
   def generate_open_graph_image_async
     GenerateOpenGraphImageJob.perform_later(self.id)
   end
@@ -174,4 +219,4 @@ def handle_invalid_url(type, error)
     Rails.logger.error "Invalid URL for #{type}: #{error.message}"
     self[type] = type == :avatar ? FALLBACK_AVATAR_URL : FALLBACK_BANNER_URL
   end
-end
+end

app/services/open_graph_image_generator.rb

@@ -105,47 +105,55 @@ def generate
     end
   end
 
-  private
-
   def valid_image_url?(url)
+    return true if url.start_with?('https://linkarooie.syd1.digitaloceanspaces.com/')
     return false if url.blank?
 
     begin
       uri = URI.parse(url)
       return false unless uri.is_a?(URI::HTTP) || uri.is_a?(URI::HTTPS)
       return false if uri.host.nil?
 
-      # Check if the response is an image
-      response = Net::HTTP.get_response(uri)
-      response.is_a?(Net::HTTPSuccess) && response['Content-Type'].start_with?('image/')
-    rescue URI::InvalidURIError, SocketError, Errno::ECONNREFUSED => e
+      response = fetch_head(uri)
+      return response.is_a?(Net::HTTPSuccess) && response['Content-Type'].to_s.start_with?('image/')
+    rescue URI::InvalidURIError, SocketError, Errno::ECONNREFUSED, Net::OpenTimeout, OpenSSL::SSL::SSLError => e
       Rails.logger.error("Invalid or unreachable URL: #{url}. Error: #{e.message}.")
       false
     end
   end
 
   def download_image(url)
+    return MiniMagick::Image.open(url) if url.start_with?('https://linkarooie.syd1.digitaloceanspaces.com/')
+
     uri = URI.parse(url)
-    begin
-      response = Net::HTTP.get_response(uri)
-      if response.is_a?(Net::HTTPSuccess)
-        content_type = response['Content-Type']
-
-        # Only proceed if the content-type is an image
-        if content_type.start_with?('image/')
-          MiniMagick::Image.read(response.body)
-        else
-          Rails.logger.error("URL does not point to an image: #{url}. Content-Type: #{content_type}. Using fallback.")
-          MiniMagick::Image.open(Rails.root.join('public', 'avatars', 'default_avatar.jpg'))
-        end
+    response = Net::HTTP.get_response(uri)
+
+    if response.is_a?(Net::HTTPSuccess)
+      content_type = response['Content-Type']
+
+      if content_type.to_s.start_with?('image/')
+        MiniMagick::Image.read(response.body)
       else
-        Rails.logger.error("Failed to download image from URL: #{url}. HTTP Error: #{response.code} #{response.message}. Using fallback.")
-        MiniMagick::Image.open(Rails.root.join('public', 'avatars', 'default_avatar.jpg'))
+        handle_invalid_image("URL does not point to an image: #{url}. Content-Type: #{content_type}.")
       end
-    rescue StandardError => e
-      Rails.logger.error("Failed to download image from URL: #{url}. Error: #{e.message}. Using fallback.")
-      MiniMagick::Image.open(Rails.root.join('public', 'avatars', 'default_avatar.jpg'))
+    else
+      handle_invalid_image("Failed to download image from URL: #{url}. HTTP Error: #{response.code} #{response.message}.")
     end
+  rescue StandardError => e
+    handle_invalid_image("Failed to download image from URL: #{url}. Error: #{e.message}.")
+  end
+
+  private
+
+  def fetch_head(uri)
+    Net::HTTP.start(uri.host, uri.port, use_ssl: uri.scheme == 'https', open_timeout: 5, read_timeout: 5) do |http|
+      http.request_head(uri.path)
+    end
+  end
+
+  def handle_invalid_image(error_message)
+    Rails.logger.error(error_message)
+    MiniMagick::Image.open(Rails.root.join('public', 'avatars', 'default_avatar.jpg'))
   end
 
   def escape_text(text)