Smarty

[andreas]

Hi!

Some time ago I started [url=http://www.phpinsider.com/smarty-forum/viewtopic.php?t=4992]a discussion on using htmlspecialchars() on every assigned variable. "default_modifiers" turned out to be not very useful at all ("I see default_modifiers as a dead-horse. Let them rest in peace"), I posted some ideas how to improve the code, but the Smarty Devs said that global/default output escaping is not the right solution for the problem.

(since I cannot see a difference between the text and link format, here the url of the thread from last year: http://www.phpinsider.com/smarty-forum/viewtopic.php?t=4992)

I still disagree. Of course you can construct special situations where this is not useful, but Smarty has a lot of features which are not useful in every situation I think
In my opinion, something like that (if well implemented) would be very useful for the majority of users - and can prevent a lot of XSS security problems.

The current situation is:

option 1 - do something like that in PHP-Code:
[php]$smarty->assign('string', htmlspecialchars($string, ENT_QUOTES, 'UTF-8'));
foreach ($array as $k=>$v) {
$array[$k] = tmlspecialchars($v, ENT_QUOTES, 'UTF-8');
}
$smarty->assign('array', $array);
...[/php]

or option 2 - do something like that in your template:

[boots]

Hi andreas.

Looking at that thread (again! ), it seems that there was agreement that the behaviour you suggest is an improvement, but the stopping issue is BC. I am still sympathetic to this idea. Yet I think it would be useful if it was not completely automatic. By that, I mean that it should be possible to configure what (if any) escaping function is auto-applied to scalars. It might also be useful to allow individual overrides where required in the template itself (though it seems that would be tricky to implement). Maybe instead of $smarty->escape_output = t/f have $smarty->output_escape_func = 'htmlspecialchars'; and have the default be false (or maybe null). The addition of the an output_charset seems like a good idea, but again, it should, by default be null.

Of course I can't speak to how Monte or Messju (or for that matter, others in the community!) would react, but FWIW, if you have a patch that does this cleanly and does not affect BC, I would support it.

Best Regards.

[andreas]

Aside from that - how can a Template Designer know, which variables he can trust and which not? Since the common goal is to seperate the Design/HTML-Code from Business Logic, a Template Designer should only know the variables, not where the values in it come from, and what has happened to them in PHP-Code before (values hard-coded in PHP-Code? values from database? user-input? already escaped in PHP?).

If you follow these common design priniples, template-variables containing HTML-Code should be avoided, because it blurs the seperation of Logic and Design.
However, if you have another opinion, if you want to pass HTML regardless of those arguments, you still have a lot of choices:

1. simply don't use $smarty->escape_output if you don't need it
2. use own smarty plugins (output of those don't gets escaped)
3. Perhaps a modifier can be added to prevent escaping, if the modifier is used (e.g. {$var|no_escape})

Another important point is, if you compare my old code above, with the new one, the escaping is really inconvenient, and if something like that is inconvenient, people tend to neglect it.

[boots]

Hi andreas.

I agree with your reasoning (particularly about passing HTML in as variables -- that is a bad practice). I think you may not appreciate the fact that HTML is not the only thing that Smarty is used to template.

Regarding your comments:

- we should filter input (ie: data validation) but not alter it; this includes input into the templates. Otherwise, as you astutely point-out, one can not know what has happened to the data before it arrived. This was a major problem with magic_quotes et al and lead to unnecessary confusion. The key point is that data taken as an input should not already be transformed into a target format. IMO, a template designer should be able to "trust" all inputs.

- because templates form the output portion (ie: data consumed and transformed into a target format), it is completely reasonable that a template designer be knowledgeable of escaping said output. That doesn't imply that the process can not be automated to an extent (ie: give them tools) but I don't think that we should take the view that template designers (as well as programmers implementing Smarty in their view layer) have NO responsibility in this regard. IMO, a template designer should ensure "trust" for all outputs. That said, it may be possible to do this by agreeing with the application designer that certain Smarty features (such as those being discussed) will be turned on and have specific values. This is part of the application/template contract that I am so fond of mentioning

Anyhow, I don't think that auto-escaping on the assumption of HTML should be the default for Smarty 2.x. I suspect these issues will be revisited in Smarty 3.x (whenever that happens ) but in that case, I'd rather have different modes so that all escaping can be abstracted based on doctypes. That's a more involved (and I suppose rather debatable) idea and I don't see much room for it in the 2.x architecture. Thoughts?

[andreas]

Hi boots!

[boots]

IIR, some of the bundled plugins use echo -- but they shouldn't. All plugins should return the result. Maybe normalizing that in the codebase can be the first step to see this through. Unfrtunately, that doesn't address the issue for non-bundled (ie: 3rd party) plugins...

[andreas]

[boots]

Hi Andreas!

I think we cross-posted and that that is where the confusion came from. Your first post and mine in this thread seem to be where we both are on the same page. Forget the noise and consider the patch Sorry for any confusion there....

Best regards.

ps. an output_escape_func and output_unescape_func might be required to allow for an unescaping at template time -- if you catch my meaning.

[andreas]

Ok, here is a first patch against CVS-HEAD:

I added a $escape_output and $output_charset to the Smarty class, which are passed to SmartyCompiler too. Both default to NULL.

In SmartyCompiler I added a "_escape_var()" methode, which puts an escape-function around the variable name. It's called from _compile_tag() line 449.

This does not break BC, you have to change the new member variable to get any effect.

But I'm not sure if I already catched all points where output is printed. As written in this thread, I found 11 places, where the SmartyCompiler uses "echo", but most of them write things like tags, which are not modifiable by any input. But I'm not yet an expert of smarty source, so there are probably some more places to catch. What I catch now, are variables and methode calls.

If you look at the plugins, my function is called after modifiers are applied, so the output generated by modifiers will be escaped too.

Other plugins like html_options call the smarty_function_escape_special_chars() function to escape user-input. So with an old/unpatched version of smarty, you allways get the values in plugins using smarty_function_escape_special_chars(), ecaped by:

[php] $string = htmlspecialchars($string);[/php]

This does not escape '. That's what the plugin situation is like today.

But why is this a function and not a Smarty methode? this would make it easier to change this function. Now it's hardcoded everywhere and it's difficult to change htmlspecialchars() to htmlentities() or add ENT_QUOTES... Plugins allways get the Smarty object, so this could be easily handled, if they would use a Smarty methode instead of a function, which does not know the Smarty object. The easiest way to change this without breaking BC, is passing a parameter, which says what escaping to use - or pass the Smarty object.
In my opinion the better way would be to go across all plugins, and change them to use a smarty methode instead of smarty_function_escape_special_chars().

Example code to make use of the patch:

[php]$smarty = new Smarty;
$smarty->escape_output='html';
$smarty->assign('xss', '<XSS>"\'');
$smarty->display('xss.tpl');[/php]

[andreas]

I looked at many plugins now, the "modifiers" don't seem to be an issue, because the results will be escaped, and I don't see a problem with it (perhaps "escape" modifier should be filtered in my patch too).

Than you have the "build-in" functions - I also don't see any need for a change here, and no BC problems...

Only the "custom functions" are a little bit more problematic.

Most html_* functions use the smarty_function_escape_special_chars() function, which seems like a good start. In my previous post I have already written something about that.

But there are some plugins, where input is returned without ecaping:

- cycle
- html_table
- mailto
- popup
- popup_init
- textformat

First I'm wondering, why html_table does not use smarty_function_escape_special_chars(), like other html_* functions do. So content in <option> gets escaped today, content in tables does not.

mailto, of course you can pass XSS code to an email field, so I'd like to use escaping here too. (again, remember it's only an option )

and for cycle, I'm not sure, but I'd suggest to use escaping here too. Usually you do things like

[boots]

messju, monte? This would be a good time for folks to give some input

[boots]

I still prefer $output_escape_func rather than $escape_output both for naming consistency and for the fact that I'd rather have this passed off to a callback rather than use a "mode". This seems more inline with Smarty's typical design considerations, IMHO.

[andreas]

[boots]

[andreas]