Improved solution to #318

[migeruhito]

The Jinja2 automatic autoescape has been enabled. Now the flask application adopts the Flask defaults and the Jinja2 environment is initialized by the flask.Flask class. Now, any externally generated html must be declared explicitly as safe not to be escaped. This should help to make the Notebook more secure.

Does that solve any problem related to #319?

This changeset include non trivial changes and should be tested intensively. Several new bugs are expected.

[kcrisman]

Does that solve any problem related to #319?

I doubt it, as that is just sending an ordinary query to sagenb that one might make anyway.

[kcrisman]

See also #252.

[migeruhito]

I doubt it, as that is just sending an ordinary query to sagenb that one might make anyway.

The session cookie is needed. An ordinary request is not sufficient. Only a proper CSR works. But CSRF is usually exploited via XSS. The CSRF risk can be reduced by avoiding code injection. This patch points to this direction but I agree with you, I realize (#319) this doesn't solve the current XSS Notebook's problems.

[gutow]

Eyeball code review looks OK. I will try to test before the week is out. It even looks compatible with #328.

[gutow]

I haven't found anything this breaks and can verify that it is compatible with #328 as well. Positive review.

[kcrisman]

Thanks for that! I agree that nothing obviously bad sticks out. I still want to look at it carefully as well, though. @migeruhito - any sense on what you would most expect to break?

[migeruhito]

First I reverted #323, because this pull request itself fix #318. Not reverting that, could be confusing for a future collaborator. Redundant code is bad and not only for efficiency reasons.

any sense on what you would most expect to break?

It could break two kind of things:

1. Some parts of the notebook could not be correctly displayed and crude html code might appear.
2. The jinja2 template engine initialization is different. Now it is cleaner and standard, but now sagenb.notebook.template is flask.app-dependent. This module can't be used any more as and standalone html code generator. I think that the regular notebook code makes no use of this feature, but i don't know if it breaks any test suite.

[kcrisman]

@gutow - Thanks for the positive review. Do you think that any update you would make on #328 would affect the stuff here? I'd like to merge it but since #328 is so big I would rather do that one first unless it's orthogonal.

[migeruhito]

It seems to me that @gutow has partially merged this branch on gutow:master (my first two commits). Since that, I have made other commits and rebases. When this branch is merged with gutow:master, two conflicts arise:

1. The first one is on sagenb/data/sage/html/worksheet_listing.html. It is not important and is related with a blank that I have added.
2. The second is on sagenb/notebook/worksheet.py. I think this is not related to migeruhito:autoescape but to Simplify moving files from worksheet to cell #340/Copy instead of move such that owner is correct #349. My branch is rebased on those commits, but I don't know if they are properly merged on gutow:master.

[kcrisman]

Since @gutow has indicated he won't be able to look at his stuff immediately anyway, I'm happy to merge this based on his review. I'll just be looking at it again over the next few days and possibly trying a bit out, but expect merge soon. Thanks!

[kcrisman]

I'm testing this currently, expect to include in http://trac.sagemath.org/ticket/19616

[kcrisman]

It could break two kind of things:
The jinja2 template engine initialization is different. Now it is cleaner and standard, but now sagenb.notebook.template is flask.app-dependent. This module can't be used any more as and standalone html code generator. I think that the regular notebook code makes no use of this feature, but i don't know if it breaks any test suite.

Apparently it does.

----------------------------------------------------------------------
sage -t --long sagenb/notebook/cell.py  # 2 doctests failed
sage -t --long sagenb/notebook/worksheet.py  # 2 doctests failed
sage -t --long sagenb/notebook/notebook.py  # 8 doctests failed
sage -t --long sagenb/notebook/challenge.py  # 2 doctests failed
sage -t --long sagenb/notebook/template.py  # 4 doctests failed
----------------------------------------------------------------------

They all look like this:

sage -t --long sagenb/notebook/cell.py
**********************************************************************
File "sagenb/notebook/cell.py", line 558, in sagenb.notebook.cell.TextCell.?
Failed example:
    C.html()
Exception raised:
    Traceback (most recent call last):
<snip>
      File "/Users/.../sage/local/lib/python2.7/site-packages/Flask-0.10.1-py2.7.egg/flask/globals.py", line 34, in _find_app
        raise RuntimeError('working outside of application context')
    RuntimeError: working outside of application context
**********************************************************************

I wonder if there is an easy way to fix the doctest. Certainly the notebook itself seems to work fine.

[kcrisman]

To be clear, I don't want to leave it untested, because untested is even more broken, but I am happy to do any quick fixes I can if @migeruhito has any pointers for me. The type of test is exactly what you suspected:

            sage: nb = sagenb.notebook.notebook.load_notebook(tmp_dir(ext='.sagenb'))
            sage: nb.user_manager().add_user('sage','sage','sage@sagemath.org',force=True)
            sage: W = nb.create_new_worksheet('Test', 'sage')
            sage: C = sagenb.notebook.cell.Cell(0, '2+3', '5', W)
            sage: C.html()
            u'...cell_outer_0...2+3...5...'

So we just need some alternate way to test that this html is generated correctly without actually launching the app. As a first very naive attempt, I did

            sage: import jinja2
            sage: e = jinja2.Environment(loader=jinja2.FileSystemLoader(TEMPLATE_PATH))
            sage: e.filters['repr_str'] = lambda x: repr(unicode_str(x))[1:]
            sage: from flask.ext.babel import gettext
            sage: f = e.get_template(os.path.join('html', 'notebook', 'text_cell.html'))
            sage: f.render(cell=C)
            u'...text_cell...2+3...'

but that fails with UndefinedError: 'gettext' is undefined - perhaps it doesn't want to import flask nicely.

An alternate approach that would take more time is to create an app that doesn't actually launch anything that could be used only for testing purposes, but figuring out how to do that could take me quite a bit of time. E.g. here:

app = Flask(__name__)
ctx = app.app_context()
ctx.push()

but I don't know whether that would be a good idea to try. This discussion also seems relevant.

[migeruhito]

I think that your last approach is the way to be taken. I'm working on it.

[migeruhito]

The doctests are fixed (I hope).

[kcrisman]

Oh, I'm so pleased that some of my research was useful! Yes, it passes tests now. THANK you!

[kcrisman]

I was happy otherwise and @gutow likes it, so I'm merging. I think this will be the last thing in the next point release of sagenb.

[kcrisman]

@migeruhito - this seems to work pretty fine overall. But I am noticing that now the default "safe" mode means that certain live documentation doesn't work properly, because it uses things that are escaped.

E.g. http://localhost:8080/doc/live/tutorial/tour_rings.html should have something like this as the first cell:

ratpoly.<t> = PolynomialRing(QQ)
realpoly.<z> = PolynomialRing(RR)

But of course this is escaped to

ratpoly.<t> = PolynomialRing(QQ)
realpoly.<z> = PolynomialRing(RR)

Neither is

var('z')   # define z to be a variable

instead of

var('z')   # define z to be a variable

in http://localhost:8080/doc/live/tutorial/tour_functions.html

Any ideas how to get around that? I believe there should be some setting for live cells and maybe in that event we could add |safe inside the cells. Presumably we already would do this for the cells in normal worksheets!

[kcrisman]

Maybe at https://github.com/sagemath/sagenb/blob/master/sagenb/data/sage/html/notebook/cell.html#L83

            {% else %}
                <textarea class="{{ input_cls }}" rows="{{ (1, cell.input_text().strip()|number_of_rows(80))|max }}"
                          cols="80" spellcheck="false"
                          id="cell_input_{{ cell.id() }}">{{ cell.input_text().rstrip() }}</textarea>
                <input type="button"
                   class="eval_button"
                   id="eval_button{{ cell.id() }}" 
                   title="{{ gettext('Click here or press shift-return to evaluate') }}"
                   value="{{ gettext('evaluate') }}" />
            {% endif %}

could have a |safe added in cell.input_text().rstrip(), at least { % if cell.worksheet().docbrowser() }?

[migeruhito]

I'm working on it.

[migeruhito]

If we add | safe in cell.input_text().rstrip() we have the same behavior as before merging autoscape. It seems to solve the problem, but... The notebook was buggy at this point. For instance, in a previous version of the notebook (for example at tag 0.11.4):

1. Create a new worksheet.
2. Type </textarea> on the first cell.
3. Save and exit.
4. Open the worksheet again.
5. Oh! uh!, where has my </textarea> gone?

This works fine with autoescape.

The actual problem is the following:

• With autoescape the cell content is ratpoly.<t> = ....
• Without autoescape the cell content is ratpoly.<t> = ..., that is correct.

It seems that a | safe or a flask.Markup must be added somewhere. I'll try to find where.

Still working.

[migeruhito]

A worst example of the above mentioned bug:

1. Create a new worksheet.
2. Type a = '<';a on the first cell and evaluate.
3. Save and exit.
4. Reopen the worksheet.
5. See what happen. Funny.

Now the bug persists with the autoescaped notebook.
Correction: The bug is not present with the current autoescaped version

I think that the problem is in the methods to save and load worksheets.

[migeruhito]

See #354 for a (suboptimal) solution.