mirror of
https://github.com/coollabsio/coolify.git
synced 2026-11-06 13:10:37 -04:00
fix(oauth): require a verified provider email for password users and use the instance callback URL
- Only password-less users from before OAuth identities skip the provider email verification claim. Otherwise an unverified provider account with the email of a password user (including root) could sign in as them. - Build the default callback URL from the instance URL, not the request Host header, and do not save it during a login. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
55bbe92d94
commit
3dabda5c56
@@ -9,12 +9,20 @@ use Laravel\Socialite\Two\GithubProvider;
|
||||
use SocialiteProviders\Discord\Provider;
|
||||
use SocialiteProviders\Manager\Config;
|
||||
|
||||
/**
|
||||
* Callback URL built from the instance URL, never from the request Host header.
|
||||
*/
|
||||
function oauth_default_redirect_uri(string $provider): string
|
||||
{
|
||||
return rtrim(base_url(), '/').route('auth.callback', $provider, false);
|
||||
}
|
||||
|
||||
function get_socialite_provider(string $provider)
|
||||
{
|
||||
$oauth_setting = OauthSetting::firstWhere('provider', $provider);
|
||||
|
||||
if (! filled($oauth_setting->redirect_uri)) {
|
||||
$oauth_setting->update(['redirect_uri' => route('auth.callback', $provider)]);
|
||||
$oauth_setting->redirect_uri = oauth_default_redirect_uri($provider);
|
||||
}
|
||||
|
||||
if ($provider === 'azure') {
|
||||
|
||||
+9
-4
@@ -8,9 +8,12 @@ use Illuminate\Support\Facades\Schema;
|
||||
return new class extends Migration
|
||||
{
|
||||
/**
|
||||
* Before OAuth identities existed, OAuth sign-in matched users by email only.
|
||||
* Mark every user that exists at upgrade time so their first OAuth identity
|
||||
* links without a provider email verification claim.
|
||||
* Before OAuth identities existed, OAuth sign-in matched users by email only
|
||||
* and created users without a password. Mark those password-less users so
|
||||
* their first OAuth identity links without a provider email verification
|
||||
* claim. Users with a password (including root) never get this exemption:
|
||||
* otherwise anyone with an unverified provider account with their email
|
||||
* could sign in as them.
|
||||
*/
|
||||
public function up(): void
|
||||
{
|
||||
@@ -18,7 +21,9 @@ return new class extends Migration
|
||||
$table->boolean('created_before_oauth_identities')->default(false);
|
||||
});
|
||||
|
||||
DB::table('users')->update(['created_before_oauth_identities' => true]);
|
||||
DB::table('users')
|
||||
->whereNull('password')
|
||||
->update(['created_before_oauth_identities' => true]);
|
||||
}
|
||||
|
||||
public function down(): void
|
||||
|
||||
@@ -58,7 +58,7 @@
|
||||
@if ($provider === 'oidc')
|
||||
<x-forms.input canGate="update" :canResource="$settings"
|
||||
id="oauth_settings_map.{{ $provider }}.redirect_uri"
|
||||
placeholder="{{ route('auth.callback', $provider) }}" label="Redirect URI" />
|
||||
placeholder="{{ oauth_default_redirect_uri($provider) }}" label="Redirect URI" />
|
||||
<x-forms.input canGate="update" :canResource="$settings"
|
||||
id="oauth_settings_map.{{ $provider }}.base_url" label="Issuer URL" required
|
||||
helper="OpenID Provider issuer URL, for example https://example.okta.com. Coolify uses it to discover the authorization, token, userinfo, and JWKS endpoints." />
|
||||
@@ -81,7 +81,7 @@
|
||||
@else
|
||||
<x-forms.input canGate="update" :canResource="$settings"
|
||||
id="oauth_settings_map.{{ $provider }}.redirect_uri"
|
||||
placeholder="{{ route('auth.callback', $provider) }}" label="Redirect URI" />
|
||||
placeholder="{{ oauth_default_redirect_uri($provider) }}" label="Redirect URI" />
|
||||
<x-forms.input canGate="update" :canResource="$settings"
|
||||
id="oauth_settings_map.{{ $provider }}.client_id" label="Client ID" required />
|
||||
<x-forms.input canGate="update" :canResource="$settings"
|
||||
|
||||
@@ -257,18 +257,108 @@ it('links an existing account on the first login after upgrade from the real pro
|
||||
]],
|
||||
]);
|
||||
|
||||
it('marks only users that exist at upgrade time as created before OAuth identities', function () {
|
||||
$existingUser = User::factory()->create();
|
||||
function rerunCreatedBeforeOauthIdentitiesMigration(): void
|
||||
{
|
||||
$migration = require database_path('migrations/2026_09_29_200325_add_created_before_oauth_identities_to_users_table.php');
|
||||
$migration->down();
|
||||
$migration->up();
|
||||
}
|
||||
|
||||
$newUser = User::factory()->create();
|
||||
/**
|
||||
* Brings the users into the state after an upgrade from v4.3.23.
|
||||
*/
|
||||
function upgradeUsersToOauthIdentities(): void
|
||||
{
|
||||
rerunCreatedBeforeOauthIdentitiesMigration();
|
||||
}
|
||||
|
||||
expect($existingUser->refresh()->created_before_oauth_identities)->toBeTrue()
|
||||
it('marks only password-less users that exist at upgrade time as created before OAuth identities', function () {
|
||||
$rootUser = User::factory()->create(['id' => 0]);
|
||||
$passwordUser = User::factory()->create();
|
||||
$oauthCreatedUser = User::factory()->create(['password' => null]);
|
||||
|
||||
rerunCreatedBeforeOauthIdentitiesMigration();
|
||||
|
||||
$newUser = User::factory()->create(['password' => null]);
|
||||
|
||||
expect($oauthCreatedUser->refresh()->created_before_oauth_identities)->toBeTrue()
|
||||
->and($rootUser->refresh()->created_before_oauth_identities)->toBeFalse()
|
||||
->and($passwordUser->refresh()->created_before_oauth_identities)->toBeFalse()
|
||||
->and($newUser->refresh()->created_before_oauth_identities)->toBeFalse();
|
||||
});
|
||||
|
||||
it('does not link an unverified provider email to a password user after the upgrade', function (string $provider, array $rawClaims) {
|
||||
$user = User::factory()->create(['id' => 0, 'email' => 'root@example.com']);
|
||||
$setting = OauthSetting::updateOrCreate(['provider' => $provider], [
|
||||
'client_id' => 'client-id',
|
||||
'client_secret' => 'client-secret',
|
||||
'base_url' => 'https://auth.example.com',
|
||||
'enabled' => true,
|
||||
]);
|
||||
upgradeUsersToOauthIdentities();
|
||||
|
||||
expect(fn () => app(OauthLoginService::class)->login($provider, (object) [
|
||||
'email' => 'root@example.com',
|
||||
'name' => 'Attacker',
|
||||
'id' => 'attacker-provider-id',
|
||||
'user' => $rawClaims,
|
||||
], $setting))->toThrow(HttpException::class, 'OAuth provider did not verify the email address');
|
||||
|
||||
$this->assertGuest();
|
||||
expect(OauthIdentity::count())->toBe(0)
|
||||
->and($user->refresh()->created_before_oauth_identities)->toBeFalse();
|
||||
})->with([
|
||||
'discord unverified' => ['discord', ['verified' => false]],
|
||||
'authentik default email scope' => ['authentik', ['email_verified' => false]],
|
||||
]);
|
||||
|
||||
it('links a verified provider email to a password user after the upgrade', function (string $provider, array $rawClaims) {
|
||||
$user = User::factory()->create(['email' => 'member@example.com']);
|
||||
$setting = OauthSetting::updateOrCreate(['provider' => $provider], [
|
||||
'client_id' => 'client-id',
|
||||
'client_secret' => 'client-secret',
|
||||
'enabled' => true,
|
||||
]);
|
||||
upgradeUsersToOauthIdentities();
|
||||
|
||||
$resolvedUser = app(OauthLoginService::class)->login($provider, (object) [
|
||||
'email' => 'member@example.com',
|
||||
'name' => 'Member',
|
||||
'id' => 'member-provider-id',
|
||||
'user' => $rawClaims,
|
||||
], $setting);
|
||||
|
||||
expect($resolvedUser->is($user))->toBeTrue()
|
||||
->and(OauthIdentity::where(['user_id' => $user->id, 'provider' => $provider])->exists())->toBeTrue();
|
||||
$this->assertAuthenticatedAs($user);
|
||||
})->with([
|
||||
'github' => ['github', []],
|
||||
'google verified' => ['google', ['email_verified' => true, 'hd' => 'example.com']],
|
||||
'discord verified' => ['discord', ['verified' => true]],
|
||||
]);
|
||||
|
||||
it('links a password-less user from before the upgrade without an email verification claim', function () {
|
||||
$user = User::factory()->create(['email' => 'legacy@example.com', 'password' => null]);
|
||||
$setting = OauthSetting::create([
|
||||
'provider' => 'discord',
|
||||
'client_id' => 'discord-client-id',
|
||||
'client_secret' => 'discord-client-secret',
|
||||
'enabled' => true,
|
||||
]);
|
||||
upgradeUsersToOauthIdentities();
|
||||
|
||||
$resolvedUser = app(OauthLoginService::class)->login('discord', (object) [
|
||||
'email' => 'legacy@example.com',
|
||||
'name' => 'Legacy User',
|
||||
'id' => 'legacy-discord-id',
|
||||
'user' => ['verified' => false],
|
||||
], $setting);
|
||||
|
||||
expect($resolvedUser->is($user))->toBeTrue()
|
||||
->and($user->refresh()->created_before_oauth_identities)->toBeFalse();
|
||||
$this->assertAuthenticatedAs($user);
|
||||
});
|
||||
|
||||
it('links a user from before the upgrade without an email verification claim', function (string $provider, array $rawClaims, ?string $password) {
|
||||
$user = User::factory()->create([
|
||||
'email' => 'legacy@example.com',
|
||||
@@ -610,3 +700,21 @@ it('rejects a Google account outside the configured Workspace even when its emai
|
||||
expect(OauthIdentity::count())->toBe(0);
|
||||
$this->assertGuest();
|
||||
});
|
||||
|
||||
it('sends the instance callback url without saving it when a forged host starts an oauth login', function () {
|
||||
InstanceSettings::query()->whereKey(0)->update(['fqdn' => 'https://coolify.example.com']);
|
||||
Once::flush();
|
||||
OauthSetting::create([
|
||||
'provider' => 'github',
|
||||
'client_id' => 'github-client-id',
|
||||
'client_secret' => 'github-client-secret',
|
||||
'enabled' => true,
|
||||
]);
|
||||
|
||||
$response = $this->get('http://attacker.example/auth/github/redirect');
|
||||
|
||||
$response->assertRedirect();
|
||||
parse_str((string) parse_url($response->headers->get('Location'), PHP_URL_QUERY), $query);
|
||||
expect($query['redirect_uri'])->toBe('https://coolify.example.com/auth/github/callback')
|
||||
->and(OauthSetting::where('provider', 'github')->value('redirect_uri'))->toBeNull();
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user