EasyAdminBundle icon indicating copy to clipboard operation
EasyAdminBundle copied to clipboard

[v4.6.0] Security issue with ajaxEdit

Open tomxw opened this issue 2 years ago • 1 comments

Currently it is possible for users to modify the "fieldName" in the XMLHttpRequest to any field of their own choosing and update its value.

  • When you try to modify a boolean that is not displayed on the index page, you will succeed.
  • When you try to modify a property that is not a boolean it overwrites the value with an empty string.

The problem is that within \EasyCorp\Bundle\EasyAdminBundle\Controller\AbstractCrudController::ajaxEdit it is not checked whether $propertyName:

  • Is a property for which modification is actually allowed
  • Is a BooleanField that is displayed on the index page and is rendered as a switch

We solved the issue for ourselves by extending the \EasyCorp\Bundle\EasyAdminBundle\Controller\AbstractCrudController and adding the following code to it:

protected function ajaxEdit(EntityDto $entityDto, ?string $propertyName, bool $newValue): AfterCrudActionEvent
{
    if (!$this->isAjaxEditAllowed($propertyName)) {
        throw new AccessDeniedException(
            sprintf('Editing property "%s" of entity "%s" is not allowed', $propertyName, $entityDto->getFqcn())
        );
    }
    return parent::ajaxEdit($entityDto, $propertyName, $newValue);
}

protected function isAjaxEditAllowed(?string $propertyName): bool
{
    $fields = FieldCollection::new($this->configureFields(Crud::PAGE_INDEX));
    if (is_string($propertyName)) {
        $field = $fields->getByProperty($propertyName);
        if (
            $field instanceof FieldDto &&
            $field->isDisplayedOn(Action::INDEX) &&
            $field->getFieldFqcn() === BooleanField::class &&
            $field->getCustomOption(BooleanField::OPTION_RENDER_AS_SWITCH)
        ) {
            return true;
        }
    }
    return false;
}

tomxw avatar Mar 20 '23 15:03 tomxw

As of v4.9.4 the security issue seems to be fixed, although I'm not entirely convinced about the way this was fixed.

The ajax edit is now validating whether a field can be updated based on the fields defined for the edit action. This limitation is a bit strange, in our case we don't have an edit action, we only have a switch on the list page.

It should be possible to define a field on the list without having it in the edit page as well I think. Permissions should be checked on the presence in the list as a switch rather than on the edit page.

tomxw avatar Mar 28 '24 14:03 tomxw