diff --git a/decidim-core/app/controllers/decidim/links_controller.rb b/decidim-core/app/controllers/decidim/links_controller.rb index d2b63503a775..642e5c626464 100644 --- a/decidim-core/app/controllers/decidim/links_controller.rb +++ b/decidim-core/app/controllers/decidim/links_controller.rb @@ -21,24 +21,21 @@ def new def invalid_url flash[:alert] = I18n.t("decidim.links.invalid_url") - redirect_to decidim.root_path + if request.xhr? + render "invalid_url" + else + redirect_to decidim.root_path + end end def parse_url + raise Decidim::InvalidUrlError if params[:external_url].blank? raise Decidim::InvalidUrlError unless external_url - - parts = external_url.match %r{\A(([a-z]+):)?//([^/]+)(/.*)?\z} - raise Decidim::InvalidUrlError unless parts - - @url_parts = { - protocol: parts[1], - domain: parts[3], - path: parts[4] - } + raise Decidim::InvalidUrlError unless %w(http https).include?(external_url.scheme) end def external_url - @external_url ||= URI.parse(params[:external_url]).to_s + @external_url ||= URI.parse(params[:external_url]) end end end diff --git a/decidim-core/app/helpers/decidim/external_domain_helper.rb b/decidim-core/app/helpers/decidim/external_domain_helper.rb index 0f7a01eecadb..fd03d0ecce9e 100644 --- a/decidim-core/app/helpers/decidim/external_domain_helper.rb +++ b/decidim-core/app/helpers/decidim/external_domain_helper.rb @@ -3,10 +3,21 @@ module Decidim module ExternalDomainHelper def highlight_domain + highlighted_domain = [ + external_url.host, + (external_url.port && [80, 443].include?(external_url.port) ? "" : ":#{external_url.port}") + ].join + + path = [ + external_url.path, + (external_url.query ? "?#{external_url.query}" : ""), + (external_url.fragment ? "##{external_url.fragment}" : "") + ].join + tag.div do - content_tag(:span, "#{@url_parts[:protocol]}//") + - content_tag(:span, @url_parts[:domain], class: "alert") + - content_tag(:span, @url_parts[:path]) + content_tag(:span, "#{external_url.scheme}://") + + content_tag(:span, highlighted_domain, class: "text-alert") + + content_tag(:span, path) end end end diff --git a/decidim-core/app/views/decidim/links/_invalid_url_modal.html.erb b/decidim-core/app/views/decidim/links/_invalid_url_modal.html.erb new file mode 100644 index 000000000000..c0aff46f74c3 --- /dev/null +++ b/decidim-core/app/views/decidim/links/_invalid_url_modal.html.erb @@ -0,0 +1,17 @@ +<%= decidim_modal id: "external-domain-warning" do %> +
+ <%= icon "external-link-line" %> +

<%= t("decidim.links.warning.title") %>

+
+ + + <%= flash[:alert] %> + +
+
+
+ +
+<% end %> diff --git a/decidim-core/app/views/decidim/links/_modal.html.erb b/decidim-core/app/views/decidim/links/_modal.html.erb index bc2f849c6900..683f230f630f 100644 --- a/decidim-core/app/views/decidim/links/_modal.html.erb +++ b/decidim-core/app/views/decidim/links/_modal.html.erb @@ -13,7 +13,7 @@
- <%= link_to t("decidim.links.warning.proceed"), params[:external_url], target: "_blank", data: { close: "" }, class: "button primary button--nomargin" %> + <%= link_to t("decidim.links.warning.proceed"), external_url.to_s, target: "_blank", data: { close: "" }, class: "button primary button--nomargin" %> diff --git a/decidim-core/app/views/decidim/links/invalid_url.js.erb b/decidim-core/app/views/decidim/links/invalid_url.js.erb new file mode 100644 index 000000000000..5908b463a0d9 --- /dev/null +++ b/decidim-core/app/views/decidim/links/invalid_url.js.erb @@ -0,0 +1,24 @@ +(function() { + const create = (selector) => { + const element = document.createElement("div") + element.id = selector + document.body.append(element) + return element + } + + const selector = "external-domain-warning" + const selectorContainer = `${selector}-container` + + // if the container does not exist in the DOM, it creates a new one, otherwise, replace the content + const externalDomainWarning = document.getElementById(selectorContainer) || create(selectorContainer) + + externalDomainWarning.innerHTML = '' + externalDomainWarning.innerHTML = '<%= j(render partial: "invalid_url_modal").strip.html_safe %>' + + new window.Decidim.Dialogs(`#${selector}`, { + closingSelector: `[data-dialog-close="${selector}"]`, + backdropSelector: `[data-dialog="${selector}"]`, + labelledby: `dialog-title-${selector}`, + describedby: `dialog-desc-${selector}` + }).open() +})() diff --git a/decidim-core/app/views/decidim/links/new.html.erb b/decidim-core/app/views/decidim/links/new.html.erb index 43ea4af8a70e..7944fa0a9913 100644 --- a/decidim-core/app/views/decidim/links/new.html.erb +++ b/decidim-core/app/views/decidim/links/new.html.erb @@ -12,7 +12,7 @@
- <%= link_to t("decidim.links.warning.proceed"), params[:external_url], class: "button expanded primary" %> + <%= link_to t("decidim.links.warning.proceed"), external_url.to_s, class: "button expanded primary" %>
diff --git a/decidim-core/spec/helpers/decidim/external_domain_helper_spec.rb b/decidim-core/spec/helpers/decidim/external_domain_helper_spec.rb index 70845ace524f..7b4be78cbcdc 100644 --- a/decidim-core/spec/helpers/decidim/external_domain_helper_spec.rb +++ b/decidim-core/spec/helpers/decidim/external_domain_helper_spec.rb @@ -5,10 +5,14 @@ module Decidim describe ExternalDomainHelper do context "when everything is OK" do - before { helper.instance_variable_set(:@url_parts, { protocol: "https:", domain: "decidim.barcelona", path: "/processes" }) } + before do + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(ActionView::Base).to receive(:external_url).and_return(URI.parse("https://decidim.barcelona/processes")) + # rubocop:enable RSpec/AnyInstance + end it "highlights domain" do - expect(helper.highlight_domain).to include('class="alert">decidim.barcelona') + expect(helper.highlight_domain).to include('class="text-alert">decidim.barcelona') end end end diff --git a/decidim-core/spec/system/external_domain_warning_spec.rb b/decidim-core/spec/system/external_domain_warning_spec.rb index e49d0186e291..018d4469e340 100644 --- a/decidim-core/spec/system/external_domain_warning_spec.rb +++ b/decidim-core/spec/system/external_domain_warning_spec.rb @@ -37,11 +37,84 @@ end end - context "when the url is malformed" do - let(:invalid_url) do - "http://#{organization.host}/link?external_url=javascript:alert(document.location.host)//%0ahttps://www.example.org" + context "when url has missing protocols" do + let(:invalid_url) { "http://#{organization.host}/link?external_url=//example.org/some/path" } + + it "shows invalid url alert" do + visit invalid_url + expect(page).to have_content("Invalid URL") end - let!(:invalid_url2) do + end + + context "when url has a port" do + let(:destination) { "http://example.org:3000/some/path" } + let(:invalid_url) { "http://#{organization.host}/link?external_url=#{destination}" } + + it "shows invalid url alert" do + visit invalid_url + expect(page).not_to have_content("Invalid URL") + expect(page).to have_content(destination) + expect(page).to have_link("Proceed", href: destination) + end + end + + context "when url has invalid protocols" do + let(:invalid_url) { "http://#{organization.host}/link?external_url=javascript://example.org%250aalert(document.location.host)" } + + it "shows invalid url alert" do + visit invalid_url + expect(page).to have_content("Invalid URL") + end + end + + context "when url is double encoded" do + let(:invalid_url) { "http://#{organization.host}/link?external_url=http://example.org%250ajavascript:alert(document.location.host)" } + + it "shows invalid url alert" do + visit invalid_url + expect(page).to have_content("Invalid URL") + end + end + + context "when url is not HTTP" do + let(:invalid_url) { "http://#{organization.host}/link?external_url=ftp://example.org/some/path" } + + it "shows invalid url alert" do + visit invalid_url + expect(page).to have_content("Invalid URL") + end + end + + context "when url starts with HTTP but is not valid protocol" do + let(:invalid_url) { "http://#{organization.host}/link?external_url=httpfoobar://example.org/some/path" } + + it "shows invalid url alert" do + visit invalid_url + expect(page).to have_content("Invalid URL") + end + end + + context "when url starts with HTTPS but is not valid protocol" do + let(:invalid_url) { "http://#{organization.host}/link?external_url=httpsfoobar://example.org/some/path" } + + it "shows invalid url alert" do + visit invalid_url + expect(page).to have_content("Invalid URL") + end + end + + context "when the url is malformed using a simple scenario" do + let(:invalid_url) { "http://#{organization.host}/link?external_url=javascript:alert(document.location.host)//%0ahttps://www.example.org" } + + it "shows invalid url alert when using simple scenario" do + visit invalid_url + expect(page).to have_content("Invalid URL") + expect(page).to have_current_path(decidim.root_path, ignore_query: true) + end + end + + context "when the url is malformed using a complex scenario" do + let(:invalid_url) do %W( http://#{organization.host}/link?external_url=javascript:fetch%28%22%2Fprocesses%2Fconsequuntur%2Daperiam%2Ff%2F12%2F proposals%2F8%2Fproposal%5Fvote%22%2C%20%7B%22headers%22%3A%7B%22x%2Dcsrf%2Dtoken%22%3Adocument%2EquerySelectorAll%28 @@ -51,24 +124,18 @@ ).join end - it "shows invalid url alert when using simple scenario" do - visit invalid_url - expect(page).to have_content("Invalid URL") - expect(page).to have_current_path(decidim.root_path, ignore_query: true) - end - it "shows invalid url alert when using complex scenario" do - visit invalid_url2 + visit invalid_url expect(page).to have_content("Invalid URL") expect(page).to have_current_path(decidim.root_path, ignore_query: true) end end context "without param" do - let(:no_param) { "http://#{organization.host}/link" } + let(:invalid_url) { "http://#{organization.host}/link" } it "shows invalid url alert" do - visit no_param + visit invalid_url expect(page).to have_content("Invalid URL") expect(page).to have_current_path decidim.root_path end