Json and PHP serializers should throw excepiton on failure

anandhi karnan → made their first commit to this issue’s fork.

I have updated the serializers to handle failure scenarios more robustly:

PHP Serializer (PhpSerialize):

• Added exception handling for unserialization errors.
• Incorporated the allowed_classes option to enhance security and prevent unserialization of objects.

JSON Serializer (Json):

• Implemented exception handling for JSON encoding and decoding, including handling invalid JSON and circular references.

Testing:

• Added tests to verify correct behavior for both valid and invalid inputs, covering various edge cases.

This broke all the tests it seems.

Also made validation go down so that should be looked at.

Removing ['allowed_classes' => FALSE] fixed the majority of the failures.

On a cursory glance many of the remaining failures appear to be test design failures in JSON API related tests:

Json::decode((string) $response->getBody()); Appears to be called in several places where getBody() may be an empty string (which is NOT a valid JSON string).

ResourceTestBase::assertResourceResponse() (a basic helper method) appears to trigger this as it will attempt to cast the response body as a decoding string to be used in the message without any knowledge of what the string value will be.

Later in assertResourceResponse() it appears the test (incorrectly) was written to depend upon GetDocumentFromResponseTrait::getDocumentFromResponse() returning NULL on invalid JSON which is off-spec for the API.

In Oembed:
core/modules/media/src/OEmbed/ProviderRepository.php:116 appears to be checked by looking at the value being empty rather than looking for an error (another off-spec use of the API).
core/modules/media/src/OEmbed/ResourceFetcher.php:67 appears to check the internal use of json_decode with json_last_error(). #3186415: Make oEmbed resource fetcher more tolerant of unexpected Content-Type headers → called out that the JSON Decoder failed to throw an exception.

Essentially: Legitimate test failures caused by off-api usage of methods.

We have a passing test stack now. Setting needs review to get eyes on below:

$ grep -R Json::decode *|grep -v Test

The following entries are already converted to handle the exception (based on failing tests):

core/modules/system/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php:        $json_payload = Json::decode($response);
core/modules/jsonapi/tests/src/Traits/GetDocumentFromResponseTrait.php:      $document = Json::decode((string) $response->getBody());
core/modules/media/src/OEmbed/ResourceFetcher.php:        $data = Json::decode($content);
core/modules/media/src/OEmbed/ProviderRepository.php:      $providers = Json::decode((string) $response->getBody());

We may want to convert the following areas of code with no apparent failing tests to catch the exception. If so do we need to do that here or is that a followup? If we need to do that here can we skip the test gate? These are well out of scope of the bug being reported here.

core/modules/announcements_feed/src/AnnounceFetcher.php:        $announcements = Json::decode($feed_content);	
core/modules/jsonapi/src/Controller/EntityResource.php:      $document = Json::decode($request->getContent());
core/modules/jsonapi/src/Controller/EntityResource.php:    $body = Json::decode($request->getContent());
core/modules/ckeditor5/src/Plugin/Editor/CKEditor5.php:    $toolbar_items = Json::decode($json);

Unsure on if we want these to be patched, or if we want the exceptions to rise through the stack:

core/modules/package_manager/src/PathExcluder/UnknownPathExcluder.php:    $repositories = Json::decode($repositories);
core/modules/package_manager/src/PathExcluder/UnknownPathExcluder.php:    $extra = Json::decode($this->composerInspector->getConfig('extra', $packages['drupal/core']->path . '/composer.json'));
core/modules/package_manager/src/Validator/AllowedScaffoldPackagesValidator.php:    $extra = Json::decode($this->composerInspector->getConfig('extra', $path . '/composer.json'));
core/modules/package_manager/src/Validator/ComposerPatchesValidator.php:      $extra = Json::decode($this->composerInspector->getConfig('extra', $working_dir));
core/modules/package_manager/src/Validator/PhpTufValidator.php:    $repositories = Json::decode($repositories);
core/modules/contextual/contextual.module:    $element['#links'] = Json::decode(rawurldecode($encoded_links));

package_manager: We might want those errors to show, in either case as the module is experimental can we deffer that to package_manager project to figure out before it makes it to stable?

contextual.module: This appears to be developer facing, we may want this to rise through the stack.

Suggest we do not consider patching:
core/modules/media/src/OEmbed/ResourceFetcher.php: return Json::decode($data);
This is decoding data that just went through encode(), if this fails something is significantly wrong with the deployment.

Needs a manual rebase. This is one may need to ping a framework manager or core committer (usually same group)

Manual rebase (conflict was due to new comments in ResourceTestBase.php).

Reran jobs with random failures.

Coming from 📌 Make igbinary the default serializer if available, it saves 50% time on unserialize and memory footprint Active , the use case there is that we would have a serializer that uses igbinary when available, but we need to update the database cache backend to deal with such an error for edge cases when receiving a igbinary serialized string when that isn't available.

I mentioned in Slack that the raised concern with Database Cache Backend likely needs significant architectural decisions that should not be rushed into this issue however I would implement it if it is the only way to get this to move forward.

It turns out that there already is an issue to handle the request in 🐛 Database backends cannot deal with corrupt serialized data: app completey broken Needs review .

Setting back to NR.

Rebased as the MR had gotten 400+ commits back. All tests are still passing

Looking at the MR I saw only 1 thread for feedback that was addressed so closed that.

Not sure if this needs framework manager sign off but all feedback appears to be addressed, fingers crossed.

One specific question on the MR, but also in general I'm wondering whether this needs to be a deprecation (or unsilenced E_USER_WARNING) changing to an exception in 12.x

Responded to MR comment.

wondering whether this needs to be a deprecation (or unsilenced E_USER_WARNING) changing to an exception in 12.x

My personal opinion is no. I can understand why this would be asked since this can be disruptive and given by the code being corrected there is clear proof that some implementations have misused this

However the API is well defined, the proper response is an exception. Code that has properly implemented SerilizationInterface::decode would already have a try/catch block surrounding it.

Today 3rd parties could replace the decoder using a class alias at the auto loader meaning the code that is broken by us fixing this can already be broken by a contrib module following reasonable design standards.

The FALSE return was/is an internal component of the JSON decode method that leaked and should not have been utilized.

As noted in the issue summary, this failure to throw an exception as documented on the API can lead to undetected data loss for those applications that properly follow the documented API.

This bug fundamentally breaks the ability to use DI of the serialization services.

I have no proven case to show this however a worst case fear is that silent data corruption could lead to security incidents.

CR created https://www.drupal.org/node/3519175 →

CR reads well to me.

Thanks for working on this and I agree we should make the change, however, I’ve discussed this with @catch and share his concerns about the impacts of changing the code to throw exceptions here. Yes the change is correct from a “programme to the interface” perspective but as we can see from core code callers are more often then not coded to work with the actual behaviour of the code. We need to give fair warning to custom and contrib that this change is coming. Therefore I think we should trigger an un-silenced E_USER_DEPRECATED to indicate that an exception will be thrown in Drupal 12 and we should file a follow-up to make the change in Drupal 12 and the changes to the callers.

This way modules can continue to work on 11 the way they do know and get ready for the changes in 12.x and it'll be easier to provide a continuous upgrade path. @cmlara I know that this process is likely to frustrate you - and understand that - it would be different if we had concrete cases where this is causing data loss or a security issue but we don;'t have that atm so we should prioritise the ability of sites to update without unexpected behaviour changes.

The approach in #19 (an un-silenced E_USER_DEPRECATED warning users that the situation will correctly use an exception in Drupal 12) has both framework and release management signoff so it's the approach we'll go forward with for now.

I definitely understand the concern to callers implementing the API correctly and receiving a silent response when they should not, but we have to balance the different risks. It would be good to help capture some of the risk in the message we add.