Issue 395 - Filter hits statistics backend

sitescripts/filterhits/web/query.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,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
+import json
 import os
+import traceback
+from urlparse import parse_qsl
+
 import MySQLdb
-import json
-from urlparse import parse_qsl
 
 from sitescripts.web import url_handler
 from sitescripts.utils import cached, setupStderr
-from sitescripts.filterhits import common, db
+from sitescripts.filterhits import db
+from sitescripts.filterhits.web import common
 
-def query(domain=None, filter=None, skip=0, take=20, order="DESC", order_by="hits", **_):
+def query(domain=None, filter=None, skip=0, take=20, order="DESC", order_by="frequency", **_):
   """
   Returns the SQL and parameters needed to perform a query of the filterhits data.
   """
-  sql = """SELECT SQL_CALC_FOUND_ROWS domain, filter, hits
+  sql = """SELECT SQL_CALC_FOUND_ROWS domain, filter, frequency
            FROM frequencies as freq
            LEFT JOIN filters as f ON f.sha1=freq.filter_sha1
            %s
            ORDER BY %s
            LIMIT %%s, %%s"""
 
-  where_fields = [(s, "%" + p + "%") for s, p in (("domain", domain),
-                                                  ("filter", filter)) if p]
-  where = " AND ".join([f[0] + " LIKE %s" for f in where_fields])
-  where_sql = "WHERE " + where if where else ""

[Wladimir Palant, 2015/03/27 16:29:06]

This is confusing, why the intermediate step?

where_fields = ["%s LIKE %%%s%%" % (s, p) for s, p in ...]
where_sql = "WHERE " + " AND ".join(where_fields) if where_fields else ""

However, the value we are searching for should *not* be inserted into the query
- that's an SQL injection vulnerability. *All* dynamic strings should be added
to the parameters of the prepared statement. So it should really be something
like:

where_fields, params = zip(*[("%s LIKE %%s" % s, "%%%s%%" % p) for s, p in ...])
where_sql = "WHERE " + " AND ".join(where_fields) if where_fields else ""

[kzar, 2015/03/27 22:15:00]

You're right this code was confusing, I hadn't looked at it for a while and I
found it hard to re-grok. I've tidied it up a bit now like you suggested.
(Although unfortunately I needed to add an extra step as we can't unpack a
potentially empty array into two values.)

That said in my defence I don't think there was a SQL vulnerability here, f[0]
is either "domain" or "filter" and did not come from user input. f[1] is used
below, when it is added to the parameter list which is then passed to the
database query function. The database query function should then take care of
the escaping of those parameters for us.

+  where = zip(*[("%s LIKE %%s" % s, "%%%s%%" % p) for s, p in (("domain", domain),
+                                                               ("filter", filter)) if p])
+  if where:
+    where_fields, params = where
+    where_sql = "WHERE " + " AND ".join(where_fields)
+  else:
+    where_sql = ""
+    params = []
 
   order = order.upper() if order.upper() in ("ASC", "DESC") else "ASC"
-  order_by_sql = "`%s` %s" % (MySQLdb.escape_string(order_by), order)

[Wladimir Palant, 2015/03/27 16:29:06]

How about you only allow certain values for order_by instead of escaping some
bogus field name? As I said, no dynamic strings in the query please.

[kzar, 2015/03/27 22:15:00]

Done.

+  if order_by not in ["filter", "domain", "frequency"]:
+    order_by = "frequency"
+  order_by_sql = "`%s` %s" % (order_by, order)
 
-  params = [f[1] for f in where_fields] + [int(skip), int(take)]
+  params = list(params) + [int(skip), int(take)]
   return [sql % (where_sql, order_by_sql)] + params
 
 @url_handler("/query")
 def query_handler(environ, start_response):
   setupStderr(environ["wsgi.errors"])
-  params = dict(parse_qsl(environ.get('QUERY_STRING', '')))
+  params = dict(parse_qsl(environ.get("QUERY_STRING", "")))
 
   try:
     db_connection = db.connect()
     try:
       results = db.query(db_connection, *query(**params), dict_result=True)
       total = db.query(db_connection, "SELECT FOUND_ROWS()")[0][0]
     finally:
       db_connection.close()
   except MySQLdb.Error:
+    traceback.print_exc()
     return common.show_error("Failed to query database!", start_response,
                              "500 Database error")

[kzar, 2015/03/27 22:15:00]

Done.

 
   try:
     echo = int(params["echo"])
   except (ValueError, KeyError):
     echo = 0
 
-  response_headers = [("Content-type", "application/json")]

[Wladimir Palant, 2015/03/27 16:29:06]

"application/json; charset=utf-8" please.

+  response_headers = [("Content-type", "application/json; charset=utf-8")]
   start_response("200 OK", response_headers)
   return [json.dumps({"results": results, "echo": echo,
-                      "total": total, "count": len(results)})]

[Wladimir Palant, 2015/03/27 16:29:06]

This should be json.dumps(..., ensure_ascii=False).encode("utf-8")

[kzar, 2015/03/27 22:15:00]

Done.

+                      "total": total, "count": len(results)},
+                     ensure_ascii=False).encode("utf-8")]