Make WordPress Core

Opened 3 weeks ago

Closed 2 weeks ago

#65695 closed defect (bug) (fixed)

Filesystem API: Fix inaccurate `WP_Filesystem_Direct::chmod()` file permissions comparison

Reported by: jeremyfelt Owned by: SergeyBiryukov
Priority: normal Milestone: 7.1
Component: Filesystem API Version: 7.0
Severity: normal Keywords: has-patch
Cc: Focuses:

Description

In [61601] via #64610, WP_Filesystem_Direct::chmod() was changed in an attempt to avoid calling chmod() when the requested mode were already set.

The use of | 0644 in that change alters the permission bits read from the file to a minimum floor before comparing them with the requested mode, which results in an inaccurate comparison.

That should be removed to allow the & 0777 mask to perform as intended: strip the filetype bits from the value returned by fileperms( $file ) so that the current permission bits can be used for comparison with the requested mode.

A review on the PR that led to [61601] suggested | 0644 for consistency with other core patterns, but those use | 0755 and | 0644 to ensure FS_CHMOD_DIR and FS_CHMOD_FILE are set with the minimum permission bits required, not for direct comparison with a requested mode.

In the current state: chmod() is sometimes skipped when it should run and sometimes run when it should skip.

Change History (4)

This ticket was mentioned in PR #12648 on WordPress/wordpress-develop by @jeremyfelt.


3 weeks ago
#1

The use of | 0644 alters the permission bits read from the file to a minimum floor before comparing them with the requested mode.

This results in an inaccurate comparison.

Removing this allows the & 0777 mask to perform as intended: strip the filetype bits from the value returned by fileperms( $file ) so that the current permission bits can be used for comparison with the requested mode.

This was added during review as part of #64610 for consistency with other parts of core, but those are using | 0755 and | 0644 to ensure FS_CHMOD_DIR and FS_CHMOD_FILE are set with the minimum permission bits required.

Trac ticket: https://core.trac.wordpress.org/ticket/65695

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Opus 4.8
Used for: Code review

#2 @softglaze
3 weeks ago

Reproduced the issue on trunk and verified PR 12648 fixes it.

Environment: Windows host, wordpress-develop Docker environment, tested via wp eval-file against a file in the container's native filesystem (/tmp) to avoid Windows bind-mount permission quirks.

On trunk (before patch):

File at 0600, requesting 0644 via WP_Filesystem_Direct::chmod() → file remained 0600 (chmod incorrectly skipped)
File at 0640, requesting 0600 → resulted in 0600

With PR 12648 applied:

File at 0600, requesting 0644 → correctly resulted in 0644
File at 0640, requesting 0600 → correctly resulted in 0600

The one-line change tests well.

#3 @SergeyBiryukov
2 weeks ago

  • Milestone Awaiting Review7.1

#4 @SergeyBiryukov
2 weeks ago

  • Owner set to SergeyBiryukov
  • Resolutionfixed
  • Status newclosed

In 62854:

Filesystem API: Correct permissions comparison in WP_Filesystem_Direct::chmod().

The & 0777 mask should be used to strip the filetype bits from the fileperms( $file ) value so that the current permission bits can be used for comparison with the requested mode.

This commit removes the extra | 0644 part of the mask, which altered the permission bits read from the file to a minimum floor before comparing them with the requested mode, resulting in an inaccurate comparison.

Follow-up to [61601].

Props jeremyfelt, softglaze, SergeyBiryukov.
Fixes #65695.

Note: See TracTickets for help on using tickets.