Issue 6920 - Only parse metadata from the top of the file

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*(?:([\w-]+)|(?:\S.*?))\s*:\s*(.*)')
+METADATA_REGEXP = re.compile(r'([\w-]+)\s*:\s*(.*)')
 INCLUDE_REGEXP = re.compile(r'%include\s+(.+)%')
 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_comment(text, metadata_closed):
-    match = METADATA_REGEXP.match(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 key and (not metadata_closed or key.lower() == 'checksum'):
-            return Metadata(key, value), metadata_closed
-    else:
-        # The regular expression matches as well if we see a malformed key
-        # (e.g. "Last modified"). In that case we want to keep looking for
-        # more metadata, but return a Comment instead of a Metadata object.
-        # Hence we only consider the the metadata section closed if the
-        # regular expression doesn't match.
-        metadata_closed = True
-    return Comment(text[1:].lstrip()), metadata_closed
-
-
 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:
@@ skipping 82 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, metadata_closed):
-    if isinstance(line_text, type(b'')):
-        line_text = line_text.decode('utf-8')
-
-    content = line_text.strip()
-
-    if content.startswith('!'):
-        line, metadata_closed = _parse_comment(content, metadata_closed)
-    elif content.startswith('[') and content.endswith(']'):
-        line = _parse_header(content)
-    else:
-        if content == '':
-            line = EmptyLine()
-        elif content.startswith('%') and content.endswith('%'):
-            line = _parse_instruction(content)
-        else:
-            line = parse_filter(content)
-        metadata_closed = True
-
-    return line, metadata_closed
-
-
 def parse_line(line_text):
     """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.
 
     Parameters
     ----------
     line_text : str
         Line of a filter list.
 
     Returns
     -------
     namedtuple
         Parsed line (see `_line_type`).
 
     Raises
     ------
     ParseError
         ParseError: If the line can't be parsed.
     """
-    return _parse_line(line_text, True)[0]

[Vasily Kuznetsov, 2018/09/04 10:03:30]

Here we're changing the behavior of `parse_line` -- it will never parse comments
as metadata now. This could create problems for people who use `parse_line`
directly (and it's part of the external API). I also think it would be good to
still be able to parse metadata comments using `parse_line`.

I would propose that we have `metadata_closed` parameter in this function with a
default of `False`. This way the behavior will be the same as before for people
who use it directly (and they can choose if they want to disable metadata
comment parsing as they like). We would need to move the logic of determining
when to consider metadata closed into `parse_filterlist` (now it's in
`_parse_comment`) but arguably that's where it belongs anyway.

[Sebastian Noack, 2018/09/04 16:01:03]

I moved the logic as requested to parse_filterlist(). But IMO, this makes it
even more clear why parse_line() should never return a Metadata() object.
Metadata (and headers but there is a separate issue for them) only have any
meaning in the context of the whole filter list (and only if they occur in a
particular position). This is consistent with how the parser in Adblock Plus is
implemented, i.e. right after downloading a filter list, the header and metadata
are processed, and then the remaining lines are parsed individually.

[Vasily Kuznetsov, 2018/09/04 16:49:50]

What I had in mind was to keep _parse_comment() with metadata_closed parameter
and to also have it in parse_line() -- this way the caller could control what
behavior they want. Then the logic in parse_filterlist() would be high level,
something like parse_line(..., metadata_closed=False) until we want to stop
accepting metadata and after that parse_line(..., metadata_close=True).

Unfortunately my plan didn't take into account malformed keys like "Last
modified". The logic we have for them is rather special, we consider them
regular comments but we still don't close metadata. I suppose we could consider
them regular comments in the middle of metadata if we allow that -- you can
judge better whether this would work in practice or not.

+    if isinstance(line_text, type(b'')):
+        line_text = line_text.decode('utf-8')
+
+    content = line_text.strip()
+
+    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)
+
+    assert line.to_string().replace(' ', '') == content.replace(' ', '')
+    return line
+
 
 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

[Vasily Kuznetsov, 2018/09/04 10:03:30]

What do you think about filter lists like this?

    [Adblock  Plus 1.1]
    ! Homepage: https://foo.bar/
    ! This is the filter list that foos the bar.
    ! Expires: 1 day
    ||example.com^

It seems that allowing comments inside of the "header" part could be useful for
the filter authors and it looks kind of natural. Maybe we should declare the
metadata closed on the first filter instead of on the first obviously
non-metadata comment? Or maybe (also) on an empty line?

Another example, that could be handled not the way we like:

    [Adblock Plus 1.1]
    ! Homepage: https://foo.bar

    ! WARNING: The following filters will drink your beer
    ||beer.de

[Sebastian Noack, 2018/09/04 16:01:03]

In practice nobody seems to put comments in between metadata. But what you see
(a lot) in practice are comments right after metadata (with no blank line in
between), e.g. EasyList is doing that. So I think the logic I implemented here
makes more sense.

[Vasily Kuznetsov, 2018/09/04 16:49:50]

Well, "Last modified" is considered a comment and it is in the middle of
metadata, so this does happen. Maybe we should just consider such comments still
metadata and get more consistent behavior?

+
     for line in lines:
-        parsed, metadata_closed = _parse_line(line, metadata_closed)
-        yield parsed
+        result = parse_line(line)
+
+        if isinstance(result, 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':
+                    yield Metadata(key, value)
+                    continue
+
+            if not result.text:
+                metadata_closed = True
+        elif not isinstance(result, Header):
+            metadata_closed = True
+
+        yield result