[13.x] Fix FileStore cache deserialization with sub-10-digit timestamps#59327
Closed
JoshSalway wants to merge 10 commits intolaravel:13.xfrom
Closed
[13.x] Fix FileStore cache deserialization with sub-10-digit timestamps#59327JoshSalway wants to merge 10 commits intolaravel:13.xfrom
JoshSalway wants to merge 10 commits intolaravel:13.xfrom
Conversation
|
Thanks for submitting a PR! Note that draft PRs are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment was marked as spam.
This comment was marked as spam.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
FileStore::getPayload()reads cache files by splitting at a hardcoded 10-character boundary:expiration()returns a plainint, and timestamps before 2001-09-09 are only 9 digits (e.g.990464460). When a 9-digit timestamp is concatenated with the serialized value, the read side grabs 10 characters and eats the first byte of the data.Fixes #56075.
Prior discussion
@murrant reported this in #56075. @crynobone noted it would be hard to fix without a breaking change.
This approach avoids a breaking change.
expiration()is untouched (same signature, same return type, same behavior). The padding is applied at the call sites only.How it breaks (before)
Writing a cache entry while time-traveling to May 2001:
Reading it back:
The user gets
null. No error, no exception. The data was written but can never be read.How it works (after)
New files get padded to 10 digits on write with
sprintf('%010d', ...):Old files (written before the fix) are recovered.
strspndetects the short timestamp:Self-healing: old files get corrected on the next write. No
cache:clearneeded after upgrading.The fix
Fixing only the write side would prevent new broken files but leave old ones unreadable. Fixing only the read side would recover old files but keep writing broken ones. Fixing both makes the cache self-healing: any file written before or after this fix is handled correctly.
Write side (
put()andadd()): wraps$this->expiration($seconds)withsprintf('%010d', ...)so new cache files always have 10-digit timestamps.Read side (
getPayload()andadd()): usesstrspn($contents, '0123456789')to count leading digits and find the boundary between timestamp and serialized data. Serialized PHP always starts with a type-prefix letter (s:,i:,a:,b:,d:,N;,O:,C:), so the boundary is unambiguous. Works for any timestamp length.When would someone hit this?
Carbon::setTestNow()orDate::withTestNow()with a date before September 9, 2001. This is the scenario @murrant reported: time-traveling in tests.Test plan
Two new tests:
put()/get()works and correctly expiresAll existing FileStore tests pass.
Scope and safety
This change is limited to
FileStore.php, affectingput(),add(), andgetPayload()only. No other cache drivers are affected. No method signatures change. Theexpiration()method is untouched.Existing cache files cannot be corrupted by this change. The read side is backwards-compatible with both old (unpadded) and new (padded) formats. Worst case if anything goes wrong is a cache miss, which is the same failure mode as before.
Note on timestamp range
strspnhandles any digit length (8, 9, 10, 11+) andsprintf('%010d', ...)pads anything shorter than 10 digits. Timestamps outside the typical range (pre-1973 for 8-digit, post-2286 for 11-digit) work at the serialization level. Edge cases like negative timestamps (pre-1970) are outside the scope of this fix since they involve broader limitations in Unix timestamp representation.