Improve the link handling

* Improve the link handling

* Improve further the link handling

* More iterations

* Another iteration

* Enforce http or https protocols

* Temporary commit

* Running linters

* Refactor the links controller

* Apply review recommendations

* Add handling for invalid url

* Further improve the controller

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

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

decidim-core/app/views/decidim/links/_invalid_url_modal.html.erb

@@ -0,0 +1,17 @@
+<%= decidim_modal id: "external-domain-warning" do %>
+  <div data-dialog-container>
+    <%= icon "external-link-line" %>
+    <h2 id="dialog-title-external-domain-warning" tabindex="-1" data-dialog-title><%= t("decidim.links.warning.title") %></h2>
+    <div>
+
+      <code class="mt-5 block break-all text-alert">
+        <%= flash[:alert] %>
+      </code>
+    </div>
+  </div>
+  <div data-dialog-actions>
+    <button class="button button__lg button__transparent-secondary" data-dialog-close="external-domain-warning">
+      <%= t("decidim.links.warning.cancel") %>
+    </button>
+  </div>
+<% end %>

decidim-core/app/views/decidim/links/_modal.html.erb

@@ -13,7 +13,7 @@
       </div>
     </div>
     <div class="row buttons">
-      <%= 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" %>
       <button class="button clear" data-close>
         <%= t("decidim.links.warning.cancel") %>
       </button>

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()
+})()

decidim-core/app/views/decidim/links/new.html.erb

@@ -12,7 +12,7 @@
           </div>
           <div class="row">
             <div class="columns large-12 text-center">
-              <%= 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" %>
             </div>
           </div>
         </div>

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

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