Primerize static link helper and use that for external links consistently

app/components/enterprise_edition/upsell_buttons_component.rb

@@ -103,21 +103,14 @@ module EnterpriseEdition
     def more_info_button
       return if @feature_key == :teaser
 
-      render(Primer::Beta::Link.new(href: enterprise_link, target: "_blank")) do |link|
-        link.with_trailing_visual_icon(icon: "link-external")
-        link_title
-      end
+      fallback_link = OpenProject::Static::Links.url_for(:enterprise_features, :default)
+      helpers.static_link_to(:enterprise_features, feature_key,
+                             href: fallback_link,
+                             label: link_title)
     end
 
     def link_title
       I18n.t("ee.upsell.#{feature_key}.link_title", default: I18n.t(:label_more_information))
     end
-
-    def enterprise_link
-      href_value = OpenProject::Static::Links.url_for(:enterprise_features, feature_key)
-      default_value = OpenProject::Static::Links.url_for(:enterprise_features, :default)
-
-      href_value || default_value
-    end
   end
 end

app/components/homescreen/blocks/community.html.erb

@@ -12,61 +12,31 @@
       <%= static_link_to(EnterpriseToken.active? ? :enterprise_support : :enterprise_support_as_community) %>
     </li>
     <li>
-      <%= link_to(
-            t("label_openproject_website"),
-            OpenProject::Static::Links.url_for(
-              :website,
-              url_params: {
-                utm_source: "unknown",
-                utm_medium: "op-instance",
-                utm_campaign: "website-home-screen"
-              }
-            ),
-            {
-              aria: { label: t("label_openproject_website") },
-              target: "_blank",
-              title: t("label_openproject_website"),
-              rel: "noopener"
-            }
-          ) %>
+      <%= static_link_to :website,
+                         label: t("label_openproject_website"),
+                         url_params: {
+                           utm_source: "unknown",
+                           utm_medium: "op-instance",
+                           utm_campaign: "website-home-screen"
+                         } %>
     </li>
     <li>
-      <%= link_to(
-            t("homescreen.links.security_alerts"),
-            OpenProject::Static::Links.url_for(
-              :security_alerts,
-              url_params: {
-                utm_source: "unknown",
-                utm_medium: "op-instance",
-                utm_campaign: "security-alerts-home-screen"
-              }
-            ),
-            {
-              aria: { label: t("homescreen.links.security_alerts") },
-              target: "_blank",
-              title: t("homescreen.links.security_alerts"),
-              rel: "noopener"
-            }
-          ) %>
+      <%= static_link_to :security_alerts,
+                         label: t("homescreen.links.security_alerts"),
+                         url_params: {
+                           utm_source: "unknown",
+                           utm_medium: "op-instance",
+                           utm_campaign: "security-alerts-home-screen"
+                         } %>
     </li>
     <li>
-      <%= link_to(
-            t("homescreen.links.newsletter"),
-            OpenProject::Static::Links.url_for(
-              :newsletter,
-              url_params: {
-                utm_source: "unknown",
-                utm_medium: "op-instance",
-                utm_campaign: "newsletter-home-screen"
-              }
-            ),
-            {
-              aria: { label: t("homescreen.links.newsletter") },
-              target: "_blank",
-              title: t("homescreen.links.newsletter"),
-              rel: "noopener"
-            }
-          ) %>
+      <%= static_link_to :newsletter,
+                         label: t("homescreen.links.newsletter"),
+                         url_params: {
+                           utm_source: "unknown",
+                           utm_medium: "op-instance",
+                           utm_campaign: "newsletter-home-screen"
+                         } %>
     </li>
     <li>
       <%= static_link_to :blog %>

app/components/homescreen/link_component.html.erb

@@ -30,6 +30,7 @@ See COPYRIGHT and LICENSE files for more details.
 <%= link_to url,
             class: "homescreen--links--item",
             target: "_blank",
+            data: { allow_external_link: true },
             aria_label: title,
             rel: "noopener" do %>
   <%= render(Primer::Beta::Octicon.new(icon: link[:icon], size: :medium)) %>

app/components/work_packages/activities_tab/journals/new_component.html.erb

@@ -78,10 +78,8 @@
                     data: { "#{internal_comment_stimulus_controller}-target": "learnMoreLink" },
                     pb: 1
                   ) do
-                    render(Primer::Beta::Link.new(href: learn_more_static_link_url, target: "_blank")) do |link|
-                      link.with_trailing_visual_icon(icon: "link-external", size: :small)
-                      I18n.t("label_learn_more")
-                    end
+                    helpers.static_link_to(:enterprise_features, :internal_comments,
+                                           label: I18n.t(:label_learn_more))
                   end
                 end
               end

app/controllers/external_link_warning_controller.rb

@@ -34,7 +34,7 @@ class ExternalLinkWarningController < ApplicationController
   skip_before_action :check_if_login_required
   no_authorization_required! :show
 
-  before_action :parse_redirect_url, only: [:show]
+  before_action :parse_external_url, only: [:show]
   before_action :verify_capture_enabled, only: [:show]
 
   def show; end
@@ -43,7 +43,7 @@ class ExternalLinkWarningController < ApplicationController
 
   def verify_capture_enabled
     unless capture_enabled?
-      redirect_to @redirect_url, allow_other_host: true, status: :see_other
+      redirect_to @external_url, allow_other_host: true, status: :see_other
     end
   end
 
@@ -51,21 +51,23 @@ class ExternalLinkWarningController < ApplicationController
     Setting.capture_external_links? && EnterpriseToken.allows_to?(:capture_external_links)
   end
 
-  def parse_redirect_url
+  def parse_external_url
     external_url = params[:url]
-    @redirect_url = parse_url(CGI.unescape(external_url)) if external_url.present?
+    @external_url = parse_url(CGI.unescape(external_url)) if external_url.present?
 
-    if @redirect_url.nil?
+    if @external_url.nil?
       redirect_to home_path, status: :see_other
     end
   end
 
   def parse_url(url)
-    return false if url.blank?
+    return nil if url.blank?
 
     uri = URI.parse(url)
-    uri.is_a?(URI::HTTP) || uri.is_a?(URI::HTTPS)
+    return url if uri.is_a?(URI::HTTP) || uri.is_a?(URI::HTTPS)
+
+    nil
   rescue URI::InvalidURIError
-    false
+    nil
   end
 end

app/helpers/static_links_helper.rb

@@ -31,16 +31,25 @@
 module StaticLinksHelper
   ##
   # Create a static link to the given key entry
-  def static_link_to(*path, label: nil)
-    href = OpenProject::Static::Links.url_for(*path)
+  # *path - the path segments to look up the link for static links
+  # href: - optional override for the href if no static link is found or given
+  # label: - optional override for the label if no static link label is found or given
+  def static_link_to(*path, href: nil, label: nil, url_params: {}, **system_arguments)
+    link = OpenProject::Static::Links.url_for(*path, url_params:) || href
+    raise ArgumentError, "No href found for static link #{path.inspect}" if link.nil?
+
     label_text = label || OpenProject::Static::Links.label_for(*path)
 
-    link_to label_text,
-            href,
-            class: "openproject--static-link",
-            data: { allow_external_link: true },
-            target: "_blank",
-            rel: "noopener"
+    render(
+      Primer::Beta::Link.new(href: link,
+                             data: { allow_external_link: true },
+                             **system_arguments,
+                             rel: "noopener",
+                             target: "_blank")
+    ) do |link|
+      link.with_trailing_visual_icon(icon: "link-external")
+      label_text
+    end
   end
 
   ##

app/views/external_link_warning/show.html.erb

@@ -37,7 +37,10 @@ See COPYRIGHT and LICENSE files for more details.
        tag: :a,
        href: @external_url,
        scheme: :danger,
-       data: { turbo: false }
+       data: {
+         turbo: false,
+         allow_external_link: true
+       }
      ) { I18n.t("external_link_warning.continue_button") } %>
 
   <%= render(Primer::Box.new(color: :muted)) do %>

lib/redmine/menu_manager/menu_helper.rb

@@ -212,7 +212,10 @@ module Redmine::MenuManager::MenuHelper
     html_options[:title] ||= selected ? t(:description_current_position) + caption : caption
     html_options[:class] = "#{html_options[:class]} #{menu_class}--item-action"
     html_options["data-test-selector"] = "#{menu_class}--item-action"
-    html_options["target"] = "_blank" if item.icon_after.present? && item.icon_after == "link-external"
+    if item.icon_after.present? && item.icon_after == "link-external"
+      html_options["target"] = "_blank"
+      html_options["data-allow-external-link"] = "true"
+    end
 
     link_to link_text, url, html_options
   end

lib/redmine/menu_manager/top_menu/help_menu.rb

@@ -45,6 +45,7 @@ module Redmine::MenuManager::TopMenu::HelpMenu
                                             pl: 1,
                                             test_selector: "header-help-button",
                                             aria: { label: I18n.t(:label_help) },
+                                            data: { allow_external_link: true },
                                             **item.html_options))
       else
         render_help_dropdown
@@ -112,7 +113,8 @@ module Redmine::MenuManager::TopMenu::HelpMenu
                            label: t(:label_videos),
                            content_arguments: {
                              target: "_blank",
-                             rel: "noopener"
+                             rel: "noopener",
+                             data: { allow_external_link: true }
                            },
                            test_selector: "op-menu--item-action")
       menu_group.with_item(**link_options_for(:shortcuts))
@@ -175,7 +177,8 @@ module Redmine::MenuManager::TopMenu::HelpMenu
       label: label,
       content_arguments: {
         target: "_blank",
-        rel: "noopener"
+        rel: "noopener",
+        data: { allow_external_link: true }
       }
     }
   end

lib_static/redmine/i18n.rb

@@ -115,8 +115,7 @@ module Redmine
     # @param links [Hash] Link names mapped to URLs.
     # @param external [Boolean] Whether the links should be opened as external links, i.e. in a new tab (default: true)
     # @param underline [Boolean] Whether to underline links inserted into the text (default: true)
-    def link_translate(i18n_key, links: {}, external: true, underline: true)
-      # rubocop:disable Metrics/AbcSize
+    def link_translate(i18n_key, links: {}, external: true, underline: true) # rubocop:disable Metrics/AbcSize
       translation = ::I18n.t(i18n_key.to_s)
       result = translation.scan(link_regex).inject(translation) do |t, matches|
         link, text, key = matches

modules/documents/app/components/documents/admin/collaboration_settings/page_header_component.rb

@@ -38,16 +38,8 @@ module Documents
         end
 
         def hocuspocus_server_link
-          render(
-            Primer::Beta::Link.new(
-              href: OpenProject::Static::Links.url_for(:hocuspocus_server_docs),
-              target: "_blank",
-              underline: true
-            )
-          ) do |link|
-            link.with_trailing_visual_icon(icon: :"link-external")
-            I18n.t("documents.admin.collaboration_settings.hocuspocus_server")
-          end
+          helpers.static_link_to(:hocuspocus_server_docs,
+                                 label: I18n.t("documents.admin.collaboration_settings.hocuspocus_server"))
         end
       end
     end

modules/documents/app/components/documents/admin/document_categories/deprecation_notice_component.html.erb

@@ -43,6 +43,7 @@
 
       component.with_secondary_action(
         label: I18n.t("documents.document_categories_deprecation_notice.secondary_action"),
-        href: OpenProject::Static::Links.url_for(:documents_docs)
+        href: OpenProject::Static::Links.url_for(:documents_docs),
+        data: { allow_external_link: true }
       ).with_content(I18n.t("documents.document_categories_deprecation_notice.secondary_action"))
     end %>

modules/storages/app/components/storages/admin/general_info_component.html.erb

@@ -11,10 +11,9 @@
         if can_show_open_link?
           concat(render(Primer::Beta::Text.new) { " - " })
           concat(
-            render(Primer::Beta::Link.new(href: open_href, target: "_blank", underline: true)) do |link|
-              link.with_trailing_visual_icon(icon: "link-external", size: :small)
-              I18n.t("storages.buttons.open_storage")
-            end
+            helpers.static_link_to(href: open_href,
+                                   label: I18n.t("storages.buttons.open_storage"),
+                                   underline: true)
           )
         end
       end

modules/storages/app/components/storages/admin/health/check_result_component.html.erb

@@ -47,10 +47,9 @@ See COPYRIGHT and LICENSE files for more details.
 
         if data[:error_code].present?
           row.with_column do
-            render(Primer::Beta::Link.new(href: data[:docs_href], underline: true)) do |link|
-              link.with_trailing_visual_icon(icon: :"link-external")
-              I18n.t(:label_more_information)
-            end
+            helpers.static_link_to(href: data[:docs_href],
+                                   label: I18n.t(:label_more_information),
+                                   underline: true)
           end
         end
       end

modules/storages/app/components/storages/admin/oauth_application_info_copy_component.rb

@@ -44,6 +44,7 @@ module Storages::Admin
       render(
         Primer::Beta::Link.new(
           href: ::Storages::UrlBuilder.url(storage.uri, "settings/admin/openproject"),
+          data: { allow_external_link: true },
           target: "_blank"
         )
       ) { I18n.t("storages.instructions.oauth_application_details_link_text") }

spec/components/enterprise_edition/banner_component_spec.rb

@@ -81,7 +81,7 @@ RSpec.describe EnterpriseEdition::BannerComponent, type: :component do
 
     allow(OpenProject::Static::Links)
       .to receive(:url_for)
-      .with(:enterprise_features, :some_enterprise_feature)
+      .with(:enterprise_features, :some_enterprise_feature, any_args)
       .and_return(enterprise_feature_link)
 
     allow(OpenProject::Token)

spec/controllers/external_link_warning_controller_spec.rb

@@ -34,7 +34,28 @@ RSpec.describe ExternalLinkWarningController do
   render_views
 
   describe "GET #show" do
-    context "with a valid external URL" do
+    context "when capture is disabled" do
+      before do
+        allow(Setting).to receive(:capture_external_links?).and_return(false)
+      end
+
+      it "redirects directly to the external URL" do
+        get :show, params: { url: "https://example.com" }
+
+        expect(response).to redirect_to("https://example.com")
+      end
+
+      it "unescapes the URL parameter before redirecting" do
+        encoded_url = CGI.escape("https://example.com/path?param=value")
+        get :show, params: { url: encoded_url }
+
+        expect(response).to redirect_to("https://example.com/path?param=value")
+      end
+    end
+
+    context "when capture is enabled",
+            with_ee: %i[capture_external_links],
+            with_settings: { capture_external_links: true } do
       it "renders the warning page" do
         get :show, params: { url: "https://example.com" }
 

spec/features/a11y/external_links_spec.rb

@@ -54,10 +54,8 @@ RSpec.describe "External links", :js do
       document.body.appendChild(link);
     JS
 
-    # Wait for mutation observer to detect and update the link
-    expect(page).to have_link("External Example", href: "https://example.com", target: "_blank")
-
-    link = find_link("External Example", href: "https://example.com", match: :first)
+    href = external_redirect_path(url: "https://example.com/")
+    link = page.find_link("External Example", href:, match: :first)
 
     # Verify accessibility and security attributes
     expect(link[:target]).to eq("_blank")

spec/features/external_link_capture_spec.rb

@@ -34,7 +34,7 @@ RSpec.describe "External link capture", :js, :selenium do
   shared_let(:admin) { create(:admin) }
 
   let(:project) { create(:project, enabled_module_names: %w[wiki]) }
-  let(:external_url) { "http://0.0.0.0:3001/" }
+  let(:external_url) { "https://www.openprojet.org/" }
   let!(:wiki_page) do
     create(:wiki_page,
            wiki: project.wiki,
@@ -51,7 +51,8 @@ RSpec.describe "External link capture", :js, :selenium do
     it "keeps the default external link behaviour" do
       visit project_wiki_path(project, wiki_page)
 
-      link = page.find(%(a[href^="#{external_url}"]))
+      href = external_redirect_path(url: external_url)
+      link = page.find_link("OpenProject", href:)
       new_window = window_opened_by { link.click }
 
       within_window new_window do

spec/features/projects/project_custom_fields/overview_page/dialog/inputs_spec.rb

@@ -203,7 +203,8 @@ RSpec.describe "Edit project custom fields on project overview page", :js do
 
         it "renders the custom field as a link" do
           page.within_test_selector "project-custom-field-#{link_project_custom_field.id}" do
-            expect(page).to have_link("https://www.openproject.org", href: "https://www.openproject.org")
+            href = external_redirect_path(url: "https://www.openproject.org/")
+            expect(page).to have_link("https://www.openproject.org", href:)
           end
         end
       end

spec/features/wiki/wiki_page_external_link_spec.rb

@@ -34,22 +34,24 @@ RSpec.describe "Wiki page external link", :js, :selenium do
   shared_let(:admin) { create(:admin) }
   current_user { admin }
 
+  let(:external_url) { "https://www.openprojet.org/" }
   let(:project) { create(:project, enabled_module_names: %w[wiki]) }
   let!(:wiki_page) do
     create(:wiki_page,
            wiki: project.wiki,
            author: admin,
            title: "Wiki Page No. 55",
-           text: 'A link to <a href="http://0.0.0.0:3001/">OpenProject</a>.')
+           text: "A link to <a href='#{external_url}'>OpenProject</a>.")
   end
 
   it "opens that link in a new window or tab" do
     visit project_wiki_path(project, wiki_page)
 
-    link = page.find('a[href^="http://0.0.0.0:3001/"]')
+    href = external_redirect_path(url: external_url)
+    link = page.find_link("OpenProject", href:)
     new_window = window_opened_by { link.click }
     within_window new_window do
-      expect(page.current_url).to start_with "http://0.0.0.0:3001/"
+      expect(page.current_url).to start_with external_url
     end
     new_window.close
   end

spec/features/work_packages/details/custom_fields/custom_field_spec.rb

@@ -251,7 +251,8 @@ RSpec.describe "custom field inplace editor", :js do
       field.update "http://example.com"
 
       field.expect_state_text "http://example.com"
-      expect(field.display_element).to have_css('a[href="http://example.com"]')
+      href = external_redirect_path(url: "http://example.com/")
+      expect(field.display_element).to have_link("http://example.com", href:)
 
       field.update "bogus", expect_failure: true
 
@@ -261,7 +262,8 @@ RSpec.describe "custom field inplace editor", :js do
       field.save!
 
       field.expect_state_text "http://community.openproject.org"
-      expect(field.display_element).to have_css('a[href="http://community.openproject.org"]')
+      href = external_redirect_path(url: "http://community.openproject.org/")
+      expect(field.display_element).to have_link("http://community.openproject.org", href:)
     end
   end
 end

spec/features/work_packages/details/markdown/todolist_spec.rb

@@ -186,8 +186,11 @@ RSpec.describe "Todolists in CKEditor", :js, :selenium do
       expect(page).to have_css(".op-uc-list--task-checkbox", count: 3)
       expect(page).to have_css(".op-uc-list--task-checkbox[checked]", count: 1)
 
-      expect(page).to have_css('.op-uc-list--item a[href="https://community.openproject.com/"]')
-      nested_link = page.find('.op-uc-list--item .op-uc-list--item a[href="https://community.openproject.com/nested"]')
+      link = external_redirect_path(url: "https://community.openproject.com/")
+      expect(page).to have_css(".op-uc-list--item a[href='#{link}']")
+
+      link = external_redirect_path(url: "https://community.openproject.com/nested")
+      nested_link = page.find(".op-uc-list--item .op-uc-list--item a[href='#{link}']")
       expect(nested_link.text).to eq "This is a link"
 
       description = WorkPackage.last.description

spec/features/wysiwyg/linking_spec.rb

@@ -59,7 +59,7 @@ RSpec.describe "Wysiwyg linking", :js do
         "[http://example.org/link with spaces](http://example.org/link%20with%20spaces)"
       )
 
-      expect(page).to have_link(href: "http://example.org/link%20with%20spaces")
+      expect(page).to have_link(href: external_redirect_path(url: "http://example.org/link%20with%20spaces"))
     end
   end
 
@@ -76,7 +76,7 @@ RSpec.describe "Wysiwyg linking", :js do
       editor.insert_link "https://example.org/path"
       click_on "Save"
       expect_flash(message: "Successful update.")
-      expect(page).to have_link(href: "https://example.org/path")
+      expect(page).to have_link(href: external_redirect_path(url: "https://example.org/path"))
     end
 
     it "allows linking the custom protocol" do

spec/lib/open_project/text_formatting/markdown/user_provided_links_spec.rb

@@ -31,11 +31,10 @@
 require "spec_helper"
 require_relative "expected_markdown"
 
-RSpec.describe OpenProject::TextFormatting,
-               "user provided links" do
+RSpec.describe OpenProject::TextFormatting, "user provided links" do # rubocop:disable RSpec/SpecFilePathFormat
   include_context "expected markdown modules"
 
-  context "hardened against tabnabbing" do
+  describe "hardened against tabnabbing" do
     it_behaves_like "format_text produces" do
       let(:raw) do
         <<~RAW
@@ -53,7 +52,7 @@ RSpec.describe OpenProject::TextFormatting,
     end
   end
 
-  context "strips data-allow-external-link attribute to prevent bypassing link capture" do
+  describe "strips data-allow-external-link attribute to prevent bypassing link capture" do
     it_behaves_like "format_text produces" do
       let(:raw) do
         <<~RAW
@@ -71,7 +70,7 @@ RSpec.describe OpenProject::TextFormatting,
     end
   end
 
-  context "autolinks" do
+  describe "autolinks" do
     context "for urls" do
       it_behaves_like "format_text produces" do
         let(:raw) do
@@ -109,8 +108,8 @@ RSpec.describe OpenProject::TextFormatting,
     end
   end
 
-  context "relative URLS" do
-    context "path_only is true (default)" do
+  describe "relative URLS" do
+    context "when path_only is true (default)" do
       it_behaves_like "format_text produces" do
         let(:raw) do
           <<~RAW
@@ -128,7 +127,7 @@ RSpec.describe OpenProject::TextFormatting,
       end
     end
 
-    context "path_only is false", with_settings: { host_name: "openproject.org" } do
+    context "when path_only is false", with_settings: { host_name: "openproject.org" } do
       let(:options) { { only_path: false } }
 
       it_behaves_like "format_text produces" do

spec/lib/redmine/i18n_spec.rb

@@ -199,11 +199,16 @@ module OpenProject
 
       it "allows to insert links into translations" do
         translated = link_translate :translation_with_a_link, links: urls, external: false
+        fragment = Capybara.string(translated)
 
-        expect(translated).to eq(
-          'There is a <a href="http://openproject.com/foo" data-view-component="true" class="Link Link--underline">link</a>' +
-          ' in this translation! Maybe even <a href="/baz" data-view-component="true" class="Link Link--underline">two</a>?'
-        )
+        links = fragment.all("a")
+        expect(links.size).to eq(2)
+
+        expect(links[0].text).to eq("link")
+        expect(links[0][:href]).to eq("http://openproject.com/foo")
+
+        expect(links[1].text).to eq("two")
+        expect(links[1][:href]).to eq("/baz")
       end
 
       context "when passing URLs as a list of symbols" do
@@ -219,11 +224,16 @@ module OpenProject
 
         it "resolves the links from static links" do
           translated = link_translate :translation_with_a_link, links: urls, external: false
+          fragment = Capybara.string(translated)
 
-          expect(translated).to eq(
-            'There is a <a href="https://example.com/a-b" data-view-component="true" class="Link Link--underline">link</a>' +
-            ' in this translation! Maybe even <a href="/a-c" data-view-component="true" class="Link Link--underline">two</a>?'
-          )
+          links = fragment.all("a")
+          expect(links.size).to eq(2)
+
+          expect(links[0].text).to eq("link")
+          expect(links[0][:href]).to eq("https://example.com/a-b")
+
+          expect(links[1].text).to eq("two")
+          expect(links[1][:href]).to eq("/a-c")
         end
       end
     end