From ed4c0f256e2bc3edb7ea882ea799795ba570e368 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 9 Aug 2018 15:05:12 +1000 Subject: [PATCH] FIX: check permalinks for deleted topics - allow to specify 410 vs 404 in Discourse::NotFound exception - remove unused `permalink_redirect_or_not_found` which - handle JS side links to topics via Discourse-Xhr-Redirect mechanism --- .../javascripts/discourse/lib/ajax.js.es6 | 12 ++++ app/controllers/application_controller.rb | 55 ++++++++++++------- app/controllers/list_controller.rb | 4 +- app/controllers/tags_controller.rb | 2 +- app/controllers/topics_controller.rb | 8 ++- lib/discourse.rb | 13 ++++- spec/requests/application_controller_spec.rb | 20 +++++++ 7 files changed, 88 insertions(+), 26 deletions(-) diff --git a/app/assets/javascripts/discourse/lib/ajax.js.es6 b/app/assets/javascripts/discourse/lib/ajax.js.es6 index 098676437c2..dc81ce6cd1a 100644 --- a/app/assets/javascripts/discourse/lib/ajax.js.es6 +++ b/app/assets/javascripts/discourse/lib/ajax.js.es6 @@ -29,6 +29,17 @@ export function handleLogoff(xhr) { } } +function handleRedirect(data) { + if ( + data && + data.getResponseHeader && + data.getResponseHeader("Discourse-Xhr-Redirect") + ) { + window.location.replace(data.responseText); + window.location.reload(); + } +} + /** Our own $.ajax method. Makes sure the .then method executes in an Ember runloop for performance reasons. Also automatically adjusts the URL to support installs @@ -76,6 +87,7 @@ export function ajax() { } args.success = (data, textStatus, xhr) => { + handleRedirect(data); handleLogoff(xhr); Ember.run(() => { diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 87d372f6299..d5adf6ee1a0 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -173,7 +173,16 @@ class ApplicationController < ActionController::Base end end - rescue_from Discourse::NotFound, PluginDisabled, ActionController::RoutingError do + rescue_from Discourse::NotFound do |e| + rescue_discourse_actions( + :not_found, + e.status, + check_permalinks: e.check_permalinks, + original_path: e.original_path + ) + end + + rescue_from PluginDisabled, ActionController::RoutingError do rescue_discourse_actions(:not_found, 404) end @@ -194,12 +203,37 @@ class ApplicationController < ActionController::Base render_json_error I18n.t('read_only_mode_enabled'), type: :read_only, status: 503 end + def redirect_with_client_support(url, options) + if request.xhr? + response.headers['Discourse-Xhr-Redirect'] = 'true' + render plain: url + else + redirect_to url, options + end + end + def rescue_discourse_actions(type, status_code, opts = nil) opts ||= {} show_json_errors = (request.format && request.format.json?) || (request.xhr?) || ((params[:external_id] || '').ends_with? '.json') + if type == :not_found && opts[:check_permalinks] + url = opts[:original_path] || request.fullpath + permalink = Permalink.find_by_url(url) + + if permalink.present? + # permalink present, redirect to that URL + if permalink.external_url + redirect_with_client_support permalink.external_url, status: :moved_permanently + return + elsif permalink.target_url + redirect_with_client_support "#{Discourse::base_uri}#{permalink.target_url}", status: :moved_permanently + return + end + end + end + message = opts[:custom_message_translated] || I18n.t(opts[:custom_message] || type) if show_json_errors @@ -444,25 +478,6 @@ class ApplicationController < ActionController::Base request.session_options[:skip] = true end - def permalink_redirect_or_not_found - url = request.fullpath - permalink = Permalink.find_by_url(url) - - if permalink.present? - # permalink present, redirect to that URL - if permalink.external_url - redirect_to permalink.external_url, status: :moved_permanently - elsif permalink.target_url - redirect_to "#{Discourse::base_uri}#{permalink.target_url}", status: :moved_permanently - else - raise Discourse::NotFound - end - else - # redirect to 404 - raise Discourse::NotFound - end - end - def secure_session SecureSession.new(session["secure_session_id"] ||= SecureRandom.hex) end diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index 6deaf817e7d..a2e94b26176 100644 --- a/app/controllers/list_controller.rb +++ b/app/controllers/list_controller.rb @@ -344,7 +344,7 @@ class ListController < ApplicationController parent_category_id = nil if parent_slug_or_id.present? parent_category_id = Category.query_parent_category(parent_slug_or_id) - permalink_redirect_or_not_found && (return) if parent_category_id.blank? && !id + raise Discourse::NotFound.new("category not found", check_permalinks: true) if parent_category_id.blank? && !id end @category = Category.query_category(slug_or_id, parent_category_id) @@ -355,7 +355,7 @@ class ListController < ApplicationController (redirect_to category.url, status: 301) && return if category end - permalink_redirect_or_not_found && (return) if !@category + raise Discourse::NotFound.new("category not found", check_permalinks: true) if !@category @description_meta = @category.description_text raise Discourse::NotFound unless guardian.can_see?(@category) diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index b2916e8c9db..fccddd0e4ab 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -90,7 +90,7 @@ class TagsController < ::ApplicationController canonical_url "#{Discourse.base_url_no_prefix}#{public_send(path_name, *(params.slice(:parent_category, :category, :tag_id).values.map { |t| t.force_encoding("UTF-8") }))}" if @list.topics.size == 0 && params[:tag_id] != 'none' && !Tag.where(name: @tag_id).exists? - permalink_redirect_or_not_found + raise Discourse::NotFound.new("tag not found", check_permalinks: true) else respond_with_list(@list) end diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 44b362ccbb5..a4ffc8aaf28 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -135,8 +135,12 @@ class TopicsController < ApplicationController end if ex.obj && Topic === ex.obj && guardian.can_see_topic_if_not_deleted?(ex.obj) - rescue_discourse_actions(:not_found, 410) - return + raise Discourse::NotFound.new( + "topic was deleted", + status: 410, + check_permalinks: true, + original_path: ex.obj.relative_url + ) end raise ex diff --git a/lib/discourse.rb b/lib/discourse.rb index 4ebb87eb722..50793e334c7 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -77,7 +77,18 @@ module Discourse end # When something they want is not found - class NotFound < StandardError; end + class NotFound < StandardError + attr_reader :status + attr_reader :check_permalinks + attr_reader :original_path + + def initialize(message = nil, status: 404, check_permalinks: false, original_path: nil) + @status = status + @check_permalinks = check_permalinks + @original_path = original_path + super(message) + end + end # When a setting is missing class SiteSettingMissing < StandardError; end diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index f45f1c4fa5d..5ad3aa60993 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -17,6 +17,26 @@ RSpec.describe ApplicationController do describe 'build_not_found_page' do describe 'topic not found' do + + it 'should return permalink for deleted topics' do + topic = create_post.topic + external_url = 'https://somewhere.over.rainbow' + Permalink.create!(url: topic.relative_url, external_url: external_url) + topic.trash! + + get topic.relative_url + expect(response.status).to eq(301) + expect(response).to redirect_to(external_url) + + get "/t/#{topic.id}.json" + expect(response.status).to eq(301) + expect(response).to redirect_to(external_url) + + get "/t/#{topic.id}.json", xhr: true + expect(response.status).to eq(200) + expect(response.body).to eq(external_url) + end + it 'should return 404 and show Google search' do get "/t/nope-nope/99999999" expect(response.status).to eq(404)