Default cache keys based on JPATH_CONFIGURATION instead of null

[okonomiyaki3000]

Summary of Changes

Set the default _application property of CacheStorage to md5(JPATH_CONFIGURATION) instead of null. Of course, the user may still specify any value he wishes using the cache options array.

The property is used in generating (supposedly) unique keys for cached values. This is particularly important when you have multiple sites sharing a single cache location (I'm looking at you, APC). If they are all using null here, they may have some collisions and will write to and read from each others' cached files. There's no sensible reason you'd ever want that to happen.

We therefore need some value here which is unique per site. JPATH_CONFIGURATION is the best option available. You might serve multiple sites from the same JPATH_ROOT but you wouldn't serve them with the same JPATH_CONFIGURATION.

Testing Instructions

Have a few different sites. Set them all up to use caching. Preferably, APC. Hit them so they start caching things.

Expected result

Each site should read and write to only his own cached items.

Actual result

After the patch, I reckon the result is as expected. Before...not so much.

Documentation Changes Required

Nope

[okonomiyaki3000]

These failed checks are not really related this PR. Looks like a lot of deprecation notices.

[mbabker]

Actually, the failure IS because of this PR.

1) JCacheStorageTest::testGetInstance
Unexpected value for _application.
Failed asserting that '34eaab8a5eb6f33cbc0a7b5ea0e3a044' matches expected null.

[okonomiyaki3000]

Ah! Missed it. I guess the test needs to change.

[okonomiyaki3000]

Lol... OK, so the test actually sets $options['application'] as null. Then, this fails the check for isset() and we get the new default value. Since the old default value was null, the test is ambiguous. Are we testing that the value gets set to what is set in $options (even if it is null), or are we testing that a value of null in $options['application'] results in using the default value?

If we want to get the default value when $options['application'] is null then we need to change the test because the default value has changed.
If we want to blindly use whatever value is in $options['application'] then the code should change. We'll need to check with array_key_exists() instead of isset().

I don't have a strong opinion either way but I kind of lean toward null being replaced by the default value because, if you wanted null here, what you really wanted was empty string. Because the only use of this property is to be concatenated into a string so a value of null will be coerced into empty string anyway.

[mbabker]

Since the lifetime value is getting calculated and not being left as null, I'd say the test should be checking the options get transformed/initialized correctly.

[okonomiyaki3000]

That's good enough for me.

[okonomiyaki3000]

This Drone failure is exactly the same one that I get in #20221 which is a totally unrelated PR. The problem is Drone. If a.ui is undefined, it means that some page tried to load jquery.ui.sortable.min.js before loading jquery.ui.core.min.js or that the core file failed to load for some reason.

[okonomiyaki3000]

I guess we need one more approval here? Can we ignore the drone failure since it's clearly nonsense?

[mbabker]

There's now a passing Drone build on this PR. People don't have to run away in fear because they see a red X.

[okonomiyaki3000]

Thanks!

[okonomiyaki3000]

Hey, this is all green, is it getting merged?

[infograf768]

I guess we need 2 positive tests before merge as every PR

[okonomiyaki3000]

Guys, come on. This is actually fixes a pretty real issue.

[brianteeman]

and a very hard one for people to test unless they have access to apc server etc - thats the problem

[okonomiyaki3000]

In the past, on rare occasions, PRs have been approved on the basis of code review alone. This is a change of a single line. I would think that three or four smart people could have a look at it and determine whether or not it does what it should do while causing no harm.

[brianteeman]

thats true but until they do .....

[HLeithner]

Is there a reason why you don't have a unique secret per installation? That should have the same affect.

If I interpret the code correctly application is not the right place for getting a unique "instance" key.

	protected function _getCacheId($id, $group)
	{
		$name          = md5($this->_application . '-' . $id . '-' . $this->_language);
		$this->rawname = $this->_hash . '-' . $name;
		return Cache::getPlatformPrefix() . $this->_hash . '-cache-' . $group . '-' . $name;
	}

The key has 3 parts, first part the hash this is generated based on the secret and the group part separated by "-cache-" and the final generated name also separated by "-".

So you should set a unique secret for each site and the problem you describe is solved.

Btw. this is not only relevant for APC, also for all other caches incl. file cache so the test could be done by everyone.

[mbabker]

This has nothing to do with whether the secret key is set in the site's global configuration.

[HLeithner]

His problem is that the cache uses the same keys, the reason for this is that the secret key is the same for all installations.

There is no note what "application" is but it looks like its a part of the joomla component and not the joomla application at least how the cache id is formed.

[mbabker]

And yet this application parameter is used as part of the cache key generation, therefore it makes sense to give it a default value of something other than null. Opinions about the secret key or what the application parameter's actual use is being irrelevant here.

[HLeithner]

Sure you break the debug capability.
Now you can have 2 your test site and your live site.
if you have 2 different secret keys you can still compare the cached files.
if you have 2 different application names in this patch case the path to the configuration file, the last part of the cache not longer compare able.

ex.
live site:
09045c1b1d1915c60128dc55c81d0ff2-cache-com_templates-d65c890aa77991545128dab15bcbab5f
test site:
5656be6e93486a020e99a3952d8711c5-cache-com_templates-d65c890aa77991545128dab15bcbab5f

With this patch the second md5 is not longer the same and debugging is harder.
Or is there any other reason why there are to 2 md5 hashes?

[HLeithner]

I would add the configuration path to the _hash parameter

[mbabker]

if you have 2 different secret keys you can still compare the cached files.
if you have 2 different application names in this patch case the path to the configuration file, the last part of the cache not longer compare able.

If you need the cache keys to be consistent across environments then I would suggest that it's on you to make that happen.

I would add the configuration path to the _hash parameter

And that has a benefit over assigning a default value to the application parameter in what way? It still creates the exact same issue you just raised one comment before.

[HLeithner]

I would add the configuration path to the _hash parameter

And that has a benefit over assigning a default value to the application parameter in what way? It still creates the exact same issue you just raised one comment before.

no it doesn't the first md5 hash is only based on _hash and the second is based on $this->_application . '-' . $id . '-' . $this->_language

that's the different, the second hash stays the same and is only affected by the application and not by the installation

[mbabker]

And if you make $this->_hash = md5($config->get('secret') . JPATH_CONFIGURATION); it is basically creating the same problem as assigning $this->_application = JPATH_CONFIGURATION as it relates to your changing cache key issue.

Unless there is a real issue introduced with this PR, I will probably merge it shortly after this release. If there is a real issue introduced with this PR, show concrete evidence of it (and no, different cache keys per environment is not an issue IMO).

[HLeithner]

And if you make $this->_hash = md5($config->get('secret') . JPATH_CONFIGURATION); it is basically creating the same problem as assigning $this->_application = JPATH_CONFIGURATION as it relates to your changing cache key issue.

Sorry but that's plain wrong. $this->_hash is the first md5 hasn and _application is part of the second. So its not creating the same problem.

First md5 hash is the installation second md5 is the application.

	{
		$name          = md5($this->_application . '-' . $id . '-' . $this->_language);
		$this->rawname = $this->_hash . '-' . $name;
		return Cache::getPlatformPrefix() . $this->_hash . '-cache-' . $group . '-' . $name;
	}

There is a semantics that you should not break, or simply remove the second md5 hash completely because its useless and only costs performance. IMO.

[mbabker]

:eyeroll:

You just groaned that changing the seed changing the hashes across environments, then said it's OK to change the seed which changes the hashes across environments. See the inconsistency here?

Why is the second MD5 hash so sacred but the first MD5 hash can be changed on a whim (by your arguments)?

We do not make a promise of consistent hashes across environments. Therefore this PR has no B/C concerns and your semantics argument is purely hypothetical at best. The "application" portion of the hash is actually the identifier for that specific cache item, the first ("installation") portion could be dropped from the cache keys and would have no viable side effect as it is the same value for every cache item. So yes, it is important that this segment, especially where you have shared cache in use, be properly unique otherwise you have cache collisions across environments and that could be considered a security breach.

[HLeithner]

I say, changing the seed for the first hash is no problem because this is installation depended.
I only say it's not a good idea to change the seed of the second MD5 hash with an installation variable because till now the second MD5 hash depends only on the programmer.

Maybe I failed to write it correctly but can't see where, maybe you can point me the the comment where I was inconsistent.

Yes there is no promise but keeping consistent isn't wrong. Last time:

live site:
09045c1b1d1915c60128dc55c81d0ff2-cache-com_templates-d65c890aa77991545128dab15bcbab5f
test site:
5656be6e93486a020e99a3952d8711c5-cache-com_templates-d65c890aa77991545128dab15bcbab5f

All cache entries of the live site starts with 09045c1b1d1915c60128dc55c81d0ff2
All cache entries of the test site starts with 5656be6e93486a020e99a3952d8711c5

Cache entries having the same parameters on both sites having the same second MD5 hash ending with d65c890aa77991545128dab15bcbab5f

Anyway merge it and please make a new pr to remove the second md5 hash and merge the seed for this into the first one. Or do you see any reason for 2 hashes?

[mbabker]

IMO there should be one MD5 hash, that takes into account both application data (i.e. supplied key) and installation specific data (i.e. file path or secret key). We really don't need both and the only practical benefit to both is if you are manually reviewing a shared storage space (a directory, APC key list, Redis backend, etc.) where you can identify the cache data for a particular site based on the first hash.

But it is particularly dangerous to rely on that second hash being consistent. If you remove the first hash (the site identifier), you're left with multiple cache-com_templates-d65c890aa77991545128dab15bcbab5f keys and as I pointed out this can be a security breach. So regardless of the end result, the remaining MD5 hash should always have enough entropy to ensure uniqueness for every site.

[HLeithner]

Then please merge it and if you have time write a PR against joomla-framework caching layer? If you don't have the time to do it I can do this for you.

edit: only wanted that it doesn't get lost here.

[mbabker]

The Framework's cache package is completely different code (and deprecated anyway, PSR compliant cache handlers aren't an option in Joomla and we saw no benefit to maintaining 2 cache APIs). So we just need to clean things up here in this repo. Probably a 4.0 patch just to be safe.

[HLeithner]

ok thx for the info.

[ghost]

is this PR a new Feature @HLeithner?

[HLeithner]

thx