Skip to content

is_readable() returns writability, not readability (breaks dirlist on read-only dirs) #34

Description

@remyperona

Summary

VirtualFilesystemDirect::is_readable() returns a file's writability, not its readability. A read-only entry (e.g. 0444) is readable but not writable, so it is wrongly reported as "not readable".

// VirtualFilesystemDirect.php
public function is_readable( $file ) {
    return is_writeable( $this->getUrl( $file ) ); // ← wrong predicate
}

Impact

This is not purely cosmetic. dirlist() gates on is_readable():

// VirtualFilesystemDirect.php (dirlist)
if ( ! $this->is_dir( $path ) || ! $this->is_readable( $path ) ) {
    return false;
}

So listing a read-only directory returns false — precisely the kind of permission scenario a filesystem mock exists to reproduce. Any consumer test that sets a directory read-only and asserts on its listing gets a false negative.

Why it hasn't surfaced

The unit test only covers 0777 (→ true) and 000 (→ false), both of which happen to pass under the buggy delegation. A 0444 (read-only) case would fail. See Tests/Unit/VirtualFilesystemDirect/isReadable.php.

Failure scenario

$file = $this->filesystem->getFile( 'baz/index.html' );
$file->chmod( 0444 );                                   // read-only
$this->assertTrue( $this->filesystem->is_readable( 'baz/index.html' ) ); // FAILS (returns false)

Proposed fix

public function is_readable( $file ) {
    return is_readable( $this->getUrl( $file ) );
}

Plus regression coverage:

  • isReadable.php: add a 0444 case asserting true.
  • isWritable.php: add a 0444 case asserting false (guards against the inverse regression).
  • dirlist.php: add a read-only directory case asserting the listing is returned, not false.

Found during an audit of WP Rocket's integration test suite, which depends on this package.

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions