Skip to content

Commit

Permalink
Merge branch 'master' into rhymes/add-vips
Browse files Browse the repository at this point in the history
  • Loading branch information
rhymes committed Feb 21, 2021
2 parents fd48067 + f90e14c commit aaa52b8
Show file tree
Hide file tree
Showing 9 changed files with 77 additions and 48 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ jobs:
- ruby: jruby-head
gemfile: gemfiles/rails-6-0.gemfile
experimental: true
runs-on: ubuntu-16.04
runs-on: ubuntu-20.04
services:
postgres:
image: postgres:11
Expand Down
32 changes: 16 additions & 16 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ a migration:
Open your model file and mount the uploader:

```ruby
class User < ActiveRecord::Base
class User < ApplicationRecord
mount_uploader :avatar, AvatarUploader
end
```
Expand Down Expand Up @@ -157,7 +157,7 @@ Open your model file and mount the uploader:


```ruby
class User < ActiveRecord::Base
class User < ApplicationRecord
mount_uploaders :avatars, AvatarUploader
serialize :avatars, JSON # If you use SQLite, add this line.
end
Expand Down Expand Up @@ -230,15 +230,15 @@ end
## Securing uploads

Certain files might be dangerous if uploaded to the wrong location, such as PHP
files or other script files. CarrierWave allows you to specify a whitelist of
files or other script files. CarrierWave allows you to specify an allowlist of
allowed extensions or content types.

If you're mounting the uploader, uploading a file with the wrong extension will
make the record invalid instead. Otherwise, an error is raised.

```ruby
class MyUploader < CarrierWave::Uploader::Base
def extension_whitelist
def extension_allowlist
%w(jpg jpeg gif png)
end
end
Expand All @@ -249,45 +249,45 @@ Let's say we need an uploader that accepts only images. This can be done like th

```ruby
class MyUploader < CarrierWave::Uploader::Base
def content_type_whitelist
def content_type_allowlist
/image\//
end
end
```

You can use a blacklist to reject content types.
You can use a denylist to reject content types.
Let's say we need an uploader that reject JSON files. This can be done like this

```ruby
class NoJsonUploader < CarrierWave::Uploader::Base
def content_type_blacklist
def content_type_denylist
['application/text', 'application/json']
end
end
```

### CVE-2016-3714 (ImageTragick)
This version of CarrierWave has the ability to mitigate CVE-2016-3714. However, you **MUST** set a content_type_whitelist in your uploaders for this protection to be effective, and you **MUST** either disable ImageMagick's default SVG delegate or use the RSVG delegate for SVG processing.
This version of CarrierWave has the ability to mitigate CVE-2016-3714. However, you **MUST** set a content_type_allowlist in your uploaders for this protection to be effective, and you **MUST** either disable ImageMagick's default SVG delegate or use the RSVG delegate for SVG processing.


A valid whitelist that will restrict your uploader to images only, and mitigate the CVE is:
A valid allowlist that will restrict your uploader to images only, and mitigate the CVE is:

```ruby
class MyUploader < CarrierWave::Uploader::Base
def content_type_whitelist
def content_type_allowlist
[/image\//]
end
end
```

**WARNING**: A `content_type_whitelist` is the only form of whitelist or blacklist supported by CarrierWave that can effectively mitigate against CVE-2016-3714. Use of `extension_whitelist` will not inspect the file headers, and thus still leaves your application open to the vulnerability.
**WARNING**: A `content_type_allowlist` is the only form of allowlist or denylist supported by CarrierWave that can effectively mitigate against CVE-2016-3714. Use of `extension_allowlist` will not inspect the file headers, and thus still leaves your application open to the vulnerability.

### Filenames and unicode chars

Another security issue you should care for is the file names (see
[Ruby On Rails Security Guide](http://guides.rubyonrails.org/security.html#file-uploads)).
By default, CarrierWave provides only English letters, arabic numerals and some symbols as
white-listed characters in the file name. If you want to support local scripts (Cyrillic letters, letters with diacritics and so on), you
allowlisted characters in the file name. If you want to support local scripts (Cyrillic letters, letters with diacritics and so on), you
have to override `sanitize_regexp` method. It should return regular expression which would match
all *non*-allowed symbols.

Expand Down Expand Up @@ -980,10 +980,10 @@ errors:
carrierwave_processing_error: failed to be processed
carrierwave_integrity_error: is not of an allowed file type
carrierwave_download_error: could not be downloaded
extension_whitelist_error: "You are not allowed to upload %{extension} files, allowed types: %{allowed_types}"
extension_blacklist_error: "You are not allowed to upload %{extension} files, prohibited types: %{prohibited_types}"
content_type_whitelist_error: "You are not allowed to upload %{content_type} files, allowed types: %{allowed_types}"
content_type_blacklist_error: "You are not allowed to upload %{content_type} files"
extension_allowlist_error: "You are not allowed to upload %{extension} files, allowed types: %{allowed_types}"
extension_denylist_error: "You are not allowed to upload %{extension} files, prohibited types: %{prohibited_types}"
content_type_allowlist_error: "You are not allowed to upload %{content_type} files, allowed types: %{allowed_types}"
content_type_denylist_error: "You are not allowed to upload %{content_type} files"
rmagick_processing_error: "Failed to manipulate with rmagick, maybe it is not an image?"
mini_magick_processing_error: "Failed to manipulate with MiniMagick, maybe it is not an image? Original Error: %{e}"
min_size_error: "File size should be greater than %{min_size}"
Expand Down
2 changes: 1 addition & 1 deletion lib/carrierwave/storage/fog.rb
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ def authenticated_url(options = {})
# avoid a get by using local references
local_directory = connection.directories.new(:key => @uploader.fog_directory)
local_file = local_directory.files.new(:key => path)
expire_at = options[:expire_at] || ::Fog::Time.now + @uploader.fog_authenticated_url_expiration
expire_at = options[:expire_at] || ::Fog::Time.now.since(@uploader.fog_authenticated_url_expiration.to_i)
case @uploader.fog_credentials[:provider]
when 'AWS', 'Google'
# Older versions of fog-google do not support options as a parameter
Expand Down
4 changes: 2 additions & 2 deletions lib/generators/templates/uploader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ def store_dir
# process resize_to_fit: [50, 50]
# end
# Add a white list of extensions which are allowed to be uploaded.
# Add an allowlist of extensions which are allowed to be uploaded.
# For images you might use something like this:
# def extension_whitelist
# def extension_allowlist
# %w(jpg jpeg gif png)
# end
Expand Down
12 changes: 6 additions & 6 deletions spec/mount_multiple_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,10 @@ def rotate
instance.images = [test_file_stub]
end

context "if the images fails a white list integrity check" do
context "if the images fails an allowlist integrity check" do
before do
uploader.class_eval do
def extension_whitelist
def extension_allowlist
%w(txt)
end
end
Expand All @@ -218,10 +218,10 @@ def extension_whitelist
end
end

describe "if the images fails a black list integrity check" do
describe "if the images fails a denylist integrity check" do
before do
uploader.class_eval do
def extension_blacklist
def extension_denylist
%w(jpg)
end
end
Expand Down Expand Up @@ -715,7 +715,7 @@ def monkey
describe "when an integrity check fails" do
before do
uploader.class_eval do
def extension_whitelist
def extension_allowlist
%w(txt)
end
end
Expand Down Expand Up @@ -965,7 +965,7 @@ def fish
let(:uploader) do
Class.new(CarrierWave::Uploader::Base).tap do |u|
u.class_eval do
def extension_whitelist
def extension_allowlist
%w(txt)
end
end
Expand Down
12 changes: 6 additions & 6 deletions spec/mount_single_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -145,19 +145,19 @@ def rotate
@instance.image = stub_file('test.jpg')
end

it "should fail silently if the image fails a white list integrity check" do
it "should fail silently if the image fails an allowlist integrity check" do
@uploader.class_eval do
def extension_whitelist
def extension_allowlist
%w(txt)
end
end
@instance.image = stub_file('test.jpg')
expect(@instance.image).to be_blank
end

it "should fail silently if the image fails a black list integrity check" do
it "should fail silently if the image fails a denylist integrity check" do
@uploader.class_eval do
def extension_blacklist
def extension_denylist
%w(jpg)
end
end
Expand Down Expand Up @@ -462,7 +462,7 @@ def default_url
describe "when an integrity check fails" do
before do
@uploader.class_eval do
def extension_whitelist
def extension_allowlist
%w(txt)
end
end
Expand Down Expand Up @@ -695,7 +695,7 @@ def fish
@instance = @class.new

@uploader.class_eval do
def extension_whitelist
def extension_allowlist
%w(txt)
end
end
Expand Down
32 changes: 16 additions & 16 deletions spec/orm/activerecord_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -246,10 +246,10 @@ def reset_class(class_name)
end


context 'when validating white list integrity' do
context 'when validating allowlist integrity' do
before do
@uploader.class_eval do
def extension_whitelist
def extension_allowlist
%w(txt)
end
end
Expand All @@ -259,10 +259,10 @@ def extension_whitelist
# Localize the error message to Dutch
change_locale_and_store_translations(:nl, :errors => {
:messages => {
:extension_whitelist_error => "Het opladen van %{extension} bestanden is niet toe gestaan. Geaccepteerde types: %{allowed_types}"
:extension_allowlist_error => "Het opladen van %{extension} bestanden is niet toe gestaan. Geaccepteerde types: %{allowed_types}"
}
}) do
# Assigning image triggers check_whitelist! and thus should be inside change_locale_and_store_translations
# Assigning image triggers check_allowlist! and thus should be inside change_locale_and_store_translations
@event.image = stub_file('test.jpg')
expect(@event).to_not be_valid
@event.valid?
Expand All @@ -271,10 +271,10 @@ def extension_whitelist
end
end

context 'when validating black list integrity' do
context 'when validating denylist integrity' do
before do
@uploader.class_eval do
def extension_blacklist
def extension_denylist
%w(jpg)
end
end
Expand All @@ -284,10 +284,10 @@ def extension_blacklist
# Localize the error message to Dutch
change_locale_and_store_translations(:nl, :errors => {
:messages => {
:extension_blacklist_error => "You are not allowed to upload %{extension} files, prohibited types: %{prohibited_types}"
:extension_denylist_error => "You are not allowed to upload %{extension} files, prohibited types: %{prohibited_types}"
}
}) do
# Assigning image triggers check_blacklist! and thus should be inside change_locale_and_store_translations
# Assigning image triggers check_denylist! and thus should be inside change_locale_and_store_translations
@event.image = stub_file('test.jpg')
expect(@event).to_not be_valid
@event.valid?
Expand Down Expand Up @@ -1041,10 +1041,10 @@ def filename
expect(@event.images).to be_empty
end

context 'when validating white list integrity' do
context 'when validating allowlist integrity' do
before do
@uploader.class_eval do
def extension_whitelist
def extension_allowlist
%w(txt)
end
end
Expand All @@ -1054,10 +1054,10 @@ def extension_whitelist
# Localize the error message to Dutch
change_locale_and_store_translations(:nl, :errors => {
:messages => {
:extension_whitelist_error => "Het opladen van %{extension} bestanden is niet toe gestaan. Geaccepteerde types: %{allowed_types}"
:extension_allowlist_error => "Het opladen van %{extension} bestanden is niet toe gestaan. Geaccepteerde types: %{allowed_types}"
}
}) do
# Assigning images triggers check_whitelist! and thus should be inside change_locale_and_store_translations
# Assigning images triggers check_allowlist! and thus should be inside change_locale_and_store_translations
@event.images = [stub_file('test.jpg')]
expect(@event).to_not be_valid
@event.valid?
Expand All @@ -1066,10 +1066,10 @@ def extension_whitelist
end
end

context 'when validating black list integrity' do
context 'when validating denylist integrity' do
before do
@uploader.class_eval do
def extension_blacklist
def extension_denylist
%w(jpg)
end
end
Expand All @@ -1079,10 +1079,10 @@ def extension_blacklist
# Localize the error message to Dutch
change_locale_and_store_translations(:nl, :errors => {
:messages => {
:extension_blacklist_error => "You are not allowed to upload %{extension} files, prohibited types: %{prohibited_types}"
:extension_denylist_error => "You are not allowed to upload %{extension} files, prohibited types: %{prohibited_types}"
}
}) do
# Assigning images triggers check_blacklist! and thus should be inside change_locale_and_store_translations
# Assigning images triggers check_denylist! and thus should be inside change_locale_and_store_translations
@event.images = [stub_file('test.jpg')]
expect(@event).to_not be_valid
@event.valid?
Expand Down
1 change: 1 addition & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
require "webmock/rspec"
require 'mini_magick'
require "vips"
require 'active_support/core_ext'

I18n.enforce_available_locales = false

Expand Down
28 changes: 28 additions & 0 deletions spec/storage/fog_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,34 @@ def check_file
end
end

context 'in a timezone with DST' do
before do
@prev_tz = ENV['TZ']
ENV['TZ'] = 'US/Pacific'
end
after { ENV['TZ'] = @prev_tz }

it "should generate a proper X-Amz-Expires when expires spans a move to DST" do
if @provider == 'AWS'
Timecop.freeze(Time.at(1477932000)) do |now|
expiration = 7 * 24 * 60 * 60 # 1 week
allow(@uploader).to receive(:fog_authenticated_url_expiration).and_return(expiration)
expect(@fog_file.authenticated_url).to include("X-Amz-Expires=#{expiration.to_s}")
end
end
end

it "should generate a proper X-Amz-Expires when expires spans a move to DST and an ActiveRecord::Duration is provided" do
if @provider == 'AWS'
Timecop.freeze(Time.at(1477932000)) do |now|
expiration = 1.week
allow(@uploader).to receive(:fog_authenticated_url_expiration).and_return(expiration)
expect(@fog_file.authenticated_url).to include("X-Amz-Expires=#{expiration.to_s}")
end
end
end
end

it 'should generate correct filename' do
expect(@fog_file.filename).to eq('private.txt')
end
Expand Down

0 comments on commit aaa52b8

Please sign in to comment.