Merge branch 'gitlab-mask-secret-token' into 'master'

Mask the "Secret token" input in the webhook form

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/101775



Merged-by: Savas Vedova <svedova@gitlab.com>
Approved-by: Gregory Havenga <11164960-ghavenga@users.noreply.gitlab.com>
Approved-by: Heinrich Lee Yu <heinrich@gitlab.com>
Approved-by: Matej Latin <mlatin@gitlab.com>
Approved-by: Miranda Fluharty <mfluharty@gitlab.com>
Co-authored-by: bmarjanovic <bmarjanovic@gitlab.com>
Co-authored-by: Justin Ho <hduong@gitlab.com>
Co-authored-by: Pham Sy Minh <phamminh0227@gmail.com>

app/controllers/concerns/web_hooks/hook_actions.rb

@@ -53,6 +53,8 @@ def hook_params
 
       ps = params.require(:hook).permit(*permitted).to_h
 
+      ps.delete(:token) if action_name == 'update' && ps[:token] == WebHook::SECRET_MASK
+
       ps[:url_variables] = ps[:url_variables].to_h { [_1[:key], _1[:value].presence] } if ps.key?(:url_variables)
 
       if action_name == 'update' && ps.key?(:url_variables)

app/models/hooks/web_hook.rb

@@ -11,6 +11,7 @@ class WebHook < ApplicationRecord
   INITIAL_BACKOFF = 1.minute
   MAX_BACKOFF = 1.day
   BACKOFF_GROWTH_FACTOR = 2.0
+  SECRET_MASK = '************'
 
   attr_encrypted :token,
                  mode: :per_attribute_iv,
@@ -210,6 +211,10 @@ def update_last_failure
     # Overridden in child classes.
   end
 
+  def masked_token
+    token.present? ? SECRET_MASK : nil
+  end
+
   private
 
   def next_failure_count

app/views/shared/web_hooks/_form.html.haml

@@ -10,11 +10,11 @@
       = s_('Webhooks|URL must be percent-encoded if it contains one or more special characters.')
 .form-group
   = form.label :token, s_('Webhooks|Secret token'), class: 'label-bold'
-  = form.text_field :token, class: 'form-control gl-form-input', placeholder: ''
+  = form.password_field :token, value: hook.masked_token, autocomplete: 'new-password', class: 'form-control gl-form-input'
   %p.form-text.text-muted
     - code_start = '<code>'.html_safe
     - code_end = '</code>'.html_safe
-    = s_('Webhooks|Used to validate received payloads. Sent with the request in the %{code_start}X-Gitlab-Token HTTP%{code_end} header.').html_safe % { code_start: code_start, code_end: code_end }
+    = s_('Webhooks|Used to validate received payloads. Sent with the request in the %{code_start}X-Gitlab-Token%{code_end} HTTP header.').html_safe % { code_start: code_start, code_end: code_end }
 .form-group
   = form.label :url, s_('Webhooks|Trigger'), class: 'label-bold'
   %ul.list-unstyled

locale/gitlab.pot

@@ -45517,7 +45517,7 @@ msgstr ""
 msgid "Webhooks|URL preview"
 msgstr ""
-msgid "Webhooks|Used to validate received payloads. Sent with the request in the %{code_start}X-Gitlab-Token HTTP%{code_end} header."
+msgid "Webhooks|Used to validate received payloads. Sent with the request in the %{code_start}X-Gitlab-Token%{code_end} HTTP header."
 msgstr ""
 msgid "Webhooks|Webhook disabled"

spec/controllers/admin/hooks_controller_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Admin::HooksController do
-  let(:admin) { create(:admin) }
+  let_it_be(:admin) { create(:admin) }
 
   before do
     sign_in(admin)
@@ -33,7 +33,23 @@
   end
 
   describe 'POST #update' do
-    let!(:hook) { create(:system_hook) }
+    let_it_be_with_reload(:hook) { create(:system_hook) }
+
+    context 'with an existing token' do
+      hook_params = {
+        token: WebHook::SECRET_MASK,
+        url: "http://example.com"
+      }
+
+      it 'does not change a token' do
+        expect do
+          post :update, params: { id: hook.id, hook: hook_params }
+        end.not_to change { hook.reload.token }
+
+        expect(response).to have_gitlab_http_status(:found)
+        expect(flash[:alert]).to be_blank
+      end
+    end
 
     it 'sets all parameters' do
       hook.update!(url_variables: { 'foo' => 'bar', 'baz' => 'woo' })
@@ -61,8 +77,8 @@
   end
 
   describe 'DELETE #destroy' do
-    let!(:hook) { create(:system_hook) }
-    let!(:log) { create(:web_hook_log, web_hook: hook) }
+    let_it_be(:hook) { create(:system_hook) }
+    let_it_be(:log) { create(:web_hook_log, web_hook: hook) }
     let(:params) { { id: hook } }
 
     it_behaves_like 'Web hook destroyer'

spec/controllers/projects/hooks_controller_spec.rb

@@ -29,6 +29,22 @@
       { namespace_id: project.namespace, project_id: project, id: hook.id }
     end
 
+    context 'with an existing token' do
+      hook_params = {
+        token: WebHook::SECRET_MASK,
+        url: "http://example.com"
+      }
+
+      it 'does not change a token' do
+        expect do
+          post :update, params: params.merge({ hook: hook_params })
+        end.not_to change { hook.reload.token }
+
+        expect(response).to have_gitlab_http_status(:found)
+        expect(flash[:alert]).to be_blank
+      end
+    end
+
     it 'adds, updates and deletes URL variables' do
       hook.update!(url_variables: { 'a' => 'bar', 'b' => 'woo' })
 

spec/models/hooks/web_hook_spec.rb

@@ -299,7 +299,7 @@
   end
 
   describe '#executable?' do
-    let(:web_hook) { create(:project_hook, project: project) }
+    let_it_be_with_reload(:web_hook) { create(:project_hook, project: project) }
 
     where(:recent_failures, :not_until, :executable) do
       [
@@ -757,4 +757,14 @@ def run_expectation
       expect { described_class.new.update_last_failure }.not_to raise_error
     end
   end
+
+  describe '#masked_token' do
+    it { expect(hook.masked_token).to be_nil }
+
+    context 'with a token' do
+      let(:hook) { build(:project_hook, :token, project: project) }
+
+      it { expect(hook.masked_token).to eq described_class::SECRET_MASK }
+    end
+  end
 end