Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure config cache is only used when there is a correct path #15687

Merged
merged 1 commit into from Mar 11, 2020

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Mar 9, 2020

refs #14617 where this feature was implemented.

We noticed an issue that the app might break when the config cache was used or created in a wrong context.

Typically, the user settings file is an absolute path like PIWIK_DOCUMENT_ROOT . '/config/config.ini.php'. For example /foo/bar/config/config.ini.php.

However, when eg js/tracker.php is opened, then PIWIK_DOCUMENT_ROOT is set to ... So user settings file is ../config/config.ini.php. In some other cases it is set to ../.. etc.

The cached config file includes the config file path like

$this->settingsChain = [
    $globalIniPath = [.... settings ...],
    $commonIniPath = [.... settings ...],
    $localIniPath = [.... settings ...],
];

A problem is eg when the config cache file does not exist, then js/tracker.php is called. Then the cache will include this

$this->settingsChain = [
    '../config/global.ini.php' = [.... settings ...],
     '../config/common.ini.php'= [.... settings ...],
     '../config/config.ini.php'= [.... settings ...],
];

But when the actual app is opened, the absolute path is used. This means when we then request the list of default activated plugins from the global ini, it calls:

$iniReader->getFrom('/foo/bar/config/global.ini.php', 'Plugins');

This entry does not exist in $this->settingsChain and therefore the app assumes no plugins exist by default and many core plugins aren't detected as core plugins and considered bogus because they don't have a plugin.json etc.

This PR fixes this by only accepting the cache when the user settings file is an absolute path, meaning it won't be applied in js/tracker.php (which is fine for us), and we also ignore the cache if the cached entry does not actually contain an entry for that $settingsChain[$userConfigFile]. Meaning we only accept a cache when it can be actually used and is valid.

There are heaps of other ways to fix this, such as always using the absolute PIWIK_DOCUMENT_ROOT, or resolving relative paths, etc. For our case as we only use that feature anyway this is the easiest fix and works well for us on production. Other changes could cause maybe regressions and we also can't fully control that all PIWIK_DOCUMENT_ROOT entries are always correct etc. We may want to use consistent PIWIK_DOCUMENT_ROOT on top eventually (in a different PR).

We're using this in production and works nicely. Should be fine to merge if tests pass.

@tsteur tsteur added the Needs Review PRs that need a code review label Mar 9, 2020
@tsteur tsteur added this to the 3.13.4 milestone Mar 9, 2020
$cache = new Cache();
$values = $cache->doFetch(self::CONFIG_CACHE_KEY);

if (!empty($values)
&& isset($values['mergedSettings'])
&& isset($values['settingsChain'])) {
&& isset($values['settingsChain'][$userSettingsFile])) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we make sure the cached entry actually has $values['settingsChain']['/home/path/config/config.ini.php'] and is not eg $values['settingsChain']['../../config.ini.php'] . It shouldn't be needed probably but it protects using the cache maybe in some other edge cases where the app is sending a different absolute path than expected for some reason. Eg should there be a double slash somewhere in the path etc.

@diosmosis
Copy link
Member

Is there a reason we can't just use realpath()?

@tsteur
Copy link
Member Author

tsteur commented Mar 11, 2020

I'm not 100% sure how it behaves with symlinks when one of the upper directories is a symlink etc.

For our purpose this PR would work fine. Eventually will ideally in a different PR ensure to use absolute paths in eg js/tracker.php too

@diosmosis diosmosis merged commit 92232cd into 3.x-dev Mar 11, 2020
@diosmosis diosmosis deleted the fixconfigcache branch March 11, 2020 11:34
jbuget pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants