ACP CHMOD Check Fixes

[effone]

Resolves #4360
Addresses #4342 Regression

[lairdshaw]

This assumes that the current value of the setting (as entered by the admin) is a relative path (which has been converted in inc/init.php to an absolute one). If the current setting value, prior to conversion, is an absolute path though, then stripping MYBB_ROOT from the beginning is the wrong action to take here, and will display an incorrect value.

[effone]

No, the trim affects to any kind provided, not only absolute paths. Notice, we are not only stripping the MyBB_ROOT part from an absolute path there, we are stripping the initial dot (.) from a relative path as well. Thats how its neutralizing regardless the type of the path provided.

The array ensures to have all relative paths (or better: may say splitted suffix to MYBB_ROOT of a path , not the actual path in effect), because we are prepending MYBB_ROOT to all values altogether while checking writability just after:

if (is_writable(MYBB_ROOT . $path))

So the array values are not path. They are partial strings of a path. I'll even not say relative paths, they are just text strings to make the path. I don't see any wrong action there.

[lairdshaw]

What I mean is that let's say in Server and Optimization Options I set Uploads Path to the absolute path /path/to/mybb/uploads. When I open the System Health page, shouldn't I expect to see the value of File Uploads Directory set to the exact same value that I'd entered in those settings, namely /path/to/mybb/uploads? But with this PR, I don't. Instead, I see the relative path ./uploads. Isn't the fact that admins weren't always seeing here the same value that they entered in settings the whole reason you created this PR, in which case, shouldn't this PR ensure that I see /path/to/mybb/uploads here rather than ./uploads in the scenario I've given?

[effone]

Again, those are not paths in the array. They are parts.
This action doesn't modify any path in system. It fetches the required part from a path. No matter what admin sets and what PR #4342 manipulates. It affects to all types of value provided.

If you are confused about the effect of the ltrim there, please see:

echo ltrim('lairdshaw', 'airdl'); // shaw

echo ltrim('lairdshaw', 'drealbin'); // shaw
echo ltrim('bernardshaw', 'drealbin'); // shaw

[effone]

Sorry, may be I am not getting you right. Do you mean to say we should display all the absolute paths there?
The reason of my confusion is where exactly you have pointed. Thats not the visual part of the code, thats the working part. If you bother only about what admins see and what they should, you should have pointed here:

$table->construct_cell('./' . trim($path, '/'));

[effone]

I would like to know:

1. Which necessary part is removed from the path?
2. What is the procedure of reverse-manipulation of the base settings already manipulated by PR Fixes #4338 / #3754 - Relative/absolute path #4342 and overwritten in the settings value itself? Where is the backup of the original value before manipulation? Why I need to run an extra query for the info I already had, but deliberately forgot?
3. What are the test case scenarios that has failed with this PR (new code, as above)?
4. Why we are at all considering some arbitrary absolute path out of domain root and in that case how we are managing the display of attachment image thumbnails in posts?

[lairdshaw]

1. Which necessary part is removed from the path?

It occurs in a scenario like this:

The admin sets Uploads Path in Server and Optimization Settings to:

/some-dir/mybb/uploads

when MYBB_ROOT is set to:

/mybb

In this scenario (though tested using different directories applicable on my dev machine), the new code you propose above for this PR will mistakenly display beside File Uploads Directory in System Health:

/some-dir./uploads

2. What is the procedure of reverse-manipulation of the base settings already manipulated by PR Fixes #4338 / #3754 - Relative/absolute path #4342 and overwritten in the settings value itself?

It is currently not possible to reliably reverse those changes, because it is impossible to know whether the setting was originally an absolute path or whether it was changed by PR #4342 from a relative path to an absolute one. Hence my suggestions above (I prefer the first): #4361 (comment)

3. What are the test case scenarios that has failed with this PR?

See number 1 above.

4. Why we are at all considering some arbitrary absolute path out of domain root

Because it's a valid value for the setting, and we should handle valid values for settings.

and in that case how we are managing the display of attachment image thumbnails in posts?

I haven't confirmed this understanding, but my understanding is that neither full-sized nor thumbnail images are required to be web-accessible, only accessible to, and processed through, PHP via the web server, so that they can exist anywhere on the filesystem from/to which PHP (via the web server) can read and write, not necessarily under MYBB_ROOT. If I'm wrong about this, I hope you'll correct me.

[effone]

Attachment thumbnails are a very basic feature of MyBB and is there for ages. As per my understanding it was always considered that the upload attachments are under the domain root only.

If an admin deliberately inputs a settings like what you said, /some-dir/mybb/uploads then this is simply intentional and there are many settings like that in MyBB, infact anywhere - placing wrong settings simply fail the system. Thats the efficiency of Admin and thats why default values are there to hint. You can't ever make a setting input bullet-proof this way, no matter what you do. Speaking to the use case scenario you have pointed out that can be easily corrected in my above code and you also know how. In place of improving the effort your straight away rejection is stating that simply you are against this PR.

I am leaving this PR here as a reference of the valuable discussion we already had for others.
You can close the issue / PR if you really feel its not a regression. Keeping your concept in consideration, if at all MyBB works this way then there is much more important core changes are required to support out-of-domain paths (starting with a proxy image dispatcher).

Sorry friend. I failed to conclude this on a common and agreed opinion.

[lairdshaw]

As per my understanding it was always considered that the upload attachments are under the domain root only.

Well, in any case, deliberate or not, an uploads directory outside of the domain root works just fine (tested). This feature seems worth supporting given the extra security it provides given that the uploads directory can't then be accessed directly via the web.

Speaking to the use case scenario you have pointed out that can be easily corrected in my above code and you also know how.

Correct. However, the original setting value (pre being altered by PR #4342) can't be determined, which is the real problem.

you are against this PR.

I just don't understand the point of it. The only point to it that I can see would be to ensure that the exact value of the Uploads Path setting entered by the admin in the settings is displayed in System Health given that that value is altered by PR #4342 - but it doesn't even achieve this.

So, what is its point? To ensure that a relative path is displayed where possible - even if the admin entered an absolute path in the settings? If so, why?

Sorry friend. I failed to conclude this on a common and agreed opinion.

That's OK. Disagreements happen, even to friends.

[lairdshaw]

If #4370 is accepted, then we could go with this (tested):

$writables = array(
	'config_file' => 'inc/config.php',
	'settings_file' => 'inc/settings.php',
	'file_upload_dir' => $mybb->settings['uploadspath'],
	'avatar_upload_dir' => $mybb->settings['avataruploadpath'],
	'language_files' => 'inc/languages',
	'backup_dir' => $config['admin_dir'] . '/backups',
	'cache_dir' => 'cache',
	'themes_dir' => 'cache/themes'
);

$errors = 0; // Reset errors
$table = new Table;
$table->construct_header($lang->checked_path);
$table->construct_header($lang->location);
$table->construct_header($lang->status_chmod, array('width' => 250));
foreach ($writables as $langvar => $path)
{
	$table->construct_cell("<strong>{$lang->{$langvar}}</strong>");
	$table->construct_cell($path);
	if (is_writable(mk_path_abs($path)))
	{
		$table->construct_cell("<span style=\"color: green;\">{$lang->writable}</span>");
	}
	else
	{
		$table->construct_cell("<strong><span style=\"color: #C00\">{$lang->not_writable}</span></strong><br />{$lang->please_chmod_777}");
		++$errors;
	}

	$table->construct_row();
}

Your thoughts?