Refactor notebook templates to use Jinja2

[crbates]

This is a new version of PR #2354 (squashing commits and then pushing made github very angry) that closes the first part of #1585

[travisbot]

This pull request fails (merged 8ff5f90 into 3e7df85).

[ellisonbg]

Thanks for working on this! I am still +1 on moving to jinja2 but haven't had the chance to review the code yet. One thing to keep in mind is that we are eventually going to move to a model where each page or web service in the web app has its own subdirectories with its templates, js, CSS and handlers. that is part of the reason I wanted to move to ninja is that it makes that easier. But that will probably be in a later PR.

[travisbot]

This pull request passes (merged 8ff5f90 into 3e7df85).

[crbates]

Whoops still learning github. I tried re-arranging folders, but I usually ended up with something that seemed less organized than when I started and had lots of duplicates. I'm still figuring out the internals of the tornado/ipython/javascript interplay and how to best organize all that with jinja2.

[ellisonbg]

Let's save the folder rearranging to a later PR.

On Sat, Sep 1, 2012 at 11:06 AM, Cameron Bates notifications@github.comwrote:

Whoops still learning github. I tried re-arranging folders, but I usually
ended up with something that seemed less organized than when I started and
had lots of duplicates. I'm still figuring out the internals of the
tornado/ipython/javascript interplay and how to best organize all that with
jinja2.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2363#issuecomment-8215162.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

[Carreau]

I'll still wait for @ellisonbg full review, but it's ok for me.

[crbates]

@ellisonbg I have another commit to check the notebook metadata for an alternative notebook.html location. Should this be it's own PR?

[ellisonbg]

Can you describe a bit more about what you mean?

On Tue, Sep 25, 2012 at 1:18 PM, Cameron Bates notifications@github.comwrote:

@ellisonbg https://github.com/ellisonbg I have another commit to check
the notebook metadata for an alternative notebook.html location. Should
this be it's own PR?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2363#issuecomment-8868639.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

[crbates]

The commit is at: c05ad05. It checks for the template metadata object in a notebook when loaded and render the notebook using notebook.html in the provided absolute directory.

[ellisonbg]

Cameron,

Hi, on had a quick look at this and, unless I don't understand what this is
doing, I think we need to steer clear of tightly coupling the notebook
format to the notebook server details. Consider the following:

• Someone writes a completely different notebook server or maybe even a
  native app. This other notebook server/native app would have no idea what
  to do with this metadata attribute and the notebook would behave the same
  for those users.
• We rewrite the internals of the notebook server (this is actually going
  to happen) and completely break the model of what you are doing, so that
  everyone who has notebooks with this metadata attribute have broken
  notebooks.
• The Ruby folks decid they love the notebook format and start to use it in
  ways we never intended, completely decoupled from our notebook server.

Also, from a philosophical perspective, we need to keep a very long term,
big picture for the notebook format, including various metadata attributes.

But the bigger question is this: why do you want this capability? Maybe
then we can see if there is a way of supporting it with less coupling
between the format and server.

One final note about design ideas for the notebook. We want to minimize
the configurability/customizability of the notebook (it commits us to
brittle implementation details) and maximize the reusability of it (create
modular building block that other people can assemble in cool new ways).
From this perspective, I feel this commit is going in the wrong direction.

Note that moving to jinja2 will help us build a more modular code base -
it's just the first step though.

On Tue, Sep 25, 2012 at 1:53 PM, Cameron Bates notifications@github.comwrote:

The commit is at: c05ad05c05ad05.
It checks for the template metadata object in a notebook when loaded and
render the notebook using notebook.html in the provided absolute directory.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2363#issuecomment-8869863.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

[crbates]

Good to know. I was just exploring this as an alternative to putting html + javascript in a markdown cell because I am having issues when I accidentally render the markdown cell multiple times. I am running into this when adding javascript timers to markdown cells because there is no way to know the cell is being re-rendered and the old javascript continues to run.

[ellisonbg]

We are likely going to disallow JavaScript in markdown cells, so I would
move away from doing that. It turns out there are too many security issues
with it.

On Tue, Sep 25, 2012 at 11:07 PM, Cameron Bates notifications@github.comwrote:

Good to know. I was just exploring this as an alternative to putting html

• javascript in a markdown cell because I am having issues when I
  accidentally render the markdown cell multiple times. I am running into
  this when adding javascript timers to markdown cells because there is no
  way to know the cell is being re-rendered and the old javascript continues
  to run.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2363#issuecomment-8879875.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

[Carreau]

Le 26 sept. 2012 à 21:15, Brian E. Granger a écrit :

We are likely going to disallow JavaScript in markdown cells, so I would
move away from doing that. It turns out there are too many security issues
with it.

I don't think disabling is neither good solution, nor a solution at all.
The problem is not in the cells it is in the output area that render what is given to it.
disallowing Javascript won't prevent forged notebook with script in output when loading.

The only reliable way would be :
1 - To strip js from all output ( markdown and codecell)
2 - To sandbox each cell with html5-iframe.

There is no way we wish to go back to 1. or display Js is useless.

With 2 js would be restricted in it's div with no way of leaking outside. The timeout would be killed on MD regeneration.
when using display_javascript, we could allow a specific flag, explicitly set that allow the iframe to access parent namespace.

I don't know if it is possible, but we could even listen on wether the iframe is trying to access parent window, and make it red with a
warning :
"This iframe is trying to access global name space :

allow it ? [yes], [No], [What are the risks]."

Matthias

On Tue, Sep 25, 2012 at 11:07 PM, Cameron Bates notifications@github.comwrote:

Good to know. I was just exploring this as an alternative to putting html

• javascript in a markdown cell because I am having issues when I
  accidentally render the markdown cell multiple times. I am running into
  this when adding javascript timers to markdown cells because there is no
  way to know the cell is being re-rendered and the old javascript continues
  to run.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2363#issuecomment-8879875.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com
—
Reply to this email directly or view it on GitHub.

[ellisonbg]

Matthias,

There are multiple layers of vulnerability here:

a) A user loading a notebook (but not running anything) and at load
malicious JavaScript code running.
b) A user running Python code that sends malicious HTML/JS output that is
then run on the page.

As far as (a) is concerned, it should never ever happen. The problem with
(a) is that this vulnerability happens at page load literally before the
user even get's to see the code that is about to be run. As far as I know,
the only way that (a) can happen is with <script> tags in markdown cells.
If we clean <script> tags from markdown cells we have solved this problem.

Solving (b) is both easier and more difficult to solve:

It is easier because <B) can only happen as a result of a direct user
action and that action is precisely the user's request to run arbitrary
code. The bottom line is that when you run code, in the notebook or
elsewhere, you better trust it 100% or you shouldn't run it. Even if we
completely disabled dynamic JS in notebooks this will always be the case =
"don't run notebooks you don't trust 100%".

But, the fact the Js code can be dynamically generated and run in the
browser does introduce additional vulnerabilties. The main one is that Js
code gets run in an authenticated context - the malicious Js code can not
only mess up the server it can also do things on the net on behalf of the
user.

The only reliable way would be :
1 - To strip js from all output ( markdown and codecell)
2 - To sandbox each cell with html5-iframe.

There is no way we wish to go back to 1. or display Js is useless.

Yep.

With 2 js would be restricted in it's div with no way of leaking outside.
The timeout would be killed on MD regeneration.
when using display_javascript, we could allow a specific flag, explicitly
set that allow the iframe to access parent namespace.

I don't know enough about the execution context of iframes yet. Could the
iframe's Javascript still access the global IPython Js objects, such as
Notebook and Kernel? In order to support JS code that makes calls back
into the kernel, it would need to. My feeling is that if the sandboxing is
solid enough to prevent these vulnerabilities, it will also prevent the
iframe from accessing these things (that we want it to access).

Here is my current thinking on the issue.

• We either allow dynamic JS or we don't. If we do, we need to accept the
  vulnerabilities and mitigate them through other means.
• These vulnerabilities are primarily relevant in multiuser authenticated
  contexts. In those contexts I am thinking we could introduce the notion of
  "trusted" notebooks and could build that into any web applications.
  Basically, in order for code to be trusted, it has to be "reviewed and
  approved" by others. We could then build the web app to completely
  disallow JS/HTML output when running non-trusted code. Basically, we would
  be developing a way that forces people to be careful about the notebooks
  they are running and require explicit approval to have JS/HTML output.
  There are a number of different ways we could manage this approval
  process. I view this as a social rather than technical approach to the
  issue.

But I think we need to investigate exactly what the limitations of iframes
are to see what level of features/security they provide.

But, being security, I also worry that I am drastically misunderstanding
everything.

I don't know if it is possible, but we could even listen on wether the

iframe is trying to access parent window, and make it red with a
warning :
"This iframe is trying to access global name space :

allow it ? [yes], [No], [What are the risks]."

Matthias

On Tue, Sep 25, 2012 at 11:07 PM, Cameron Bates <
notifications@github.com>wrote:

Good to know. I was just exploring this as an alternative to putting
html

• javascript in a markdown cell because I am having issues when I
  accidentally render the markdown cell multiple times. I am running
  into
  this when adding javascript timers to markdown cells because there is
  no
  way to know the cell is being re-rendered and the old javascript
  continues
  to run.

—
Reply to this email directly or view it on GitHub<
https://github.com/ipython/ipython/pull/2363#issuecomment-8879875>.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2363#issuecomment-8902143.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

[Carreau]

From my phone, so sorry if short.

I will look at the code again, but i think you missrepresent the difference
between md and code cell. Both have input and output, difference is with md
cell you cannot see in and out at the same time.
Second thing, you mix display_all_but_js and display_js

Display_js does indead eval js which is not reexecuted at load time vs
display_* that does put it in output, and is then displayed as is at load
time.
execution pathways are irrelevent here, because someone malicious will
generate a notebook by hand, not respecting what current cell do.

Whatever cell you wish to target you just have to create a script in its
output. Maybe some implementation subtilities, but the principle is the
same.

As for html5 iframe. By default they disallow js. One can pass a flag to
selectively allow js; allow js to acces top frame namespace,allow explicite
cross origin policies. Note that as it applies to output frame, it
does not prevent binding codemirror event.

Though, i don't know wether you can selectively give acces to only some js
function. But selectively enable js on a per output basis is imho a huge
improvement of security which should be considered for notebook and
nbviewer.
Le 26 sept. 2012 22:31, "Brian E. Granger" notifications@github.com a
écrit :

[ellisonbg]

Matthias,

You raise very good points, replies inline...

On Thu, Sep 27, 2012 at 12:46 AM, Bussonnier Matthias <
notifications@github.com> wrote:

From my phone, so sorry if short.

I will look at the code again, but i think you missrepresent the
difference
between md and code cell. Both have input and output, difference is with
md
cell you cannot see in and out at the same time.
Second thing, you mix display_all_but_js and display_js

Display_js does indead eval js which is not reexecuted at load time vs
display_* that does put it in output, and is then displayed as is at load
time.
execution pathways are irrelevent here, because someone malicious will
generate a notebook by hand, not respecting what current cell do.

Whatever cell you wish to target you just have to create a script in its
output. Maybe some implementation subtilities, but the principle is the
same.

Yep, I had missed this vulnerability. Because we just insert output into
the DOM in a rather simple manner, any script tags in the output are run,
even if they are not in the HTML/JS output. And that does happen at load
time :(

As for html5 iframe. By default they disallow js. One can pass a flag to
selectively allow js; allow js to acces top frame namespace,allow
explicite
cross origin policies. Note that as it applies to output frame, it
does not prevent binding codemirror event.

OK I will try to read up more on this.

Though, i don't know wether you can selectively give acces to only some js
function. But selectively enable js on a per output basis is imho a huge
improvement of security which should be considered for notebook and
nbviewer.
Le 26 sept. 2012 22:31, "Brian E. Granger" notifications@github.com a
écrit :

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2363#issuecomment-8928490.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

[Carreau]

@ellisonbg , back on original PR.
Do you think it's ready for merge ? are you ready to move to jinja now ?
I think i've a few PR that will conflict with this one, but i'm ok to rebase them.

After i'll like to start introducing bootrap one step at a time, so the sooner everything nbrelated is merged, the better.

[ellisonbg]

There should only be one env instance that is created by the application object and used by the different handlers.

[Carreau]

Pinging the status of this one.

the 2 point to fix are :
env should be created only in one place.

static_url macro discussion, which is fine for me if the ?v= attribute is not added. Practical reason is that we already have an HTML header which state wether or not the fiel as changed.

[Carreau]

This does not merge cleanly anymore.
Sorry about that.

[crbates]

Sorry, been really busy since Sept. I'll pull from head and look into what needs to be done sometime this week.

[ellisonbg]

Thanks that would be great, I don't think it should be too much work.

[Carreau]

This one has been rebase and merge cleanly again ! Yay !

env is now a property.
static_url is not as before, but this is not necessary IMHO.

+1 for merging,

Reserve on env as a property (is it slow to recompute each time, would it make sens to make it a trait ?)
but this could be done as separate PR.

[ellisonbg]

OK, I will do a final review and then we can merge this.

[ellisonbg]

But wait, unless I am misunderstanding something, I think this property has the same problems as before:

1. Each handler instance will have its own Environment.
2. That environment is recreated everytime the property is accessed.

I think the environment creation logic needs to be moved to the Notebook Application object, and the handlers would then access it as self.application.jinja2_env. Does this make sense? I want to make sure that one and only one of these environments is created per notebook process.

[crbates]

@ellisonbg yeah I didn't have that good of an understanding of how the whole tornado notebook web application + handlers worked, I think the new commit is the solution you were looking for.

[cldeploy]

In the various handlers when you grab the template, you call the variable nb as in "notebook", but the templates are most often for non-notebook pages. Can you rename these local variables to t or temp or template to not confuse future devs.

[cldeploy]

@crbates Yes, this is it exactly. There is just one more change (super minor) that I just commented on and then we can merge. Great work!

[Carreau]

Perfect last comment has been fixed, I'm going to merge this !
Thanks !

[cpcloud]

Looks like login.html needs to be updated as well. I'm getting an error about closing with end instead of endblock. Mysteriously, it works fine on a different computer with the same version of ipython. The only difference is that in one version I use an encrypted version of the notebook and in the other it's not encrypted.

[ellisonbg]

@Carreau, do you think you can fix this one?

On Mon, Dec 17, 2012 at 12:18 PM, Phillip Cloud notifications@github.comwrote:

Looks like login.html needs to be updated as well. I'm getting an error
about closing with end instead of endblock.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2363#issuecomment-11459379.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

[cpcloud]

So I've narrowed it down to something with the encryption. I can use the notebook over an unencrypted connection but not over an encrypted connection. Don't have time right now to fully debug.

[crbates]

Yeah just fixed and tested password protected/encrypted notebooks. see #2699