From 39717c214d7b8163d4d7c85e40ee4f8430dc4c26 Mon Sep 17 00:00:00 2001 From: wlipa Date: Wed, 23 Sep 2015 10:20:19 -0700 Subject: [PATCH 1/2] Prevent expanded paths from getting into the history cache Currently dependencies are mutated after they're pulled out of the cache. These same dependencies can then be re-stored later ``` cache.set(key, history.unshift(deps).take(limit)) ``` This makes it possible for a project to load the wrong asset. --- lib/sprockets/loader.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/sprockets/loader.rb b/lib/sprockets/loader.rb index fb420dfe6..017b37b8c 100644 --- a/lib/sprockets/loader.rb +++ b/lib/sprockets/loader.rb @@ -306,8 +306,10 @@ def fetch_asset_from_dependency_cache(unloaded, limit = 3) history = cache.get(key) || [] history.each_with_index do |deps, index| - deps.map! { |path| path.start_with?("file-digest://") ? expand_from_root(path) : path } - if asset = yield(deps) + expanded_deps = deps.map do |path| + path.start_with?("file-digest://") ? expand_from_root(path) : path + end + if asset = yield(expanded_deps) cache.set(key, history.rotate!(index)) if index > 0 return asset end From 1f00e02f3494647ba0998d38597f6b62b2d07314 Mon Sep 17 00:00:00 2001 From: schneems Date: Fri, 25 Sep 2015 13:26:13 -0500 Subject: [PATCH 2/2] Test #141 for 3.x This isn't ideal, we're directly inspecting the cache, but I don't know how to cause a second asset to actually use the information in the cache to incorrectly load an asset. If someone could show me how, that would be great and I'll update the test. To understand the failure mode see: #141 (comment) --- test/test_caching.rb | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/test/test_caching.rb b/test/test_caching.rb index 426e1e818..4d06828f7 100644 --- a/test/test_caching.rb +++ b/test/test_caching.rb @@ -427,6 +427,41 @@ def teardown end end + test "history cache is not polluted" do + dependency_file = File.join(fixture_path('asset'), "dependencies", "a.js") + env1 = Sprockets::Environment.new(fixture_path('asset')) do |env| + env.append_path(".") + env.cache = @cache + end + env1['required_assets.js'] + + sandbox dependency_file do + write(dependency_file, "var aa = 2;") + env1['required_assets.js'] + + # We must use private APIs to test this behavior + # https://github.com/rails/sprockets/pull/141 + cache_entries = @cache.send(:find_caches).map do |file, _| + key = file.gsub(/\.cache\z/, ''.freeze).split(@cache_dir).last + result = @cache.get(key) + + if result.is_a?(Array) + result if result.first.is_a?(Set) + else + nil + end + end.compact + + assert cache_entries.any? + + cache_entries.each do |sets| + sets.each do |set| + refute set.any? {|uri| uri.include?(env1.root) }, "Expected entry in cache to not include absolute paths but did: #{set.inspect}" + end + end + end + end + test "no absolute paths are retuned from cache" do env1 = Sprockets::Environment.new(fixture_path('default')) do |env| env.append_path(".")