Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/assets/javascripts/discourse/lib/utilities.js
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ Discourse.Utilities = {

// check file size
var fileSizeKB = file.size / 1024;
var maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb'];
var maxSizeKB = 10 * 1024; // 10MB
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Client validation ignores site configuration.

Hardcoding maxSizeKB to 10 * 1024 ignores the site's configured max_image_size_kb or max_attachment_size_kb settings. Since the feature downsizes large images server-side, the client validation should allow larger uploads or match the server's actual limit, not enforce an arbitrary 10MB cap that may conflict with site configuration.

Fix in Cursor Fix in Web

if (fileSizeKB > maxSizeKB) {
bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));
return false;
Expand Down Expand Up @@ -243,7 +243,7 @@ Discourse.Utilities = {

// entity too large, usually returned from the web server
case 413:
var maxSizeKB = Discourse.SiteSettings.max_image_size_kb;
var maxSizeKB = 10 * 1024; // 10 MB
bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));
return;

Expand Down
11 changes: 10 additions & 1 deletion app/controllers/uploads_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,23 @@ def create_upload(type, file, url)
begin
# API can provide a URL
if file.nil? && url.present? && is_api?
tempfile = FileHelper.download(url, SiteSetting.max_image_size_kb.kilobytes, "discourse-upload-#{type}") rescue nil
tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") rescue nil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Hardcoded Limits Bypass Admin Control

Hardcoding the download limit to 10.megabytes ignores SiteSetting.max_image_size_kb for API-provided URLs. This creates inconsistent behavior where the configured site limit is bypassed, and may allow downloads that exceed or unnecessarily restrict what the site administrator intended.

Fix in Cursor Fix in Web

filename = File.basename(URI.parse(url).path)
else
tempfile = file.tempfile
filename = file.original_filename
content_type = file.content_type
end

# allow users to upload large images that will be automatically reduced to allowed size
if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename)
attempt = 5
while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes
OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
attempt -= 1
end
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Infinite Downsizing Loop for Oversized Images

The downsizing loop always runs exactly 5 times for oversized images because tempfile.size reflects the in-memory object's size attribute, not the actual file size on disk after OptimizedImage.downsize modifies it. The loop condition tempfile.size > SiteSetting.max_image_size_kb.kilobytes never becomes false, causing unnecessary repeated downsizing to approximately 33% of original size (0.8^5) regardless of when the target size is reached.

Fix in Cursor Fix in Web


upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type)

if upload.errors.empty? && current_user.admin?
Expand Down
17 changes: 8 additions & 9 deletions app/models/optimized_image.rb
Original file line number Diff line number Diff line change
Expand Up @@ -139,25 +139,24 @@ def self.downsize_instructions_animated(from, to, dimensions, opts={})
end

def self.resize(from, to, width, height, opts={})
optimize("resize", from, to, width, height, opts)
optimize("resize", from, to, "#{width}x#{height}", opts)
end

def self.downsize(from, to, max_width, max_height, opts={})
optimize("downsize", from, to, max_width, max_height, opts)
optimize("downsize", from, to, "#{max_width}x#{max_height}", opts)
end

def self.optimize(operation, from, to, width, height, opts={})
dim = dimensions(width, height)
def self.downsize(from, to, dimensions, opts={})
optimize("downsize", from, to, dimensions, opts)
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Method Overload Conflict Breaks Downsize

Ruby doesn't support method overloading, so defining downsize twice causes the second definition to completely replace the first. The 4-parameter version downsize(from, to, max_width, max_height, opts={}) is lost, breaking existing callers like resize_emoji.rb that pass separate width and height values. The methods need different names or a single method that handles both parameter patterns.

Fix in Cursor Fix in Web


def self.optimize(operation, from, to, dimensions, opts={})
method_name = "#{operation}_instructions"
method_name += "_animated" if !!opts[:allow_animation] && from =~ /\.GIF$/i
instructions = self.send(method_name.to_sym, from, to, dim, opts)
instructions = self.send(method_name.to_sym, from, to, dimensions, opts)
convert_with(instructions, to)
end

def self.dimensions(width, height)
"#{width}x#{height}"
end

def self.convert_with(instructions, to)
`#{instructions.join(" ")} &> /dev/null`
return false if $?.exitstatus != 0
Expand Down