Redirect url in the link validated for being external

Merge branch 'security-sg-redirect-check-for-releases-17-1' into '17-1-stable-ee'

See merge request gitlab-org/security/gitlab!4442

Changelog: security

app/controllers/projects/releases_controller.rb

@@ -33,7 +33,13 @@ def index
   end
   def downloads
-    redirect_to link.url
+    parsed_redirect_uri = URI.parse(link.url)
+
+    if internal_url?(parsed_redirect_uri)
+      redirect_to link.url
+    else
+      render "projects/releases/redirect", locals: { redirect_uri: parsed_redirect_uri }, layout: false
+    end
   end
   def latest_permalink
@@ -79,4 +85,8 @@ def fetch_latest_tag
   def validate_suffix_path
     Gitlab::PathTraversal.check_path_traversal!(params[:suffix_path]) if params[:suffix_path]
   end
+
+  def internal_url?(redirect_url)
+    redirect_url.host == Gitlab.config.gitlab.host && redirect_url.port == Gitlab.config.gitlab.port
+  end
 end

app/views/projects/releases/redirect.html.haml

+.tree-holder
+  %h2= _("You are being redirected away from GitLab")
+  %p= _("Redirect url is an external url, it may contain user-generated content and malicious code. Do not continue unless you trust the author and source.")
+
+%div
+  - redirect_link_start = '<a href="%{redirect_uri}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: redirect_uri }
+  = html_escape(s_('%{redirect_link_start} Click here to redirect to %{redirect_uri_val} %{redirect_link_end}')) % { redirect_uri_val: redirect_uri, redirect_link_start: redirect_link_start, redirect_link_end: '</a>'.html_safe }

locale/gitlab.pot

@@ -1127,6 +1127,9 @@ msgstr ""
 msgid "%{project_path} is a project that you can use to add a README to your GitLab profile. Create a public project and initialize the repository with a README to get started. %{help_link_start}Learn more%{help_link_end}."
 msgstr ""
+msgid "%{redirect_link_start} Click here to redirect to %{redirect_uri_val} %{redirect_link_end}"
+msgstr ""
+
 msgid "%{reference} %{divider} created %{createdAt} by %{author} %{milestone}"
 msgstr ""
@@ -43311,6 +43314,9 @@ msgstr ""
 msgid "Redirect to SAML provider to test configuration"
 msgstr ""
+msgid "Redirect url is an external url, it may contain user-generated content and malicious code. Do not continue unless you trust the author and source."
+msgstr ""
+
 msgid "Redirecting"
 msgstr ""

spec/requests/projects/releases_controller_spec.rb

@@ -9,10 +9,15 @@
   # Added as a request spec because of https://gitlab.com/gitlab-org/gitlab/-/issues/232386
   describe 'GET #downloads' do
     let_it_be(:release) { create(:release, project: project, tag: 'v11.9.0-rc2' ) }
-    let!(:link) { create(:release_link, release: release, name: 'linux-amd64 binaries', filepath: filepath, url: 'https://aws.example.com/s3/project/bin/hello-darwin-amd64') }
+    let(:internal_redirect_url) { "https://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.port}/abcd" }
+    let!(:link) do
+      create(:release_link, release: release, name: 'internal gitlab url', filepath: filepath,
+        url: internal_redirect_url)
+    end
+
     let_it_be(:url) { "#{project_releases_path(project)}/#{release.tag}/downloads/bin/darwin-amd64" }
-    let(:subject) { get url }
+    subject(:download_request) { get url }
     context 'filepath redirection' do
       before do
@@ -23,23 +28,41 @@
         let(:filepath) { '/bin/darwin-amd64' }
         it 'redirects to the asset direct link' do
-          subject
-          expect(response).to redirect_to('https://aws.example.com/s3/project/bin/hello-darwin-amd64')
+          download_request
+          expect(response).to redirect_to(internal_redirect_url)
         end
         it 'redirects with a status of 302' do
-          subject
+          download_request
           expect(response).to have_gitlab_http_status(:redirect)
         end
+
+        context 'when redirect_url is external' do
+          let(:external_redirect_url) { "https://aws.example.com/s3/project/bin/hello-darwin-amd64" }
+          let!(:link) do
+            create(:release_link, release: release, name: 'linux-amd64 binaries', filepath: filepath,
+              url: external_redirect_url)
+          end
+
+          let(:redirect_text) { "Click here to redirect to #{external_redirect_url}" }
+
+          it "shows the warning page with redirect link" do
+            download_request
+
+            expect(response).to render_template(:redirect)
+            expect(response.body).to have_text(_("You are being redirected away from GitLab"))
+            expect(response.body).to have_link(_(redirect_text))
+          end
+        end
       end
       context 'invalid filepath' do
         let(:filepath) { '/binaries/win32' }
         it 'is not found' do
-          subject
+          download_request
           expect(response).to have_gitlab_http_status(:not_found)
         end