From 1165b2163747a686d5d463c891b150582d38d3bd Mon Sep 17 00:00:00 2001 From: David Dreschner Date: Fri, 16 Oct 2026 11:03:14 +0200 Subject: [PATCH] fix(sharebymail): Send readable password by mail instead of hash Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: David Dreschner --- apps/sharebymail/lib/ShareByMailProvider.php | 22 +++--- .../tests/ShareByMailProviderTest.php | 68 +++++++++++++++++++ lib/private/Share20/Manager.php | 12 +++- tests/lib/Share20/ManagerTest.php | 66 ++++++++++++++++++ 4 files changed, 155 insertions(+), 13 deletions(-) diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index 71f984444b3ed..6f51cf09b7c85 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -101,25 +101,24 @@ public function create(IShare $share): IShare { // if the admin enforces a password for all mail shares we create a // random password and send it to the recipient - $password = $share->getPassword(); - if ($password === null && $this->shareManager->shareApiLinkEnforcePassword()) { - $password = $this->autoGeneratePassword($share); - $share->setPasswordHash($this->hasher->hash($password)); + $autoGeneratedPassword = null; + if ($share->getPassword() === null && $this->shareManager->shareApiLinkEnforcePassword()) { + $autoGeneratedPassword = $this->autoGeneratePassword($share); + $share->setPasswordHash($this->hasher->hash($autoGeneratedPassword)); } $shareId = $this->createMailShare($share); $this->createShareActivity($share); $data = $this->getRawShare($shareId); + $createdShare = $this->createShareObject($data); - // Temporary set the clear password again to send it by mail - // This need to be done after the share was created in the database - // as the password is hashed in between. - if ($password !== null) { - $data['password'] = $password; + // The auto-generated password is only known here, so it is put back in clear to send it by mail + if ($autoGeneratedPassword !== null) { + $createdShare->setPassword($autoGeneratedPassword); } - return $this->createShareObject($data); + return $createdShare; } /** @@ -278,7 +277,8 @@ public function sendMailNotification(IShare $share): bool { $this->sendEmail($share, $validEmails); // If we have a password set, we send it to the recipient - if ($share->getPassword() !== null) { + // A password hash is never sent, as it can't be used to open the share + if ($share->getPassword() !== null && !$share->isPasswordHashed()) { // If sending the password by mail is disabled, we send the password to the owner. // If share-by-talk password is enabled, we do not send the password to the recipient. // They can request it from the owner after opening the link. diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php index 3d0e8cb8d2485..1051c825e7267 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -714,6 +714,59 @@ public function testCreateFailed(): void { ); } + private function newMailShare(): Share { + $node = $this->createMock(File::class); + $node->method('getId')->willReturn(42); + + $share = new Share($this->rootFolder, $this->userManager); + $share->setShareType(IShare::TYPE_EMAIL) + ->setSharedWith('receiver@example.com') + ->setSharedBy('owner') + ->setShareOwner('owner') + ->setPermissions(Constants::PERMISSION_READ) + ->setToken('token') + ->setNode($node); + + return $share; + } + + public function testCreateStoresGivenPasswordHash(): void { + $this->shareManager->method('shareApiLinkEnforcePassword')->willReturn(true); + + $instance = $this->getInstance(['createShareActivity', 'autoGeneratePassword']); + $instance->expects($this->never())->method('autoGeneratePassword'); + + $passwordHash = Server::get(IHasher::class)->hash('password'); + $createdShare = $instance->create($this->newMailShare()->setPasswordHash($passwordHash)); + + $this->assertSame($passwordHash, $createdShare->getPassword()); + $this->assertTrue($createdShare->isPasswordHashed()); + $this->assertSame($passwordHash, $instance->getShareById($createdShare->getId())->getPassword()); + } + + public function testCreateStoresAutoGeneratedPasswordHashAndMailsClearPassword(): void { + $this->shareManager->method('shareApiLinkEnforcePassword')->willReturn(true); + $this->settingsManager->method('sendPasswordByMail')->willReturn(true); + $this->secureRandom->method('generate') + ->with(8, ISecureRandom::CHAR_HUMAN_READABLE) + ->willReturn('autogeneratedPassword'); + + $instance = $this->getInstance(['createShareActivity', 'sendEmail', 'sendPassword']); + + $createdShare = $instance->create($this->newMailShare()); + + $this->assertSame('autogeneratedPassword', $createdShare->getPassword()); + $this->assertFalse($createdShare->isPasswordHashed()); + + $storedShare = $instance->getShareById($createdShare->getId()); + $this->assertTrue($storedShare->isPasswordHashed()); + $this->assertTrue(Server::get(IHasher::class)->verify('autogeneratedPassword', $storedShare->getPassword())); + + $instance->expects($this->once())->method('sendEmail')->with($createdShare, ['receiver@example.com']); + $instance->expects($this->once())->method('sendPassword')->with($createdShare, 'autogeneratedPassword', ['receiver@example.com'])->willReturn(true); + $instance->sendMailNotification($createdShare); + } + public function testCreateMailShare(): void { $this->share->expects($this->any())->method('getToken')->willReturn(''); $this->share->expects($this->once())->method('setToken')->with('token'); @@ -1461,6 +1514,21 @@ public function testSendEmailListsPasswordInDetails(): void { self::invokePrivate($provider, 'sendEmail', [$share, ['john@doe.com']]); } + public function testSendMailNotificationDoesNotMailPasswordHash(): void { + $this->settingsManager->method('sendPasswordByMail')->willReturn(true); + + $instance = $this->getInstance(['sendEmail', 'sendPassword', 'sendPasswordToOwner']); + $share = $this->newMailShare() + ->setId('42') + ->setPasswordHash(Server::get(IHasher::class)->hash('password')); + + $instance->expects($this->once())->method('sendEmail')->with($share, ['receiver@example.com']); + $instance->expects($this->never())->method('sendPassword'); + $instance->expects($this->never())->method('sendPasswordToOwner'); + + $this->assertTrue($instance->sendMailNotification($share)); + } + public function testSendMailNotificationWithSameUserAndUserEmailAndNote(): void { $provider = $this->getInstance(); $user = $this->createMock(IUser::class); diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 2d2720dfb8f5b..1971abbdb6508 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -536,6 +536,7 @@ public function createShare(IShare $share): IShare { } } + $plainTextPassword = null; try { // Verify share type if ($share->getShareType() === IShare::TYPE_USER) { @@ -569,7 +570,8 @@ public function createShare(IShare $share): IShare { // If a password is set. Hash it! if (($share->getShareType() === IShare::TYPE_LINK || $share->getShareType() === IShare::TYPE_EMAIL) && $share->getPassword() !== null && !$share->isPasswordHashed()) { - $share->setPasswordHash($this->hasher->hash($share->getPassword())); + $plainTextPassword = $share->getPassword(); + $share->setPasswordHash($this->hasher->hash($plainTextPassword)); } } @@ -641,7 +643,13 @@ public function createShare(IShare $share): IShare { if ($share->getMailSend()) { $provider = $this->factory->getProviderForType($share->getShareType()); if ($provider instanceof IShareProviderWithNotification) { - $provider->sendMailNotification($share); + $notifiedShare = $share; + if ($plainTextPassword !== null) { + // The share only holds the hash, the mail needs the clear password given by the caller + $notifiedShare = clone $share; + $notifiedShare->setPassword($plainTextPassword); + } + $provider->sendMailNotification($notifiedShare); } else { $this->logger->debug('Share notification not sent because the provider does not support it.', ['app' => 'share']); } diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 1d62c7301c519..a706eff66d1f3 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -3504,6 +3504,72 @@ public function testCreateShareMail(): void { $this->assertEquals('token', $share->getToken()); } + public function testCreateShareMailSendsClearPassword(): void { + $manager = $this->createManagerMock() + ->onlyMethods([ + 'generalChecks', + 'pathCreateChecks', + 'validateExpirationDateLink', + 'verifyPassword', + 'setLinkParent', + ]) + ->getMock(); + + $shareOwner = $this->createMock(IUser::class); + $shareOwner->method('getUID')->willReturn('shareOwner'); + + $path = $this->createMock(File::class); + $path->method('getOwner')->willReturn($shareOwner); + $path->method('getName')->willReturn('target'); + $path->method('getId')->willReturn(1); + $path->method('getStorage')->willReturn($this->createMock(IStorage::class)); + + $share = $this->manager->newShare(); + $share->setShareType(IShare::TYPE_EMAIL) + ->setNode($path) + ->setSharedBy('sharedBy') + ->setPermissions(Constants::PERMISSION_ALL) + ->setMailSend(true) + ->setPassword('password'); + + $manager->method('validateExpirationDateLink')->willReturnArgument(0); + $manager->expects($this->once()) + ->method('verifyPassword') + ->with('password'); + + $this->secureRandom->method('generate') + ->willReturn('token'); + $this->config->method('getSystemValueBool') + ->with('sharing.enable_share_mail', true) + ->willReturn(true); + + $storedPassword = null; + $this->defaultProvider + ->expects($this->once()) + ->method('create') + ->willReturnCallback(function (Share $share) use (&$storedPassword): Share { + $storedPassword = $share->getPassword(); + $storedShare = clone $share; + return $storedShare->setId('42'); + }); + + $mailedPassword = null; + $this->defaultProvider + ->expects($this->once()) + ->method('sendMailNotification') + ->willReturnCallback(function (IShare $share) use (&$mailedPassword): bool { + $mailedPassword = $share->getPassword(); + return true; + }); + + $createdShare = $manager->createShare($share); + + $this->assertTrue(Server::get(IHasher::class)->verify('password', $storedPassword)); + $this->assertSame('password', $mailedPassword); + $this->assertSame($storedPassword, $createdShare->getPassword()); + $this->assertTrue($createdShare->isPasswordHashed()); + } + public function testCreateShareHookError(): void { $this->expectException(\Exception::class); $this->expectExceptionMessage('I won\'t let you share');