1197471 - SQL syntax error in collections view

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Discovery Pane, defect)

Description

[Kris Maglione [:kmag]]

I don't know if this is exploitable, but the fact that there's an SQL syntax error at all suggests that we're doing something very wrong.

This is just another one of the several places we generate raw SQL and assume that escaping is not necessary. Why do we do this at all?

Sentry URL:

http://sentry.mktmon.services.phx1.mozilla.com/mkt/addonsmozillaorg/group/17726/


Aggregate Details:

* First Seen: July 21, 2015
* Last Seen: 18 hours ago
* Number of tracebacks: undefined


Traceback:

Stacktrace (most recent call last):

  File "django/core/handlers/base.py", line 112, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "newrelic/packages/wrapt/wrappers.py", line 454, in __call__
    args, kwargs)
  File "newrelic/hooks/framework_django.py", line 492, in wrapper
    return wrapped(*args, **kwargs)
  File "django/views/decorators/csrf.py", line 57, in wrapped_view
    return view_func(*args, **kwargs)
  File "amo/decorators.py", line 51, in wrapper
    return f(request, *args, **kw)
  File "discovery/views.py", line 227, in recommendations
    c.set_addons(addon_ids)
  File "bandwagon/models.py", line 571, in set_addons
    VALUES %s""" % ','.join(values))
  File "django/db/backends/util.py", line 53, in execute
    return self.cursor.execute(sql, params)
  File "django/db/utils.py", line 99, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "django/db/backends/util.py", line 51, in execute
    return self.cursor.execute(sql)
  File "django/db/backends/mysql/base.py", line 124, in execute
    return self.cursor.execute(query, args)
  File "newrelic/hooks/database_dbapi2.py", line 22, in execute
    *args, **kwargs)
  File "MySQLdb/cursors.py", line 205, in execute
    self.errorhandler(self, exc, value)
  File "MySQLdb/connections.py", line 36, in defaulterrorhandler
    raise errorclass, errorvalue

Comment 1

[Mathieu Agopian [:magopian]]

From my investigation, this happens where an empty addon GUIDs list is posted to the recommendations view from the discovery pane:
https://github.com/mozilla/olympia/blob/6b7c51bc2ae2b7f2420ab4a5452be80604633c75/apps/discovery/views.py#L227
then
https://github.com/mozilla/olympia/blob/6b7c51bc2ae2b7f2420ab4a5452be80604633c75/apps/bandwagon/models.py#L571

I don't think this is vulnerable, as we call set_addons with a list of add-on IDs using https://github.com/mozilla/olympia/blob/6b7c51bc2ae2b7f2420ab4a5452be80604633c75/apps/discovery/views.py#L252. :kmag what do you think, can you remove the security flag?

The fix consists of checking that there's addons in the list before trying to "set" them ;)

Comment 2

[Kris Maglione [:kmag]]

I can't remove the security flag, alas. Only members of the security group can.

Can you think of any reason not to remove the raw SQL entirely, there? That seems like the more correct fix.

Comment 3

[Mathieu Agopian [:magopian]]

To be honest, it's exactly what I'm thinking about ;) It's simply a matter of calling a bulk create, and be done with it.

Comment 4

[Mathieu Agopian [:magopian]]

PR: https://github.com/mozilla/olympia/pull/680

Comment 5

[[github robot]]

Commits pushed to master at https://github.com/mozilla/olympia

https://github.com/mozilla/olympia/commit/bb6e0bdfc2eaae86a1030491e72f61af8bcb5f06
Replace generated raw sql: fix error on empty addons list (bug 1197471)

https://github.com/mozilla/olympia/commit/6f63a7ddad976c05700df2bb07e75f19a314b1cb
Merge pull request #680 from magopian/1197471-recs-check-addons-posted

Replace generated raw sql: fix error on empty addons list (bug 1197471)