Public methods in Mustache are available as lambda

[dswitzer]

One of the issues with the way that a component can extend the Mustache.cfc and use it's public members as context, is all public methods end up being available as lambdas.

This means the Mustache methods of init, render, getPartials and setPartials all are available. If you end up with a template that has the text "{{init}}" in the code, then CF ends up throwing an error because the "init" function tags a struct as an optional argument (not a string.)

The bigger issue is that these public methods shouldn't even be allowed to be part of the context.

One way to solve that would be to filter out these public methods from being included as context. This would fix the problem, but it does limit templates from ever using these keys as valid context (which could be problematic for some corner use cases.)

We probably also want a way to register other public methods that should be excluded from context--in cases where you've extended Mustache w/additional functionality.

[pmcelhaney]

If you pass the context explicitly ( mustache.render(template, context) ) you should be able to avoid problem. (In theory, at least. I haven't tested. :))

[dswitzer]

That does work when the context is passed in specifically, but it doesn't work with examples like Winner.cfc. If the Winner.mustache template has {{init}} in it, the code breaks. That's the bug I'm pointing out.

Part of the reason I've been using structs (which has brought it's own pain points, as you're aware from our other discussions) is that keeps the context very clean and well defined.

I'm just pointing out that there's an issue w/the current implementation that causes issues if your templates reference public members in the Mustache component.

[pmcelhaney]

Yes, that's a good catch. It's a drawback to extending Mustache.cfc instead of passing the context directly.

Have you considered creating a component that doesn't extend Mustache and passing that as the context?

<cfset context = createObject("component", "myComponent")>
<cfset template = "{{init}}">
<cfset mustache.render(template, context)>

[dswitzer]

@pmcelhaney

What I've played around is doing something like (but I actually keep the context private in my code:)

<cfcomponent extends="Mustache" output="false" persistent="false">
    <cfset this.context = {} />

    <cffunction name="render" access="public" output="false" hint="main function to call to a new template">
        <cfargument name="template" default="" />
        <cfargument name="partials" hint="the partial objects" required="true" default="#structNew()#" />

        <cfset arguments.context = this.context />

        <cfreturn super.render(argumentCollection=arguments) />
    </cffunction>

</cfcomponent>

This keeps all the context in a struct that's isolated from other public members.

[dswitzer]

@pmcelhaney

Also, the problem with code like this:

<cfset context = createObject("component", "myComponent")>
<cfset template = "{{init}}">
<cfset mustache.render(template, context)>

is that it only works if your context objects are really dumb objects.

I think in a real use cases, a context object is going to need at least one public method used for grabbing the contextual data from a persistent storage layer.

Dummy objects like above don't work for me, because at some point I always need access to more data than I'm willing to expose to my context. For example, my lambda methods will generally have to have some awareness of primary keys--but I don't want to expose those to my context objects.

Anyway, that's the design issue I keep running into when using objects for context. The best solution I've found is to use a struct for context--as I can keep that data clean.

[pmcelhaney]

Looks like you've solved the problem and didn't need to add any additional hooks to Mustache.cfc. :)

[dswitzer]

@pmcelhaney

While my code shows how you can work around it, it doesn't appear to be how Mustache.cfc was designed. If you implement usage based upon the directions, the {{init}} problem arises.

So, if the following code should still be allowable:

<cfset mustache = createObject("component", "Mustache").init()>
<cfset template = "Hello, {{thing}}!">
<cfset context = structNew()>
<cfset context['thing'] = 'World'>

<cfouptut>#mustache.render(template, context)#</cfouptut>

Then I think the render() method should not fail if the template contains the {{init}} token, as it's not really part of the "context" of your data.

I still think we either have to change the way this implementation works or we have to build a filter to exclude base public methods from being evaluated.

[pmcelhaney]

The render method doesn't fail. I copied your code and changed "Hello, {{thing}}" to "Hello {{thing}} and {{init}}". The output was:

Hello, World and !

If I actually have a key called init

 <cfset context['init'] = "Universe" />

The output was:

Hello, World and Universe!

Is that not what you expect? Are you getting different results? I tested CF9 -- maybe CF8 is different?

[dswitzer]

@pmcelhaney

Are you running the latest version of Mustache?

If I change /tests/Winner.mustache to:

Hello {{name}} and {{init}}

Then I see the following error thrown in the unit tests under both CF8 and CF9:

Complex object types cannot be converted to simple values. 

If you're using an older version of Mustache, you won't see the issue because the init() method doesn't have the argument line in it:

    <cffunction name="init" access="public" output="false"
        hint="initalizes and returns the object">
        <!-- this line causes problems in the current version of the code, because init() is passed a string -->
        <cfargument name="partials" hint="the partial objects" default="#StructNew()#">

        <cfset setPartials(arguments.partials)/>

        <cfreturn this/>
    </cffunction>

The error is caused because the init() function ends up getting passed in the "template" argument, which is a string.

Older versions, where the init() method didn't have any arguments, the function would still being invoked but just didn't return content. So while there wasn't an error generated, the function runs which could produce unexpected behaviors.

My point is that I don't think the core public members from the Mustache component should be able to be triggered from within the template.

Like I said, it's not a big deal if all your templates are closed and written in a controlled environment, but I suspect the reason most people would use Mustache is because they want to give some control to users to create dynamic content.

[pmcelhaney]

No, I'm not expecting the Winner.mustache example to work. The first example from the readme (hello world), in which the context is passed to the render method, should work.

[dswitzer]

@pmcelhaney

Structs as context works fine--which is why I've been using them.

This issue was specifically about using components that extend Mustache.cfc for the context.

[pmcelhaney]

Yeah, I don't think anything can be done about views that extend Mustache.cfc paired with templates that could potentially contain "{{init}}". However, I'm scratching my head and wondering why that would be a problem for you.

The option to create a Winner.cfc that extends Mustache.cfc is just a convenience. It allows you to create Winner.mustache in the same directory and call render() without any arguments (convention over configuration).

But in your case, the templates are created by users, not developers, right? So there's never going to be a Winner.mustache file. There's just a string in a database that gets passed to the Mustache.render() method. The context, which is also passed to the render() method, could be a struct or a CFC that doesn't extend Mustache and therefore doesn't inherit the init and partials methods.

Does that make sense?

[dswitzer]

@pmcelhaney

First and foremost, the purpose of this issue was to bring up a problem that other people may run into and cause discussion on whether anything should be done to resolve the issue w/in the code base.

Your last message still does address this issue at hand. For people using components as context any public methods become usable and potentially cause problems in your templates. This is true whether or not you extend the Mustache or not.

I see this as a bit problematic, because I can't think of any very useful components that won't end up with at least some kind of public method used for initialize.

Can you make really dumb objects that don't do anything, of course, but you might as well just use a struct in that case.

It just feels like the readme and examples all point your to using components for context (either by inheriting Mustache.cfc or passing the object in as an argument to render().)

In either situation, you're very likely to end up with methods that should not be considered part of the context. While you can certainly argue that it's up to the developer to solve that problem (and I'd agree to a degree,) it feels like the Mustache.cfc is encouraging people to use components for context.

However, this behavior doesn't seem to documented anywhere and I think most developers will not initially think of this as being a problem.

The problem that we have in ColdFusion (at least to support CF8) is that CF does not have a native constructor for components--which all the other languages I've seen Mustache implemented seem to at least have some kind of concept of.

So, while filtering out certain public members may technically break the spec, we don't have a way to construct an object and pass in some initialization values--we have to create at least one public method which we use as a pseudo-constructor. This is where the problem comes into play. I can't imagine any scenario in which you'd want that init method usable as a lambda.

Since this is a problem that can bit someone, I think we need to do something. It could be all or some of the following

1. Provide some mechanism for filtering out specific public methods from being used as lambdas.
2. Don't recommend using components as context.
3. Promote safer ways to extend Mustache.cfc by showing developers how to do it so public methods aren't exposed.

Can you really think of any real world components that wouldn't end up exposing at least one public method that you really wouldn't want to be executed? I can't.

[pmcelhaney]

Okay, I get it now! Even if you pass a "clean" component to the render method, odds are that component will at least have an init() method that you don't want to expose. Thanks for bearing with me.

A few ideas:

1. Have an optional property for contexts, called mustacheBlacklist or something similar, that contains a list of keys that should not be considered part of the context.
2. Indiscriminately ignore the init method, because no one in their right mind who knows what they're doing would put {{init}} in a template. That would put Musatche.cfc on par with other implementations in which the language has a real constructor.
3. Create a utility to filter a template, escaping or removing a list of tags, and encourage developers to use it when the template comes from user input.
4. Add an optional, third parameter to the render method that specifies a tag whitelist.

[dswitzer]

@pmcelhaney

Glad we're finally on the same page.

Since this problem really only affects components, I've been thinking about something on the lines of your first two suggestions (but blended together.)

Since "init" is such a standard pseudo-constructor naming convention, I was thinking for all components we'd automatically blacklist the "init" method. This seems reasonable to me and the likeliness that someone what's to use {{init}} in a template pretty minimal. If you do, you'll just need to solve the problem in another manor.

We'd also add a public method to Mustache.cfc called "setContextBlacklist", which could be used to define other methods that should be ignored from the context.

We'd end up adding code something like:

<!--- a list of public members to ignore as valid context --->
<cfset variables.Mustache.ContextBlackList = "" />

<cffunction name="setContextBlacklist" access="public" output="false"
    hint="sets a list of public members that should not be allowed as context">
    <cfargument name="blacklist"/>

    <!--- set the context --->
    <cfset variables.Mustache.ContextBlackList = arguments.blacklist />
</cffunction>

<cffunction name="isContextBlacklist" access="public" output="false"
    hint="checks if the current key should be executed">
    <cfargument name="context"/>
    <cfargument name="name"/>

    <!--- always blacklist the "init" method --->
    <cfset var blacklist = listAppend("init", variables.Mustache.ContextBlackList) />
    <cfset var metadata = "" />

    <!--- if extending the mustache object, ignore the public methods --->
    <cfif isObject(arguments.context)>
        <cfset metadata = getMetaData(arguments.context) />

        <!--- loop through the metadata to see if we extend the mustache object and ignore public methods --->
    </cfif>

    <!--- set the context --->
    <cfset variables.Mustache.ContextBlackList = arguments.blacklist />
</cffunction>

Obviously the code's not complete, just a skeleton of an idea.

The concept is if you extend Mustache, then the public methods in Mustache would automatically be ignored from the context. You can also always call setContextBlacklist() to manually specify which public members should be ignored. This would allow you to ignore public members when your passing in a component to the render() method.

The isContextBlacklist() would be used to determine if context should be ignored or not.

Anyway, these are just some ideas on how the problem could be resolved. Hopefully someone else will jump in with their 2 cents.

[pmcelhaney]

That's a possibility. I'm not completely sold on the blacklist though, now that I've given it some more thought. Aside from init, I'm not sure it should be Mustache's responsibility to know which parts of the context are safe or unsafe. Wouldn't it be better to give Mustache a context with only safe properties (aside from init)?

That doesn't mean we have to create dumb objects. What it does mean is instead of passing business objects directly to Mustache, we create views that reference the business objects. It might seem tedious and pointless in the short run if you have a 1:1 mapping of business object to view, but that's not likely to remain true as the app evolves.

For example, say you have an order object that you use as the context for a confirmation page and email. Later, you need to display the order again in an admin interface. The fields that are available on the customer's confirmation page vs. the store's administration page will probably vary. So it will be useful to have two or three different types of views backed by the same order component.

On the other hand, it can be useful to have a view that pulls information from multiple business objects.

When I started working with Mustache it took me a while to get my head around the idea that a template isn't really the view of MVC. It's kind of an extension of the view, but the view is an object in it's own right, aka the context.

Good discussion. And I agree, would love to hear others' thoughts.

[dswitzer]

@pmcelhaney

Since I'm mainly using Mustache in conjunction with user generated templates, maybe my experience isn't the norm, but using my normal objects for Mustache doesn't make much sense. I need special objects where limited data is exposed, which is specifically formatted and the context is intuitive for users. I really don't want my context verbiage to even really map to my internal API verbiage, not only to mask that from end users, but also because it's not always very intuitive for end users.

For example, I might be dealing with variable names like:

• CustomerFullname
• CustomerPhoneNo
• CustomerEmailAddress

While those names may be intuitive w/in the context of the app's code base, for a non programmer they're not great. A much cleaner solution would be:

• Customer.Name
• Customer.Phone
• Customer.Email

I like the idea of namespaces, because I might have things like "Assignee", "Creator", "Customer", etc all w/in the same context that all has the same information available. Namespaces I think make it much easier to remember.

Anyway, I'm almost always going to need a specially object/context just for Mustache usage. That means having a blacklist isn't going to be a huge deal for me personally.

However, if you do have the ability to use your native objects, then I do think there are times when some of the methods you'd still want to hide. While you could extend your base object and make those public methods return empty strings (or null) it seems like a lot of extra work, but maybe it's still the safest method.

Lastly, we still have a problem if a user extends Mustache. There are several public methods and I still think those need to be blacklisted from the context--unless we're going to stop promoting extended Mustache as a valid way to go. Once again, the public methods are:

• init
• render
• getPartials
• setPartials

While init() is the only what currently breaks, they could all cause some really unexpected behavior. This potentially could get worse as the code grows/changes. For example, if we add a compile() method to compile a template to native code, we're most certainly not going to want that to be evaluate as context.

[pmcelhaney]

The exposure of the render, getPartials, and setPartials becomes a problem if two conditions are true:

1. The templates are created by users rather than developers.
2. The render() method is called without passing in the context, which implies that a view was created by subclassing Mustache.cfc.

I can't think of a good reason for both of these conditions to be true at the same time. If you create a Winner.cfc that extends Mustache.cfc, it's probably because you're also creating a winner.mustache file. What other advantage is there? And if you're creating a winner.mustache file, that implies the first condition isn't true.

While init() is the only what currently breaks, they could all cause some really unexpected behavior. This potentially could get worse as the code grows/changes. For example, if we add a compile() method to compile a template to native code, we're most certainly not going to want that to be evaluate as context.

That's a keen observation. We need to be careful about adding new public methods to Mustache.cfc because they could break existing templates. It might be better to put the compile() method in a new component (MustacheTemplate.cfc?) and modify the render() method so it can accept a compiled template.

[dswitzer]

@pmcelhaney

While I agree that developers shouldn't extend Mustache to a component they plan on using with user generated templates, I think we do either need to warn people that's an issue or potentially filter that out from the usable context.

I just think to the vast number of developers, they won't immediately connect the fact that the public methods are available as context. Just to illustrate my point, it even took us going back and forth quite a bit until you saw what I was trying to say (which was probably my fault for not describing the problem in enough detail.) My point is that I don't think the issue is something that is immediately clear.

[pmcelhaney]

Sure, we can add a warning to the documentation.

I'd also like to make sure that by default, extending Mustache isn't really any more dangerous than passing a context to the render function.

1. init should be filtered in all cases. I don't even see any real value to making the filter optional.
2. setPartials at worst will just cause an error. We should throw a descriptive error if a struct isn't passed.
3. getPartials returns a struct. We need to make sure a struct isn't converted to a string and printed in the template.

4. render I'm guessing would end being called recursively until the stack overflows. I wonder if we could detect that by setting a flag within the render function?

   <cffunction name="render" access="public">
          <cfparam name="variables.imDoingRender" default="false">
          <cfif variables.imDoingRender>
             <cfreturn "">
          </cfif>
          <cfset variables.imDoingRender = true>
          <!--- existing body of the render function --->
          <cfset variables.imDoingRender = false>
          <cfreturn results/>
   </cffunction>

Finally, we should be very, very careful about adding public functions to Mustache in the future.

[dswitzer]

@pmcelhaney

I still think we'd be better off just blacklisting public methods in Mustache.cfc when the context object was extended from Mustache. Any blacklisted item would be treated as if it didn't exist. I think this is simpler in the long run and depending on how we implement it, could even be future proof. See my isContextBlacklist() function above. While it's incomplete, it shows the basic structure.

[pmcelhaney]

Okay, I should have let this go a long time ago. Your solution makes sense and it's simpler. Go ahead and do the blacklist.

[rip747]

sorry i'm late to the party. i've been reading the thread and trying to comprehend everything. From what I'm getting the issue is that you want to exclude certain public method from being called form a template since they can potentially cause an error. from what I'm reading, the solution proposed is to create a blacklist of method name that will cannot be used as tokens in the template.

what i would like to proposed is why not just make these methods as private. It shouldn't be too hard to then alter the render method to check what the "access" of the method is through metadata and not allow calling of private methods. i think this would be a better solution as it follow the normal flow of coding rather then creating and managing an internal list.

[markmandel]

I like the private approach, but I'd suggest using an annotation, something like:

mu:notLambda="true" (or something better, I can't think of one).

You don't want to be forced into having to declare all your lamdas as public - you may not want them to be.

[rip747]

All methods in CF are public by default. That's the beauty of this approach.

[dswitzer]

@rip747 @markmandel

The problem is there are methods in Mustache.cfc that are public by default--init, render, getPartials and setPartials. So, if your "context" object is a CFC that extends Mustache, then all of these are available as Lambas--which is the problem.

That's the root of the problem.

Also, if you're using a CFC as the "context" (even if you don't extend Mustache) you will almost always need at least one public method used for initialization or defining the context from persistent source. Since this method is public, it means it's also available as context for you object.

The docs imply that extending Mustache is an appropriate design choice, but doesn't mention that you may be exposing things as context that you did not anticipate and could potentially cause problems w/the execution of user-defined templates.

[pmcelhaney]

How about this?

<cffunction name="imALambda">
   ...
</cffunction>

<cffunction name="imNotALambda" mustache:ignore="true">
   ...
</cffunction>

[rip747]

@dan, i think the only methods that really need to be public in Mustache are init and render. everything else can be marked as private. with that said, it would be easy to just filter out those two method names when the render method is called.

[pmcelhaney]

@rip747 Private methods are already ignored by Mustache. We're trying to solve for the case where:

1. The context object has a method, foo, which has to be public.
2. The method isn't meant to be part of the context for Mustache -- there's no reason to put {{foo}} in a template.
3. The template comes from user input, so we can't be sure {{foo}} won't be part of the template, even though it makes no sense.

[dswitzer]

@rip747

While that may be true, it doesn't address the issue that other methods that will need to be made public in order to make a context object potentially useful.

I like the Patrick's metadata. It's still a blacklist, but one supported directly via metadata. This gives us a syntax to blacklist that supported via the code.

[rip747]

maybe i'm just not getting what the issue is and i need a clearer explanation and/or example. i don't see why if you're creating your own object to be used in the context, why you can't just have the method that the context needs access to marked as public and mark all other methods as private.

[dswitzer]

@rip747

There's two issues at hand:

1. Mustache has two methods "init" and "render" that are public and must be public. If you look at the Winner.mustache example and add "{{init}}" to the text, you're going to run into problems because running "init" w/a string argument throws an error. The bigger issue is that init and render really shouldn't be available as context to begin with.
2. Unless you're using really dumb objects for context--which are nothing more than the Winner.cfc example--then you're going to need at least one public method that populates the context. I'm using context objects that have methods that load data from a persistent source. This means I end up w/at least one method that needs to be public, to load my content into the context.

While this is a poor example, I think most people are going to end up doing something like:

<cfcomponent extends="mustache.Mustache">
  <cffunction name="loadWinner">
     <cfargument name="id" />

     <cfset var data = "" />
     <cfquery name="data">
       select
         name, email
        from
          Winners
        where
           id = #id#
     </cfquery>
     <cfset this.name = data.name />
     <cfset this.email= data.email/>
  </cffunction>
</cfcomponent>

While the problem is you don't want "loadWinner" to be available as context in your template. It's not a problem if you're all your templates are created by developers, but it's an issue if your templates may be user created.

[rip747]

thank you very much dan for the excellent explanation.

What I don't understand from your example is why you couldn't just name the method init and call super.init(). Having and following a convention like that would not only make it easier for us since we still only have to deal with init and render being public and filtered out of the context, but also for others who are looking at the code since init is the universal constructor in CFML.

so changing your example:

<cfcomponent extends="mustache.Mustache">
  <cffunction name="init">
     <cfargument name="id" />
     <cfset var data = "" />
     <!--- call Mustache init --->
     <cfset super.init()>
     <cfquery name="data">
       select
         name, email
        from
          Winners
        where
           id = #id#
     </cfquery>
     <cfset this.name = data.name />
     <cfset this.email= data.email/>
  </cffunction>
</cfcomponent>

[dswitzer]

@rip747

Yes, that particular example could use init()--which is why I said it's poor example.

However, the fact remains that you may need other public members to support configuring/loading the correct context data (or even changing the context data) and that's the root problem I'm pointing out.

Are there ways that the problem could be worked around? Yes. I just brought the issue up because we either should add support to blacklist these methods, suggest a different method that doesn't expose the public method problem or at bare minimum warn users of the problem. The current README makes it seem like the ideal way to use Mustache.cfc is to extend it, but doesn't warn of this problem.

[rip747]

i don't see why you couldn't just pass all those argument into the init.

Please understand that I'm not trying to be a jerk about this. I just don't want to be adding functionality to to the project that we would have to maintain when there is a way to get the same result and establish a convention.

That all said, you brought to light a very real security concern that we need to address. At the moment, filtering out init and render from the context and making sure any other internal methods are marked private should solve the issue.

[dswitzer]

@rip747

In many cases you probably could, in other more advanced cases that might not be an option. There are many ways you can potentially work around the problem. I'm just pointing out the problem--as it's not immediately apparent (and that's confirmed by how many comments there are in this issue trying to point out the problem.)

[pmcelhaney]

As a compromise, how about we create a GroomedMustache.cfc that extends Mustache.cfc and adds some form of blacklist/whitelist/metadata check? To keep it decoupled from Mustache.cfc, we could implement it as a preprocessor that checks the template and escapes any tags that should be ignored (e.g. {{loadWinner}} is changed to \{\{loadWinner\}\}).

[rip747]

i'd rather not have a another CFC that extends Mustache as it will just cause confusion. I know there is a way that we can solve this issue without having to add a black/white list.

i'll see if i can dig into the ruby source as see what they're doing to solve this issue.