mirror of
https://github.com/projectsend/projectsend.git
synced 2026-10-06 05:11:59 +00:00
A provider account's first password comes from its inbox, not its session
An account that signs in through a provider has no password to prove, so the password screen let the signed-in session choose one with no proof at all. A stolen session could then make itself permanent: set a password, confirm it, enrol its own second factor and remove the owner's last provider, since the account now read as local. The screen now refuses to set a provider account's password and offers to email a link instead: the ordinary reset link, to the account's own address, so whoever sets the password must read that inbox. The reset pages accept a signed-in visitor, since the owner opens the link in the browser they are signed in with; the token, not the session, is the authority. Using the link signs out every session holding the old password, the one that asked for it included. Compulsory two-factor lets the link through, so a provider account still has a way to enrol. Ordinary accounts are unchanged: they prove their current password. GHSA-4r8h-mwfm-f5f4
This commit is contained in:
@@ -11,6 +11,8 @@ use Illuminate\Http\RedirectResponse;
|
||||
use Illuminate\Http\Request;
|
||||
use Illuminate\Support\Facades\Auth;
|
||||
use Illuminate\Support\Facades\Hash;
|
||||
use Illuminate\Support\Facades\Password as PasswordBroker;
|
||||
use Illuminate\Validation\ValidationException;
|
||||
use Illuminate\Validation\Rules\Password;
|
||||
use Inertia\Inertia;
|
||||
use Inertia\Response;
|
||||
@@ -53,32 +55,24 @@ class PasswordController extends Controller
|
||||
// honest answer is to refuse rather than to appear to work.
|
||||
abort_if($user->auth_source === AuthSource::Ldap, 403);
|
||||
|
||||
$setsFirstPassword = $user->auth_source === AuthSource::Social;
|
||||
// An account that signs in through a provider has no password to
|
||||
// prove, so this screen cannot ask it for one, and the session
|
||||
// alone is not enough: a stolen session that could choose the
|
||||
// password became the account for good (GHSA-4r8h-mwfm-f5f4). Its
|
||||
// first password comes from a link emailed to its own address,
|
||||
// which sendLink() asks for and NewPasswordController completes.
|
||||
if ($user->auth_source === AuthSource::Social) {
|
||||
throw ValidationException::withMessages([
|
||||
'password' => __('Ask for a link by email to set your first password.'),
|
||||
]);
|
||||
}
|
||||
|
||||
$validated = $request->validate([
|
||||
// Not asked of an account that has never had one: it signs in
|
||||
// through a provider, and its stored hash is a generated
|
||||
// string nobody has seen. Asking anyway left those accounts
|
||||
// with no way to set a password — and so no way to enrol in
|
||||
// two-factor, which an installation can make compulsory.
|
||||
'current_password' => $setsFirstPassword ? ['nullable'] : ['required', 'current_password'],
|
||||
'current_password' => ['required', 'current_password'],
|
||||
'password' => ['required', Password::defaults(), 'confirmed'],
|
||||
]);
|
||||
|
||||
$attributes = ['password' => Hash::make($validated['password'])];
|
||||
|
||||
// The same line NewPasswordController writes when a provider
|
||||
// account resets its password, for the same reason: the hash is
|
||||
// now what signs this account in, and `has_local_password` is read
|
||||
// off this column all over the settings screens.
|
||||
if ($setsFirstPassword) {
|
||||
$attributes['auth_source'] = AuthSource::Local;
|
||||
}
|
||||
|
||||
// forceFill, not update(): `auth_source` is guarded, so a mass
|
||||
// assignment drops it silently — which left the account still
|
||||
// reading as passwordless after it had a password.
|
||||
$user->forceFill($attributes)->save();
|
||||
$user->forceFill(['password' => Hash::make($validated['password'])])->save();
|
||||
|
||||
// Changing a password is how someone reacts to a session they think
|
||||
// is stolen, so it has to actually end that session. AuthenticateSession
|
||||
@@ -93,4 +87,35 @@ class PasswordController extends Controller
|
||||
|
||||
return back();
|
||||
}
|
||||
|
||||
/**
|
||||
* Email an account that signs in through a provider a link to set its
|
||||
* first password.
|
||||
*
|
||||
* The ordinary reset link, to the account's own address: whoever sets
|
||||
* the password has to read that inbox, which a stolen session cannot
|
||||
* do. Opening it completes through NewPasswordController, which turns
|
||||
* the account local, and changing the password signs out every session
|
||||
* holding the old one, the one that asked for the link included.
|
||||
*/
|
||||
public function sendLink(Request $request): RedirectResponse
|
||||
{
|
||||
$user = $request->user();
|
||||
assert($user !== null);
|
||||
|
||||
// An account with a password uses the form above; a directory
|
||||
// account's password is not this installation's to set.
|
||||
abort_unless($user->auth_source === AuthSource::Social, 403);
|
||||
|
||||
$status = PasswordBroker::broker()->sendResetLink(['email' => $user->email]);
|
||||
|
||||
// Their own account, so being told to wait reveals nothing.
|
||||
if ($status === PasswordBroker::ResetThrottled) {
|
||||
throw ValidationException::withMessages([
|
||||
'link' => __('A link was sent a moment ago. Check your email, or try again in a minute.'),
|
||||
]);
|
||||
}
|
||||
|
||||
return back()->with('status', 'password-link-sent');
|
||||
}
|
||||
}
|
||||
|
||||
@@ -57,7 +57,12 @@ class EnforceTwoFactor
|
||||
// too. The loop then had no exit at all, which is how an
|
||||
// installation that made two-factor compulsory locked out
|
||||
// everybody who signs in with Microsoft.
|
||||
if ($request->routeIs('two-factor.*', 'password.confirm*', 'password.edit', 'password.update', 'logout', 'locale.update')) {
|
||||
//
|
||||
// password.link, password.reset and password.store complete that
|
||||
// same exit now that a provider account's first password arrives by
|
||||
// email: asking for the link, opening it and saving it all happen
|
||||
// while signed in, before there is a password to enrol with.
|
||||
if ($request->routeIs('two-factor.*', 'password.confirm*', 'password.edit', 'password.update', 'password.link', 'password.reset', 'password.store', 'logout', 'locale.update')) {
|
||||
return $next($request);
|
||||
}
|
||||
|
||||
|
||||
@@ -10,6 +10,7 @@ import { SaveButton } from '@/components/save-button';
|
||||
import { Input } from '@/components/ui/input';
|
||||
import { Label } from '@/components/ui/label';
|
||||
import { PasswordRequirements } from '@/components/password-requirements';
|
||||
import { Button } from '@/components/ui/button';
|
||||
import { useTranslation } from '@/hooks/use-translation';
|
||||
|
||||
interface PasswordProps {
|
||||
@@ -17,9 +18,11 @@ interface PasswordProps {
|
||||
has_local_password: boolean;
|
||||
/** True for an account whose password lives in a directory, which this screen cannot change. */
|
||||
managed_elsewhere: boolean;
|
||||
/** 'password-link-sent' once the link for a first password has been emailed. */
|
||||
status?: string;
|
||||
}
|
||||
|
||||
export default function Password({ has_local_password, managed_elsewhere }: PasswordProps) {
|
||||
export default function Password({ has_local_password, managed_elsewhere, status }: PasswordProps) {
|
||||
const { t } = useTranslation();
|
||||
|
||||
const breadcrumbs: BreadcrumbItem[] = [
|
||||
@@ -38,6 +41,17 @@ export default function Password({ has_local_password, managed_elsewhere }: Pass
|
||||
password_confirmation: '',
|
||||
});
|
||||
|
||||
// An account that signs in through a provider gets its first password
|
||||
// from a link emailed to its own address, not from this form: the
|
||||
// session alone must not be able to choose it.
|
||||
const needsLink = !has_local_password && !managed_elsewhere;
|
||||
const linkForm = useForm({});
|
||||
|
||||
const sendLink: FormEventHandler = (e) => {
|
||||
e.preventDefault();
|
||||
linkForm.post(route('password.link'), { preserveScroll: true });
|
||||
};
|
||||
|
||||
const updatePassword: FormEventHandler = (e) => {
|
||||
e.preventDefault();
|
||||
|
||||
@@ -79,7 +93,22 @@ export default function Password({ has_local_password, managed_elsewhere }: Pass
|
||||
</p>
|
||||
)}
|
||||
|
||||
<form onSubmit={updatePassword} className="space-y-6" hidden={managed_elsewhere}>
|
||||
{needsLink && (
|
||||
<form onSubmit={sendLink} className="space-y-4">
|
||||
<p className="text-muted-foreground text-sm">
|
||||
{t('We will email you a link to set it. Opening the link signs you out everywhere, so sign in again with your new password afterwards.')}
|
||||
</p>
|
||||
<Button type="submit" disabled={linkForm.processing}>
|
||||
{t('Email me a link')}
|
||||
</Button>
|
||||
{status === 'password-link-sent' && (
|
||||
<p className="text-sm font-medium text-green-600">{t('We sent a link to your email address. It works for one hour.')}</p>
|
||||
)}
|
||||
<InputError message={(linkForm.errors as Partial<Record<'link', string>>).link} />
|
||||
</form>
|
||||
)}
|
||||
|
||||
<form onSubmit={updatePassword} className="space-y-6" hidden={managed_elsewhere || needsLink}>
|
||||
<div className="grid gap-2" hidden={!has_local_password}>
|
||||
<Label htmlFor="current_password">{t('Current password')}</Label>
|
||||
|
||||
|
||||
+13
-6
@@ -73,12 +73,6 @@ Route::middleware('guest')->group(function () {
|
||||
->middleware('throttle:6,1,password-email')
|
||||
->name('password.email');
|
||||
|
||||
Route::get('reset-password/{token}', [NewPasswordController::class, 'create'])
|
||||
->name('password.reset');
|
||||
|
||||
Route::post('reset-password', [NewPasswordController::class, 'store'])
|
||||
->middleware('throttle:6,1,password-reset')
|
||||
->name('password.store');
|
||||
|
||||
Route::get('two-factor-challenge', [TwoFactorChallengeController::class, 'create'])
|
||||
->name('two-factor.challenge');
|
||||
@@ -87,6 +81,19 @@ Route::middleware('guest')->group(function () {
|
||||
->middleware('throttle:6,1,two-factor');
|
||||
});
|
||||
|
||||
// In neither group too. A reset link is also how an account that signs in
|
||||
// through a provider sets its first password (PasswordController::sendLink),
|
||||
// and its owner opens that link in the browser they are signed in with. The
|
||||
// token is the authority here, not the session: it was emailed to the
|
||||
// account's own address, so a signed-in visitor gains nothing a stranger
|
||||
// holding the same link would not.
|
||||
Route::get('reset-password/{token}', [NewPasswordController::class, 'create'])
|
||||
->name('password.reset');
|
||||
|
||||
Route::post('reset-password', [NewPasswordController::class, 'store'])
|
||||
->middleware('throttle:6,1,password-reset')
|
||||
->name('password.store');
|
||||
|
||||
// Deliberately in neither group. Signing in through a provider must not
|
||||
// require a session, and connecting one to an existing account requires
|
||||
// exactly that — so the guard is the intent written into the session
|
||||
|
||||
@@ -62,6 +62,12 @@ Route::middleware('auth')->group(function () {
|
||||
Route::put('settings/password', [PasswordController::class, 'update'])
|
||||
->middleware('throttle:6,1,password-update')
|
||||
->name('password.update');
|
||||
// An account without a password asks for one by email instead: see
|
||||
// PasswordController::sendLink. Its own bucket, and a small one, since
|
||||
// each request sends an email.
|
||||
Route::post('settings/password/link', [PasswordController::class, 'sendLink'])
|
||||
->middleware('throttle:3,1,password-link')
|
||||
->name('password.link');
|
||||
|
||||
Route::get('settings/two-factor', [TwoFactorEnrollmentController::class, 'show'])->name('two-factor.show');
|
||||
|
||||
|
||||
Reference in New Issue
Block a user