Discussion: Add view layer

[chrismccord]

Template engine wise, EEx has no great means for xss protection, but we don't have many other options yet. I'm hoping to include @nurugger07's haml implementation once it's ready (https://github.com/nurugger07/calliope). If we can get xss protection in EEx, I'd be perfectly happy with that as the default template engine, but José has said as implemented today with just string concat that it can't easily be done.

[nurugger07]

calliope is coming along nicely. I can actually parse basic haml structures now and looking to have scripting done this week (fingers crossed).

[chrismccord]

Exciting! 95% of the markup I write is haml so I can't wait for this.

[nurugger07]

It's making our designers really happy too :)

[nurugger07]

Thought you might like to see these:

https://github.com/nurugger07/phoenix/blob/render_haml_with_calliope/test/phoenix/controller_test.exs

https://github.com/nurugger07/phoenix/blob/render_haml_with_calliope/lib/phoenix/controller/controller.ex#L28-30

Calliope is almost there so I wanted to see how hard it would be to add in. Feedback is appreciated :)

[chrismccord]

Oh this is awesome! I can't wait to try this out. Any thoughts to how to handle runtime rendering? Are you planning on rendering flat .haml files or can we work in some kind of precompile mechanism that compiles the haml into the application for later rendering?

[nurugger07]

I just got done testing in an actual browser and everything renders perfectly. I wanted to get a first pass at an integration to see what works best. As you can see that was actually pretty trivial.

Calliope's render takes a string of haml and the arguments needed to compile the flat html. It seems like passing the file at runtime would be the best option. Plus, having calliope expect anything other than a haml string and arguments, seems too coupled to an implementation. In this case, Phoenix can decide the when, how, & from where the haml comes from and then pass it along to be rendered. My thought is the haml function would use __MODULE__ to determine a folder and the atom passed in would be a file. If a string is passed, the function would assume a string of haml and just pass it on(?).

[darkofabijan]

@nurugger07 nice! Regarding XSS, colliope is making this easier to handle than EEx at the moment? I am kind of lost on XSS atm :)

Having haml/eex compiled could give serious speed improvements, of course in that case haml file discovery could not be done at runtime. In that case we would probably need whole new layer that compiles haml/eex, and eex, haml functions would need to grab it from some template index.

[josevalim]

I would recommend to take a look at Dynamo / Chicago Boss. For development, we compile the templates into a fake module (Dynamo.Templates1) and dispatch to and reload those modules at will. For production, we compile everything into Dynamo.CompiledTemplates and simply have an entry point in the module.

Any solution that is not based on compilation will be very slow, given that eval in Elixir is slow.

[nurugger07]

@josevalim Thanks, I'll take a look at how they are handling this. My first goal to get something working and see it deliver html to the browser correctly :) If you get a chance to look at the calliope code I'd appreciate some thoughts/feedback. You can email me directly or through that repo.

[slogsdon]

Not sure if this would be desired or not, but I've been working on a general helper library for template engines. It currently supports ErlyDTL (used by ChicagoBoss) and HAML via @nurugger07's Calliope with plans to add in EEx soon. Allowing multiple types of templates could aid in adoption of Phoenix, where developers could bring over existing templates without having to start from scratch.

End goal of the project is to allow web applications to use any supported templates, using pre-compilation to help speed up the rendering process.

It's very much a work in progress, with quite a bit of work left including configuration and a defined API, but both ErlyDTL (compiled on first use) and Calliope (no compilation) engines return HTML when called directly. Take a look at the project at slogsdon/templates, and let me know what you think.

[chrismccord]

Awesome! I'll take a look. One thing we need to be careful of is proper xss protection, and pre-compilation with whatever template engine is supported. EEx as implemented cannot properly protect against xss, but there are discussions currently on how we can get it in place. We also can't rely on Code.eval_string in production for reliable performance. https://github.com/zambal/eml is also worth a look. I agree a general helper library would be extremely nice that we could leverage across different engines.

[slogsdon]

@chrismccord Agreed re: xss and evaling. I'll take a look into eml. It looks like a nice option.

[josevalim]

The templates project is a good initiative! I believe that whatever approach we choose needs to support explicit pre-compilation though (and not on demand like ErlyDTL) because when you build a release all the code is loaded upfront (the virtual machine does not load code dynamically).

A couple feedback regarding templates:

1. I would suggest to leave Templates.Finder out of it (it is almost always framework specific);
2. Have the source in the template and get rid of filename (templates should be able to come from the database, for example). Advise handler implementations to get rid of the source after compilation (and keep the source if they need it at runtime, like calliope);
3. Get compile/1 to return the beam source (gives you more flexibility to where to put the beam source later);
4. Make the key an atom (it is very likely that's how the majority of templates will use it);

[slogsdon]

@josevalim Thanks for the advice! Pre-compilation is definitely a goal, and the benefits of it are entirely understood. The current ErlyDTL implementation was a test run to make sure I was headed in the right direction.

[josevalim]

You definitely are. The template engine api you have is similar to the ones in chicago boss and dynamo.

[nurugger07]

Glad I saw this :) I was debating on adding a template engine to calliope. I'm trying to work on integrating calliope with phoenix to test performance.

[josevalim]

Ok, since many people working with web frameworks are in this thread, I would like to suggest a structure to avoid XSS attacks that would definitely work with EEx. It is very likely to work with Calliope too (please confirm).

The problem with EEx is that it interpolates strings. And, in a regular string, we don't know if the contents are safe or not. The idea here is to come up with a more structured way to represent HTML nodes in Elixir. Basically, a node can be represented as:

@type html_node :: {tag :: atom, attributes :: Keyword.t, [html_node]} | String.t

When the node is a tuple, it represents an html tag and the contents are expected to be html safe. I.e. they won't be escaped. Strings are considered to be raw and will always be escaped.

This is nice because creating tags can be done as:

{:a, [href: "/"], ["Home"]}

And the engine will translate that to the proper HTML representation.

What do you think?

So far I can think of only one (solvable) issue in the proposal above: there is no way to represent a HTML safe string without wrapping it in a tag and that may not always be desired. People may also want to concatenate two nodes (a tuple one and a string) without wrapping it in a tag. This means we need a third representation that can have many elements without generating a tag. We have two options:

1. Use the {tag, attributes, body} structure but have a special tag name for it. For example, the tag name could be :group or :safe. The issue with this approach is that the attributes will never be used and if HTML adds a :group tag, we are screwed. :)

2. We could use a {:safe, body } tag. It has a different structure, that reflects the lack of a tag. The updated node would be:

   @type html_node :: {tag :: atom, attributes :: Keyword.t, [html_node]} | {:safe, [html_node]} | String.t

The only downside of this approach is that code that traverses the tree now needs to traverse three different structures, but most of this work will likely be done by functions anyway.

Thoughts? /cc @ericmj @devinus

[nurugger07]

@josevalim calliope parses the haml/elixir into a structure like prior to being compiled:

[ line_number: 1, tag: "h1", content: "Calliope" ],
[ line_number: 2, tag: "div", children: [
  [ line_number: 3, indent: 1, script: "subtitle" ]]],
[ line_number: 4, smart_script: "lc {id, content} inlist posts do", children: [
  [ line_number: 5, indent: 1, tag: "div", classes: ["row"] children: [
    [ line_number: 6, indent: 2, tag: "a", attributes: "href='posts/'\#{id}'", content: "'\#{content}"]]]]

Finally, the nested structure is compiled into html and that is the point when the elixir is evaluated. Currently, strings are being interpolated but the "smart_scripts" are being constructed and run through string_to_quoted before being passed to eval_quoted. (Is there any advantage in this?)

I just added a validation function that could "scrub" scripts. It runs before the nesting and compilation. I'll look at implementing an html_safe function to run around interpolated content.

If anyone has thoughts or ideas for this, I'd greatly appreciate it.

[devinus]

Notes from a discussion I had with @josevalim:

An HTMLSafeEngine for EEx could simply escape by default but expect String.t | { :safe, [html_node] } for the vars. Helper functions would generate { :safe, [html_node] } which the engine could then traverse, build the HTML strings and insert unescaped.

[nurugger07]

I've been working this into calliope. I have a separate module to handle evaluating and escaping. By default, args passed to the template are made HTML safe prior to the code being evaluated so this:

render "%h1= title", [title: ""]

would render:

<script&rt;a bad script</script&rt;

However, you could prevent the default behavior by doing the following:

render "%h1= Safe.script(title)", [title: ""]

and that would render:

You can also use the Calliope.Safe.clean/1 from Elixir to scrub any string.

Here is a link to the branch if you want to check it out but keep in mind it's not complete: https://github.com/nurugger07/calliope/blob/html_safe/lib/calliope/safe.ex and tests: https://github.com/nurugger07/calliope/blob/html_safe/test/calliope/safe_test.exs

[josevalim]

@nurugger07 Nice! How do you know if a value was already escaped?

[nurugger07]

@josevalim for now, I don't and I assume values need to be escaped unless it has been designated as a safe script. This should suffice in most cases but eventually it will need to be smarter. The nice thing is it's abstracted into its own module so modifications should be isolated. I was debating making Safe a small library but still haven't decided.

I need to address rendering partials today because I know @knewter is compiling the haml and passing it in as an argument. Which should be fine if he wraps the args in Safe.script but really this is the job of a render_partial function.

[josevalim]

@nurugger07 from what I understand, Safe.script exists only in HAML, which means it is very hard to write helpers/functions that receive possibly safe strings and emit possibly safe or unsafe strings. This means writing helpers like link_to, my_menu_link become quite hard or you need to declare something as safe multiple times, no?

[nurugger07]

@josevalim for now, yes. Safe.script bypasses the argument checks. There is a Safe.clean function that works outside of HAML to "clean" strings/lists. I tried to implement a solution that would work for now but I could refactor later with new features.

What do you think of these helpers?

Arguments are cleaned:

= link_to "#{subject}", "/posts/#{id}"

Arguments are not cleaned:

= safe_link_to "#{subject}", "/posts/#{id}"
= my_custom_link_to(subject, id)

def my_custom_link_to(text, id) do
  "<a href='/posts/#{Safe.clean(id)}'>#{Safe.clean(text)}</a>"
end

I'm working on a project that I'll be demoing at Erlang Factory that provides a "real-world" application to test out the implementations. That will help drive a better long term solution. I'm open to any ideas or contributions :)

Regarding the pre-compiling, I was thinking about working on the template engine to index and compile the scripts to increase performance. I would create a function at compile time that takes the arg as a parameter and returns the result. I would just replace the script with a generated name. Also, appreciate thoughts on this :)

[josevalim]

What do you think of these helpers?

Sorry, I couldn't see the difference in between the arguments being cleaned and not being cleaned. Do you mean link_to is somehow special cased and my_custom_link_to is not?

[nurugger07]

Sorry I meant to say = safe_link_to. Copy & paste error :/

[nurugger07]

My thought was that there would be built in helpers that would safely escape args but then there would be "safe" helpers that the args wouldn't be escaped. Custom helpers would also assume that devs are escaping the html.

That being said, I could be totally wrong in that line of thinking :)

[josevalim]

Custom helpers would also assume that devs are escaping the html.

That's a very dangerous assumption to make even more if you consider that helpers are just functions. If you have more strict rules about creating and adding helpers, maybe that would work, but it is still is the opposite of what you'd want to do.

I would rather assume that nothing is safe and ask users to explicitly mark what is safe. But even an approach like that is tricky. If you need to explicitly mark even basic functions like link_to as safe, people would just end up marking all functions as safe, because if they don't, their code would appear escaped.

That's why I was thinking of a more explicit tagging approach. We don't assume anything is safe, we rather ask the function to return something that is safe or not. And link_to could return something safe by default, which means it works pretty much out of the box.

The nice thing about using explicit tags is that they could be used by any engine. The API is actually rather simple:

defmodule HTMLSafe do
  @type t :: safe | unsafe
  @type safe :: {:safe, String.t}
  @type unsafe :: String.t

  @spec safe(t) :: safe
  def safe({:safe, _} = safe), do: safe
  def safe(string) when is_binary(string), do: {:safe, escape(string)}

  @spec concat(unsafe, unsafe) :: unsafe
  @spec concat(t, t) :: safe
  def concat({:safe, s1}, {:safe, s2}), do: {:safe, s1 <> s2}
  def concat(s1, s2) when is_binary(s1) and is_binary(s2), do: s1 <> s2
  def concat(s1, s2), do: concat(escape(s1), escape(s2))
end

I have originally proposed representing the HTML nodes as tree structures as well which would lead to a slightly more complex implementation but it comes with the nice side-effect that you would actually be able to traverse HTML code as data.

[josevalim]

Regarding the pre-compiling, I was thinking about working on the template engine to index and compile the scripts to increase performance.

For precompilation, I believe you should just emit quoted expressions. Take a look at the EEx main functions (function from file, function from string and so on). For example, in EEx, when you write:

foo
<%= :bar %>
baz

It literally compiles to (represented as quoted expressions):

"foo\n" <> to_string(:bar) <> "\nbaz\n"

This way, you can just inject this code into an elixir function and elixir does the job of optimizing what it can out of it.

[devinus]

:+1: but I would make concat make IO lists.

[josevalim]

@devinus What is the benefit we would get out of it? If we start inserting io lists in the user code (which concat/2 is definitely part of), it means our types now become:

@type t :: String.t | iolist

And that is just asking for massive headaches. Now you can't even extract something out of it and call downcase on it.

If we represent HTML tags as nodes, then the underlying template engine could convert them to iolists instead of binaries, but that should only be done internally. But even though we need to do benchmarks to see if there is some real benefit. I definitely do not think it should be part of the user concern.

So the questions are:

1. Do we want to represent HTML nodes as tuples? For example, link_to/2 would return {:a, [href: target], [text]} instead of "<a href=#{inspect target}>#{text}</a>";
2. If so, can template engines like Calliope and EEx get a nice performance out of it by compiling them to iolists?

[BjRo]

@chrismccord @josevalim I took a stab at ripping out whats currently view wise in dynamo and adapt it to phoenix. It's basically a copy/paste of the current dynamo view layer with adjustments from dynamo/connection to plug/connection functionality.

You can take a look at it here: https://github.com/BjRo/phoenix/compare/dynamo_view_layer

Please note that there're right now no tests for that. I just wanted to see how far I can get, porting a dynamo side project over to phoenix. I'm able to render a pretty complex view with lots of partials with that code which is nice so far.

To give you a bit background on the why. My current employer is so nice to give us time to work on innovation projects every 8 weeks. The last time around I and 2 colleagues wrote a small stackoverflow clone with (guess what :-)) dynamo and I'm currently trying to port over what we have to phoenix.

Judging from the discussion so far, it's probably not 100% what you would like to have for phoenix, but it enables my project to move our code over to phoenix for the moment (until there's a full blown solution in phoenix for that).

Thoughts?

[patrickdet]

@BjRo wouldn't it be best to extract those view modules into a project on its own which can be used by Dynamo and Phoenix? Or are they too tightly coupled to other framework modules?

[nurugger07]

The view engine I'm working on for calliope is easy to add to any library and should work with Phoenix or Dynamo with very little effort.

[BjRo]

@patrickdet Yes, probably. To be honest though, it was an intermediate step for us that allows us to switch to phoenix. In the long run, we definitely want to switch to @nurugger07's calliope for our views. That's why I'm a bit shy to invest a lot of time there :-)

[josevalim]

@nurugger07 did you finally settle on any internal implementation for safe html chunks in calliope?

[nurugger07]

@josevalim yes and no. So there were a couple of issues I was trying to address and figured the best way was to break it up. The first thing I did was to change the output of to be an eex formatted string for compile time and then I utilize EEx if you want to compile the template at runtime.

require EEx

def render(haml, args \\ []) do
  precompile(haml) |> eval(args)
end

def precompile(haml) do
  tokenize(haml) |> parse |> compile
end

defp eval(html, []), do: html
defp eval(html, args), do: EEx.eval_string(html, args)

Now I'm focusing on a view engine to read the haml templates in and compile them into named functions. I lose XSS support because of EEx but I'm going to circle back around and address it. I may look at a solution for EEx if that's ok and then leverage it in calliope.

I punted mainly because I want calliope to output an Elixir AST but I know people are waiting on compiled templates. I figure putting in a public api that allows me to abstract the internals will make it easier to transition later. Plus, I can depend on everyone having EEx :)

[josevalim]

Ok, if I understood it correct: you are currently outputting an EEx string because you can compile it but in the future you want to output Elixir AST directly, which then allows you to skip EEx altogether, right?

[nurugger07]

That's the plan today but I'm not entirely sure it's necessary. If I can work XSS support into EEx is there much benefit in skipping it?

[josevalim]

@nurugger07 if you are going to generate Elixir AST from calliope then there is no need to use EEx (unless you are using EEx to generate the Elixir AST). The compilation functions should be easy to port to Calliope once you have a function that receives a Calliope template and returns Elixir AST.

[knewter]

So haml (at least originally) output erb files into an erb binding, which led to a lot of occasional odd error messages. That might be one reason to avoid taking roughly the same path? Compare this to slim, which I believe actually implements its own bottom layer for the templating rather than outputting erb templates.

On Wed, Mar 26, 2014 at 1:46 PM, Johnny Winn notifications@github.comwrote:

That's the plan today but I'm not entirely sure it's necessary. If I can work XSS support into EEx is there much benefit in skipping it?

Reply to this email directly or view it on GitHubhttps://github.com/phoenixframework/phoenix/issues/11#issuecomment-38723746 .

Josh Adams CTO | isotope|eleven http://www.isotope11.com cell 215-3957 work 476-8671 x201

[knewter]

I just wanted to say - I started looking into building a haml parser yesterday with yecc, and ended up deciding that using a LALR(1) parser for haml is possibly one of the dumber ideas I've had :) Still holding out hope for a peg parser based on sean cribbs' neotoma.

On Wed, Mar 26, 2014 at 1:48 PM, José Valim notifications@github.comwrote:

@nurugger07 https://github.com/nurugger07 if you are going to generate Elixir AST from calliope then there is no need to use EEx (unless you are using EEx to generate the Elixir AST). The compilation functions should be easy to port to Calliope once you have a function that receives a Calliope template and returns Elixir AST.

Reply to this email directly or view it on GitHubhttps://github.com/phoenixframework/phoenix/issues/11#issuecomment-38723917 .

Josh Adams CTO | isotope|eleven http://www.isotope11.com cell 215-3957 work 476-8671 x201

[nurugger07]

The EEx solution is temporary and abstracted so it should be easy to remove the dependency without affecting consumers. I really just wanted a chance to get the templates compiled and kept going back and forth on how best to implement one. Using EEx is a short-term solution.

[josevalim]

@nurugger07 awesome! :)

[nurugger07]

@josevalim I find that sometimes when I punt on things and circle back around, I have a clear view :) Unfortunately, sometimes that's not an option.

[chrismccord]

I'm experimenting with a precompiled/xss-safe Templating engine in cm-templates branch: https://github.com/phoenixframework/phoenix/tree/cm-templates

Big thanks to @ericmj for the EEx Html-safe engine.

What this branch gives you:

defmodule MyApp.Templates do
  use Phoenix.Template.Compiler, path: Path.join([__DIR__, "templates"])

  # This mod contains all templates and serves as base for view layer,i.e.:
  def number_to_currency(number), do: ...
end

# Render from outside Controller, file : templates/show.html.eex
iex> MyApp.Templates.render("show.html", message: "Hello!")
"<h1>Hello!</h1>"

# Render from within Controller, file: templates/pages/home.html.eex, 
# file extension picked based on request mime content-type
def show(conn) do
  render conn, "pages/home", title: "Home"
end

# templates/layouts/application.html.eex automatically rendered with
# template as inner content, i.e.:
# <html>
#  <title><%= @title %></title>
#  <body>
#    <%= @inner %>
#  </body>
# </html>
# override with layout: false, or layout: "my_other_layout"
def show(conn) do
  render conn, "pages/home", layout: false, title: "Home"
end

I'd like to see how we can take this and grow it to support other engines than EEx, i.e. @nurugger07 's Calliope.

Thoughts?

[josevalim]

Nice!

I really like how templates are compiled and clearly injected into the module. And I do have a couple questions:

1. Should we start worrying about how to compartmentalize the templates? The current implementation will embed all templates recursively but we probably want the users templates in a module, the posts templates in a module and so on. I am aware this is possible with the current implementation but what about subdirectories?

2. Is there a need to define functions (like render) in the template module? If not, I would avoid and instead do: Phoenix.Template.render(MyTemplate, ..., ...). I usually define functions in a module are if they are one of:

   1. When those functions are necessary for some behaviour (like a Plug needs to implement init & call, GenServer.Behaviour, etc)
   2. When we have a facade module (like MyRepo in Ecto.Repo)
   3. The functions exported are all metadata related (like __templates__)

   The main reasoning is that injected functions are hard to document, test, etc.

3. Maybe we should make a distinction in between views and templates? View is the module where the template is rendered. The template is the file with the contents (usually EEx or Calliope). I am saying this because Rails calls views and templates the same thing, which is very confusing, and we end up needing to create names like view context and so on.

I have a couple notes regarding the implementation:

1. There is a lot of work being done inside the functions defined via quote. It is always better for them to be simply a delegation to actual implementations
2. It seems a controller can call a template and a template can call a controller functions. I would try to have the flow (the dependency) just one way (the controller calls the template and that is it. Maybe you will have to move common functions to the template).

[chrismccord]

Thanks for the feedback @josevalim,

1) This is something worth exploring. For large apps, we'll otherwise end up with, render(conn, "admin/institutions/edit"...). A glance at the largest web apps I've worked on shows rarely more than 3 levels of view directories, but the controllers using the nesting might get tiresome. This might be heresy , but what if we used camel-cased template directories for the times we wanted a "submodule" and lowercase directories for times we wanted to simply nest the templates. Ie:

├── Admin
│   ├── institutions
│   │   ├── _edit.html.eex
│   │   └── new.html.eex
├── Profiles
│   └── new.html.eex

iex> MyApp.Views.Admin."institutions/edit.html"([])
iex> MyApp.Views.Profiles."new.html"([])

2) I wasn't thrilled with render being inside the template, but couldn't think of a better way at the time to get the nice render/3 in the controller. I very much like Phoenix.Template.render(MyTemplate, ..., ...), and with the proposed namespacing of templates, it would let us:

defmodule MyApp.Controllers.Admin.Users do
  use Phoenix.Controller

  @views MyApp.Views.Admin

  # render/3 imported from Phoenix.Views/Templates
  def show(conn) do
    render conn, @views, "users/show"
  end
end

3) I very much like the distinction of views/templates and I'm already having a hard time keeping the naming straight, likely because of Rails :). I think the base module that gets all the injected templates should be a "Views" module and will update the naming.

I wasn't thrilled with having to inject render inside the template within a quote, but I couldn't figure out how to best get the controller rendering in place. #2 proposal will solve that by simply passing the view module in to an outside render func. This will also clean up the requirement of having to call the controller code from within the template. Thanks @josevalim , this has been really helpful feedback!

[josevalim]

This might be heresy , but what if we used camel-cased template directories for the times we wanted a "submodule" and lowercase directories for times we wanted to simply nest the templates.

I really like this! I like it because it makes it clear in the directory structure how it will be organized. It can also help us automatically get the template path without explicit configuration. One potential pain point though: what about shared templates?

MyApp.Views.Admin."institutions/edit.html"([])

I wouldn't invoke it like this though for a couple reasons:

1. It means we will need to convert the template paths to atoms when rendering (and it could be lead to vulnerabilities due to atom injection)
2. Maybe we need to know other information about the template. For example, ask if the template exists before rendering it or its format (although the format can be extracted from its name).

Just to be clear, it is fine if we keep it as render(name, assigns) as long as it is not coupled to the connection and this is specifically marked as the template API. If we do this though, we will need another function to return the template metadata (maybe __template__(path)).

:+1: I really like the direction this is going (and it is one of the things I didn't like and was too hacky in Dynamo).

[alco]

A slight off-topic: in JS community there's this new thing being developed called HTMLBars. It uses Handlebars' template language, but compiles into a DOM tree instead of a string.

@josevalim has proposed doing something similar with EEx, so I'm posting this just as something to keep an eye on to see how it turns out in terms of usefulness and community response.