From f2e7b74d888dba5da8641636370ea740fb38e06b Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 1 Feb 2018 12:26:45 +1100 Subject: [PATCH] FIX: don't return 200s when login is required to paths When running `ensure_login_required` it should always happen prior to `check_xhr` cause check xhr will trigger a 200 response --- app/controllers/about_controller.rb | 2 +- app/controllers/admin/admin_controller.rb | 4 +- .../admin/embeddable_hosts_controller.rb | 2 - app/controllers/admin/embedding_controller.rb | 2 +- app/controllers/categories_controller.rb | 2 +- .../category_hashtags_controller.rb | 2 +- app/controllers/composer_controller.rb | 2 +- .../composer_messages_controller.rb | 2 +- app/controllers/draft_controller.rb | 3 +- app/controllers/email_controller.rb | 2 +- app/controllers/groups_controller.rb | 2 +- app/controllers/inline_onebox_controller.rb | 2 +- app/controllers/invites_controller.rb | 6 ++- app/controllers/notifications_controller.rb | 2 +- app/controllers/onebox_controller.rb | 2 +- app/controllers/post_actions_controller.rb | 2 +- app/controllers/posts_controller.rb | 2 +- app/controllers/steps_controller.rb | 2 +- app/controllers/tag_groups_controller.rb | 2 +- app/controllers/tags_controller.rb | 9 ++-- app/controllers/topics_controller.rb | 51 ++++++++++--------- app/controllers/uploads_controller.rb | 2 +- app/controllers/user_api_keys_controller.rb | 2 +- app/controllers/users_controller.rb | 14 +++-- app/controllers/users_email_controller.rb | 2 +- app/controllers/wizard_controller.rb | 5 +- spec/controllers/wizard_controller_spec.rb | 7 +++ spec/requests/admin/admin_controller_spec.rb | 3 ++ 28 files changed, 81 insertions(+), 59 deletions(-) diff --git a/app/controllers/about_controller.rb b/app/controllers/about_controller.rb index 0ca88b6945f..48c401dee9a 100644 --- a/app/controllers/about_controller.rb +++ b/app/controllers/about_controller.rb @@ -1,8 +1,8 @@ require_dependency 'rate_limiter' class AboutController < ApplicationController + prepend_before_action :check_xhr, :ensure_logged_in, only: [:live_post_counts] skip_before_action :check_xhr, only: [:index] - before_action :ensure_logged_in, only: [:live_post_counts] def index return redirect_to path('/login') if SiteSetting.login_required? && current_user.nil? diff --git a/app/controllers/admin/admin_controller.rb b/app/controllers/admin/admin_controller.rb index 1b786ac0dec..e4dd34119f6 100644 --- a/app/controllers/admin/admin_controller.rb +++ b/app/controllers/admin/admin_controller.rb @@ -1,7 +1,7 @@ class Admin::AdminController < ApplicationController - before_action :ensure_logged_in - before_action :ensure_staff + prepend_before_action :check_xhr, :ensure_logged_in + prepend_before_action :check_xhr, :ensure_staff def index render body: nil diff --git a/app/controllers/admin/embeddable_hosts_controller.rb b/app/controllers/admin/embeddable_hosts_controller.rb index f43ff955050..667db524fe8 100644 --- a/app/controllers/admin/embeddable_hosts_controller.rb +++ b/app/controllers/admin/embeddable_hosts_controller.rb @@ -1,7 +1,5 @@ class Admin::EmbeddableHostsController < Admin::AdminController - before_action :ensure_logged_in, :ensure_staff - def create save_host(EmbeddableHost.new) end diff --git a/app/controllers/admin/embedding_controller.rb b/app/controllers/admin/embedding_controller.rb index ada7c668b9c..ebac31e576e 100644 --- a/app/controllers/admin/embedding_controller.rb +++ b/app/controllers/admin/embedding_controller.rb @@ -2,7 +2,7 @@ require_dependency 'embedding' class Admin::EmbeddingController < Admin::AdminController - before_action :ensure_logged_in, :ensure_staff, :fetch_embedding + before_action :fetch_embedding def show render_serialized(@embedding, EmbeddingSerializer, root: 'embedding', rest_serializer: true) diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index 56c49b9da9b..7b9a25ec5cd 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -2,7 +2,7 @@ require_dependency 'category_serializer' class CategoriesController < ApplicationController - before_action :ensure_logged_in, except: [:index, :categories_and_latest, :show, :redirect, :find_by_slug] + prepend_before_action :check_xhr, :ensure_logged_in, except: [:index, :categories_and_latest, :show, :redirect, :find_by_slug] before_action :fetch_category, only: [:show, :update, :destroy] before_action :initialize_staff_action_logger, only: [:create, :update, :destroy] skip_before_action :check_xhr, only: [:index, :categories_and_latest, :redirect] diff --git a/app/controllers/category_hashtags_controller.rb b/app/controllers/category_hashtags_controller.rb index e0b6f768d7b..34e8bb0367c 100644 --- a/app/controllers/category_hashtags_controller.rb +++ b/app/controllers/category_hashtags_controller.rb @@ -1,5 +1,5 @@ class CategoryHashtagsController < ApplicationController - before_action :ensure_logged_in + prepend_before_action :check_xhr, :ensure_logged_in def check category_slugs = params[:category_slugs] diff --git a/app/controllers/composer_controller.rb b/app/controllers/composer_controller.rb index 6eb78a5d061..7d103c634e9 100644 --- a/app/controllers/composer_controller.rb +++ b/app/controllers/composer_controller.rb @@ -2,7 +2,7 @@ require_dependency 'html_to_markdown' class ComposerController < ApplicationController - before_action :ensure_logged_in + prepend_before_action :check_xhr, :ensure_logged_in def parse_html markdown_text = HtmlToMarkdown.new(params[:html]).to_markdown diff --git a/app/controllers/composer_messages_controller.rb b/app/controllers/composer_messages_controller.rb index 60bc8a75ba5..64b3bc53822 100644 --- a/app/controllers/composer_messages_controller.rb +++ b/app/controllers/composer_messages_controller.rb @@ -2,7 +2,7 @@ require_dependency 'composer_messages_finder' class ComposerMessagesController < ApplicationController - before_action :ensure_logged_in + prepend_before_action :check_xhr, :ensure_logged_in def index finder = ComposerMessagesFinder.new(current_user, params.slice(:composer_action, :topic_id, :post_id)) diff --git a/app/controllers/draft_controller.rb b/app/controllers/draft_controller.rb index 5734a0326a4..c3f9345c5b2 100644 --- a/app/controllers/draft_controller.rb +++ b/app/controllers/draft_controller.rb @@ -1,6 +1,5 @@ class DraftController < ApplicationController - before_action :ensure_logged_in - # TODO really do we need to skip this? + prepend_before_action :ensure_logged_in skip_before_action :check_xhr, :preload_json def show diff --git a/app/controllers/email_controller.rb b/app/controllers/email_controller.rb index 9701cfc096d..f0556f608dd 100644 --- a/app/controllers/email_controller.rb +++ b/app/controllers/email_controller.rb @@ -1,7 +1,7 @@ class EmailController < ApplicationController - skip_before_action :check_xhr, :preload_json, :redirect_to_login_if_required layout 'no_ember' + skip_before_action :check_xhr, :preload_json, :redirect_to_login_if_required before_action :ensure_logged_in, only: :preferences_redirect def preferences_redirect diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 3fde94894cb..e199c7cc89b 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -1,6 +1,6 @@ class GroupsController < ApplicationController - before_action :ensure_logged_in, only: [ + prepend_before_action :check_xhr, :ensure_logged_in, only: [ :set_notifications, :mentionable, :messageable, diff --git a/app/controllers/inline_onebox_controller.rb b/app/controllers/inline_onebox_controller.rb index c7f4e30f75c..33d2a61f271 100644 --- a/app/controllers/inline_onebox_controller.rb +++ b/app/controllers/inline_onebox_controller.rb @@ -1,7 +1,7 @@ require_dependency 'inline_oneboxer' class InlineOneboxController < ApplicationController - before_action :ensure_logged_in + prepend_before_action :check_xhr, :ensure_logged_in def show oneboxes = InlineOneboxer.new(params[:urls] || []).process diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index 9bd0c0e3918..aea0c9782ff 100644 --- a/app/controllers/invites_controller.rb +++ b/app/controllers/invites_controller.rb @@ -2,11 +2,15 @@ require_dependency 'rate_limiter' class InvitesController < ApplicationController + prepend_before_action :check_xhr, :ensure_logged_in, only: [ + :destroy, :create, :create_invite_link, :rescind_all_invites, + :resend_invite, :resend_all_invites, :upload_csv + ] + skip_before_action :check_xhr, except: [:perform_accept_invitation] skip_before_action :preload_json, except: [:show] skip_before_action :redirect_to_login_if_required - before_action :ensure_logged_in, only: [:destroy, :create, :create_invite_link, :rescind_all_invites, :resend_invite, :resend_all_invites, :upload_csv] before_action :ensure_new_registrations_allowed, only: [:show, :perform_accept_invitation] before_action :ensure_not_logged_in, only: [:show, :perform_accept_invitation] diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index bedfb6b515f..16780e44d9d 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -2,7 +2,7 @@ require_dependency 'notification_serializer' class NotificationsController < ApplicationController - before_action :ensure_logged_in + prepend_before_action :check_xhr, :ensure_logged_in def index user = diff --git a/app/controllers/onebox_controller.rb b/app/controllers/onebox_controller.rb index 15b99a76486..65b66b5964c 100644 --- a/app/controllers/onebox_controller.rb +++ b/app/controllers/onebox_controller.rb @@ -1,7 +1,7 @@ require_dependency 'oneboxer' class OneboxController < ApplicationController - before_action :ensure_logged_in + prepend_before_action :check_xhr, :ensure_logged_in def show unless params[:refresh] == 'true' diff --git a/app/controllers/post_actions_controller.rb b/app/controllers/post_actions_controller.rb index 2bf6027ba26..022a3072e2c 100644 --- a/app/controllers/post_actions_controller.rb +++ b/app/controllers/post_actions_controller.rb @@ -1,7 +1,7 @@ require_dependency 'discourse' class PostActionsController < ApplicationController - before_action :ensure_logged_in + prepend_before_action :check_xhr, :ensure_logged_in before_action :fetch_post_from_params before_action :fetch_post_action_type_id_from_params diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 639ee168c29..0ac234182d8 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -8,7 +8,7 @@ require_dependency 'post_locker' class PostsController < ApplicationController - before_action :ensure_logged_in, except: [ + prepend_before_action :check_xhr, :ensure_logged_in, except: [ :show, :replies, :by_number, diff --git a/app/controllers/steps_controller.rb b/app/controllers/steps_controller.rb index ebfd4b530d4..8a341d53d3f 100644 --- a/app/controllers/steps_controller.rb +++ b/app/controllers/steps_controller.rb @@ -5,7 +5,7 @@ require_dependency 'wizard/step_updater' class StepsController < ApplicationController before_action :ensure_wizard_enabled - before_action :ensure_logged_in + prepend_before_action :check_xhr, :ensure_logged_in before_action :ensure_admin def update diff --git a/app/controllers/tag_groups_controller.rb b/app/controllers/tag_groups_controller.rb index b55120954e1..e039fb6cb7c 100644 --- a/app/controllers/tag_groups_controller.rb +++ b/app/controllers/tag_groups_controller.rb @@ -1,6 +1,6 @@ class TagGroupsController < ApplicationController + prepend_before_action :check_xhr, :ensure_logged_in, except: [:index, :show] skip_before_action :check_xhr, only: [:index, :show] - before_action :ensure_logged_in, except: [:index, :show] before_action :fetch_tag_group, only: [:show, :update, :destroy] def index diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index a73416d04f3..0c63c7a7eed 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -7,8 +7,7 @@ class TagsController < ::ApplicationController before_action :ensure_tags_enabled - skip_before_action :check_xhr, only: [:tag_feed, :show, :index] - before_action :ensure_logged_in, except: [ + prepend_before_action :check_xhr, :ensure_logged_in, except: [ :index, :show, :tag_feed, @@ -16,7 +15,11 @@ class TagsController < ::ApplicationController :check_hashtag, Discourse.anonymous_filters.map { |f| :"show_#{f}" } ].flatten - before_action :set_category_from_params, except: [:index, :update, :destroy, :tag_feed, :search, :notifications, :update_notifications] + + skip_before_action :check_xhr, only: [:tag_feed, :show, :index] + + before_action :set_category_from_params, except: [:index, :update, :destroy, + :tag_feed, :search, :notifications, :update_notifications] def index @description_meta = I18n.t("tags.title") diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 439c5683439..6d4f0555344 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -6,31 +6,32 @@ require_dependency 'discourse_event' require_dependency 'rate_limiter' class TopicsController < ApplicationController - before_action :ensure_logged_in, only: [:timings, - :destroy_timings, - :update, - :star, - :destroy, - :recover, - :status, - :invite, - :mute, - :unmute, - :set_notifications, - :move_posts, - :merge_topic, - :clear_pin, - :re_pin, - :status_update, - :timer, - :bulk, - :reset_new, - :change_post_owners, - :change_timestamps, - :archive_message, - :move_to_inbox, - :convert_topic, - :bookmark] + prepend_before_action :check_xhr, :ensure_logged_in, only: [ + :timings, + :destroy_timings, + :update, + :destroy, + :recover, + :status, + :invite, + :mute, + :unmute, + :set_notifications, + :move_posts, + :merge_topic, + :clear_pin, + :re_pin, + :status_update, + :timer, + :bulk, + :reset_new, + :change_post_owners, + :change_timestamps, + :archive_message, + :move_to_inbox, + :convert_topic, + :bookmark + ] before_action :consider_user_for_promotion, only: :show diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index dcfaa4ed3c5..13754d3f274 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -2,7 +2,7 @@ require "mini_mime" require_dependency 'upload_creator' class UploadsController < ApplicationController - before_action :ensure_logged_in, except: [:show] + prepend_before_action :check_xhr, :ensure_logged_in, except: [:show] skip_before_action :preload_json, :check_xhr, :redirect_to_login_if_required, only: [:show] def create diff --git a/app/controllers/user_api_keys_controller.rb b/app/controllers/user_api_keys_controller.rb index c6d41d2cc30..2dbc75ea238 100644 --- a/app/controllers/user_api_keys_controller.rb +++ b/app/controllers/user_api_keys_controller.rb @@ -2,9 +2,9 @@ class UserApiKeysController < ApplicationController layout 'no_ember' + prepend_before_action :check_xhr, :ensure_logged_in, only: [:create, :revoke, :undo_revoke] skip_before_action :redirect_to_login_if_required, only: [:new] skip_before_action :check_xhr, :preload_json - before_action :ensure_logged_in, only: [:create, :revoke, :undo_revoke] AUTH_API_VERSION ||= 2 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index f4706ed8582..29e52290c44 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -8,10 +8,18 @@ require_dependency 'admin_confirmation' class UsersController < ApplicationController skip_before_action :authorize_mini_profiler, only: [:avatar] - skip_before_action :check_xhr, only: [:show, :badges, :password_reset, :update, :account_created, :activate_account, :perform_account_activation, :user_preferences_redirect, :avatar, :my_redirect, :toggle_anon, :admin_login, :confirm_admin] - before_action :ensure_logged_in, only: [:username, :update, :user_preferences_redirect, :upload_user_image, - :pick_avatar, :destroy_user_image, :destroy, :check_emails, :topic_tracking_state] + prepend_before_action :check_xhr, :ensure_logged_in, only: [ + :username, :update, :user_preferences_redirect, :upload_user_image, + :pick_avatar, :destroy_user_image, :destroy, :check_emails, :topic_tracking_state, + :preferences + ] + + skip_before_action :check_xhr, only: [ + :show, :badges, :password_reset, :update, :account_created, + :activate_account, :perform_account_activation, :user_preferences_redirect, :avatar, + :my_redirect, :toggle_anon, :admin_login, :confirm_admin + ] before_action :respond_to_suspicious_request, only: [:create] diff --git a/app/controllers/users_email_controller.rb b/app/controllers/users_email_controller.rb index 6e1e2a1f0c7..e34e905232c 100644 --- a/app/controllers/users_email_controller.rb +++ b/app/controllers/users_email_controller.rb @@ -4,7 +4,7 @@ require_dependency 'email_updater' class UsersEmailController < ApplicationController - before_action :ensure_logged_in, only: [:index, :update] + prepend_before_action :check_xhr, :ensure_logged_in, only: [:index, :update] skip_before_action :check_xhr, only: [:confirm] skip_before_action :redirect_to_login_if_required, only: [:confirm] diff --git a/app/controllers/wizard_controller.rb b/app/controllers/wizard_controller.rb index 3c52ee68f31..78f224b17f2 100644 --- a/app/controllers/wizard_controller.rb +++ b/app/controllers/wizard_controller.rb @@ -2,10 +2,9 @@ require_dependency 'wizard' require_dependency 'wizard/builder' class WizardController < ApplicationController + prepend_before_action :check_xhr, :ensure_admin, except: [:qunit] + prepend_before_action :check_xhr, :ensure_logged_in, except: [:qunit] before_action :ensure_wizard_enabled, only: [:index] - before_action :ensure_logged_in, except: [:qunit] - before_action :ensure_admin, except: [:qunit] - skip_before_action :check_xhr, :preload_json layout false diff --git a/spec/controllers/wizard_controller_spec.rb b/spec/controllers/wizard_controller_spec.rb index c1476d12588..0875e31c264 100644 --- a/spec/controllers/wizard_controller_spec.rb +++ b/spec/controllers/wizard_controller_spec.rb @@ -14,6 +14,13 @@ describe WizardController do expect(response.status).to eq(403) end + it 'needs you to be logged in' do + get :index + # for whatever reason, no access is 404 + # we may want to revisit this at some point and make it 403 + expect(response.status).to eq(404) + end + it "raises an error if you aren't an admin" do log_in(:moderator) get :index, format: :json diff --git a/spec/requests/admin/admin_controller_spec.rb b/spec/requests/admin/admin_controller_spec.rb index 031bb7dd06c..c479ada153b 100644 --- a/spec/requests/admin/admin_controller_spec.rb +++ b/spec/requests/admin/admin_controller_spec.rb @@ -4,5 +4,8 @@ RSpec.describe Admin::AdminController do it "should return the right response if user isn't a staff" do get "/admin", params: { api_key: 'asdiasiduga' } expect(response.status).to eq(404) + + get "/admin" + expect(response.status).to eq(404) end end