Conversation
Support ssrf_filter 1.1
chore: minor refactor from select first to detect
Raise DownloadError when no content is returned
…ptions fix: ruby 2.7 kwarg warning in uploader process
Given that it is not advised to put other stuff in the same directory as CarrierWave's cache, this fix makes clean_cache! not break then. Based on the similar fix for the Fog storage clean_cache!.
Local storage clean_cache! fix for off-topic files
Enable to override configurations by 'false' on a per-uploader basis
Also the call to write_#{column}_identifier doesn't need to be in CarrierWave::ActiveRecord, since what it does is not specific to ActiveRecord.
Fixes #2540
Closes #1962
Since the appropriate wait time depends on the each of the remote server, it is better to be able to configure the wait time on the application side.
Support to configure download retry wait time
By providing a 'changelog_uri' in the metadata of the gemspec a 'Changelog' link will be shown on https://rubygems.org/gems/carrierwave which makes it quick and easy for someone to check on the changes introduced with a new version. Details of this functionality can be found on https://guides.rubygems.org/specification-reference/#metadata
Adds a commented example of `content_type_allowlist` (/image\//) to the generated uploader. It mirrors the existing `extension_allowlist` comment and helps users mitigate content-type spoofing attacks.
Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 5. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...v5) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Fix "Lint/Void: Variable storage used in void context." The current logic does nothing. So just add a comment to fix the linter. * Avoid to test against JRuby 10.1.x It seems that activerecord-jdbc-adapter doesn't work with the latest version.
Bumps [actions/checkout](https://github.com/actions/checkout) from 5 to 6. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v5...v6) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/checkout](https://github.com/actions/checkout) from 6 to 7. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v6...v7) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
…ctions/actions/checkout-7 Bump actions/checkout from 6 to 7
* upgrade marcel gem currently locked to '~> 1.0.0', lock to '~> 1.0' * Bump actions/checkout from 6 to 7 Bumps [actions/checkout](https://github.com/actions/checkout) from 6 to 7. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v6...v7) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> * loosen marcel restriction * fix issue from upgraded MiniMagick --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: RobL <contact@robl.me> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…te!` (#2817) Previously, when `manipulate!` was called with a block that returned nil for every frame, the resulting empty frames array caused an `ArgumentError` in frames.append(true)`. This is because RMagick's `ImageList#append` does not accept a bare true value on an empty list. https://github.com/rmagick/rmagick/blob/9788a6d43b3aebb5a1ad72013f1fe0b3affa300f/ext/RMagick/rmilist.cpp#L143 https://github.com/rmagick/rmagick/blob/9788a6d43b3aebb5a1ad72013f1fe0b3affa300f/ext/RMagick/rmilist.cpp#L877 This made the error hard to diagnose since the root cause (unsupported format or a block that always returns nil) was not surfaced. It looks like this happens when an image file isn't supported by ImageMagick. (In our case, this happens with HEIC files with unsupported compatible brands) So this changed to check frames and if it's empty, raises an appropriate error.
#process_uri decoded the path and re-encoded it, which loses the distinction between %2F and '/', and between %2B and '+'. Every fix in this area has so far had to pick which of the two to break: a literal + in a path became %2B and 404'd (#2505, #2590, #2800), %2F used as part of an object name was decoded into a path separator (#2808), and percent-encoded sub-delims in a signed URL were decoded, invalidating the signature (#858). Escape only what cannot appear in each component instead, leaving existing %XX untouched. The operation is then idempotent and leaves an already valid URL unchanged, so those shapes no longer have to be traded off against each other. Split the three operations that #encode_path was serving at once: encoding raw data into a component, decoding a component back for display, and repairing a string that is already a URI. Only the second may decode, so the filename taken from a URL now uses URI semantics rather than CGI.unescape, which is for form encoding and turns '+' into a space. Also strip whitespace and newlines left over from pasting and supply a missing scheme, as remote_#{column}_url= is meant to be fed from a text field. Fixes #2800 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bumps the JRuby build from 10.0.5.0 to the latest 10.1.1.0 release. It stays on Rails 7.2, since activerecord-jdbc-adapter has no stable release supporting Rails 8.x yet (only an 80.0.pre1 prerelease); pointing it at 8.1 made bundler silently resolve down to the ancient 1.3.22, which calls alias_method_chain and blows up loading ActiveRecord. The previous Rails 8.0 combination is kept in the matrix, tested against Ruby 4.0. Also drops the stale "~> 1.0" sqlite3 pin in the root Gemfile, added back to fix Rails 7 builds when that Gemfile always resolved to whatever Rails was latest at the time. Now that latest Rails is 8.x, which requires sqlite3 >= 2.1, the stale pin broke activerecord_spec instead. gemfiles/rails-7-2.gemfile and newer already leave sqlite3 unpinned; do the same here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
) * Simplify tests around use of `Open3` within `mini_magick` Throughout their history they were ultimately calling either `popen3` or `capture3`, and it has been noted that `capture3` internally calls `popen3` within the standard library, so we only need to stub at the lowest level to cover all versions. * Relax dependency on image_processing to allow updates to `~> 2.0` Add `ruby-vips` as a dependency for testing because `image_processing` no longer hard-requires it. --------- Co-authored-by: Amir Yalon <git@please.nospammail.net>
ActiveStorage on Rails main now requires marcel `~> 2.0`, which conflicts with our `< 2` pin and breaks bundle install on the rails-main build. Relax the constraint to `< 3`. On Ruby < 3.3 (and JRuby) Bundler keeps resolving marcel 1.x, since marcel 2.0 requires Ruby >= 3.3. marcel 2 also validates the declared MIME type against the RFC grammar instead of splitting on the first `;`, `,` or space, so a header carrying more than one type (e.g. "image/png; text/html") now falls back to application/octet-stream. Extract the first media type ourselves before handing it to Marcel to preserve the previous behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Caching to a remote storage costs a PUT, a COPY and a DELETE per file, repeated for every version. Only the PUT carries the bytes; the COPY and the DELETE exist solely to move the object from the cache key to the store key, and are pure overhead whenever the record is saved right away, which is the common case. The server-side copy is not free either, having been reported as the dominant cost on OpenStack (#2605). The cache defaulted to local storage until 629afec made it follow #storage, to avoid the confusion of having to configure the two separately (#2353), knowingly accepting that overhead in return. Deferring the upload removes the need to choose between the two: Storage::Fog now stages the file in the local cache and uploads it only when it turns out to be needed beyond the current request, which is when its cache name is read to be carried over by a form, or its URL is generated to be fetched by a browser. The happy path becomes a single PUT straight to the store, while redisplaying a form after a validation error keeps working across servers exactly as before. No configuration changes: storages which have nothing to defer are unaffected, as Storage::Abstract#materialize_cache! returns the file it is given. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Uploader#blank? delegated to the file, whose #empty? costs a HEAD request on a remote storage. ActiveSupport implements #present? as !#blank?, so anything Rails-idiomatic reached the network: `user.avatar?`, `user.avatar.present?`, `validates :avatar, presence: true`, serializers, view guards, once per record on a page listing many of them (#2802, #2776, #2784). CarrierWave::Storage::Fog::File#empty? was added in 3.1.0 to make presence mean the same thing on both storages (#1926), since SanitizedFile#empty? checks the filesystem. That aligned them on the expensive reading. Two questions were being conflated: whether a file is assigned, which the identifier already answers for free, and whether it is actually in the storage, which nothing but the storage can answer. Split them: #blank? and #present? answer the first, and the new #exists? answers the second. A cached file is still verified, as a cache name can point at a cache which is gone and the check is local either way. A version now reports itself present whenever its parent is, which #1926 relied on to tell whether a conditional version had been created; #exists? answers that explicitly. Also fix two ways the remote storage was asked more than it had to be: directory.files.head was memoized with ||=, so a file which is not there was looked up again on every access (#2698, #2793), and #size called #zero? on a content length the storage may not report (#2787). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Retrieving an uploader ran the :if/:unless condition of every version right away, to decide which ones to retrieve. The conditions receive the file, and they exist precisely to look at it, so a condition on the content type or the dimensions costs a HEAD request on a remote storage, or an ImageMagick call, once per record. It is paid by anything that merely touches the mounted column, whether or not a version is ever used (#2461, #2132). Nothing forces that work to happen at retrieval time. Retrieval itself is free, and the conditions are answered against the same file whenever they are asked. Record the retrieval instead and run it on the first access to #versions, which every path to a version goes through (#2669). Reading only the original file now touches the storage no more than it has to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only the identifier is persisted, and everything else about a stored file is worked out again on retrieval, by re-running the uploader's definition and by asking the storage. Some of it cannot be worked out correctly. Which versions a conditional creates is settled when the file is stored, and the conditions receive the file precisely so they can look at it, so re-deciding later gives whatever the current answer is: a version that exists loses its URL, or one that was never created gets a URL leading to a 404 (#2148, #1926). The rest of it is merely expensive, costing a request per record on a remote storage. Give the mount a column to record it in, and the facts are read back rather than guessed: mount_uploader :image, ImageUploader, metadata_column: :image_metadata The uploader collects them in #build_metadata when the file is about to be stored, which is the moment they are true, and it is overridable so that uploaders can record their own. Which versions were created, their filenames, the size and the content type are recorded by default; #version_active?, #size, #content_type and #store_path consult them instead of working them out. Nothing changes for those who do not name a column, and records stored before one was added have nothing recorded and keep behaving as they did, so no backfill is needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`process convert: format` sets force_extension on the class, from the declaration alone, while whether the conversion runs is decided per file by the condition. A JPEG which the condition spared was still stored under a .webp name (#2723). Set the extension where the decision is made, in #process!, so it follows what actually happened. The class-level one stays for an unconditional conversion, where the declaration is the fact and can be relied on to work the extension out again on retrieval. A conditional one cannot, so it has to survive to the request the file is retrieved in, where nothing re-runs the processing. Both ends of the trip carry it now instead of deriving it: the cache name, so a redisplayed form finds the cached file, and the recorded filename, so retrieval from the store finds the stored one. This is why the case could not be fixed on its own, and why the mount now refuses an uploader which converts conditionally unless it is given a metadata_column to record the result in. The mount is where that can be told, having both the uploader and the options, and it fails at boot rather than on a save. What it cannot see is a version's processors, which are only built once a version is used, so those warn once per uploader when the conversion runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.