Merge pull request #276 from jeremy/fix-stuck-assets-in-dev-due-to-preference-for-stale-manifest

Fix stuck assets in dev due to a stale precompiled manifest

Author: jeremy

lib/sprockets/rails/helper.rb

@@ -73,21 +73,13 @@ def compute_asset_path(path, options = {})
       #
       # Returns String path or nil if no asset was found.
       def asset_digest_path(path, options = {})
-        if manifest = assets_manifest
-          if digest_path = manifest.assets[path]
-            return digest_path
-          end
-        end
-
-        if environment = assets_environment
-          if asset = environment[path]
-            unless options[:debug]
-              if !precompiled_asset_checker.call(asset.logical_path)
-                raise AssetNotPrecompiled.new(asset.logical_path)
-              end
-            end
-            return asset.digest_path
+        if assets_environment
+          if asset = assets_environment[path]
+            raise_unless_precompiled_asset asset.logical_path unless options[:debug]
+            asset.digest_path
           end
+        else
+          assets_manifest.assets[path]
         end
       end
 
@@ -98,42 +90,25 @@ def asset_digest_path(path, options = {})
       #
       # Returns String integrity attribute or nil if no asset was found.
       def asset_integrity(path, options = {})
-        path = path.to_s
-        if extname = compute_asset_extname(path, options)
-          path = "#{path}#{extname}"
-        end
+        path = path_with_extname(path, options)
 
-        if manifest = assets_manifest
-          if digest_path = manifest.assets[path]
-            if metadata = manifest.files[digest_path]
-              return metadata["integrity"]
-            end
+        if assets_environment
+          if asset = assets_environment[path]
+            asset.integrity
           end
-        end
-
-        if environment = assets_environment
-          if asset = environment[path]
-            return asset.integrity
+        elsif digest_path = assets_manifest.assets[path]
+          if metadata = assets_manifest.files[digest_path]
+            metadata["integrity"]
           end
         end
-
-        nil
       end
 
       # Override javascript tag helper to provide debugging support.
       #
       # Eventually will be deprecated and replaced by source maps.
       def javascript_include_tag(*sources)
         options = sources.extract_options!.stringify_keys
-
-        unless request_ssl?
-          options.delete("integrity")
-        end
-
-        case options["integrity"]
-        when true, false, nil
-          compute_integrity = options.delete("integrity")
-        end
+        integrity = compute_integrity?(options)
 
         if options["debug"] != false && request_debug_assets?
           sources.map { |source|
@@ -151,9 +126,8 @@ def javascript_include_tag(*sources)
           }.flatten.uniq.join("\n").html_safe
         else
           sources.map { |source|
-            super(source, compute_integrity ?
-              options.merge("integrity" => asset_integrity(source, :type => :javascript)) :
-              options)
+            options = options.merge('integrity' => asset_integrity(source, :type => :javascript)) if integrity
+            super source, options
           }.join("\n").html_safe
         end
       end
@@ -163,15 +137,7 @@ def javascript_include_tag(*sources)
       # Eventually will be deprecated and replaced by source maps.
       def stylesheet_link_tag(*sources)
         options = sources.extract_options!.stringify_keys
-
-        unless request_ssl?
-          options.delete("integrity")
-        end
-
-        case options["integrity"]
-        when true, false, nil
-          compute_integrity = options.delete("integrity")
-        end
+        integrity = compute_integrity?(options)
 
         if options["debug"] != false && request_debug_assets?
           sources.map { |source|
@@ -189,43 +155,58 @@ def stylesheet_link_tag(*sources)
           }.flatten.uniq.join("\n").html_safe
         else
           sources.map { |source|
-            super(source, compute_integrity ?
-              options.merge("integrity" => asset_integrity(source, :type => :stylesheet)) :
-              options)
+            options = options.merge('integrity' => asset_integrity(source, :type => :stylesheet)) if integrity
+            super source, options
           }.join("\n").html_safe
         end
       end
 
       protected
-        def request_ssl?
+        def compute_integrity?(options)
+          if secure_subresource_integrity_context?
+            case options['integrity']
+            when nil, false, true
+              options.delete('integrity') == true
+            end
+          else
+            options.delete 'integrity'
+            false
+          end
+        end
+
+        # Only serve integrity metadata for HTTPS requests:
+        #   http://www.w3.org/TR/SRI/#non-secure-contexts-remain-non-secure
+        def secure_subresource_integrity_context?
           respond_to?(:request) && self.request && self.request.ssl?
         end
 
         # Enable split asset debugging. Eventually will be deprecated
         # and replaced by source maps in Sprockets 3.x.
         def request_debug_assets?
           debug_assets || (defined?(controller) && controller && params[:debug_assets])
-        rescue
-          return false
+        rescue # FIXME: what exactly are we rescuing?
+          false
         end
 
         # Internal method to support multifile debugging. Will
         # eventually be removed w/ Sprockets 3.x.
         def lookup_debug_asset(path, options = {})
-          return unless env = assets_environment
-          path = path.to_s
-          if extname = compute_asset_extname(path, options)
-            path = "#{path}#{extname}"
+          if assets_environment && asset = assets_environment[path_with_extname(path, options), pipeline: :debug]
+            raise_unless_precompiled_asset asset.logical_path.sub('.debug', '')
+            asset
           end
+        end
 
-          if asset = env[path, pipeline: :debug]
-            original_path = asset.logical_path.gsub('.debug', '')
-            unless precompiled_asset_checker.call(original_path)
-              raise AssetNotPrecompiled.new(original_path)
-            end
+        def raise_unless_precompiled_asset(logical_path)
+          if !precompiled_asset_checker.call(logical_path)
+            raise AssetNotPrecompiled.new(logical_path)
           end
+        end
 
-          asset
+        # compute_asset_extname is in AV::Helpers::AssetUrlHelper
+        def path_with_extname(path, options)
+          path = path.to_s
+          "#{path}#{compute_asset_extname(path, options)}"
         end
     end
   end

test/test_helper.rb

@@ -34,6 +34,9 @@ def setup
     @assets.context_class.assets_prefix = @view.assets_prefix
     @assets.context_class.config        = @view.config
 
+    @foo_js_integrity  = @assets['foo.js'].integrity
+    @foo_css_integrity = @assets['foo.css'].integrity
+
     @foo_js_digest  = @assets['foo.js'].etag
     @foo_css_digest = @assets['foo.css'].etag
     @bar_js_digest  = @assets['bar.js'].etag
@@ -391,14 +394,14 @@ def test_javascript_include_tag_integrity
     assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js"></script>),
       @view.javascript_include_tag("foo", integrity: nil)
 
-    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="sha256-TvVUHzSfftWg1rcfL6TIJ0XKEGrgLyEq6lEpcmrG9qs="></script>),
+    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="#{@foo_js_integrity}"></script>),
       @view.javascript_include_tag("foo", integrity: true)
-    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="sha256-TvVUHzSfftWg1rcfL6TIJ0XKEGrgLyEq6lEpcmrG9qs="></script>),
+    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="#{@foo_js_integrity}"></script>),
       @view.javascript_include_tag("foo.js", integrity: true)
-    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="sha256-TvVUHzSfftWg1rcfL6TIJ0XKEGrgLyEq6lEpcmrG9qs="></script>),
+    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="#{@foo_js_integrity}"></script>),
       @view.javascript_include_tag(:foo, integrity: true)
 
-    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="sha256-TvVUHzSfftWg1rcfL6TIJ0XKEGrgLyEq6lEpcmrG9qs="></script>\n<script src="/assets/bar-#{@bar_js_digest}.js" integrity="sha256-g0JYFeYSYGXe376R0JrRzS6CpYpC1HiqtwBsVt/XAWU="></script>),
+    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="#{@foo_js_integrity}"></script>\n<script src="/assets/bar-#{@bar_js_digest}.js" integrity="sha256-g0JYFeYSYGXe376R0JrRzS6CpYpC1HiqtwBsVt/XAWU="></script>),
       @view.javascript_include_tag(:foo, :bar, integrity: true)
   end
 
@@ -410,14 +413,14 @@ def test_stylesheet_link_tag_integrity
     assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" />),
       @view.stylesheet_link_tag("foo", integrity: nil)
 
-    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="sha256-5YzTQPuOJz/EpeXfN/+v1sxsjAj/dw8q26abiHZM3A4=" />),
+    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="#{@foo_css_integrity}" />),
       @view.stylesheet_link_tag("foo", integrity: true)
-    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="sha256-5YzTQPuOJz/EpeXfN/+v1sxsjAj/dw8q26abiHZM3A4=" />),
+    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="#{@foo_css_integrity}" />),
       @view.stylesheet_link_tag("foo.css", integrity: true)
-    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="sha256-5YzTQPuOJz/EpeXfN/+v1sxsjAj/dw8q26abiHZM3A4=" />),
+    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="#{@foo_css_integrity}" />),
       @view.stylesheet_link_tag(:foo, integrity: true)
 
-    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="sha256-5YzTQPuOJz/EpeXfN/+v1sxsjAj/dw8q26abiHZM3A4=" />\n<link href="/assets/bar-#{@bar_css_digest}.css" media="screen" rel="stylesheet" integrity="sha256-Vd370+VAW4D96CVpZcjFLXyeHoagI0VHwofmzRXetuE=" />),
+    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="#{@foo_css_integrity}" />\n<link href="/assets/bar-#{@bar_css_digest}.css" media="screen" rel="stylesheet" integrity="sha256-Vd370+VAW4D96CVpZcjFLXyeHoagI0VHwofmzRXetuE=" />),
       @view.stylesheet_link_tag(:foo, :bar, integrity: true)
   end
 
@@ -636,12 +639,8 @@ def setup
     @manifest.assets["foo.js"] = "foo-#{@foo_js_digest}.js"
     @manifest.assets["foo.css"] = "foo-#{@foo_css_digest}.css"
 
-    @manifest.files["foo-#{@foo_js_digest}.js"] = {
-      "integrity" => "sha-256-TvVUHzSfftWg1rcfL6TIJ0XKEGrgLyEq6lEpcmrG9qs="
-    }
-    @manifest.files["foo-#{@foo_css_digest}.css"] = {
-      "integrity" => "sha-256-5YzTQPuOJz/EpeXfN/+v1sxsjAj/dw8q26abiHZM3A4="
-    }
+    @manifest.files["foo-#{@foo_js_digest}.js"] = { "integrity" => @foo_js_integrity }
+    @manifest.files["foo-#{@foo_css_digest}.css"] = { "integrity" => @foo_css_integrity }
 
     @view.digest_assets = true
     @view.assets_environment = nil
@@ -673,22 +672,22 @@ def test_stylesheet_link_tag
   def test_javascript_include_tag_integrity
     super
 
-    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="sha-256-TvVUHzSfftWg1rcfL6TIJ0XKEGrgLyEq6lEpcmrG9qs="></script>),
+    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="#{@foo_js_integrity}"></script>),
       @view.javascript_include_tag("foo", integrity: true)
-    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="sha-256-TvVUHzSfftWg1rcfL6TIJ0XKEGrgLyEq6lEpcmrG9qs="></script>),
+    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="#{@foo_js_integrity}"></script>),
       @view.javascript_include_tag("foo.js", integrity: true)
-    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="sha-256-TvVUHzSfftWg1rcfL6TIJ0XKEGrgLyEq6lEpcmrG9qs="></script>),
+    assert_dom_equal %(<script src="/assets/foo-#{@foo_js_digest}.js" integrity="#{@foo_js_integrity}"></script>),
       @view.javascript_include_tag(:foo, integrity: true)
   end
 
   def test_stylesheet_link_tag_integrity
     super
 
-    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="sha-256-5YzTQPuOJz/EpeXfN/+v1sxsjAj/dw8q26abiHZM3A4=" />),
+    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="#{@foo_css_integrity}" />),
       @view.stylesheet_link_tag("foo", integrity: true)
-    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="sha-256-5YzTQPuOJz/EpeXfN/+v1sxsjAj/dw8q26abiHZM3A4=" />),
+    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="#{@foo_css_integrity}" />),
       @view.stylesheet_link_tag("foo.css", integrity: true)
-    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="sha-256-5YzTQPuOJz/EpeXfN/+v1sxsjAj/dw8q26abiHZM3A4=" />),
+    assert_dom_equal %(<link href="/assets/foo-#{@foo_css_digest}.css" media="screen" rel="stylesheet" integrity="#{@foo_css_integrity}" />),
       @view.stylesheet_link_tag(:foo, integrity: true)
   end
 
@@ -728,6 +727,34 @@ def test_stylesheet_link_tag_integrity
   end
 end
 
+class StaleManifestVsEnvironmentHelperTest < HelperTest
+  def setup
+    super
+
+    @view.digest_assets = true
+
+    @view.assets_manifest = Sprockets::Manifest.new(@assets, FIXTURES_PATH).tap do |stale|
+      stale.assets["foo.js"] = "foo-stale.js"
+      @manifest.files["foo-stale.js"] = { "integrity" => "stale-manifest" }
+      @manifest.files["foo-#{@foo_js_digest}.js"] = { "integrity" => "current-manifest" }
+
+      stale.assets["foo.css"] = "foo-stale.css"
+      @manifest.files["foo-stale.css"] = { "integrity" => "stale-manifest" }
+      @manifest.files["foo-#{@foo_css_digest}.css"] = { "integrity" => "current-manifest" }
+    end
+  end
+
+  def test_digest_prefers_asset_environment_over_manifest
+    assert_equal "foo-#{@foo_js_digest}.js", @view.asset_digest_path("foo.js")
+    assert_equal "foo-#{@foo_css_digest}.css", @view.asset_digest_path("foo.css")
+  end
+
+  def test_digest_prefers_asset_environment_over_manifest
+    assert_equal @foo_js_integrity, @view.asset_integrity("foo.js")
+    assert_equal @foo_css_integrity, @view.asset_integrity("foo.css")
+  end
+end
+
 class AssetUrlHelperLinksTarget < HelperTest
   def test_precompile_allows_links
     @view.assets_precompile = ["url.css"]