From b5450eeccbc85b7ef9f540d437671035dab8372e Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Fri, 12 May 2017 14:29:17 +0000 Subject: [PATCH] Adds SRI integrity attribute to template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit How to test ----------- In the application using govuk_template, you will have to set `config.assets.debug = false` in config/environments/development.rb or run the application in production mode. This is because sprockets-rails checks for this flag when deciding if it should calculate the integrity attribute: https://github.com/rails/sprockets-rails/blob/10bc1bd096b39a3dd632571dd517788314657056/lib/sprockets/rails/helper.rb#L168 What is SRI ----------- SRI will add an integrity attribute on your script tags: The example above is generated automatically by sprockets-rails in your project if you do this: <%= stylesheet_script_tag 'example', integrity: true %> The way the digest value for the integrity attribute is calculated is by applying SHA256 on the contents of the file. In that way, the browser can double check that the right file is being received and hasn’t been tampered with (which would mean the contents would have changed and the resulting digest would be incorrect). The problem with using this in govuk_template --------------------------------------------- The css files it provided by govuk_template (for example: to the static project), are still css.erb files. In which case the contents of those files will change when they are processed in static (specifically there’s multiple <%= asset_path(...)%>s in there). So here’s the problem illustrated: 1. The govuk_template gem compiles a file called govuk_template.css.erb which contains multiple asset_path lines, e.g. background-image: url(<%= asset_path 'images/govuk-crest.png' %>); 2. static receives this file and replaces all the asset_path lines with it’s own path, e.g. background-image: url(**http://static.dev.gov.uk/static**/images/govuk-crest-2x.png); Notice that this url will vary depending on the app that uses the gem => this means the contents of the resulting govuk_template.css will vary depending on which app is using the gem so the digest will also vary. This means you can’t calculate the digest inside the gem, and will have to be left to the apps to do it individually 3. You cannot use sprockets-rails in govuk_template directly because the assets it serves are also compiled into django, play, liquid, mustache and other crazy formats. So you can’t do this: <%= stylesheet_link_tag ‘govuk-template-print’, integrity: true%> and then leave it up to sprockets-rails to calculate the digest for you, because this will not work for django and all the other formats. They don’t know what stylesheet_tag is. Instead, this is what govuk_template does so it can be compiled into multiple languages: " media="print" rel="stylesheet" /> 4. You cannot calculate the integrity digest by hand in the gem, because of point 2 -> the content will change Proposed solution ----------------- We use stylesheet_link_tag and set integrity: true, but we leave it up to the apps using the gem to actually calculate the integrity digest. For the other languages (django, jijna, liquid, mustache) we have defined a stylesheet_include_tag method in their respective processors that will translate that tag into something they can understand: " media="print" rel="stylesheet" /> --- build_tools/compiler/template_processor.rb | 27 +++---------- govuk_template.gemspec | 2 +- source/views/layouts/govuk_template.html.erb | 9 +++-- spec/support/examples/processor.rb | 40 +++++++++----------- spec/support/uses_of_yield.rb | 6 +++ 5 files changed, 35 insertions(+), 49 deletions(-) diff --git a/build_tools/compiler/template_processor.rb b/build_tools/compiler/template_processor.rb index 965a081f..1f301aa8 100644 --- a/build_tools/compiler/template_processor.rb +++ b/build_tools/compiler/template_processor.rb @@ -1,4 +1,6 @@ require 'erb' +require 'active_support/core_ext/hash' +require 'active_support/core_ext/array' module Compiler class TemplateProcessor @@ -41,7 +43,7 @@ def method_missing(name, *args) end def stylesheet_link_tag(*sources) - options = stringify_keys(extract_options!(sources)) + options = sources.extract_options!.stringify_keys sources.uniq.map { |source| link_options = { "rel" => "stylesheet", @@ -53,7 +55,7 @@ def stylesheet_link_tag(*sources) end def javascript_include_tag(*sources) - options = stringify_keys(extract_options!(sources)) + options = sources.extract_options!.stringify_keys sources.uniq.map { |source| script_options = { "src" => asset_path(source) @@ -70,27 +72,8 @@ def tag(name, options) "<#{name}#{options}/>" end - def extract_options!(options) - if options.last.is_a?(Hash) && extractable_options?(options.last) - options.pop - else - {} - end - end - - def extractable_options?(options) - options.instance_of?(Hash) - end - - def stringify_keys(options) - result = {} - options.each_key do |key| - result[key.to_s] = options[key] - end - result - end - def tag_options(options) + options = options.except("integrity", "crossorigin") return if options.empty? output = "".dup sep = " " diff --git a/govuk_template.gemspec b/govuk_template.gemspec index 1682507f..a847da24 100644 --- a/govuk_template.gemspec +++ b/govuk_template.gemspec @@ -28,5 +28,5 @@ Gem::Specification.new do |spec| spec.add_development_dependency 'mustache', '0.99.7' spec.add_development_dependency 'nokogiri', '1.6.6.2' spec.add_development_dependency 'octokit', '3.4.2' - spec.add_development_dependency 'pry' + spec.add_development_dependency 'pry-byebug' end diff --git a/source/views/layouts/govuk_template.html.erb b/source/views/layouts/govuk_template.html.erb index 51c660b6..22a0a56c 100644 --- a/source/views/layouts/govuk_template.html.erb +++ b/source/views/layouts/govuk_template.html.erb @@ -6,15 +6,16 @@ <%= content_for?(:page_title) ? yield(:page_title) : "GOV.UK - The best place to find government services and information" %> - <%= stylesheet_link_tag "govuk-template.css" %> + <%= stylesheet_link_tag "govuk-template.css", integrity: true, crossorigin: "anonymous" %> - <%= stylesheet_link_tag "govuk-template-print.css", media: "print" %> + <%= stylesheet_link_tag "govuk-template-print.css", media: "print", integrity: true, crossorigin: "anonymous" %> - <%= stylesheet_link_tag "fonts.css", media: "all" %> - + <%= stylesheet_link_tag "fonts.css", media: "all", integrity: true, crossorigin: "anonymous" %> + <%= stylesheet_link_tag "fonts.css", media: "all", integrity: true, crossorigin: "anonymous" %> + <%# the colour used for mask-icon is the standard palette $black from diff --git a/spec/support/examples/processor.rb b/spec/support/examples/processor.rb index 6673152d..8cbb6284 100644 --- a/spec/support/examples/processor.rb +++ b/spec/support/examples/processor.rb @@ -2,7 +2,7 @@ require 'set' shared_examples_for "a processor" do - let(:html_erb_file) {"a/file.css"} + let(:html_erb_file) { "a/file.css" } let(:processor) { described_class.new(html_erb_file) } describe "convert rails tags into html" do @@ -11,29 +11,41 @@ let(:js_source) { "ie.js" } describe "#stylesheet_link_tag" do - let(:css_options) { {"media" => "print"} } + let(:css_options) { {"media" => "print"} } + let(:integrity_attribute) { {"integrity" => "true"} } 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 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 the integrity attribute is present, it should ignore it" do + it "should parse the stylesheet tag without the integrity attribute" do + expect(processor.stylesheet_link_tag(css_source, integrity_attribute)).to eql("") + end + end end describe "#javascript_include_tag" do - let(:js_options) { {"charset" => "UTF-8"} } + let(:js_options) { {"charset" => "UTF-8"} } + let(:integrity_attribute) { {"integrity" => "true"} } it "should parse the javascript tag" do expect(processor.javascript_include_tag(js_source)).to eql("") end - it "should parse the javascript tag" do + it "should parse the javascript tag and extra options" do expect(processor.javascript_include_tag(js_source, js_options)).to eql("") end + + it "if the integrity attribute is present, it should ignore it" do + expect(processor.javascript_include_tag(js_source, integrity_attribute)).to eql("") + end end describe "#content_tag" do @@ -48,24 +60,8 @@ end end - describe "#extract_options!" do - let(:options) { ["govuk-template.css", {"media" => "print"}]} - - it "should extract the last part of the options" do - expect(processor.extract_options!(options)).to eql({"media" => "print"}) - end - end - - describe "#stringify_keys" do - let(:options) { {:media => "print"} } - - it "should turn keys of a hash into strings" do - expect(processor.stringify_keys(options)).to eql({"media"=>"print"}) - end - end - describe "#tag_options" do - let(:options) { {"rel"=>"stylesheet", "media"=>"screen", "href"=>processor.asset_path(css_source)} } + let(:options) { {"rel"=>"stylesheet", "media"=>"screen", "href"=>processor.asset_path(css_source), "integrity" => true, "crossorigin" => "anonymous" } } it "should parse the hash" do expect(processor.tag_options(options)).to eql(" rel=\"stylesheet\" media=\"screen\" href=\"#{processor.asset_path(css_source)}\"") diff --git a/spec/support/uses_of_yield.rb b/spec/support/uses_of_yield.rb index 56ec5a98..3d3df4f8 100644 --- a/spec/support/uses_of_yield.rb +++ b/spec/support/uses_of_yield.rb @@ -32,6 +32,12 @@ 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