diff --git a/Gemfile b/Gemfile index 62ed26a0a..6e173f9d9 100644 --- a/Gemfile +++ b/Gemfile @@ -139,6 +139,7 @@ end group :test do gem "approvals", "~> 0.0" # A library to make it easier to do golden-master style testing in Ruby + gem "exifr", ">= 1.3.6" # EXIF Reader is a module to read EXIF from JPEG and TIFF images gem "factory_bot_rails", "~> 4.11" # factory_bot is a fixtures replacement with a straightforward definition syntax, support for multiple build strategies gem "launchy", "~> 2.4" # Launchy is helper class for launching cross-platform applications in a fire and forget manner. gem "pundit-matchers", "~> 1.6" # A set of RSpec matchers for testing Pundit authorisation policies diff --git a/Gemfile.lock b/Gemfile.lock index 624db990b..0dd16cec3 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -313,6 +313,7 @@ GEM eventmachine (1.2.5) excon (0.65.0) execjs (2.7.0) + exifr (1.3.6) factory_bot (4.11.1) activesupport (>= 3.0.0) factory_bot_rails (4.11.1) @@ -896,6 +897,7 @@ DEPENDENCIES emoji_regex (~> 2.0) envied (~> 0.9) erb_lint (~> 0.0) + exifr (>= 1.3.6) factory_bot_rails (~> 4.11) faker (~> 2.6) fast_jsonapi (~> 1.5) diff --git a/app/uploaders/article_image_uploader.rb b/app/uploaders/article_image_uploader.rb index 2f2a464b8..b33b294c8 100644 --- a/app/uploaders/article_image_uploader.rb +++ b/app/uploaders/article_image_uploader.rb @@ -1,7 +1,4 @@ -class ArticleImageUploader < CarrierWave::Uploader::Base - include CarrierWave::BombShelter - # Adds resolution size limit to images of 4096x4096 - +class ArticleImageUploader < BaseUploader def store_dir "i/" end @@ -9,8 +6,4 @@ class ArticleImageUploader < CarrierWave::Uploader::Base def filename "#{Array.new(20) { rand(36).to_s(36) }.join}.#{file.extension}" if original_filename.present? end - - def extension_whitelist - %w[jpg jpeg jpe gif png ico bmp dng] - end end diff --git a/app/uploaders/badge_uploader.rb b/app/uploaders/badge_uploader.rb index 0bb4a9390..026475933 100644 --- a/app/uploaders/badge_uploader.rb +++ b/app/uploaders/badge_uploader.rb @@ -1,10 +1,4 @@ -class BadgeUploader < CarrierWave::Uploader::Base - include CarrierWave::BombShelter - - def store_dir - "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" - end - +class BadgeUploader < BaseUploader def extension_whitelist %w[jpg jpeg gif png] end diff --git a/app/uploaders/base_uploader.rb b/app/uploaders/base_uploader.rb new file mode 100644 index 000000000..69424e1e7 --- /dev/null +++ b/app/uploaders/base_uploader.rb @@ -0,0 +1,26 @@ +class BaseUploader < CarrierWave::Uploader::Base + include CarrierWave::BombShelter # limits size to 4096x4096 + include CarrierWave::MiniMagick # adds processing operations + + process :strip_exif + + def store_dir + # eg. uploads/user/profile_image/1/e481b7ee.jpg + "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" + end + + def extension_whitelist + %w[jpg jpeg jpe gif png ico bmp dng] + end + + protected + + # strip EXIF (and GPS) data + def strip_exif + manipulate! do |image| + image.strip + image = yield(image) if block_given? + image + end + end +end diff --git a/app/uploaders/cover_image_uploader.rb b/app/uploaders/cover_image_uploader.rb index ef65c8dc8..a6b8b07fd 100644 --- a/app/uploaders/cover_image_uploader.rb +++ b/app/uploaders/cover_image_uploader.rb @@ -1,11 +1,2 @@ -class CoverImageUploader < CarrierWave::Uploader::Base - include CarrierWave::BombShelter - - def store_dir - "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" - end - - def extension_whitelist - %w[jpg jpeg jpe gif png ico bmp dng] - end +class CoverImageUploader < BaseUploader end diff --git a/app/uploaders/profile_image_uploader.rb b/app/uploaders/profile_image_uploader.rb index 9982e38aa..84ccb0a67 100644 --- a/app/uploaders/profile_image_uploader.rb +++ b/app/uploaders/profile_image_uploader.rb @@ -1,19 +1,8 @@ -class ProfileImageUploader < CarrierWave::Uploader::Base - include CarrierWave::BombShelter - # Adds resolution size limit to images of 4096x4096 - - def store_dir - "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" - end - +class ProfileImageUploader < BaseUploader def filename "#{secure_token}.#{file.extension}" if original_filename.present? end - def extension_whitelist - %w[jpg jpeg jpe gif png ico bmp dng] - end - def size_range 1..2.megabytes end @@ -24,11 +13,4 @@ class ProfileImageUploader < CarrierWave::Uploader::Base var = :"@#{mounted_as}_secure_token" model.instance_variable_get(var) || model.instance_variable_set(var, SecureRandom.uuid) end - # version :big_thumb do - # process :resize_to_limit => [280, 280] - # end - # - # version :small_thumb do - # process :resize_to_limit => [100, 100] - # end end diff --git a/config/initializers/carrierwave.rb b/config/initializers/carrierwave.rb index fa986d676..f578a0e46 100644 --- a/config/initializers/carrierwave.rb +++ b/config/initializers/carrierwave.rb @@ -1,7 +1,9 @@ CarrierWave.configure do |config| - if Rails.env.development? || Rails.env.test? + if Rails.env.test? config.storage = :file config.enable_processing = false + elsif Rails.env.development? + config.storage = :file else config.fog_provider = "fog/aws" config.fog_credentials = { diff --git a/docs/installation/linux.md b/docs/installation/linux.md index c1922150c..4deb4bc59 100644 --- a/docs/installation/linux.md +++ b/docs/installation/linux.md @@ -25,6 +25,12 @@ There are two ways to install Yarn. There are more than one ways to setup PostgreSQL. For additional configuration, check out our [PostgreSQL setup guide](/installation/postgresql) or the official [PostgreSQL](https://www.postgresql.org/) site for further information. +### ImageMagick + +DEV uses [ImageMagick](https://imagemagick.org/) to manipulate images on upload. + +Please refer to ImageMagick's [instructions](https://imagemagick.org/script/download.php) on how to install it. + ## Installing DEV 1. Fork DEV's repository, e.g. diff --git a/docs/installation/mac.md b/docs/installation/mac.md index f75815346..60262d076 100644 --- a/docs/installation/mac.md +++ b/docs/installation/mac.md @@ -21,6 +21,12 @@ DEV requires PostgreSQL version 9.4 or higher. The easiest way to get started is For additional configuration options, check our [PostgreSQL setup guide](/installation/postgresql). +### ImageMagick + +DEV uses [ImageMagick](https://imagemagick.org/) to manipulate images on upload. + +You can install ImageMagick with `brew install imagemagick`. + ## Installing DEV 1. Fork DEV's repository, e.g. diff --git a/docs/installation/windows.md b/docs/installation/windows.md index b57ffe4ff..fed11a285 100644 --- a/docs/installation/windows.md +++ b/docs/installation/windows.md @@ -99,6 +99,12 @@ Pay attention to the username and password you setup during installation of Post For additional configuration options, check our [PostgreSQL setup guide](/installation/postgresql). +### ImageMagick + +DEV uses [ImageMagick](https://imagemagick.org/) to manipulate images on upload. + +Please refer to ImageMagick's [instructions](https://imagemagick.org/script/download.php) on how to install it. + ## Installing DEV 1. Fork DEV's repository, eg. diff --git a/docs/technical-overview.md b/docs/technical-overview.md index 917ba1406..93cd852fe 100644 --- a/docs/technical-overview.md +++ b/docs/technical-overview.md @@ -17,6 +17,7 @@ For the Dev.to tech stack we use: - [_Redcarpet_](https://github.com/vmg/redcarpet) and [_Rouge_](https://github.com/jneen/rouge) to parse Markdown - [_Carrierwave_](https://github.com/carrierwaveuploader/carrierwave), [_Fog_](https://github.com/fog/fog-aws) and [_AWS S3_](https://aws.amazon.com/s3/) for image upload/storage - [_InstantClick_](http://instantclick.io/) (a modified version) instead of _Turbolinks_ to accelerate navigation +- [_ImageMagick_](https://imagemagick.org/) to manipulate images on upload - [_Heroku_](https://www.heroku.com) for hosting - [_Heroku scheduler_](https://devcenter.heroku.com/articles/scheduler) for scheduled jobs - [_Sendgrid_](https://sendgrid.com/) for transactional mailing diff --git a/spec/fixtures/files/800x600.jpg b/spec/fixtures/files/800x600.jpg new file mode 100644 index 000000000..d99c19932 Binary files /dev/null and b/spec/fixtures/files/800x600.jpg differ diff --git a/spec/fixtures/files/800x600.png b/spec/fixtures/files/800x600.png new file mode 100644 index 000000000..09c36fb88 Binary files /dev/null and b/spec/fixtures/files/800x600.png differ diff --git a/spec/fixtures/files/800x600.webp b/spec/fixtures/files/800x600.webp new file mode 100644 index 000000000..c38fce040 Binary files /dev/null and b/spec/fixtures/files/800x600.webp differ diff --git a/spec/fixtures/files/image_gps_data.jpg b/spec/fixtures/files/image_gps_data.jpg new file mode 100644 index 000000000..4a2c1552b Binary files /dev/null and b/spec/fixtures/files/image_gps_data.jpg differ diff --git a/spec/uploaders/article_image_uploader_spec.rb b/spec/uploaders/article_image_uploader_spec.rb new file mode 100644 index 000000000..0eefb731a --- /dev/null +++ b/spec/uploaders/article_image_uploader_spec.rb @@ -0,0 +1,68 @@ +require "rails_helper" +require "carrierwave/test/matchers" +require "exifr/jpeg" + +describe ArticleImageUploader do + include CarrierWave::Test::Matchers + + let_it_be(:uploader) { described_class.new } + let_it_be(:image_jpg) { fixture_file_upload("files/800x600.jpg", "image/jpeg") } + let_it_be(:image_png) { fixture_file_upload("files/800x600.png", "image/png") } + let_it_be(:image_webp) { fixture_file_upload("files/800x600.webp", "image/webp") } + let_it_be(:image_with_gps) { fixture_file_upload("files/image_gps_data.jpg", "image/jpeg") } + + before do + described_class.include CarrierWave::MiniMagick # needed for processing + described_class.enable_processing = true + end + + after do + described_class.enable_processing = false + uploader.remove! + end + + it "stores files in the correct directory" do + expect(uploader.store_dir).to eq("i/") + end + + describe "filename" do + it "defaults to nil" do + expect(uploader.filename).to be_nil + end + + it "contains the original file extension when a file is stored" do + uploader.store!(image_jpg) + expect(uploader.filename).to match(/\.jpg\z/) + end + end + + describe "formats" do + it "permits a set of extensions" do + expect(uploader.extension_whitelist).to eq(%w[jpg jpeg jpe gif png ico bmp dng]) + end + + it "permits jpegs" do + uploader.store!(image_jpg) + expect(uploader).to be_format("jpeg") + end + + it "permits pngs" do + uploader.store!(image_png) + expect(uploader).to be_format("png") + end + + it "rejects unsupported formats like webp" do + expect { uploader.store!(image_webp) }.to raise_error(CarrierWave::IntegrityError) + end + end + + describe "exif removal" do + it "removes EXIF and GPS data on upload" do + expect(EXIFR::JPEG.new(image_with_gps.path).exif?).to be(true) + expect(EXIFR::JPEG.new(image_with_gps.path).gps.present?).to be(true) + uploader.store!(image_with_gps) + expect(EXIFR::JPEG.new(uploader.file.path).exif?).to be(false) + expect(EXIFR::JPEG.new(uploader.file.path).gps.present?).to be(false) + end + end +end diff --git a/spec/uploaders/badge_uploader_spec.rb b/spec/uploaders/badge_uploader_spec.rb new file mode 100644 index 000000000..b661dd4db --- /dev/null +++ b/spec/uploaders/badge_uploader_spec.rb @@ -0,0 +1,59 @@ +require "rails_helper" +require "carrierwave/test/matchers" +require "exifr/jpeg" + +describe BadgeUploader do + include CarrierWave::Test::Matchers + + let_it_be(:badge) { create(:badge) } + let_it_be(:uploader) { described_class.new(badge, :badge_image) } + let_it_be(:image_jpg) { fixture_file_upload("files/800x600.jpg", "image/jpeg") } + let_it_be(:image_png) { fixture_file_upload("files/800x600.png", "image/png") } + let_it_be(:image_webp) { fixture_file_upload("files/800x600.webp", "image/webp") } + let_it_be(:image_with_gps) { fixture_file_upload("files/image_gps_data.jpg", "image/jpeg") } + + before do + described_class.include CarrierWave::MiniMagick # needed for processing + described_class.enable_processing = true + end + + after do + described_class.enable_processing = false + uploader.remove! + end + + it "stores files in the correct directory" do + expect(uploader.store_dir).to eq("uploads/badge/badge_image/#{badge.id}") + end + + describe "formats" do + it "permits a set of extensions" do + expect(uploader.extension_whitelist).to eq(%w[jpg jpeg gif png]) + end + + it "permits jpegs" do + uploader.store!(image_jpg) + expect(uploader).to be_format("jpeg") + end + + it "permits pngs" do + uploader.store!(image_png) + expect(uploader).to be_format("png") + end + + it "rejects unsupported formats like webp" do + expect { uploader.store!(image_webp) }.to raise_error(CarrierWave::IntegrityError) + end + end + + describe "exif removal" do + it "removes EXIF and GPS data on upload" do + expect(EXIFR::JPEG.new(image_with_gps.path).exif?).to be(true) + expect(EXIFR::JPEG.new(image_with_gps.path).gps.present?).to be(true) + badge.badge_image = image_with_gps + badge.save! + expect(EXIFR::JPEG.new(badge.badge_image.path).exif?).to be(false) + expect(EXIFR::JPEG.new(badge.badge_image.path).gps.present?).to be(false) + end + end +end diff --git a/spec/uploaders/cover_image_uploader_spec.rb b/spec/uploaders/cover_image_uploader_spec.rb new file mode 100644 index 000000000..9382a35ab --- /dev/null +++ b/spec/uploaders/cover_image_uploader_spec.rb @@ -0,0 +1,59 @@ +require "rails_helper" +require "carrierwave/test/matchers" +require "exifr/jpeg" + +describe CoverImageUploader do + include CarrierWave::Test::Matchers + + let_it_be(:event) { create(:event) } + let_it_be(:uploader) { described_class.new(event, :cover_image) } + let_it_be(:image_jpg) { fixture_file_upload("files/800x600.jpg", "image/jpeg") } + let_it_be(:image_png) { fixture_file_upload("files/800x600.png", "image/png") } + let_it_be(:image_webp) { fixture_file_upload("files/800x600.webp", "image/webp") } + let_it_be(:image_with_gps) { fixture_file_upload("files/image_gps_data.jpg", "image/jpeg") } + + before do + described_class.include CarrierWave::MiniMagick # needed for processing + described_class.enable_processing = true + end + + after do + described_class.enable_processing = false + uploader.remove! + end + + it "stores files in the correct directory" do + expect(uploader.store_dir).to eq("uploads/event/cover_image/#{event.id}") + end + + describe "formats" do + it "permits a set of extensions" do + expect(uploader.extension_whitelist).to eq(%w[jpg jpeg jpe gif png ico bmp dng]) + end + + it "permits jpegs" do + uploader.store!(image_jpg) + expect(uploader).to be_format("jpeg") + end + + it "permits pngs" do + uploader.store!(image_png) + expect(uploader).to be_format("png") + end + + it "rejects unsupported formats like webp" do + expect { uploader.store!(image_webp) }.to raise_error(CarrierWave::IntegrityError) + end + end + + describe "exif removal" do + it "removes EXIF and GPS data on upload" do + expect(EXIFR::JPEG.new(image_with_gps.path).exif?).to be(true) + expect(EXIFR::JPEG.new(image_with_gps.path).gps.present?).to be(true) + event.cover_image = image_with_gps + event.save! + expect(EXIFR::JPEG.new(event.cover_image.path).exif?).to be(false) + expect(EXIFR::JPEG.new(event.cover_image.path).gps.present?).to be(false) + end + end +end diff --git a/spec/uploaders/profile_image_uploader_spec.rb b/spec/uploaders/profile_image_uploader_spec.rb new file mode 100644 index 000000000..afb1b9605 --- /dev/null +++ b/spec/uploaders/profile_image_uploader_spec.rb @@ -0,0 +1,77 @@ +require "rails_helper" +require "carrierwave/test/matchers" +require "exifr/jpeg" + +describe ProfileImageUploader do + include CarrierWave::Test::Matchers + + let_it_be(:mounted_as) { :profile_image } + let_it_be(:user) { create(:user) } + let_it_be(:uploader) { described_class.new(user, mounted_as) } + let_it_be(:image_jpg) { fixture_file_upload("files/800x600.jpg", "image/jpeg") } + let_it_be(:image_png) { fixture_file_upload("files/800x600.png", "image/png") } + let_it_be(:image_webp) { fixture_file_upload("files/800x600.webp", "image/webp") } + let_it_be(:image_with_gps) { fixture_file_upload("files/image_gps_data.jpg", "image/jpeg") } + + before do + described_class.include CarrierWave::MiniMagick # needed for processing + described_class.enable_processing = true + end + + after do + described_class.enable_processing = false + uploader.remove! + end + + it "stores files in the correct directory" do + expect(uploader.store_dir).to eq("uploads/user/profile_image/#{user.id}") + end + + describe "filename" do + it "defaults to nil" do + expect(uploader.filename).to be_nil + end + + it "contains a secure token" do + uploader.store!(image_jpg) + token = user.instance_variable_get(:"@#{mounted_as}_secure_token") + expect(uploader.filename).to include(token) + end + + it "contains the original file extension when a file is stored" do + uploader.store!(image_jpg) + expect(uploader.filename).to match(/\.jpg\z/) + end + end + + describe "formats" do + it "permits a set of extensions" do + expect(uploader.extension_whitelist).to eq(%w[jpg jpeg jpe gif png ico bmp dng]) + end + + it "permits jpegs" do + uploader.store!(image_jpg) + expect(uploader).to be_format("jpeg") + end + + it "permits pngs" do + uploader.store!(image_png) + expect(uploader).to be_format("png") + end + + it "rejects unsupported formats like webp" do + expect { uploader.store!(image_webp) }.to raise_error(CarrierWave::IntegrityError) + end + end + + describe "exif removal" do + it "removes EXIF and GPS data on upload" do + expect(EXIFR::JPEG.new(image_with_gps.path).exif?).to be(true) + expect(EXIFR::JPEG.new(image_with_gps.path).gps.present?).to be(true) + user.profile_image = image_with_gps + user.save! + expect(EXIFR::JPEG.new(user.profile_image.path).exif?).to be(false) + expect(EXIFR::JPEG.new(user.profile_image.path).gps.present?).to be(false) + end + end +end