Commit Graph

85 Commits

Author SHA1 Message Date
yuuji.yaginuma
c2ef8bbf52 Fix broken ActiveStorage::BlobTest
`ActiveStorage::Filename#parameters` was removed by #33829.
2018-11-28 10:16:07 +09:00
Rosa Gutierrez
06ab7b27ea Prevent content type and disposition bypass in storage service URLs
* Force content-type to binary on service urls for relevant content types

We have a list of content types that must be forcibly served as binary,
but in practice this only means to serve them as attachment always. We
should also set the Content-Type to the configured binary type.

As a bonus: add text/cache-manifest to the list of content types to be
served as binary by default.

* Store content-disposition and content-type in GCS

Forcing these in the service_url when serving the file works fine for S3
and Azure, since these services include params in the signature.
However, GCS specifically excludes response-content-disposition and
response-content-type from the signature, which means an attacker can
modify these and have files that should be served as text/plain attachments
served as inline HTML for example. This makes our attempt to force
specific files to be served as binary and as attachment can be easily
bypassed.

The only way this can be forced in GCS is by storing
content-disposition and content-type in the object metadata.

* Update GCS object metadata after identifying blob

In some cases we create the blob and upload the data before identifying
the content-type, which means we can't store that in GCS right when
uploading. In these, after creating the attachment, we enqueue a job to
identify the blob, and set the content-type.

In other cases, files are uploaded to the storage service via direct
upload link. We create the blob before the direct upload, which happens
independently from the blob creation itself. We then mark the blob as
identified, but we have already the content-type we need without having
put it in the service.

In these two cases, then, we need to update the metadata in the GCS
service.

* Include content-type and disposition in the verified key for disk service

This prevents an attacker from modifying these params in the service
signed URL, which is particularly important when we want to force them
to have specific values for security reasons.

* Allow only a list of specific content types to be served inline

This is different from the content types that must be served as binary
in the sense that any content type not in this list will be always
served as attachment but with its original content type. Only types in
this list are allowed to be served either inline or as attachment.

Apart from forcing this in the service URL, for GCS we need to store the
disposition in the metadata.

Fix CVE-2018-16477.
2018-11-27 15:36:27 -05:00
Kasper Timm Hansen
ed56a03104
Merge pull request #33829 from mtsmfm/encode-filename
Encode Content-Disposition filenames on send_data and send_file
2018-09-23 19:43:06 +02:00
yuuji.yaginuma
1b86d90136 Enable Performance/UnfreezeString cop
In Ruby 2.3 or later, `String#+@` is available and `+@` is faster than `dup`.

```ruby
# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "benchmark-ips"
end

Benchmark.ips do |x|
  x.report('+@') { +"" }
  x.report('dup') { "".dup }
  x.compare!
end
```

```
$ ruby -v benchmark.rb
ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux]
Warming up --------------------------------------
                  +@   282.289k i/100ms
                 dup   187.638k i/100ms
Calculating -------------------------------------
                  +@      6.775M (± 3.6%) i/s -     33.875M in   5.006253s
                 dup      3.320M (± 2.2%) i/s -     16.700M in   5.032125s

Comparison:
                  +@:  6775299.3 i/s
                 dup:  3320400.7 i/s - 2.04x  slower

```
2018-09-23 08:56:55 +09:00
Fumiaki MATSUSHIMA
890485cfce Encode Content-Disposition filenames on send_data and send_file 2018-09-13 21:38:46 +09:00
Jasper Martin
934fccd522 Ignore ActiveRecord::InvalidForeignKey in ActiveStorage::Blob#purge
Do nothing instead of raising an error when it’s called on an attached blob.
2018-07-26 09:24:31 -04:00
George Claghorn
562ec3dcd1 Test that ActiveStorage::Blob#purge fails when attachments exist 2018-07-20 10:28:14 -04:00
George Claghorn
2ae3a29508 Add a foreign-key constraint to the attachments table for blobs 2018-07-19 20:43:33 -04:00
George Claghorn
af02b9b78f Remove unnecessary tap 2018-07-17 16:23:53 -04:00
George Claghorn
e13e16f5ad Fix replacing many attachments via assign and attach 2018-07-17 09:33:48 -04:00
George Claghorn
379d98dcd4 Correct test name 2018-07-16 15:59:17 -04:00
George Claghorn
36ec5428bf Fix that successive ActiveStorage::Attached::Many#attach calls would overwrite previous attachments 2018-07-16 15:57:43 -04:00
George Claghorn
1d13de4e39 Test removing attachments via #attach 2018-07-16 08:59:23 -04:00
George Claghorn
bd5eba1adf Clear attachment changes on reload 2018-07-13 14:48:45 -04:00
George Claghorn
28db8ba60e Implement ActiveStorage::Attached::{One,Many}#attach in terms of changes 2018-07-13 13:29:33 -04:00
George Claghorn
d20d6c7326 Fix that detaching could purge 2018-07-13 12:17:33 -04:00
George Claghorn
63951072af Fix analyzing new blobs from uploaded files on attach 2018-07-13 06:33:45 -04:00
George Claghorn
0b85123cd8 Raise an ArgumentError instead of a RuntimeError 2018-07-08 11:35:27 -04:00
George Claghorn
e8682c5bf0
Store newly-uploaded files on save rather than assignment 2018-07-07 23:25:33 -04:00
George Claghorn
03afddd2eb Fix that models can clobber each others' attachment reflections
Consider the following model definitions:

    class User < ApplicationRecord
      has_one_attached :avatar
    end

    class Group < ApplicationRecord
      has_one_attached :avatar
    end

If you attempt to reflect on the User model's avatar attachment via User.reflect_on_attachment, you could receive a reflection for the Group model's avatar attachment. Fix this by ensuring that each model class uses its own Hash object to track attachment reflections.
2018-07-07 17:09:31 -04:00
George Claghorn
b21f50d8ae Permit configuring the default service URL expiry 2018-06-21 11:06:32 -04:00
bogdanvlviv
8bc9062aee
Refactor activestorage/test/models/attached_test.rb
Don't include `ActiveJob::TestHelper` since there is no test that uses it.

Ensure removing of overridden User's methods.

Related to https://github.com/rails/rails/pull/33085#issuecomment-395548563
Module#remove_method is private in Ruby 2.4.

Related to fd0bd1bf682622f064ac437ceee4e1b2a6b6d3b9
2018-06-07 23:29:03 +03:00
Rafael França
c1844477a1
Merge pull request #33018 from kddeisz/defined-attachments
ActiveStorage reflection
2018-06-01 14:24:06 -04:00
Kevin Deisz
ce337d1757
Move ActiveStorage reflection logic entirely into ActiveStorage 2018-05-31 09:33:46 -04:00
George Claghorn
bd7ebf61fb Remove errant debugger call 2018-05-30 20:09:30 -04:00
George Claghorn
a6d80e164f Include blob ID in tempfile name for debugging convenience 2018-05-30 20:05:39 -04:00
Kevin Deisz
bc3b6ea461
Reflection for attachments
Add the ability to reflect on the attachments that have been defined using ActiveRecord::Reflection.
2018-05-30 13:28:22 -04:00
George Claghorn
1bdaccc0b8 Verify integrity after chunked download 2018-05-28 16:28:46 -04:00
Javan Makhmali
b60ee86d94 Change video preview format from PNG to JPG 2018-05-23 14:32:34 -04:00
Jacob Smith
0210ac0b43 Disable variant options when false or nil present
In response to https://github.com/rails/rails/issues/32917

In the current implementation, ActiveStorage passes all options to the underlying processor,
including when a key has a value of false.

For example, passing:

```
avatar.variant(resize: "100x100", monochrome: false, flip: "-90")
```

will return a monochrome image (or an error, pending on ImageMagick configuration) because
it passes `-monochrome false` to the command (but the command line does not allow disabling
flags this way, as usually a user would omit the flag entirely to disable that feature).

This fix only passes those keys forward to the underlying processor if the value responds to
`present?`. In practice, this means that `false` or `nil` will be filtered out before going
to the processor.

One possible use case would be for a user to be able to apply different filters to an avatar.
The code might look something like:

```
  variant_options = {
    monochrome: params[:monochrome],
    resize:     params[:resize]
  }

  avatar.variant(*variant_options)
```

Obviously some sanitization may be beneficial in a real-world scenario, but this type of
configuration object could be used in many other places as well.

- Add removing falsy values from varaints to changelog

- The entirety of #image_processing_transformation inject block was wrapped in `list.tap`
 to guard against the default `nil` being returned if no conditional was called.

- add test for explicitly true variant options
2018-05-21 10:38:15 -04:00
George Claghorn
9f95767979 Permit opening a blob in a custom tempdir 2018-05-17 19:14:11 -04:00
Josh Susser
fd0bd1bf68 Generate getter and setter methods in mixin
Generated attachment getter and setter methods are created within
the model's `GeneratedAssociationMethods` module to allow overriding
and composition using `super`.

Includes tests for new functionality.

Co-authored-by: Josh Susser <josh@hasmanythrough.com>
Co-authored-by: Jamon Douglas <terrildouglas@gmail.com>
2018-05-17 14:51:15 -07:00
George Claghorn
ee21b7c2eb Add ActiveStorage::Blob#open
[David Robertson & George Claghorn]
2018-05-16 22:12:31 -04:00
Ryan Davidson
8e98bb7758 Add option to ActiveStorage::Blob to set extract_content_type_from_io
This adds a boolean argument called identify to ActiveStorage::Blob
methods #create_after_upload, #build_after_upload and #upload. It
allows a user to bypass the automatic content_type inference from
the io.
2018-05-08 23:01:57 +01:00
George Claghorn
5f2ee4c0bb Stream blobs from disk in 5 MB chunks
Match other services, which all use a 5 MB chunk size.
2018-04-29 07:07:59 -04:00
Eileen M. Uchitelle
ad0220a71a
Merge pull request #31956 from fatkodima/has_attached-presence-validation
has_(one/many)_attached presence validation
2018-04-27 14:35:21 -04:00
Janko Marohnić
7fc8b6d82c
Show ImageProcessing macros in a dedicated example 2018-04-23 12:21:42 +02:00
Janko Marohnić
f01e249890
Rename ActiveStorage.processor to .variant_processor 2018-04-22 23:40:42 +02:00
Janko Marohnić
ca12968587
Use ImageProcessing gem for ActiveStorage variants
ImageProcessing gem is a wrapper around MiniMagick and ruby-vips, and
implements an interface for common image resizing and processing. This
is the canonical image processing gem recommended in [Shrine], and
that's where it developed from. The initial implementation was extracted
from Refile, which also implements on-the-fly transformations.

Some features that ImageProcessing gem adds on top of MiniMagick:

  * resizing macros
    - #resize_to_limit
    - #resize_to_fit
    - #resize_to_fill
    - #resize_and_pad
  * automatic orientation
  * automatic thumbnail sharpening
  * avoids the complex and inefficient MiniMagick::Image class
  * will use "magick" instead of "convert" on ImageMagick 7

However, the biggest feature of the ImageProcessing gem is that it has
an alternative implementation that uses libvips. Libvips is an
alternative to ImageMagick that can process images very rapidly (we've
seen up 10x faster than ImageMagick).

What's great is that the ImageProcessing gem provides the same interface
for both implementations. The macros are named the same, and the libvips
implementation does auto orientation and thumbnail sharpening as well;
only the operations/options specific to ImageMagick/libvips differ. The
integration provided by this PR should work for both implementations.

The plan is to introduce the ImageProcessing backend in Rails 6.0 as the
default backend and deprecate the MiniMagick backend, then in Rails 6.1
remove the MiniMagick backend.
2018-04-18 17:46:25 +02:00
Andrew White
9436c22e2a
Use a current model to provide the host for service urls
Trying to pass the current request down to the service so that it can
create full urls instead of paths makes the API messy so use a model
based on ActiveSupport::CurrentAttributes to provide the current host
to services that need it (primarily the disk service).
2018-04-06 20:07:52 +01:00
Dwight Watson
8e8f09fa18 Flip the order of the after_create callbacks
Addresses rails/rails#32247

Add test that checks identify and analyze work in correct order

Break out direct upload test helper

Review changes for direct-upload test helper
2018-03-27 12:58:19 +11:00
Nicholas Shirley
f9a5839083 Allow selectively purging attached blobs 2018-03-06 13:03:02 -05:00
George Claghorn
9cc88043e7 Fix purging dependent blobs when attachments aren't loaded 2018-03-05 17:01:31 -05:00
George Claghorn
8228d12a43 Delete dependent attachments with record
[Matt Jones & George Claghorn]
2018-03-05 15:57:52 -05:00
George Claghorn
ccac681122 Generate root-relative paths in Active Storage disk service URL methods
Fixes #32129.
2018-03-05 11:54:43 -05:00
George Claghorn
7e658588fa Handle another case where a blob might be erroneously purged 2018-03-04 18:54:31 -05:00
George Claghorn
4ed4c75c1f Avoid purging attached blob when replacing it with itself 2018-03-04 17:56:45 -05:00
George Claghorn
3915a470d2 Support varying ICO files
Closes #32096.
2018-02-24 15:27:57 -05:00
fatkodima
0c463f50ea Add ActiveStorage::Blob.unattached scope 2018-02-12 22:13:00 +02:00
fatkodima
42259ce904 has_(one/many)_attached presence validation 2018-02-11 17:53:23 +02:00