From bde86c10e457572de87b79ffd622cb0f1d5068fa Mon Sep 17 00:00:00 2001
From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com>
Date: Fri, 28 Aug 2026 23:59:11 +0200
Subject: [PATCH] Make linking a provider re-prove the password

Connecting a provider needed nothing but the session. Anyone holding one
could POST /settings/connected-accounts/google, follow the returned
Inertia::location(), sign in at the provider as *themselves*, and
completeLink() would bind their identity to the victim's account.

SocialAccount says what that row is:

    This row *is* the authorization to sign in as that account.

So it is not a preference -- it is a credential, and one that outlives
every way the victim has of ending the session that created it. It
survives a password change, it survives Auth::logoutOtherDevices(), it
survives invalidating every session. Where a stolen session gives an
attacker access until it is noticed, this gives them an account.

routes/settings.php already makes exactly this argument, twenty lines
down, for the two-factor block and the API token routes:

    a token outlives the session that minted it, so a stolen session must
    not be enough to mint one

The link has that property too, and was the one thing on this screen
without the gate. Now it has it.

The gate goes on `connect`, not on the callback: starting the flow is what
writes the intent the callback completes, and the callback deliberately
sits outside every group so a provider sign-in works without a session.

Not changed, deliberately: `connected-accounts.destroy`. Disconnecting
removes a way in rather than adding one, and destroy() already refuses to
remove the last one ("This is the only way you can sign in. Set a password
first"). Putting it behind password.confirm would fall hardest on the
accounts a provider provisioned -- they hold a Str::password(64) nobody
has ever seen -- and leave them unable to disconnect anything at all.
There is a test pinning that it stays reachable.

Also not changed: the account owner still is not told. SocialLoginController
writes an activity log entry, and that sits behind `staff` +
can:view_actions_log, so a client never sees it. Notifying them is a real
gap and a separate change; this one closes the door rather than adding a
bell to it.

Tests: two that fail against the ungated route -- the redirect, and the
whole attack end to end with a stranger identity never binding. The
existing connect() helper now confirms the password, the way
enableTwoFactor() already did, so the rest of the file keeps exercising
the real gate rather than asserting around it.
---
 routes/settings.php                           | 20 ++++++-
 .../Identity/ConnectedAccountsTest.php        | 58 +++++++++++++++++++
 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/routes/settings.php b/routes/settings.php
index e157a7cf..51b55f7a 100644
--- a/routes/settings.php
+++ b/routes/settings.php
@@ -57,9 +57,27 @@ Route::middleware('auth')->group(function () {
     // property of your account, not of your role.
     Route::get('settings/connected-accounts', [ConnectedAccountsController::class, 'edit'])
         ->name('connected-accounts.edit');
+    // Behind password.confirm for the same reason as the two-factor block
+    // and the API tokens below: a SocialAccount row "*is* the
+    // authorization to sign in as that account", so it outlives the
+    // session that created it. It survives a password change, it survives
+    // Auth::logoutOtherDevices(), and it survives every session being
+    // invalidated -- which makes "attach my provider identity to your
+    // account" the most durable thing a stolen session can do. Starting
+    // the flow is what binds it, because the callback finishes with
+    // whichever provider account signed in at the other end, not
+    // necessarily the victim's.
     Route::post('settings/connected-accounts/{provider}', [SocialLoginController::class, 'connect'])
-        ->middleware('throttle:20,1,social-connect')
+        ->middleware(['password.confirm', 'throttle:20,1,social-connect'])
         ->name('connected-accounts.connect');
+
+    // Deliberately without it. Disconnecting removes a way in
+    // rather than adding one, and destroy() already refuses to remove the
+    // last one ("This is the only way you can sign in"). Requiring a
+    // password confirmation here would fall hardest on exactly the
+    // accounts that have no local password to confirm with -- the ones
+    // provisioned by a provider -- and leave them unable to disconnect
+    // anything.
     Route::delete('settings/connected-accounts/{provider}', [ConnectedAccountsController::class, 'destroy'])
         ->name('connected-accounts.destroy');
 
diff --git a/tests/Feature/Identity/ConnectedAccountsTest.php b/tests/Feature/Identity/ConnectedAccountsTest.php
index 0cff0d84..cd534a41 100644
--- a/tests/Feature/Identity/ConnectedAccountsTest.php
+++ b/tests/Feature/Identity/ConnectedAccountsTest.php
@@ -31,6 +31,10 @@ function connect(User $user, SocialIdentity $identity): TestResponse
 {
     test()->swap(SocialGateway::class, new FakeSocialGateway($identity));
 
+    // Starting the flow is behind password.confirm -- see the route, and
+    // the test below that pins it.
+    confirmPassword($user);
+
     test()->actingAs($user)->post(route('connected-accounts.connect', ['provider' => $identity->provider->value]));
 
     return test()->actingAs($user)->get(route('social.callback', ['provider' => $identity->provider->value]));
@@ -101,6 +105,7 @@ test('reconnecting a different account at the same provider replaces the link',
 
 test('connecting from the settings screen navigates the browser, not the XHR', function () {
     test()->swap(SocialGateway::class, new FakeSocialGateway);
+    confirmPassword($this->staff);
 
     $this->actingAs($this->staff)
         ->post(route('connected-accounts.connect', ['provider' => 'google']), [], ['X-Inertia' => 'true'])
@@ -110,6 +115,7 @@ test('connecting from the settings screen navigates the browser, not the XHR', f
 
 test('a plain request is still given the provider redirect itself', function () {
     test()->swap(SocialGateway::class, new FakeSocialGateway);
+    confirmPassword($this->staff);
 
     $this->actingAs($this->staff)
         ->post(route('connected-accounts.connect', ['provider' => 'google']))
@@ -215,3 +221,55 @@ test('only usable providers are listed', function () {
     $this->actingAs($this->staff)->get('/settings/connected-accounts')
         ->assertInertia(fn ($page) => $page->has('providers', 1)->where('providers.0.provider', 'google'));
 });
+
+// A SocialAccount row "*is* the authorization to sign in as that account",
+// so binding one is exactly the kind of thing a stolen session must not be
+// enough to do -- the same argument routes/settings.php already makes for
+// the two-factor block and for minting an API token.
+test('connecting a provider requires a fresh password confirmation', function () {
+    $this->withSession(['auth.password_confirmed_at' => null]);
+
+    $this->actingAs($this->staff)
+        ->post(route('connected-accounts.connect', ['provider' => 'google']))
+        ->assertRedirect(route('password.confirm'));
+
+    expect(SocialAccount::query()->where('user_id', $this->staff->id)->exists())->toBeFalse();
+});
+
+test('a session that never proved the password cannot bind a stranger identity', function () {
+    // The whole attack in one test: the flow is never started, so the
+    // callback has no intent in the session to complete, and nothing is
+    // bound to the victim's account.
+    $attacker = new SocialIdentity(SocialProvider::Google, 'attacker-sub', 'attacker@evil.test', true, 'Attacker');
+    $this->swap(SocialGateway::class, new FakeSocialGateway($attacker));
+
+    $this->withSession(['auth.password_confirmed_at' => null]);
+
+    $this->actingAs($this->staff)
+        ->post(route('connected-accounts.connect', ['provider' => 'google']))
+        ->assertRedirect(route('password.confirm'));
+
+    $this->actingAs($this->staff)->get(route('social.callback', ['provider' => 'google']));
+
+    expect(SocialAccount::query()
+        ->where('user_id', $this->staff->id)
+        ->where('provider_user_id', 'attacker-sub')
+        ->exists())->toBeFalse();
+});
+
+test('disconnecting stays reachable without one', function () {
+    // Deliberately outside the gate: it removes a way in rather than
+    // adding one, and destroy() already refuses to remove the last. An
+    // account provisioned by a provider has no password to confirm with,
+    // so requiring one here would strand exactly them.
+    linkRow($this->staff, 'google-sub');
+
+    $this->withSession(['auth.password_confirmed_at' => null]);
+
+    $this->actingAs($this->staff)
+        ->delete(route('connected-accounts.destroy', ['provider' => 'google']))
+        ->assertRedirect()
+        ->assertSessionHasNoErrors();
+
+    expect(SocialAccount::query()->where('user_id', $this->staff->id)->exists())->toBeFalse();
+});