From 1859025228313b981c5a14210460d7f125b0344c Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Tue, 21 Mar 2023 15:58:42 +1100 Subject: [PATCH] FIX: my links in sidebar section (#20754) Links like `/my/preferences` were invalid in custom section. The reason is that `/my` links are just redirects from backend, and they are not recognized as valid Ember paths. https://github.com/discourse/discourse/blob/main/config/routes.rb#L433 Therefore, regex match allowlist was added - similar to backend check: https://github.com/discourse/discourse/blob/main/app/controllers/users_controller.rb#L471 /safe-mode is same case --- .../app/controllers/sidebar-section-form.js | 8 ++++++-- app/models/sidebar_url.rb | 8 +++++++- app/serializers/sidebar_section_serializer.rb | 2 +- app/serializers/sidebar_url_serializer.rb | 9 +++++++++ .../sidebar_sections_controller_spec.rb | 7 ++++++- spec/system/custom_sidebar_sections_spec.rb | 19 +++++++++++++++++++ 6 files changed, 48 insertions(+), 5 deletions(-) create mode 100644 app/serializers/sidebar_url_serializer.rb diff --git a/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js b/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js index 59698abc4aa..d23ff569cb2 100644 --- a/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js +++ b/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js @@ -9,6 +9,8 @@ import { sanitize } from "discourse/lib/text"; import { tracked } from "@glimmer/tracking"; import { A } from "@ember/array"; +const FULL_RELOAD_LINKS_REGEX = [/^\/my\/[a-z_\-\/]+$/, /^\/safe-mode$/]; + class Section { @tracked title; @tracked links; @@ -95,7 +97,10 @@ class SectionLink { } #validInternal() { - return this.router.recognize(this.path).name !== "unknown"; + return ( + this.router.recognize(this.path).name !== "unknown" || + FULL_RELOAD_LINKS_REGEX.some((regex) => this.path.match(regex)) + ); } get validValue() { @@ -200,7 +205,6 @@ export default Controller.extend(ModalFunctionality, { icon: link.icon, name: link.name, value: link.path, - external: link.external, _destroy: link._destroy, }; }), diff --git a/app/models/sidebar_url.rb b/app/models/sidebar_url.rb index 670ac656227..73731ec605a 100644 --- a/app/models/sidebar_url.rb +++ b/app/models/sidebar_url.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class SidebarUrl < ActiveRecord::Base + FULL_RELOAD_LINKS_REGEX = [%r{\A/my/[a-z_\-/]+\z}, %r{\A/safe-mode\z}] + validates :icon, presence: true, length: { maximum: 40 } validates :name, presence: true, length: { maximum: 80 } validates :value, presence: true, length: { maximum: 200 } @@ -11,7 +13,7 @@ class SidebarUrl < ActiveRecord::Base def path_validator if external? - raise ActionController::RoutingError if value !~ Discourse::Utils::URI_REGEXP + raise ActionController::RoutingError.new("Not Found") if value !~ Discourse::Utils::URI_REGEXP else Rails.application.routes.recognize_path(value) end @@ -29,6 +31,10 @@ class SidebarUrl < ActiveRecord::Base def set_external self.external = value.start_with?("http://", "https://") end + + def full_reload? + FULL_RELOAD_LINKS_REGEX.any? { |regex| value =~ regex } + end end # == Schema Information diff --git a/app/serializers/sidebar_section_serializer.rb b/app/serializers/sidebar_section_serializer.rb index 27b16251ba2..77df4a13ac7 100644 --- a/app/serializers/sidebar_section_serializer.rb +++ b/app/serializers/sidebar_section_serializer.rb @@ -4,7 +4,7 @@ class SidebarSectionSerializer < ApplicationSerializer attributes :id, :title, :links, :slug, :public def links - object.sidebar_section_links.map(&:linkable) + object.sidebar_section_links.map { |link| SidebarUrlSerializer.new(link.linkable, root: false) } end def slug diff --git a/app/serializers/sidebar_url_serializer.rb b/app/serializers/sidebar_url_serializer.rb new file mode 100644 index 00000000000..a7df7de798f --- /dev/null +++ b/app/serializers/sidebar_url_serializer.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class SidebarUrlSerializer < ApplicationSerializer + attributes :id, :name, :value, :icon, :external + + def external + object.external? || object.full_reload? + end +end diff --git a/spec/requests/sidebar_sections_controller_spec.rb b/spec/requests/sidebar_sections_controller_spec.rb index 3ea42a95d42..bbba5431b12 100644 --- a/spec/requests/sidebar_sections_controller_spec.rb +++ b/spec/requests/sidebar_sections_controller_spec.rb @@ -61,6 +61,7 @@ RSpec.describe SidebarSectionsController do }, { icon: "address-book", name: "tags", value: "/tags" }, { icon: "external-link-alt", name: "Discourse", value: "https://discourse.org" }, + { icon: "external-link-alt", name: "My preferences", value: "/my/preferences" }, ], } @@ -73,7 +74,7 @@ RSpec.describe SidebarSectionsController do expect(sidebar_section.user).to eq(user) expect(sidebar_section.public).to be false expect(UserHistory.count).to eq(0) - expect(sidebar_section.sidebar_urls.count).to eq(3) + expect(sidebar_section.sidebar_urls.count).to eq(4) expect(sidebar_section.sidebar_urls.first.icon).to eq("link") expect(sidebar_section.sidebar_urls.first.name).to eq("categories") expect(sidebar_section.sidebar_urls.first.value).to eq("/categories") @@ -86,6 +87,10 @@ RSpec.describe SidebarSectionsController do expect(sidebar_section.sidebar_urls.third.name).to eq("Discourse") expect(sidebar_section.sidebar_urls.third.value).to eq("https://discourse.org") expect(sidebar_section.sidebar_urls.third.external).to be true + expect(sidebar_section.sidebar_urls.fourth.icon).to eq("external-link-alt") + expect(sidebar_section.sidebar_urls.fourth.name).to eq("My preferences") + expect(sidebar_section.sidebar_urls.fourth.value).to eq("/my/preferences") + expect(sidebar_section.sidebar_urls.fourth.external).to be false end it "does not allow regular user to create public section" do diff --git a/spec/system/custom_sidebar_sections_spec.rb b/spec/system/custom_sidebar_sections_spec.rb index f0e944aafb6..75c6108d8ab 100644 --- a/spec/system/custom_sidebar_sections_spec.rb +++ b/spec/system/custom_sidebar_sections_spec.rb @@ -34,6 +34,25 @@ describe "Custom sidebar sections", type: :system, js: true do expect(sidebar).to have_link("Sidebar Tags") end + it "allows the user to create custom section with /my link" do + visit("/latest") + sidebar.open_new_custom_section + + expect(section_modal).to be_visible + expect(section_modal).to have_disabled_save + expect(sidebar.custom_section_modal_title).to have_content("Add custom section") + + section_modal.fill_name("My section") + + section_modal.fill_link("My preferences", "/my/preferences") + expect(section_modal).to have_enabled_save + + section_modal.save + + expect(page).to have_button("My section") + expect(sidebar).to have_link("My preferences") + end + it "allows the user to create custom section with external link" do visit("/latest") sidebar.open_new_custom_section