From 79945d291b26e72da05c0084930a60b5daeaf750 Mon Sep 17 00:00:00 2001 From: dcouture Date: Mon, 23 Nov 2020 12:03:54 -0500 Subject: [PATCH 1/4] Force a nonce on all script tags This leverages an existing helper we had to set defer attributes everywhere and adds nonce to all script tags regardless of if the script is inline or external. It also removes all the now-redundant nonce: true used in various places. --- app/helpers/defer_script_tag_helper.rb | 16 +++++++- app/views/layouts/_google_analytics.html.haml | 2 +- .../_google_tag_manager_head.html.haml | 2 +- app/views/layouts/_img_loader.html.haml | 2 +- .../layouts/_init_auto_complete.html.haml | 2 +- .../_init_client_detection_flags.html.haml | 2 +- app/views/layouts/_snowplow.html.haml | 2 +- .../layouts/_startup_css_activation.haml | 2 +- app/views/layouts/_startup_js.html.haml | 2 +- app/views/layouts/errors.html.haml | 2 +- app/views/layouts/group.html.haml | 2 +- app/views/layouts/project.html.haml | 2 +- app/views/layouts/snippets.html.haml | 2 +- .../projects/merge_requests/_widget.html.haml | 2 +- .../projects/merge_requests/show.html.haml | 2 +- spec/helpers/defer_script_tag_helper_spec.rb | 38 +++++++++++++++++-- 16 files changed, 63 insertions(+), 19 deletions(-) diff --git a/app/helpers/defer_script_tag_helper.rb b/app/helpers/defer_script_tag_helper.rb index be927c67aaa551..7a5ae0a6cc17d0 100644 --- a/app/helpers/defer_script_tag_helper.rb +++ b/app/helpers/defer_script_tag_helper.rb @@ -4,7 +4,21 @@ module DeferScriptTagHelper # Override the default ActionView `javascript_include_tag` helper to support page specific deferred loading. # PLEASE NOTE: `defer` is also critical so that we don't run JavaScript entrypoints before the DOM is ready. # Please see https://gitlab.com/groups/gitlab-org/-/epics/4538#note_432159769. + # The helper also makes sure the `nonce` attribute is included in every script when the content security + # policy is enabled. def javascript_include_tag(*sources) - super(*sources, defer: true) + super(*sources, defer: true, nonce: true) + end + + # The helper makes sure the `nonce` attribute is included in every script when the content security + # policy is enabled. + def javascript_tag(content_or_options_with_block = nil, html_options = {}) + if content_or_options_with_block.is_a?(Hash) + content_or_options_with_block[:nonce] = true + else + html_options[:nonce] = true + end + + super end end diff --git a/app/views/layouts/_google_analytics.html.haml b/app/views/layouts/_google_analytics.html.haml index e8a5359e79126c..759e9ef36b9449 100644 --- a/app/views/layouts/_google_analytics.html.haml +++ b/app/views/layouts/_google_analytics.html.haml @@ -1,4 +1,4 @@ -= javascript_tag nonce: true do += javascript_tag do :plain var _gaq = _gaq || []; _gaq.push(['_setAccount', '#{extra_config.google_analytics_id}']); diff --git a/app/views/layouts/_google_tag_manager_head.html.haml b/app/views/layouts/_google_tag_manager_head.html.haml index ab03f1e76704a5..48eb9e40cc49a6 100644 --- a/app/views/layouts/_google_tag_manager_head.html.haml +++ b/app/views/layouts/_google_tag_manager_head.html.haml @@ -1,5 +1,5 @@ - if google_tag_manager_enabled? - = javascript_tag nonce: true do + = javascript_tag do :plain (function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start': new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0], diff --git a/app/views/layouts/_img_loader.html.haml b/app/views/layouts/_img_loader.html.haml index cddcd6e0af6ee0..f6d7d163e6f9bc 100644 --- a/app/views/layouts/_img_loader.html.haml +++ b/app/views/layouts/_img_loader.html.haml @@ -1,4 +1,4 @@ -= javascript_tag nonce: true do += javascript_tag do :plain if ('loading' in HTMLImageElement.prototype) { document.querySelectorAll('img.lazy').forEach(img => { diff --git a/app/views/layouts/_init_auto_complete.html.haml b/app/views/layouts/_init_auto_complete.html.haml index 82ec92988eb4fc..509f5be809787d 100644 --- a/app/views/layouts/_init_auto_complete.html.haml +++ b/app/views/layouts/_init_auto_complete.html.haml @@ -4,7 +4,7 @@ - datasources = autocomplete_data_sources(object, noteable_type) - if object - = javascript_tag nonce: true do + = javascript_tag do :plain gl = window.gl || {}; gl.GfmAutoComplete = gl.GfmAutoComplete || {}; diff --git a/app/views/layouts/_init_client_detection_flags.html.haml b/app/views/layouts/_init_client_detection_flags.html.haml index 6537b86085fb6a..03967bbbfcf91f 100644 --- a/app/views/layouts/_init_client_detection_flags.html.haml +++ b/app/views/layouts/_init_client_detection_flags.html.haml @@ -1,7 +1,7 @@ - client = client_js_flags - if client - = javascript_tag nonce: true do + = javascript_tag do :plain gl = window.gl || {}; gl.client = #{client.to_json}; diff --git a/app/views/layouts/_snowplow.html.haml b/app/views/layouts/_snowplow.html.haml index d7ff5ad1094119..9d14dfb378605a 100644 --- a/app/views/layouts/_snowplow.html.haml +++ b/app/views/layouts/_snowplow.html.haml @@ -1,6 +1,6 @@ - return unless Gitlab::CurrentSettings.snowplow_enabled? -= javascript_tag nonce: true do += javascript_tag do :plain ;(function(p,l,o,w,i,n,g){if(!p[i]){p.GlobalSnowplowNamespace=p.GlobalSnowplowNamespace||[]; p.GlobalSnowplowNamespace.push(i);p[i]=function(){(p[i].q=p[i].q||[]).push(arguments) diff --git a/app/views/layouts/_startup_css_activation.haml b/app/views/layouts/_startup_css_activation.haml index a426d686c34633..5fb53385acc9b7 100644 --- a/app/views/layouts/_startup_css_activation.haml +++ b/app/views/layouts/_startup_css_activation.haml @@ -1,6 +1,6 @@ - return unless use_startup_css? -= javascript_tag nonce: true do += javascript_tag do :plain document.querySelectorAll('link[media="print"]').forEach(linkTag => { linkTag.setAttribute('data-startupcss', 'loading'); diff --git a/app/views/layouts/_startup_js.html.haml b/app/views/layouts/_startup_js.html.haml index 9c488e4f40d0a1..35cd191c600407 100644 --- a/app/views/layouts/_startup_js.html.haml +++ b/app/views/layouts/_startup_js.html.haml @@ -1,6 +1,6 @@ - return unless page_startup_api_calls.present? || page_startup_graphql_calls.present? -= javascript_tag nonce: true do += javascript_tag do :plain var gl = window.gl || {}; gl.startup_calls = #{page_startup_api_calls.to_json}; diff --git a/app/views/layouts/errors.html.haml b/app/views/layouts/errors.html.haml index dc924a0e25dc25..25fe4c898caa9b 100644 --- a/app/views/layouts/errors.html.haml +++ b/app/views/layouts/errors.html.haml @@ -8,7 +8,7 @@ %body .page-container = yield - = javascript_tag nonce: true do + = javascript_tag do :plain (function(){ var goBackElement = document.querySelector('.js-go-back'); diff --git a/app/views/layouts/group.html.haml b/app/views/layouts/group.html.haml index 6d2c5870e4367e..58fed89dfe768b 100644 --- a/app/views/layouts/group.html.haml +++ b/app/views/layouts/group.html.haml @@ -8,7 +8,7 @@ - content_for :page_specific_javascripts do - if current_user - = javascript_tag nonce: true do + = javascript_tag do :plain window.uploads_path = "#{group_uploads_path(@group)}"; diff --git a/app/views/layouts/project.html.haml b/app/views/layouts/project.html.haml index 62e5431e2902e6..2df502d2899153 100644 --- a/app/views/layouts/project.html.haml +++ b/app/views/layouts/project.html.haml @@ -10,7 +10,7 @@ - content_for :project_javascripts do - project = @target_project || @project - if current_user - = javascript_tag nonce: true do + = javascript_tag do :plain window.uploads_path = "#{project_uploads_path(project)}"; diff --git a/app/views/layouts/snippets.html.haml b/app/views/layouts/snippets.html.haml index 6cc53ba3342a7e..54b5ec85cccff7 100644 --- a/app/views/layouts/snippets.html.haml +++ b/app/views/layouts/snippets.html.haml @@ -4,7 +4,7 @@ - content_for :page_specific_javascripts do - if snippets_upload_path - = javascript_tag nonce: true do + = javascript_tag do :plain window.uploads_path = "#{snippets_upload_path}"; diff --git a/app/views/projects/merge_requests/_widget.html.haml b/app/views/projects/merge_requests/_widget.html.haml index 9736071b03f342..123affeb5d694b 100644 --- a/app/views/projects/merge_requests/_widget.html.haml +++ b/app/views/projects/merge_requests/_widget.html.haml @@ -1,4 +1,4 @@ -= javascript_tag nonce: true do += javascript_tag do :plain window.gl = window.gl || {}; window.gl.mrWidgetData = #{serialize_issuable(@merge_request, serializer: 'widget', issues_links: true)} diff --git a/ee/app/views/projects/merge_requests/show.html.haml b/ee/app/views/projects/merge_requests/show.html.haml index 54683c2282d711..a1e8a9f45f6a12 100644 --- a/ee/app/views/projects/merge_requests/show.html.haml +++ b/ee/app/views/projects/merge_requests/show.html.haml @@ -1,6 +1,6 @@ = render_ce "projects/merge_requests/show" -= javascript_tag nonce: true do += javascript_tag do :plain // Append static, server-generated data not included in merge request entity (EE-Only) // Object.assign would be useful here, but it blows up Phantom.js in tests diff --git a/spec/helpers/defer_script_tag_helper_spec.rb b/spec/helpers/defer_script_tag_helper_spec.rb index 14317e353ab5fe..f5d8ecfca835ee 100644 --- a/spec/helpers/defer_script_tag_helper_spec.rb +++ b/spec/helpers/defer_script_tag_helper_spec.rb @@ -3,12 +3,42 @@ require 'spec_helper' RSpec.describe DeferScriptTagHelper do - describe 'script tag' do - script_url = 'test.js' + before do + allow_any_instance_of(DeferScriptTagHelper).to receive(:content_security_policy_nonce).and_return('noncevalue') + end + + describe 'external script tag' do + let(:script_url) { 'test.js' } - it 'returns an script tag with defer=true' do + it 'returns a script tag with defer=true and a nonce' do expect(javascript_include_tag(script_url).to_s) - .to eq "" + .to eq "" + end + end + + describe 'inline script tag' do + let(:tag_with_nonce) {""} + let(:tag_with_nonce_and_type) {""} + + it 'returns a script tag with a nonce using block syntax' do + expect(javascript_tag { 'alert(1)' }.to_s).to eq tag_with_nonce + end + + it 'returns a script tag with a nonce using block syntax with options' do + expect(javascript_tag(type: 'application/javascript') { 'alert(1)' }.to_s).to eq tag_with_nonce_and_type + end + + it 'returns a script tag with a nonce using argument syntax' do + expect(javascript_tag('alert(1)').to_s).to eq tag_with_nonce + end + + it 'returns a script tag with a nonce using argument syntax with options' do + expect(javascript_tag( 'alert(1)', type: 'application/javascript').to_s).to eq tag_with_nonce_and_type + end + + # This scenario does not really make sense, but it's supported so we test it + it 'returns a script tag with a nonce using argument and block syntax with options' do + expect(javascript_tag( '// ignored', type: 'application/javascript') { 'alert(1)' }.to_s).to eq tag_with_nonce_and_type end end end -- GitLab From 30ffdabf49a7d712085c1882247e925c9be5aca5 Mon Sep 17 00:00:00 2001 From: dcouture Date: Mon, 23 Nov 2020 12:07:15 -0500 Subject: [PATCH 2/4] Rename helper to reflect its new role It handles more than the `defer` attribute now so it's renamed to GitlabScriptTagHelper --- ...defer_script_tag_helper.rb => gitlab_script_tag_helper.rb} | 2 +- ...pt_tag_helper_spec.rb => gitlab_script_tag_helper_spec.rb} | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) rename app/helpers/{defer_script_tag_helper.rb => gitlab_script_tag_helper.rb} (96%) rename spec/helpers/{defer_script_tag_helper_spec.rb => gitlab_script_tag_helper_spec.rb} (91%) diff --git a/app/helpers/defer_script_tag_helper.rb b/app/helpers/gitlab_script_tag_helper.rb similarity index 96% rename from app/helpers/defer_script_tag_helper.rb rename to app/helpers/gitlab_script_tag_helper.rb index 7a5ae0a6cc17d0..467f3f7305b44b 100644 --- a/app/helpers/defer_script_tag_helper.rb +++ b/app/helpers/gitlab_script_tag_helper.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -module DeferScriptTagHelper +module GitlabScriptTagHelper # Override the default ActionView `javascript_include_tag` helper to support page specific deferred loading. # PLEASE NOTE: `defer` is also critical so that we don't run JavaScript entrypoints before the DOM is ready. # Please see https://gitlab.com/groups/gitlab-org/-/epics/4538#note_432159769. diff --git a/spec/helpers/defer_script_tag_helper_spec.rb b/spec/helpers/gitlab_script_tag_helper_spec.rb similarity index 91% rename from spec/helpers/defer_script_tag_helper_spec.rb rename to spec/helpers/gitlab_script_tag_helper_spec.rb index f5d8ecfca835ee..b5e0073fb11a14 100644 --- a/spec/helpers/defer_script_tag_helper_spec.rb +++ b/spec/helpers/gitlab_script_tag_helper_spec.rb @@ -2,9 +2,9 @@ require 'spec_helper' -RSpec.describe DeferScriptTagHelper do +RSpec.describe GitlabScriptTagHelper do before do - allow_any_instance_of(DeferScriptTagHelper).to receive(:content_security_policy_nonce).and_return('noncevalue') + allow_any_instance_of(GitlabScriptTagHelper).to receive(:content_security_policy_nonce).and_return('noncevalue') end describe 'external script tag' do -- GitLab From c3c05892f74f4570b811f3582d823779a66ffb1c Mon Sep 17 00:00:00 2001 From: dcouture Date: Tue, 24 Nov 2020 13:04:23 -0500 Subject: [PATCH 3/4] Remove usage of allow_any_instance_of --- spec/helpers/gitlab_script_tag_helper_spec.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/spec/helpers/gitlab_script_tag_helper_spec.rb b/spec/helpers/gitlab_script_tag_helper_spec.rb index b5e0073fb11a14..37413b9b1c273a 100644 --- a/spec/helpers/gitlab_script_tag_helper_spec.rb +++ b/spec/helpers/gitlab_script_tag_helper_spec.rb @@ -4,14 +4,14 @@ RSpec.describe GitlabScriptTagHelper do before do - allow_any_instance_of(GitlabScriptTagHelper).to receive(:content_security_policy_nonce).and_return('noncevalue') + allow(helper).to receive(:content_security_policy_nonce).and_return('noncevalue') end describe 'external script tag' do let(:script_url) { 'test.js' } it 'returns a script tag with defer=true and a nonce' do - expect(javascript_include_tag(script_url).to_s) + expect(helper.javascript_include_tag(script_url).to_s) .to eq "" end end @@ -21,24 +21,24 @@ let(:tag_with_nonce_and_type) {""} it 'returns a script tag with a nonce using block syntax' do - expect(javascript_tag { 'alert(1)' }.to_s).to eq tag_with_nonce + expect(helper.javascript_tag { 'alert(1)' }.to_s).to eq tag_with_nonce end it 'returns a script tag with a nonce using block syntax with options' do - expect(javascript_tag(type: 'application/javascript') { 'alert(1)' }.to_s).to eq tag_with_nonce_and_type + expect(helper.javascript_tag(type: 'application/javascript') { 'alert(1)' }.to_s).to eq tag_with_nonce_and_type end it 'returns a script tag with a nonce using argument syntax' do - expect(javascript_tag('alert(1)').to_s).to eq tag_with_nonce + expect(helper.javascript_tag('alert(1)').to_s).to eq tag_with_nonce end it 'returns a script tag with a nonce using argument syntax with options' do - expect(javascript_tag( 'alert(1)', type: 'application/javascript').to_s).to eq tag_with_nonce_and_type + expect(helper.javascript_tag( 'alert(1)', type: 'application/javascript').to_s).to eq tag_with_nonce_and_type end # This scenario does not really make sense, but it's supported so we test it it 'returns a script tag with a nonce using argument and block syntax with options' do - expect(javascript_tag( '// ignored', type: 'application/javascript') { 'alert(1)' }.to_s).to eq tag_with_nonce_and_type + expect(helper.javascript_tag( '// ignored', type: 'application/javascript') { 'alert(1)' }.to_s).to eq tag_with_nonce_and_type end end end -- GitLab From d5ad516a9a3bc94b50fa41ac2b35c18f8cdeb141 Mon Sep 17 00:00:00 2001 From: dcouture Date: Thu, 26 Nov 2020 10:29:45 -0500 Subject: [PATCH 4/4] Remove redundant nonce: true --- app/views/layouts/_matomo.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/layouts/_matomo.html.haml b/app/views/layouts/_matomo.html.haml index 1297d120683473..fcd3156a1628b8 100644 --- a/app/views/layouts/_matomo.html.haml +++ b/app/views/layouts/_matomo.html.haml @@ -1,5 +1,5 @@ -= javascript_tag nonce: true do += javascript_tag do :plain var _paq = window._paq = window._paq || []; _paq.push(['trackPageView']); -- GitLab