From be9c272ce490e65e989decb7958c5270f3017f24 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Thu, 23 Sep 2021 00:38:19 +0200
Subject: [PATCH] Remove default Active Storage direct upload action

We're already using a custom controller to handle direct uploads.

Besides, as mentioned by one of Active Storage co-authors [1], the
Active Storage DirectUploadsController doesn't provide any
authentication or validation at all, meaning anyone could create blobs
in our database by posting to `/rails/active_storage/direct_uploads`.
The response there could be then used to upload any file (again, without
validation) to `/rails/active_storage/disk/`.

For now, we're monkey-patching the controllers in order to send
unauthorized responses, since we aren't using these routes. If we ever
enable direct uploads with Active Storage, we'll have to add some sort
of authentication.

Similar upload solutions like CKEditor don't have this issue since their
controllers inherit from `ApplicationController` (which includes
authorization rules), while Active Storage controllers inherit from
`ActionController::Base`.

[1] https://discuss.rubyonrails.org/t/activestorage-direct-uploads-safe-by-default-how-to-make-it-safe/74863/2
---
 .../initializers/disable_active_storage_uploads.rb | 11 +++++++++++
 .../direct_uploads_controller_spec.rb              | 14 ++++++++++++++
 .../active_storage/disk_controller_spec.rb         | 13 +++++++++++++
 spec/factories/files.rb                            |  6 ++++++
 4 files changed, 44 insertions(+)
 create mode 100644 config/initializers/disable_active_storage_uploads.rb
 create mode 100644 spec/controllers/active_storage/direct_uploads_controller_spec.rb
 create mode 100644 spec/controllers/active_storage/disk_controller_spec.rb

diff --git a/config/initializers/disable_active_storage_uploads.rb b/config/initializers/disable_active_storage_uploads.rb
new file mode 100644
index 000000000..fa5a52a91
--- /dev/null
+++ b/config/initializers/disable_active_storage_uploads.rb
@@ -0,0 +1,11 @@
+ActiveStorage::DirectUploadsController.class_eval do
+  def create
+    head :unauthorized
+  end
+end
+
+ActiveStorage::DiskController.class_eval do
+  def update
+    head :unauthorized
+  end
+end
diff --git a/spec/controllers/active_storage/direct_uploads_controller_spec.rb b/spec/controllers/active_storage/direct_uploads_controller_spec.rb
new file mode 100644
index 000000000..e4bb0e8e6
--- /dev/null
+++ b/spec/controllers/active_storage/direct_uploads_controller_spec.rb
@@ -0,0 +1,14 @@
+require "rails_helper"
+
+describe ActiveStorage::DirectUploadsController do
+  describe "POST create" do
+    it "doesn't allow anonymous users to upload files" do
+      blob_attributes = { filename: "logo.pdf", byte_size: 30000, checksum: SecureRandom.hex(32) }
+
+      post :create, params: { blob: blob_attributes }
+
+      expect(ActiveStorage::Blob.count).to eq 0
+      expect(response).to be_unauthorized
+    end
+  end
+end
diff --git a/spec/controllers/active_storage/disk_controller_spec.rb b/spec/controllers/active_storage/disk_controller_spec.rb
new file mode 100644
index 000000000..48fc34d7a
--- /dev/null
+++ b/spec/controllers/active_storage/disk_controller_spec.rb
@@ -0,0 +1,13 @@
+require "rails_helper"
+
+describe ActiveStorage::DiskController do
+  describe "PUT update" do
+    it "doesn't allow anonymous users to upload files" do
+      blob = create(:active_storage_blob)
+
+      put :update, params: { encoded_token: blob.signed_id }
+
+      expect(response).to be_unauthorized
+    end
+  end
+end
diff --git a/spec/factories/files.rb b/spec/factories/files.rb
index 2290ccab5..0926ee576 100644
--- a/spec/factories/files.rb
+++ b/spec/factories/files.rb
@@ -55,4 +55,10 @@ FactoryBot.define do
     end
     initialize_with { new(attributes) }
   end
+
+  factory :active_storage_blob, class: "ActiveStorage::Blob" do
+    filename { "sample.pdf" }
+    byte_size { 3000 }
+    checksum { SecureRandom.hex(32) }
+  end
 end