Multiple errors in PasswordItem::preSave(), leads to data loss if field is translatable

Created on 29 June 2015, almost 9 years ago
Updated 29 March 2024, 3 months ago

Problem/Motivation

The implementation of PasswordItem::preSave() contains multiple errors.

if ($entity->isNew()) {
  $this->value = \Drupal::service('password')->hash(trim($this->value));
  if (!$this->value) {
    throw new EntityMalformedException('The entity does not have a password.');
  }
}

If an empty string is passed as password value for a new entity, the value that gets stored is the hash of an empty string. So the value is set instead of throwing an Exception with 'The entity does not have a password.'.

Now to something really dangerous:
The PasswordItem is a normal field. For example it can be used to store a password on an entity that implements a custom web service. Therefor this field could be translatable.
Saving an empty password on an existing entity means keeping the password that is stored in database.

if (!$entity->isNew()) {
  // If the password is empty, that means it was not changed, so use the
  // original password.
  if (empty($this->value)) {
    $this->value = $entity->original->{$this->getFieldDefinition()->getName()}->value;
  }
}

What happens here is that all passwords of all translations are set to the value of the "untranslated" entity on save!

Proposed resolution

PasswordItem::preSave() needs to be completely rewritten to deal with Translations and to remove the code that is erroneous even for untranslated password fields.

Remaining tasks

Review the existing patch.

User interface changes

none

API changes

none

Data model changes

none

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Field 

Last updated about 5 hours ago

Created by

🇩🇪Germany mkalkbrenner 🇩🇪

Live updates comments and jobs are added and updated live.
  • D8MI

    (Drupal 8 Multilingual Initiative) is the tag used by the multilingual initiative to mark core issues (and some contributed module issues). For versions other than Drupal 8, use the i18n (Internationalization) tag on issues which involve or affect multilingual / multinational support. That is preferred over Translation.

  • Security

    It is used for security vulnerabilities which do not need a security advisory. For example, security issues in projects which do not have security advisory coverage, or forward-porting a change already disclosed in a security advisory. See Drupal’s security advisory policy for details. Be careful publicly disclosing security vulnerabilities! Use the “Report a security vulnerability” link in the project page’s sidebar. See how to report a security issue for details.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇺🇸United States smustgrave

    Still not entirely sure the issue but since there hasn't been a follow up to #49 in a year going to close for now. If still a valid bug please reopen, adding steps to issue summary for D10+

    Thanks!

  • Status changed to Needs work 3 months ago
  • 🇷🇴Romania amateescu

    Even though there are no steps to reproduce in core because the user's password field is not translatable, I think the patches and all the discussion in the issue highlight the deficiencies of PasswordItem, so I think the issue is still valid, just needs someone to pick it up :)

Production build 0.69.0 2024