Skip to content
Closed
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
52 changes: 45 additions & 7 deletions system/Cache/Handlers/FileHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -112,18 +112,33 @@ public function save(string $key, mixed $value, int $ttl = 60): bool

public function delete(string $key): bool
{
$key = static::validateKey($key, $this->prefix);
$key = static::validateKey($key, $this->prefix);
$file = $this->path . $key;

if (! is_file($file)) {
return false;
}

$result = @unlink($file);

return is_file($this->path . $key) && unlink($this->path . $key);
if ($result === false) {
log_message('error', 'Failed to delete cache file: ' . $file);
}

return $result;
}
Comment on lines 113 to 129
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure if we actually need this logging or not. Since the delete() method already returns bool, whether to log or not would be user's preference rather than framework responsibility. If they decide to not log and just pass on, this log_message() call would be redundant in that case


public function deleteMatching(string $pattern): int
{
$deleted = 0;

foreach (glob($this->path . $pattern, GLOB_NOSORT) as $filename) {
if (is_file($filename) && @unlink($filename)) {
$deleted++;
if (is_file($filename)) {
if (@unlink($filename)) {
$deleted++;
} else {
log_message('error', 'Failed to delete cache file: ' . $filename);
}
}
}

Expand Down Expand Up @@ -200,6 +215,8 @@ protected function getItem(string $filename): array|false
$content = @file_get_contents($this->path . $filename);

if ($content === false) {
log_message('error', 'Failed to read cache file: ' . $this->path . $filename);

return false;
}
Comment on lines 217 to 221
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again, same as above, since we're already returning false, that would already mean the read operation failed, so we can just return false instead of logging.


Expand All @@ -222,7 +239,11 @@ protected function getItem(string $filename): array|false
}

if ($data['ttl'] > 0 && Time::now()->getTimestamp() > $data['time'] + $data['ttl']) {
@unlink($this->path . $filename);
$file = $this->path . $filename;

if (is_file($file) && ! @unlink($file)) {
log_message('error', 'Failed to delete expired cache file: ' . $file);
}

return false;
}
Expand All @@ -242,6 +263,8 @@ protected function getItem(string $filename): array|false
protected function writeFile($path, $data, $mode = 'wb'): bool
{
if (($fp = @fopen($path, $mode)) === false) {
log_message('error', 'Failed to open cache file for writing: ' . $path);

return false;
}

Expand Down Expand Up @@ -280,6 +303,8 @@ protected function deleteFiles(string $path, bool $delDir = false, bool $htdocs
$path = rtrim($path, '/\\');

if (! $currentDir = @opendir($path)) {
log_message('error', 'Failed to open cache directory: ' . $path);

return false;
}

Expand All @@ -288,14 +313,27 @@ protected function deleteFiles(string $path, bool $delDir = false, bool $htdocs
if (is_dir($path . DIRECTORY_SEPARATOR . $filename) && $filename[0] !== '.') {
$this->deleteFiles($path . DIRECTORY_SEPARATOR . $filename, $delDir, $htdocs, $_level + 1);
} elseif (! $htdocs || preg_match('/^(\.htaccess|index\.(html|htm|php)|web\.config)$/i', $filename) !== 1) {
@unlink($path . DIRECTORY_SEPARATOR . $filename);
$file = $path . DIRECTORY_SEPARATOR . $filename;
if (! @unlink($file)) {
log_message('error', 'Failed to delete cache file: ' . $file);
}
}
}
}

closedir($currentDir);

return ($delDir && $_level > 0) ? @rmdir($path) : true;
if ($delDir && $_level > 0) {
$result = @rmdir($path);

if (! $result) {
log_message('error', 'Failed to remove cache directory: ' . $path);
}

return $result;
}

return true;
}

/**
Expand Down
6 changes: 3 additions & 3 deletions system/Database/BaseResult.php
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ public function getRowArray(int $n = 0)
$this->currentRow = $n;
}

return $result[$this->currentRow];
return $result[$this->currentRow] ?? null;
}

/**
Expand All @@ -350,11 +350,11 @@ public function getRowObject(int $n = 0)
return null;
}

if ($n !== $this->customResultObject && isset($result[$n])) {
if ($n !== $this->currentRow && isset($result[$n])) {
$this->currentRow = $n;
}

return $result[$this->currentRow];
return $result[$this->currentRow] ?? null;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion system/Model.php
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@ public function update($id = null, $row = null): bool

protected function objectToRawArray($object, bool $onlyChanged = true, bool $recursive = false): array
{
return parent::objectToRawArray($object, $onlyChanged);
return parent::objectToRawArray($object, $onlyChanged, $recursive);
}

/**
Expand Down
2 changes: 2 additions & 0 deletions system/Security/Security.php
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,7 @@ private function removeTokenInRequest(IncomingRequest $request): void
try {
$json = json_decode($body, flags: JSON_THROW_ON_ERROR);
} catch (JsonException) {
log_message('error', 'Invalid JSON in request body during CSRF token removal');
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.

An error level failure is logged by default in the production environment, which allows a requester to cheaply generate disk-backed log entries. I don't believe we need to log this at all.

$json = null;
}

Expand Down Expand Up @@ -346,6 +347,7 @@ private function getPostedToken(IncomingRequest $request): ?string
try {
$json = json_decode($body, flags: JSON_THROW_ON_ERROR);
} catch (JsonException) {
log_message('error', 'Invalid JSON in request body during CSRF token retrieval');
$json = null;
}

Expand Down
83 changes: 83 additions & 0 deletions tests/system/Cache/Handlers/FileHandlerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -398,4 +398,87 @@ public function testReconnect(): void
{
$this->assertTrue($this->handler->reconnect());
}

public function testDeleteReturnsTrueForExistingFile(): void
{
$this->handler->save(self::$key1, 'value');

$this->assertTrue($this->handler->delete(self::$key1));
$this->assertNull($this->handler->get(self::$key1));
}

public function testDeleteReturnsFalseForNonExistentFile(): void
{
$this->assertFalse($this->handler->delete(self::$dummy));
}

public function testGetItemWithCorruptedDataDoesNotLogError(): void
{
$filePath = $this->config->file['storePath'] . DIRECTORY_SEPARATOR
. $this->config->prefix . self::$key2;

file_put_contents($filePath, 'corrupted_serialized_data_that_cannot_be_unserialized');

$this->assertNull($this->handler->get(self::$key2));

// Verify it did not log a "read cache file" error
$this->assertNull($this->handler->getMetaData(self::$key2));
}

public function testGetItemWithExpiredFileDeletesWithoutError(): void
{
// Save with 0 TTL (permanent) then manually modify the file to have a past time
$this->handler->save(self::$key3, 'value', 0);

$filePath = $this->config->file['storePath'] . DIRECTORY_SEPARATOR
. $this->config->prefix . self::$key3;

// Overwrite with expired data
$expiredData = serialize(['data' => 'value', 'ttl' => 1, 'time' => 100]);
file_put_contents($filePath, $expiredData);

$this->assertNull($this->handler->get(self::$key3));
$this->assertFileDoesNotExist($filePath);
}

#[RequiresOperatingSystem('Linux|Darwin')]
public function testDeleteWithUnwritableDirectoryLogsError(): void
{
$this->handler->save(self::$key1, 'value');

// Make the cache directory read-only so unlink fails
chmod($this->config->file['storePath'], 0555);

$this->handler->delete(self::$key1);

// Restore permissions before assertions
chmod($this->config->file['storePath'], 0777);

// Verify log message was recorded
$this->assertLogContains('error', 'Failed to delete cache file');
}

#[RequiresOperatingSystem('Linux|Darwin')]
public function testDeleteMatchingWithUnwritableDirectoryLogsError(): void
{
$this->handler->save(self::$key1, 'value');
$this->handler->save(self::$key2, 'value');

chmod($this->config->file['storePath'], 0555);

$this->handler->deleteMatching('*');

chmod($this->config->file['storePath'], 0777);

$this->assertLogContains('error', 'Failed to delete cache file');
}

public function testCleanRemovesAllFiles(): void
{
$this->handler->save(self::$key1, 'value');
$this->handler->save(self::$key2, 'value');

$this->assertTrue($this->handler->clean());
$this->assertCount(0, $this->handler->getCacheInfo());
}
}
Loading