From 4fb4721072e552dc2fa7541f7bdeb1737a6c113e 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/public_controller.rb | 1 + .../api/v1/timelines/tag_controller.rb | 3 ++- .../v1/scheduled_statuses_controller_spec.rb | 11 ++++++++ .../statuses/translations_controller_spec.rb | 20 ++++++++++++++ .../api/v1/timelines/tag_controller_spec.rb | 19 +++++++++++--- spec/requests/api/v1/timelines/public_spec.rb | 26 ++++++++++++++----- 8 files changed, 71 insertions(+), 11 deletions(-) diff --git a/app/controllers/api/v1/scheduled_statuses_controller.rb b/app/controllers/api/v1/scheduled_statuses_controller.rb index 2220b6d22e..b33b534ebb 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 ec5ea5b85b..5e5ee7d38e 100644 --- a/app/controllers/api/v1/statuses/translations_controller.rb +++ b/app/controllers/api/v1/statuses/translations_controller.rb @@ -4,6 +4,7 @@ class Api::V1::Statuses::TranslationsController < Api::BaseController include Authorization before_action -> { doorkeeper_authorize! :read, :'read:statuses' } + before_action :require_user! before_action :set_status before_action :set_translation diff --git a/app/controllers/api/v1/timelines/public_controller.rb b/app/controllers/api/v1/timelines/public_controller.rb index 5bbd92b9ee..0ff2c5aee2 100644 --- a/app/controllers/api/v1/timelines/public_controller.rb +++ b/app/controllers/api/v1/timelines/public_controller.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class Api::V1::Timelines::PublicController < Api::BaseController + before_action -> { authorize_if_got_token! :read, :'read:statuses' } before_action :require_user!, only: [:show], if: :require_auth? after_action :insert_pagination_headers, unless: -> { @statuses.empty? } diff --git a/app/controllers/api/v1/timelines/tag_controller.rb b/app/controllers/api/v1/timelines/tag_controller.rb index a79d65c124..ffea89c258 100644 --- a/app/controllers/api/v1/timelines/tag_controller.rb +++ b/app/controllers/api/v1/timelines/tag_controller.rb @@ -1,7 +1,8 @@ # frozen_string_literal: true class Api::V1::Timelines::TagController < Api::BaseController - before_action -> { doorkeeper_authorize! :read, :'read:statuses' }, only: :show, if: :require_auth? + before_action -> { authorize_if_got_token! :read, :'read:statuses' } + before_action :require_user!, if: :require_auth? before_action :load_tag after_action :insert_pagination_headers, unless: -> { @statuses.empty? } diff --git a/spec/controllers/api/v1/scheduled_statuses_controller_spec.rb b/spec/controllers/api/v1/scheduled_statuses_controller_spec.rb index 256c4b272a..cc3b65f37d 100644 --- a/spec/controllers/api/v1/scheduled_statuses_controller_spec.rb +++ b/spec/controllers/api/v1/scheduled_statuses_controller_spec.rb @@ -13,6 +13,17 @@ describe Api::V1::ScheduledStatusesController do allow(controller).to receive(:doorkeeper_token) { token } 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 :index + + expect(response) + .to have_http_status(422) + end + end + describe 'GET #index' do it 'returns http success' do get :index diff --git a/spec/controllers/api/v1/statuses/translations_controller_spec.rb b/spec/controllers/api/v1/statuses/translations_controller_spec.rb index 6257494ae1..da152843b2 100644 --- a/spec/controllers/api/v1/statuses/translations_controller_spec.rb +++ b/spec/controllers/api/v1/statuses/translations_controller_spec.rb @@ -9,6 +9,26 @@ describe Api::V1::Statuses::TranslationsController do let(:app) { Fabricate(:application, name: 'Test app', website: 'http://testapp.com') } let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: 'read:statuses', application: app) } + context 'with an application token' do + let(:token) { Fabricate(:accessible_access_token, resource_owner_id: nil, scopes: 'read:statuses', application: app) } + + before do + allow(controller).to receive(:doorkeeper_token) { token } + end + + describe 'POST /api/v1/statuses/:status_id/translate' do + let(:status) { Fabricate(:status, account: user.account, text: 'Hola', language: 'es') } + + before do + post :create, params: { status_id: status.id } + end + + it 'returns http unprocessable entity' do + expect(response).to have_http_status(422) + end + end + end + context 'with an oauth token' do before do allow(controller).to receive(:doorkeeper_token) { token } diff --git a/spec/controllers/api/v1/timelines/tag_controller_spec.rb b/spec/controllers/api/v1/timelines/tag_controller_spec.rb index 1c60798fcf..89622a41a2 100644 --- a/spec/controllers/api/v1/timelines/tag_controller_spec.rb +++ b/spec/controllers/api/v1/timelines/tag_controller_spec.rb @@ -6,7 +6,8 @@ describe Api::V1::Timelines::TagController do render_views let(:user) { Fabricate(:user) } - let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: 'read:statuses') } + let(:scopes) { 'read:statuses' } + let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) } before do allow(controller).to receive(:doorkeeper_token) { token } @@ -48,13 +49,23 @@ describe Api::V1::Timelines::TagController do Form::AdminSettings.new(timeline_preview: false).save end - context 'when the user is not authenticated' do + context 'without an access token' do let(:token) { nil } - 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 c436262407..03bde3da87 100644 --- a/spec/requests/api/v1/timelines/public_spec.rb +++ b/spec/requests/api/v1/timelines/public_spec.rb @@ -32,6 +32,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 @@ -96,13 +98,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 @@ -111,6 +109,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