[bug fix] Dynamic links get broken in send_email (#1380)

Addresses: #1052 Demo: https://user-images.githubusercontent.com/50227291/188492173-82f9a8ed-609a-44b1-a608-77aa0e1d1600.mov Co-authored-by: Pralish Kayastha <[email protected]> Co-authored-by: Pralish Kayastha <[email protected]>
restarone · Jan 25, 2023 · b43d050 · b43d050
1 parent fe18115
commit b43d050
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 2 deletions.
diff --git a/app/models/concerns/dynamic_attribute.rb b/app/models/concerns/dynamic_attribute.rb
@@ -34,6 +34,10 @@ def parse_dynamic_attribute(value, context = {})
 
       def attribute_value_string(attribute)
         value = public_send(attribute.to_sym)
+
+        # decoding special characters that action text encoded
+        value = Addressable::URI.unencode(value.to_s) if value.is_a? ActionText::RichText
+
         # Deep copy of non-enumerable column like string-type would get passed and the evaluated value would get reflected as change.
         value.is_a?(Enumerable) ? JSON.generate(value) : value.to_s.dup
       end

diff --git a/app/models/concerns/safe_executable_validator.rb b/app/models/concerns/safe_executable_validator.rb
@@ -28,8 +28,10 @@ class SafeExecutableValidator < ActiveModel::EachValidator
     SPLIT_DELIMITERS = ['(', ')', /\s/, '.', /\n/, /(?<!\:)\:(?!\:)/, '#{', '}', '=>', '"', '\'']
 
     def validate_each(record,attribute,value)
-        keywords = value.to_s.split(Regexp.union(SPLIT_DELIMITERS)).reject(&:blank?)
+        # decode and unescape string before checking for blacklisted kwywords
+        keywords = CGI.unescapeHTML(Addressable::URI.unencode(value.to_s)).split(Regexp.union(SPLIT_DELIMITERS)).reject(&:blank?)
         blacklisted_keywords_in_attribute = keywords & (BLACKLISTED_KEYWORDS - (options[:skip_keywords] || []))
+
         unless blacklisted_keywords_in_attribute.empty?
             record.errors.add(attribute, "contains disallowed keyword: #{blacklisted_keywords_in_attribute.to_s}. Please refactor #{attribute} accordingly")
         end

diff --git a/test/models/concerns/dynamic_attribute_test.rb b/test/models/concerns/dynamic_attribute_test.rb
@@ -30,7 +30,6 @@ def initialize(test_column: nil)
     assert_equal safe_instance.test_column_evaluated, 'Test 2'
   end
 
-
   test 'should raise invalid record error dynamic field contains unsafe string' do
     unsafe_instance = @dummy_class.new(test_column: "Test \#{User.destroy_all}")
     refute unsafe_instance.valid?
@@ -41,6 +40,16 @@ def initialize(test_column: nil)
     end
   end
 
+  test 'should raise invalid record error if dynamic field contains encoded unsafe string' do
+    unsafe_instance = @dummy_class.new(test_column: "&lt;%=%20eval%20%1+2%20%&gt;")
+    refute unsafe_instance.valid?
+    assert_no_difference "User.all.size" do
+      assert_raises ActiveRecord::RecordInvalid do
+        unsafe_instance.test_column_evaluated
+      end
+    end
+  end
+
   test 'should support erb syntax' do
     api_resource_test = api_resources(:user)
     erb_syntax = '<% food = "Ice cream" %>'\
@@ -106,4 +115,11 @@ def initialize(test_column: nil)
     safe_instance = @dummy_class.new(test_column: { test_key: "<%= 1 + 1 %> and \#{2 + 2}" })
     assert_equal safe_instance.test_column_evaluated, { test_key: "2 and 4" }.to_json
   end
+
+  test 'should unescape html and decode uri-encoded characters' do
+    safe_instance = @dummy_class.new(test_column: ActionText::RichText.new(body: '<a href="<%= 2 and 4 %>">'))
+    # action text encoded and escaped special characters
+    assert_equal "<div class=\"trix-content\">\n  <a href=\"&lt;%=%202%20and%204%20%&gt;\"></a>\n</div>\n", safe_instance.test_column.to_s
+    assert_equal "<div class=\"trix-content\">\n  <a href=\"4\"></a>\n</div>\n", safe_instance.test_column_evaluated
+  end
 end