Fix media attachments enumeration (mastodon#14254)

* Fix media attachment enumeration * Switch media_attachments id to snowflake ids Co-authored-by: Thibaut Girka <[email protected]>
hachyderm · Jul 7, 2020 · 6e25574 · 6e25574
1 parent 81a3db1
commit 6e25574
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 16 deletions.
diff --git a/app/controllers/media_proxy_controller.rb b/app/controllers/media_proxy_controller.rb
@@ -2,6 +2,7 @@
 
 class MediaProxyController < ApplicationController
   include RoutingHelper
+  include Authorization
 
   skip_before_action :store_current_location
   skip_before_action :require_functional!
@@ -10,12 +11,14 @@ class MediaProxyController < ApplicationController
 
   rescue_from ActiveRecord::RecordInvalid, with: :not_found
   rescue_from Mastodon::UnexpectedResponseError, with: :not_found
+  rescue_from Mastodon::NotPermittedError, with: :not_found
   rescue_from HTTP::TimeoutError, HTTP::ConnectionError, OpenSSL::SSL::SSLError, with: :internal_server_error
 
   def show
     RedisLock.acquire(lock_options) do |lock|
       if lock.acquired?
-        @media_attachment = MediaAttachment.remote.find(params[:id])
+        @media_attachment = MediaAttachment.remote.attached.find(params[:id])
+        authorize @media_attachment.status, :show?
         redownload! if @media_attachment.needs_redownload? && !reject_media?
       else
         raise Mastodon::RaceConditionError

diff --git a/db/migrate/20200622213645_media_attachment_ids_to_timestamp_ids.rb b/db/migrate/20200622213645_media_attachment_ids_to_timestamp_ids.rb
@@ -0,0 +1,17 @@
+class MediaAttachmentIdsToTimestampIds < ActiveRecord::Migration[5.1]
+  def up
+    # Set up the media_attachments.id column to use our timestamp-based IDs.
+    safety_assured do
+      execute("ALTER TABLE media_attachments ALTER COLUMN id SET DEFAULT timestamp_id('media_attachments')")
+    end
+
+    # Make sure we have a sequence to use.
+    Mastodon::Snowflake.ensure_id_sequences_exist
+  end
+
+  def down
+    execute("LOCK media_attachments")
+    execute("SELECT setval('media_attachments_id_seq', (SELECT MAX(id) FROM media_attachments))")
+    execute("ALTER TABLE media_attachments ALTER COLUMN id SET DEFAULT nextval('media_attachments_id_seq')")
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
@@ -77,6 +77,16 @@
     t.index ["target_account_id"], name: "index_account_moderation_notes_on_target_account_id"
   end
 
+  create_table "account_notes", force: :cascade do |t|
+    t.bigint "account_id"
+    t.bigint "target_account_id"
+    t.text "comment", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.index ["account_id", "target_account_id"], name: "index_account_notes_on_account_id_and_target_account_id", unique: true
+    t.index ["target_account_id"], name: "index_account_notes_on_target_account_id"
+  end
+
   create_table "account_pins", force: :cascade do |t|
     t.bigint "account_id"
     t.bigint "target_account_id"
@@ -471,7 +481,7 @@
     t.index ["user_id", "timeline"], name: "index_markers_on_user_id_and_timeline", unique: true
   end
 
-  create_table "media_attachments", force: :cascade do |t|
+  create_table "media_attachments", id: :bigint, default: -> { "timestamp_id('media_attachments'::text)" }, force: :cascade do |t|
     t.bigint "status_id"
     t.string "file_file_name"
     t.string "file_content_type"
@@ -833,16 +843,6 @@
     t.index ["user_id"], name: "index_user_invite_requests_on_user_id"
   end
 
-  create_table "account_notes", force: :cascade do |t|
-    t.bigint "account_id"
-    t.bigint "target_account_id"
-    t.text "comment", null: false
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
-    t.index ["account_id", "target_account_id"], name: "index_account_notes_on_account_id_and_target_account_id", unique: true
-    t.index ["target_account_id"], name: "index_account_notes_on_target_account_id"
-  end
-
   create_table "users", force: :cascade do |t|
     t.string "email", default: "", null: false
     t.datetime "created_at", null: false
@@ -918,6 +918,8 @@
   add_foreign_key "account_migrations", "accounts", on_delete: :cascade
   add_foreign_key "account_moderation_notes", "accounts"
   add_foreign_key "account_moderation_notes", "accounts", column: "target_account_id"
+  add_foreign_key "account_notes", "accounts", column: "target_account_id", on_delete: :cascade
+  add_foreign_key "account_notes", "accounts", on_delete: :cascade
   add_foreign_key "account_pins", "accounts", column: "target_account_id", on_delete: :cascade
   add_foreign_key "account_pins", "accounts", on_delete: :cascade
   add_foreign_key "account_stats", "accounts", on_delete: :cascade
@@ -999,8 +1001,6 @@
   add_foreign_key "statuses_tags", "tags", name: "fk_3081861e21", on_delete: :cascade
   add_foreign_key "tombstones", "accounts", on_delete: :cascade
   add_foreign_key "user_invite_requests", "users", on_delete: :cascade
-  add_foreign_key "account_notes", "accounts", column: "target_account_id", on_delete: :cascade
-  add_foreign_key "account_notes", "accounts", on_delete: :cascade
   add_foreign_key "users", "accounts", name: "fk_50500f500d", on_delete: :cascade
   add_foreign_key "users", "invites", on_delete: :nullify
   add_foreign_key "users", "oauth_applications", column: "created_by_application_id", on_delete: :nullify

diff --git a/spec/controllers/media_controller_spec.rb b/spec/controllers/media_controller_spec.rb
@@ -28,9 +28,8 @@
     end
 
     it 'raises when not permitted to view' do
-      status = Fabricate(:status)
+      status = Fabricate(:status, visibility: :direct)
       media_attachment = Fabricate(:media_attachment, status: status)
-      allow_any_instance_of(MediaController).to receive(:authorize).and_raise(ActiveRecord::RecordNotFound)
       get :show, params: { id: media_attachment.to_param }
 
       expect(response).to have_http_status(404)

diff --git a/spec/controllers/media_proxy_controller_spec.rb b/spec/controllers/media_proxy_controller_spec.rb
@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe MediaProxyController do
+  render_views
+
+  before do
+    stub_request(:get, 'http://example.com/attachment.png').to_return(request_fixture('avatar.txt'))
+  end
+
+  describe '#show' do
+    it 'redirects when attached to a status' do
+      status = Fabricate(:status)
+      media_attachment = Fabricate(:media_attachment, status: status, remote_url: 'http://example.com/attachment.png')
+      get :show, params: { id: media_attachment.id }
+
+      expect(response).to have_http_status(302)
+    end
+
+    it 'responds with missing when there is not an attached status' do
+      media_attachment = Fabricate(:media_attachment, status: nil, remote_url: 'http://example.com/attachment.png')
+      get :show, params: { id: media_attachment.id }
+
+      expect(response).to have_http_status(404)
+    end
+
+    it 'raises when id cant be found' do
+      get :show, params: { id: 'missing' }
+
+      expect(response).to have_http_status(404)
+    end
+
+    it 'raises when not permitted to view' do
+      status = Fabricate(:status, visibility: :direct)
+      media_attachment = Fabricate(:media_attachment, status: status, remote_url: 'http://example.com/attachment.png')
+      get :show, params: { id: media_attachment.id }
+
+      expect(response).to have_http_status(404)
+    end
+  end
+end