SECURITY: ERB execution in custom Email Style

SECURITY: ERB execution in custom Email Style

diff --git a/app/helpers/email_helper.rb b/app/helpers/email_helper.rb
index c579473..14364d4 100644
--- a/app/helpers/email_helper.rb
+++ b/app/helpers/email_helper.rb
@@ -25,12 +25,8 @@ module EmailHelper
     raw "<a href='#{Discourse.base_url}#{url}' style='color: ##{@anchor_color}'>#{title}</a>"
   end
 
-  def email_html_template(binding_arg)
-    template = EmailStyle.new.html.sub(
-      '%{email_content}',
-      '<%= yield %><% if defined?(html_body) %><%= html_body %><% end %>'
-    )
-    ERB.new(template).result(binding_arg)
+  def email_html_template
+    EmailStyle.new.html.sub('%{email_content}', yield).html_safe
   end
 
   protected
diff --git a/app/views/layouts/email_template.html.erb b/app/views/layouts/email_template.html.erb
index 80da92e..611eba0 100644
--- a/app/views/layouts/email_template.html.erb
+++ b/app/views/layouts/email_template.html.erb
@@ -2,5 +2,8 @@
   <%= yield %>
   <% if defined?(html_body) %><%= html_body %><% end %>
 <% else %>
-  <%= email_html_template(binding).html_safe %>
+  <%= email_html_template do %>
+    <%= yield %>
+    <% if defined?(html_body) %><%= html_body %><% end %>
+  <% end %>
 <% end %>
diff --git a/spec/integration/email_style_spec.rb b/spec/integration/email_style_spec.rb
index ef69850..395dd19 100644
--- a/spec/integration/email_style_spec.rb
+++ b/spec/integration/email_style_spec.rb
@@ -3,128 +3,139 @@
 require "rails_helper"
 
 describe EmailStyle do
-  before do
-    SiteSetting.email_custom_template = "<body><h1>FOR YOU</h1><div>%{email_content}</div></body>"
-    SiteSetting.email_custom_css = 'h1 { color: red; } div.body { color: #FAB; }'
-    SiteSetting.email_custom_css_compiled = SiteSetting.email_custom_css
-  end
-
-  after do
-    SiteSetting.remove_override!(:email_custom_template)
-    SiteSetting.remove_override!(:email_custom_css)
-  end
-
-  context 'invite' do
-    fab!(:invite) { Fabricate(:invite) }
-    let(:invite_mail) { InviteMailer.send_invite(invite) }
 
-    subject(:mail_html) { Email::Renderer.new(invite_mail).html }
-
-    it 'applies customizations' do
-      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}")
+  context "ERB evaluation" do
+    it "does not evaluate ERB outside of the email itself" do
+      SiteSetting.email_custom_template = "<div>%{email_content}</div><%= (111 * 333) %>"
+      html = Email::Renderer.new(UserNotifications.signup(Fabricate(:user))).html
+      expect(html).not_to match("36963")
     end
+  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).to match("#{Discourse.base_url}/invites/#{invite.invite_key}")
+  context "with a custom template" do
+    before do
+      SiteSetting.email_custom_template = "<body><h1>FOR YOU</h1><div>%{email_content}</div></body>"
+      SiteSetting.email_custom_css = 'h1 { color: red; } div.body { color: #FAB; }'
+      SiteSetting.email_custom_css_compiled = SiteSetting.email_custom_css
     end
 
-    it 'can apply RTL attrs' do
-      SiteSetting.default_locale = 'he'
-      body_attrs = mail_html.match(/<body ([^>])+/)
-      expect(body_attrs[0]&.downcase).to match(/text-align:\s*right/)
-      expect(body_attrs[0]&.downcase).to include('dir="rtl"')
+    after do
+      SiteSetting.remove_override!(:email_custom_template)
+      SiteSetting.remove_override!(:email_custom_css)
     end
-  end
 
-  context 'user_replied' do
-    let(:response_by_user) { Fabricate(:user, name: "John Doe") }
-    let(:category) { Fabricate(:category, name: 'India') }
-    let(:topic) { Fabricate(:topic, category: category, title: "Super cool topic") }
-    let(:post) { Fabricate(:post, topic: topic, raw: 'This is My super duper cool topic') }
-    let(:response) { Fabricate(:basic_reply, topic: post.topic, user: response_by_user) }
-    let(:user) { Fabricate(:user) }
-    let(:notification) { Fabricate(:replied_notification, user: user, post: response) }
-
-    let(:mail) do
-      UserNotifications.user_replied(
-        user,
-        post: response,
-        notification_type: notification.notification_type,
-        notification_data_hash: notification.data_hash
-      )
-    end
+    context 'invite' do
+      fab!(:invite) { Fabricate(:invite) }
+      let(:invite_mail) { InviteMailer.send_invite(invite) }
 
-    subject(:mail_html) { Email::Renderer.new(mail).html }
+      subject(:mail_html) { Email::Renderer.new(invite_mail).html }
 
-    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)
-      matches = mail_html.match(/<div style="([^"]+)">#{post.raw}/)
-      expect(matches[1]).to include('color: #FAB;') # custom
-      expect(matches[1]).to include('padding-top:5px;') # div.body
-    end
-
-    # TODO: translation override
-  end
+      it 'applies customizations' do
+        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
 
-  context 'signup' do
-    let(:signup_mail) { UserNotifications.signup(Fabricate(:user)) }
-    subject(:mail_html) { Email::Renderer.new(signup_mail).html }
+      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).to match("#{Discourse.base_url}/invites/#{invite.invite_key}")
+      end
 
-    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).to include('activate-account')
+      it 'can apply RTL attrs' do
+        SiteSetting.default_locale = 'he'
+        body_attrs = mail_html.match(/<body ([^>])+/)
+        expect(body_attrs[0]&.downcase).to match(/text-align:\s*right/)
+        expect(body_attrs[0]&.downcase).to include('dir="rtl"')
+      end
     end
 
-    context 'translation override' do
-      before do
-        TranslationOverride.upsert!(
-          'en',
-          'user_notifications.signup.text_body_template',
-          "CLICK THAT LINK: %{base_url}/u/activate-account/%{email_token}"
+    context 'user_replied' do
+      let(:response_by_user) { Fabricate(:user, name: "John Doe") }
+      let(:category) { Fabricate(:category, name: 'India') }
+      let(:topic) { Fabricate(:topic, category: category, title: "Super cool topic") }
+      let(:post) { Fabricate(:post, topic: topic, raw: 'This is My super duper cool topic') }
+      let(:response) { Fabricate(:basic_reply, topic: post.topic, user: response_by_user) }
+      let(:user) { Fabricate(:user) }
+      let(:notification) { Fabricate(:replied_notification, user: user, post: response) }
+
+      let(:mail) do
+        UserNotifications.user_replied(
+          user,
+          post: response,
+          notification_type: notification.notification_type,
+          notification_data_hash: notification.data_hash
         )
       end
 
-      after do
-        TranslationOverride.revert!('en', ['user_notifications.signup.text_body_template'])
-      end
+      subject(:mail_html) { Email::Renderer.new(mail).html }
 
-      it "applies customizations when translation override exists" do
+      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('CLICK THAT LINK').count).to eq(1)
+        matches = mail_html.match(/<div style="([^"]+)">#{post.raw}/)
+        expect(matches[1]).to include('color: #FAB;') # custom

[... diff too long, it was truncated ...]

GitHub sha: d11c4621

1 Like