CVE-2023-22794

@@ -146,7 +146,16 @@ def quoted_binary(value) # :nodoc:
       end
 
       def sanitize_as_sql_comment(value) # :nodoc:
-        value.to_s.gsub(%r{ (/ (?: | \g<1>) \*) \+? \s* | \s* (\* (?: | \g<2>) /) }x, "")
+        # Sanitize a string to appear within a SQL comment
+        # For compatibility, this also surrounding "/*+", "/*", and "*/"
+        # charcacters, possibly with single surrounding space.
+        # Then follows that by replacing any internal "*/" or "/ *" with
+        # "* /" or "/ *"
+        comment = value.to_s.dup
+        comment.gsub!(%r{\A\s*/\*\+?\s?|\s?\*/\s*\Z}, "")
+        comment.gsub!("*/", "* /")
+        comment.gsub!("/*", "/ *")
+        comment
       end
 
       def column_name_matcher # :nodoc:

@@ -33,6 +33,8 @@ module ActiveRecord
   # want to add to the comment. Dynamic content can be created by setting a proc or lambda value in a hash,
   # and can reference any value stored in the +context+ object.
   #
+  # Escaping is performed on the string returned, however untrusted user input should not be used.
+  #
   # Example:
   #
   #    tags = [
@@ -109,7 +111,16 @@ def uncached_comment
         end
 
         def escape_sql_comment(content)
-          content.to_s.gsub(%r{ (/ (?: | \g<1>) \*) \+? \s* | \s* (\* (?: | \g<2>) /) }x, "")
+          # Sanitize a string to appear within a SQL comment
+          # For compatibility, this also surrounding "/*+", "/*", and "*/"
+          # charcacters, possibly with single surrounding space.
+          # Then follows that by replacing any internal "*/" or "/ *" with
+          # "* /" or "/ *"
+          comment = content.to_s.dup
+          comment.gsub!(%r{\A\s*/\*\+?\s?|\s?\*/\s*\Z}, "")
+          comment.gsub!("*/", "* /")
+          comment.gsub!("/*", "/ *")
+          comment
         end
 
         def tag_content

@@ -1216,6 +1216,8 @@ def skip_preloading! # :nodoc:
     #   # SELECT "users"."name" FROM "users" /* selecting */ /* user */ /* names */
     #
     # The SQL block comment delimiters, "/*" and "*/", will be added automatically.
+    #
+    # Some escaping is performed, however untrusted user input should not be used.
     def annotate(*args)
       check_if_method_has_arguments!(__callee__, args)
       spawn.annotate!(*args)

@@ -18,17 +18,22 @@ def test_annotate_wraps_content_in_an_inline_comment
   def test_annotate_is_sanitized
     quoted_posts_id, quoted_posts = regexp_escape_table_name("posts.id"), regexp_escape_table_name("posts")
 
-    assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* foo \*/}i) do
+    assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* \* /foo/ \* \*/}i) do
       posts = Post.select(:id).annotate("*/foo/*")
       assert posts.first
     end
 
-    assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* foo \*/}i) do
+    assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* \*\* //foo// \*\* \*/}i) do
       posts = Post.select(:id).annotate("**//foo//**")
       assert posts.first
     end
 
-    assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* foo \*/ /\* bar \*/}i) do
+    assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* \* \* //foo// \* \* \*/}i) do
+      posts = Post.select(:id).annotate("* *//foo//* *")
+      assert posts.first
+    end
+
+    assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* \* /foo/ \* \*/ /\* \* /bar \*/}i) do
       posts = Post.select(:id).annotate("*/foo/*").annotate("*/bar")
       assert posts.first
     end

@@ -42,8 +42,9 @@ def test_escaping_good_comment
   end
 
   def test_escaping_bad_comments
-    assert_equal "; DROP TABLE USERS;", ActiveRecord::QueryLogs.send(:escape_sql_comment, "*/; DROP TABLE USERS;/*")
-    assert_equal "; DROP TABLE USERS;", ActiveRecord::QueryLogs.send(:escape_sql_comment, "**//; DROP TABLE USERS;/*")
+    assert_equal "* /; DROP TABLE USERS;/ *", ActiveRecord::QueryLogs.send(:escape_sql_comment, "*/; DROP TABLE USERS;/*")
+    assert_equal "** //; DROP TABLE USERS;/ *", ActiveRecord::QueryLogs.send(:escape_sql_comment, "**//; DROP TABLE USERS;/*")
+    assert_equal "* * //; DROP TABLE USERS;// * *", ActiveRecord::QueryLogs.send(:escape_sql_comment, "* *//; DROP TABLE USERS;//* *")
   end
 
   def test_basic_commenting

@@ -345,7 +345,7 @@ def test_relation_with_annotation_chains_sql_comments
 
     def test_relation_with_annotation_filters_sql_comment_delimiters
       post_with_annotation = Post.where(id: 1).annotate("**//foo//**")
-      assert_match %r{= 1 /\* foo \*/}, post_with_annotation.to_sql
+      assert_includes post_with_annotation.to_sql, "= 1 /* ** //foo// ** */"
     end
 
     def test_relation_with_annotation_includes_comment_in_count_query
@@ -367,13 +367,9 @@ def test_relation_without_annotation_does_not_include_an_empty_comment
 
     def test_relation_with_optimizer_hints_filters_sql_comment_delimiters
       post_with_hint = Post.where(id: 1).optimizer_hints("**//BADHINT//**")
-      assert_match %r{BADHINT}, post_with_hint.to_sql
-      assert_no_match %r{\*/BADHINT}, post_with_hint.to_sql
-      assert_no_match %r{\*//BADHINT}, post_with_hint.to_sql
-      assert_no_match %r{BADHINT/\*}, post_with_hint.to_sql
-      assert_no_match %r{BADHINT//\*}, post_with_hint.to_sql
+      assert_includes post_with_hint.to_sql, "/*+ ** //BADHINT// ** */"
       post_with_hint = Post.where(id: 1).optimizer_hints("/*+ BADHINT */")
-      assert_match %r{/\*\+ BADHINT \*/}, post_with_hint.to_sql
+      assert_includes post_with_hint.to_sql, "/*+ BADHINT */"
     end
 
     def test_does_not_duplicate_optimizer_hints_on_merge