Merge b5a46cd7be into 2da2a1dae9

.env.test

@@ -4,7 +4,8 @@ NODE_ENV=production
 LOCAL_DOMAIN=cb6e6126.ngrok.io
 LOCAL_HTTPS=true
 
-# Required by ActiveRecord encryption feature
-ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY=fkSxKD2bF396kdQbrP1EJ7WbU7ZgNokR
-ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT=r0hvVmzBVsjxC7AMlwhOzmtc36ZCOS1E
-ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY=PhdFyyfy5xJ7WVd2lWBpcPScRQHzRTNr
+# Secret values required by ActiveRecord encryption feature
+# Use `bin/rails db:encryption:init` to generate fresh secrets
+ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY=test_determinist_key_DO_NOT_USE_IN_PRODUCTION
+ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT=test_salt_DO_NOT_USE_IN_PRODUCTION
+ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY=test_primary_key_DO_NOT_USE_IN_PRODUCTION

app/controllers/api/v1/apps/credentials_controller.rb

@@ -4,6 +4,6 @@ class Api::V1::Apps::CredentialsController < Api::BaseController
   def show
     return doorkeeper_render_error unless valid_doorkeeper_token?
 
-    render json: doorkeeper_token.application, serializer: REST::ApplicationSerializer, fields: %i(name website vapid_key client_id scopes)
+    render json: doorkeeper_token.application, serializer: REST::ApplicationSerializer
   end
 end

app/controllers/api/v1/apps_controller.rb

@@ -5,7 +5,7 @@ class Api::V1::AppsController < Api::BaseController
 
   def create
     @app = Doorkeeper::Application.create!(application_options)
-    render json: @app, serializer: REST::ApplicationSerializer
+    render json: @app, serializer: REST::CredentialApplicationSerializer
   end
 
   private
@@ -24,6 +24,6 @@ class Api::V1::AppsController < Api::BaseController
   end
 
   def app_params
-    params.permit(:client_name, :redirect_uris, :scopes, :website)
+    params.permit(:client_name, :scopes, :website, :redirect_uris, redirect_uris: [])
   end
 end

app/lib/application_extension.rb

@@ -23,6 +23,12 @@ module ApplicationExtension
     redirect_uri.lines.first.strip
   end
 
+  def redirect_uris
+    # Doorkeeper stores the redirect_uri value as a newline delimeted list in
+    # the database:
+    redirect_uri.split
+  end
+
   def push_to_streaming_api
     # TODO: #28793 Combine into a single topic
     payload = Oj.dump(event: :kill)

app/lib/vacuum/applications_vacuum.rb

@@ -1,10 +0,0 @@
-# frozen_string_literal: true
-
-class Vacuum::ApplicationsVacuum
-  def perform
-    Doorkeeper::Application.where(owner_id: nil)
-                           .where.missing(:created_users, :access_tokens, :access_grants)
-                           .where(created_at: ...1.day.ago)
-                           .in_batches.delete_all
-  end
-end

app/models/admin/action_log_filter.rb

@@ -59,6 +59,7 @@ class Admin::ActionLogFilter
     unsuspend_account: { target_type: 'Account', action: 'unsuspend' }.freeze,
     update_announcement: { target_type: 'Announcement', action: 'update' }.freeze,
     update_custom_emoji: { target_type: 'CustomEmoji', action: 'update' }.freeze,
+    update_report: { target_type: 'Report', action: 'update' }.freeze,
     update_status: { target_type: 'Status', action: 'update' }.freeze,
     update_user_role: { target_type: 'UserRole', action: 'update' }.freeze,
     update_ip_block: { target_type: 'IpBlock', action: 'update' }.freeze,

app/serializers/rest/application_serializer.rb

@@ -1,24 +1,18 @@
 # frozen_string_literal: true
 
 class REST::ApplicationSerializer < ActiveModel::Serializer
-  attributes :id, :name, :website, :scopes, :redirect_uri,
-             :client_id, :client_secret
+  attributes :id, :name, :website, :scopes, :redirect_uris
 
   # NOTE: Deprecated in 4.3.0, needs to be removed in 5.0.0
   attribute :vapid_key
 
+  # We should consider this property deprecated for 4.3.0
+  attribute :redirect_uri
+
   def id
     object.id.to_s
   end
 
-  def client_id
-    object.uid
-  end
-
-  def client_secret
-    object.secret
-  end
-
   def website
     object.website.presence
   end

app/serializers/rest/credential_application_serializer.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class REST::CredentialApplicationSerializer < REST::ApplicationSerializer
+  attributes :client_id, :client_secret
+
+  def client_id
+    object.uid
+  end
+
+  def client_secret
+    object.secret
+  end
+end

app/workers/scheduler/vacuum_scheduler.rb

@@ -22,7 +22,6 @@ class Scheduler::VacuumScheduler
       preview_cards_vacuum,
       backups_vacuum,
       access_tokens_vacuum,
-      applications_vacuum,
       feeds_vacuum,
       imports_vacuum,
     ]
@@ -56,10 +55,6 @@ class Scheduler::VacuumScheduler
     Vacuum::ImportsVacuum.new
   end
 
-  def applications_vacuum
-    Vacuum::ApplicationsVacuum.new
-  end
-
   def content_retention_policy
     ContentRetentionPolicy.current
   end

config/initializers/active_record_encryption.rb

@@ -5,7 +5,7 @@
   ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT
   ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY
 ).each do |key|
-  ENV.fetch(key) do
+  value = ENV.fetch(key) do
     abort <<~MESSAGE
 
       Mastodon now requires that these variables are set:
@@ -14,9 +14,18 @@
         - ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT
         - ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY
 
-      Run `bin/rails db:encryption:init` to generate values and then assign the environment variables.
+      Run `bin/rails db:encryption:init` to generate new secrets and then assign the environment variables.
     MESSAGE
   end
+
+  next unless Rails.env.production? && value.end_with?('DO_NOT_USE_IN_PRODUCTION')
+
+  abort <<~MESSAGE
+
+    It looks like you are trying to run Mastodon in production with a #{key} value from the test environment.
+
+    Please generate fresh secrets using `bin/rails db:encryption:init` and use them instead.
+  MESSAGE
 end
 
 Rails.application.configure do

config/initializers/rack_attack.rb

@@ -105,6 +105,10 @@ class Rack::Attack
     req.authenticated_user_id if (req.post? && req.path.match?(API_DELETE_REBLOG_REGEX)) || (req.delete? && req.path.match?(API_DELETE_STATUS_REGEX))
   end
 
+  throttle('throttle_oauth_application_registrations/ip', limit: 5, period: 30.minutes) do |req|
+    req.throttleable_remote_ip if req.post? && req.path == '/api/v1/apps'
+  end
+
   throttle('throttle_sign_up_attempts/ip', limit: 25, period: 5.minutes) do |req|
     req.throttleable_remote_ip if req.post? && req.path_matches?('/auth')
   end

config/locales/en.yml

@@ -285,6 +285,7 @@ en:
         update_custom_emoji_html: "%{name} updated emoji %{target}"
         update_domain_block_html: "%{name} updated domain block for %{target}"
         update_ip_block_html: "%{name} changed rule for IP %{target}"
+        update_report_html: "%{name} updated report %{target}"
         update_status_html: "%{name} updated post by %{target}"
         update_user_role_html: "%{name} changed %{target} role"
       deleted_account: deleted account

lib/tasks/db.rake

@@ -8,7 +8,7 @@ namespace :db do
     desc 'Generate a set of keys for configuring Active Record encryption in a given environment'
     task :init do # rubocop:disable Rails/RakeEnvironment
       puts <<~MSG
-        Add these environment variables to your Mastodon environment:#{' '}
+        Add these secret environment variables to your Mastodon environment (e.g. .env.production):#{' '}
 
         ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY=#{SecureRandom.alphanumeric(32)}
         ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT=#{SecureRandom.alphanumeric(32)}

spec/config/initializers/rack/attack_spec.rb

@@ -131,4 +131,22 @@ describe Rack::Attack, type: :request do
       it_behaves_like 'throttled endpoint'
     end
   end
+
+  describe 'throttle excessive oauth application registration requests by IP address' do
+    let(:throttle) { 'throttle_oauth_application_registrations/ip' }
+    let(:limit)  { 5 }
+    let(:period) { 30.minutes }
+    let(:path)   { '/api/v1/apps' }
+    let(:params) do
+      {
+        client_name: 'Throttle Test',
+        redirect_uris: 'urn:ietf:wg:oauth:2.0:oob',
+        scopes: 'read',
+      }
+    end
+
+    let(:request) { -> { post path, params: params, headers: { 'REMOTE_ADDR' => remote_ip } } }
+
+    it_behaves_like 'throttled endpoint'
+  end
 end

spec/lib/vacuum/applications_vacuum_spec.rb

@@ -1,48 +0,0 @@
-# frozen_string_literal: true
-
-require 'rails_helper'
-
-RSpec.describe Vacuum::ApplicationsVacuum do
-  subject { described_class.new }
-
-  describe '#perform' do
-    let!(:app_with_token)  { Fabricate(:application, created_at: 1.month.ago) }
-    let!(:app_with_grant)  { Fabricate(:application, created_at: 1.month.ago) }
-    let!(:app_with_signup) { Fabricate(:application, created_at: 1.month.ago) }
-    let!(:app_with_owner)  { Fabricate(:application, created_at: 1.month.ago, owner: Fabricate(:user)) }
-    let!(:unused_app)      { Fabricate(:application, created_at: 1.month.ago) }
-    let!(:recent_app)      { Fabricate(:application, created_at: 1.hour.ago) }
-
-    before do
-      Fabricate(:access_token, application: app_with_token)
-      Fabricate(:access_grant, application: app_with_grant)
-      Fabricate(:user, created_by_application: app_with_signup)
-
-      subject.perform
-    end
-
-    it 'does not delete applications with valid access tokens' do
-      expect { app_with_token.reload }.to_not raise_error
-    end
-
-    it 'does not delete applications with valid access grants' do
-      expect { app_with_grant.reload }.to_not raise_error
-    end
-
-    it 'does not delete applications that were used to create users' do
-      expect { app_with_signup.reload }.to_not raise_error
-    end
-
-    it 'does not delete owned applications' do
-      expect { app_with_owner.reload }.to_not raise_error
-    end
-
-    it 'does not delete applications registered less than a day ago' do
-      expect { recent_app.reload }.to_not raise_error
-    end
-
-    it 'deletes unused applications' do
-      expect { unused_app.reload }.to raise_error ActiveRecord::RecordNotFound
-    end
-  end
-end

spec/requests/api/v1/apps/credentials_spec.rb

@@ -20,14 +20,26 @@ describe 'Credentials' do
 
         expect(body_as_json).to match(
           a_hash_including(
+            id: token.application.id.to_s,
             name: token.application.name,
             website: token.application.website,
-            vapid_key: Rails.configuration.x.vapid_public_key,
             scopes: token.application.scopes.map(&:to_s),
-            client_id: token.application.uid
+            redirect_uris: token.application.redirect_uris,
+            # Deprecated properties as of 4.3:
+            redirect_uri: token.application.redirect_uri.split.first,
+            vapid_key: Rails.configuration.x.vapid_public_key
           )
         )
       end
+
+      it 'does not expose the client_id or client_secret' do
+        subject
+
+        expect(response).to have_http_status(200)
+
+        expect(body_as_json[:client_id]).to_not be_present
+        expect(body_as_json[:client_secret]).to_not be_present
+      end
     end
 
     context 'with a non-read scoped oauth token' do
@@ -46,11 +58,14 @@ describe 'Credentials' do
 
         expect(body_as_json).to match(
           a_hash_including(
+            id: token.application.id.to_s,
             name: token.application.name,
             website: token.application.website,
-            vapid_key: Rails.configuration.x.vapid_public_key,
             scopes: token.application.scopes.map(&:to_s),
-            client_id: token.application.uid
+            redirect_uris: token.application.redirect_uris,
+            # Deprecated properties as of 4.3:
+            redirect_uri: token.application.redirect_uri.split.first,
+            vapid_key: Rails.configuration.x.vapid_public_key
           )
         )
       end

spec/requests/api/v1/apps_spec.rb

@@ -9,8 +9,9 @@ RSpec.describe 'Apps' do
     end
 
     let(:client_name)   { 'Test app' }
-    let(:scopes)        { nil }
-    let(:redirect_uris) { 'urn:ietf:wg:oauth:2.0:oob' }
+    let(:scopes)        { 'read write' }
+    let(:redirect_uri)  { 'urn:ietf:wg:oauth:2.0:oob' }
+    let(:redirect_uris) { [redirect_uri] }
     let(:website)       { nil }
 
     let(:params) do
@@ -26,13 +27,63 @@ RSpec.describe 'Apps' do
       it 'creates an OAuth app', :aggregate_failures do
         subject
 
+        expect(response).to have_http_status(200)
+
+        app = Doorkeeper::Application.find_by(name: client_name)
+
+        expect(app).to be_present
+        expect(app.scopes.to_s).to eq scopes
+        expect(app.redirect_uris).to eq redirect_uris
+
+        expect(body_as_json).to match(
+          a_hash_including(
+            id: app.id.to_s,
+            client_id: app.uid,
+            client_secret: app.secret,
+            name: client_name,
+            website: website,
+            scopes: ['read', 'write'],
+            redirect_uris: redirect_uris,
+            # Deprecated properties as of 4.3:
+            redirect_uri: redirect_uri,
+            vapid_key: Rails.configuration.x.vapid_public_key
+          )
+        )
+      end
+    end
+
+    context 'without scopes being supplied' do
+      let(:scopes) { nil }
+
+      it 'creates an OAuth App with the default scope' do
+        subject
+
         expect(response).to have_http_status(200)
         expect(Doorkeeper::Application.find_by(name: client_name)).to be_present
 
         body = body_as_json
 
-        expect(body[:client_id]).to be_present
-        expect(body[:client_secret]).to be_present
+        expect(body[:scopes]).to eq Doorkeeper.config.default_scopes.to_a
+      end
+    end
+
+    # FIXME: This is a bug: https://github.com/mastodon/mastodon/issues/30152
+    context 'with scopes as an array' do
+      let(:scopes) { %w(read write follow) }
+
+      it 'creates an OAuth App with the default scope' do
+        subject
+
+        expect(response).to have_http_status(200)
+
+        app = Doorkeeper::Application.find_by(name: client_name)
+
+        expect(app).to be_present
+        expect(app.scopes.to_s).to eq 'read'
+
+        body = body_as_json
+
+        expect(body[:scopes]).to eq ['read']
       end
     end
 
@@ -77,8 +128,8 @@ RSpec.describe 'Apps' do
       end
     end
 
-    context 'with a too-long redirect_uris' do
-      let(:redirect_uris) { "https://foo.bar/#{'hoge' * 2_000}" }
+    context 'with a too-long redirect_uri' do
+      let(:redirect_uris) { "https://app.example/#{'hoge' * 2_000}" }
 
       it 'returns http unprocessable entity' do
         subject
@@ -87,8 +138,80 @@ RSpec.describe 'Apps' do
       end
     end
 
-    context 'without required params' do
-      let(:client_name)   { '' }
+    # NOTE: This spec currently tests the same as the "with a too-long redirect_uri test case"
+    context 'with too many redirect_uris' do
+      let(:redirect_uris) { (0...500).map { |i| "https://app.example/#{i}/callback" } }
+
+      it 'returns http unprocessable entity' do
+        subject
+
+        expect(response).to have_http_status(422)
+      end
+    end
+
+    context 'with multiple redirect_uris as a string' do
+      let(:redirect_uris) { "https://redirect1.example/\napp://redirect2.example/" }
+
+      it 'creates an OAuth application with multiple redirect URIs' do
+        subject
+
+        expect(response).to have_http_status(200)
+
+        app = Doorkeeper::Application.find_by(name: client_name)
+
+        expect(app).to be_present
+        expect(app.redirect_uri).to eq redirect_uris
+        expect(app.redirect_uris).to eq redirect_uris.split
+
+        body = body_as_json
+
+        expect(body[:redirect_uri]).to eq redirect_uris
+        expect(body[:redirect_uris]).to eq redirect_uris.split
+      end
+    end
+
+    context 'with multiple redirect_uris as an array' do
+      let(:redirect_uris) { ['https://redirect1.example/', 'app://redirect2.example/'] }
+
+      it 'creates an OAuth application with multiple redirect URIs' do
+        subject
+
+        expect(response).to have_http_status(200)
+
+        app = Doorkeeper::Application.find_by(name: client_name)
+
+        expect(app).to be_present
+        expect(app.redirect_uri).to eq redirect_uris.join "\n"
+        expect(app.redirect_uris).to eq redirect_uris
+
+        body = body_as_json
+
+        expect(body[:redirect_uri]).to eq redirect_uris.join "\n"
+        expect(body[:redirect_uris]).to eq redirect_uris
+      end
+    end
+
+    context 'with an empty redirect_uris array' do
+      let(:redirect_uris) { [] }
+
+      it 'returns http unprocessable entity' do
+        subject
+
+        expect(response).to have_http_status(422)
+      end
+    end
+
+    context 'with just a newline as the redirect_uris string' do
+      let(:redirect_uris) { "\n" }
+
+      it 'returns http unprocessable entity' do
+        subject
+
+        expect(response).to have_http_status(422)
+      end
+    end
+
+    context 'with an empty redirect_uris string' do
       let(:redirect_uris) { '' }
 
       it 'returns http unprocessable entity' do
@@ -97,5 +220,30 @@ RSpec.describe 'Apps' do
         expect(response).to have_http_status(422)
       end
     end
+
+    context 'without a required param' do
+      let(:client_name) { '' }
+
+      it 'returns http unprocessable entity' do
+        subject
+
+        expect(response).to have_http_status(422)
+      end
+    end
+
+    context 'with a website' do
+      let(:website) { 'https://app.example/' }
+
+      it 'creates an OAuth application with the website specified' do
+        subject
+
+        expect(response).to have_http_status(200)
+
+        app = Doorkeeper::Application.find_by(name: client_name)
+
+        expect(app).to be_present
+        expect(app.website).to eq website
+      end
+    end
   end
 end