[biz87]

Closes #888

Проблема шире, чем в issue

EmailChannel::buildBody() собирает тело Fenom-чанком вместе с полями заказа, а потом прогоняет готовую строку через processElementTags(). Ключевой момент: при установленном pdoTools $modx->getParser() — это не разбор MODX-тегов, а Fenom. Стектрейс:

EmailChannel.php:149 → PdoTools\Parsing\Parser->processElementTags()
                     → Fenom->process() → Fenom\Template.php(487): eval()'d code

В Fenom pdoTools через $_modx доступны runSnippet() и runProcessor(). То есть из комментария покупателя исполняется не только [[++setting]], но и произвольный код шаблонизатора.

Вход — комментарий, имя и адрес в публичном оформлении заказа. Триггер — отправка любого письма по заказу. pdoTools формально не в requires (_build/build.php:119 закомментирован), но без него Fenom-чанки писем не рендерятся вовсе, так что на рабочих сайтах он стоит всегда.

Второй вектор — тема письма

StatusChangedNotification::processSubject() подставлял данные покупателя в строку темы, а buildSubject() компилировал эту строку как шаблон через '@INLINE ':

тема в настройках:    Заказ {num}: {order_comment}
после processSubject: Заказ 2610/1: A{"inj"|upper}B{$_modx->lexicon("ms3_err_unknown")}C
итоговая тема:        Заказ 2610/1: AINJB«Неизвестная ошибка»C

Третье место

Utils::sendEmail() (Utils.php:199-200) содержал те же два вызова парсера по готовому телу. Внутри MS3 не используется — это публичный метод для расширений.

Проверка на стенде

Один и тот же комментарий заказа, tpl.msEmail.new.manager, до и после правки:

что проверяли	до	после
{$_modx->runSnippet("ms3Probe888")} — сниппет возвращает метку, которой нет в нагрузке	исполнен	нет
[[++emailsender]] — настройка прочитана	да	нет
{"zz"|upper} — Fenom-модификатор	исполнен	нет
<img src=x onerror=alert(1)>	живой тег	экранирован
<a href="http://evil">	живой тег	экранирован
имя <b>Боб</b>	не экранировано	экранировано

После правки комментарий в теле письма выглядит так:

<td>{$_modx-&gt;runSnippet(&quot;ms3Probe888&quot;)}|[[++emailsender]]|{&quot;zz&quot;|upper}|&lt;img src=x onerror=alert(1)&gt;|&lt;a href=&quot;http://evil&quot;&gt;click&lt;/a&gt;</td>

Решение

Тело: данные нейтрализуются вокруг парсера

Перед рендером в значениях плейсхолдеров рекурсивно подменяются открывающие последовательности [[ и {, после прохода парсера они возвращаются на место. Метка разовая на каждое письмо — с постоянной покупатель вписал бы её в комментарий и получил обратно [[ или { после разворачивания.

Такой порядок выбран ради обратной совместимости: прямое удаление processElementTags сломало бы чанки магазинов, написанные на MODX-тегах. Проверено на чанке в «старом» стиле — после правки в самом шаблоне по-прежнему работает всё:

FENOM_VAR:2610/1
FENOM_LEX:Комментарий
MODX_SETTING:MODX Revolution
MODX_LEX:Комментарий
MODX_SNIPPET:SNIPPET-FROM-CHUNK
MODX_SNIPPET_UNCACHED:SNIPPET-FROM-CHUNK
CUSTOMER_COMMENT:A[[++site_name]]B{$num}C     ← данные остались текстом

Тема: тот же механизм

processSubject() удалён — он подставлял данные в текст шаблона до компиляции.

Первая редакция PR заменяла его подстановкой после компиляции, но этого оказалось мало. Ревью нашло два дефекта, исправлены в 0f3b1455 и 6f468b8e:

• CoreTools::getChunk() у pdoTools после Fenom сам подставляет [[+key]] и прогоняет остаток через разбор MODX-тегов, поэтому значение, вставленное Fenom через {$order_comment} или {$address.first_name}, доезжало до парсера шаблоном. Тема закрыта тем же механизмом, что и тело: значения нейтрализуются.
• Плоские {key}, отданные в @INLINE как есть, валили компиляцию Fenom, а pdoTools на ошибке подставляет пустой шаблон — тема со смесью {num} и любой Fenom-конструкции уходила пустой. Теперь плоские ключи маскируются в самом шаблоне и разворачиваются после парсеров, перед восстановлением открывающих последовательностей.

Подстановка плоских ключей нужна, потому что Fenom понимает только {$key}. Существующие темы работают в обоих синтаксисах:

Заказ {num}                                   -> Заказ 2610/1
Заказ {$order.num}                            -> Заказ 2610/1
Заказ {num} / {$status_name}                  -> Заказ 2610/1 / Новый
Заказ {num}: {'ms3_frontend_comment'|lexicon} -> Заказ 2610/1: Комментарий
Заказ {num} [[++site_name]]                   -> Заказ 2610/1 MODX Revolution
Заказ {num}1 шт                               -> Заказ 2610/11 шт
Заказ {num}: {order_comment}                  -> Заказ 2610/1: <комментарий текстом>
{$order_comment} / {$address.first_name}      -> текстом, теги не исполняются
[[+order_comment]]                            -> текстом, теги не исполняются

Заодно убран str_replace("{$key}", …) из fallback-ветки buildSubject() — без фигурных скобок он подменял любое вхождение имени ключа в тексте темы.

Экранирование

|e на поля покупателя и снимок заказа в ms3_email.tpl и ms3_email_new_manager.tpl. Скрипты в письме не исполняются, но <a href> и <img src> работают, то есть покупатель мог подсунуть менеджеру ссылку или трекинг-пиксель. Автоэкранирования в pdoTools нет — двойного экранирования не будет; format->price() и format->weightWithUnit() HTML не содержат, их не трогал.

Важно: экранирование само по себе от исполнения не защищает — htmlspecialchars не трогает [ и {. Нужны оба механизма.

Тесты

• tests/Unit/Notifications/EmailChannelTagHandlingTest.php — нейтрализация не оставляет открывающих последовательностей, round-trip восстанавливает исходный текст, обход идёт по вложенным массивам и не портит типы, метка разная на каждое письмо, плоская подстановка берёт только {key} и не переинтерпретирует значение.
• tests/EmailTagInjectionGuardTest.php — сторожит порядок операций в исходниках (нейтрализация до рендера, восстановление после парсера, подстановка темы после парсера), отсутствие processSubject, отсутствие парсера в Utils::sendEmail() и наличие |e у каждого вывода полей покупателя в штатных чанках.

На коде до правки smoke падает первым же утверждением, unit — все 18 кейсов.

Гейты локально: PHPUnit 818/818, smoke 123/123, PHPStan по src/Notifications/ и Utils.php чисто.

Для dev-стендов

Чанки писем правятся в elements/, а на стендах они обычно лежат в БД (static=0) — после обновления нужно перезалить код tpl.msEmail и tpl.msEmail.new.manager.

[biz87]

Оба критичных замечания воспроизвёл и закрыл. Коммит 0f3b1455.

Критичное 1 — инъекция в теме

Подтвердилось полностью. Я ошибся, решив, что в pdoTools-ветке buildSubject() парсера за Fenom нет: он внутри CoreTools::getChunk() — после Fenom идёт подстановка [[+key]] через makePlaceholders, а затем $chunk->process() по остатку. Значение, вставленное Fenom в вывод, доезжало до парсера шаблоном.

Прогон реального buildSubject(), нагрузка A[[++site_url]]B в order_comment / address.first_name:

форма темы	до	после
{order_comment}	текст	текст
{$order_comment}	Ahttp://ms3.test/B	текст
{$address.first_name}	Ahttp://ms3.test/B	текст
[[+order_comment]]	Ahttp://ms3.test/B	текст

Остальные нагрузки из ревью в теме {$order_comment} после правки остаются литералами: [[$tpl.msEmail]], [[~1? &scheme=\full`]], [[pdoResources? &parents=`0` &limit=`2` &tpl=`@INLINE {$pagetitle}`]]`.

Критичное 2 — BC темы со смесью

Тоже подтвердилось, и причина ровно та, что в ревью: Fenom спотыкается на {num} при компиляции, _compileChunk() на ошибке подставляет пустой шаблон.

тема	до PR	PR до правки	сейчас
Заказ {num}	Заказ 2610/1	Заказ 2610/1	Заказ 2610/1
Заказ {$order.num}	—	Заказ 2610/1	Заказ 2610/1
Заказ {num} / {$status_name}	Заказ 2610/1 / Новый	(пусто)	Заказ 2610/1 / Новый
Заказ {num}: {'ms3_frontend_comment' | lexicon}	Заказ 2610/1: Комментарий	(пусто)	Заказ 2610/1: Комментарий
Заказ {num} [[++site_name]]	—	работало	Заказ 2610/1 MODX Revolution

Моё утверждение в описании PR про @INLINE было верным только для темы без Fenom — обобщил зря.

Как сделано

Ровно одним механизмом, как и предлагалось. В buildSubject(): плоские {key} маскируются в самом шаблоне (maskFlatPlaceholders()), значения плейсхолдеров нейтрализуются, после парсера сначала restoreTags(), затем разворачиваются значения плоских ключей. Парсеров за этим нет, так что значение шаблоном не станет, а Fenom больше не спотыкается на {num}.

fillFlatPlaceholders() заменён на маскировку, разворачивание идёт через strtr — заодно уходит замечание о последовательной подстановке: значение с {другим_ключом} внутри под следующий поиск не попадает, есть тест.

Некритичные

• Регистр метки. Метка теперь только из цифр (tagNonce() на random_int), метки открывающих последовательностей — 1 и 2. Проверено на чанке с модификаторами: RAW:x[[++site_url]]y{$num}z, UP:X[[++SITE_URL]]Y{$NUM}Z, LOW:... — восстановление проходит, маркер не протекает.
• |truncate всё равно рвёт маркер — TRUNC:x93010403567.... От обрезки цифровая метка не спасает, развернуть обрезанный маркер нельзя. Ограничение описал в докблоке tagNonce(): косметика, и только если покупатель вписал в поле открывающую последовательность.
• Окно 1200 символов в стороже sendEmail() заменено на срез до следующего function .
• Utils::sendEmail() в CHANGELOG — согласен, конвенция тут «только на релизе», поэтому отметил себе на релизную запись, в PR не тащу.

Гейты локально: PHPUnit 820/820 (2899 assertions, 1 чужая deprecation), smoke 123/123, PHPStan по src/Notifications/ и Utils.php чисто. Оба теста падают на коде без правки: smoke — первым же утверждением, unit — все кейсы маскировки.

[biz87]

Воспроизвёл и закрыл. Коммит 6f468b8e.

Диагноз точный: маркер заканчивается меткой, restoreTags() шёл раньше разворачивания и искал метку с цифрой 1/2, поэтому соседняя цифра складывалась с хвостом маркера.

до:
  Заказ {num}1 шт  -> Заказ 99669582318436946930[[ шт     СЛОМАНО
  Заказ {num}2026  -> Заказ 98649116705278225930{026      СЛОМАНО
  Заказ {num}3 шт  -> Заказ 2610/13 шт                    ок
  Заказ {num} 1 шт -> Заказ 2610/1 1 шт                   ок

после:
  Заказ {num}1 шт  -> Заказ 2610/11 шт                    ок
  Заказ {num}2026  -> Заказ 2610/12026                    ок
  Заказ {num}3 шт  -> Заказ 2610/13 шт                    ок
  Заказ {num} 1 шт -> Заказ 2610/1 1 шт                   ок

Сделал как предложено — перемена порядка двух последних шагов: сначала strtr($subject, $flatValues), потом restoreTags(). Рассуждение про висячую метку проверил: нейтрализованные значения держат её только в составе пары, а сырое значение плоского ключа 18-значную метку содержать не может, так что после разворачивания ей взяться неоткуда.

Сторож перевёрнут на posSubjectExpand < posSubjectRestore с пояснением, почему именно так. В unit добавлен датапровайдер суффиксов сразу за плоским ключом: 1 шт, 2026, 3 шт, 1 шт, 12, 21, пустой — проверяется и результат, и отсутствие метки в выводе.

Прежние проверки не поехали: четыре формы инъекции в теме остаются текстом, смесь плоского ключа с Fenom по-прежнему собирается.

Гейты локально: PHPUnit 827/827 (2913 assertions, 1 чужая deprecation), smoke 123/123, PHPStan по src/Notifications/ чисто.