request_store broken for ActiveJob

[henkesn]

Hi guys, at first, thank you for your great work! I noticed today that I get outdated/wrong data within Mailers/ActiveJob. When I had a look at the stack traces, I noticed that there seems to be missing some kind of middleware which inits RequestLocals. For Sidekick there is a second gem to handle this. As ActiveJob is core feature of rails, perhaps an equivalent should be included in this gem. I (and propably some more people) am using RequestLocals this way:

RequestLocals.store[:some_stuff] ||= calculate_some_stuff

That's why I think it would be good to raise an error at least when using uninitialized RequestLocals, cause this can lead to severe data leaks regarding multi-user/multi-tenant systems.

[ggambetti]

Just ran into this issue with Mailers/ActiveJob.

It was specifically an interaction when using active_job.queue_adapter = :async and there never being a RequestStore.clear! call around when jobs were run. This leads to infinitely caching data.

In follow up discussion we found RequestStore.active? (from https://github.com/steveklabnik/request_store/pull/49) and had the thought that RequestStore.fetch should take it into account instead of always caching data. This would be a breaking change (since it changes RequestStore.fetch behaviour), but would make it safer to call code using RequestStore in contexts where no wrapping middleware is setup.

ex:

class RequestStore
  def self.fetch(key)
    # No caching occurs when caching is not activated by a wrapping block.
    return yield unless active?
    store[key] = yield unless exist?(key)
    store[key]
  end
end

# The wrapping middleware:
def with_caching(&block)
  RequestStore.begin!
  yield
ensure
  RequestStore.end!
  RequestStore.clear!
end

This way, application code can be written with less concern for the calling context and the caller will get the expected caching behaviour. This makes naive use of RequestStore.fetch safer compared to the current situation.

An example:

class SettingsSingleton
  def self.instance
    RequestStore.fetch(:settings_singleton) do
      # load from database or create as needed.
    end
  end
end

# Somewhere in the app we do something with settings.
# 
# If foo is called anywhere without middleware that will call
# RequestStore.clear! the instance of SettingsSingleton will be cached
# forever, and any content changes will be ignored in every subsequent
# request. However with the new RequestStore.fetch behaviour this is
# safe to call from anywhere, because caching is disabled by default.
def foo
  settings = SettingsSingleton.instance
end