Skip to content

Commit

Permalink
Fix storing absolute path in cache (again)
Browse files Browse the repository at this point in the history
Introduced in #441 the `to_load` and `to_link` sets are used to store assets that will be loaded or linked. At the time of implementation it was thought that the key was removed in the Bundle processor before being stored in the cache. Unfortunately this is not the case as Sprockets pipelines are confusing and the Bundle processor gets called in the “self” pipeline which does not get called until after the “default” pipeline produces an asset that is stored to the cache.

This PR fixes this issue by forcing the path into a relative path before being put in the cache, and then expanding from relative path after reading from cache.

I also added a test that should catch if __any__ absolute values make their way into the cache, not just in the keys that we specify.
  • Loading branch information
schneems committed Mar 9, 2018
1 parent 9f5656f commit dac2199
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
13 changes: 12 additions & 1 deletion lib/sprockets/loader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ def asset_from_cache(key)
asset[:metadata][:links].map! { |uri| expand_from_root(uri) } if asset[:metadata][:links]
asset[:metadata][:stubbed].map! { |uri| expand_from_root(uri) } if asset[:metadata][:stubbed]
asset[:metadata][:required].map! { |uri| expand_from_root(uri) } if asset[:metadata][:required]
asset[:metadata][:to_load].map! { |uri| expand_from_root(uri) } if asset[:metadata][:to_load]
asset[:metadata][:to_link].map! { |uri| expand_from_root(uri) } if asset[:metadata][:to_link]
asset[:metadata][:dependencies].map! { |uri| uri.start_with?("file-digest://") ? expand_from_root(uri) : uri } if asset[:metadata][:dependencies]

asset[:metadata].each_key do |k|
Expand Down Expand Up @@ -140,7 +142,6 @@ def load_from_unloaded(unloaded)

# Read into memory and process if theres a processor pipeline
if processors.any?

result = call_processors(processors, {
environment: self,
cache: self.cache,
Expand Down Expand Up @@ -224,6 +225,16 @@ def store_asset(asset, unloaded)
cached_asset[:metadata][:required].map! { |uri| compress_from_root(uri) }
end

if cached_asset[:metadata][:to_load] && !cached_asset[:metadata][:to_load].empty?
cached_asset[:metadata][:to_load] = cached_asset[:metadata][:to_load].dup
cached_asset[:metadata][:to_load].map! { |uri| compress_from_root(uri) }
end

if cached_asset[:metadata][:to_link] && !cached_asset[:metadata][:to_link].empty?
cached_asset[:metadata][:to_link] = cached_asset[:metadata][:to_link].dup
cached_asset[:metadata][:to_link].map! { |uri| compress_from_root(uri) }
end

if cached_asset[:metadata][:dependencies] && !cached_asset[:metadata][:dependencies].empty?
cached_asset[:metadata][:dependencies] = cached_asset[:metadata][:dependencies].dup
cached_asset[:metadata][:dependencies].map! do |uri|
Expand Down
15 changes: 15 additions & 0 deletions test/test_caching.rb
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,21 @@ def teardown
end
end

test "no absolute paths are stored in the cache by accident" do
environment = Sprockets::Environment.new(fixture_path('default')) do |env|
env.append_path(".")
env.cache = @cache
end
cache = environment.cache
def cache.set(key, value, local = false)
if value.to_s.match?(Dir.pwd)
raise "Expected '#{value}' to not contain absolute path '#{Dir.pwd}' but did"
end
end

environment['schneems.js']
end

test "no absolute paths are retuned from cache" do
env1 = Sprockets::Environment.new(fixture_path('default')) do |env|
env.append_path(".")
Expand Down

0 comments on commit dac2199

Please sign in to comment.