Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filesystem Driver: Fix dir split not finishing cleanly #405

Merged
merged 1 commit into from
Feb 21, 2022

Conversation

the-eater
Copy link
Contributor

@the-eater the-eater commented Feb 13, 2022

I was using Stash as the cache backend for Doctrine, and, being nosy replaced the key hash function with a transparent function (fn($data) => $data) and noticed that the names of the files are not completely finished

Screenshot showing the path "ca/ch/stash/_defaul"

which was a bit alarming because this can cause unintentional cache conflicts or poisoning*

After digging around I found

for ($i = 0; $i < $this->directorySplit; $i++) {
$start = $len * $i;
if ($i == $this->directorySplit) {
$len = $sLen - $start;
}
$path .= substr($value, $start, $len) . DIRECTORY_SEPARATOR;
}

the $i == $this->directorySplit was supposed to prevent this, however this will always be false, thus I have now replaced it with a bit more robust way of splitting that should prevent this.

and which now works correctly as shown here

Screenshot showing the path "ca/che/stash/_default"

*: So this can actually open to cache poisoning attacks, but since the default uses md5 which has an even amount of bytes this is a non-issue unless people use key hash functions with an uneven length return, but these are scarce so ¯\_(ツ)_/¯, an additional "is the key as stored in the item the same as requested" check might be a nice to have, and could prevent this

PS: I am on PHP 8.1 and can't run the outdated phpunit (ok ran my new check by hotfixing it)

@the-eater the-eater marked this pull request as ready for review February 13, 2022 01:04
@tedivm tedivm merged commit c1add92 into tedious:master Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants