From ec79f312323b127a50254ed1afb30a5a20207751 Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Fri, 12 May 2017 12:42:05 +0000 Subject: [PATCH 1/2] Adds rails stylesheet and javascript tags back This will support work for adding SRI to our templates. This will let us add an 'integrity' attribute to both the stylesheet_link_tag and the javascript_include_tag. What this does It adds the ability to understand and convert stylesheet_link_tag and javascript_include_tag to html for the other languages in which our erb template is being compiled to (e.g. Django, Mustache, Liquid, Jinja) --- build_tools/compiler/template_processor.rb | 64 ++++++++++++++++ source/views/layouts/govuk_template.html.erb | 8 +- .../compiler/django_processor_spec.rb | 1 + .../compiler/ejs_processor_spec.rb | 1 + .../compiler/jinja_processor_spec.rb | 1 + .../compiler/liquid_processor_spec.rb | 11 +++ .../mustache_inheritance_processor_spec.rb | 11 +++ .../compiler/mustache_processor_spec.rb | 3 +- .../compiler/plain_processor_spec.rb | 11 +++ .../compiler/play_processor_spec.rb | 2 + spec/support/examples/processor.rb | 76 +++++++++++++++++++ 11 files changed, 183 insertions(+), 6 deletions(-) create mode 100644 spec/build_tools/compiler/liquid_processor_spec.rb create mode 100644 spec/build_tools/compiler/mustache_inheritance_processor_spec.rb create mode 100644 spec/build_tools/compiler/plain_processor_spec.rb create mode 100644 spec/support/examples/processor.rb diff --git a/build_tools/compiler/template_processor.rb b/build_tools/compiler/template_processor.rb index f6fd3e89..965a081f 100644 --- a/build_tools/compiler/template_processor.rb +++ b/build_tools/compiler/template_processor.rb @@ -39,5 +39,69 @@ def asset_path(file, options={}) def method_missing(name, *args) puts "#{name} #{args.inspect}" end + + def stylesheet_link_tag(*sources) + options = stringify_keys(extract_options!(sources)) + 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 = stringify_keys(extract_options!(sources)) + sources.uniq.map { |source| + script_options = { + "src" => asset_path(source) + }.merge!(options) + content_tag(:script, tag_options(script_options)) + }.join("\n") + end + + def content_tag(name, options = nil) + "<#{name}#{options}>" + end + + 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) + 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/source/views/layouts/govuk_template.html.erb b/source/views/layouts/govuk_template.html.erb index f3a7124d..677ecfe0 100644 --- a/source/views/layouts/govuk_template.html.erb +++ b/source/views/layouts/govuk_template.html.erb @@ -6,15 +6,15 @@ <%= content_for?(:page_title) ? yield(:page_title) : "GOV.UK - The best place to find government services and information" %> - " media="screen" rel="stylesheet" /> + <%= stylesheet_link_tag "govuk-template.css" %> - " media="print" rel="stylesheet" /> + <%= stylesheet_link_tag "govuk-template-print.css", media: "print" %> - " media="all" rel="stylesheet" /> - + <%= stylesheet_link_tag "fonts.css", media: "all" %> + <%# 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 35fee585..912c289f 100644 --- a/spec/build_tools/compiler/django_processor_spec.rb +++ b/spec/build_tools/compiler/django_processor_spec.rb @@ -26,6 +26,7 @@ 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| diff --git a/spec/build_tools/compiler/ejs_processor_spec.rb b/spec/build_tools/compiler/ejs_processor_spec.rb index dcbeebd7..f4b935c2 100644 --- a/spec/build_tools/compiler/ejs_processor_spec.rb +++ b/spec/build_tools/compiler/ejs_processor_spec.rb @@ -23,6 +23,7 @@ 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| diff --git a/spec/build_tools/compiler/jinja_processor_spec.rb b/spec/build_tools/compiler/jinja_processor_spec.rb index 5ee7b97c..b73c8290 100644 --- a/spec/build_tools/compiler/jinja_processor_spec.rb +++ b/spec/build_tools/compiler/jinja_processor_spec.rb @@ -27,6 +27,7 @@ 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| diff --git a/spec/build_tools/compiler/liquid_processor_spec.rb b/spec/build_tools/compiler/liquid_processor_spec.rb new file mode 100644 index 00000000..ca3ffbef --- /dev/null +++ b/spec/build_tools/compiler/liquid_processor_spec.rb @@ -0,0 +1,11 @@ +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 new file mode 100644 index 00000000..57ebd37b --- /dev/null +++ b/spec/build_tools/compiler/mustache_inheritance_processor_spec.rb @@ -0,0 +1,11 @@ +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 4cff2a2d..ff34746e 100644 --- a/spec/build_tools/compiler/mustache_processor_spec.rb +++ b/spec/build_tools/compiler/mustache_processor_spec.rb @@ -19,10 +19,10 @@ def valid_sections end describe Compiler::MustacheProcessor do - 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| @@ -64,5 +64,4 @@ def valid_sections end end end - end diff --git a/spec/build_tools/compiler/plain_processor_spec.rb b/spec/build_tools/compiler/plain_processor_spec.rb new file mode 100644 index 00000000..c8860cc9 --- /dev/null +++ b/spec/build_tools/compiler/plain_processor_spec.rb @@ -0,0 +1,11 @@ +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 448820f8..1657d524 100644 --- a/spec/build_tools/compiler/play_processor_spec.rb +++ b/spec/build_tools/compiler/play_processor_spec.rb @@ -34,6 +34,8 @@ 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 new file mode 100644 index 00000000..6673152d --- /dev/null +++ b/spec/support/examples/processor.rb @@ -0,0 +1,76 @@ +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"} } + + 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 + expect(processor.stylesheet_link_tag(css_source, css_options)).to eql("") + end + end + end + + describe "#javascript_include_tag" do + let(:js_options) { {"charset" => "UTF-8"} } + + it "should parse the javascript tag" do + expect(processor.javascript_include_tag(js_source)).to eql("") + end + + it "should parse the javascript tag" do + expect(processor.javascript_include_tag(js_source, js_options)).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 "#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)} } + + it "should parse the hash" do + expect(processor.tag_options(options)).to eql(" rel=\"stylesheet\" media=\"screen\" href=\"#{processor.asset_path(css_source)}\"") + end + end + + end +end From 4d58b1a6c1e64b514ad92e1726fe46a5dc2ed013 Mon Sep 17 00:00:00 2001 From: Elena Tanasoiu Date: Fri, 12 May 2017 14:29:17 +0000 Subject: [PATCH 2/2] 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" /> --- CHANGELOG.md | 4 ++ build_tools/compiler/template_processor.rb | 30 ++++--------- docs/using-with-rails.md | 17 ++++++++ source/views/layouts/govuk_template.html.erb | 9 ++-- spec/support/examples/processor.rb | 46 +++++++++++--------- spec/support/uses_of_yield.rb | 6 +++ 6 files changed, 65 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6f69537..269f483b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# Development + +- 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. + # 0.20.1 - Fix invalid html from Apple touch icons syntax ([PR #300]https://github.com/alphagov/govuk_template/pull/300) and update icon sizes to match Apple specs diff --git a/build_tools/compiler/template_processor.rb b/build_tools/compiler/template_processor.rb index 965a081f..f8026811 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 = exclude_sri_fields(sources.extract_options!) 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 = exclude_sri_fields(sources.extract_options!) sources.uniq.map { |source| script_options = { "src" => asset_path(source) @@ -62,6 +64,10 @@ def javascript_include_tag(*sources) }.join("\n") end + def exclude_sri_fields(options) + options.stringify_keys.except("integrity", "crossorigin") + end + def content_tag(name, options = nil) "<#{name}#{options}>" end @@ -70,26 +76,6 @@ 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) return if options.empty? output = "".dup diff --git a/docs/using-with-rails.md b/docs/using-with-rails.md index b28fed4a..513236d4 100644 --- a/docs/using-with-rails.md +++ b/docs/using-with-rails.md @@ -38,3 +38,20 @@ 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 677ecfe0..b155d57b 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..5fd64bb5 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(: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 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(: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" 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 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 @@ -48,27 +60,19 @@ 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"} } + describe "#tag_options" do + let(:options) { {"rel"=>"stylesheet", "media"=>"screen", "href"=>processor.asset_path(css_source) } } - it "should turn keys of a hash into strings" do - expect(processor.stringify_keys(options)).to eql({"media"=>"print"}) + 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 "#tag_options" do - let(:options) { {"rel"=>"stylesheet", "media"=>"screen", "href"=>processor.asset_path(css_source)} } + describe "#exclude_sri_fields" do + 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)}\"") + 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 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