From cfb6199a9511773c7eeffe16e42ad62cc45138ce Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 2 Dec 2021 15:12:25 +0000 Subject: [PATCH] FIX: Don't redirect XHR/JSON requests when login is required (#15093) When redirecting to login, we store a destination_url cookie, which the user is then redirected to after login. We never want the user to be redirected to a JSON URL. Instead, we should return a 403 in these situations. This should also be much less confusing for API consumers - a 403 is a better representation than a 302. --- app/controllers/application_controller.rb | 6 ++- spec/integration/api_keys_spec.rb | 4 +- spec/requests/application_controller_spec.rb | 8 ++++ spec/requests/topics_controller_spec.rb | 39 ++++++++++++++++---- 4 files changed, 46 insertions(+), 11 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7dd73b3f82c..dbe83dc2ef4 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -819,7 +819,11 @@ class ApplicationController < ActionController::Base if !current_user && SiteSetting.login_required? flash.keep - redirect_to_login + if (request.format && request.format.json?) || request.xhr? || !request.get? + ensure_logged_in + else + redirect_to_login + end return end diff --git a/spec/integration/api_keys_spec.rb b/spec/integration/api_keys_spec.rb index 0e8d7249b05..60598cfbcfe 100644 --- a/spec/integration/api_keys_spec.rb +++ b/spec/integration/api_keys_spec.rb @@ -45,7 +45,7 @@ describe 'api keys' do # Confirm not allowed for json get "/latest.json?api_key=#{api_key.key}&api_username=#{user.username.downcase}" - expect(response.status).to eq(302) + expect(response.status).to eq(403) end context "with a plugin registered filter" do @@ -96,7 +96,7 @@ describe 'user api keys' do # Confirm not allowed for json get "/latest.json?user_api_key=#{user_api_key.key}" - expect(response.status).to eq(302) + expect(response.status).to eq(403) end it "can restrict scopes by parameters" do diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index 112e637271d..4ffee8d2e32 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -120,6 +120,14 @@ RSpec.describe ApplicationController do expect(response.body).not_to include("data-authentication-data=") expect(response.headers["Set-Cookie"]).to include("authentication_data=;") # Delete cookie end + + it "returns a 403 for json requests" do + get '/latest' + expect(response.status).to eq(302) + + get '/latest.json' + expect(response.status).to eq(403) + end end describe '#redirect_to_second_factor_if_required' do diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 7a76aa043ac..875fc507338 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -1928,19 +1928,21 @@ RSpec.describe TopicsController do let!(:nonexistent_topic_id) { Topic.last.id + 10000 } fab!(:secure_accessible_topic) { Fabricate(:topic, category: accessible_category) } - shared_examples "various scenarios" do |expected| + shared_examples "various scenarios" do |expected, request_json: false| expected.each do |key, value| it "returns #{value} for #{key}" do slug = key == :nonexistent ? "garbage-slug" : send(key.to_s).slug topic_id = key == :nonexistent ? nonexistent_topic_id : send(key.to_s).id - get "/t/#{slug}/#{topic_id}.json" + format = request_json ? ".json" : "" + get "/t/#{slug}/#{topic_id}#{format}" expect(response.status).to eq(value) end end expected_slug_response = expected[:secure_topic] == 200 ? 301 : expected[:secure_topic] it "will return a #{expected_slug_response} when requesting a secure topic by slug" do - get "/t/#{secure_topic.slug}" + format = request_json ? ".json" : "" + get "/t/#{secure_topic.slug}#{format}" expect(response.status).to eq(expected_slug_response) end end @@ -1981,6 +1983,23 @@ RSpec.describe TopicsController do include_examples "various scenarios", expected end + context 'anonymous with login required, requesting json' do + before do + SiteSetting.login_required = true + end + expected = { + normal_topic: 403, + secure_topic: 403, + private_topic: 403, + deleted_topic: 403, + deleted_secure_topic: 403, + deleted_private_topic: 403, + nonexistent: 403, + secure_accessible_topic: 403 + } + include_examples "various scenarios", expected, request_json: true + end + context 'normal user' do before do sign_in(user) @@ -2070,7 +2089,7 @@ RSpec.describe TopicsController do nonexistent: 404, secure_accessible_topic: 403 } - include_examples "various scenarios", expected + include_examples "various scenarios", expected, request_json: true end context 'anonymous with login required' do @@ -2105,7 +2124,7 @@ RSpec.describe TopicsController do nonexistent: 404, secure_accessible_topic: 403 } - include_examples "various scenarios", expected + include_examples "various scenarios", expected, request_json: true end context 'allowed user' do @@ -2395,12 +2414,16 @@ RSpec.describe TopicsController do context 'and the user is not logged in' do let(:api_key) { Fabricate(:api_key, user: topic.user) } - it 'redirects to the login page' do - get "/t/#{topic.slug}/#{topic.id}.json" - + it 'redirects browsers to the login page' do + get "/t/#{topic.slug}/#{topic.id}" expect(response).to redirect_to login_path end + it 'raises a 403 for json requests' do + get "/t/#{topic.slug}/#{topic.id}.json" + expect(response.status).to eq(403) + end + it 'shows the topic if valid api key is provided' do get "/t/#{topic.slug}/#{topic.id}.json", headers: { "HTTP_API_KEY" => api_key.key }