WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 3 weeks ago

#51340 new defect (bug)

Stop chmodding files and folders

Reported by: malthert Owned by:
Milestone: Awaiting Review Priority: normal
Severity: major Version: 5.3
Component: Filesystem API Keywords:
Focuses: Cc:

Description

WP's filesystem handler has a chmod function, that is used e.g. when updating,...

To conform with standards, enforce proper usage of umask by the server admin as well as avoid errors when the file owner is not the same as the user running WP, WP should not be chmodding files whatsoever.

Linux, for obvious security reasons, only allows chmod for the owner of the file (independent of permissions, except root).
Thus, it makes sense to have the WP files owned by user A, but run php(-fpm) by user B.

When WP now tries to chmod, which it shouldnt, as we have established that may cause a security issue, it will obviously create a PHP error.

Change History (6)

This ticket was mentioned in Slack in #core by helen. View the logs.


4 months ago

#2 @helen
4 months ago

  • Version trunk deleted

I am clearing the version here as I don't think it's new to trunk (5.6 beta), please feel free to change back and comment with more info if it is new so it can be investigated appropriately.

#3 @p00ya
4 months ago

I'll add another use case for when chmod is bad: some plugins support uploading to cloud storage (instead of filesystem wp-content/uploads) via stream wrappers. Stream wrappers don't necessarily support chmod, see:
https://www.php.net/manual/en/streamwrapper.stream-metadata.php

While these errors aren't fatal, they do confuse users of these plugins, since they're the most visible things in the logs.

#4 follow-up: @l3rady
3 weeks ago

  • Version set to 5.6.1

I too use streawrappers and Chmod is not implemented. A fix would be simply adding error suppression in the form of

@chmod(...

This is done in many areas of the WP codebase but has been missed here. I'm not sure at what version this was introduced but I can see the error back in WordPress 5.3 but don't see it in WP 4.9 so was introduced at some point between them.

#5 in reply to: ↑ 4 ; follow-up: @SergeyBiryukov
3 weeks ago

  • Version changed from 5.6.1 to 5.3

Replying to l3rady:

This is done in many areas of the WP codebase but has been missed here. I'm not sure at what version this was introduced but I can see the error back in WordPress 5.3 but don't see it in WP 4.9 so was introduced at some point between them.

It looks like the error suppression for chmod() was intentionally removed in [45611] / #47632 per the coding standards, setting the version accordingly.

Could you share the exact error message? There are at least 10 instances of chmod() in core, so it's not quite clear yet whether the ticket applies to the one in WP_Filesystem_Direct or any others too.

#6 in reply to: ↑ 5 @l3rady
3 weeks ago

Replying to SergeyBiryukov:

Could you share the exact error message? There are at least 10 instances of chmod() in core, so it's not quite clear yet whether the ticket applies to the one in WP_Filesystem_Direct or any others too.

ErrorException: chmod(): Aws\S3\StreamWrapper::stream_metadata is not implemented!
#6 includes/file.php(892): handleError
#5 /var/www/*******/releases/20210210123136/vendor/sentry/sentry/lib/Raven/ErrorHandler.php(0): chmod
#4 includes/file.php(892): _wp_handle_upload
#3 includes/file.php(953): wp_handle_upload
#2 includes/media.php(301): media_handle_upload
#1 includes/ajax-actions.php(2542): wp_ajax_upload_attachment
#0 async-upload.php(33): null

https://internal-s3-lon-cloud-bigfish-co-uk.s3.eu-west-2.amazonaws.com/scott/Screenshot%202021-02-12%20at%2011.25.54.png

In our case, we use AWS S3 stream wrapper and it doesn't support things like chmod. We have worked around the issue by extending Aws\S3\StreamWrapper and adding a stream_metadata function that returns false. However, that only fixes for our use case.

There are many other filesystems out there and not all of them support thinks like chmod, touch and mkdir. Things were fine in WP land as all filesystem calls were error suppressed with @.

It looks like someone has made the decision to not suppress errors as it feels very janky to do so, but when it comes to filesystem calls I think it is perfectly reasonable to do it.

There is still the argument to be made if WordPress should be making chmod calls in the first place as per the OP's argument.

I should have maybe posted my response as a new ticket however both the OP and my input has been triggered by the removal of error suppression.

Note: See TracTickets for help on using tickets.