Skip to content

Conversation

@GrozaZool
Copy link

Исправление сортировки по своим полям.
Без этой правки нельзя сортировать например по id DESC.

@Leonix
Copy link
Contributor

Leonix commented Jan 20, 2025

Выглядит так, что теперь возможен SQL injection произвольной строки через параметр 'order'. Без валидации так нельзя.

@GrozaZool
Copy link
Author

@Leonix order_by во первых, разбивается explode'ом, и пересобираеться в $collection->orderBy.
хотелось бы посмотреть на попытки что-нибудь тут сделать. Ну а во вторых - это узкая функция управляемая кодом, а не пользовательским вводом. Проблемы я тут не вижу.
Если я ошибаюсь, уделите пару минут времени, просвятите

@WinterSilence
Copy link
Contributor

@GrozaZool он прав, достаточно взглянуть на \teamGroupAction::execute() - там значение order прилетает из запроса, добавь просто очистку значения

        if ($order_by) {
            $order_by = preg_replace('/[^\w\s]+/', '', $order_by);
        } else {
            $order_by = 'name ASC';
        }

@GrozaZool
Copy link
Author

Допустим, мы предпочтем этот вариант, нежели замечать что в $collection->orderBy оно попадет разбитый explode'ом как 2 элемента.
Повторюсь, я бы очень хотел посмотреть на то, как тет можно было бы вызвать SQL injection
teamUser::getList('users/all', ['order' => 'YOUR_SQL_INJECTION'])
Кто-то может привести YOUR_SQL_INJECTION чтоб я понял что не прав?)

ведь из id DESC; DROP TABLE table остается только id[0] DESC[1].

@Leonix
Copy link
Contributor

Leonix commented May 28, 2025

Ну, я бы, конечно, предпочёл, чтобы глядя на код было очевидно, что Injection невозможен, потому что аргумент "никто пока не нашёл" - не очень сильный аргумент.

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.

3 participants