[1.0.x] File Rename

Thank you for applying! Reviewers will review the project files, describing what needs to be changed.

Please read Review process for security advisory coverage: What to expect for more details and Security advisory coverage application checklist to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.

To reviewers: Please read How to review security advisory coverage applications, What to cover in an application review, and Drupal.org security advisory coverage application workflow.

While this application is open, only the user who opened the application can make commits to the project used for the application.

Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.

1. Fix PHPCS issues.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml file_rename/

FILE: file_rename/file_rename.module
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 172 | ERROR | Type hint "array" missing for $element
 172 | ERROR | Type hint "array" missing for $form
----------------------------------------------------------------------


FILE: file_rename/README.md
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 47 | WARNING | Line exceeds 80 characters; contains 127 characters
----------------------------------------------------------------------


FILE: file_rename/src/Form/SettingsForm.php
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 8 | ERROR | Missing short description in doc comment
---------------------------------------------------------------------------

2. FILE: file_rename.info.yml

core_version_requirement: ^8 || ^9 || ^10

The Drupal Core versions before 8.7.7 do not recognize the core_version_requirement: key.

@granik,

I have reviewed the changes, and they look fine to me.

Let’s wait for other reviewers to take a look and if everything goes fine, you will get the role.

Thanks

manual review:

1. config schema is missing, see https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-...
2. where do you validate that the new file name does not contain slashes etc.? The permissions is restricted to trusted users only, but would be still good to run the usual file name validation in FileRenameForm. You need to do something similar as in FileUploadHandler::handleFileUpload() where a FileUploadSanitizeNameEvent is dispatched. Your are already restricting the file length, so that should be fine.

Not a hard security blocker, but I think sanitizing the file name would be good to rule out any issues there.

Ah great - overlooked that! Then I think only the rest of file name validation is missing like NULL bytes and dots for hidden files. Check SecurityFileUploadEventSubscriber::sanitizeName().

There could also be other event subscribers on the file name security event, so I think it is important to invoke it.

Not sure where Drupal core removes forward slashes and backslashes (or if that is handled by PHP in the upload somehow), but probably makes sense for you to forbid both of them.

Hi,

According to EntityFormInterface, the save entity should return an integer value:

   * @return int
   *   Either SAVED_NEW or SAVED_UPDATED, depending on the operation performed.
   */
  public function save(array $form, FormStateInterface $form_state);

but in Form\FileRenameForm, save() method returns also NULL:

  public function save(array $form, FormStateInterface $form_state) {
    $pathinfo = pathinfo($this->entity->getFileUri());
    $filename_new = $form_state->getValue('new_filename') . '.' . $pathinfo['extension'];

    if ($filename_new != $this->entity->getFilename()) {
      $filepath_new = str_replace($this->entity->getFilename(), $filename_new, $this->entity->getFileUri());
      // Invoke pre-rename hooks.
      $this->moduleHandler->invokeAll('file_prerename', [$this->entity]);
      // Rename existing file.
      $this->fileSystem->move($this->entity->getFileUri(), $filepath_new, FileSystemInterface::EXISTS_REPLACE);
      $log_args = [
        '%old' => $this->entity->getFilename(),
        '%new' => $filename_new,
      ];
      // Update file entity.
      $this->entity->setFilename($filename_new);
      $this->entity->setFileUri($filepath_new);
      $status = $this->entity->save();
      // Notify and log.
      $this->messenger()->addStatus($this->t('File %old was renamed to %new', $log_args));
      $this->logger('file_entity')->info($this->t('File %old renamed to %new'), $log_args);
      // Invoke hooks if there are some.
      $this->moduleHandler->invokeAll('file_rename', [$this->entity]);

      return $status;
    }
    return NULL;
  }

Great @granik

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the Slack #contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the reviewers.

Congrats granik, happy to see this one approved!