Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion system/Cache/FactoriesCache/FileVarExportHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public function save(string $key, mixed $val): void

// Two processes may try to create the directory at the same time.
// is_dir() confirms it exists, so suppressing the warning is safe.
if (! is_dir($this->path) && ! @mkdir($this->path, 0777, true) && ! is_dir($this->path)) {
if (! is_dir($this->path) && ! @mkdir($this->path, 0755, true) && ! is_dir($this->path)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the permission of the cache file itself below be fixed too? via chmod?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe so - good catch. With a permissive umask, the file remains world-writable. We should restrict its permissions before rename() too.

log_message('error', 'FactoriesCache: cannot create cache directory: ' . $this->path);

return;
Expand Down
32 changes: 32 additions & 0 deletions tests/system/Cache/FactoriesCacheFileVarExportHandlerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,36 @@ protected function createFactoriesCache(): void
$this->handler = new FileVarExportHandler();
$this->cache = new FactoriesCache($this->handler);
}

public function testSaveCreatesDirectoryWithCorrectPermissions(): void
{
$dir = WRITEPATH . 'cache_test_dir_' . uniqid('', true);
$oldUmask = umask(0000);

try {
$handler = new FileVarExportHandler();
$this->setPrivateProperty($handler, 'path', $dir);

$handler->save('test_key', ['data']);

$this->assertDirectoryExists($dir);

if (! is_windows()) {
$perms = fileperms($dir) & 0777;
$this->assertSame(0755, $perms);
}
} finally {
umask($oldUmask);

if (is_dir($dir)) {
$files = glob("{$dir}/*");

if ($files !== false) {
array_map(unlink(...), $files);
}
Comment on lines +52 to +56

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can reuse delete_files()


rmdir($dir);
}
}
}
Comment thread
michalsn marked this conversation as resolved.
}
Loading