From 0f725c4f0554764b927759a0b018ea63cbec5488 Mon Sep 17 00:00:00 2001 From: Eleazar Resendez Date: Thu, 27 Aug 2026 14:06:35 -0600 Subject: [PATCH 1/3] fix(users): prevent stored XSS in profile fields --- .../Http/Controllers/Api/UserController.php | 62 ++++- .../ValidateEditUserAndPasswordPermission.php | 7 +- ProcessMaker/Models/User.php | 6 +- ProcessMaker/Rules/PlainText.php | 25 ++ .../users/components/DeletedUsersListing.vue | 5 +- .../admin/users/components/UsersListing.vue | 5 +- resources/views/profile/edit.blade.php | 35 ++- tests/Feature/Api/UsersTest.php | 225 +++++++++++++++++- .../unit/ProcessMaker/Rules/PlainTextTest.php | 46 ++++ 9 files changed, 404 insertions(+), 12 deletions(-) create mode 100644 ProcessMaker/Rules/PlainText.php create mode 100644 tests/unit/ProcessMaker/Rules/PlainTextTest.php diff --git a/ProcessMaker/Http/Controllers/Api/UserController.php b/ProcessMaker/Http/Controllers/Api/UserController.php index d08e3374db..32b5067aff 100644 --- a/ProcessMaker/Http/Controllers/Api/UserController.php +++ b/ProcessMaker/Http/Controllers/Api/UserController.php @@ -35,6 +35,39 @@ class UserController extends Controller public $doNotSanitize = [ 'username', // has alpha_dash rule 'password', + 'firstname', // validated as plain text by User::rules() + 'lastname', // validated as plain text by User::rules() + 'title', // validated as plain text by User::rules() + ]; + + /** + * Fields accepted when a non-administrative user updates their own profile. + * + * @var array + */ + private const SELF_SERVICE_UPDATE_FIELDS = [ + 'username', + 'password', + 'firstname', + 'lastname', + 'title', + 'email', + 'address', + 'city', + 'state', + 'postal', + 'country', + 'phone', + 'fax', + 'cell', + 'timezone', + 'datetime_format', + 'status', + 'avatar', + 'preferences_2fa', + 'connected_accounts', + 'meta', + 'valpassword', ]; /** @@ -452,12 +485,14 @@ public function getPinnnedControls(User $user) */ public function update(User $user, Request $request) { - if (!Auth::user()->can('edit', $user)) { + $authenticatedUser = Auth::user(); + if (!$authenticatedUser->can('edit', $user)) { throw new AuthorizationException(__('Not authorized to update this user.')); } - $request->validate(User::rules($user)); $fields = $request->json()->all(); + $this->authorizeSelfServiceUpdate($authenticatedUser, $user, $fields); + $request->validate(User::rules($user)); if (isset($fields['password'])) { $fields['password'] = Hash::make($fields['password']); $fields['password_changed_at'] = Carbon::now()->toDateTimeString(); @@ -564,6 +599,29 @@ public function update(User $user, Request $request) return response([], 204); } + /** + * Authorize and constrain self-service profile updates. + */ + private function authorizeSelfServiceUpdate(User $authenticatedUser, User $targetUser, array $fields): void + { + $isSelfServiceUpdate = $authenticatedUser->id === $targetUser->id + && !$authenticatedUser->is_administrator + && !$authenticatedUser->hasPermission('edit-users'); + + if (!$isSelfServiceUpdate) { + return; + } + + if (!$authenticatedUser->hasPermission('edit-personal-profile')) { + throw new AuthorizationException(__('Not authorized to update this user.')); + } + + $disallowedFields = array_diff(array_keys($fields), self::SELF_SERVICE_UPDATE_FIELDS); + if ($disallowedFields !== []) { + throw new AuthorizationException(__('Not authorized to update one or more user fields.')); + } + } + /** * Validate the phone number for SMS two-factor authentication. * diff --git a/ProcessMaker/Http/Middleware/ValidateEditUserAndPasswordPermission.php b/ProcessMaker/Http/Middleware/ValidateEditUserAndPasswordPermission.php index 416abdbb32..b9375fc620 100644 --- a/ProcessMaker/Http/Middleware/ValidateEditUserAndPasswordPermission.php +++ b/ProcessMaker/Http/Middleware/ValidateEditUserAndPasswordPermission.php @@ -15,8 +15,11 @@ public function handle(Request $request, Closure $next) { $user = $request->route('user'); $fields = $request->json()->all(); - if (($fields['username'] !== $user->getAttribute('username') || in_array('password', $fields)) && - !Auth::user()->hasPermission('edit-user-and-password') && !Auth::user()->is_administrator) { + $usernameChanged = array_key_exists('username', $fields) + && $fields['username'] !== $user->getAttribute('username'); + $passwordChanged = array_key_exists('password', $fields); + if (($usernameChanged || $passwordChanged) && + !Auth::user()->hasPermission('edit-user-and-password') && !Auth::user()->is_administrator) { throw new AuthorizationException(__('Not authorized to update the username and password.')); } diff --git a/ProcessMaker/Models/User.php b/ProcessMaker/Models/User.php index c13c133659..87a8ed4487 100644 --- a/ProcessMaker/Models/User.php +++ b/ProcessMaker/Models/User.php @@ -16,6 +16,7 @@ use ProcessMaker\Models\EmptyModel; use ProcessMaker\Notifications\ResetPassword as ResetPasswordNotification; use ProcessMaker\Query\Traits\PMQL; +use ProcessMaker\Rules\PlainText; use ProcessMaker\Rules\StringHasAtLeastOneUpperCaseCharacter; use ProcessMaker\Traits\Exportable; use ProcessMaker\Traits\HasAuthorization; @@ -184,9 +185,10 @@ public static function rules(self $existing = null) return [ // The following characters where not included in the regexp: & % ' " ? / 'username' /****/ => ['required', 'regex:/^[a-zA-Z0-9.!#$*+=^_`|~\-@]+$/', 'min:2', 'max:255', $unique], - 'firstname' /***/ => ['required', 'max:50'], - 'lastname' /****/ => ['required', 'max:50'], + 'firstname' /***/ => ['required', 'max:50', new PlainText()], + 'lastname' /****/ => ['required', 'max:50', new PlainText()], 'email' /*******/ => ['required', 'email'], + 'title' /*******/ => ['nullable', 'max:255', new PlainText()], 'birthdate' /***/ => ['nullable', 'date'], 'phone' /*******/ => ['nullable', 'regex:/^[+\.0-9x\)\(\-\s\/]*$/'], 'fax' /*********/ => ['nullable', 'regex:/^[+\.0-9x\)\(\-\s\/]*$/'], diff --git a/ProcessMaker/Rules/PlainText.php b/ProcessMaker/Rules/PlainText.php new file mode 100644 index 0000000000..f3a5fb76e2 --- /dev/null +++ b/ProcessMaker/Rules/PlainText.php @@ -0,0 +1,25 @@ + {{ props.rowData.username }} + @@ -93,7 +96,7 @@ export default { }, { title: () => this.$t("Full Name"), - name: "fullname", + name: "__slot:fullname", sortField: "fullname" }, { diff --git a/resources/js/admin/users/components/UsersListing.vue b/resources/js/admin/users/components/UsersListing.vue index 36d62668c9..c0794bccdf 100644 --- a/resources/js/admin/users/components/UsersListing.vue +++ b/resources/js/admin/users/components/UsersListing.vue @@ -23,6 +23,9 @@ + @@ -98,7 +101,7 @@ export default { }, { title: () => this.$t("Full Name"), - name: "fullname", + name: "__slot:fullname", sortField: "fullname" }, { diff --git a/resources/views/profile/edit.blade.php b/resources/views/profile/edit.blade.php index d7d24757a2..6ba7525187 100644 --- a/resources/views/profile/edit.blade.php +++ b/resources/views/profile/edit.blade.php @@ -128,6 +128,30 @@ } } }; + const SELF_SERVICE_PROFILE_FIELDS = [ + 'username', + 'password', + 'firstname', + 'lastname', + 'title', + 'email', + 'address', + 'city', + 'state', + 'postal', + 'country', + 'phone', + 'fax', + 'cell', + 'timezone', + 'datetime_format', + 'status', + 'avatar', + 'preferences_2fa', + 'connected_accounts', + 'meta', + 'valpassword', + ]; let formVueInstance = new Vue({ el: '#editProfile', mixins:addons, @@ -251,6 +275,15 @@ closeModal() { $('#validateModal').modal('hide'); }, + profilePayload() { + return SELF_SERVICE_PROFILE_FIELDS.reduce((payload, field) => { + if (Object.prototype.hasOwnProperty.call(this.formData, field)) { + payload[field] = this.formData[field]; + } + + return payload; + }, {}); + }, saveProfileChanges() { this.resetErrors(); if (@json($enabled2FA) && this.global2FAEnabled.length === 0) { @@ -270,7 +303,7 @@ if (this.image === false) { this.formData.avatar = false; } - ProcessMaker.apiClient.put('users/' + this.formData.id, this.formData) + ProcessMaker.apiClient.put('users/' + this.formData.id, this.profilePayload()) .then((response) => { // reset the slack configuration error this.slackConfigurationError = false; diff --git a/tests/Feature/Api/UsersTest.php b/tests/Feature/Api/UsersTest.php index c1dacd9e18..8c58db83fb 100644 --- a/tests/Feature/Api/UsersTest.php +++ b/tests/Feature/Api/UsersTest.php @@ -5,6 +5,7 @@ use Database\Seeders\PermissionSeeder; use Faker\Factory as Faker; use Illuminate\Http\UploadedFile; +use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Redis; use ProcessMaker\Models\Group; use ProcessMaker\Models\GroupMember; @@ -72,6 +73,18 @@ public function getUpdatedData() ]; } + private function getSelfServiceUpdateData(User $user, array $overrides = []): array + { + return array_merge([ + 'username' => $user->username, + 'firstname' => $user->firstname, + 'lastname' => $user->lastname, + 'title' => $user->title, + 'email' => $user->email, + 'status' => $user->status, + ], $overrides); + } + protected function withUserSetup() { $this->user->is_administrator = true; @@ -823,14 +836,17 @@ public function testUpdateUserNotAdmin() // Validate the header status code $response->assertStatus(403); - // With permission + // With both permissions + $this->user->giveDirectPermission('edit-personal-profile'); $this->user->giveDirectPermission('edit-user-and-password'); $this->user->save(); $this->user->refresh(); $this->flushSession(); - $updateData = $this->getUpdatedData(); - $updateData['email'] = $verify['email']; + $updateData = $this->getSelfServiceUpdateData($this->user, [ + 'username' => 'newusername', + 'firstname' => 'Updated', + ]); // Post saved success $response = $this->apiCall('PUT', $url, $updateData); @@ -845,6 +861,209 @@ public function testUpdateUserNotAdmin() $this->assertNotEquals($verify, $verifyNew); } + public function testUserWithoutProfilePermissionCannotUpdateTheirOwnProfile(): void + { + $this->user = User::factory()->create(['is_administrator' => false, 'status' => 'ACTIVE']); + $this->flushSession(); + $originalFirstname = $this->user->firstname; + + $response = $this->apiCall( + 'PUT', + self::API_TEST_URL . '/' . $this->user->id, + $this->getSelfServiceUpdateData($this->user, ['firstname' => 'Unauthorized']) + ); + + $response->assertStatus(403); + $this->assertDatabaseHas('users', [ + 'id' => $this->user->id, + 'firstname' => $originalFirstname, + ]); + } + + public function testUserWithProfilePermissionCanUpdateAllowedFields(): void + { + $this->user = User::factory()->create(['is_administrator' => false, 'status' => 'ACTIVE']); + $this->user->giveDirectPermission('edit-personal-profile'); + $this->user->refresh(); + $this->flushSession(); + + $response = $this->apiCall( + 'PUT', + self::API_TEST_URL . '/' . $this->user->id, + $this->getSelfServiceUpdateData($this->user, [ + 'firstname' => 'Éléazar', + 'lastname' => 'Reséndez', + 'title' => 'Research & Development', + ]) + ); + + $response->assertStatus(204); + $this->assertDatabaseHas('users', [ + 'id' => $this->user->id, + 'firstname' => 'Éléazar', + 'lastname' => 'Reséndez', + 'title' => 'Research & Development', + ]); + } + + public function testSelfServiceUpdateRejectsDisallowedFieldsWithoutPersistingChanges(): void + { + $this->user = User::factory()->create(['is_administrator' => false, 'status' => 'ACTIVE']); + $this->user->giveDirectPermission('edit-personal-profile'); + $this->user->refresh(); + $this->flushSession(); + $originalFirstname = $this->user->firstname; + $disallowedFields = [ + 'manager_id' => User::factory()->create()->id, + 'delegation_user_id' => User::factory()->create()->id, + 'schedule' => ['monday' => []], + 'force_change_password' => true, + 'is_administrator' => true, + ]; + + foreach ($disallowedFields as $field => $value) { + $response = $this->apiCall( + 'PUT', + self::API_TEST_URL . '/' . $this->user->id, + $this->getSelfServiceUpdateData($this->user, [ + 'firstname' => 'Should Not Persist', + $field => $value, + ]) + ); + + $response->assertStatus(403); + } + + $this->assertDatabaseHas('users', [ + 'id' => $this->user->id, + 'firstname' => $originalFirstname, + 'manager_id' => null, + 'delegation_user_id' => null, + 'force_change_password' => false, + 'is_administrator' => false, + ]); + } + + public function testUsernameUpdateRequiresCredentialPermission(): void + { + $this->user = User::factory()->create(['is_administrator' => false, 'status' => 'ACTIVE']); + $this->user->giveDirectPermission('edit-personal-profile'); + $this->user->refresh(); + $this->flushSession(); + $originalUsername = $this->user->username; + $payload = $this->getSelfServiceUpdateData($this->user, ['username' => 'four32745-username']); + + $response = $this->apiCall('PUT', self::API_TEST_URL . '/' . $this->user->id, $payload); + $response->assertStatus(403); + $this->assertDatabaseHas('users', ['id' => $this->user->id, 'username' => $originalUsername]); + + $this->user->giveDirectPermission('edit-user-and-password'); + $this->user->refresh(); + $this->flushSession(); + + $response = $this->apiCall('PUT', self::API_TEST_URL . '/' . $this->user->id, $payload); + $response->assertStatus(204); + $this->assertDatabaseHas('users', ['id' => $this->user->id, 'username' => 'four32745-username']); + } + + public function testPasswordUpdateRequiresCredentialPermission(): void + { + $this->user = User::factory()->create(['is_administrator' => false, 'status' => 'ACTIVE']); + $this->user->giveDirectPermission('edit-personal-profile'); + $this->user->refresh(); + $this->flushSession(); + $password = $this->makePassword(); + $originalPassword = $this->user->password; + $payload = $this->getSelfServiceUpdateData($this->user, ['password' => $password]); + + $response = $this->apiCall('PUT', self::API_TEST_URL . '/' . $this->user->id, $payload); + $response->assertStatus(403); + $this->assertSame($originalPassword, $this->user->fresh()->password); + + $this->user->giveDirectPermission('edit-user-and-password'); + $this->user->refresh(); + $this->flushSession(); + + $response = $this->apiCall('PUT', self::API_TEST_URL . '/' . $this->user->id, $payload); + $response->assertStatus(204); + $this->assertTrue(Hash::check($password, $this->user->fresh()->password)); + } + + public function testUserEditorCanUpdateAnotherUserThroughAdministrativeFlow(): void + { + $this->user = User::factory()->create(['is_administrator' => false, 'status' => 'ACTIVE']); + $this->user->giveDirectPermission('edit-users'); + $this->user->refresh(); + $this->flushSession(); + $targetUser = User::factory()->create(['status' => 'ACTIVE']); + + $response = $this->apiCall( + 'PUT', + self::API_TEST_URL . '/' . $targetUser->id, + $this->getSelfServiceUpdateData($targetUser, [ + 'firstname' => 'Administrative', + 'manager_id' => $this->user->id, + ]) + ); + + $response->assertStatus(204); + $this->assertDatabaseHas('users', [ + 'id' => $targetUser->id, + 'firstname' => 'Administrative', + 'manager_id' => $this->user->id, + ]); + } + + public function testUserProfileMarkupIsRejectedWithoutChangingStoredValues(): void + { + $user = User::factory()->create([ + 'firstname' => 'Original First', + 'lastname' => 'Original Last', + 'title' => 'Original Title', + 'status' => 'ACTIVE', + ]); + $payloads = [ + ['firstname', '<img src=x onerror=alert(document.domain)>'], + ['lastname', ''], + ['title', ''], + ['title', ''], + ]; + + foreach ($payloads as [$field, $markup]) { + $response = $this->apiCall( + 'PUT', + self::API_TEST_URL . '/' . $user->id, + $this->getSelfServiceUpdateData($user, [$field => $markup]) + ); + + $response->assertStatus(422)->assertJsonValidationErrors($field); + } + + $this->assertDatabaseHas('users', [ + 'id' => $user->id, + 'firstname' => 'Original First', + 'lastname' => 'Original Last', + 'title' => 'Original Title', + ]); + } + + public function testUserCreationRejectsProfileMarkup(): void + { + $response = $this->apiCall('POST', self::API_TEST_URL, [ + 'username' => 'four32745qa', + 'firstname' => '<img src=x onerror=alert(document.domain)>', + 'lastname' => 'FOUR-32745 QA', + 'title' => '', + 'email' => 'four32745qa@example.invalid', + 'status' => 'ACTIVE', + 'password' => $this->makePassword(), + ]); + + $response->assertStatus(422)->assertJsonValidationErrors(['firstname', 'title']); + $this->assertDatabaseMissing('users', ['username' => 'four32745qa']); + } + public function testDisableRecommendations() { RecommendationUser::factory()->create([ diff --git a/tests/unit/ProcessMaker/Rules/PlainTextTest.php b/tests/unit/ProcessMaker/Rules/PlainTextTest.php new file mode 100644 index 0000000000..ce414b2609 --- /dev/null +++ b/tests/unit/ProcessMaker/Rules/PlainTextTest.php @@ -0,0 +1,46 @@ +img src=x onerror=alert(document.domain)>', + '', + '', + '', + '', + '', + ]; + + foreach ($payloads as $payload) { + $validator = Validator::make(['value' => $payload], ['value' => [new PlainText()]]); + + $this->assertTrue($validator->fails(), $payload); + } + } + + public function testItAcceptsPlainText(): void + { + $values = [ + 'Éléazar Reséndez', + 'Research & Development', + 'VP < Sales', + '<img src=x>', + 'javascript:alert(document.domain)', + ]; + + foreach ($values as $value) { + $validator = Validator::make(['value' => $value], ['value' => [new PlainText()]]); + + $this->assertFalse($validator->fails(), $value); + } + } +} From b5f87924d3465f92d8e3345940d1fb609f665370 Mon Sep 17 00:00:00 2001 From: Eleazar Resendez Date: Fri, 28 Aug 2026 10:03:45 -0600 Subject: [PATCH 2/3] fix(users): close remaining stored XSS paths --- .../Http/Controllers/Api/UserController.php | 91 +++++++--- .../groups/components/UsersInGroupListing.vue | 11 +- .../users/components/DeletedUsersListing.vue | 6 +- .../admin/users/components/UsersListing.vue | 6 +- resources/views/profile/edit.blade.php | 9 +- tests/Feature/Api/UsersTest.php | 162 ++++++++++++++++++ 6 files changed, 253 insertions(+), 32 deletions(-) diff --git a/ProcessMaker/Http/Controllers/Api/UserController.php b/ProcessMaker/Http/Controllers/Api/UserController.php index 32b5067aff..c1dd318b8f 100644 --- a/ProcessMaker/Http/Controllers/Api/UserController.php +++ b/ProcessMaker/Http/Controllers/Api/UserController.php @@ -70,6 +70,15 @@ class UserController extends Controller 'valpassword', ]; + /** + * Metadata fields accepted during a self-service profile update. + * + * @var array + */ + private const SELF_SERVICE_META_FIELDS = [ + 'disableRecommendations', + ]; + /** * Display a listing of the resource. * @@ -491,8 +500,16 @@ public function update(User $user, Request $request) } $fields = $request->json()->all(); - $this->authorizeSelfServiceUpdate($authenticatedUser, $user, $fields); - $request->validate(User::rules($user)); + $isSelfServiceUpdate = $this->authorizeSelfServiceUpdate($authenticatedUser, $user, $fields); + $rules = User::rules($user); + if ($isSelfServiceUpdate) { + $rules['meta'] = ['sometimes', 'array']; + $rules['meta.disableRecommendations'] = ['sometimes', 'boolean']; + } + $request->validate($rules); + if ($isSelfServiceUpdate) { + $fields = $this->normalizeSelfServiceMeta($user, $fields); + } if (isset($fields['password'])) { $fields['password'] = Hash::make($fields['password']); $fields['password_changed_at'] = Carbon::now()->toDateTimeString(); @@ -501,6 +518,7 @@ public function update(User $user, Request $request) session()->forget('login-error'); } $original = $user->getOriginal(); + $isLdapUser = $user->meta?->authenticationType === 'ldap'; $user->fill($fields); if (array_key_exists('cell', $fields)) { $response = $this->validateCellPhoneNumber($user, $fields['cell']); @@ -509,28 +527,24 @@ public function update(User $user, Request $request) } } if ($fields['email'] !== $original['email']) { + $ssoUser = $isLdapUser; if (class_exists(SsoUser::class)) { // Check if the user is an SSO user (including SAML) - $ssoUser = SsoUser::where('user_id', $user->id)->exists(); - - // Check if the user is an LDAP user - if (isset($user->meta?->authenticationType) && $user->meta->authenticationType === 'ldap') { - $ssoUser = true; - } - if ($ssoUser) { - return response([ - 'message' => __( - "The email can't be edited. This action is only available for SSO-synced users." - ), - 'errors' => [ - 'email' => [ - __( - "The email can't be edited. This action is only available for SSO-synced users." - ), - ], + $ssoUser = $ssoUser || SsoUser::where('user_id', $user->id)->exists(); + } + if ($ssoUser) { + return response([ + 'message' => __( + "The email can't be edited. This action is only available for SSO-synced users." + ), + 'errors' => [ + 'email' => [ + __( + "The email can't be edited. This action is only available for SSO-synced users." + ), ], - ], 422); - } + ], + ], 422); } if (!isset($fields['valpassword'])) { return response([ @@ -602,14 +616,14 @@ public function update(User $user, Request $request) /** * Authorize and constrain self-service profile updates. */ - private function authorizeSelfServiceUpdate(User $authenticatedUser, User $targetUser, array $fields): void + private function authorizeSelfServiceUpdate(User $authenticatedUser, User $targetUser, array $fields): bool { $isSelfServiceUpdate = $authenticatedUser->id === $targetUser->id && !$authenticatedUser->is_administrator && !$authenticatedUser->hasPermission('edit-users'); if (!$isSelfServiceUpdate) { - return; + return false; } if (!$authenticatedUser->hasPermission('edit-personal-profile')) { @@ -620,6 +634,37 @@ private function authorizeSelfServiceUpdate(User $authenticatedUser, User $targe if ($disallowedFields !== []) { throw new AuthorizationException(__('Not authorized to update one or more user fields.')); } + + if (isset($fields['meta']) && is_array($fields['meta'])) { + $disallowedMetaFields = array_diff(array_keys($fields['meta']), self::SELF_SERVICE_META_FIELDS); + if ($disallowedMetaFields !== []) { + throw new AuthorizationException(__('Not authorized to update one or more user fields.')); + } + } + + return true; + } + + /** + * Merge self-service metadata into the persisted server-managed values. + */ + private function normalizeSelfServiceMeta(User $user, array $fields): array + { + if (!array_key_exists('meta', $fields)) { + return $fields; + } + + $meta = (array) $user->meta; + if (array_key_exists('disableRecommendations', $fields['meta'])) { + if ($fields['meta']['disableRecommendations']) { + $meta['disableRecommendations'] = true; + } else { + unset($meta['disableRecommendations']); + } + } + $fields['meta'] = $meta ?: null; + + return $fields; } /** diff --git a/resources/js/admin/groups/components/UsersInGroupListing.vue b/resources/js/admin/groups/components/UsersInGroupListing.vue index 66e32471b7..c52d4736ad 100644 --- a/resources/js/admin/groups/components/UsersInGroupListing.vue +++ b/resources/js/admin/groups/components/UsersInGroupListing.vue @@ -15,6 +15,9 @@ > +