Strip EXIF and GPS data from uploaded images (#4450)

* Add specs for ArticleImageUploader

* Add specs for BadgeUploader

* Add specs for CoverImageUploader

* Add specs for ProfileImageUploader

* Refactor uploaders to inherit from BaseUploader

* Strip EXIF and GPS data from uploaded images

* Add ImageMagick to the docs

* Protect strip_exif

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

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)

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

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

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

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

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

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 = {

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. <https://github.com/thepracticaldev/dev.to/fork>

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. <https://github.com/thepracticaldev/dev.to/fork>

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. <https://github.com/thepracticaldev/dev.to/fork>

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

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

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

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

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