diff --git a/.forgejo/SECURITY.md b/.forgejo/SECURITY.md index 5c044c946..8f6d111ff 100644 --- a/.forgejo/SECURITY.md +++ b/.forgejo/SECURITY.md @@ -8,18 +8,32 @@ since we aim to provide a fairly stable upgrade path for new versions. ## Security Notifications -If you'd like to be notified of new potential security concerns you can [sign-up to the BookStack security mailing list](https://updates.bookstackapp.com/signup/bookstack-security-updates). +If you'd like to be notified of new potential security concerns, you can [sign-up to the BookStack security mailing list](https://updates.bookstackapp.com/signup/bookstack-security-updates). ## Reporting a Vulnerability -If you've found an issue that likely has no impact to existing users (For example, an issue only in the development branch) +If you've found an issue that likely has no impact on existing users (For example, an issue only in the development branch) feel free to raise it via a standard Codeberg bug report issue. -If the issue could have a security impact to BookStack instances, +If the issue could have a security impact on BookStack instances, please directly contact the lead maintainer via email Dan Brown using the [details found here](https://www.bookstackapp.com/links/contact/). -Please be patient while the vulnerability is being reviewed. Deploying the fix to address the vulnerability -can often take a little time due to the amount of preparation required, to ensure the vulnerability has -been covered, and to create the content required to adequately notify the user-base. +When contacting us, please note any names (and optionally any profile/company/website links) that you'd like to be used in +any attribution within our release notes and content. -Thank you for keeping BookStack instances safe! \ No newline at end of file +Please be patient while the vulnerability is being reviewed. Deploying the fix to address the vulnerability +can often take a little time due to the amount of preparation required to ensure the vulnerability has +been covered and to create the content required to adequately notify the user-base. + +Thank you for keeping BookStack instances safe! + +### CVE Creation + +We're generally happy for (and prefer) researchers to raise CVEs for issues they've discovered. +We ask that you first confirm with us to ensure the vulnerability is valid and that it hasn't yet been discovered +and reported by someone else. + +We can raise CVEs ourselves, but we would only go to the effort for security issues with a significant level of risk to users. +We typically won't pursue CVEs if there's a lesser level of risk. + +_Note: Our reporting may change as the Cyber Resilience Act comes into effect._ \ No newline at end of file diff --git a/app/Access/SocialAuthService.php b/app/Access/SocialAuthService.php index bdcfb45c8..faaa99601 100644 --- a/app/Access/SocialAuthService.php +++ b/app/Access/SocialAuthService.php @@ -55,8 +55,10 @@ class SocialAuthService */ public function handleRegistrationCallback(string $socialDriver, SocialUser $socialUser): SocialUser { + $socialDriver = trim(strtolower($socialDriver)); + // Check social account has not already been used - if (SocialAccount::query()->where('driver_id', '=', $socialUser->getId())->exists()) { + if ($this->getSocialAccountById($socialDriver, $socialUser->getId())) { throw new UserRegistrationException(trans('errors.social_account_in_use', ['socialAccount' => $socialDriver]), '/login'); } @@ -69,6 +71,19 @@ class SocialAuthService return $socialUser; } + /** + * Get an existing social account by its ID. + */ + protected function getSocialAccountById(string $socialDriver, string $socialId): SocialAccount|null + { + $socialDriver = trim(strtolower($socialDriver)); + + return SocialAccount::query() + ->where('driver', '=', $socialDriver) + ->where('driver_id', '=', $socialId) + ->first(); + } + /** * Get the social user details via the social driver. * @@ -93,7 +108,7 @@ class SocialAuthService $socialId = $socialUser->getId(); // Get any attached social accounts or users - $socialAccount = SocialAccount::query()->where('driver_id', '=', $socialId)->first(); + $socialAccount = $this->getSocialAccountById($socialDriver, $socialId); $isLoggedIn = auth()->check(); $currentUser = user(); $titleCaseDriver = Str::title($socialDriver); @@ -153,7 +168,7 @@ class SocialAuthService public function newSocialAccount(string $socialDriver, SocialUser $socialUser): SocialAccount { return new SocialAccount([ - 'driver' => $socialDriver, + 'driver' => trim(strtolower($socialDriver)), 'driver_id' => $socialUser->getId(), 'avatar' => $socialUser->getAvatar(), ]); @@ -164,6 +179,7 @@ class SocialAuthService */ public function detachSocialAccount(string $socialDriver): void { + $socialDriver = trim(strtolower($socialDriver)); user()->socialAccounts()->where('driver', '=', $socialDriver)->delete(); } diff --git a/tests/Auth/SocialAuthTest.php b/tests/Auth/SocialAuthTest.php index 89b8fd167..6980962b7 100644 --- a/tests/Auth/SocialAuthTest.php +++ b/tests/Auth/SocialAuthTest.php @@ -3,6 +3,7 @@ namespace Tests\Auth; use BookStack\Access\SocialAccount; +use BookStack\Access\SocialAuthService; use BookStack\Activity\ActivityType; use BookStack\Users\Models\User; use Illuminate\Support\Facades\DB; @@ -239,4 +240,60 @@ class SocialAuthTest extends TestCase $user = $user->whereEmail($user->email)->first(); $this->assertDatabaseHas('social_accounts', ['user_id' => $user->id]); } + + public function test_registration_allows_creation_with_id_which_already_used_by_another_user_and_service() + { + $user = $this->users->editor(); + SocialAccount::factory()->create(['driver' => 'google', 'driver_id' => '123abc', 'user_id' => $user->id]); + + $this->setSettings(['registration-enabled' => 'true']); + config(['GITHUB_APP_ID' => 'abc123', 'GITHUB_APP_SECRET' => '123abc']); + session()->put('social-callback', 'register'); + + $socialUser = new \Laravel\Socialite\One\User(); + $socialUser->id = '123abc'; + $socialUser->email = 'barrygithub@example.com'; + $socialUser->name = 'BarryGitHub'; + $socialUser->avatar = 'avatar_placeholder'; + + $serviceMock = $this->partialMockService(SocialAuthService::class); + $serviceMock->shouldReceive('getSocialUser')->andReturn($socialUser); + + $resp = $this->get('/login/service/github/callback'); + $resp->assertRedirect('/'); + + $this->assertTrue(auth()->check()); + $this->assertDatabaseHas('social_accounts', [ + 'user_id' => user()->id, + 'driver' => 'github', + 'driver_id' => '123abc' + ]); + $this->assertDatabaseHas('social_accounts', [ + 'user_id' => $user->id, + 'driver' => 'google', + 'driver_id' => '123abc' + ]); + } + + public function test_login_account_link_restrained_by_service() + { + config([ + 'GOOGLE_APP_ID' => 'abc123', 'GOOGLE_APP_SECRET' => '123abc', + ]); + $existingUser = $this->users->viewer(); + SocialAccount::factory()->create(['driver' => 'github', 'driver_id' => 'logintest123', 'user_id' => $existingUser->id]); + session()->put('social-callback', 'login'); + + $socialUser = new \Laravel\Socialite\One\User(); + $socialUser->id = 'logintest123'; + $serviceMock = $this->partialMockService(SocialAuthService::class); + $serviceMock->shouldReceive('getSocialUser')->andReturn($socialUser); + + $resp = $this->followingRedirects()->get('/login/service/google/callback'); + + $this->assertFalse(auth()->check()); + $this->assertNotEquals($existingUser->id, user()->id); + $resp->assertSee('login-form'); + $resp->assertSee(trans('errors.social_account_not_used', ['socialAccount' => 'Google'])); + } } diff --git a/tests/TestCase.php b/tests/TestCase.php index c6f811b31..12ad917d5 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -17,6 +17,8 @@ use Illuminate\Support\Facades\File; use Illuminate\Support\Facades\Log; use Illuminate\Testing\Assert as PHPUnit; use Illuminate\Testing\Constraints\HasInDatabase; +use Mockery; +use Mockery\MockInterface; use Monolog\Handler\TestHandler; use Monolog\Logger; use Ssddanbrown\AssertHtml\TestsHtml; @@ -106,6 +108,19 @@ abstract class TestCase extends BaseTestCase } } + /** + * Creates a partial mock of the given service class from a default + * container resolved instance. Compared to the default $this->partialMock function, + * this resolves mock of a service with its dependencies resolved from the container. + */ + protected function partialMockService(string $service): MockInterface + { + $realService = app($service); + $serviceMock = Mockery::mock($realService)->makePartial(); + $this->instance($service, $serviceMock); + return $serviceMock; + } + /** * Mock the http client used in BookStack http calls. */