fix: preserve Liquid variables in email template link hrefs - #1343
Conversation
HTMLPurifier URI encoding was turning {{ order.number }} into
%7B%7B%20order.number%20%7D%7D on save. Protect Liquid tokens around
purification for email template bodies.
Fixes HiEventsDev#1183
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
🟡 Changes recommended
The new Liquid-preserving purifier can bypass URL-scheme enforcement and can throw on preg_replace_callback() null results, which needs to be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a regression where HTMLPurifier percent-encodes Liquid tokens inside href attributes when saving email templates (e.g. {{ order.number }} → %7B%7B...%7D%7D), by adding a purification path that preserves Liquid tokens during sanitization and using it for email template create/update.
Changes:
- Added
HtmlPurifierService::purifyPreservingLiquid()to protect/restore{{ }}and{% %}tokens around HTMLPurifier. - Updated email template create/update handlers to use the new purification method for template
body. - Added unit tests asserting Liquid tokens remain intact in
hrefattributes after the new purification flow.
File summaries
| File | Description |
|---|---|
| backend/app/Services/Infrastructure/HtmlPurifier/HtmlPurifierService.php | Adds Liquid-preserving HTML purification method used by email template persistence. |
| backend/app/Services/Application/Handlers/EmailTemplate/CreateEmailTemplateHandler.php | Switches template body purification to preserve Liquid tokens on create. |
| backend/app/Services/Application/Handlers/EmailTemplate/UpdateEmailTemplateHandler.php | Switches template body purification to preserve Liquid tokens on update. |
| backend/tests/Unit/Services/Infrastructure/HtmlPurifier/HtmlPurifierServiceTest.php | Adds coverage for Liquid token preservation vs default purification behavior. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| $tokens = []; | ||
| $protected = preg_replace_callback( | ||
| '/\{\{[\s\S]*?\}\}|\{%[\s\S]*?%\}/', | ||
| static function (array $matches) use (&$tokens): string { | ||
| $placeholder = 'LIQUIDTOKEN'.count($tokens).'X'; | ||
| $tokens[$placeholder] = $matches[0]; | ||
|
|
||
| return $placeholder; | ||
| }, | ||
| $html, | ||
| ); | ||
|
|
||
| $purified = $this->htmlPurifier->purify($protected, $this->config); | ||
|
|
There was a problem hiding this comment.
Accepted — per-call random placeholder prefix and null-safe fallback to normal purify(). Falling back when an href starts with Liquid would break the reported use case (query params with {{ }}); tokens are restored after purification so scheme checks still run on the placeholder URI.
| /** | ||
| * Purify HTML while preserving Liquid template tokens such as | ||
| * `{{ order.number }}` and `{% if ... %}` so URI encoding does not turn | ||
| * them into `%7B%7B...%7D%7D` inside hrefs and other attributes. | ||
| */ |
There was a problem hiding this comment.
Accepted — removed the explanatory docblock.
Use a per-call random placeholder prefix, fall back to normal purify when preg_replace_callback fails, and drop the explanatory docblock per project conventions.
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
recheck |
# Conflicts: # backend/tests/Unit/Services/Infrastructure/HtmlPurifier/HtmlPurifierServiceTest.php
|
Thank you for this change! I made a slight adjustment as your code allowed some XSS to slip through. |
What changes I've made
HtmlPurifierService::purifyPreservingLiquid()that temporarily replaces Liquid{{ }}/{% %}tokens with per-call random placeholders before HTMLPurifier runs, then restores thempurify()still encodes braces)Fixes #1183
Why I've made these changes
Saving an email template ran the body through HTMLPurifier, which URI-encodes
{/}inhrefs ({{ order.number }}→%7B%7B%20order.number%20%7D%7D). Preview looked fine before save because purification only runs server-side on create/update. That matches the bug report on #1183 (claimed before opening this PR).How I've tested these changes
HtmlPurifierServiceTestasserting Liquid tokens survive in hrefs / tag tokens, and that default purify still percent-encodes bracesSuggested manual check:
{{ order.number }}%7B%7B...)Checklist