merge security fixes

Gemfile.lock

@@ -465,7 +465,7 @@ GEM
     net-smtp (0.4.0.1)
       net-protocol
     nio4r (2.5.9)
-    nokogiri (1.16.0)
+    nokogiri (1.16.2)
       mini_portile2 (~> 2.8.2)
       racc (~> 1.4)
     oj (3.16.3)
@@ -712,7 +712,7 @@ GEM
       rufus-scheduler (~> 3.2)
       sidekiq (>= 6, < 8)
       tilt (>= 1.4.0)
-    sidekiq-unique-jobs (7.1.31)
+    sidekiq-unique-jobs (7.1.33)
       brpoplpush-redis_script (> 0.1.1, <= 2.0.0)
       concurrent-ruby (~> 1.0, >= 1.0.5)
       redis (< 5.0)

app/controllers/auth/omniauth_callbacks_controller.rb

@@ -7,7 +7,7 @@ class Auth::OmniauthCallbacksController < Devise::OmniauthCallbacksController
   def self.provides_callback_for(provider)
     define_method provider do
       @provider = provider
-      @user = User.find_for_oauth(request.env['omniauth.auth'], current_user)
+      @user = User.find_for_omniauth(request.env['omniauth.auth'], current_user)
 
       if @user.persisted?
         record_login_activity

app/helpers/jsonld_helper.rb

@@ -155,7 +155,7 @@ module JsonLdHelper
     end
   end
 
-  def fetch_resource(uri, id_is_known, on_behalf_of = nil, request_options: {})
+  def fetch_resource(uri, id_is_known, on_behalf_of = nil)
     unless id_is_known
       json = fetch_resource_without_id_validation(uri, on_behalf_of)
 
@@ -164,14 +164,14 @@ module JsonLdHelper
       uri = json['id']
     end
 
-    json = fetch_resource_without_id_validation(uri, on_behalf_of, request_options: request_options)
+    json = fetch_resource_without_id_validation(uri, on_behalf_of)
     json.present? && json['id'] == uri ? json : nil
   end
 
-  def fetch_resource_without_id_validation(uri, on_behalf_of = nil, raise_on_temporary_error = false, request_options: {})
+  def fetch_resource_without_id_validation(uri, on_behalf_of = nil, raise_on_temporary_error = false)
     on_behalf_of ||= Account.representative
 
-    build_request(uri, on_behalf_of, options: request_options).perform do |response|
+    build_request(uri, on_behalf_of).perform do |response|
       raise Mastodon::UnexpectedResponseError, response unless response_successful?(response) || response_error_unsalvageable?(response) || !raise_on_temporary_error
 
       body_to_json(response.body_with_limit) if response.code == 200
@@ -204,8 +204,8 @@ module JsonLdHelper
     response.code == 501 || ((400...500).cover?(response.code) && ![401, 408, 429].include?(response.code))
   end
 
-  def build_request(uri, on_behalf_of = nil, options: {})
-    Request.new(:get, uri, **options).tap do |request|
+  def build_request(uri, on_behalf_of = nil)
+    Request.new(:get, uri).tap do |request|
       request.on_behalf_of(on_behalf_of) if on_behalf_of
       request.add_headers('Accept' => 'application/activity+json, application/ld+json')
     end

app/lib/application_extension.rb

@@ -4,14 +4,34 @@ module ApplicationExtension
   extend ActiveSupport::Concern
 
   included do
+    include Redisable
+
     has_many :created_users, class_name: 'User', foreign_key: 'created_by_application_id', inverse_of: :created_by_application
 
     validates :name, length: { maximum: 60 }
     validates :website, url: true, length: { maximum: 2_000 }, if: :website?
     validates :redirect_uri, length: { maximum: 2_000 }
+
+    # The relationship used between Applications and AccessTokens is using
+    # dependent: delete_all, which means the ActiveRecord callback in
+    # AccessTokenExtension is not run, so instead we manually announce to
+    # streaming that these tokens are being deleted.
+    before_destroy :push_to_streaming_api, prepend: true
   end
 
   def confirmation_redirect_uri
     redirect_uri.lines.first.strip
   end
+
+  def push_to_streaming_api
+    # TODO: #28793 Combine into a single topic
+    payload = Oj.dump(event: :kill)
+    access_tokens.in_batches do |tokens|
+      redis.pipelined do |pipeline|
+        tokens.ids.each do |id|
+          pipeline.publish("timeline:access_token:#{id}", payload)
+        end
+      end
+    end
+  end
 end

app/models/concerns/user/omniauthable.rb

@@ -19,17 +19,18 @@ module User::Omniauthable
   end
 
   class_methods do
-    def find_for_oauth(auth, signed_in_resource = nil)
+    def find_for_omniauth(auth, signed_in_resource = nil)
       # EOLE-SSO Patch
       auth.uid = (auth.uid[0][:uid] || auth.uid[0][:user]) if auth.uid.is_a? Hashie::Array
-      identity = Identity.find_for_oauth(auth)
+      identity = Identity.find_for_omniauth(auth)
 
       # If a signed_in_resource is provided it always overrides the existing user
       # to prevent the identity being locked with accidentally created accounts.
       # Note that this may leave zombie accounts (with no associated identity) which
       # can be cleaned up at a later date.
       user   = signed_in_resource || identity.user
-      user ||= create_for_oauth(auth)
+      user ||= reattach_for_auth(auth)
+      user ||= create_for_auth(auth)
 
       if identity.user.nil?
         identity.user = user
@@ -39,19 +40,35 @@ module User::Omniauthable
       user
     end
 
-    def create_for_oauth(auth)
-      # Check if the user exists with provided email. If no email was provided,
+    private
+
+    def reattach_for_auth(auth)
+      # If allowed, check if a user exists with the provided email address,
+      # and return it if they does not have an associated identity with the
+      # current authentication provider.
+
+      # This can be used to provide a choice of alternative auth providers
+      # or provide smooth gradual transition between multiple auth providers,
+      # but this is discouraged because any insecure provider will put *all*
+      # local users at risk, regardless of which provider they registered with.
+
+      return unless ENV['ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH'] == 'true'
+
+      email, email_is_verified = email_from_auth(auth)
+      return unless email_is_verified
+
+      user = User.find_by(email: email)
+      return if user.nil? || Identity.exists?(provider: auth.provider, user_id: user.id)
+
+      user
+    end
+
+    def create_for_auth(auth)
+      # Create a user for the given auth params. If no email was provided,
       # we assign a temporary email and ask the user to verify it on
       # the next step via Auth::SetupController.show
 
-      strategy          = Devise.omniauth_configs[auth.provider.to_sym].strategy
-      assume_verified   = strategy&.security&.assume_email_is_verified
-      email_is_verified = auth.info.verified || auth.info.verified_email || auth.info.email_verified || assume_verified
-      email             = auth.info.verified_email || auth.info.email
-
-      user = User.find_by(email: email) if email_is_verified
-
-      return user unless user.nil?
+      email, email_is_verified = email_from_auth(auth)
 
       user = User.new(user_params_from_auth(email, auth))
 
@@ -66,7 +83,14 @@ module User::Omniauthable
       user
     end
 
-    private
+    def email_from_auth(auth)
+      strategy          = Devise.omniauth_configs[auth.provider.to_sym].strategy
+      assume_verified   = strategy&.security&.assume_email_is_verified
+      email_is_verified = auth.info.verified || auth.info.verified_email || auth.info.email_verified || assume_verified
+      email             = auth.info.verified_email || auth.info.email
+
+      [email, email_is_verified]
+    end
 
     def user_params_from_auth(email, auth)
       {

app/models/identity.rb

@@ -17,7 +17,7 @@ class Identity < ApplicationRecord
   validates :uid, presence: true, uniqueness: { scope: :provider }
   validates :provider, presence: true
 
-  def self.find_for_oauth(auth)
+  def self.find_for_omniauth(auth)
     find_or_create_by(uid: auth.uid, provider: auth.provider)
   end
 end

app/models/user.rb

@@ -342,6 +342,16 @@ class User < ApplicationRecord
     Doorkeeper::AccessToken.by_resource_owner(self).in_batches do |batch|
       batch.update_all(revoked_at: Time.now.utc)
       Web::PushSubscription.where(access_token_id: batch).delete_all
+
+      # Revoke each access token for the Streaming API, since `update_all``
+      # doesn't trigger ActiveRecord Callbacks:
+      # TODO: #28793 Combine into a single topic
+      payload = Oj.dump(event: :kill)
+      redis.pipelined do |pipeline|
+        batch.ids.each do |id|
+          pipeline.publish("timeline:access_token:#{id}", payload)
+        end
+      end
     end
   end
 

config/initializers/doorkeeper.rb

@@ -21,9 +21,14 @@ Doorkeeper.configure do
     user unless user&.otp_required_for_login?
   end
 
-  # If you want to restrict access to the web interface for adding oauth authorized applications, you need to declare the block below.
+  # Doorkeeper provides some administrative interfaces for managing OAuth
+  # Applications, allowing creation, edit, and deletion of applications from the
+  # server. At present, these administrative routes are not integrated into
+  # Mastodon, and as such, we've disabled them by always return a 403 forbidden
+  # response for them. This does not affect the ability for users to manage
+  # their own OAuth Applications.
   admin_authenticator do
-    current_user&.admin? || redirect_to(new_user_session_url)
+    head 403
   end
 
   # Authorization Code expiration time (default 10 minutes).

config/routes.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-require 'sidekiq_unique_jobs/web'
+require 'sidekiq_unique_jobs/web' if ENV['ENABLE_SIDEKIQ_UNIQUE_JOBS_UI'] == true
 require 'sidekiq-scheduler/web'
 
 class RedirectWithVary < ActionDispatch::Routing::PathRedirect

lib/tasks/sidekiq_unique_jobs.rake

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+namespace :sidekiq_unique_jobs do
+  task delete_all_locks: :environment do
+    digests = SidekiqUniqueJobs::Digests.new
+    digests.delete_by_pattern('*', count: digests.count)
+
+    expiring_digests = SidekiqUniqueJobs::ExpiringDigests.new
+    expiring_digests.delete_by_pattern('*', count: expiring_digests.count)
+  end
+end

spec/models/identity_spec.rb

@@ -3,19 +3,19 @@
 require 'rails_helper'
 
 RSpec.describe Identity do
-  describe '.find_for_oauth' do
+  describe '.find_for_omniauth' do
     let(:auth) { Fabricate(:identity, user: Fabricate(:user)) }
 
     it 'calls .find_or_create_by' do
       allow(described_class).to receive(:find_or_create_by)
 
-      described_class.find_for_oauth(auth)
+      described_class.find_for_omniauth(auth)
 
       expect(described_class).to have_received(:find_or_create_by).with(uid: auth.uid, provider: auth.provider)
     end
 
     it 'returns an instance of Identity' do
-      expect(described_class.find_for_oauth(auth)).to be_instance_of described_class
+      expect(described_class.find_for_omniauth(auth)).to be_instance_of described_class
     end
   end
 end

spec/models/user_spec.rb

@@ -438,7 +438,10 @@ RSpec.describe User do
     let!(:access_token) { Fabricate(:access_token, resource_owner_id: user.id) }
     let!(:web_push_subscription) { Fabricate(:web_push_subscription, access_token: access_token) }
 
+    let(:redis_pipeline_stub) { instance_double(Redis::Namespace, publish: nil) }
+
     before do
+      allow(redis).to receive(:pipelined).and_yield(redis_pipeline_stub)
       user.reset_password!
     end
 
@@ -455,6 +458,10 @@ RSpec.describe User do
       expect(Doorkeeper::AccessToken.active_for(user).count).to eq 0
     end
 
+    it 'revokes streaming access for all access tokens' do
+      expect(redis_pipeline_stub).to have_received(:publish).with("timeline:access_token:#{access_token.id}", Oj.dump(event: :kill)).once
+    end
+
     it 'removes push subscriptions' do
       expect(Web::PushSubscription.where(user: user).or(Web::PushSubscription.where(access_token: access_token)).count).to eq 0
       expect { web_push_subscription.reload }.to raise_error(ActiveRecord::RecordNotFound)

spec/requests/disabled_oauth_endpoints_spec.rb

@@ -0,0 +1,83 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe 'Disabled OAuth routes' do
+  # These routes are disabled via the doorkeeper configuration for
+  # `admin_authenticator`, as these routes should only be accessible by server
+  # administrators. For now, these routes are not properly designed and
+  # integrated into Mastodon, so we're disabling them completely
+  describe 'GET /oauth/applications' do
+    it 'returns 403 forbidden' do
+      get oauth_applications_path
+
+      expect(response).to have_http_status(403)
+    end
+  end
+
+  describe 'POST /oauth/applications' do
+    it 'returns 403 forbidden' do
+      post oauth_applications_path
+
+      expect(response).to have_http_status(403)
+    end
+  end
+
+  describe 'GET /oauth/applications/new' do
+    it 'returns 403 forbidden' do
+      get new_oauth_application_path
+
+      expect(response).to have_http_status(403)
+    end
+  end
+
+  describe 'GET /oauth/applications/:id' do
+    let(:application) { Fabricate(:application, scopes: 'read') }
+
+    it 'returns 403 forbidden' do
+      get oauth_application_path(application)
+
+      expect(response).to have_http_status(403)
+    end
+  end
+
+  describe 'PATCH /oauth/applications/:id' do
+    let(:application) { Fabricate(:application, scopes: 'read') }
+
+    it 'returns 403 forbidden' do
+      patch oauth_application_path(application)
+
+      expect(response).to have_http_status(403)
+    end
+  end
+
+  describe 'PUT /oauth/applications/:id' do
+    let(:application) { Fabricate(:application, scopes: 'read') }
+
+    it 'returns 403 forbidden' do
+      put oauth_application_path(application)
+
+      expect(response).to have_http_status(403)
+    end
+  end
+
+  describe 'DELETE /oauth/applications/:id' do
+    let(:application) { Fabricate(:application, scopes: 'read') }
+
+    it 'returns 403 forbidden' do
+      delete oauth_application_path(application)
+
+      expect(response).to have_http_status(403)
+    end
+  end
+
+  describe 'GET /oauth/applications/:id/edit' do
+    let(:application) { Fabricate(:application, scopes: 'read') }
+
+    it 'returns 403 forbidden' do
+      get edit_oauth_application_path(application)
+
+      expect(response).to have_http_status(403)
+    end
+  end
+end

spec/requests/omniauth_callbacks_spec.rb

@@ -39,16 +39,33 @@ describe 'OmniAuth callbacks' do
           Fabricate(:user, email: 'user@host.example')
         end
 
-        it 'matches the existing user, creates an identity, and redirects to root path' do
-          expect { subject }
-            .to not_change(User, :count)
-            .and change(Identity, :count)
-            .by(1)
-            .and change(LoginActivity, :count)
-            .by(1)
+        context 'when ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH is set to true' do
+          around do |example|
+            ClimateControl.modify ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH: 'true' do
+              example.run
+            end
+          end
 
-          expect(Identity.find_by(user: User.last).uid).to eq('123')
-          expect(response).to redirect_to(root_path)
+          it 'matches the existing user, creates an identity, and redirects to root path' do
+            expect { subject }
+              .to not_change(User, :count)
+              .and change(Identity, :count)
+              .by(1)
+              .and change(LoginActivity, :count)
+              .by(1)
+
+            expect(Identity.find_by(user: User.last).uid).to eq('123')
+            expect(response).to redirect_to(root_path)
+          end
+        end
+
+        context 'when ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH is not set to true' do
+          it 'does not match the existing user or create an identity' do
+            expect { subject }
+              .to not_change(User, :count)
+              .and not_change(Identity, :count)
+              .and not_change(LoginActivity, :count)
+          end
         end
       end
 
@@ -96,7 +113,7 @@ describe 'OmniAuth callbacks' do
 
     context 'when a user cannot be built' do
       before do
-        allow(User).to receive(:find_for_oauth).and_return(User.new)
+        allow(User).to receive(:find_for_omniauth).and_return(User.new)
       end
 
       it 'redirects to the new user signup page' do