Use Array.join rather than URI.join

Add spec for cases where URI.join does not work as expected

4 files changed, 35 insertions, 11 deletions

diff --git a/app/models/environment.rb b/app/models/environment.rb
index 392481ea0cc..513427ac2c5 100644
--- a/app/models/environment.rb
+++ b/app/models/environment.rb
@@ -204,7 +204,7 @@ class Environment < ApplicationRecord
     public_path = project.public_path_for_source_path(path, commit_sha)
     return unless public_path
 
-    [external_url, public_path].join('/')
+    [external_url.delete_suffix('/'), public_path.delete_prefix('/')].join('/')
   end
 
   def expire_etag_cache
diff --git a/changelogs/unreleased/double-slash-64592.yml b/changelogs/unreleased/double-slash-64592.yml
new file mode 100644
index 00000000000..e3b5b197ac5
--- /dev/null
+++ b/changelogs/unreleased/double-slash-64592.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent double slash in review apps path
+merge_request: 31212
+author:
+type: fixed
diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb
index ce0681c4331..d2e0bed721e 100644
--- a/spec/models/environment_spec.rb
+++ b/spec/models/environment_spec.rb
@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe Environment, :use_clean_rails_memory_store_caching do
   include ReactiveCachingHelpers
+  using RSpec::Parameterized::TableSyntax
 
   let(:project) { create(:project, :stubbed_repository) }
   subject(:environment) { create(:environment, project: project) }
@@ -782,12 +783,9 @@ describe Environment, :use_clean_rails_memory_store_caching do
     let(:source_path) { 'source/file.html' }
     let(:sha) { RepoHelpers.sample_commit.id }
 
-    before do
-      environment.external_url = 'http://example.com'
-    end
-
     context 'when the public path is not known' do
       before do
+        environment.external_url = 'http://example.com'
         allow(project).to receive(:public_path_for_source_path).with(source_path, sha).and_return(nil)
       end
 
@@ -797,12 +795,23 @@ describe Environment, :use_clean_rails_memory_store_caching do
     end
 
     context 'when the public path is known' do
-      before do
-        allow(project).to receive(:public_path_for_source_path).with(source_path, sha).and_return('file.html')
-      end
-
-      it 'returns the full external URL' do
-        expect(environment.external_url_for(source_path, sha)).to eq('http://example.com/file.html')
+      where(:external_url, :public_path, :full_url) do
+        'http://example.com'          | 'file.html'         | 'http://example.com/file.html'
+        'http://example.com/'         | 'file.html'         | 'http://example.com/file.html'
+        'http://example.com'          | '/file.html'        | 'http://example.com/file.html'
+        'http://example.com/'         | '/file.html'        | 'http://example.com/file.html'
+        'http://example.com/subpath'  | 'public/file.html'  | 'http://example.com/subpath/public/file.html'
+        'http://example.com/subpath/' | 'public/file.html'  | 'http://example.com/subpath/public/file.html'
+        'http://example.com/subpath'  | '/public/file.html' | 'http://example.com/subpath/public/file.html'
+        'http://example.com/subpath/' | '/public/file.html' | 'http://example.com/subpath/public/file.html'
+      end
+      with_them do
+        it 'returns the full external URL' do
+          environment.external_url = external_url
+          allow(project).to receive(:public_path_for_source_path).with(source_path, sha).and_return(public_path)
+
+          expect(environment.external_url_for(source_path, sha)).to eq(full_url)
+        end
       end
     end
   end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 32fb0c0ff73..157103123ad 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2950,6 +2950,16 @@ describe Project do
           expect(project.public_path_for_source_path('file.html', sha)).to be_nil
         end
       end
+
+      it 'returns a public path with a leading slash unmodified' do
+        route_map = Gitlab::RouteMap.new(<<-MAP.strip_heredoc)
+          - source: 'source/file.html'
+            public: '/public/file'
+        MAP
+        allow(project).to receive(:route_map_for).with(sha).and_return(route_map)
+
+        expect(project.public_path_for_source_path('source/file.html', sha)).to eq('/public/file')
+      end
     end
 
     context 'when there is no route map' do