Merge branch 'fe-show-security-warning-for-fork-pipelines' into 'master'

Show Security Warning Modal for fork pipelines

Closes #219754

See merge request gitlab-org/gitlab!36951

app/assets/javascripts/commit/pipelines/pipelines_table.vue

 <script>
-import { GlDeprecatedButton, GlLoadingIcon } from '@gitlab/ui';
+import { GlButton, GlLoadingIcon, GlModal, GlLink } from '@gitlab/ui';
 import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils';
 import PipelinesService from '~/pipelines/services/pipelines_service';
 import PipelineStore from '~/pipelines/stores/pipelines_store';
@@ -12,8 +12,10 @@ import CIPaginationMixin from '~/vue_shared/mixins/ci_pagination_api_mixin';
 export default {
   components: {
     TablePagination,
-    GlDeprecatedButton,
+    GlButton,
     GlLoadingIcon,
+    GlModal,
+    GlLink,
   },
   mixins: [pipelinesMixin, CIPaginationMixin],
   props: {
@@ -38,11 +40,21 @@ export default {
       required: false,
       default: 'child',
     },
-    canRunPipeline: {
+    canCreatePipelineInTargetProject: {
       type: Boolean,
       required: false,
       default: false,
     },
+    sourceProjectFullPath: {
+      type: String,
+      required: false,
+      default: '',
+    },
+    targetProjectFullPath: {
+      type: String,
+      required: false,
+      default: '',
+    },
     projectId: {
       type: String,
       required: false,
@@ -63,6 +75,7 @@ export default {
       state: store.state,
       page: getParameterByName('page') || '1',
       requestData: {},
+      modalId: 'create-pipeline-for-fork-merge-request-modal',
     };
   },
 
@@ -75,13 +88,28 @@ export default {
     },
     /**
      * The Run Pipeline button can only be rendered when:
-     * - In MR view -  we use `canRunPipeline` for that purpose
+     * - In MR view -  we use `canCreatePipelineInTargetProject` for that purpose
      * - If the latest pipeline has the `detached_merge_request_pipeline` flag
      *
      * @returns {Boolean}
      */
     canRenderPipelineButton() {
-      return this.canRunPipeline && this.latestPipelineDetachedFlag;
+      return this.latestPipelineDetachedFlag;
+    },
+    isForkMergeRequest() {
+      return this.sourceProjectFullPath !== this.targetProjectFullPath;
+    },
+    isLatestPipelineCreatedInTargetProject() {
+      const latest = this.state.pipelines[0];
+
+      return latest?.project?.full_path === `/${this.targetProjectFullPath}`;
+    },
+    shouldShowSecurityWarning() {
+      return (
+        this.canCreatePipelineInTargetProject &&
+        this.isForkMergeRequest &&
+        !this.isLatestPipelineCreatedInTargetProject
+      );
     },
     /**
      * Checks if either `detached_merge_request_pipeline` or
@@ -148,6 +176,13 @@ export default {
         mergeRequestId: this.mergeRequestId,
       });
     },
+    tryRunPipeline() {
+      if (!this.shouldShowSecurityWarning) {
+        this.onClickRunPipeline();
+      } else {
+        this.$refs.modal.show();
+      }
+    },
   },
 };
 </script>
@@ -171,16 +206,53 @@ export default {
 
     <div v-else-if="shouldRenderTable" class="table-holder">
       <div v-if="canRenderPipelineButton" class="nav justify-content-end">
-        <gl-deprecated-button
-          v-if="canRenderPipelineButton"
+        <gl-button
           variant="success"
           class="js-run-mr-pipeline prepend-top-10 btn-wide-on-xs"
           :disabled="state.isRunningMergeRequestPipeline"
-          @click="onClickRunPipeline"
+          @click="tryRunPipeline"
         >
           <gl-loading-icon v-if="state.isRunningMergeRequestPipeline" inline />
           {{ s__('Pipelines|Run Pipeline') }}
-        </gl-deprecated-button>
+        </gl-button>
+
+        <gl-modal
+          :id="modalId"
+          ref="modal"
+          :modal-id="modalId"
+          :title="s__('Pipelines|Are you sure you want to run this pipeline?')"
+          :ok-title="s__('Pipelines|Run Pipeline')"
+          ok-variant="danger"
+          @ok="onClickRunPipeline"
+        >
+          <p>
+            {{
+              s__(
+                'Pipelines|This pipeline will run code originating from a forked project merge request. This means that the code can potentially have security considerations like exposing CI variables.',
+              )
+            }}
+          </p>
+          <p>
+            {{
+              s__(
+                "Pipelines|It is recommended the code is reviewed thoroughly before running this pipeline with the parent project's CI resource.",
+              )
+            }}
+          </p>
+          <p>
+            {{
+              s__(
+                'Pipelines|If you are unsure, please ask a project maintainer to review it for you.',
+              )
+            }}
+          </p>
+          <gl-link
+            href="/help/ci/merge_request_pipelines/index.html#create-pipelines-in-the-parent-project-for-merge-requests-from-a-forked-project"
+            target="_blank"
+          >
+            {{ s__('Pipelines|More Information') }}
+          </gl-link>
+        </gl-modal>
       </div>
 
       <pipelines-table-component

app/assets/javascripts/merge_request_tabs.js

@@ -358,7 +358,11 @@ export default class MergeRequestTabs {
         emptyStateSvgPath: pipelineTableViewEl.dataset.emptyStateSvgPath,
         errorStateSvgPath: pipelineTableViewEl.dataset.errorStateSvgPath,
         autoDevopsHelpPath: pipelineTableViewEl.dataset.helpAutoDevopsPath,
-        canRunPipeline: true,
+        canCreatePipelineInTargetProject: Boolean(
+          mrWidgetData?.can_create_pipeline_in_target_project,
+        ),
+        sourceProjectFullPath: mrWidgetData?.source_project_full_path || '',
+        targetProjectFullPath: mrWidgetData?.target_project_full_path || '',
         projectId: pipelineTableViewEl.dataset.projectId,
         mergeRequestId: mrWidgetData ? mrWidgetData.iid : null,
       },

app/serializers/merge_request_widget_entity.rb

@@ -14,6 +14,10 @@ class MergeRequestWidgetEntity < Grape::Entity
     merge_request.project&.full_path
   end
 
+  expose :can_create_pipeline_in_target_project do |merge_request|
+    can?(current_user, :create_pipeline, merge_request.target_project)
+  end
+
   expose :email_patches_path do |merge_request|
     project_merge_request_path(merge_request.project, merge_request, format: :patch)
   end

changelogs/unreleased/fe-show-security-warning-for-fork-pipelines.yml

+---
+title: Show Security Warning Modal for fork pipelines
+merge_request: 36951
+author:
+type: added

doc/ci/merge_request_pipelines/index.md

@@ -166,31 +166,33 @@ Read the [documentation on Pipelines for Merged Results](pipelines_for_merged_re
 
 Read the [documentation on Merge Trains](pipelines_for_merged_results/merge_trains/index.md).
 
-## Important notes about merge requests from forked projects
-
-Note that the current behavior is subject to change. In the usual contribution
-flow, external contributors follow the following steps:
-
-1. Fork a parent project.
-1. Create a merge request from the forked project that targets the `master` branch
-   in the parent project.
-1. A pipeline runs on the merge request.
-1. A maintainer from the parent project checks the pipeline result, and merge
-   into a target branch if the latest pipeline has passed.
-
-Currently, those pipelines are created in a **forked** project, not in the
-parent project. This means you cannot completely trust the pipeline result,
-because, technically, external contributors can disguise their pipeline results
-by tweaking their GitLab Runner in the forked project.
-
-There are multiple reasons why GitLab doesn't allow those pipelines to be
-created in the parent project, but one of the biggest reasons is security concern.
-External users could steal secret variables from the parent project by modifying
-`.gitlab-ci.yml`, which could be some sort of credentials. This should not happen.
-
-We're discussing a secure solution of running pipelines for merge requests
-that are submitted from forked projects,
-see [the issue about the permission extension](https://gitlab.com/gitlab-org/gitlab/-/issues/11934).
+## Create pipelines in the parent project for merge requests from a forked project
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/217451) in GitLab 13.3.
+
+By default, external contributors working from forks can't create pipelines in the
+parent project. When a pipeline for merge requests is triggered by a merge request
+coming from a fork:
+
+- It's created and runs in the fork (source) project, not the parent (target) project.
+- It uses the fork project's CI/CD configuration and resources.
+
+Sometimes parent project members want the pipeline to run in the parent
+project. This could be to ensure that the post-merge pipeline passes in the parent project.
+For example, a fork project could try to use a corrupted Runner that doesn't execute
+test scripts properly, but reports a passed pipeline. Reviewers in the parent project
+could mistakenly trust the merge request because it passed a faked pipeline.
+
+Parent project members with at least [Developer permissions](../../user/permissions.md)
+can create pipelines in the parent project for merge requests
+from a forked project. In the merge request, go to the **Pipelines** and click
+**Run Pipeline** button.
+
+CAUTION: **Caution:**
+Fork merge requests could contain malicious code that tries to steal secrets in the
+parent project when the pipeline runs, even before merge. Reviewers must carefully
+check the changes in the merge request before triggering the pipeline. GitLab shows
+a warning that must be accepted before the pipeline can be triggered.
 
 ## Additional predefined variables
 

doc/ci/merge_request_pipelines/pipelines_for_merged_results/index.md

@@ -45,8 +45,6 @@ To enable pipelines for merge results:
 
 - You must have maintainer [permissions](../../../user/permissions.md).
 - You must be using [GitLab Runner](https://gitlab.com/gitlab-org/gitlab-runner) 11.9 or later.
-- You must not be forking or using cross-repo workflows. To follow progress,
-  see [#11934](https://gitlab.com/gitlab-org/gitlab/-/issues/11934).
 - You must not be using
   [fast forward merges](../../../user/project/merge_requests/fast_forward_merge.md) yet.
   To follow progress, see [#58226](https://gitlab.com/gitlab-org/gitlab/-/issues/26996).

ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue

@@ -389,7 +389,7 @@ export default {
           >
             {{
               s__(
-                'mrWidget|Fork merge requests do not create merge request pipelines which validate a post merge result',
+                'mrWidget|Fork project merge requests do not create merge request pipelines that validate a post merge result unless invoked by a project member.',
               )
             }}
           </mr-widget-alert-message>

ee/spec/features/merge_request/user_sees_merge_widget_spec.rb

@@ -70,13 +70,14 @@ RSpec.describe 'Merge request > User sees merge widget', :js do
       let(:traits) { [:with_detached_merge_request_pipeline] }
       let(:options) { {} }
 
-      it 'shows a warning that fork project cannot create merge request pipelines', :sidekiq_might_not_need_inline do
+      it 'shows a warning that fork project merge request does not create merge request pipelines by default', :sidekiq_might_not_need_inline do
         visit project_merge_request_path(project, merge_request)
 
         within('.warning_message') do
           expect(page)
-            .to have_content('Fork merge requests do not create merge request' \
-                             ' pipelines which validate a post merge result')
+            .to have_content('Fork project merge requests do not create merge' \
+                             ' request pipelines that validate a post merge result' \
+                             ' unless invoked by a project member.')
         end
       end
     end

locale/gitlab.pot

@@ -17158,6 +17158,9 @@ msgstr ""
 msgid "Pipelines|API"
 msgstr ""
 
+msgid "Pipelines|Are you sure you want to run this pipeline?"
+msgstr ""
+
 msgid "Pipelines|Build with confidence"
 msgstr ""
 
@@ -17182,9 +17185,18 @@ msgstr ""
 msgid "Pipelines|Group %{namespace_name} has exceeded its pipeline minutes quota. Unless you buy additional pipeline minutes, no new jobs or pipelines in its projects will run."
 msgstr ""
 
+msgid "Pipelines|If you are unsure, please ask a project maintainer to review it for you."
+msgstr ""
+
+msgid "Pipelines|It is recommended the code is reviewed thoroughly before running this pipeline with the parent project's CI resource."
+msgstr ""
+
 msgid "Pipelines|Loading Pipelines"
 msgstr ""
 
+msgid "Pipelines|More Information"
+msgstr ""
+
 msgid "Pipelines|Project cache successfully reset."
 msgstr ""
 
@@ -17206,6 +17218,9 @@ msgstr ""
 msgid "Pipelines|This is a child pipeline within the parent pipeline"
 msgstr ""
 
+msgid "Pipelines|This pipeline will run code originating from a forked project merge request. This means that the code can potentially have security considerations like exposing CI variables."
+msgstr ""
+
 msgid "Pipelines|This project is not currently set up to run pipelines."
 msgstr ""
 
@@ -28472,6 +28487,9 @@ msgstr ""
 msgid "mrWidget|Fork merge requests do not create merge request pipelines which validate a post merge result"
 msgstr ""
 
+msgid "mrWidget|Fork project merge requests do not create merge request pipelines that validate a post merge result unless invoked by a project member."
+msgstr ""
+
 msgid "mrWidget|If the %{branch} branch exists in your local repository, you can merge this merge request manually using the"
 msgstr ""
 

spec/features/merge_request/user_sees_pipelines_spec.rb

@@ -123,14 +123,24 @@ RSpec.describe 'Merge request > User sees pipelines', :js do
     context 'when actor is a developer in parent project' do
       let(:actor) { developer_in_parent }
 
-      it 'creates a pipeline in the parent project' do
+      it 'creates a pipeline in the parent project when user proceeds with the warning' do
         visit project_merge_request_path(parent_project, merge_request)
 
         create_merge_request_pipeline
+        act_on_security_warning(action: 'Run Pipeline')
 
         check_pipeline(expected_project: parent_project)
         check_head_pipeline(expected_project: parent_project)
       end
+
+      it 'does not create a pipeline in the parent project when user cancels the action' do
+        visit project_merge_request_path(parent_project, merge_request)
+
+        create_merge_request_pipeline
+        act_on_security_warning(action: 'Cancel')
+
+        check_no_pipelines
+      end
     end
 
     context 'when actor is a developer in fork project' do
@@ -187,6 +197,19 @@ RSpec.describe 'Merge request > User sees pipelines', :js do
         expect(page.find('.pipeline-id')[:href]).to include(expected_project.full_path)
       end
     end
+
+    def act_on_security_warning(action:)
+      page.within('#create-pipeline-for-fork-merge-request-modal') do
+        expect(page).to have_content('Are you sure you want to run this pipeline?')
+        click_button(action)
+      end
+    end
+
+    def check_no_pipelines
+      page.within('.ci-table') do
+        expect(page).to have_selector('.commit', count: 1)
+      end
+    end
   end
 
   describe 'race condition' do

spec/frontend/commit/pipelines/pipelines_spec.js

@@ -121,14 +121,14 @@ describe('Pipelines table in Commits and Merge requests', () => {
       pipelineCopy = { ...pipeline };
     });
 
-    describe('when latest pipeline has detached flag and canRunPipeline is true', () => {
+    describe('when latest pipeline has detached flag', () => {
       it('renders the run pipeline button', done => {
         pipelineCopy.flags.detached_merge_request_pipeline = true;
         pipelineCopy.flags.merge_request_pipeline = true;
 
         mock.onGet('endpoint.json').reply(200, [pipelineCopy]);
 
-        vm = mountComponent(PipelinesTable, { ...props, canRunPipeline: true });
+        vm = mountComponent(PipelinesTable, { ...props });
 
         setImmediate(() => {
           expect(vm.$el.querySelector('.js-run-mr-pipeline')).not.toBeNull();
@@ -137,14 +137,14 @@ describe('Pipelines table in Commits and Merge requests', () => {
       });
     });
 
-    describe('when latest pipeline has detached flag and canRunPipeline is false', () => {
+    describe('when latest pipeline does not have detached flag', () => {
       it('does not render the run pipeline button', done => {
-        pipelineCopy.flags.detached_merge_request_pipeline = true;
-        pipelineCopy.flags.merge_request_pipeline = true;
+        pipelineCopy.flags.detached_merge_request_pipeline = false;
+        pipelineCopy.flags.merge_request_pipeline = false;
 
         mock.onGet('endpoint.json').reply(200, [pipelineCopy]);
 
-        vm = mountComponent(PipelinesTable, { ...props, canRunPipeline: false });
+        vm = mountComponent(PipelinesTable, { ...props });
 
         setImmediate(() => {
           expect(vm.$el.querySelector('.js-run-mr-pipeline')).toBeNull();
@@ -153,39 +153,47 @@ describe('Pipelines table in Commits and Merge requests', () => {
       });
     });
 
-    describe('when latest pipeline does not have detached flag and canRunPipeline is true', () => {
-      it('does not render the run pipeline button', done => {
-        pipelineCopy.flags.detached_merge_request_pipeline = false;
-        pipelineCopy.flags.merge_request_pipeline = false;
+    describe('on click', () => {
+      const findModal = () =>
+        document.querySelector('#create-pipeline-for-fork-merge-request-modal');
 
-        mock.onGet('endpoint.json').reply(200, [pipelineCopy]);
+      beforeEach(() => {
+        pipelineCopy.flags.detached_merge_request_pipeline = true;
 
-        vm = mountComponent(PipelinesTable, { ...props, canRunPipeline: true });
+        mock.onGet('endpoint.json').reply(200, [pipelineCopy]);
 
-        setImmediate(() => {
-          expect(vm.$el.querySelector('.js-run-mr-pipeline')).toBeNull();
-          done();
+        vm = mountComponent(PipelinesTable, {
+          ...props,
+          canRunPipeline: true,
+          projectId: '5',
+          mergeRequestId: 3,
         });
       });
-    });
 
-    describe('when latest pipeline does not have detached flag and merge_request_pipeline is true', () => {
-      it('does not render the run pipeline button', done => {
-        pipelineCopy.flags.detached_merge_request_pipeline = false;
-        pipelineCopy.flags.merge_request_pipeline = true;
+      it('updates the loading state', done => {
+        jest.spyOn(Api, 'postMergeRequestPipeline').mockReturnValue(Promise.resolve());
 
-        mock.onGet('endpoint.json').reply(200, [pipelineCopy]);
+        setImmediate(() => {
+          vm.$el.querySelector('.js-run-mr-pipeline').click();
 
-        vm = mountComponent(PipelinesTable, { ...props, canRunPipeline: false });
+          vm.$nextTick(() => {
+            expect(findModal()).toBeNull();
+            expect(vm.state.isRunningMergeRequestPipeline).toBe(true);
 
-        setImmediate(() => {
-          expect(vm.$el.querySelector('.js-run-mr-pipeline')).toBeNull();
-          done();
+            setImmediate(() => {
+              expect(vm.state.isRunningMergeRequestPipeline).toBe(false);
+
+              done();
+            });
+          });
         });
       });
     });
 
-    describe('on click', () => {
+    describe('on click for fork merge request', () => {
+      const findModal = () =>
+        document.querySelector('#create-pipeline-for-fork-merge-request-modal');
+
       beforeEach(() => {
         pipelineCopy.flags.detached_merge_request_pipeline = true;
 
@@ -193,26 +201,23 @@ describe('Pipelines table in Commits and Merge requests', () => {
 
         vm = mountComponent(PipelinesTable, {
           ...props,
-          canRunPipeline: true,
           projectId: '5',
           mergeRequestId: 3,
+          canCreatePipelineInTargetProject: true,
+          sourceProjectFullPath: 'test/parent-project',
+          targetProjectFullPath: 'test/fork-project',
         });
       });
 
-      it('updates the loading state', done => {
+      it('shows a security warning modal', done => {
         jest.spyOn(Api, 'postMergeRequestPipeline').mockReturnValue(Promise.resolve());
 
         setImmediate(() => {
           vm.$el.querySelector('.js-run-mr-pipeline').click();
 
           vm.$nextTick(() => {
-            expect(vm.state.isRunningMergeRequestPipeline).toBe(true);
-
-            setImmediate(() => {
-              expect(vm.state.isRunningMergeRequestPipeline).toBe(false);
-
-              done();
-            });
+            expect(findModal()).not.toBeNull();
+            done();
           });
         });
       });

spec/serializers/merge_request_widget_entity_spec.rb

@@ -31,6 +31,28 @@ RSpec.describe MergeRequestWidgetEntity do
     end
   end
 
+  describe 'can_create_pipeline_in_target_project' do
+    context 'when user has permission' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'includes the correct permission info' do
+        expect(subject[:can_create_pipeline_in_target_project]).to eq(true)
+      end
+    end
+
+    context 'when user does not have permission' do
+      before do
+        project.add_guest(user)
+      end
+
+      it 'includes the correct permission info' do
+        expect(subject[:can_create_pipeline_in_target_project]).to eq(false)
+      end
+    end
+  end
+
   describe 'issues links' do
     it 'includes issues links when requested' do
       data = described_class.new(resource, request: request, issues_links: true).as_json