From 32343d78b162f9b6dea6107670d5be7142e838e1 Mon Sep 17 00:00:00 2001 From: Murray Steele Date: Mon, 22 May 2017 10:50:58 +0100 Subject: [PATCH] Revert #301 Add SRI This is a manual revert, as the PR could not be reverted automatically. We're reverting this PR because there is a bug in the SRI implementation of Firefox versions upto 52 which at time of writing accounts for 0.7% of total traffic (~315k users). We still want to implement SRI, but for now we're holding off until we'd impact fewer users. --- CHANGELOG.md | 4 + build_tools/compiler/template_processor.rb | 51 ------------ docs/using-with-rails.md | 17 ---- source/views/layouts/govuk_template.html.erb | 9 +-- .../compiler/django_processor_spec.rb | 2 - .../compiler/ejs_processor_spec.rb | 2 - .../compiler/jinja_processor_spec.rb | 2 - .../compiler/liquid_processor_spec.rb | 11 --- .../mustache_inheritance_processor_spec.rb | 11 --- .../compiler/mustache_processor_spec.rb | 2 - .../compiler/plain_processor_spec.rb | 11 --- .../compiler/play_processor_spec.rb | 2 - spec/support/examples/processor.rb | 80 ------------------- spec/support/uses_of_yield.rb | 6 -- 14 files changed, 8 insertions(+), 202 deletions(-) delete mode 100644 spec/build_tools/compiler/liquid_processor_spec.rb delete mode 100644 spec/build_tools/compiler/mustache_inheritance_processor_spec.rb delete mode 100644 spec/build_tools/compiler/plain_processor_spec.rb delete mode 100644 spec/support/examples/processor.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a23bca1..9865bb2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# Development + +- Revert SRI to avoid breaking the site for Firefox users on versions less than 52 [PR #308](https://github.com/alphagov/govuk_template/pull/301) + # 0.21.0 - Adds SRI to js and css assets ([PR #301](https://github.com/alphagov/govuk_template/pull/301)). This requires `sprockets-rails` >= 3.0 in the projects using this gem. diff --git a/build_tools/compiler/template_processor.rb b/build_tools/compiler/template_processor.rb index f8026811..43bee6e0 100644 --- a/build_tools/compiler/template_processor.rb +++ b/build_tools/compiler/template_processor.rb @@ -1,10 +1,7 @@ require 'erb' -require 'active_support/core_ext/hash' -require 'active_support/core_ext/array' module Compiler class TemplateProcessor - def initialize(file) @file = file @is_stylesheet = !!(file =~ /\.css\.erb\z/) @@ -41,53 +38,5 @@ def asset_path(file, options={}) def method_missing(name, *args) puts "#{name} #{args.inspect}" end - - def stylesheet_link_tag(*sources) - options = exclude_sri_fields(sources.extract_options!) - sources.uniq.map { |source| - link_options = { - "rel" => "stylesheet", - "media" => "screen", - "href" => asset_path(source) - }.merge!(options) - tag(:link, tag_options(link_options)) - }.join("\n") - end - - def javascript_include_tag(*sources) - options = exclude_sri_fields(sources.extract_options!) - sources.uniq.map { |source| - script_options = { - "src" => asset_path(source) - }.merge!(options) - content_tag(:script, tag_options(script_options)) - }.join("\n") - end - - def exclude_sri_fields(options) - options.stringify_keys.except("integrity", "crossorigin") - end - - def content_tag(name, options = nil) - "<#{name}#{options}>" - end - - def tag(name, options) - "<#{name}#{options}/>" - end - - def tag_options(options) - return if options.empty? - output = "".dup - sep = " " - options.each_pair do |key, value| - if !value.nil? - output << sep - output << %(#{key}="#{value}") - end - end - output unless output.empty? - end - end end diff --git a/docs/using-with-rails.md b/docs/using-with-rails.md index 513236d4..b28fed4a 100644 --- a/docs/using-with-rails.md +++ b/docs/using-with-rails.md @@ -38,20 +38,3 @@ Or to add content to ``, for stylesheets or similar: ``` Check out the [full list of blocks](template-blocks.md) you can use to customise the template. - -## SRI - -`govuk_template` >= 20.0.0 can be used together with `sprockets-rails` >= 3.0.0 in order to make use of the SRI - -You can read more about SRI [here](https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity). - -SRI will add an `integrity` attribute on your script tags: - -`` - -The example above is generated automatically by sprockets-rails in your project if the integrity option is set to true: - -`<%= stylesheet_script_tag 'example', integrity: true %>` - diff --git a/source/views/layouts/govuk_template.html.erb b/source/views/layouts/govuk_template.html.erb index b155d57b..f3a7124d 100644 --- a/source/views/layouts/govuk_template.html.erb +++ b/source/views/layouts/govuk_template.html.erb @@ -6,16 +6,15 @@ <%= content_for?(:page_title) ? yield(:page_title) : "GOV.UK - The best place to find government services and information" %> - <%= stylesheet_link_tag "govuk-template.css", integrity: true, crossorigin: "anonymous" %> + " media="screen" rel="stylesheet" /> - <%= stylesheet_link_tag "govuk-template-print.css", media: "print", integrity: true, crossorigin: "anonymous" %> + " media="print" rel="stylesheet" /> - <%= stylesheet_link_tag "fonts.css", media: "all", integrity: true, crossorigin: "anonymous" %> - <%= stylesheet_link_tag "fonts.css", media: "all", integrity: true, crossorigin: "anonymous" %> - + " media="all" rel="stylesheet" /> + <%# the colour used for mask-icon is the standard palette $black from diff --git a/spec/build_tools/compiler/django_processor_spec.rb b/spec/build_tools/compiler/django_processor_spec.rb index 912c289f..48ce74d5 100644 --- a/spec/build_tools/compiler/django_processor_spec.rb +++ b/spec/build_tools/compiler/django_processor_spec.rb @@ -26,8 +26,6 @@ def valid_sections let(:file) {"some/file.erb"} subject {described_class.new(file)} - it_behaves_like "a processor" - describe "#handle_yield" do valid_sections.each do |key, content| it "should render #{content} for #{key}" do diff --git a/spec/build_tools/compiler/ejs_processor_spec.rb b/spec/build_tools/compiler/ejs_processor_spec.rb index f4b935c2..bc780d73 100644 --- a/spec/build_tools/compiler/ejs_processor_spec.rb +++ b/spec/build_tools/compiler/ejs_processor_spec.rb @@ -23,8 +23,6 @@ def valid_sections let(:file) {"some/file.erb"} subject {described_class.new(file)} - it_behaves_like "a processor" - describe "#handle_yield" do valid_sections.each do |key, content| it "should render #{content} for #{key}" do diff --git a/spec/build_tools/compiler/jinja_processor_spec.rb b/spec/build_tools/compiler/jinja_processor_spec.rb index b73c8290..2ffa5582 100644 --- a/spec/build_tools/compiler/jinja_processor_spec.rb +++ b/spec/build_tools/compiler/jinja_processor_spec.rb @@ -27,8 +27,6 @@ def valid_sections let(:file) {"some/file.erb"} subject {described_class.new(file)} - it_behaves_like "a processor" - describe "#handle_yield" do valid_sections.each do |key, content| it "should render #{content} for #{key}" do diff --git a/spec/build_tools/compiler/liquid_processor_spec.rb b/spec/build_tools/compiler/liquid_processor_spec.rb deleted file mode 100644 index ca3ffbef..00000000 --- a/spec/build_tools/compiler/liquid_processor_spec.rb +++ /dev/null @@ -1,11 +0,0 @@ -require 'spec_helper' -require File.join(PROJECT_ROOT, 'build_tools/compiler/liquid_processor.rb') - -describe Compiler::LiquidProcessor do - - let(:file) {"some/file.erb"} - subject {described_class.new(file)} - - it_behaves_like "a processor" - -end diff --git a/spec/build_tools/compiler/mustache_inheritance_processor_spec.rb b/spec/build_tools/compiler/mustache_inheritance_processor_spec.rb deleted file mode 100644 index 57ebd37b..00000000 --- a/spec/build_tools/compiler/mustache_inheritance_processor_spec.rb +++ /dev/null @@ -1,11 +0,0 @@ -require 'spec_helper' -require File.join(PROJECT_ROOT, 'build_tools/compiler/mustache_inheritance_processor.rb') - -describe Compiler::MustacheInheritanceProcessor do - - let(:file) {"some/file.erb"} - subject {described_class.new(file)} - - it_behaves_like "a processor" - -end diff --git a/spec/build_tools/compiler/mustache_processor_spec.rb b/spec/build_tools/compiler/mustache_processor_spec.rb index ff34746e..a59a6a35 100644 --- a/spec/build_tools/compiler/mustache_processor_spec.rb +++ b/spec/build_tools/compiler/mustache_processor_spec.rb @@ -22,8 +22,6 @@ def valid_sections let(:file) {"some/file.erb"} subject {described_class.new(file)} - it_behaves_like "a processor" - describe "#handle_yield" do valid_sections.each do |key, content| it "should render #{content} for #{key}" do diff --git a/spec/build_tools/compiler/plain_processor_spec.rb b/spec/build_tools/compiler/plain_processor_spec.rb deleted file mode 100644 index c8860cc9..00000000 --- a/spec/build_tools/compiler/plain_processor_spec.rb +++ /dev/null @@ -1,11 +0,0 @@ -require 'spec_helper' -require File.join(PROJECT_ROOT, 'build_tools/compiler/plain_processor.rb') - -describe Compiler::PlainProcessor do - - let(:file) {"some/file.erb"} - subject {described_class.new(file)} - - it_behaves_like "a processor" - -end diff --git a/spec/build_tools/compiler/play_processor_spec.rb b/spec/build_tools/compiler/play_processor_spec.rb index 1657d524..448820f8 100644 --- a/spec/build_tools/compiler/play_processor_spec.rb +++ b/spec/build_tools/compiler/play_processor_spec.rb @@ -34,8 +34,6 @@ def expected_parameter_names describe Compiler::PlayProcessor do subject { described_class.new("dummy filename") } - it_behaves_like "a processor" - describe "top_of_page" do it "declares all of the template parameters" do expected_parameter_names.each do |parameter_name| diff --git a/spec/support/examples/processor.rb b/spec/support/examples/processor.rb deleted file mode 100644 index 5fd64bb5..00000000 --- a/spec/support/examples/processor.rb +++ /dev/null @@ -1,80 +0,0 @@ -require 'spec_helper' -require 'set' - -shared_examples_for "a processor" do - let(:html_erb_file) { "a/file.css" } - let(:processor) { described_class.new(html_erb_file) } - - describe "convert rails tags into html" do - - let(:css_source) { "govuk-template.css" } - let(:js_source) { "ie.js" } - - describe "#stylesheet_link_tag" do - let(:css_options) { {"media" => "print"} } - let(:sri_attributes) { {"integrity" => true, "crossorigin" => "anonymous"} } - - it "should parse the stylesheet tag" do - expect(processor.stylesheet_link_tag(css_source)).to eql("") - end - - context "if css file is for print" do - it "should parse the stylesheet tag and extra options" do - expect(processor.stylesheet_link_tag(css_source, css_options)).to eql("") - end - end - - context "if sri attributes are present, it should ignore them" do - it "should parse the stylesheet tag without the integrity attribute" do - expect(processor.stylesheet_link_tag(css_source, sri_attributes)).to eql("") - end - end - end - - describe "#javascript_include_tag" do - let(:js_options) { {"charset" => "UTF-8"} } - let(:sri_attributes) { {"integrity" => true, "crossorigin" => "anonymous"} } - - it "should parse the javascript tag" do - expect(processor.javascript_include_tag(js_source)).to eql("") - end - - it "should parse the javascript tag and extra options" do - expect(processor.javascript_include_tag(js_source, js_options)).to eql("") - end - - it "if sri attributes are present, it should ignore them" do - expect(processor.javascript_include_tag(js_source, sri_attributes)).to eql("") - end - end - - describe "#content_tag" do - it "should return the correct html script tag" do - expect(processor.content_tag(:script, " src=\"#{processor.asset_path(js_source)}\"")).to eql("") - end - end - - describe "#tag" do - it "should return the correct html link tag" do - expect(processor.tag(:link, " rel=\"stylesheet\" media=\"screen\" href=\"#{processor.asset_path(css_source)}\"")).to eql("") - end - end - - describe "#tag_options" do - let(:options) { {"rel"=>"stylesheet", "media"=>"screen", "href"=>processor.asset_path(css_source) } } - - it "flattens the hash into a string of quoted html attributes" do - expect(processor.tag_options(options)).to eql(" rel=\"stylesheet\" media=\"screen\" href=\"#{processor.asset_path(css_source)}\"") - end - end - - describe "#exclude_sri_fields" do - let(:options) { {"rel"=>"stylesheet", "media"=>"screen", "href"=>processor.asset_path(css_source), "integrity" => true, "crossorigin" => "anonymous" } } - - it "should remove the integrity and crossorigin keys from the hash" do - expect(processor.exclude_sri_fields(options)).to eql({"href" => processor.asset_path(css_source), "media" => "screen", "rel" => "stylesheet"}) - end - end - - end -end diff --git a/spec/support/uses_of_yield.rb b/spec/support/uses_of_yield.rb index 3d3df4f8..56ec5a98 100644 --- a/spec/support/uses_of_yield.rb +++ b/spec/support/uses_of_yield.rb @@ -32,12 +32,6 @@ def asset_path(*args) def method_missing(name, *args) puts "#{name} #{args.inspect}" end - - def stylesheet_link_tag(*sources) - end - - def javascript_include_tag(*sources) - end end # return an array of unique values passed to yield in the templates