Automated Test: large-image-processing #330
Conversation
📝 WalkthroughWalkthroughFile upload handling now enforces a fixed 10 MB size limit for client-side validation and URL-based uploads. The uploads controller gained automatic image downsize processing that attempts up to five times to reduce oversized images by 80% increments. The OptimizedImage model refactored its dimension parameter handling from separate width and height arguments to a single combined string format. Changes
Sequence DiagramsequenceDiagram
participant Client
participant UploadsController
participant OptimizedImage
participant FileSystem
Client->>UploadsController: Upload image via URL
UploadsController->>UploadsController: Download file (max 10 MB)
UploadsController->>UploadsController: Check if image exceeds max_image_size_kb
alt Image exceeds size limit
UploadsController->>OptimizedImage: downsize(from, to, dimensions)
loop Up to 5 downsize attempts
OptimizedImage->>OptimizedImage: optimize(downsize, from, to, dimensions)
OptimizedImage->>FileSystem: Reduce image by 80%
OptimizedImage->>UploadsController: Return optimized file
UploadsController->>UploadsController: Check if new size acceptable
end
end
UploadsController->>FileSystem: Store final upload
UploadsController->>Client: Upload complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@app/assets/javascripts/discourse/lib/utilities.js`:
- Around line 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.
In `@app/controllers/uploads_controller.rb`:
- 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).
- Around line 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.
- Around line 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.
In `@app/models/optimized_image.rb`:
- Around line 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.
- Around line 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.
🧹 Nitpick comments (3)
app/assets/javascripts/discourse/lib/utilities.js (1)
182-182: Hardcoded 10 MB limit is duplicated.The
10 * 1024magic number appears here and again at line 246. Consider extracting it as a constant at the top ofDiscourse.Utilities(or module scope) to keep a single source of truth and avoid drift.+// At the top of Discourse.Utilities or file scope: +var MAX_UPLOAD_SIZE_KB = 10 * 1024; // 10 MBThen reference
MAX_UPLOAD_SIZE_KBin bothvalidateUploadedFile(line 182) anddisplayErrorForUpload(line 246).app/models/optimized_image.rb (1)
141-143:resizestill uses the old(from, to, width, height)signature — consider aligning.
resizeat line 141 still takes separatewidthandheightand constructs the dimensions string internally, whiledownsizenow takes a singledimensionsstring. This asymmetry is fine for now sinceresizeis only called internally fromcreate_for(line 49), but consider aligning both methods to the same pattern for consistency.app/controllers/uploads_controller.rb (1)
66-68: Downsizing by "80%" reduces dimensions, not necessarily file size.The
"80%"argument reduces image dimensions to 80% per iteration, but an image can be large in bytes despite small dimensions (e.g., high color-depth, uncompressed formats). The loop could exhaust all 5 attempts without the file size ever dropping belowmax_image_size_kb. This is a known limitation worth documenting, or consider also applying quality/compression reduction as part of the downsize step.
| 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; |
There was a problem hiding this comment.
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.
| 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.
| 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) |
There was a problem hiding this comment.
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
+ endAlso 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.
| 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.
| @@ -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 | |||
There was a problem hiding this comment.
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).
| # 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 |
There was a problem hiding this comment.
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.
| # 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.
| 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 |
There was a problem hiding this comment.
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)
endIf 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.convert_with(instructions, to) | ||
| `#{instructions.join(" ")} &> /dev/null` |
There was a problem hiding this comment.
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
endThis 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.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Changes