Website menu with Flask

Question

I wrote some code in Flask for site menu:

def menu(parent_id=0, menutree=None):
    menutree = menutree or []
    cur = g.db.execute('select id, parent, alias, title, ord from static where parent="'+ str(parent_id) +'" and ord>0 order by ord')
    fetch = cur.fetchall()

    if not fetch:
        return None

    return [{'id':raw[0], 'parent':raw[1], 'alias':raw[2], 'title':raw[3], 'sub':menu(raw[0])} for raw in fetch]

The data is taken from the sqlite3 table:

create table static (
  id integer primary key autoincrement,
  parent integer,
  alias string not null,
  title string not null,
  text string not null,
  ord integer
);

Variable (menu_list) is transmitted to template in each route:

@app.route('/')
def index():
    menu_list = menu()
    [...]
    return render_template('index.tpl', **locals())

Despite the fact that the code is more or less normal (except for prepared statements in a query to the database), the template is not very good:

<nav role="navigation">
  {% for menu in menu_list %}
  <li>
    <a{% if page_id == menu.id %} class="active"{% endif %} href="/{{ menu.alias }}">{{ menu.title }}</a>
    {% if menu.sub %}
    <ul>
      {% for sub in menu.sub %}
      <li><a href="/{{ menu.alias }}/{{ sub.alias }}">{{ sub.title }}</a>
        {% if sub.sub %}
        <ul>
          {% for subsub in sub.sub %}
          <li><a href="/{{ menu.alias }}/{{ sub.alias }}/{{ subsub.alias }}">{{ subsub.title }}</a>
          {% endfor %}
        </ul>
        {% endif %}
      </li>
      {% endfor %}
    </ul>
    {% endif %}
  </li>
  {% endfor %}
</nav>

Is it possible to improve the existing code / output / logic?

Answer 1

In this code, at first I didn't see the "danger" until I scrolled to the right by accident:

def menu(parent_id=0, menutree=None):
    menutree = menutree or []
    cur = g.db.execute('select id, parent, alias, title, ord from static where parent="'+ str(parent_id) +'" and ord>0 order by ord')
    fetch = cur.fetchall()

    if not fetch:
        return None

    return [{'id':raw[0], 'parent':raw[1], 'alias':raw[2], 'title':raw[3], 'sub':menu(raw[0])} for raw in fetch]

In the g.db.execute statement you're embedding a parameter and we cannot know where the parameter comes from and if it was properly validated to prevent SQL injection. Two things to do here:

1. Make the line shorter, especially when it contains potentially dangerous stuff in the far right
2. Use prepared statements with ? placeholder

It's not clear what kind of database you're using, so you might need to edit, but it should be something like this, shorter and without embedded parameters:

cur = g.db.execute('select id, parent, alias, title, ord '
                   'from static where parent = ? and ord > 0 '
                   'order by ord', parent_id)

Another small tip here, if you reverse the checking logic of if not fetch to if fetch at the end, you don't need to return None, as that's the default anyway, and the method will be a bit shorter:

def menu(parent_id=0, menutree=None):
    menutree = menutree or []
    cur = g.db.execute('select id, parent, alias, title, ord '
                       'from static where parent = ? and ord > 0 '
                       'order by ord', parent_id)
    fetch = cur.fetchall()

    if fetch:
        return [{'id': raw[0], 'parent': raw[1], 'alias': raw[2],
                 'title': raw[3], 'sub': menu(raw[0])} for raw in fetch]

And a tiny thing, PEP8 dictates to put a space after the : in a key:value pair, like so: key: value.

---

In this code:

@app.route('/')
def index():
    menu_list = menu()
    [...]
    return render_template('index.tpl', **locals())

I recommend to NOT include all **locals(), but use the list of variables you want to pass explicitly. We're all humans, one day you might accidentally expose something.