From 502cf75b160c76f963a179d46498e3ea1a9fefca Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 4 Jul 2024 16:26:49 +0200 Subject: [PATCH] Merge pull request from GHSA-58x8-3qxw-6hm7 * Fix insufficient permission checking for public timeline endpoints Note that this changes unauthenticated access failure code from 401 to 422 * Add more tests for public timelines * Require user token in `/api/v1/statuses/:id/translate` and `/api/v1/scheduled_statuses` --- .../api/v1/scheduled_statuses_controller.rb | 1 + .../v1/statuses/translations_controller.rb | 1 + .../api/v1/timelines/base_controller.rb | 6 +++++ .../api/v1/timelines/link_controller.rb | 6 +---- .../api/v1/timelines/public_controller.rb | 6 +---- .../api/v1/timelines/tag_controller.rb | 2 +- spec/requests/api/v1/scheduled_status_spec.rb | 11 ++++++++ .../api/v1/statuses/translations_spec.rb | 16 ++++++++++++ spec/requests/api/v1/timelines/link_spec.rb | 20 +++++++++++--- spec/requests/api/v1/timelines/public_spec.rb | 26 ++++++++++++++----- spec/requests/api/v1/timelines/tag_spec.rb | 10 ++++--- 11 files changed, 82 insertions(+), 23 deletions(-) diff --git a/app/controllers/api/v1/scheduled_statuses_controller.rb b/app/controllers/api/v1/scheduled_statuses_controller.rb index 45ee5865188..c62305d7114 100644 --- a/app/controllers/api/v1/scheduled_statuses_controller.rb +++ b/app/controllers/api/v1/scheduled_statuses_controller.rb @@ -6,6 +6,7 @@ class Api::V1::ScheduledStatusesController < Api::BaseController before_action -> { doorkeeper_authorize! :read, :'read:statuses' }, except: [:update, :destroy] before_action -> { doorkeeper_authorize! :write, :'write:statuses' }, only: [:update, :destroy] + before_action :require_user! before_action :set_statuses, only: :index before_action :set_status, except: :index diff --git a/app/controllers/api/v1/statuses/translations_controller.rb b/app/controllers/api/v1/statuses/translations_controller.rb index 7d406b0a36d..8cf495f78ac 100644 --- a/app/controllers/api/v1/statuses/translations_controller.rb +++ b/app/controllers/api/v1/statuses/translations_controller.rb @@ -2,6 +2,7 @@ class Api::V1::Statuses::TranslationsController < Api::V1::Statuses::BaseController before_action -> { doorkeeper_authorize! :read, :'read:statuses' } + before_action :require_user! before_action :set_translation rescue_from TranslationService::NotConfiguredError, with: :not_found diff --git a/app/controllers/api/v1/timelines/base_controller.rb b/app/controllers/api/v1/timelines/base_controller.rb index e79eba79ee5..1dba4a5bb21 100644 --- a/app/controllers/api/v1/timelines/base_controller.rb +++ b/app/controllers/api/v1/timelines/base_controller.rb @@ -3,8 +3,14 @@ class Api::V1::Timelines::BaseController < Api::BaseController after_action :insert_pagination_headers, unless: -> { @statuses.empty? } + before_action :require_user!, if: :require_auth? + private + def require_auth? + !Setting.timeline_preview + end + def pagination_collection @statuses end diff --git a/app/controllers/api/v1/timelines/link_controller.rb b/app/controllers/api/v1/timelines/link_controller.rb index af962c430ff..37ed084f062 100644 --- a/app/controllers/api/v1/timelines/link_controller.rb +++ b/app/controllers/api/v1/timelines/link_controller.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class Api::V1::Timelines::LinkController < Api::V1::Timelines::BaseController - before_action -> { doorkeeper_authorize! :read, :'read:statuses' }, only: :show, if: :require_auth? + before_action -> { authorize_if_got_token! :read, :'read:statuses' } before_action :set_preview_card before_action :set_statuses @@ -17,10 +17,6 @@ class Api::V1::Timelines::LinkController < Api::V1::Timelines::BaseController private - def require_auth? - !Setting.timeline_preview - end - def set_preview_card @preview_card = PreviewCard.joins(:trend).merge(PreviewCardTrend.allowed).find_by!(url: params[:url]) end diff --git a/app/controllers/api/v1/timelines/public_controller.rb b/app/controllers/api/v1/timelines/public_controller.rb index d164854d6a3..029e8fc2c13 100644 --- a/app/controllers/api/v1/timelines/public_controller.rb +++ b/app/controllers/api/v1/timelines/public_controller.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class Api::V1::Timelines::PublicController < Api::V1::Timelines::BaseController - before_action :require_user!, only: [:show], if: :require_auth? + before_action -> { authorize_if_got_token! :read, :'read:statuses' } PERMITTED_PARAMS = %i(local remote limit only_media).freeze @@ -13,10 +13,6 @@ class Api::V1::Timelines::PublicController < Api::V1::Timelines::BaseController private - def require_auth? - !Setting.timeline_preview - end - def load_statuses preloaded_public_statuses_page end diff --git a/app/controllers/api/v1/timelines/tag_controller.rb b/app/controllers/api/v1/timelines/tag_controller.rb index 3bf8f374e19..2b097aab0f8 100644 --- a/app/controllers/api/v1/timelines/tag_controller.rb +++ b/app/controllers/api/v1/timelines/tag_controller.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class Api::V1::Timelines::TagController < Api::V1::Timelines::BaseController - before_action -> { doorkeeper_authorize! :read, :'read:statuses' }, only: :show, if: :require_auth? + before_action -> { authorize_if_got_token! :read, :'read:statuses' } before_action :load_tag PERMITTED_PARAMS = %i(local limit only_media).freeze diff --git a/spec/requests/api/v1/scheduled_status_spec.rb b/spec/requests/api/v1/scheduled_status_spec.rb index 49ccde275ce..f4612410bf3 100644 --- a/spec/requests/api/v1/scheduled_status_spec.rb +++ b/spec/requests/api/v1/scheduled_status_spec.rb @@ -25,6 +25,17 @@ describe 'Scheduled Statuses' do it_behaves_like 'forbidden for wrong scope', 'write write:statuses' end + context 'with an application token' do + let(:token) { Fabricate(:accessible_access_token, resource_owner_id: nil, scopes: 'read:statuses') } + + it 'returns http unprocessable entity' do + get api_v1_scheduled_statuses_path, headers: headers + + expect(response) + .to have_http_status(422) + end + end + context 'with correct scope' do let(:scopes) { 'read:statuses' } diff --git a/spec/requests/api/v1/statuses/translations_spec.rb b/spec/requests/api/v1/statuses/translations_spec.rb index 5b0a994561c..e2ab5d0b806 100644 --- a/spec/requests/api/v1/statuses/translations_spec.rb +++ b/spec/requests/api/v1/statuses/translations_spec.rb @@ -8,6 +8,22 @@ describe 'API V1 Statuses Translations' do let(:scopes) { 'read:statuses' } let(:headers) { { 'Authorization' => "Bearer #{token.token}" } } + context 'with an application token' do + let(:token) { Fabricate(:accessible_access_token, resource_owner_id: nil, scopes: scopes) } + + describe 'POST /api/v1/statuses/:status_id/translate' do + let(:status) { Fabricate(:status, account: user.account, text: 'Hola', language: 'es') } + + before do + post "/api/v1/statuses/#{status.id}/translate", headers: headers + end + + it 'returns http unprocessable entity' do + expect(response).to have_http_status(422) + end + end + end + context 'with an oauth token' do describe 'POST /api/v1/statuses/:status_id/translate' do let(:status) { Fabricate(:status, account: user.account, text: 'Hola', language: 'es') } diff --git a/spec/requests/api/v1/timelines/link_spec.rb b/spec/requests/api/v1/timelines/link_spec.rb index a219c9bcdde..e1c914ab81a 100644 --- a/spec/requests/api/v1/timelines/link_spec.rb +++ b/spec/requests/api/v1/timelines/link_spec.rb @@ -41,6 +41,8 @@ describe 'Link' do end end + it_behaves_like 'forbidden for wrong scope', 'profile' + context 'when there is no preview card' do let(:preview_card) { nil } @@ -80,13 +82,25 @@ describe 'Link' do Form::AdminSettings.new(timeline_preview: false).save end - context 'when the user is not authenticated' do + it_behaves_like 'forbidden for wrong scope', 'profile' + + context 'without an authentication token' do let(:headers) { {} } - it 'returns http unauthorized' do + it 'returns http unprocessable entity' do subject - expect(response).to have_http_status(401) + expect(response).to have_http_status(422) + end + end + + context 'with an application access token, not bound to a user' do + let(:token) { Fabricate(:accessible_access_token, resource_owner_id: nil, scopes: scopes) } + + it 'returns http unprocessable entity' do + subject + + expect(response).to have_http_status(422) end end diff --git a/spec/requests/api/v1/timelines/public_spec.rb b/spec/requests/api/v1/timelines/public_spec.rb index 364e48d3d2f..100f6c1bfc1 100644 --- a/spec/requests/api/v1/timelines/public_spec.rb +++ b/spec/requests/api/v1/timelines/public_spec.rb @@ -34,6 +34,8 @@ describe 'Public' do context 'when the instance allows public preview' do let(:expected_statuses) { [local_status, remote_status, media_status] } + it_behaves_like 'forbidden for wrong scope', 'profile' + context 'with an authorized user' do it_behaves_like 'a successful request to the public timeline' end @@ -99,13 +101,9 @@ describe 'Public' do Form::AdminSettings.new(timeline_preview: false).save end - context 'with an authenticated user' do - let(:expected_statuses) { [local_status, remote_status, media_status] } + it_behaves_like 'forbidden for wrong scope', 'profile' - it_behaves_like 'a successful request to the public timeline' - end - - context 'with an unauthenticated user' do + context 'without an authentication token' do let(:headers) { {} } it 'returns http unprocessable entity' do @@ -114,6 +112,22 @@ describe 'Public' do expect(response).to have_http_status(422) end end + + context 'with an application access token, not bound to a user' do + let(:token) { Fabricate(:accessible_access_token, resource_owner_id: nil, scopes: scopes) } + + it 'returns http unprocessable entity' do + subject + + expect(response).to have_http_status(422) + end + end + + context 'with an authenticated user' do + let(:expected_statuses) { [local_status, remote_status, media_status] } + + it_behaves_like 'a successful request to the public timeline' + end end end end diff --git a/spec/requests/api/v1/timelines/tag_spec.rb b/spec/requests/api/v1/timelines/tag_spec.rb index 8611341702d..5e1415bb1ac 100644 --- a/spec/requests/api/v1/timelines/tag_spec.rb +++ b/spec/requests/api/v1/timelines/tag_spec.rb @@ -30,6 +30,8 @@ RSpec.describe 'Tag' do let(:params) { {} } let(:hashtag) { 'life' } + it_behaves_like 'forbidden for wrong scope', 'profile' + context 'when given only one hashtag' do let(:expected_statuses) { [life_status] } @@ -93,13 +95,15 @@ RSpec.describe 'Tag' do Form::AdminSettings.new(timeline_preview: false).save end - context 'when the user is not authenticated' do + it_behaves_like 'forbidden for wrong scope', 'profile' + + context 'without an authentication token' do let(:headers) { {} } - it 'returns http unauthorized' do + it 'returns http unprocessable entity' do subject - expect(response).to have_http_status(401) + expect(response).to have_http_status(422) end end