Issue 3402 - Properly handle nested inline tags

cms/converters.py

 # coding: utf-8
 
 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2015 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
@@ skipping 163 matching lines @@
 
     # Insert attributes
     result = escape(result)
     def stringify_attribute((name, value)):
       return '%s="%s"' % (
         escape(name),
         escape(self.insert_localized_strings(value, {}))
       )
 
     for tag in self.whitelist:
+      allowed_tags = [t % re.escape(tag_name)
+                      for t in ("\<%s[^<>]*?\>", "\<\/%s\>")

[Sebastian Noack, 2016/01/21 13:55:56]

Nit: None of your escaping applies to regexp as you don't use double backslashes
or  r"..". However, that shows that escaping isn't even necessary here.

Nit: You could simplify the surrounding Python code if you use a single regexp:
"<(?:%s[^<>]*?|/%s)>"

[kzar, 2016/02/19 14:32:28]

Done.

+                      for tag_name in self.whitelist.difference({tag})]

[Sebastian Noack, 2016/01/21 13:55:56]

Nit: self.whitelist - {tag}

Besides being more concise, it's probably also easier to understand as it's
clear that arithmetic semantics apply and you don't wonder whether
Set.difference() is some symmetric operation.

[kzar, 2016/02/19 14:32:28]

Done.

+      allowed_contents = "(?:%s)" % "|".join(["[^<>]"] + allowed_tags)
       saved = saved_attributes.get(tag, [])
       for attrs in saved:
         attrs = map(stringify_attribute, attrs)
         result = re.sub(
-          r"%s([^<>]*?)%s" % (re_escape("<%s>" % tag), re_escape("</%s>" % tag)),
+          r"%s(%s*?)%s" % (re_escape("<%s>" % tag), allowed_contents,

[Sebastian Noack, 2016/01/21 13:55:56]

I wonder whether you should parse the content recursively to make sure that tags
are closed correctly.

[kzar, 2016/02/19 14:32:28]

I guess so but probably overkill, we didn't check that before.

+                           re_escape("</%s>" % tag)),
           lambda match: r'<%s%s>%s</%s>' % (
             tag,
             " " + " ".join(attrs) if attrs else "",
             match.group(1),
             tag
           ),
           result, 1, flags=re.S
         )
       result = re.sub(
-        r"%s([^<>]*?)%s" % (re_escape("<%s>" % tag), re_escape("</%s>" % tag)),
+        r"%s(%s*?)%s" % (re_escape("<%s>" % tag), allowed_contents,
+                         re_escape("</%s>" % tag)),
         r"<%s>\1</%s>" % (tag, tag),
         result, flags=re.S
       )
     return result
 
   def insert_localized_strings(self, text, escapes, to_html=lambda s: s):
     def lookup_string(match):
       name, comment, default = match.groups()
       if default:
         default = to_html(default).strip()
@@ skipping 228 matching lines @@
         stack.pop()
       stack[-1]["subitems"].append(item)
       stack.append(item)
     return structured
 
 converters = {
   "html": RawConverter,
   "md": MarkdownConverter,
   "tmpl": TemplateConverter,
 }