Proposal: Safe Mode

[ghost]

I am creating this ticket in response to a discussion here: https://github.com/mustache/spec/issues/54#issuecomment-9006338, also related is @pvande's comment here: https://github.com/mustache/spec/issues/41#issuecomment-7407203

There are a number of scenarios where it would be useful to allow end users to edit Mustache templates. For example, a Shopify type application, a CMS, or to allow users to insert credentials into a templated email (think Campaign Monitor). I'm sure there are many more use cases.

There is a problem with this in how Mustache currently operates as the template author can access properties/methods of a given implementation's objects such as {{thing.private_instance_methods}} (Ruby example) etc. Although you could remove methods that you don't want users to call (kind of like Ruby's BlankSlate object), users will always be able to get to the underlying language methods on strings such as inspect etc.

I envisage a "safe mode" much like how Liquid templates and maybe Liquid Drops work i.e. only the keys and values that are explicitly exposed in the view object are allowed. Therefore, something like {{title.downcase}} or {{collection.inspect}} (Ruby example again) should just return empty strings unless they are explicitly defined.

I'm not entirely sure of the implementation details as yet but wanted to throw this out there for discussion. I for one would love to use Mustache for these types of 'end user' applications and I have seen the question asked a number of times, as to whether Mustache is 'safe' for end user use.

What do you think?

[groue]

Safe mode is interesting, as it would allow Mustache to safely render untrusted templates, as you say.

It actually covers two kinds of attacks: the attack through invocation of unintentionally exposed methods, and the attack through code injection (as the loophole in the Ruby implementation that has been noticed by @pvande in his comment on mustache/spec#41).

I'll only talk below about the first kind of attack, and explain why preventing invocation of unintentionally exposed methods should remain optional for both Mustache implementors and Mustache users.

1. Safe mode should remain optional for the Mustache user, because there is no reason preventing him to throw easy but dangerous objects into the template, if he does not care about safe rendering.

   Rationale:

   A Mustache implementation could require the user to send him four kinds of objects, and only them: strings, boolean, arrays, and hashes. Strings would render, booleans and arrays would control sections, and hashes would provide keys. For an example, see the C++ implementation mrtazz/plustache.

   However a Mustache implementor may want to use his language features or conventions, so that his user is not limited to those core types. For example, the Ruby implementation defunkt/mustache lets the user render any kind of object, and converts any key found in a template tag to a method call, which is trivial in Ruby. The Objective-C implementation groue/GRMustache lets the user render any kind of object, and converts any key found in a template tag to the invocation of the conventional valueForKey: method. Both those implementations are not safe at all, since their value extraction technique exposes to the templates all methods and properties shared by all Ruby and Objective-C objects, and there are many (Ruby, Objective-C).

   Could Ruby and Objective-C implementations keep using their language features and become safe at the same time? Would they internally keep a black list of forbidden keys, this would still not prevent unintended user-defined methods to be exposed in the template. The only solution for these languages is requiring the user to explicitly white list the template-exposed keys, with some kind of specific APIs.

   Requiring safe mode would thus complexify the "view-model preparation" phase, which arguably is a downside of Mustache that does not need any more fostering. The Mustache user who does not need template safety should not be forced to wrap his unsafe objects into safe containers.

2. Safe mode should remain optional for the Mustache implementors, because it's unnecessarily bold to deny the "Mustache" label to an implementation that does not provide it. We need a living Mustache ecosystem, where implementations compare on speed, expressiveness, features, integration with other tools.

[groue]

About code injection attack:

The Ruby loophole can be seen as a bug in the Ruby implementation. Unfortunately, this very Mustache spec enforces the shape of Ruby Mustache API by embedding actual lambdas definition right into its lambda tests (see https://github.com/mustache/spec/blob/master/specs/%7Elambdas.yml#L87).

As a consequence, the spec today specifies this loophole, and prevents a more robust Ruby implementation to pass the spec tests. Oh the irony!

This Mustache spec should stop right away to specify the shape of Mustache APIs, if it wants to let implementors support "safe mode".

[groue]

GRMustache 6 lets you make sure some keys always return the same value (contexts opened by Mustache sections can not shadow those keys) : https://github.com/groue/GRMustache/blob/master/Guides/protected_contexts.md

[groue]

Protected objects are more complex than I initially thought. Key shadowing is always there, waiting to bite. GRMustache has long chosen the Handlebars way of exposing as much as possible of the rendering APIs to the end-user - and the problem with protected objects is that you don't want the end user to mess with it. The result is "you must use full paths to your deep protected objects, or they won't be found." : https://github.com/groue/GRMustache/blob/master/Guides/protected_contexts.md#protected-namespaces.

[ghost]

Hi,

Not ignoring responses, just away at the moment and not enough time to respond. Really keen to explore this topic some more as can see a number of uses for "safe mode". I am back in a week and will respond properly then.

Thanks,

Jamie

[groue]

Hi Jamie @thelucid ! Don't worry, and have a nice week :-)

[ghost]

Hi @groue,

Sorry for the delay.

I completely agree with your two point, "safe" templates should be optional and should be optional for implementors.

I guess the only true way to implement safe templates is to have some kind of whitelist functionality. Liquid solves this with Liquid Drops (http://liquid.rubyforge.org/classes/Liquid/Drop.html) that basically decorate an object, only allowing explicitly defined methods to be exposed. This wouldn't work in something like JavaScript as you would still have access to all other Object methods in a view (I guess you do in Ruby too though). Liquid also looks for a to_liquid method on an object and uses the returned hash if it exists.

I'm not sure of the best way to implement the whitelist functionality, perhaps something as simple as an array of allowed keys would suffice (kind of like attr_accessible) in Rails, or even an API similar to https://github.com/rails/strong_parameters

# attr_accessible style solution
class Person < Mustache
  permit :name, :age

  def name
    "Jamie"
  end

  def age
    33
  end
end

...again the JavaScript implementation might be a bit tricky, maybe something like:

var view = {
  $permit: ['name', 'age'],
  name: "Jamie", age: 33
}

This still wouldn't get around the problem whereby any method can still be called on a string however this could be worked around with something like:

class SafeString < String
  include Mustache::Permit
  permit :to_s # or maybe "permit nil"
end

What are your thoughts on this. I guess @defunkt would be the man to ask when it comes to the Ruby API.

Cheers,

Jamie

[ghost]

An example of introducing a memory leak in the current Ruby implementation:

{{thing.to_sym}}

This is why strings would need to be locked down i.e. strings should not allow method calls. If you wanted an uppercase string in "safe mode", you would just have to supply it in the view i.e. {{uppercase_thing}}

[ghost]

I'm currently visiting the idea of a separate Ruby Mustache implementation that requires the Mustache::Parser class but implements a new Generator with no eval.

I'm calling the project "Tache" for now i.e. "Just enough Mustache for end users". Will let you know how it goes.

[groue]

Sorry I did not answer your message last month, Jamie.

Today, the white-list aspect of the "safe mode" looks to me as a matter of implementation only. Maybe something could enter the spec, but I don't know what yet. I'm curious actually to see what you're about to show that could enter the spec.

The protected contexts of GRMustache focus on another side of safety: the rendering of untrusted templates and data. There is something that the spec could take, here (namely, the mandatory dot for accessing deep protected data: {{ safe.name }}).

[ghost]

I'm talking more about an implementation that is usable by end users in a CMS or Shopify style application. I'm looking at how to rewrite the Ruby implementation to use no eval's and therefore easily restrict the properties that are allowed in the template. I'm using Liquid as a guide.

[ghost]

Hello,

I've finally found some time (thanks to the xmas break) to have a crack at a safe Mustache implementation that allows for templates to be edited by end users without fear of jeopardising an app's security.

In order to gain a full understanding of how Mustache works, I decided to first write a full Mustache implementation from the ground up. Once this was done, I looked at ways to implement 'safe views' using Liquid as inspiration. My implementation passes the current spec, apart from a few whitespace differences and one spec to do with tag switching.

I have introduced few new concepts in order to achieve 'safe views':

• A 'safe' view class.
• A 'safe' context class.
• A 'drop' class (concept borrowed from Liquid) for exposing object properties to a template.
• A to_tache instance method that must be defined on all objects exposed to a template.

All core types such as String, Array, Hash, Number etc. get a to_tache instance method that returns self. As any other objects that are exposed to a template are either 'safe' view objects or 'drop' objects, the developer doesn't need to be concerned or even be aware of the to_tache method.

The general workflow from a developer perspective when it comes to 'safe views' is as simple as:

• Subclassing a 'safe' view class (in this case Tache::Safe).
• Subclassing a 'drop' class (in this case Tache::Drop) where required.

Section lambdas and lambdas in general work as expected thanks to the to_tache method on core types.

Given what I have learned with this implementation, I believe I could patch Mustache itself with these changes although the main problem with the current Ruby implementation when it comes to 'safe views' is that is promotes symbols as view keys which can cause memory leaks in an end user environment. Tache forces the developer to use string keys to get around this problem, however we could just impose this restriction only on 'safe' views to get around this in Mustache itself.

There are a couple of things on my todo list including true compilation, but for now my focus is less on speed and more on a clean codebase to be able to try out new ideas. Dynamic partials will be my next endeavour.

Here's the repo/readme: https://github.com/thelucid/tache ...the code is pretty self explanatory so I encourage taking a look at how it is implemented

Many thanks for your help on this... Happy New Year!

Jamie

[groue]

Nice job, @thelucid! Happy new year :-) I don't exactly get how you render a graph of objects in a safe way, for a template like, for instance:

{{#groups}}
    Group: {{name}}
    {{destroy}}
    {{#people}}
        - Person: {{name}}
        {{destroy}}
        {{#pets}}
            -- Pet: {{name}}
            {{destroy}}
        {{/pets}}
    {{/people}}
{{/groups}}

[ghost]

@groue Thanks. That example works just fine, the destroy methods will never be called unless explicitly exposed via a 'drop'.

[groue]

Yes. I wasn't wondering if it works. Only how to do it. I can't imagine the code that brings me from a object tree of groups, people and pets, to a tree of safe groups, safe people, and safe pets.

[ghost]

@groue The how lies pretty much in the combination of a to_tache on every object and a safe context object that returns the response of the to_tache method only if it's a hash, drop, or Tache::Safe instance. See: https://github.com/thelucid/tache/blob/master/lib/tache/safe.rb#L30-L39

[ghost]

@groue The https://github.com/thelucid/tache/blob/master/lib/tache/safe.rb file contains everything to do with safe views, apart from that, everything else is standard Mustache.

[groue]

The how lies pretty much in the combination of a to_tache on every object and a safe context object that returns the response of the to_tache method only if it's a hash, drop, or Tache::Safe instance.

I'd be curious of seeing some actual code, actually.

[ghost]

@groue What code do you mean, usage code?

[ghost]

@groue Here are the tests for safe views: https://github.com/thelucid/tache/blob/master/test/safe_test.rb

[groue]

Yes, some usage code for a slightly complex graph of objects (as the groups of people that own pets).

[ghost]

@groue No problem, I'll add a test for that example. In essence though, a nested object graph will work in just the same way as a flat one... it's turtles all the way down ;)

[groue]

Actually my question lies at the level of the conversion of a graph of objects belonging to the model realm, to a graph of objects ready for being mustached. How would a non-trivial example look like?

[ghost]

@groue Just working on a good example and some additional functionality, will get back to you.

[ghost]

@groue Creating a complex object graph longhand using drops can get a little verbose (https://gist.github.com/4436684), however I've just added a tache class method that makes this process super simple:

class Person < MyORM
  tache :name, :age, :etc
end

Your example using this method: https://gist.github.com/4437186

[groue]

@thelucid: it would be nice if the model would not decide which keys it exposes to the templates. Since you're a Rails guys, you know how the white/black-listed model attributes are now under the controller's responsibility, for many good reasons. Do you think you could avoid the tache model method, while preventing the class explosion of the first gist?

[ghost]

@groue Perhaps, I took the idea directly from Liquid. There may be a way of achieving the same thing in a similar fashion to strong parameters although I quite like the simplicity of the tache method.

[groue]

Yes. Well done, @thelucid. I guess a delegation/injection pattern could let the controller in charge. Something like "Hey, dear delegate, I'm about to put this object in the Mustache context stack, and eventually wildly extract some keys from it. Should I keep it as it is, or do you have some other object I should use instead?". Controller could then rely on a tache-like model method, or provide its own custom logic. "I'm so glad you're asking! Take this safe object instead, please - Sure, pal." Safety would then be a controller-driven topic, not a feature of the core Mustache. Thoughts?

[ghost]

@groue I kind of follow, however we don't have delegates as a common practice in Ruby in the same vein as Objective-C. I guess this could be achieved with some kind of callback but it wouldn't be as transparent to the user i.e. they would have to do more for the same benefit.

I'd be intrigued to see an example of your ideal API for this.

[groue]

Call it dependency injection, if you prefer. There's a nice post by Jamis Buck about DI in Ruby: http://weblog.jamisbuck.org/2008/11/9/legos-play-doh-and-programming. Sure an Objective-C API would use the "delegate" wording, since this is how we do it. Yet the principle remains the same: keep the controller, i.e. the object between the model and the view, in charge.

[ghost]

@groue Still, not convinced that this could produce a nice API without the user having to do a lot of the work that I am currently handling for them.

Could you supply a code example of how you see this working as I can't envisage how it would look?

[groue]

Well... Something like that?

view = Tache.compile(template)
view.on_push do |object|
  # make sure we do not expose unwanted keys to the template
  object.to_safe_object
end
view.render('groups' => groups)

Not quite convinced myself. Haven't written enough Ruby lately :-)

Also, GRMustache already has a delegate, that deals precisely with objects that enter the context stack: https://github.com/groue/GRMustache/blob/master/Guides/delegate.md. It's a very powerful tool. However it only deals with the value of expressions such as name, person.name, or uppercase(person.name). It does not handle, today, each object involved in a compound path as person.pet.name, as your "safe" proposal implies. Hmmm...

[ghost]

@groue I see what you're trying to achieve but as you're calling to_safe_object on the unsafe object anyway, you may as well have just called the tache method on it beforehand. This makes it a concern of the developer rather than a useful part of the library.

In order for this to be useful I believe it needs to be a core part of the implementation or at least an optional extra like my 'tache/safe' implementation.