Warning: is_dir(): open_basedir restriction in effect

added incident label

changed the description

Thanks @herbdool. Some thoughts:

For https://github.com/civicrm/civicrm-core/pull/21589 the alternate solution noted there is easy and doesn't involve @.
For the others some ideas:
1. A wrapper function for is_dir that checks if open_basedir is in effect and does something smart.
2. These are currently output as E_WARNINGS, which many production sites will log. It's possible to convert these with a wrapper error_handler to something else. Most production sites don't log E_USER_DEPRECATED - it's not really the right value to use but might be acceptable. E_USER_NOTICE would be better but most sites will still log those.
3. There might be another way to do the type of "find the right file" hunting that those code places are doing. But I don't have a good idea at the moment.
4. Syslog and other logging systems have a feature to consolidate repeated instances of the same log entry so you only get one every X. This is a generally useful feature beyond this issue. I don't know if having it in civi itself is a good idea and I think belongs in an extension, but it would mitigate this issue without losing logging for non-basedir-related errors and unit tests and other reasons.
  - Similarly some other logging systems let you filter by phrases for what gets written to the log.

PR here: https://github.com/civicrm/civicrm-core/pull/21923 for your first bullet point @DaveD

Just had an idea for an alternative for some of them. In some cases, such as in CRM_Admin_Page_APIExplorer::uniquePaths() they are using get_include_path(). We could at that point ensure that all those paths are within the open_basedir restrictions before returning the paths.

Could create a separate utility function such as CRM_Utils_File::isPathInOpenbasedir() so it can be used to check in uniquePaths() or in CRM_Utils_File::isDir() if still needed.

Maybe two things:

uniquepaths is a private function so there's no risk of that causing confusion by other uses of the function
Not a blocker just I'd be curious what the situation is where the include_path contains paths that are not in open_basedir, because they'll never be able to be loaded by anything, and so they might want to fix that. I suppose the situation where the apiexplorer warning is coming up is where you don't have control over the main webserver config, and the hosting provider has a mismatch between include_path and open_basedir.

Thanks @DaveD. Regarding your second point, I looked into it. We actually have control over it but we're using Plesk which seems to default to adding /opt/plesk/php/7.4/share/pear to the include_path but not including it in the open_basedir. I'm not sure why and I'm asking co-workers about that.

added improvement label

added git:civicrm-core label

added triaged label

@DaveD regarding your first point "uniquepaths is a private function so there's no risk of that causing confusion by other uses of the function". Are you suggesting we include the check within uniquePaths or do as I suggest by calling a separate, new method from uniquePaths?

The separate method makes sense - my comment was along the lines of review criteria https://docs.civicrm.org/dev/en/latest/standards/review/#r-tech. I was confirming that it isn't being called from other places and so changing what uniquePaths returns seems ok.

More generally:

I am for hiding or handling warnings that are expected, such as when you are looping and testing a couple possibilities some of which you are aware might not exist.
I am against hiding warnings that are pointing out things that might be wrong, but I agree I don't want to see the warning 8000 times a day. Alternatively, I want a way to turn it off without also turning off all the other E_WARNINGS.

So this particular one in uniquePaths - it seems like more of the second type. But I don't want to overcomplicate it since it sounds like having include_path and open_basedir mismatched might be common.

By the way I did start on some unit tests. It looks like php itself has some that can be ~~stolen~~ borrowed from: https://github.com/php/php-src/blob/5b01c4863fe9e4bc2702b2bbf66d292d23001a18/tests/security/open_basedir_is_dir.phpt

Trying out a different approach at https://github.com/civicrm/civicrm-core/pull/22107

@DaveD okay, so in your approach we'd still get exceptions for open_basedir. This would mean that we'd still need to have a fix outside of CiviCRM?

The example I'm thinking of is where it's trying to find all API files so it searches all directories listed in get_include_path(). Your function would help with that by returning FALSE for directories that don't exist. But in the case where not all paths listed in get_include_path() are in the open_basedir restriction, then it'll throw exceptions. And if we don't want to see those exceptions, and if we don't change CiviCRM to not look outside open_basedir, then we'll have to adjust open_basedir settings to also include whatever get_include_path() is providing. In our case, it's required us to customize open_basedir to be {WEBSPACEROOT}{/}{:}{TMP}{/}:/opt/plesk/php/7.4/share/pear.

I'm not sure where's best to place the responsibility. Plesk should have smarter defaults, but also it seems that CiviCRM should also be smarter to restrict itself to open_basedir.

we'd still get exceptions for open_basedir

No, it catches the ones related to openbasedir so they disappear.

Ah right :), thanks for clarifying.

That makes sense to me now and avoids the messiness of trying to parse directories.

added needs:testing label

I've updated the alternate PR: https://github.com/civicrm/civicrm-core/pull/22107

(Aside: the uniquePaths() function doesn't work fully on windows because it tries to parse via DIRECTORY_SEPARATOR which can be mixed on windows within a given path - I guess I never look at the api explorer examples...)

Merged - https://github.com/civicrm/civicrm-core/pull/22107

Warning: is_dir(): open_basedir restriction in effect

Overview

Reproduction steps

Comments

Child items ...

Activity