Skip to content

Fix: validate item type and rights in lifecycle actions - #213

Open
Rom1-B wants to merge 1 commit into
mainfrom
fix_validation
Open

Fix: validate item type and rights in lifecycle actions#213
Rom1-B wants to merge 1 commit into
mainfrom
fix_validation

Conversation

@Rom1-B

@Rom1-B Rom1-B commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Checklist before requesting a review

Please delete options that are not relevant.

  • I have performed a self-review of my code.
  • I have added tests (when available) that prove my fix is effective or that my feature works.
    (no automated test suite exists in this plugin)
  • I have updated the CHANGELOG with a short functional description of the fix or new feature.
  • This change requires a documentation update.

Description

  • fixes #N/A
  • The uninstall and replace actions now check that the target item type is one of the supported asset types, and that the current user actually has the required rights on each processed item, before applying any change, instead of relying only on the plugin's global right.
  • A couple of form fields were echoed without escaping and are now properly escaped, and an identifier used in a legacy query is now cast to an integer before being used.

Screenshots (if appropriate):

@Rom1-B
Rom1-B requested a review from stonebuzz August 25, 2026 10:02
Comment thread inc/replace.class.php
$olditem = new $type();
$olditem->getFromDB($olditem_id);
if (!$olditem->getFromDB($olditem_id) || !$olditem->can($olditem_id, UPDATE)) {
continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New rights checks continue silently when an item fails, leaving the caller with no visibility into partial failures. $count is already incremented before the check (line 110), so the progress output counts skipped items as processed, and front/action.php unconditionally outputs "Replacement successful" after the call returns. A user replacing 10 items where 3 are denied gets the same feedback as a fully successful run.

The mass-action handler in inc/uninstall.class.php correctly uses MassiveAction::ACTION_NORIGHT; the replace() method should at minimum track and display skipped items in its HTML table, or return a count of failures to the caller.

Comment thread front/action.php
$model->getConfig($_REQUEST["model_id"]);

//Case of a uninstallation initiated from the object form
if (isset($_REQUEST["uninstall"])) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miisng check for action "uninstall" (like L60)

Comment thread front/action.php
);
Html::footer();
}
} elseif ($model->fields['types_id'] == PluginUninstallModel::TYPE_MODEL_UNINSTALL) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miisng check for action "uninstall" (like L60)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants