bug symfony#53183 [Messenger] PhpSerializer: TypeError should throw MessageDecodingFailedException (B-Galati)

This PR was merged into the 5.4 branch.

Discussion
----------

[Messenger] PhpSerializer: TypeError should throw `MessageDecodingFailedException`

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT

<!--
Replace this notice by a description of your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the latest branch.
 - For new features, provide some code snippets to help understand usage.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

At the moment, some unserialization issues don't throw a `MessageDecodingFailedException` which prevent transport to trigger their decoding failure logic.

There are many possibilities to fix the issue. I've implemented a very conservative one. I believe we could simplify the code by a lot if we rely on the global error handler in user land.

Commits
-------

ebe5c3a [Messenger] PhpSerializer: TypeError should throw MessageDecodingFailedException

Author: nicolas-grekas

src/Symfony/Component/Messenger/Tests/Fixtures/DummyMessageTyped.php

@@ -0,0 +1,18 @@
+<?php
+
+namespace Symfony\Component\Messenger\Tests\Fixtures;
+
+class DummyMessageTyped implements DummyMessageInterface
+{
+    private string $message;
+
+    public function __construct(string $message)
+    {
+        $this->message = $message;
+    }
+
+    public function getMessage(): string
+    {
+        return $this->message;
+    }
+}

src/Symfony/Component/Messenger/Tests/Transport/Serialization/PhpSerializerTest.php

@@ -16,6 +16,7 @@
 use Symfony\Component\Messenger\Exception\MessageDecodingFailedException;
 use Symfony\Component\Messenger\Stamp\NonSendableStampInterface;
 use Symfony\Component\Messenger\Tests\Fixtures\DummyMessage;
+use Symfony\Component\Messenger\Tests\Fixtures\DummyMessageTyped;
 use Symfony\Component\Messenger\Transport\Serialization\PhpSerializer;
 
 class PhpSerializerTest extends TestCase
@@ -33,45 +34,45 @@ public function testEncodedIsDecodable()
 
     public function testDecodingFailsWithMissingBodyKey()
     {
+        $serializer = new PhpSerializer();
+
         $this->expectException(MessageDecodingFailedException::class);
         $this->expectExceptionMessage('Encoded envelope should have at least a "body", or maybe you should implement your own serializer');
 
-        $serializer = new PhpSerializer();
-
         $serializer->decode([]);
     }
 
     public function testDecodingFailsWithBadFormat()
     {
+        $serializer = new PhpSerializer();
+
         $this->expectException(MessageDecodingFailedException::class);
         $this->expectExceptionMessageMatches('/Could not decode/');
 
-        $serializer = new PhpSerializer();
-
         $serializer->decode([
             'body' => '{"message": "bar"}',
         ]);
     }
 
     public function testDecodingFailsWithBadBase64Body()
     {
+        $serializer = new PhpSerializer();
+
         $this->expectException(MessageDecodingFailedException::class);
         $this->expectExceptionMessageMatches('/Could not decode/');
 
-        $serializer = new PhpSerializer();
-
         $serializer->decode([
             'body' => 'x',
         ]);
     }
 
     public function testDecodingFailsWithBadClass()
     {
+        $serializer = new PhpSerializer();
+
         $this->expectException(MessageDecodingFailedException::class);
         $this->expectExceptionMessageMatches('/class "ReceivedSt0mp" not found/');
 
-        $serializer = new PhpSerializer();
-
         $serializer->decode([
             'body' => 'O:13:"ReceivedSt0mp":0:{}',
         ]);
@@ -99,6 +100,22 @@ public function testNonUtf8IsBase64Encoded()
         $this->assertTrue((bool) preg_match('//u', $encoded['body']), 'Encodes non-UTF8 payloads');
         $this->assertEquals($envelope, $serializer->decode($encoded));
     }
+
+    /**
+     * @requires PHP 7.4
+     */
+    public function testDecodingFailsForPropertyTypeMismatch()
+    {
+        $serializer = new PhpSerializer();
+        $encodedEnvelope = $serializer->encode(new Envelope(new DummyMessageTyped('true')));
+        // Simulate a change of property type in the code base
+        $encodedEnvelope['body'] = str_replace('s:4:\"true\"', 'b:1', $encodedEnvelope['body']);
+
+        $this->expectException(MessageDecodingFailedException::class);
+        $this->expectExceptionMessageMatches('/Could not decode/');
+
+        $serializer->decode($encodedEnvelope);
+    }
 }
 
 class DummyPhpSerializerNonSendableStamp implements NonSendableStampInterface

src/Symfony/Component/Messenger/Transport/Serialization/PhpSerializer.php

@@ -20,9 +20,6 @@
  */
 class PhpSerializer implements SerializerInterface
 {
-    /**
-     * {@inheritdoc}
-     */
     public function decode(array $encodedEnvelope): Envelope
     {
         if (empty($encodedEnvelope['body'])) {
@@ -38,9 +35,6 @@ public function decode(array $encodedEnvelope): Envelope
         return $this->safelyUnserialize($serializeEnvelope);
     }
 
-    /**
-     * {@inheritdoc}
-     */
     public function encode(Envelope $envelope): array
     {
         $envelope = $envelope->withoutStampsOfType(NonSendableStampInterface::class);
@@ -62,24 +56,30 @@ private function safelyUnserialize(string $contents)
             throw new MessageDecodingFailedException('Could not decode an empty message using PHP serialization.');
         }
 
-        $signalingException = new MessageDecodingFailedException(sprintf('Could not decode message using PHP serialization: %s.', $contents));
         $prevUnserializeHandler = ini_set('unserialize_callback_func', self::class.'::handleUnserializeCallback');
-        $prevErrorHandler = set_error_handler(function ($type, $msg, $file, $line, $context = []) use (&$prevErrorHandler, $signalingException) {
+        $prevErrorHandler = set_error_handler(function ($type, $msg, $file, $line, $context = []) use (&$prevErrorHandler) {
             if (__FILE__ === $file) {
-                throw $signalingException;
+                throw new \ErrorException($msg, 0, $type, $file, $line);
             }
 
             return $prevErrorHandler ? $prevErrorHandler($type, $msg, $file, $line, $context) : false;
         });
 
         try {
-            $meta = unserialize($contents);
+            /** @var Envelope */
+            $envelope = unserialize($contents);
+        } catch (\Throwable $e) {
+            if ($e instanceof MessageDecodingFailedException) {
+                throw $e;
+            }
+
+            throw new MessageDecodingFailedException('Could not decode Envelope: '.$e->getMessage(), 0, $e);
         } finally {
             restore_error_handler();
             ini_set('unserialize_callback_func', $prevUnserializeHandler);
         }
 
-        return $meta;
+        return $envelope;
     }
 
     /**