mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
3244be6bac
Reported by Nooraldden Khalel as GHSA-f32x-fgmp-q353. The profile screen let a signed-in session change its own email address with nothing else, and that address is where a password reset is sent. So a stolen session was enough: point the account at your own inbox, ask for a reset, set a password, and temporary access is permanent ownership. Clearing email_verified_at did not stand in the way, because the model does not implement MustVerifyEmail and the reset broker never asks. destroy(), thirty lines further down the same controller, has always required the current password, and its comment says why: "the rule every other door into this already asks". This door leads to the same place and was not asking. Only a *different* address asks. A name, a timezone or a custom field is not a credential, so the rest of the screen saves with nothing extra — which is why the rule is excluded rather than flat, and why the comparison is trimmed and lowercased: re-saving a profile with the address typed in a different case must not demand a password for nothing. An account whose credentials live in a directory or at an identity provider is refused outright and told why, rather than being asked for a password it does not have. LdapProvisioner stores Str::password(64) exactly so that local password can never be used, so asking would be a dead end dressed as a form error — and the address is not theirs to change here anyway: it is what the directory says it is. The test walks the whole chain rather than checking the field is validated, because the chain is what made this high: change the address, ask for a reset there, and confirm nothing is sent and no such account exists.
118 lines
4.3 KiB
PHP
118 lines
4.3 KiB
PHP
<?php
|
|
|
|
namespace App\Http\Requests\Settings;
|
|
|
|
use App\Models\User;
|
|
use App\Modules\Clients\ClientFieldContext;
|
|
use App\Modules\Clients\ClientPortalCustomFields;
|
|
use App\Modules\Identity\AuthSource;
|
|
use App\Support\Rules;
|
|
use Illuminate\Contracts\Validation\ValidationRule;
|
|
use Illuminate\Foundation\Http\FormRequest;
|
|
use Closure;
|
|
use Illuminate\Validation\Rule;
|
|
|
|
class ProfileUpdateRequest extends FormRequest
|
|
{
|
|
/**
|
|
* Get the validation rules that apply to the request.
|
|
*
|
|
* @return array<string, ValidationRule|array<mixed>|string>
|
|
*/
|
|
public function rules(): array
|
|
{
|
|
$rules = [
|
|
'name' => ['required', 'string', 'max:255'],
|
|
|
|
'email' => [
|
|
'required',
|
|
'string',
|
|
'lowercase',
|
|
'email',
|
|
'max:255',
|
|
Rule::unique(User::class)->ignore($this->user()?->id),
|
|
],
|
|
|
|
// Changing this address is a credential change, not a detail:
|
|
// it is where a password reset is sent, so whoever can change
|
|
// it owns the account from the next reset onwards. A stolen
|
|
// session used to be enough (GHSA-f32x-fgmp-q353) — temporary
|
|
// access became permanent ownership with one PATCH.
|
|
//
|
|
// `exclude_if` rather than a flat rule, so the rest of the
|
|
// screen keeps saving with nothing extra: a name, a timezone
|
|
// or a custom field is not a credential and must not start
|
|
// asking for a password. Only a *different* address does.
|
|
//
|
|
// The same rule destroy() one controller away has always
|
|
// asked, for the same reason: both doors lead to owning the
|
|
// account.
|
|
'current_password' => [
|
|
Rule::excludeIf(! $this->changesEmail()),
|
|
'required',
|
|
'current_password',
|
|
],
|
|
|
|
// Saved with the rest of the profile so the screen keeps one
|
|
// Save button. `timezone` is fillable, so ProfileController's
|
|
// fill() picks it up with no special handling.
|
|
//
|
|
// `sometimes`, not `required`: the form always sends it, but a
|
|
// caller that doesn't should leave the stored zone alone
|
|
// rather than be rejected — and there is no "no timezone" to
|
|
// clear it to.
|
|
'timezone' => ['sometimes', ...Rules::timezone()],
|
|
];
|
|
|
|
$user = $this->user();
|
|
|
|
// An account whose credentials live in a directory or at an
|
|
// identity provider holds a local password nobody knows — see
|
|
// LdapProvisioner, which stores Str::password(64) exactly so it
|
|
// can never be used. Asking such a person to confirm "your current
|
|
// password" is a dead end dressed as a form error, and the address
|
|
// is not theirs to change here in any case: it is what the
|
|
// directory or the provider says it is, and a local edit would
|
|
// either be overwritten or break the link.
|
|
if ($this->changesEmail() && $user !== null && $user->auth_source !== AuthSource::Local) {
|
|
$rules['email'][] = function (string $attribute, mixed $value, Closure $fail): void {
|
|
$fail(__('Your email address comes from the directory or identity provider you sign in with, and cannot be changed here.'));
|
|
};
|
|
}
|
|
|
|
if ($user?->isClient() === true) {
|
|
$rules = [
|
|
...$rules,
|
|
...app(ClientPortalCustomFields::class)->rules(ClientFieldContext::AccountEdit, $user),
|
|
];
|
|
}
|
|
|
|
return $rules;
|
|
}
|
|
|
|
/**
|
|
* Whether this request asks for an address other than the stored one.
|
|
*
|
|
* Compared lowercased and trimmed because the `lowercase` rule runs
|
|
* beside this one rather than before it: without that, re-saving the
|
|
* profile with the address typed in a different case would be read as
|
|
* a change and demand a password for nothing.
|
|
*/
|
|
private function changesEmail(): bool
|
|
{
|
|
$user = $this->user();
|
|
|
|
if ($user === null) {
|
|
return false;
|
|
}
|
|
|
|
$submitted = $this->input('email');
|
|
|
|
if (! is_string($submitted)) {
|
|
return false;
|
|
}
|
|
|
|
return mb_strtolower(trim($submitted)) !== mb_strtolower(trim((string) $user->email));
|
|
}
|
|
}
|