SECURITY: make find topic by slug adhere to SiteSetting.detailed_404 (#9898)

SECURITY: make find topic by slug adhere to SiteSetting.detailed_404 (#9898)

diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index bca0bde..ddb750f 100644
--- a/app/controllers/topics_controller.rb
+++ b/app/controllers/topics_controller.rb
@@ -945,7 +945,15 @@ class TopicsController < ApplicationController
   end
 
   def redirect_to_correct_topic(topic, post_number = nil)
-    guardian.ensure_can_see!(topic)
+    begin
+      guardian.ensure_can_see!(topic)
+    rescue Discourse::InvalidAccess => ex
+      if !SiteSetting.detailed_404
+        raise Discourse::NotFound
+      else
+        raise ex
+      end
+    end
 
     url = topic.relative_url
     url << "/#{post_number}" if post_number.to_i > 0
diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb
index bc80fa9..c542f23 100644
--- a/spec/requests/topics_controller_spec.rb
+++ b/spec/requests/topics_controller_spec.rb
@@ -1,3 +1,4 @@
+# coding: utf-8
 # frozen_string_literal: true
 
 require 'rails_helper'
@@ -1357,17 +1358,6 @@ RSpec.describe TopicsController do
       expect(response).to redirect_to(topic.relative_url)
     end
 
-    it 'will return a 403 if you try to redirect to a topic you have no access to' do
-      category = Fabricate(:category)
-      category.set_permissions(Group::AUTO_GROUPS[:staff] => :full)
-      category.save!
-
-      topic.update!(category_id: category.id)
-      get "/t/#{topic.slug}"
-
-      expect(response.status).to eq(403)
-    end
-
     it 'can find a topic when a slug has a number in front' do
       another_topic = Fabricate(:post).topic
 
@@ -1463,6 +1453,12 @@ RSpec.describe TopicsController do
             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}"
+          expect(response.status).to eq(expected_slug_response)
+        end
       end
 
       context 'without detailed error pages' do

GitHub sha: a9d92f33

2 Likes

This commit appears in #9898 which was approved by davidtaylorhq. It was merged by featheredtoast.

Minor but this can be simplified to raise(SiteSetting.detailed_404 ? ex : Discourse::NotFound) or just

if SiteSetting.detailed_404
  raise ex
else 
  raise Discourse::NotFound
end

so we don’t have to parse !SiteSetting.detailed_404

2 Likes

Updated here:

1 Like