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 124 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*(.*?)\s*:\s*(.*)')
 INCLUDE_REGEXP = re.compile(r'%include\s+(.+)%')
-HEADER_REGEXP = re.compile(r'\[(Adblock(?:\s*Plus\s*[\d\.]+?)?)\]', flags=re.I)
+HEADER_REGEXP = re.compile(r'\[(?:(Adblock(?:\s*Plus\s*[\d\.]+?)?)|.*)\]',
+                           flags=re.I)
 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)
-    return Header(match.group(1))
 
 
 def _parse_instruction(text):
     match = INCLUDE_REGEXP.match(text)
     if not match:
         raise ParseError('Unrecognized instruction', text)
     return Include(match.group(1))
 
 
 def _parse_option(option):
@@ skipping 77 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, 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.

+def parse_line(line, position='body'):
     """Parse one line of a 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.

+    The types of lines that that the parser recognizes depend on the position.
+    If position="body", the parser only recognizes filters, comments,
+    processing instructions and empty lines. If position="metadata", it in
+    addition recognizes metadata. If position="start", it also recognizes
+    headers.
+
+    Note: Checksum metadata lines are recognized in all positions for backwards
     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 : str
         Line of a filter list.
-    mode : str
-        Parsing mode, one of "start", "metadata" or "body" (default).
+    position : str
+        Position in the filter list, one of "start", "metadata" or "body"
+        (default is "body").
 
     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()
-
-    if content == '':
+    POSITIONS = {'body', 'start', 'metadata'}
+    if position not in POSITIONS:
+        raise ValueError('position should be one of {}'.format(POSITIONS))
+
+    if isinstance(line, type(b'')):
+        line = line.decode('utf-8')
+
+    stripped = line.strip()
+
+    if stripped == '':
         return EmptyLine()
 
-    if content.startswith('!'):
-        match = METADATA_REGEXP.match(line_text)
+    if position == 'start':
+        match = HEADER_REGEXP.search(line)
+        if match:
+            version = match.group(1)
+            if not version:
+                raise ParseError('Malformed header', line)
+            return Header(version)
+
+    if stripped.startswith('!'):
+        match = METADATA_REGEXP.match(line)
         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':
+            if position != '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)
+        return Comment(stripped[1:].lstrip())
+
+    if stripped.startswith('%') and stripped.endswith('%'):
+        return _parse_instruction(stripped)
+
+    return parse_filter(stripped)
 
 
 def parse_filterlist(lines):
     """Parse filter list from an iterable.
 
     Parameters
     ----------
     lines: iterable of str
         Lines of the filter list.
 
@@ skipping 15 matching lines @@
     for line in lines:
         parsed_line = parse_line(line, position)
         yield parsed_line
 
         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'