Repository: mozilla/bleach

Issue:
bleach.clean method returns unexpected value for string with unknown entities
For string strings like "&adp;" and "&*!;" actual output is "&ad;;" and "&*;;".
But I expect that such values should remain original value ("&adp;" and "&*!;" corresponding).

Discussion:

Patch:
diff --git a/bleach/html5lib_shim.py b/bleach/html5lib_shim.py
index d97d5c4..3ebfc8a 100644
--- a/bleach/html5lib_shim.py
+++ b/bleach/html5lib_shim.py
@@ -512,13 +512,13 @@ def convert_entities(text):
 def match_entity(stream):
     """Returns first entity in stream or None if no entity exists
 
-    Note: For Bleach purposes, entities must start with a "&" and end with
-    a ";". This ignoresambiguous character entities that have no ";" at the
-    end.
+    Note: For Bleach purposes, entities must start with a "&" and end with a
+    ";". This ignores ambiguous character entities that have no ";" at the end.
 
     :arg stream: the character stream
 
-    :returns: ``None`` or the entity string without "&" or ";"
+    :returns: the entity string without "&" or ";" if it's a valid character
+        entity; ``None`` otherwise
 
     """
     # Nix the & at the beginning
@@ -557,9 +557,11 @@ def match_entity(stream):
     # Handle character entities
     while stream and stream[0] not in end_characters:
         c = stream.pop(0)
-        if not ENTITIES_TRIE.has_keys_with_prefix(possible_entity):
-            break
         possible_entity += c
+        if not ENTITIES_TRIE.has_keys_with_prefix(possible_entity):
+            # If it's not a prefix, then it's not an entity and we're
+            # out
+            return None
 
     if possible_entity and stream and stream[0] == ";":
         return possible_entity


Test patch:
diff --git a/tests/test_clean.py b/tests/test_clean.py
index 9aca28a..c22ffa3 100644
--- a/tests/test_clean.py
+++ b/tests/test_clean.py
@@ -11,11 +11,14 @@ from bleach._vendor.html5lib.constants import rcdataElements
 @pytest.mark.parametrize(
     "data",
     [
-        "<span>text & </span>",
         "a < b",
         "link http://link.com",
         "text<em>",
+        # Verify idempotentcy with character entity handling
+        "<span>text & </span>",
         "jim &current joe",
+        "&&nbsp; &nbsp;&",
+        "jim &xx; joe",
         # Link with querystring items
         '<a href="http://example.com?foo=bar&bar=foo&amp;biz=bash">',
     ],
@@ -156,41 +159,47 @@ def test_bare_entities_get_escaped_correctly(text, expected):
 @pytest.mark.parametrize(
     "text, expected",
     [
-        # Test character entities
+        # Test character entities in text don't get escaped
         ("&amp;", "&amp;"),
         ("&nbsp;", "&nbsp;"),
         ("&nbsp; test string &nbsp;", "&nbsp; test string &nbsp;"),
         ("&lt;em&gt;strong&lt;/em&gt;", "&lt;em&gt;strong&lt;/em&gt;"),
-        # Test character entity at beginning of string
+        # Test character entity at beginning of string doesn't get escaped
         ("&amp;is cool", "&amp;is cool"),
-        # Test it at the end of the string
+        # Test character entity at end of the string doesn't get escaped
         ("cool &amp;", "cool &amp;"),
-        # Test bare ampersands and entities at beginning
+        # Test bare ampersands before an entity at the beginning of the string
+        # gets escaped
         ("&&amp; is cool", "&amp;&amp; is cool"),
-        # Test entities and bare ampersand at end
+        # Test ampersand after an entity at the end of the string gets escaped
         ("&amp; is cool &amp;&", "&amp; is cool &amp;&amp;"),
-        # Test missing semi-colon means we don't treat it like an entity
+        # Test missing semi-colons mean we don't treat the thing as an entity--Bleach
+        # only recognizes character entities that start with & and end with ;
         ("this &amp that", "this &amp;amp that"),
-        # Test a thing that looks like a character entity, but isn't because it's
-        # missing a ; (&current)
         (
             "http://example.com?active=true&current=true",
             "http://example.com?active=true&amp;current=true",
         ),
-        # Test character entities in attribute values are left alone
+        # Test character entities in attribute values are not escaped
         ('<a href="?art&amp;copy">foo</a>', '<a href="?art&amp;copy">foo</a>'),
         ('<a href="?this=&gt;that">foo</a>', '<a href="?this=&gt;that">foo</a>'),
-        # Ambiguous ampersands get escaped in attributes
+        # Things in attributes that aren't character entities get escaped
         (
             '<a href="http://example.com/&xx;">foo</a>',
             '<a href="http://example.com/&amp;xx;">foo</a>',
         ),
+        (
+            '<a href="http://example.com?&adp;">foo</a>',
+            '<a href="http://example.com?&amp;adp;">foo</a>',
+        ),
         (
             '<a href="http://example.com?active=true&current=true">foo</a>',
             '<a href="http://example.com?active=true&amp;current=true">foo</a>',
         ),
-        # Ambiguous ampersands in text are not escaped
-        ("&xx;", "&xx;"),
+        # Things in text that aren't character entities get escaped
+        ("&xx;", "&amp;xx;"),
+        ("&adp;", "&amp;adp;"),
+        ("&currdupe;", "&amp;currdupe;"),
         # Test numeric entities
         ("&#39;", "&#39;"),
         ("&#34;", "&#34;"),