From 0c28a505dddd13e2773cd3d5e0beef76a21eb415 Mon Sep 17 00:00:00 2001
From: Eugen Rochko <eugen@zeonfederated.com>
Date: Thu, 27 Feb 2020 12:32:54 +0100
Subject: [PATCH] Fix leak of arbitrary statuses through unfavourite action in
 REST API (#13161)

---
 .../api/v1/statuses/bookmarks_controller.rb   | 27 +++---
 .../favourited_by_accounts_controller.rb      |  3 +-
 .../api/v1/statuses/favourites_controller.rb  | 26 ++----
 .../reblogged_by_accounts_controller.rb       |  3 +-
 .../api/v1/statuses/reblogs_controller.rb     | 27 +++---
 .../v1/statuses/bookmarks_controller_spec.rb  | 69 +++++++++++----
 .../v1/statuses/favourites_controller_spec.rb | 86 +++++++++++++------
 .../v1/statuses/reblogs_controller_spec.rb    | 86 +++++++++++++------
 8 files changed, 203 insertions(+), 124 deletions(-)

diff --git a/app/controllers/api/v1/statuses/bookmarks_controller.rb b/app/controllers/api/v1/statuses/bookmarks_controller.rb
index bb9729cf50f..a7f1eed003b 100644
--- a/app/controllers/api/v1/statuses/bookmarks_controller.rb
+++ b/app/controllers/api/v1/statuses/bookmarks_controller.rb
@@ -5,35 +5,28 @@ class Api::V1::Statuses::BookmarksController < Api::BaseController
 
   before_action -> { doorkeeper_authorize! :write, :'write:bookmarks' }
   before_action :require_user!
+  before_action :set_status
 
   respond_to :json
 
   def create
-    @status = bookmarked_status
+    current_account.bookmarks.find_or_create_by!(account: current_account, status: @status)
     render json: @status, serializer: REST::StatusSerializer
   end
 
   def destroy
-    @status = requested_status
-    @bookmarks_map = { @status.id => false }
+    bookmark = current_account.bookmarks.find_by(status: @status)
+    bookmark&.destroy!
 
-    bookmark = Bookmark.find_by!(account: current_user.account, status: @status)
-    bookmark.destroy!
-
-    render json: @status, serializer: REST::StatusSerializer, relationships: StatusRelationshipsPresenter.new([@status], current_user&.account_id, bookmarks_map: @bookmarks_map)
+    render json: @status, serializer: REST::StatusSerializer, relationships: StatusRelationshipsPresenter.new([@status], current_account.id, bookmarks_map: { @status.id => false })
   end
 
   private
 
-  def bookmarked_status
-    authorize_with current_user.account, requested_status, :show?
-
-    bookmark = Bookmark.find_or_create_by!(account: current_user.account, status: requested_status)
-
-    bookmark.status.reload
-  end
-
-  def requested_status
-    Status.find(params[:status_id])
+  def set_status
+    @status = Status.find(params[:status_id])
+    authorize @status, :show?
+  rescue Mastodon::NotPermittedError
+    not_found
   end
 end
diff --git a/app/controllers/api/v1/statuses/favourited_by_accounts_controller.rb b/app/controllers/api/v1/statuses/favourited_by_accounts_controller.rb
index 99eff360ec8..05f4acc331b 100644
--- a/app/controllers/api/v1/statuses/favourited_by_accounts_controller.rb
+++ b/app/controllers/api/v1/statuses/favourited_by_accounts_controller.rb
@@ -69,8 +69,7 @@ class Api::V1::Statuses::FavouritedByAccountsController < Api::BaseController
     @status = Status.find(params[:status_id])
     authorize @status, :show?
   rescue Mastodon::NotPermittedError
-    # Reraise in order to get a 404 instead of a 403 error code
-    raise ActiveRecord::RecordNotFound
+    not_found
   end
 
   def pagination_params(core_params)
diff --git a/app/controllers/api/v1/statuses/favourites_controller.rb b/app/controllers/api/v1/statuses/favourites_controller.rb
index cceee906097..f18ace996c1 100644
--- a/app/controllers/api/v1/statuses/favourites_controller.rb
+++ b/app/controllers/api/v1/statuses/favourites_controller.rb
@@ -5,34 +5,26 @@ class Api::V1::Statuses::FavouritesController < Api::BaseController
 
   before_action -> { doorkeeper_authorize! :write, :'write:favourites' }
   before_action :require_user!
+  before_action :set_status
 
   respond_to :json
 
   def create
-    @status = favourited_status
+    FavouriteService.new.call(current_account, @status)
     render json: @status, serializer: REST::StatusSerializer
   end
 
   def destroy
-    @status = requested_status
-    @favourites_map = { @status.id => false }
-
-    UnfavouriteWorker.perform_async(current_user.account_id, @status.id)
-
-    render json: @status, serializer: REST::StatusSerializer, relationships: StatusRelationshipsPresenter.new([@status], current_user&.account_id, favourites_map: @favourites_map)
+    UnfavouriteWorker.perform_async(current_account.id, @status.id)
+    render json: @status, serializer: REST::StatusSerializer, relationships: StatusRelationshipsPresenter.new([@status], current_account.id, favourites_map: { @status.id => false })
   end
 
   private
 
-  def favourited_status
-    service_result.status.reload
-  end
-
-  def service_result
-    FavouriteService.new.call(current_user.account, requested_status)
-  end
-
-  def requested_status
-    Status.find(params[:status_id])
+  def set_status
+    @status = Status.find(params[:status_id])
+    authorize @status, :show?
+  rescue Mastodon::NotPermittedError
+    not_found
   end
 end
diff --git a/app/controllers/api/v1/statuses/reblogged_by_accounts_controller.rb b/app/controllers/api/v1/statuses/reblogged_by_accounts_controller.rb
index cc285ad2318..fa60e7d8468 100644
--- a/app/controllers/api/v1/statuses/reblogged_by_accounts_controller.rb
+++ b/app/controllers/api/v1/statuses/reblogged_by_accounts_controller.rb
@@ -66,8 +66,7 @@ class Api::V1::Statuses::RebloggedByAccountsController < Api::BaseController
     @status = Status.find(params[:status_id])
     authorize @status, :show?
   rescue Mastodon::NotPermittedError
-    # Reraise in order to get a 404 instead of a 403 error code
-    raise ActiveRecord::RecordNotFound
+    not_found
   end
 
   def pagination_params(core_params)
diff --git a/app/controllers/api/v1/statuses/reblogs_controller.rb b/app/controllers/api/v1/statuses/reblogs_controller.rb
index 42381a37fdb..67106ccbe81 100644
--- a/app/controllers/api/v1/statuses/reblogs_controller.rb
+++ b/app/controllers/api/v1/statuses/reblogs_controller.rb
@@ -5,33 +5,34 @@ class Api::V1::Statuses::ReblogsController < Api::BaseController
 
   before_action -> { doorkeeper_authorize! :write, :'write:statuses' }
   before_action :require_user!
+  before_action :set_reblog
 
   respond_to :json
 
   def create
-    @status = ReblogService.new.call(current_user.account, status_for_reblog, reblog_params)
+    @status = ReblogService.new.call(current_account, @reblog, reblog_params)
     render json: @status, serializer: REST::StatusSerializer
   end
 
   def destroy
-    @status = status_for_destroy.reblog
-    @reblogs_map = { @status.id => false }
+    @status = current_account.statuses.find_by(reblog_of_id: @reblog.id)
 
-    authorize status_for_destroy, :unreblog?
-    status_for_destroy.discard
-    RemovalWorker.perform_async(status_for_destroy.id)
+    if @status
+      authorize @status, :unreblog?
+      @status.discard
+      RemovalWorker.perform_async(@status.id)
+    end
 
-    render json: @status, serializer: REST::StatusSerializer, relationships: StatusRelationshipsPresenter.new([@status], current_user&.account_id, reblogs_map: @reblogs_map)
+    render json: @reblog, serializer: REST::StatusSerializer, relationships: StatusRelationshipsPresenter.new([@status], current_account.id, reblogs_map: { @reblog.id => false })
   end
 
   private
 
-  def status_for_reblog
-    Status.find params[:status_id]
-  end
-
-  def status_for_destroy
-    @status_for_destroy ||= current_user.account.statuses.where(reblog_of_id: params[:status_id]).first!
+  def set_reblog
+    @reblog = Status.find(params[:status_id])
+    authorize @reblog, :show?
+  rescue Mastodon::NotPermittedError
+    not_found
   end
 
   def reblog_params
diff --git a/spec/controllers/api/v1/statuses/bookmarks_controller_spec.rb b/spec/controllers/api/v1/statuses/bookmarks_controller_spec.rb
index b79853718d1..aa5ca433fb7 100644
--- a/spec/controllers/api/v1/statuses/bookmarks_controller_spec.rb
+++ b/spec/controllers/api/v1/statuses/bookmarks_controller_spec.rb
@@ -21,36 +21,67 @@ describe Api::V1::Statuses::BookmarksController do
         post :create, params: { status_id: status.id }
       end
 
-      it 'returns http success' do
-        expect(response).to have_http_status(:success)
+      context 'with public status' do
+        it 'returns http success' do
+          expect(response).to have_http_status(:success)
+        end
+
+        it 'updates the bookmarked attribute' do
+          expect(user.account.bookmarked?(status)).to be true
+        end
+
+        it 'returns json with updated attributes' do
+          hash_body = body_as_json
+
+          expect(hash_body[:id]).to eq status.id.to_s
+          expect(hash_body[:bookmarked]).to be true
+        end
       end
 
-      it 'updates the bookmarked attribute' do
-        expect(user.account.bookmarked?(status)).to be true
-      end
+      context 'with private status of not-followed account' do
+        let(:status) { Fabricate(:status, visibility: :private) }
 
-      it 'return json with updated attributes' do
-        hash_body = body_as_json
-
-        expect(hash_body[:id]).to eq status.id.to_s
-        expect(hash_body[:bookmarked]).to be true
+        it 'returns http not found' do
+          expect(response).to have_http_status(404)
+        end
       end
     end
 
     describe 'POST #destroy' do
-      let(:status) { Fabricate(:status, account: user.account) }
+      context 'with public status' do
+        let(:status) { Fabricate(:status, account: user.account) }
 
-      before do
-        Bookmark.find_or_create_by!(account: user.account, status: status)
-        post :destroy, params: { status_id: status.id }
+        before do
+          Bookmark.find_or_create_by!(account: user.account, status: status)
+          post :destroy, params: { status_id: status.id }
+        end
+
+        it 'returns http success' do
+          expect(response).to have_http_status(:success)
+        end
+
+        it 'updates the bookmarked attribute' do
+          expect(user.account.bookmarked?(status)).to be false
+        end
+
+        it 'returns json with updated attributes' do
+          hash_body = body_as_json
+
+          expect(hash_body[:id]).to eq status.id.to_s
+          expect(hash_body[:bookmarked]).to be false
+        end
       end
 
-      it 'returns http success' do
-        expect(response).to have_http_status(:success)
-      end
+      context 'with private status that was not bookmarked' do
+        let(:status) { Fabricate(:status, visibility: :private) }
 
-      it 'updates the bookmarked attribute' do
-        expect(user.account.bookmarked?(status)).to be false
+        before do
+          post :destroy, params: { status_id: status.id }
+        end
+
+        it 'returns http not found' do
+          expect(response).to have_http_status(404)
+        end
       end
     end
   end
diff --git a/spec/controllers/api/v1/statuses/favourites_controller_spec.rb b/spec/controllers/api/v1/statuses/favourites_controller_spec.rb
index 24a760e20e6..6e947f5d2b6 100644
--- a/spec/controllers/api/v1/statuses/favourites_controller_spec.rb
+++ b/spec/controllers/api/v1/statuses/favourites_controller_spec.rb
@@ -21,45 +21,77 @@ describe Api::V1::Statuses::FavouritesController do
         post :create, params: { status_id: status.id }
       end
 
-      it 'returns http success' do
-        expect(response).to have_http_status(200)
+      context 'with public status' do
+        it 'returns http success' do
+          expect(response).to have_http_status(200)
+        end
+
+        it 'updates the favourites count' do
+          expect(status.favourites.count).to eq 1
+        end
+
+        it 'updates the favourited attribute' do
+          expect(user.account.favourited?(status)).to be true
+        end
+
+        it 'returns json with updated attributes' do
+          hash_body = body_as_json
+
+          expect(hash_body[:id]).to eq status.id.to_s
+          expect(hash_body[:favourites_count]).to eq 1
+          expect(hash_body[:favourited]).to be true
+        end
       end
 
-      it 'updates the favourites count' do
-        expect(status.favourites.count).to eq 1
-      end
+      context 'with private status of not-followed account' do
+        let(:status) { Fabricate(:status, visibility: :private) }
 
-      it 'updates the favourited attribute' do
-        expect(user.account.favourited?(status)).to be true
-      end
-
-      it 'return json with updated attributes' do
-        hash_body = body_as_json
-
-        expect(hash_body[:id]).to eq status.id.to_s
-        expect(hash_body[:favourites_count]).to eq 1
-        expect(hash_body[:favourited]).to be true
+        it 'returns http not found' do
+          expect(response).to have_http_status(404)
+        end
       end
     end
 
     describe 'POST #destroy' do
-      let(:status) { Fabricate(:status, account: user.account) }
+      context 'with public status' do
+        let(:status) { Fabricate(:status, account: user.account) }
 
-      before do
-        FavouriteService.new.call(user.account, status)
-        post :destroy, params: { status_id: status.id }
+        before do
+          FavouriteService.new.call(user.account, status)
+          post :destroy, params: { status_id: status.id }
+        end
+
+        it 'returns http success' do
+          expect(response).to have_http_status(200)
+        end
+
+        it 'updates the favourites count' do
+          expect(status.favourites.count).to eq 0
+        end
+
+        it 'updates the favourited attribute' do
+          expect(user.account.favourited?(status)).to be false
+        end
+
+        it 'returns json with updated attributes' do
+          hash_body = body_as_json
+
+          expect(hash_body[:id]).to eq status.id.to_s
+          expect(hash_body[:favourites_count]).to eq 0
+          expect(hash_body[:favourited]).to be false
+        end
       end
 
-      it 'returns http success' do
-        expect(response).to have_http_status(200)
-      end
+      context 'with private status that was not favourited' do
+        let(:status) { Fabricate(:status, visibility: :private) }
 
-      it 'updates the favourites count' do
-        expect(status.favourites.count).to eq 0
-      end
+        before do
+          post :destroy, params: { status_id: status.id }
+        end
 
-      it 'updates the favourited attribute' do
-        expect(user.account.favourited?(status)).to be false
+        it 'returns http not found' do
+          expect(response).to have_http_status(404)
+        end
       end
     end
   end
diff --git a/spec/controllers/api/v1/statuses/reblogs_controller_spec.rb b/spec/controllers/api/v1/statuses/reblogs_controller_spec.rb
index d14ca3e8b6a..93b244cc3be 100644
--- a/spec/controllers/api/v1/statuses/reblogs_controller_spec.rb
+++ b/spec/controllers/api/v1/statuses/reblogs_controller_spec.rb
@@ -21,45 +21,77 @@ describe Api::V1::Statuses::ReblogsController do
         post :create, params: { status_id: status.id }
       end
 
-      it 'returns http success' do
-        expect(response).to have_http_status(200)
+      context 'with public status' do
+        it 'returns http success' do
+          expect(response).to have_http_status(200)
+        end
+
+        it 'updates the reblogs count' do
+          expect(status.reblogs.count).to eq 1
+        end
+
+        it 'updates the reblogged attribute' do
+          expect(user.account.reblogged?(status)).to be true
+        end
+
+        it 'returns json with updated attributes' do
+          hash_body = body_as_json
+
+          expect(hash_body[:reblog][:id]).to eq status.id.to_s
+          expect(hash_body[:reblog][:reblogs_count]).to eq 1
+          expect(hash_body[:reblog][:reblogged]).to be true
+        end
       end
 
-      it 'updates the reblogs count' do
-        expect(status.reblogs.count).to eq 1
-      end
+      context 'with private status of not-followed account' do
+        let(:status) { Fabricate(:status, visibility: :private) }
 
-      it 'updates the reblogged attribute' do
-        expect(user.account.reblogged?(status)).to be true
-      end
-
-      it 'return json with updated attributes' do
-        hash_body = body_as_json
-
-        expect(hash_body[:reblog][:id]).to eq status.id.to_s
-        expect(hash_body[:reblog][:reblogs_count]).to eq 1
-        expect(hash_body[:reblog][:reblogged]).to be true
+        it 'returns http not found' do
+          expect(response).to have_http_status(404)
+        end
       end
     end
 
     describe 'POST #destroy' do
-      let(:status) { Fabricate(:status, account: user.account) }
+      context 'with public status' do
+        let(:status) { Fabricate(:status, account: user.account) }
 
-      before do
-        ReblogService.new.call(user.account, status)
-        post :destroy, params: { status_id: status.id }
+        before do
+          ReblogService.new.call(user.account, status)
+          post :destroy, params: { status_id: status.id }
+        end
+
+        it 'returns http success' do
+          expect(response).to have_http_status(200)
+        end
+
+        it 'updates the reblogs count' do
+          expect(status.reblogs.count).to eq 0
+        end
+
+        it 'updates the reblogged attribute' do
+          expect(user.account.reblogged?(status)).to be false
+        end
+
+        it 'returns json with updated attributes' do
+          hash_body = body_as_json
+
+          expect(hash_body[:id]).to eq status.id.to_s
+          expect(hash_body[:reblogs_count]).to eq 0
+          expect(hash_body[:reblogged]).to be false
+        end
       end
 
-      it 'returns http success' do
-        expect(response).to have_http_status(200)
-      end
+      context 'with private status that was not reblogged' do
+        let(:status) { Fabricate(:status, visibility: :private) }
 
-      it 'updates the reblogs count' do
-        expect(status.reblogs.count).to eq 0
-      end
+        before do
+          post :destroy, params: { status_id: status.id }
+        end
 
-      it 'updates the reblogged attribute' do
-        expect(user.account.reblogged?(status)).to be false
+        it 'returns http not found' do
+          expect(response).to have_http_status(404)
+        end
       end
     end
   end