From 0a5f0a8b200e419df9f58bd8ffb581a0c0632c0f Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Sun, 23 Apr 2023 16:35:54 -0400 Subject: [PATCH] Remove instance variables from helper usage (#24203) --- .rubocop_todo.yml | 8 -------- app/controllers/application_controller.rb | 5 +++++ app/helpers/application_helper.rb | 2 +- app/helpers/instance_helper.rb | 18 +++++++++++------- app/helpers/jsonld_helper.rb | 8 ++++---- app/lib/activitypub/activity/create.rb | 4 ++-- app/lib/activitypub/activity/delete.rb | 2 +- app/lib/activitypub/activity/flag.rb | 2 +- app/lib/activitypub/activity/update.rb | 2 +- app/lib/activitypub/dereferencer.rb | 11 +---------- .../fetch_featured_collection_service.rb | 4 ++-- .../fetch_featured_tags_collection_service.rb | 2 +- .../activitypub/fetch_replies_service.rb | 4 ++-- ...repare_followers_synchronization_service.rb | 2 +- .../synchronize_followers_service.rb | 2 +- app/views/auth/registrations/new.html.haml | 2 +- app/views/auth/registrations/rules.html.haml | 2 +- spec/controllers/shares_controller_spec.rb | 2 +- spec/helpers/application_helper_spec.rb | 16 ++++++++++++++++ 19 files changed, 53 insertions(+), 45 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index d803602c06..2e9de6b8d5 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1400,14 +1400,6 @@ Rails/HasManyOrHasOneDependent: - 'app/models/user.rb' - 'app/models/web/push_subscription.rb' -# Configuration parameters: Include. -# Include: app/helpers/**/*.rb -Rails/HelperInstanceVariable: - Exclude: - - 'app/helpers/application_helper.rb' - - 'app/helpers/instance_helper.rb' - - 'app/helpers/jsonld_helper.rb' - # This cop supports safe autocorrection (--autocorrect). # Configuration parameters: Include. # Include: spec/**/*, test/**/* diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index c06aae10b0..d59fe88105 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -19,6 +19,7 @@ class ApplicationController < ActionController::Base helper_method :omniauth_only? helper_method :sso_account_settings helper_method :whitelist_mode? + helper_method :body_class_string rescue_from ActionController::ParameterMissing, Paperclip::AdapterRegistry::NoHandlerError, with: :bad_request rescue_from Mastodon::NotPermittedError, with: :forbidden @@ -148,6 +149,10 @@ class ApplicationController < ActionController::Base current_user.setting_theme end + def body_class_string + @body_classes || '' + end + def respond_with_error(code) respond_to do |format| format.any { render "errors/#{code}", layout: 'error', status: code, formats: [:html] } diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 828688b04d..9a30cfbc6b 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -168,7 +168,7 @@ module ApplicationHelper end def body_classes - output = (@body_classes || '').split + output = body_class_string.split output << "theme-#{current_theme.parameterize}" output << 'system-font' if current_account&.user&.setting_system_font_ui output << (current_account&.user&.setting_reduce_motion ? 'reduce-motion' : 'no-reduce-motion') diff --git a/app/helpers/instance_helper.rb b/app/helpers/instance_helper.rb index bedfe6f304..893afdd51f 100644 --- a/app/helpers/instance_helper.rb +++ b/app/helpers/instance_helper.rb @@ -9,13 +9,17 @@ module InstanceHelper @site_hostname ||= Addressable::URI.parse("//#{Rails.configuration.x.local_domain}").display_uri.host end - def description_for_sign_up - prefix = if @invite.present? - I18n.t('auth.description.prefix_invited_by_user', name: @invite.user.account.username) - else - I18n.t('auth.description.prefix_sign_up') - end + def description_for_sign_up(invite = nil) + safe_join([description_prefix(invite), I18n.t('auth.description.suffix')], ' ') + end - safe_join([prefix, I18n.t('auth.description.suffix')], ' ') + private + + def description_prefix(invite) + if invite.present? + I18n.t('auth.description.prefix_invited_by_user', name: invite.user.account.username) + else + I18n.t('auth.description.prefix_sign_up') + end end end diff --git a/app/helpers/jsonld_helper.rb b/app/helpers/jsonld_helper.rb index 24362b61e7..ce3ff094f6 100644 --- a/app/helpers/jsonld_helper.rb +++ b/app/helpers/jsonld_helper.rb @@ -63,11 +63,11 @@ module JsonLdHelper uri.nil? || !uri.start_with?('http://', 'https://') end - def invalid_origin?(url) - return true if unsupported_uri_scheme?(url) + def non_matching_uri_hosts?(base_url, comparison_url) + return true if unsupported_uri_scheme?(comparison_url) - needle = Addressable::URI.parse(url).host - haystack = Addressable::URI.parse(@account.uri).host + needle = Addressable::URI.parse(comparison_url).host + haystack = Addressable::URI.parse(base_url).host !haystack.casecmp(needle).zero? end diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index e2355bfbcc..23fbabf4cc 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -17,7 +17,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity private def create_encrypted_message - return reject_payload! if invalid_origin?(object_uri) || @options[:delivered_to_account_id].blank? + return reject_payload! if non_matching_uri_hosts?(@account.uri, object_uri) || @options[:delivered_to_account_id].blank? target_account = Account.find(@options[:delivered_to_account_id]) target_device = target_account.devices.find_by(device_id: @object.dig('to', 'deviceId')) @@ -45,7 +45,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity end def create_status - return reject_payload! if unsupported_object_type? || invalid_origin?(object_uri) || tombstone_exists? || !related_to_local_activity? + return reject_payload! if unsupported_object_type? || non_matching_uri_hosts?(@account.uri, object_uri) || tombstone_exists? || !related_to_local_activity? with_lock("create:#{object_uri}") do return if delete_arrived_first?(object_uri) || poll_vote? diff --git a/app/lib/activitypub/activity/delete.rb b/app/lib/activitypub/activity/delete.rb index 871eb39667..3af500f2b1 100644 --- a/app/lib/activitypub/activity/delete.rb +++ b/app/lib/activitypub/activity/delete.rb @@ -21,7 +21,7 @@ class ActivityPub::Activity::Delete < ActivityPub::Activity return if object_uri.nil? with_lock("delete_status_in_progress:#{object_uri}", raise_on_failure: false) do - unless invalid_origin?(object_uri) + unless non_matching_uri_hosts?(@account.uri, object_uri) # This lock ensures a concurrent `ActivityPub::Activity::Create` either # does not create a status at all, or has finished saving it to the # database before we try to load it. diff --git a/app/lib/activitypub/activity/flag.rb b/app/lib/activitypub/activity/flag.rb index b0443849a6..dc808ad364 100644 --- a/app/lib/activitypub/activity/flag.rb +++ b/app/lib/activitypub/activity/flag.rb @@ -33,6 +33,6 @@ class ActivityPub::Activity::Flag < ActivityPub::Activity end def report_uri - @json['id'] unless @json['id'].nil? || invalid_origin?(@json['id']) + @json['id'] unless @json['id'].nil? || non_matching_uri_hosts?(@account.uri, @json['id']) end end diff --git a/app/lib/activitypub/activity/update.rb b/app/lib/activitypub/activity/update.rb index e7c3bc9bf8..8e72e08237 100644 --- a/app/lib/activitypub/activity/update.rb +++ b/app/lib/activitypub/activity/update.rb @@ -22,7 +22,7 @@ class ActivityPub::Activity::Update < ActivityPub::Activity end def update_status - return reject_payload! if invalid_origin?(object_uri) + return reject_payload! if non_matching_uri_hosts?(@account.uri, object_uri) @status = Status.find_by(uri: object_uri, account_id: @account.id) diff --git a/app/lib/activitypub/dereferencer.rb b/app/lib/activitypub/dereferencer.rb index 4d7756d71d..eb99842828 100644 --- a/app/lib/activitypub/dereferencer.rb +++ b/app/lib/activitypub/dereferencer.rb @@ -40,7 +40,7 @@ class ActivityPub::Dereferencer end def perform_request(uri, headers: nil) - return if invalid_origin?(uri) + return if non_matching_uri_hosts?(@permitted_origin, uri) req = Request.new(:get, uri) @@ -57,13 +57,4 @@ class ActivityPub::Dereferencer end end end - - def invalid_origin?(uri) - return true if unsupported_uri_scheme?(uri) - - needle = Addressable::URI.parse(uri).host - haystack = Addressable::URI.parse(@permitted_origin).host - - !haystack.casecmp(needle).zero? - end end diff --git a/app/services/activitypub/fetch_featured_collection_service.rb b/app/services/activitypub/fetch_featured_collection_service.rb index 1208820df2..e8a31dade9 100644 --- a/app/services/activitypub/fetch_featured_collection_service.rb +++ b/app/services/activitypub/fetch_featured_collection_service.rb @@ -31,7 +31,7 @@ class ActivityPub::FetchFeaturedCollectionService < BaseService def fetch_collection(collection_or_uri) return collection_or_uri if collection_or_uri.is_a?(Hash) - return if invalid_origin?(collection_or_uri) + return if non_matching_uri_hosts?(@account.uri, collection_or_uri) fetch_resource_without_id_validation(collection_or_uri, local_follower, true) end @@ -46,7 +46,7 @@ class ActivityPub::FetchFeaturedCollectionService < BaseService next unless item.is_a?(String) || item['type'] == 'Note' uri = value_or_id(item) - next if ActivityPub::TagManager.instance.local_uri?(uri) || invalid_origin?(uri) + next if ActivityPub::TagManager.instance.local_uri?(uri) || non_matching_uri_hosts?(@account.uri, uri) status = ActivityPub::FetchRemoteStatusService.new.call(uri, on_behalf_of: local_follower, expected_actor_uri: @account.uri, request_id: @options[:request_id]) next unless status&.account_id == @account.id diff --git a/app/services/activitypub/fetch_featured_tags_collection_service.rb b/app/services/activitypub/fetch_featured_tags_collection_service.rb index ff1a88aa1e..cb71b37e75 100644 --- a/app/services/activitypub/fetch_featured_tags_collection_service.rb +++ b/app/services/activitypub/fetch_featured_tags_collection_service.rb @@ -43,7 +43,7 @@ class ActivityPub::FetchFeaturedTagsCollectionService < BaseService def fetch_collection(collection_or_uri) return collection_or_uri if collection_or_uri.is_a?(Hash) - return if invalid_origin?(collection_or_uri) + return if non_matching_uri_hosts?(@account.uri, collection_or_uri) fetch_resource_without_id_validation(collection_or_uri, local_follower, true) end diff --git a/app/services/activitypub/fetch_replies_service.rb b/app/services/activitypub/fetch_replies_service.rb index 3fe150ba21..b5c7759ec5 100644 --- a/app/services/activitypub/fetch_replies_service.rb +++ b/app/services/activitypub/fetch_replies_service.rb @@ -35,7 +35,7 @@ class ActivityPub::FetchRepliesService < BaseService def fetch_collection(collection_or_uri) return collection_or_uri if collection_or_uri.is_a?(Hash) return unless @allow_synchronous_requests - return if invalid_origin?(collection_or_uri) + return if non_matching_uri_hosts?(@account.uri, collection_or_uri) fetch_resource_without_id_validation(collection_or_uri, nil, true) end @@ -45,6 +45,6 @@ class ActivityPub::FetchRepliesService < BaseService # amplification attacks. # Also limit to 5 fetched replies to limit potential for DoS. - @items.map { |item| value_or_id(item) }.reject { |uri| invalid_origin?(uri) }.take(5) + @items.map { |item| value_or_id(item) }.reject { |uri| non_matching_uri_hosts?(@account.uri, uri) }.take(5) end end diff --git a/app/services/activitypub/prepare_followers_synchronization_service.rb b/app/services/activitypub/prepare_followers_synchronization_service.rb index 2d22ed701e..56ec0e44bf 100644 --- a/app/services/activitypub/prepare_followers_synchronization_service.rb +++ b/app/services/activitypub/prepare_followers_synchronization_service.rb @@ -6,7 +6,7 @@ class ActivityPub::PrepareFollowersSynchronizationService < BaseService def call(account, params) @account = account - return if params['collectionId'] != @account.followers_url || invalid_origin?(params['url']) || @account.local_followers_hash == params['digest'] + return if params['collectionId'] != @account.followers_url || non_matching_uri_hosts?(@account.uri, params['url']) || @account.local_followers_hash == params['digest'] ActivityPub::FollowersSynchronizationWorker.perform_async(@account.id, params['url']) end diff --git a/app/services/activitypub/synchronize_followers_service.rb b/app/services/activitypub/synchronize_followers_service.rb index 93cd602533..9bd6034a57 100644 --- a/app/services/activitypub/synchronize_followers_service.rb +++ b/app/services/activitypub/synchronize_followers_service.rb @@ -67,7 +67,7 @@ class ActivityPub::SynchronizeFollowersService < BaseService def fetch_collection(collection_or_uri) return collection_or_uri if collection_or_uri.is_a?(Hash) - return if invalid_origin?(collection_or_uri) + return if non_matching_uri_hosts?(@account.uri, collection_or_uri) fetch_resource_without_id_validation(collection_or_uri, nil, true) end diff --git a/app/views/auth/registrations/new.html.haml b/app/views/auth/registrations/new.html.haml index 4df0f95d51..18511a5964 100644 --- a/app/views/auth/registrations/new.html.haml +++ b/app/views/auth/registrations/new.html.haml @@ -2,7 +2,7 @@ = t('auth.register') - content_for :header_tags do - = render partial: 'shared/og', locals: { description: description_for_sign_up } + = render partial: 'shared/og', locals: { description: description_for_sign_up(@invite) } = simple_form_for(resource, as: resource_name, url: registration_path(resource_name), html: { novalidate: false }) do |f| = render 'auth/shared/progress', stage: 'details' diff --git a/app/views/auth/registrations/rules.html.haml b/app/views/auth/registrations/rules.html.haml index aa16ab9573..ab3fa864ab 100644 --- a/app/views/auth/registrations/rules.html.haml +++ b/app/views/auth/registrations/rules.html.haml @@ -2,7 +2,7 @@ = t('auth.register') - content_for :header_tags do - = render partial: 'shared/og', locals: { description: description_for_sign_up } + = render partial: 'shared/og', locals: { description: description_for_sign_up(@invite) } .simple_form = render 'auth/shared/progress', stage: 'rules' diff --git a/spec/controllers/shares_controller_spec.rb b/spec/controllers/shares_controller_spec.rb index 6d5bb4f8d8..5dcc46e47a 100644 --- a/spec/controllers/shares_controller_spec.rb +++ b/spec/controllers/shares_controller_spec.rb @@ -9,7 +9,7 @@ describe SharesController do before { sign_in user } - describe 'GTE #show' do + describe 'GET #show' do subject(:body_classes) { assigns(:body_classes) } before { get :show, params: { title: 'test title', text: 'test text', url: 'url1 url2' } } diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 2db2ee288e..88751548fc 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -27,6 +27,22 @@ describe ApplicationHelper do end end + describe 'body_classes' do + context 'with a body class string from a controller' do + before do + without_partial_double_verification do + allow(helper).to receive(:body_class_string).and_return('modal-layout compose-standalone') + allow(helper).to receive(:current_theme).and_return('default') + allow(helper).to receive(:current_account).and_return(Fabricate(:account)) + end + end + + it 'uses the controller body classes in the result' do + expect(helper.body_classes).to match(/modal-layout compose-standalone/) + end + end + end + describe 'locale_direction' do around do |example| current_locale = I18n.locale