Fix: unserialize error#153
Open
ahuininga-orisha wants to merge 2 commits into
Open
Conversation
jansentjeu
requested changes
May 15, 2026
|
|
||
| return unserialize($this->getFilterAttributes()); | ||
| try { | ||
| $result = unserialize($raw, ['allowed_classes' => false]); |
Collaborator
There was a problem hiding this comment.
Surely a class could never be stored here? This is data retrieved from the admin page's POST request.
Collaborator
Author
There was a problem hiding this comment.
While the data originates from admin POST requests (so objects aren't expected), it's a security best practice — if the database row were ever tampered with, allowed_classes => false prevents PHP object injection attacks by making unserialize() throw instead of instantiating arbitrary classes.
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.
Summary
UrlFinder::loadPageLookupArray()never successfully populating its cache entry. The root cause was thatLandingPage::getUnserializedFilterAttributes()calledunserialize()without error handling. On PHP 8.x, malformed or truncatedfilter_attributesrows trigger aWarningthat Magento'sErrorHandlerconverts into a thrownException. This exception escaped theforeachloop inloadPageLookupArray(), preventing$this->cache->save()from ever being reached. On every new request (new worker/process) the in-memory guard wasnullagain, so the heavyemico_attributelanding_pageJOIN query ran unconditionally.unserialize(): Error at offsetwarnings (issue [Performance/bug] UrlFinder::loadPageLookupArray causes constant DB lookups and unserialize warnings (Emico_AttributeLanding) #135) by wrappingunserialize()in atry/catch (\Throwable)and validating the result is an array. Corrupt or legacy rows now return[]instead of throwing.['allowed_classes' => false]is also added as a security hardening measure.UrlFinder::loadPageLookupArray()so that if any single landing page'sgetFilters()call throws for any future reason, that page is skipped and the rest of the lookup array is still built and cached normally.How to test
Scenario 1 — Normal operation: cache is populated and reused
filter_attributes.emico_attributelanding_pageJOIN query runs exactly once.InvalidateCacheObserver), then is cached again on the next load.Scenario 2 — Corrupt
filter_attributesrow: graceful degradation, cache still populatedfilter_attributesto a malformed string, e.g.:bin/magento cache:clean).unserializewarning or exception appears invar/log/exception.log.Scenario 3 — Admin form renders correctly for corrupt row
filter_attributesdynamic rows field is empty (reflecting what is actually stored).