From 7d5879375986e22e6650fce542d075909161239b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Sat, 30 Nov 2024 16:38:45 +0100
Subject: [PATCH] DEV: deduplicate inline styles in emails (#30015)

In order to limit issues with duplicate inline CSS definitions, this will now deduplicate inline CSS styles with the "last-to-be-defined-wins" strategy.

Also removes unecessary whitespaces in inline styles.

Context - https://meta.discourse.org/t/resolve-final-styles-in-email-notifications/310219

Co-authored-by: Thomas Kalka <thomas.kalka@gmail.com>
---
 lib/email/styles.rb                  | 19 +++++++++++++++++++
 spec/integration/email_style_spec.rb | 18 +++++++++---------
 spec/lib/email/sender_spec.rb        |  2 +-
 spec/lib/email/styles_spec.rb        | 19 +++++++++++++++++++
 4 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/lib/email/styles.rb b/lib/email/styles.rb
index 48b051cd88a..c378d0d7a05 100644
--- a/lib/email/styles.rb
+++ b/lib/email/styles.rb
@@ -374,12 +374,31 @@ module Email
       end
     end
 
+    def deduplicate_style(style)
+      styles = {}
+
+      style
+        .split(";")
+        .select(&:present?)
+        .map { _1.split(":", 2).map(&:strip) }
+        .each { |k, v| styles[k] = v if k.present? && v.present? }
+
+      styles.map { |k, v| "#{k}:#{v}" }.join(";")
+    end
+
+    def deduplicate_styles
+      @fragment
+        .css("[style]")
+        .each { |element| element["style"] = deduplicate_style element["style"] }
+    end
+
     def to_html
       # needs to be before class + id strip because we need to style redacted
       # media and also not double-redact already redacted from lower levels
       replace_secure_uploads_urls if SiteSetting.secure_uploads?
       strip_classes_and_ids
       replace_relative_urls
+      deduplicate_styles
 
       @fragment.to_html
     end
diff --git a/spec/integration/email_style_spec.rb b/spec/integration/email_style_spec.rb
index 49fff4d3930..b0a2d9cafee 100644
--- a/spec/integration/email_style_spec.rb
+++ b/spec/integration/email_style_spec.rb
@@ -30,13 +30,13 @@ RSpec.describe EmailStyle do
       let(:invite_mail) { InviteMailer.send_invite(invite) }
 
       it "applies customizations" do
-        expect(mail_html.scan('<h1 style="color: red;">FOR YOU</h1>').count).to eq(1)
+        expect(mail_html.scan('<h1 style="color:red">FOR YOU</h1>').count).to eq(1)
         expect(mail_html).to match("#{Discourse.base_url}/invites/#{invite.invite_key}")
       end
 
       it "applies customizations if compiled is missing" do
         SiteSetting.remove_override!(:email_custom_css_compiled)
-        expect(mail_html.scan('<h1 style="color: red;">FOR YOU</h1>').count).to eq(1)
+        expect(mail_html.scan('<h1 style="color:red">FOR YOU</h1>').count).to eq(1)
         expect(mail_html).to match("#{Discourse.base_url}/invites/#{invite.invite_key}")
       end
 
@@ -71,10 +71,10 @@ RSpec.describe EmailStyle do
       it "customizations are applied to html part of emails" do
         SiteSetting.default_email_in_reply_to = true
 
-        expect(mail_html.scan('<h1 style="color: red;">FOR YOU</h1>').count).to eq(1)
+        expect(mail_html.scan('<h1 style="color:red">FOR YOU</h1>').count).to eq(1)
         matches = mail_html.match(/<div style="([^"]+)" dm=\"body\">#{post.raw}/)
-        expect(matches[1]).to include("color: #FAB;") # custom
-        expect(matches[1]).to include("padding-top:5px;") # div.body
+        expect(matches[1]).to include("color:#FAB") # custom
+        expect(matches[1]).to include("padding-top:5px") # div.body
       end
 
       # TODO: translation override
@@ -86,7 +86,7 @@ RSpec.describe EmailStyle do
       let(:signup_mail) { UserNotifications.signup(Fabricate(:user)) }
 
       it "customizations are applied to html part of emails" do
-        expect(mail_html.scan('<h1 style="color: red;">FOR YOU</h1>').count).to eq(1)
+        expect(mail_html.scan('<h1 style="color:red">FOR YOU</h1>').count).to eq(1)
         expect(mail_html).to include("activate-account")
       end
 
@@ -107,7 +107,7 @@ RSpec.describe EmailStyle do
         end
 
         it "applies customizations when translation override exists" do
-          expect(mail_html.scan('<h1 style="color: red;">FOR YOU</h1>').count).to eq(1)
+          expect(mail_html.scan('<h1 style="color:red">FOR YOU</h1>').count).to eq(1)
           expect(mail_html.scan("CLICK THAT LINK").count).to eq(1)
         end
       end
@@ -135,13 +135,13 @@ RSpec.describe EmailStyle do
       let(:summary_email) { UserNotifications.digest(Fabricate(:user)) }
 
       it "customizations are applied to html part of emails" do
-        expect(mail_html.scan('<h1 style="color: red;">FOR YOU</h1>').count).to eq(1)
+        expect(mail_html.scan('<h1 style="color:red">FOR YOU</h1>').count).to eq(1)
         expect(mail_html).to include(popular_topic.title)
       end
 
       it "doesn't apply customizations if apply_custom_styles_to_digest is disabled" do
         SiteSetting.apply_custom_styles_to_digest = false
-        expect(mail_html).to_not include('<h1 style="color: red;">FOR YOU</h1>')
+        expect(mail_html).to_not include('<h1 style="color:red">FOR YOU</h1>')
         expect(mail_html).to_not include("FOR YOU")
         expect(mail_html).to include(popular_topic.title)
       end
diff --git a/spec/lib/email/sender_spec.rb b/spec/lib/email/sender_spec.rb
index 918ec03c524..aacfb3eeab9 100644
--- a/spec/lib/email/sender_spec.rb
+++ b/spec/lib/email/sender_spec.rb
@@ -591,7 +591,7 @@ RSpec.describe Email::Sender do
       reply.rebake!
       Email::Sender.new(message, :valid_type).send
       expected = <<~HTML
-      <a href=\"#{Discourse.base_url}#{category.url}\" data-type=\"category\" data-slug=\"dev\" data-id=\"#{category.id}\" style=\"text-decoration: none; font-weight: bold; color: #006699;\"><span>#dev</span>
+      <a href=\"#{Discourse.base_url}#{category.url}\" data-type=\"category\" data-slug=\"dev\" data-id=\"#{category.id}\" style=\"text-decoration:none;font-weight:bold;color:#006699\"><span>#dev</span>
       HTML
       expect(message.html_part.body.to_s).to include(expected.chomp)
     end
diff --git a/spec/lib/email/styles_spec.rb b/spec/lib/email/styles_spec.rb
index e7822da2cf9..eec3491c555 100644
--- a/spec/lib/email/styles_spec.rb
+++ b/spec/lib/email/styles_spec.rb
@@ -168,6 +168,25 @@ RSpec.describe Email::Styles do
     end
   end
 
+  describe "deduplicate styles" do
+    it "removes double definitions" do
+      frag = "<test style='color:green;color:red'>hello</test>"
+
+      styler = Email::Styles.new(frag)
+      styled = styler.to_html
+      styled = Nokogiri::HTML5.fragment(styled)
+      expect(styled.at("test")["style"]).to eq("color:red")
+    end
+    it "handles whitespace correctly" do
+      frag =
+        "<test style=' color :  green ; ; ;   color :    red; background:white;  background:yellow '>hello</test>"
+      styler = Email::Styles.new(frag)
+      styled = styler.to_html
+      styled = Nokogiri::HTML5.fragment(styled)
+      expect(styled.at("test")["style"]).to eq("color:red;background:yellow")
+    end
+  end
+
   describe "dark mode emails" do
     it "adds dark_mode_styles when site setting active" do
       frag = html_fragment('<div class="body">test</div>')