Repository navigation
fix(order): закрыть служебные колонки черновика от записи с витрины (#860) - #896
Merged
Merged
Conversation
…860) Web API принимал произвольный key и писал его в черновик, если такая колонка есть у заказа или адреса. Посетитель с токеном своей корзины подставлял чужой customer_id, при оформлении он уже не перезаписывается, и заказ уезжал в чужой личный кабинет. Тем же путём открывались cost, status_id, num, token и properties. OrderFieldManager::add() и remove() теперь отклоняют служебные колонки. Гард стоит в менеджере полей, а не в контроллере: через него проходят все витринные пути — order/add, order/set, order/remove, data в order/submit и сниппет ms3_order. Проверка до msOnBeforeAddToOrder, плагином её не обойти. Список запретный, не разрешительный: кастомные колонки, заведённые через extra fields, остаются писаемыми, а ключи вне моделей по-прежнему уходят в properties['_validated']. Константы читаются через static::, поэтому магазин может переопределить списки в подклассе и подставить его в ms3_order_field_manager через core/config/ms3.services.d/ — этот каталог живёт вне компонента и переживает обновление. Вторая половина issue: updateField() писал один ключ сразу в две таблицы, если колонка есть и у заказа, и у адреса (id, createdon, updatedon, properties). Из-за этого properties адреса затирались свойствами заказа — в том числе при обычном выборе сохранённого адреса, где properties заказа передаёт сам OrderAddressManager. Теперь заказ главнее и запись идёт в одну таблицу. Изменение поведения: кастомный фронт, который писал delivery_cost через order/add, получит ошибку — именно через это посетитель мог выставить себе нулевую доставку. Штатная поставка такого не делает, стоимость считают DeliveryService и OrderCostCalculator.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #860
Проблема
POST /api/v1/order/addпринимает произвольныйkeyи пишет его в черновик, если такая колонка есть у заказа или адреса. Белого списка полей формы нет.Посетитель с токеном своей корзины подставляет чужой
customer_id. При оформлении он не перезаписывается —OrderSubmitHandlerподставляет покупателя только если поле пустое. Заказ уезжает в чужой личный кабинет. Тем же путём открытыcost,cart_cost,delivery_cost,status_id,num,token,uuid,properties.Вторая половина:
updateField()писал один ключ сразу в две таблицы, если колонка есть и у заказа, и у адреса. Общие колонки —id,createdon,updatedon,properties.Решение
Гард в
OrderFieldManager::add()иremove(), а не в контроллере. Через менеджер полей проходят все витринные пути записи:POST /api/v1/order/addOrder::add()→fieldManager->add()POST /api/v1/order/setOrder::set()→ тот жеadd()в циклеPOST /api/v1/order/submitOrderSubmitHandlerпрогоняетdataчерезadd()— в issue не упомянуто, дыра та жеPOST /api/v1/order/removeremove('token')обнулял привязку заказа к сессииms3_order$ms3->order->add()Проверка стоит до
msOnBeforeAddToOrder— плагином её не обойти.Почему список запретный, а не разрешительный
Белый список молча сломал бы магазины с собственными колонками в
ms3_orders/ms3_order_addresses, заведёнными через extra fields. На тестовом стенде такая колонка уже есть (address.test_address) и после правки остаётся писаемой. Запретный список закрывает ровно то, чем распоряжаются оформление, пересчёт стоимости и смена статуса.Ключи вне обеих моделей по-прежнему уходят в
properties['_validated']— кастомные поля формы работают как раньше.Расширяемость
Константы читаются через
static::, поэтому магазин может переопределить списки в подклассеOrderFieldManagerи подставить его в DI-ключms3_order_field_managerчерезcore/config/ms3.services.d/*.php. Этот каталог живёт вне компонента и переживает обновление;ServiceRegistry::validateClass()требует именно наследника. Системной настройки сознательно нет: ослабить security-границу кликом в админке не должно быть возможно.updateField()
if+ifзаменён наif+elseif, заказ главнее. Баг срабатывал и без API:OrderAddressManagerпри выборе сохранённого адреса сам передаёт вupdateFieldproperties заказа и затирал properties адреса.Проверка на стенде
Подмена класса через
ms3.services.d/проверена отдельно: подкласс, убравшийcustomer_idиз списка, открывает это поле, остальные остаются закрытыми. Пробник удалён, защита вернулась.Тесты
OrderFieldManagerProtectedFieldsTest— отказ по каждому закрытому ключу вadd()иremove()(событие не вызывается,updateFieldне вызывается), проходимость полей формы, и сверка сminishop3.mysql.schema.xml: новая колонка в схеме валит тест, пока её осознанно не отнесут к закрытым или к полям формы.OrderDraftManagerUpdateFieldTargetTest—propertiesтолько в заказ, адресные колонки в адрес, неизвестный ключ в_validated.На коде до правки оба падают: 25 ошибок и 1 падение, в адрес уезжает
{"address_hash":"abc"}.Гейты локально: PHPUnit 800/800, smoke 122/122, PHPStan по изменённым файлам чисто.
Изменение поведения
Кастомный фронт, который писал
delivery_costчерезorder/add(самостоятельный расчёт доставки), получит ошибку. Это и есть закрываемая дыра — посетитель мог выставить себе нулевую доставку. Штатная поставка такого не делает: стоимость считаютDeliveryServiceиOrderCostCalculator, аms3_order.tplшлёт только имена, контакты, адрес,payment_id,delivery_id,comment,save_address.Хвост на потом
Приём с подменой
ms3_order_field_managerстоит описать в документации (development/services.mdуже рассказывает проms3.services.d/).