Skip to content
Jump to bottom

Automated Test: large-image-processing #330

Closed
wants to merge 1 commit into from
+20 −12
Closed

Automated Test: large-image-processing #330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/assets/javascripts/discourse/lib/utilities.js
View file
Open in desktop
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
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;
Comment on lines 245 to 248
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

var declaration inside a switch clause leaks to sibling cases.

As flagged by Biome (noSwitchDeclarations), declaring var maxSizeKB without a block scope inside case 413 makes it accessible to other cases (including fall-through). Wrap the case body in braces.

Proposed fix 
         case 413:
-          var maxSizeKB = 10 * 1024; // 10 MB
-          bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));
-          return;
+        {
+          var maxSizeKB = 10 * 1024; // 10 MB
+          bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));
+          return;
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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;
case 413:
{
var maxSizeKB = 10 * 1024; // 10 MB
bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));
return;
}
🧰 Tools
🪛 Biome (2.3.13)

[error] 246-246: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

The declaration is defined in this switch clause:

Safe fix: Wrap the declaration in a block.

(lint/correctness/noSwitchDeclarations)

🤖 Prompt for AI Agents 
In `@app/assets/javascripts/discourse/lib/utilities.js` around lines 245 - 248,
The declaration "var maxSizeKB" inside the "case 413" clause leaks to sibling
cases; fix by making the case body block-scoped (e.g., wrap the case 413
statements in { ... } or replace var with let/const) so maxSizeKB is not
accessible outside that case; update the code around the case 413 handling that
calls bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb:
maxSizeKB })) to use the block-scoped variable.


Expand Down
11 changes: 10 additions & 1 deletion app/controllers/uploads_controller.rb
View file
Open in desktop
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.

⚠️ Potential issue | 🔴 Critical

NilClass crash when URL download fails.

If the URL download at line 55 fails (tempfile = nil), execution reaches line 72 which calls tempfile.size. This will raise NoMethodError: undefined method 'size' for nil:NilClass.

Proposed guard 
+      if tempfile.nil?
+        return { errors: ["Failed to download image from URL."] }
+      end
+
       # allow users to upload large images...

Place this guard after line 61, before the downsizing logic.

Also applies to: 72-72

🧰 Tools
🪛 RuboCop (1.84.0)

[convention] 55-55: Avoid using rescue in its modifier form.

(Style/RescueModifier)

🤖 Prompt for AI Agents 
In `@app/controllers/uploads_controller.rb` at line 55, The download can return
nil causing a NoMethodError when calling tempfile.size; after calling
FileHelper.download (tempfile) and before the downsizing logic (i.e., after the
block that checks tempfile and before any use of tempfile.size), add a nil guard
that handles the failed download: log or set an appropriate error/response and
return/raise to stop processing. Reference the tempfile variable and the
FileHelper.download call so the guard prevents further use of tempfile (e.g.,
before any downsizing code that reads tempfile.size).

filename = File.basename(URI.parse(url).path)
Comment on lines 54 to 56
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

rescue nil silently swallows all download errors.

If FileHelper.download raises (network timeout, invalid URL, DNS failure, etc.), the error is swallowed and tempfile becomes nil. The code proceeds to line 72 where tempfile.size will raise a NoMethodError on nil.

At minimum, log the exception so failed URL uploads are diagnosable:

Proposed fix 
-        tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") rescue nil
+        begin
+          tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}")
+        rescue => e
+          Rails.logger.warn("Failed to download upload from URL: #{e.message}")
+          tempfile = nil
+        end

Also note: when tempfile is nil (download failed), execution falls through to line 72 which calls tempfile.size — this will raise NoMethodError. A guard (return or early error response) is needed when tempfile is nil after the URL download path.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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
filename = File.basename(URI.parse(url).path)
if file.nil? && url.present? && is_api?
begin
tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}")
rescue => e
Rails.logger.warn("Failed to download upload from URL: #{e.message}")
tempfile = nil
end
filename = File.basename(URI.parse(url).path)
🧰 Tools
🪛 RuboCop (1.84.0)

[convention] 55-55: Avoid using rescue in its modifier form.

(Style/RescueModifier)

🤖 Prompt for AI Agents 
In `@app/controllers/uploads_controller.rb` around lines 54 - 56, The code in
uploads_controller.rb silently swallows exceptions from FileHelper.download (via
`rescue nil`), which can make `tempfile` nil and later cause a NoMethodError
when calling `tempfile.size`; change the download handling to explicitly rescue
exceptions from FileHelper.download, log the exception (using the controller
logger or a suitable logger) including the URL and error message, and after a
failed download return an appropriate API error response or early return when
`tempfile` is nil; update the block around FileHelper.download and ensure
subsequent use of `tempfile` (e.g., `tempfile.size`) is guarded by this early
return.

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
Comment on lines +63 to +70
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Downsize return value is unchecked — failed attempts loop uselessly and may corrupt the file.

OptimizedImage.downsize returns false on failure, but the return value is ignored. If the first attempt fails (unsupported format, missing binary, etc.), the remaining iterations are wasted — or worse, a partially-written in-place file could be fed back into subsequent attempts.

Additionally, downsizing in-place (tempfile.path → tempfile.path) is risky: a failed conversion could leave a truncated or corrupted file. Consider writing to a separate temp file and swapping on success.

Proposed fix 
       # 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
+          downsized = OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
+          break unless downsized
+          tempfile.rewind
+          attempt -= 1
         end
       end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# 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
# 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
downsized = OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
break unless downsized
tempfile.rewind
attempt -= 1
end
end
🤖 Prompt for AI Agents 
In `@app/controllers/uploads_controller.rb` around lines 63 - 70, The loop
currently calls OptimizedImage.downsize(tempfile.path, tempfile.path, ...) and
ignores its boolean return, risking wasted retries and in-place corruption;
change it to call OptimizedImage.downsize with a distinct temporary output path
(not tempfile.path), check the returned boolean each iteration, and only
replace/swap the original tempfile with the new file when downsize returns true;
on a false return break the loop and log/handle the failure so you don't
continue attempting further in-place downsizes; keep using
FileHelper.is_image?(filename) and SiteSetting.max_image_size_kb conditions, and
ensure attempt decrements and the loop exits on success or failure.


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
View file
Open in desktop
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
Comment on lines 145 to +151
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Duplicate downsize definition — the 4-arg overload at line 145 is dead code and any old callers will break.

Ruby does not support method overloading. The second def self.downsize (line 149) silently replaces the first (line 145), making lines 145-147 unreachable. Worse, any existing caller still using the old 4-argument signature downsize(from, to, max_width, max_height) will now hit the 3-arg method, causing max_height to be interpreted as opts. Since it's an integer, not a hash, this will produce a TypeError or silently wrong behavior wherever opts is accessed as a hash (e.g., opts[:force_aspect_ratio] in downsize_instructions).

Remove the old overload:

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

If any callers still rely on the 4-arg form, they must be migrated to pass "#{w}x#{h}" explicitly.

#!/bin/bash
# Search for all call sites of OptimizedImage.downsize to verify none still use the old 4-arg signature
rg -n 'OptimizedImage\.downsize' --type ruby -C 3
🧰 Tools
🪛 RuboCop (1.84.0)

[warning] 149-149: Method OptimizedImage.downsize is defined at both app/models/optimized_image.rb:145 and app/models/optimized_image.rb:149.

(Lint/DuplicateMethods)

🤖 Prompt for AI Agents 
In `@app/models/optimized_image.rb` around lines 145 - 151, Remove the duplicate
4-argument definition of self.downsize and keep the single signature def
self.downsize(from, to, dimensions, opts={}) so the method expects a dimensions
string and opts hash; delete the older def self.downsize(from, to, max_width,
max_height, opts={}) and update any call sites of OptimizedImage.downsize that
pass (from, to, max_width, max_height) to instead pass dimensions as
"#{max_width}x#{max_height}" and an opts hash (or rely on the default), and
ensure callers no longer pass an Integer where opts is expected so
downsize_instructions and opts[:force_aspect_ratio] work correctly.


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`
Comment on lines 160 to 161
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Shell command injection risk via instructions.join(" ").

The backtick invocation on line 161 joins user-influenced strings (file paths, dimensions) into a shell command. If any argument contains shell metacharacters (e.g., a crafted filename with ; rm -rf /), it could lead to command injection. Consider using a safer invocation such as Open3.capture2e(*instructions) or system(*instructions) which bypasses shell interpretation when given an array.

Proposed safer alternative 
  def self.convert_with(instructions, to)
-   `#{instructions.join(" ")} &> /dev/null`
-   return false if $?.exitstatus != 0
+   _output, status = Open3.capture2e(*instructions)
+   return false unless status.success?

    ImageOptim.new.optimize_image!(to)
    true
  rescue
    Rails.logger.error("Could not optimize image: #{to}")
    false
  end

This would require require "open3" at the top of the file.

🧰 Tools
🪛 Brakeman (8.0.1)

[medium] 161-161: Possible command injection
Type: Command Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/command_injection/

(Command Injection)

🤖 Prompt for AI Agents 
In `@app/models/optimized_image.rb` around lines 160 - 161, The convert_with
method currently builds a shell string with instructions.join(" ") and executes
it via backticks, allowing shell injection; change it to execute the command
without invoking a shell by using a safe API (for example require "open3" and
call Open3.capture2e(*instructions) or use system(*instructions)) so arguments
are passed as an argv array rather than interpolated into a shell string; update
convert_with to return/use the result from the safe call and add the require
"open3" at the top of optimized_image.rb.

return false if $?.exitstatus != 0
Expand Down