Issue 6877 - Only parse headers in the first line of the filter list

abp/filters/parser.py

 # This file is part of Adblock Plus <https://adblockplus.org/>,
 # Copyright (C) 2006-present 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,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
@@ skipping 122 matching lines @@
 
 
 Header = _line_type('Header', 'version', '[{.version}]')
 EmptyLine = _line_type('EmptyLine', '', '')
 Comment = _line_type('Comment', 'text', '! {.text}')
 Metadata = _line_type('Metadata', 'key value', '! {0.key}: {0.value}')
 Filter = _line_type('Filter', 'text selector action options', '{.text}')
 Include = _line_type('Include', 'target', '%include {0.target}%')
 
 
-METADATA_REGEXP = re.compile(r'(.*?)\s*:\s*(.*)')
+METADATA_REGEXP = re.compile(r'\s*!\s*(.*?)\s*:\s*(.*)')
 INCLUDE_REGEXP = re.compile(r'%include\s+(.+)%')
 HEADER_REGEXP = re.compile(r'\[(Adblock(?:\s*Plus\s*[\d\.]+?)?)\]', flags=re.I)

[Sebastian Noack, 2018/09/18 15:19:08]

Does this regexp is missing a $ at the end? I didn't test it, but to me it seems
we would incorrectly consider "[Adblock]]" a valid header.

 HIDING_FILTER_REGEXP = re.compile(r'^([^/*|@"!]*?)#([@?])?#(.+)$')
 FILTER_OPTIONS_REGEXP = re.compile(
     r'\$(~?[\w-]+(?:=[^,]+)?(?:,~?[\w-]+(?:=[^,]+)?)*)$'
 )
 
 
 def _parse_header(text):
     match = HEADER_REGEXP.match(text)
     if not match:
         raise ParseError('Malformed header', text)
@@ skipping 88 matching lines @@
         Parsed filter.
 
     """
     if '#' in text:
         match = HIDING_FILTER_REGEXP.search(text)
         if match:
             return _parse_hiding_filter(text, *match.groups())
     return _parse_blocking_filter(text)
 
 
-def parse_line(line_text):
+def parse_line(line_text, mode='body'):

[Sebastian Noack, 2018/09/18 15:19:08]

We probably should call the argument here "position" too.

[Vasily Kuznetsov, 2018/09/18 18:11:44]

I originally left it as "mode" on purpose, but now it seems that "position" is
also a good name and it's more consistent with the use in parse_filterlist().

Done.

     """Parse one line of a filter list.
 
-    Note that parse_line() doesn't handle special comments, hence never returns
-    a Metadata() object, Adblock Plus only considers metadata when parsing the
-    whole filter list and only if they are given at the top of the filter list.
+    The types of lines that that the parser recognizes depend on the mode. In
+    body mode the parser only recognizes filters, comments, processing
+    instructions and empty lines. In medata mode it in addition recognizes
+    metadata. In start mode it also recognizes headers.
+
+    Note: checksum metadata lines are recognized in all modes for backwards

[Sebastian Noack, 2018/09/18 15:19:07]

Typo: Capitalize the first word after a colon.

[Vasily Kuznetsov, 2018/09/18 18:11:44]

Done.

+    compatibility. Historically, checksums can occur at the bottom of the
+    filter list. They are are no longer used by Adblock Plus, but in order to
+    strip them (in abp.filters.renderer), we have to make sure to still parse
+    them regardless of their position in the filter list.
 
     Parameters
     ----------
     line_text : str
         Line of a filter list.
+    mode : str
+        Parsing mode, one of "start", "metadata" or "body" (default).
 
     Returns
     -------
     namedtuple
         Parsed line (see `_line_type`).
 
     Raises
     ------
     ParseError
         ParseError: If the line can't be parsed.
+
     """
+    MODES = {'body', 'start', 'metadata'}
+    if mode not in MODES:
+        raise ValueError('mode should be one of {}'.format(MODES))
+
     if isinstance(line_text, type(b'')):
         line_text = line_text.decode('utf-8')
 
     content = line_text.strip()

[Sebastian Noack, 2018/09/18 15:19:07]

Given the only difference between line_text and content is that the latter is
stripped, and we now have logic that dependent on the case either needs the
stripped or unstripped string, perhaps the code is easier to understand if we
rename this variable to "stripped".

 
     if content == '':
-        line = EmptyLine()
-    elif content.startswith('!'):
-        line = Comment(content[1:].lstrip())
-    elif content.startswith('%') and content.endswith('%'):
-        line = _parse_instruction(content)
-    elif content.startswith('[') and content.endswith(']'):
-        line = _parse_header(content)
-    else:
-        line = parse_filter(content)
+        return EmptyLine()
 
-    assert line.to_string().replace(' ', '') == content.replace(' ', '')
-    return line
+    if content.startswith('!'):
+        match = METADATA_REGEXP.match(line_text)
+        if match:
+            key, value = match.groups()
+            # Metadata is only parsed in start or metadata modes but there's
+            # an exception for checksums (see docstring).

[Sebastian Noack, 2018/09/18 15:19:08]

Nit: IMO, this comment is redundant. That we special case "checksum" is obvious
from the code, the reason why is no covered in the docstring.

[Vasily Kuznetsov, 2018/09/18 18:11:43]

Done.

+            if mode != 'body' or key.lower() == 'checksum':
+                return Metadata(key, value)
+        return Comment(content[1:].lstrip())
+
+    if content.startswith('%') and content.endswith('%'):
+        return _parse_instruction(content)
+
+    if mode == 'start' and (line_text.startswith('[') and
+                            line_text.endswith(']')):

[Sebastian Noack, 2018/09/18 15:19:08]

Nit: Perhaps just rename the variable (e.g. to "line"), so that you don't need
to wrap here.

[Vasily Kuznetsov, 2018/09/18 18:11:43]

Done.

+        return _parse_header(content)

[Sebastian Noack, 2018/09/18 15:19:08]

The result should be the same, since by asserting that the lines starts end ends
with square brackets, it's implied that there are no leading or trailing
whitespaces, but it might be easier to verify the code without having to make
that assumption by passing in line_text instead of content here.

[Vasily Kuznetsov, 2018/09/18 18:11:43]

Done.

+
+    return parse_filter(content)
 
 
 def parse_filterlist(lines):
     """Parse filter list from an iterable.
 
     Parameters
     ----------
     lines: iterable of str
         Lines of the filter list.
 
     Returns
     -------
     iterator of namedtuple
         Parsed lines of the filter list.
 
     Raises
     ------
     ParseError
         Thrown during iteration for invalid filter list lines.
     TypeError
         If `lines` is not iterable.
 
     """
-    metadata_closed = False
+    position = 'start'
 
     for line in lines:
-        result = parse_line(line)
+        parsed_line = parse_line(line, position)
+        yield parsed_line
 
-        if result.type == 'comment':
-            match = METADATA_REGEXP.match(result.text)
-            if match:
-                key, value = match.groups()
-
-                # Historically, checksums can occur at the bottom of the
-                # filter list. Checksums are no longer used by Adblock Plus,
-                # but in order to strip them (in abp.filters.renderer),
-                # we have to make sure to still parse them regardless of
-                # their position in the filter list.
-                if not metadata_closed or key.lower() == 'checksum':
-                    result = Metadata(key, value)
-
-        if result.type not in {'header', 'metadata'}:
-            metadata_closed = True
-
-        yield result
+        if position != 'body' and parsed_line.type in {'header', 'metadata'}:
+            # Continue parsing metadata until it's over...
+            position = 'metadata'
+        else:
+            # ...then switch to parsing the body.
+            position = 'body'