fix: enabling html ids inside email body to permit css targetting - #13481
fix: enabling html ids inside email body to permit css targetting#13481madbob wants to merge 1 commit into
Conversation
|
Thanks! |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughHTMLPurifier now preserves and processes element ChangesHTML sanitization
Estimated code review effort: 1 (Trivial) | ~2 minutes Mergeability Score: ⚪ Minimal · up to This is a localized change to preserve HTML IDs in email bodies for CSS targeting; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ChristophWurst
left a comment
There was a problem hiding this comment.
Makes sense and can't think of a way that these IDs break anything outside the iframe.
@kesselb would appreciate a second review from you nonetheless
I'd also suggest a specific test for this in the style of
public function testSanitizeHtmlMailBodyPreservesId(): void {
$urlGenerator = $this->createStub(IURLGenerator::class);
$request = $this->createStub(IRequest::class);
$hmacGenerator = $this->createStub(ProxyHmacGenerator::class);
$html = new Html($urlGenerator, $request, $hmacGenerator);
$result = $html->sanitizeHtmlMailBody(42, '<p id="target">hello</p>', []);
$this->assertStringContainsString('id="target"', $result);
}Signed-off-by: Roberto Guido <info@madbob.org>
|
Pushed the extra test (thanks!) |
Examining the samples provided in #9976, and in particular this one, I've found that the original message includes CSS style targetting IDs inside the HTML body itself.
By default HTMLPurifier strips IDs, as it is considered unsafe to keep them and risk conflicts with IDs already existing in the web application embedding the manipulated HTML. But in Mail the full email body is confined into an iframe, so no clashing is expected to happen.
Fixes #9976
Summary by CodeRabbit