chore: fixup CryptoWrappingTest.php from review feedback

Signed-off-by: Josh <josh.t.richards@gmail.com>

tests/lib/Session/CryptoSessionDataTest.php

@@ -12,33 +12,185 @@ use OC\Session\CryptoSessionData;
 use OC\Session\Memory;
 use OCP\ISession;
 use OCP\Security\ICrypto;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\Attributes\UsesClass;
+use PHPUnit\Framework\MockObject\MockObject;
 
+/**
+ * Unit tests for CryptoSessionData, verifying encrypted session storage,
+ * tamper resistance, passphrase boundaries, and round-trip data integrity.
+ * Covers edge cases and crypto-specific behaviors beyond the base session contract.
+ *
+ * Note: ISession API conformity/contract tests are inherited from the parent
+ * (Test\Session\Session). Only crypto-specific (and pre-wrapper) additions are
+ * defined here.
+ */
+#[CoversClass(CryptoSessionData::class)]
+#[UsesClass(Memory::class)]
 class CryptoSessionDataTest extends Session {
-	/** @var \PHPUnit\Framework\MockObject\MockObject|ICrypto */
-	protected $crypto;
+	private const DUMMY_PASSPHRASE = 'dummyPassphrase';
+	private const TAMPERED_BLOB = 'garbage-data';
+	private const MALFORMED_JSON_BLOB = '{not:valid:json}';
 
-	/** @var ISession */
-	protected $wrappedSession;
+	protected ICrypto&MockObject $crypto;
+	protected ISession $session;
 
 	protected function setUp(): void {
 		parent::setUp();
 
-		$this->wrappedSession = new Memory();
 		$this->crypto = $this->createMock(ICrypto::class);
-		$this->crypto->expects($this->any())
-			->method('encrypt')
-			->willReturnCallback(function ($input) {
-				return '#' . $input . '#';
-			});
-		$this->crypto->expects($this->any())
-			->method('decrypt')
-			->willReturnCallback(function ($input) {
-				if ($input === '') {
-					return '';
-				}
-				return substr($input, 1, -1);
-			});
 
-		$this->instance = new CryptoSessionData($this->wrappedSession, $this->crypto, 'PASS');
+		$this->crypto->method('encrypt')->willReturnCallback(
+			fn ($input) => '#' . $input . '#'
+		);
+		$this->crypto->method('decrypt')->willReturnCallback(
+			fn ($input) => ($input === '' || strlen($input) < 2) ? '' : substr($input, 1, -1)
+		);
+
+		$this->session = new Memory();
+		$this->instance = new CryptoSessionData($this->session, $this->crypto, self::DUMMY_PASSPHRASE);
+	}
+
+	/**
+	 * Ensure backend never stores plaintext at-rest.
+	 */
+	public function testSessionDataStoredEncrypted(): void {
+		$keyName = 'secret';
+		$unencryptedValue = 'superSecretValue123';
+
+		$this->instance->set($keyName, $unencryptedValue);
+		$this->instance->close();
+
+		$unencryptedSessionDataJson = json_encode(["$keyName" => "$unencryptedValue"]);
+		$expectedEncryptedSessionDataBlob = $this->crypto->encrypt($unencryptedSessionDataJson, self::DUMMY_PASSPHRASE);
+
+		// Retrieve the CryptoSessionData blob directly from lower level session layer to bypass crypto decryption layer
+		$encryptedSessionDataBlob = $this->session->get('encrypted_session_data'); // should contain raw encrypted blob not the decrypted data
+		// Definitely encrypted?
+		$this->assertStringStartsWith('#', $encryptedSessionDataBlob); // Must match stubbed crypto->encrypt()
+		$this->assertStringEndsWith('#', $encryptedSessionDataBlob); // ditto
+		$this->assertNotSame($unencryptedSessionDataJson, $expectedEncryptedSessionDataBlob);
+		$this->assertSame($expectedEncryptedSessionDataBlob, $encryptedSessionDataBlob);
+	}
+
+	/**
+	 * Ensure various key/value types are storable/retrievable
+	 */
+	#[DataProvider('roundTripValuesProvider')]
+	public function testRoundTripValue($key, $value): void {
+		$this->instance->set($key, $value);
+		$this->instance->close();
+		// Simulate reload
+		$instance2 = new CryptoSessionData($this->session, $this->crypto, self::DUMMY_PASSPHRASE);
+		$this->assertSame($value, $instance2->get($key));
+	}
+
+	public static function roundTripValuesProvider(): array {
+		return [
+			'simple string' => ['foo', 'bar'],
+			'unicode value' => ['uni', 'héllo 🌍'],
+			'large value' => ['big', str_repeat('x', 4096)],
+			'large array' => ['thousand', json_encode(self::makeLargeArray())],
+			'empty string' => ['', ''],
+		];
+	}
+
+	/* Helper */
+	private static function makeLargeArray(int $size = 1000): array {
+		$result = [];
+		for ($i = 0; $i < $size; $i++) {
+			$result["key$i"] = "val$i";
+		}
+		return $result;
+	}
+
+	/**
+	 * Ensure removed values are not accessible after flush/reload.
+	 */
+	public function testRemovedValueIsGoneAfterClose(): void {
+		$this->instance->set('temp', 'gone soon');
+		$this->instance->remove('temp');
+		$this->instance->close();
+
+		$instance2 = new CryptoSessionData($this->session, $this->crypto, self::DUMMY_PASSPHRASE);
+		$this->assertNull($instance2->get('temp'));
+	}
+
+	/**
+	 * Ensure tampering is handled robustly.
+	 */
+	public function testTamperedBlobReturnsNull(): void {
+		$this->instance->set('foo', 'bar');
+		$this->instance->close();
+		// Bypass crypto layer and tamper the lower level blob
+		$this->session->set('encrypted_session_data', self::TAMPERED_BLOB);
+
+		$instance2 = new CryptoSessionData($this->session, $this->crypto, self::DUMMY_PASSPHRASE);
+		$this->assertNull($instance2->get('foo'));
+		$this->assertNull($instance2->get('notfoo'));
+	}
+
+	/**
+	 * Ensure malformed JSON is handled robustly.
+	 */
+	public function testMalformedJsonBlobReturnsNull(): void {
+		$this->instance->set('foo', 'bar');
+		$this->instance->close();
+		$this->session->set('encrypted_session_data', '#' . self::MALFORMED_JSON_BLOB . '#');
+		$instance2 = new CryptoSessionData($this->session, $this->crypto, self::DUMMY_PASSPHRASE);
+		$this->assertNull($instance2->get('foo'));
+	}
+
+	/**
+	 * Ensure an invalid passphrase is handled appropriately.
+	 */
+	public function testWrongPassphraseGivesNoAccess(): void {
+		// Override ICrypto mock/stubs for this test only
+		$crypto = $this->createPassphraseAwareCryptoMock();
+
+		// Override main instance with local ISession and local ICrypto mock/stubs
+		$session = new Memory();
+		$instance = new CryptoSessionData($session, $crypto, self::DUMMY_PASSPHRASE);
+
+		$instance->set('secure', 'yes');
+		$instance->close();
+
+		$instance2 = new CryptoSessionData($session, $crypto, 'NOT_THE_DUMMY_PASSPHRASE');
+		$this->assertNull($instance2->get('secure'));
+		$this->assertFalse($instance2->exists('secure'));
+	}
+
+	/* Helper */
+	private function createPassphraseAwareCryptoMock(): ICrypto {
+		$crypto = $this->createMock(ICrypto::class);
+
+		$crypto->method('encrypt')->willReturnCallback(function ($plain, $passphrase = null) {
+			// Set up: store a value with the passphrase embedded (fake encryption)
+			return $passphrase . '#' . $plain . '#' . $passphrase;
+		});
+		$crypto->method('decrypt')->willReturnCallback(function ($input, $passphrase = null) {
+			// Only successfully decrypt if the embedded passphrase matches
+			if (str_starts_with($input, $passphrase . '#') && str_ends_with($input, '#' . $passphrase)) {
+				// Strip off passphrase markers and return the "decrypted" string
+				return substr($input, strlen($passphrase . '#'), -strlen('#' . $passphrase));
+			}
+			// Fail to decrypt
+			return '';
+		});
+
+		return $crypto;
+	}
+
+	/**
+	 * Ensure closes are idempotent and safe.
+	 */
+	public function testDoubleCloseDoesNotCorrupt(): void {
+		$this->instance->set('safe', 'value');
+		$this->instance->close();
+		$blobBefore = $this->session->get('encrypted_session_data');
+		$this->instance->close(); // Should do nothing harmful
+		$blobAfter = $this->session->get('encrypted_session_data');
+		$this->assertSame($blobBefore, $blobAfter);
 	}
 }

tests/lib/Session/CryptoWrappingTest.php

@@ -9,57 +9,160 @@
 namespace Test\Session;
 
 use OC\Session\CryptoSessionData;
-use OCP\ISession;
+use OC\Session\CryptoWrapper;
+use OC\Session\Memory;
+use OCP\IRequest;
 use OCP\Security\ICrypto;
+use OCP\Security\ISecureRandom;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\UsesClass;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
+/**
+ * Unit tests for CryptoWrapper, focusing on session wrapping logic,
+ * passphrase handling (cookie and generation), and integration with
+ * CryptoSessionData. Ensures robust construction and non-duplication
+ * of crypto-wrapped sessions.
+ *
+ * Only wrapper-specific crypto behavior is tested here;
+ * core session encryption contract is covered in CryptoSessionDataTest.
+ *
+ * @see Test\Session\CryptoSessionDataTest For crypto storage testing logic.
+ */
+#[CoversClass(CryptoWrapper::class)]
+#[UsesClass(Memory::class)]
+#[UsesClass(CryptoSessionData::class)]
 class CryptoWrappingTest extends TestCase {
-	/** @var \PHPUnit\Framework\MockObject\MockObject|ICrypto */
-	protected $crypto;
+	private const DUMMY_PASSPHRASE = 'dummyPassphrase';
+	private const COOKIE_PASSPHRASE = 'cookiePassphrase';
+	private const GENERATED_PASSPHRASE = 'generatedPassphrase';
+	private const SERVER_PROTOCOL = 'https';
 
-	/** @var \PHPUnit\Framework\MockObject\MockObject|ISession */
-	protected $wrappedSession;
-
-	/** @var \OC\Session\CryptoSessionData */
-	protected $instance;
+	protected ICrypto&MockObject $crypto;
+	protected ISecureRandom&MockObject $random;
+	protected IRequest&MockObject $request;
 
 	protected function setUp(): void {
 		parent::setUp();
 
-		$this->wrappedSession = $this->getMockBuilder(ISession::class)
-			->disableOriginalConstructor()
-			->getMock();
-		$this->crypto = $this->getMockBuilder('OCP\Security\ICrypto')
-			->disableOriginalConstructor()
-			->getMock();
-		$this->crypto->expects($this->any())
-			->method('encrypt')
-			->willReturnCallback(function ($input) {
-				return $input;
-			});
-		$this->crypto->expects($this->any())
-			->method('decrypt')
-			->willReturnCallback(function ($input) {
-				if ($input === '') {
-					return '';
-				}
-				return substr($input, 1, -1);
-			});
-
-		$this->instance = new CryptoSessionData($this->wrappedSession, $this->crypto, 'PASS');
+		$this->crypto = $this->createMock(ICrypto::class);
+		$this->random = $this->createMock(ISecureRandom::class);
+		$this->request = $this->createMock(IRequest::class);
 	}
 
-	public function testUnwrappingGet(): void {
+	/**
+	 * Ensure wrapSession returns a CryptoSessionData when passed a basic session.
+	 */
+	public function testWrapSessionReturnsCryptoSessionData(): void {
+		$generatedPassphrase128 = str_pad(self::GENERATED_PASSPHRASE, 128, '_' . __FUNCTION__, STR_PAD_RIGHT);
+		$this->random->method('generate')->willReturn($generatedPassphrase128);
+
+		$this->request->method('getCookie')->willReturn(null);
+		$this->request->method('getServerProtocol')->willReturn(self::SERVER_PROTOCOL);
+
+		$session = new Memory();
+
+		$cryptoWrapper = new CryptoWrapper($this->crypto, $this->random, $this->request);
+		$wrappedSession = $cryptoWrapper->wrapSession($session);
+
+		$this->assertInstanceOf(CryptoSessionData::class, $wrappedSession);
+	}
+
+	/**
+	 * Ensure wrapSession returns the same instance if already wrapped.
+	 */
+	public function testWrapSessionDoesNotDoubleWrap(): void {
+		$alreadyWrapped = $this->createMock(CryptoSessionData::class);
+
+		$cryptoWrapper = new CryptoWrapper($this->crypto, $this->random, $this->request);
+		$wrappedSession = $cryptoWrapper->wrapSession($alreadyWrapped);
+
+		$this->assertSame($alreadyWrapped, $wrappedSession);
+	}
+
+	/**
+	 * Ensure a passphrase is generated and stored if no cookie is present.
+	 */
+	public function testPassphraseGeneratedIfNoCookie(): void {
+		$expectedPassphrase = str_pad(self::GENERATED_PASSPHRASE, 128, '_' . __FUNCTION__, STR_PAD_RIGHT);
+		$this->random->expects($this->once())->method('generate')->with(128)->willReturn($expectedPassphrase);
+
+		$this->request->method('getCookie')->willReturn(null);
+		$this->request->method('getServerProtocol')->willReturn(self::SERVER_PROTOCOL);
+
+		$cryptoWrapper = new CryptoWrapper($this->crypto, $this->random, $this->request);
+		$ref = new \ReflectionProperty($cryptoWrapper, 'passphrase');
+		$ref->setAccessible(true);
+
+		$this->assertTrue($ref->getValue($cryptoWrapper) !== null);
+		$this->assertSame($expectedPassphrase, $ref->getValue($cryptoWrapper));
+	}
+
+	/**
+	 * Ensure only the passphrase from cookie is used if present.
+	 */
+	public function testPassphraseReusedIfCookiePresent(): void {
+		$cookieVal = self::COOKIE_PASSPHRASE;
+		$this->request->method('getCookie')->willReturn($cookieVal);
+
+		$this->random->expects($this->never())->method('generate');
+		$this->request->method('getServerProtocol')->willReturn(self::SERVER_PROTOCOL);
+
+		$cryptoWrapper = new CryptoWrapper($this->crypto, $this->random, $this->request);
+		$ref = new \ReflectionProperty($cryptoWrapper, 'passphrase');
+		$ref->setAccessible(true);
+
+		$this->assertSame($cookieVal, $ref->getValue($cryptoWrapper));
+	}
+
+	/**
+	 * Ensure wrapSession throws if passed a non-ISession object (robustness).
+	 */
+	public function testWrapSessionThrowsTypeErrorOnInvalidInput(): void {
+		$cryptoWrapper = new CryptoWrapper($this->crypto, $this->random, $this->request);
+		$this->expectException(\TypeError::class);
+		$cryptoWrapper->wrapSession(new \stdClass());
+	}
+
+	/**
+	 * Full integration: wrap, set, get, flush, and encrypted blob.
+	 */
+	public function testIntegrationWrapSetAndGet(): void {
+		$keyName = 'someKey';
 		$unencryptedValue = 'foobar';
-		$encryptedValue = $this->crypto->encrypt($unencryptedValue);
+		$expectedPassphrase = str_pad(self::GENERATED_PASSPHRASE, 128, '_' . __FUNCTION__, STR_PAD_RIGHT);
 
-		$this->wrappedSession->expects($this->once())
-			->method('get')
-			->with('encrypted_session_data')
-			->willReturnCallback(function () use ($encryptedValue) {
-				return $encryptedValue;
-			});
+		$this->crypto->method('encrypt')->willReturnCallback(
+			fn ($input) => '#' . $input . '#'
+		);
+		$this->crypto->method('decrypt')->willReturnCallback(
+			fn ($input) => ($input === '' || strlen($input) < 2) ? '' : substr($input, 1, -1)
+		);
 
-		$this->assertSame($unencryptedValue, $this->wrappedSession->get('encrypted_session_data'));
+		$this->random->method('generate')->with(128)->willReturn($expectedPassphrase);
+		$this->request->method('getCookie')->willReturn(null);
+		$this->request->method('getServerProtocol')->willReturn(self::SERVER_PROTOCOL);
+
+		$session = new Memory();
+		$cryptoWrapper = new CryptoWrapper($this->crypto, $this->random, $this->request);
+		$wrappedSession = $cryptoWrapper->wrapSession($session);
+
+		$wrappedSession->set($keyName, $unencryptedValue);
+		$wrappedSession->close();
+
+		$this->assertTrue($wrappedSession->exists($keyName));
+		$this->assertSame($unencryptedValue, $wrappedSession->get($keyName));
+
+		$unencryptedSessionDataJson = json_encode(["$keyName" => "$unencryptedValue"]);
+		$expectedEncryptedSessionDataBlob = $this->crypto->encrypt($unencryptedSessionDataJson, $expectedPassphrase);
+
+		// Retrieve the CryptoSessionData blob directly from lower level session layer to guarantee bypass of crypto layer
+		$encryptedSessionDataBlob = $session->get('encrypted_session_data');
+		// Definitely encrypted?
+		$this->assertStringStartsWith('#', $encryptedSessionDataBlob); // Must match mocked crypto->encrypt()
+		$this->assertStringEndsWith('#', $encryptedSessionDataBlob); // ditto
+		$this->assertFalse($expectedEncryptedSessionDataBlob === $unencryptedSessionDataJson);
+		$this->assertSame($expectedEncryptedSessionDataBlob, $encryptedSessionDataBlob);
 	}
 }

tests/lib/Session/MemoryTest.php

@@ -11,6 +11,10 @@ namespace Test\Session;
 use OC\Session\Memory;
 use OCP\Session\Exceptions\SessionNotAvailableException;
 
+/**
+ * Concrete test case for OC\Session\Memory (in-memory session storage).
+ * Reuses session contract tests and adds in-memory specific assertions.
+ */
 class MemoryTest extends Session {
 	protected function setUp(): void {
 		parent::setUp();

tests/lib/Session/Session.php

@@ -8,6 +8,11 @@
 
 namespace Test\Session;
 
+/**
+ * Abstract base test class defining the contract for session storage backends.
+ * Contains generic tests for set/get/remove/clear/array session API compliance.
+ * Extend this class to provide $this->instance and validate custom session implementations.
+ */
 abstract class Session extends \Test\TestCase {
 	/**
 	 * @var \OC\Session\Session