EntityId: Assert valid serialization in unserialize()

In change I4850feb0b5, we removed some tests that called unserialize()
with a serialization that wasn’t actually valid (non-ID strings, ints,
etc.), which noted that “All these cases are kind of an injection vector
and allow constructing invalid ids.” It’s unclear why these cases were
allowed and tested to begin with; my best guess is that unserialize()
skipped validation for performance reasons, but as far as I can tell,
it’s rarely if ever called from production code, so I think it’s better
to add the validation and reject invalid serializations.

We delegate most of the validation to the constructor (which then calls
assertValidIdFormat(); however, we afterwards validate that the
resulting serialization is the same string as in the data, i.e. that the
strtoupper() in the constructor didn’t have an effect.

FederatedPropertyId is not touched because it already calls
assertValidSerialization() in its __unserialize() method.

Change-Id: Id8239ce28ac448ab65a1b911cd25315960fa881e

Author: lucaswerkmeister

src/Entity/ItemId.php

@@ -64,7 +64,10 @@ public function __serialize(): array {
 	}
 
 	public function __unserialize( array $data ): void {
-		$this->serialization = $data['serialization'];
+		$this->__construct( $data['serialization'] );
+		if ( $this->serialization !== $data['serialization'] ) {
+			throw new InvalidArgumentException( '$data contained invalid serialization' );
+		}
 	}
 
 	/**

src/Entity/NumericPropertyId.php

@@ -60,7 +60,10 @@ public function __serialize(): array {
 	}
 
 	public function __unserialize( array $data ): void {
-		$this->serialization = $data['serialization'];
+		$this->__construct( $data['serialization'] );
+		if ( $this->serialization !== $data['serialization'] ) {
+			throw new InvalidArgumentException( '$data contained invalid serialization' );
+		}
 	}
 
 	/**

tests/unit/Entity/ItemIdTest.php

@@ -96,6 +96,19 @@ public function testUnserialize() {
 		$this->assertSame( 'Q2', $id->getSerialization() );
 	}
 
+	public function testUnserializeInvalid(): void {
+		$id = new ItemId( 'Q1' );
+		$this->expectException( InvalidArgumentException::class );
+		$id->__unserialize( [ 'serialization' => 'q' ] );
+	}
+
+	public function testUnserializeNotNormalized(): void {
+		$id = new ItemId( 'Q1' );
+		$this->expectException( InvalidArgumentException::class );
+		$id->__unserialize( [ 'serialization' => 'q2' ] );
+		// 'q2' is allowed in the constructor (silently uppercased) but not in unserialize()
+	}
+
 	/**
 	 * @dataProvider numericIdProvider
 	 */

tests/unit/Entity/NumericPropertyIdTest.php

@@ -95,6 +95,19 @@ public function testUnserialize() {
 		$this->assertSame( 'P2', $id->getSerialization() );
 	}
 
+	public function testUnserializeInvalid(): void {
+		$id = new NumericPropertyId( 'P1' );
+		$this->expectException( InvalidArgumentException::class );
+		$id->__unserialize( [ 'serialization' => 'p' ] );
+	}
+
+	public function testUnserializeNotNormalized(): void {
+		$id = new NumericPropertyId( 'P1' );
+		$this->expectException( InvalidArgumentException::class );
+		$id->__unserialize( [ 'serialization' => 'p2' ] );
+		// 'p2' is allowed in the constructor (silently uppercased) but not in unserialize()
+	}
+
 	/**
 	 * @dataProvider numericIdProvider
 	 */