Potential cache key collision in Gitlab::Cache::Helpers#contextual_cache_key
Summary
Some of our cache keys are generated in a way that can be prone to collisions, because the class name is being extracted incorrectly.
Steps to reproduce
I don't have a full reproducer (yet), but was able to observe the effect in production. Likely happening in API.
As part of the analysis in gitlab-com/gl-infra/scalability#2023 (closed), we captured traffic to redis-cache. We see calls that look like this:
mget
cache:gitlab:Class:tag:702f5f52d2242ba84d00c5c34b00650dc1b1e9ce:projects/17884550-20221121151723507958
cache:gitlab:Class:tag:8bc747cfa2b4c15b04558d0ae26d5977f19e1601:projects/17884550-20221121151723507958
cache:gitlab:Class:tag:af84cff6178ec5ea97fcdd863a1cd5c3624da212:projects/17884550-20221121151723507958
...Notice the "Class" here.
I believe this "Class" comes from contextual_cache_key. This is sometimes (maybe always) getting called with a class as an argument as opposed to what it expects: an instance.
An example call site is here. It passes Entities::Branch, not Entities::Branch.new. There's a handful of similar cases in lib/api.
Example Project
n/a
What is the current bug behavior?
The cache key is not namespaced properly by entity, it simply gets the prefix "Class". If two differing entities were to be cached with the same context cache keys, the cache may return an unexpected one. This could lead to further data leaks further down the line.
As the key includes cache keys of other related objects, it's unlikely that such a collision will occur in practice. As such we may chose to declassify this and treat it as a bug instead.
What is the expected correct behavior?
Cache keys for entities should be namespaced properly, prefixed by the class name of the entity.
Relevant logs and/or screenshots
n/a
Output of checks
This bug happens on GitLab.com
Results of GitLab environment info
n/a
Results of GitLab application Check
n/a
Possible fixes
Something along these lines should do the trick.
diff --git a/lib/gitlab/cache/helpers.rb b/lib/gitlab/cache/helpers.rb
index 024fa48c066e..17b5167d8237 100644
--- a/lib/gitlab/cache/helpers.rb
+++ b/lib/gitlab/cache/helpers.rb
@@ -45,6 +45,8 @@ def render_cached(obj_or_collection, with:, cache_context: -> (_) { current_user
def contextual_cache_key(presenter, object, context)
return object.cache_key if context.nil?
+ presenter = presenter.class == Class ? presenter.name : presenter.class.name
+
[presenter.class.name, object.cache_key, context.call(object)].flatten.join(":")
end