Merge branch '51952-forking-via-webide' into 'master'

Resolve "500 error when forking via the web IDE button"

See merge request gitlab-org/gitlab-ce!29909

app/controllers/concerns/continue_params.rb

@@ -6,7 +6,7 @@ module ContinueParams
 
   def continue_params
     continue_params = params[:continue]
-    return unless continue_params
+    return {} unless continue_params
 
     continue_params = continue_params.permit(:to, :notice, :notice_now)
     continue_params[:to] = safe_redirect_path(continue_params[:to])

app/controllers/concerns/internal_redirect.rb

@@ -5,8 +5,8 @@ module InternalRedirect
 
   def safe_redirect_path(path)
     return unless path
-    # Verify that the string starts with a `/` but not a double `/`.
-    return unless path =~ %r{^/\w.*$}
+    # Verify that the string starts with a `/` and a known route character.
+    return unless path =~ %r{^/[-\w].*$}
 
     uri = URI(path)
     # Ignore anything path of the redirect except for the path, querystring and,

app/controllers/projects/forks_controller.rb

@@ -46,18 +46,14 @@ class Projects::ForksController < Projects::ApplicationController
 
     @forked_project ||= ::Projects::ForkService.new(project, current_user, namespace: namespace).execute
 
-    if @forked_project.saved? && @forked_project.forked?
-      if @forked_project.import_in_progress?
-        redirect_to project_import_path(@forked_project, continue: continue_params)
-      else
-        if continue_params
-          redirect_to continue_params[:to], notice: continue_params[:notice]
-        else
-          redirect_to project_path(@forked_project), notice: "The project '#{@forked_project.name}' was successfully forked."
-        end
-      end
-    else
+    if !@forked_project.saved? || !@forked_project.forked?
       render :error
+    elsif @forked_project.import_in_progress?
+      redirect_to project_import_path(@forked_project, continue: continue_params)
+    elsif continue_params[:to]
+      redirect_to continue_params[:to], notice: continue_params[:notice]
+    else
+      redirect_to project_path(@forked_project), notice: "The project '#{@forked_project.name}' was successfully forked."
     end
   end
   # rubocop: enable CodeReuse/ActiveRecord

app/controllers/projects/imports_controller.rb

@@ -23,7 +23,7 @@ class Projects::ImportsController < Projects::ApplicationController
 
   def show
     if @project.import_finished?
-      if continue_params&.key?(:to)
+      if continue_params[:to]
         redirect_to continue_params[:to], notice: continue_params[:notice]
       else
         redirect_to project_path(@project), notice: finished_notice
@@ -31,11 +31,7 @@ class Projects::ImportsController < Projects::ApplicationController
     elsif @project.import_failed?
       redirect_to new_project_import_path(@project)
     else
-      if continue_params && continue_params[:notice_now]
-        flash.now[:notice] = continue_params[:notice_now]
-      end
-
-      # Render
+      flash.now[:notice] = continue_params[:notice_now]
     end
   end
 

app/controllers/projects/jobs_controller.rb

@@ -103,7 +103,7 @@ class Projects::JobsController < Projects::ApplicationController
 
     @build.cancel
 
-    if continue_params
+    if continue_params[:to]
       redirect_to continue_params[:to]
     else
       redirect_to builds_project_pipeline_path(@project, @build.pipeline.id)

changelogs/unreleased/51952-forking-via-webide.yml

+---
+title: Resolve "500 error when forking via the web IDE button"
+merge_request: 29909
+author:
+type: fixed

spec/controllers/concerns/continue_params_spec.rb

@@ -18,6 +18,14 @@ describe ContinueParams do
     ActionController::Parameters.new(continue: params)
   end
 
+  it 'returns an empty hash if params are not present' do
+    allow(controller).to receive(:params) do
+      ActionController::Parameters.new
+    end
+
+    expect(controller.continue_params).to eq({})
+  end
+
   it 'cleans up any params that are not allowed' do
     allow(controller).to receive(:params) do
       strong_continue_params(to: '/hello',

spec/controllers/concerns/internal_redirect_spec.rb

@@ -15,44 +15,71 @@ describe InternalRedirect do
   subject(:controller) { controller_class.new }
 
   describe '#safe_redirect_path' do
-    it 'is `nil` for invalid uris' do
-      expect(controller.safe_redirect_path('Hello world')).to be_nil
+    where(:input) do
+      [
+        'Hello world',
+        '//example.com/hello/world',
+        'https://example.com/hello/world'
+      ]
     end
 
-    it 'is `nil` for paths trying to include a host' do
-      expect(controller.safe_redirect_path('//example.com/hello/world')).to be_nil
+    with_them 'being invalid' do
+      it 'returns nil' do
+        expect(controller.safe_redirect_path(input)).to be_nil
+      end
     end
 
-    it 'returns the path if it is valid' do
-      expect(controller.safe_redirect_path('/hello/world')).to eq('/hello/world')
+    where(:input) do
+      [
+        '/hello/world',
+        '/-/ide/project/path'
+      ]
     end
 
-    it 'returns the path with querystring if it is valid' do
-      expect(controller.safe_redirect_path('/hello/world?hello=world#L123'))
-        .to eq('/hello/world?hello=world#L123')
+    with_them 'being valid' do
+      it 'returns the path' do
+        expect(controller.safe_redirect_path(input)).to eq(input)
+      end
+
+      it 'returns the path with querystring and fragment' do
+        expect(controller.safe_redirect_path("#{input}?hello=world#L123"))
+          .to eq("#{input}?hello=world#L123")
+      end
     end
   end
 
   describe '#safe_redirect_path_for_url' do
-    it 'is `nil` for invalid urls' do
-      expect(controller.safe_redirect_path_for_url('Hello world')).to be_nil
+    where(:input) do
+      [
+        'Hello world',
+        'http://example.com/hello/world',
+        'http://test.host:3000/hello/world'
+      ]
     end
 
-    it 'is `nil` for urls from a with a different host' do
-      expect(controller.safe_redirect_path_for_url('http://example.com/hello/world')).to be_nil
+    with_them 'being invalid' do
+      it 'returns nil' do
+        expect(controller.safe_redirect_path_for_url(input)).to be_nil
+      end
     end
 
-    it 'is `nil` for urls from a with a different port' do
-      expect(controller.safe_redirect_path_for_url('http://test.host:3000/hello/world')).to be_nil
+    where(:input) do
+      [
+        'http://test.host/hello/world'
+      ]
     end
 
-    it 'returns the path if the url is on the same host' do
-      expect(controller.safe_redirect_path_for_url('http://test.host/hello/world')).to eq('/hello/world')
-    end
+    with_them 'being on the same host' do
+      let(:path) { URI(input).path }
 
-    it 'returns the path including querystring if the url is on the same host' do
-      expect(controller.safe_redirect_path_for_url('http://test.host/hello/world?hello=world#L123'))
-        .to eq('/hello/world?hello=world#L123')
+      it 'returns the path' do
+        expect(controller.safe_redirect_path_for_url(input)).to eq(path)
+      end
+
+      it 'returns the path with querystring and fragment' do
+        expect(controller.safe_redirect_path_for_url("#{input}?hello=world#L123"))
+          .to eq("#{path}?hello=world#L123")
+      end
     end
   end
 
@@ -82,12 +109,16 @@ describe InternalRedirect do
   end
 
   describe '#host_allowed?' do
-    it 'allows uris with the same host and port' do
+    it 'allows URI with the same host and port' do
       expect(controller.host_allowed?(URI('http://test.host/test'))).to be(true)
     end
 
-    it 'rejects uris with other host and port' do
+    it 'rejects URI with other host' do
       expect(controller.host_allowed?(URI('http://example.com/test'))).to be(false)
     end
+
+    it 'rejects URI with other port' do
+      expect(controller.host_allowed?(URI('http://test.host:3000/test'))).to be(false)
+    end
   end
 end

spec/controllers/projects/forks_controller_spec.rb

@@ -115,24 +115,34 @@ describe Projects::ForksController do
   end
 
   describe 'POST create' do
-    def post_create
+    def post_create(params = {})
       post :create,
         params: {
           namespace_id: project.namespace,
           project_id: project,
           namespace_key: user.namespace.id
-        }
+        }.merge(params)
     end
 
     context 'when user is signed in' do
-      it 'responds with status 302' do
+      before do
         sign_in(user)
+      end
 
+      it 'responds with status 302' do
         post_create
 
         expect(response).to have_gitlab_http_status(302)
         expect(response).to redirect_to(namespace_project_import_path(user.namespace, project))
       end
+
+      it 'passes continue params to the redirect' do
+        continue_params = { to: '/-/ide/project/path', notice: 'message' }
+        post_create continue: continue_params
+
+        expect(response).to have_gitlab_http_status(302)
+        expect(response).to redirect_to(namespace_project_import_path(user.namespace, project, continue: continue_params))
+      end
     end
 
     context 'when user is not signed in' do

spec/features/projects/files/user_edits_files_spec.rb

@@ -118,19 +118,31 @@ describe 'Projects > Files > User edits files', :js do
       wait_for_requests
     end
 
-    it 'inserts a content of a file in a forked project' do
-      click_link('.gitignore')
-      find('.js-edit-blob').click
-
+    def expect_fork_prompt
       expect(page).to have_link('Fork')
       expect(page).to have_button('Cancel')
+      expect(page).to have_content(
+        "You're not allowed to edit files in this project directly. "\
+        "Please fork this project, make your changes there, and submit a merge request."
+      )
+    end
 
-      click_link('Fork')
-
+    def expect_fork_status
       expect(page).to have_content(
         "You're not allowed to make changes to this project directly. "\
         "A fork of this project has been created that you can make changes in, so you can submit a merge request."
       )
+    end
+
+    it 'inserts a content of a file in a forked project' do
+      click_link('.gitignore')
+      click_button('Edit')
+
+      expect_fork_prompt
+
+      click_link('Fork')
+
+      expect_fork_status
 
       find('.file-editor', match: :first)
 
@@ -140,12 +152,24 @@ describe 'Projects > Files > User edits files', :js do
       expect(evaluate_script('ace.edit("editor").getValue()')).to eq('*.rbca')
     end
 
+    it 'opens the Web IDE in a forked project' do
+      click_link('.gitignore')
+      click_button('Web IDE')
+
+      expect_fork_prompt
+
+      click_link('Fork')
+
+      expect_fork_status
+
+      expect(page).to have_css('.ide .multi-file-tab', text: '.gitignore')
+    end
+
     it 'commits an edited file in a forked project' do
       click_link('.gitignore')
       find('.js-edit-blob').click
 
-      expect(page).to have_link('Fork')
-      expect(page).to have_button('Cancel')
+      expect_fork_prompt
 
       click_link('Fork')