diff options
| -rw-r--r-- | app/assets/javascripts/clusters/clusters_bundle.js | 16 | ||||
| -rw-r--r-- | app/assets/javascripts/monitoring/components/duplicate_dashboard_form.vue | 3 | ||||
| -rw-r--r-- | app/services/clusters/update_service.rb | 6 | ||||
| -rw-r--r-- | app/views/clusters/clusters/_provider_details_form.html.haml | 14 | ||||
| -rw-r--r-- | changelogs/unreleased/security-do-not-expose-kubernetes-token.yml | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/security-jivanvl-prevent-xss-duplicate-dashboard-modal.yml | 5 | ||||
| -rw-r--r-- | doc/api/projects.md | 2 | ||||
| -rw-r--r-- | locale/gitlab.pot | 18 | ||||
| -rw-r--r-- | spec/features/groups/clusters/user_spec.rb | 2 | ||||
| -rw-r--r-- | spec/features/projects/clusters/user_spec.rb | 2 | ||||
| -rw-r--r-- | spec/frontend/clusters/clusters_bundle_spec.js | 22 | ||||
| -rw-r--r-- | spec/frontend/monitoring/components/duplicate_dashboard_form_spec.js | 36 | ||||
| -rw-r--r-- | spec/services/clusters/update_service_spec.rb | 33 | ||||
| -rw-r--r--[-rwxr-xr-x] | vendor/gitignore/C++.gitignore | 0 | ||||
| -rw-r--r--[-rwxr-xr-x] | vendor/gitignore/Java.gitignore | 0 |
15 files changed, 90 insertions, 74 deletions
diff --git a/app/assets/javascripts/clusters/clusters_bundle.js b/app/assets/javascripts/clusters/clusters_bundle.js index 3699a3b8b2b..d8bfbdb458c 100644 --- a/app/assets/javascripts/clusters/clusters_bundle.js +++ b/app/assets/javascripts/clusters/clusters_bundle.js @@ -108,7 +108,6 @@ export default class Clusters { }); this.installApplication = this.installApplication.bind(this); - this.showToken = this.showToken.bind(this); this.errorContainer = document.querySelector('.js-cluster-error'); this.successContainer = document.querySelector('.js-cluster-success'); @@ -119,7 +118,6 @@ export default class Clusters { ); this.errorReasonContainer = this.errorContainer.querySelector('.js-error-reason'); this.successApplicationContainer = document.querySelector('.js-cluster-application-notice'); - this.showTokenButton = document.querySelector('.js-show-cluster-token'); this.tokenField = document.querySelector('.js-cluster-token'); this.ingressDomainHelpText = document.querySelector('.js-ingress-domain-help-text'); this.ingressDomainSnippet = @@ -258,7 +256,6 @@ export default class Clusters { } addListeners() { - if (this.showTokenButton) this.showTokenButton.addEventListener('click', this.showToken); eventHub.$on('installApplication', this.installApplication); eventHub.$on('updateApplication', data => this.updateApplication(data)); eventHub.$on('saveKnativeDomain', data => this.saveKnativeDomain(data)); @@ -275,7 +272,6 @@ export default class Clusters { } removeListeners() { - if (this.showTokenButton) this.showTokenButton.removeEventListener('click', this.showToken); eventHub.$off('installApplication', this.installApplication); eventHub.$off('updateApplication', this.updateApplication); eventHub.$off('saveKnativeDomain'); @@ -344,18 +340,6 @@ export default class Clusters { } } - showToken() { - const type = this.tokenField.getAttribute('type'); - - if (type === 'password') { - this.tokenField.setAttribute('type', 'text'); - this.showTokenButton.textContent = s__('ClusterIntegration|Hide'); - } else { - this.tokenField.setAttribute('type', 'password'); - this.showTokenButton.textContent = s__('ClusterIntegration|Show'); - } - } - hideAll() { this.errorContainer.classList.add('hidden'); this.successContainer.classList.add('hidden'); diff --git a/app/assets/javascripts/monitoring/components/duplicate_dashboard_form.vue b/app/assets/javascripts/monitoring/components/duplicate_dashboard_form.vue index 58eb8a9df8e..001cd0d47f1 100644 --- a/app/assets/javascripts/monitoring/components/duplicate_dashboard_form.vue +++ b/app/assets/javascripts/monitoring/components/duplicate_dashboard_form.vue @@ -1,6 +1,7 @@ <script> import { __, s__, sprintf } from '~/locale'; import { GlFormGroup, GlFormInput, GlFormRadioGroup, GlFormTextarea } from '@gitlab/ui'; +import { escape as esc } from 'lodash'; const defaultFileName = dashboard => dashboard.path.split('/').reverse()[0]; @@ -42,7 +43,7 @@ export default { html: sprintf( __('Commit to %{branchName} branch'), { - branchName: `<strong>${this.defaultBranch}</strong>`, + branchName: `<strong>${esc(this.defaultBranch)}</strong>`, }, false, ), diff --git a/app/services/clusters/update_service.rb b/app/services/clusters/update_service.rb index 2315df612a1..ba20826848d 100644 --- a/app/services/clusters/update_service.rb +++ b/app/services/clusters/update_service.rb @@ -10,6 +10,12 @@ module Clusters def execute(cluster) if validate_params(cluster) + token = params.dig(:platform_kubernetes_attributes, :token) + + if token.blank? + params[:platform_kubernetes_attributes]&.delete(:token) + end + cluster.update(params) else false diff --git a/app/views/clusters/clusters/_provider_details_form.html.haml b/app/views/clusters/clusters/_provider_details_form.html.haml index dd7d6182e3c..fcb5d4402d6 100644 --- a/app/views/clusters/clusters/_provider_details_form.html.haml +++ b/app/views/clusters/clusters/_provider_details_form.html.haml @@ -25,16 +25,10 @@ label: s_('ClusterIntegration|CA Certificate'), label_class: 'label-bold', input_group_class: 'gl-field-error-anchor', append: copy_ca_cert_btn - - show_token_btn = (platform_field.button s_('ClusterIntegration|Show'), - type: 'button', class: 'js-show-cluster-token btn btn-default') - - copy_token_btn = clipboard_button(text: platform.token, title: s_('ClusterIntegration|Copy Service Token'), - class: 'input-group-text btn-default') if cluster.read_only_kubernetes_platform_fields? - - = platform_field.text_field :token, type: 'password', class: 'js-select-on-focus js-cluster-token', - required: true, title: s_('ClusterIntegration|Service token is required.'), - readonly: cluster.read_only_kubernetes_platform_fields?, - label: s_('ClusterIntegration|Service Token'), label_class: 'label-bold', - input_group_class: 'gl-field-error-anchor', append: show_token_btn + copy_token_btn + = platform_field.password_field :token, type: 'password', class: 'js-select-on-focus js-cluster-token', + readonly: cluster.read_only_kubernetes_platform_fields?, autocomplete: 'new-password', + label: s_('ClusterIntegration|Enter new Service Token'), label_class: 'label-bold', + input_group_class: 'gl-field-error-anchor' = platform_field.form_group :authorization_type do = platform_field.check_box :authorization_type, { disabled: true, label: s_('ClusterIntegration|RBAC-enabled cluster'), diff --git a/changelogs/unreleased/security-do-not-expose-kubernetes-token.yml b/changelogs/unreleased/security-do-not-expose-kubernetes-token.yml new file mode 100644 index 00000000000..9297a4d927e --- /dev/null +++ b/changelogs/unreleased/security-do-not-expose-kubernetes-token.yml @@ -0,0 +1,5 @@ +--- +title: Kubernetes cluster details page no longer exposes Service Token +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-jivanvl-prevent-xss-duplicate-dashboard-modal.yml b/changelogs/unreleased/security-jivanvl-prevent-xss-duplicate-dashboard-modal.yml new file mode 100644 index 00000000000..d4d7b1dbff6 --- /dev/null +++ b/changelogs/unreleased/security-jivanvl-prevent-xss-duplicate-dashboard-modal.yml @@ -0,0 +1,5 @@ +--- +title: Prevent XSS in the monitoring dashboard +merge_request: +author: +type: security diff --git a/doc/api/projects.md b/doc/api/projects.md index 8cb8961bafa..a5bb3f9bebb 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -1198,7 +1198,7 @@ PUT /projects/:id | `approvals_before_merge` | integer | no | **(STARTER)** How many approvers should approve merge request by default | | `external_authorization_classification_label` | string | no | **(PREMIUM)** The classification label for the project | | `mirror` | boolean | no | **(STARTER)** Enables pull mirroring in a project | -| `mirror_user_id` | integer | no | **(STARTER)** User responsible for all the activity surrounding a pull mirror event | +| `mirror_user_id` | integer | no | **(STARTER)** User responsible for all the activity surrounding a pull mirror event. Can only be set by admins. | | `mirror_trigger_builds` | boolean | no | **(STARTER)** Pull mirroring triggers builds | | `only_mirror_protected_branches` | boolean | no | **(STARTER)** Only mirror protected branches | | `mirror_overwrites_diverged_branches` | boolean | no | **(STARTER)** Pull mirror overwrites diverged branches | diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 14afe67bbfd..c7c41e9a5e0 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4622,9 +4622,6 @@ msgstr "" msgid "ClusterIntegration|Copy Kubernetes cluster name" msgstr "" -msgid "ClusterIntegration|Copy Service Token" -msgstr "" - msgid "ClusterIntegration|Could not load IAM roles" msgstr "" @@ -4703,6 +4700,9 @@ msgstr "" msgid "ClusterIntegration|Enabled stack" msgstr "" +msgid "ClusterIntegration|Enter new Service Token" +msgstr "" + msgid "ClusterIntegration|Enter the details for your Amazon EKS Kubernetes cluster" msgstr "" @@ -4787,9 +4787,6 @@ msgstr "" msgid "ClusterIntegration|Helm streamlines installing and managing Kubernetes applications. Tiller runs inside of your Kubernetes Cluster, and manages releases of your charts." msgstr "" -msgid "ClusterIntegration|Hide" -msgstr "" - msgid "ClusterIntegration|If you are setting up multiple clusters and are using Auto DevOps, %{help_link_start}read this first%{help_link_end}." msgstr "" @@ -5183,9 +5180,6 @@ msgstr "" msgid "ClusterIntegration|Set the global mode for the WAF in this cluster. This can be overridden at the environmental level." msgstr "" -msgid "ClusterIntegration|Show" -msgstr "" - msgid "ClusterIntegration|Something went wrong on our end." msgstr "" @@ -22244,9 +22238,6 @@ msgstr "" msgid "This user will be the author of all events in the activity feed that are the result of an update, like new branches being created or new commits being pushed to existing branches." msgstr "" -msgid "This user will be the author of all events in the activity feed that are the result of an update, like new branches being created or new commits being pushed to existing branches. Upon creation or when reassigning you can only assign yourself to be the mirror user." -msgstr "" - msgid "This variable can not be masked." msgstr "" @@ -25014,6 +25005,9 @@ msgstr "" msgid "You will be removed from existing projects/groups" msgstr "" +msgid "You will be the author of all events in the activity feed that are the result of an update, like new branches being created or new commits being pushed to existing branches." +msgstr "" + msgid "You will first need to set up Jira Integration to use this feature." msgstr "" diff --git a/spec/features/groups/clusters/user_spec.rb b/spec/features/groups/clusters/user_spec.rb index e9ef66e31a2..a29afba99e4 100644 --- a/spec/features/groups/clusters/user_spec.rb +++ b/spec/features/groups/clusters/user_spec.rb @@ -39,7 +39,7 @@ describe 'User Cluster', :js do expect(page.find_field('cluster[platform_kubernetes_attributes][api_url]').value) .to have_content('http://example.com') expect(page.find_field('cluster[platform_kubernetes_attributes][token]').value) - .to have_content('my-token') + .to be_empty end end diff --git a/spec/features/projects/clusters/user_spec.rb b/spec/features/projects/clusters/user_spec.rb index 79676927fa2..5c82d848563 100644 --- a/spec/features/projects/clusters/user_spec.rb +++ b/spec/features/projects/clusters/user_spec.rb @@ -46,7 +46,7 @@ describe 'User Cluster', :js do expect(page.find_field('cluster[platform_kubernetes_attributes][api_url]').value) .to have_content('http://example.com') expect(page.find_field('cluster[platform_kubernetes_attributes][token]').value) - .to have_content('my-token') + .to be_empty end it 'user sees RBAC is enabled by default' do diff --git a/spec/frontend/clusters/clusters_bundle_spec.js b/spec/frontend/clusters/clusters_bundle_spec.js index d7c648bcd20..9d0ed423759 100644 --- a/spec/frontend/clusters/clusters_bundle_spec.js +++ b/spec/frontend/clusters/clusters_bundle_spec.js @@ -82,28 +82,6 @@ describe('Clusters', () => { }); }); - describe('showToken', () => { - it('should update token field type', () => { - cluster.showTokenButton.click(); - - expect(cluster.tokenField.getAttribute('type')).toEqual('text'); - - cluster.showTokenButton.click(); - - expect(cluster.tokenField.getAttribute('type')).toEqual('password'); - }); - - it('should update show token button text', () => { - cluster.showTokenButton.click(); - - expect(cluster.showTokenButton.textContent).toEqual('Hide'); - - cluster.showTokenButton.click(); - - expect(cluster.showTokenButton.textContent).toEqual('Show'); - }); - }); - describe('checkForNewInstalls', () => { const INITIAL_APP_MAP = { helm: { status: null, title: 'Helm Tiller' }, diff --git a/spec/frontend/monitoring/components/duplicate_dashboard_form_spec.js b/spec/frontend/monitoring/components/duplicate_dashboard_form_spec.js index 216ec345552..8ab7c8b9e50 100644 --- a/spec/frontend/monitoring/components/duplicate_dashboard_form_spec.js +++ b/spec/frontend/monitoring/components/duplicate_dashboard_form_spec.js @@ -3,9 +3,17 @@ import DuplicateDashboardForm from '~/monitoring/components/duplicate_dashboard_ import { dashboardGitResponse } from '../mock_data'; -describe('DuplicateDashboardForm', () => { - let wrapper; +let wrapper; + +const createMountedWrapper = (props = {}) => { + // Use `mount` to render native input elements + wrapper = mount(DuplicateDashboardForm, { + propsData: { ...props }, + sync: false, + }); +}; +describe('DuplicateDashboardForm', () => { const defaultBranch = 'master'; const findByRef = ref => wrapper.find({ ref }); @@ -20,14 +28,7 @@ describe('DuplicateDashboardForm', () => { }; beforeEach(() => { - // Use `mount` to render native input elements - wrapper = mount(DuplicateDashboardForm, { - propsData: { - dashboard: dashboardGitResponse[0], - defaultBranch, - }, - sync: false, - }); + createMountedWrapper({ dashboard: dashboardGitResponse[0], defaultBranch }); }); it('renders correctly', () => { @@ -146,3 +147,18 @@ describe('DuplicateDashboardForm', () => { }); }); }); + +describe('DuplicateDashboardForm escapes elements', () => { + const branchToEscape = "<img/src='x'onerror=alert(document.domain)>"; + + beforeEach(() => { + createMountedWrapper({ dashboard: dashboardGitResponse[0], defaultBranch: branchToEscape }); + }); + + it('should escape branch name data', () => { + const branchOptionHtml = wrapper.vm.branchOptions[0].html; + const escapedBranch = '<img/src='x'onerror=alert(document.domain)>'; + + expect(branchOptionHtml).toEqual(expect.stringContaining(escapedBranch)); + }); +}); diff --git a/spec/services/clusters/update_service_spec.rb b/spec/services/clusters/update_service_spec.rb index d487edd8850..5a7726eded8 100644 --- a/spec/services/clusters/update_service_spec.rb +++ b/spec/services/clusters/update_service_spec.rb @@ -47,6 +47,39 @@ describe Clusters::UpdateService do expect(cluster.platform.namespace).to eq('custom-namespace') end end + + context 'when service token is empty' do + let(:params) do + { + platform_kubernetes_attributes: { + token: '' + } + } + end + + it 'does not update the token' do + current_token = cluster.platform.token + is_expected.to eq(true) + cluster.platform.reload + + expect(cluster.platform.token).to eq(current_token) + end + end + + context 'when service token is not empty' do + let(:params) do + { + platform_kubernetes_attributes: { + token: 'new secret token' + } + } + end + + it 'updates the token' do + is_expected.to eq(true) + expect(cluster.platform.token).to eq('new secret token') + end + end end context 'when invalid params' do diff --git a/vendor/gitignore/C++.gitignore b/vendor/gitignore/C++.gitignore index 259148fa18f..259148fa18f 100755..100644 --- a/vendor/gitignore/C++.gitignore +++ b/vendor/gitignore/C++.gitignore diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore index a1c2a238a96..a1c2a238a96 100755..100644 --- a/vendor/gitignore/Java.gitignore +++ b/vendor/gitignore/Java.gitignore |