MyBB Community Forums

Full Version: Uploaded files and dirs not deleted from CDN static path
You're currently viewing a stripped down version of our content. View the full version with proper formatting.
Commit 9346f9 "absolutised" the paths supplied to the delete_uploaded_file() and delete_upload_directory() functions in inc/functions_upload.php, however, those functions were not adjusted to suit, and continue to expect relative paths.

The effect of this is that files and directories are not removed from under the static CDN path by these functions.

Suggested rewrites (largely untested):

/**
 * Delete an uploaded file both from the provided absolute path
 * as well as from the associated CDN path if a CDN is in use.
 *
 * @param string $abs_path The absolute path to the uploaded file.
 *                         Must resolve to a directory under MYBB_ROOT.
 *
 * @return bool Whether the file was deleted successfully.
 */
function delete_uploaded_file($abs_path) {
	global $mybb, $plugins;

	$deleted = false;

	$realpath = realpath($abs_path);
	if (substr($realpath, 0, strlen(MYBB_ROOT)) !== MYBB_ROOT || $realpath === realpath(MYBB_ROOT.'install/lock')) {
		return false;
	} else {
		$deleted = @unlink($realpath);

		$rel_path = substr($realpath, strlen(MYBB_ROOT));

		if (!empty($mybb->settings['usecdn'])) {
			$cdn_base_path = rtrim($mybb->settings['cdnpath'], '/');
			if (!empty($cdn_base_path)) {
				$cdn_path = realpath("{$cdn_base_path}/{$rel_path}");
				$deleted = $deleted && @unlink($cdn_path);
			}
		}

		$hook_params = array(
			'path' => &$rel_path,
			'deleted' => &$deleted,
		);

		$plugins->run_hooks('delete_uploaded_file', $hook_params);

		// Earlier return possible
		return $deleted;
	}
}

/**
 * Delete an upload directory on both the local filesystem and the CDN filesystem.
 *
 * @param string $abs_path The absolute path to the directory to delete.
 *
 * @return bool Whether the directory was deleted.
 */
function delete_upload_directory($abs_path) {
	global $mybb, $plugins;

	$deleted = false;

	$realpath = realpath($abs_path);
	if (substr($realpath, 0, strlen(MYBB_ROOT)) !== MYBB_ROOT) {
		return false;
	} else {
		$deleted_index = @unlink(rtrim($realpath, '/').'/index.html');

		$deleted = @rmdir($realpath);

		$rel_path = substr($realpath, strlen(MYBB_ROOT));

		if (!empty($mybb->settings['usecdn'])) {
			$cdn_base_path = rtrim($mybb->settings['cdnpath'], '/');
			if (!empty($cdn_base_path)) {
				$cdn_path = realpath("{$cdn_base_path}/{$rel_path}");
				$deleted = $deleted && @rmdir($cdn_path);
			}
		}

		$hook_params = array(
			'path' => &$rel_path,
			'deleted' => &$deleted,
		);

		$plugins->run_hooks('delete_upload_directory', $hook_params);

		// If not successfully deleted then reinstate the index file
		if (!$deleted && $deleted_index) {
			create_attachment_index($realpath);
		}

		return $deleted;
	}
}
Hi, better to create a PR to review the code and discuss Smile
(2023-03-02, 07:31 PM)Eldenroot Wrote: [ -> ]Hi, better to create a PR to review the code and discuss Smile

Hi Elden! Is the approach of "report bugs in this forum, and, if appropriate, they'll be pushed to GitHub" no longer preferred?