Smarty incompatible in S9Y 1.7?

Found a bug? Tell us!!
blog.brockha.us
Regular
Posts: 695
Joined: Tue Jul 03, 2007 3:34 am
Location: Berlin, Germany
Contact:

Smarty incompatible in S9Y 1.7?

Post by blog.brockha.us »

I installed the version 1.7 from the new S9Y repo on GitHub.
For a plugin I wanted to switch off the Smarty Security.

My S9Y understands this:

Code: Select all

$serendipity['smarty']->disableSecurity();
If this code is executed in a 1.6 S9Y Blog, it crashes (function unknown).

There I have to use the code documented in the s9y blog:

Code: Select all

$serendipity['smarty']->security = false;
But if I use this in my blog it crashes (unknown attribute).

Is there some incompatiblility?
- Grischa Brockhaus - http://blog.brockha.us
- Want to make me happy? http://wishes.brockha.us/
blog.brockha.us
Regular
Posts: 695
Joined: Tue Jul 03, 2007 3:34 am
Location: Berlin, Germany
Contact:

Re: Smarty incompatible in S9Y 1.7?

Post by blog.brockha.us »

This does solve the problem for me at the moment. But I don't like it that much..

Code: Select all

        if (version_compare($serendipity['version'], '1.7-alpha1','>=')) {
            $serendipity['smarty']->disableSecurity();
        }
        else {
            $serendipity['smarty']->security = false;
        }
- Grischa Brockhaus - http://blog.brockha.us
- Want to make me happy? http://wishes.brockha.us/
Timbalu
Regular
Posts: 4598
Joined: Sun May 02, 2004 3:04 pm

Re: Smarty incompatible in S9Y 1.7?

Post by Timbalu »

Hi Grischa

They changed a lot in Smarty3. And we set the plugin path into allowed path by default now.
So this constant might work - Smarty::SMARTY_VERSION - to switch, only available with Smarty3.

But I need some help to understand the very need of disabling security, please give me an example, as I am not sure you need this to set at all with 1.7.
Regards,
Ian

Serendipity Styx Edition and additional_plugins @ https://ophian.github.io/ @ https://github.com/ophian
blog.brockha.us
Regular
Posts: 695
Joined: Tue Jul 03, 2007 3:34 am
Location: Berlin, Germany
Contact:

Re: Smarty incompatible in S9Y 1.7?

Post by blog.brockha.us »

Well, the reason for doing this is very simple in my case I guess: I'm not very familiar with the internals of Smarty and while doing a (felt like being simple) 40 line template I was bombed with smarty security errors. ;) So the "Why" might be a lack of knowledge, I'm sure. (Perhaps you or someone else can help me with this in another thread)

But that is not the point: I am worried about incompatibilities or crashing templates / plugins because of this change. Even the s9y docu points to the old version of disabling security.

While beta testing a new plugin / template a testers blog even went white without a chance to enter the admin area to disable the plugin. So I am worried, that users will see similar problems when upgrading to s9y 1.7 but still having smarty 2.x coded plugins..

Update: When I check the smarty version given by the named constant I get "Smarty-3.1.6". Why isn't it "3.1.6"? I guess that's bad for comparing version numbers using version_compare, right?
- Grischa Brockhaus - http://blog.brockha.us
- Want to make me happy? http://wishes.brockha.us/
Timbalu
Regular
Posts: 4598
Joined: Sun May 02, 2004 3:04 pm

Re: Smarty incompatible in S9Y 1.7?

Post by Timbalu »

Hmmm,

Trusted dirs are plugins/ and templates/ with 1.7, so including template files {include file="foo.tpl"} from in there should not error...
If using {include_php}, this is deprecated with Smarty 3x.
Try to use registered plugins to properly insulate presentation from the application code.
{php} in templates is something we dont really want to have, I assume, else every security setting is obsolet. What did you try to do and what did these errors say in detail?

As far as I know, plugins or templates using individual disabled security are very rare... so having a switch for them into Smarty2 or Smarty3 Combat should be easy to provide via the $propbag->add('requirements' ...) array.
At some point we will need to break compatibility in Plugins, that is why we already started a discussion (was about {block} elements) lastly on how to do this possibly in future...
Until then we have to write some more BC mode functions into our new smarty class or do this as approached in the plugin itself.

Comparing version between Smarty2 and 3 is easy using old $_version property or the new 3x constant. Comparing version between 3x versions, needs to strip the 'Smarty-' from the Smarty Constant string, maybe its a leftover which is gona be set back in future smarty versions, ...but who knows?! :wink:
Regards,
Ian

Serendipity Styx Edition and additional_plugins @ https://ophian.github.io/ @ https://github.com/ophian
Timbalu
Regular
Posts: 4598
Joined: Sun May 02, 2004 3:04 pm

Re: Smarty incompatible in S9Y 1.7?

Post by Timbalu »

These $inclusion [INCLUDE_ANY} security things are deprecated with 1.7. (Why do you need to silence @$serendipity['smarty']->fetch()?)

Code: Select all

if (Smarty::SMARTY_VERSION) {
    $serendipity['smarty']->disableSecurity();
    $content = @$serendipity['smarty']->fetch('file:'. $tfile);
} else {
    $inclusion = $serendipity['smarty']->security_settings[@INCLUDE_ANY];
    $serendipity['smarty']->security_settings[@INCLUDE_ANY] = true;
    $serendipity['smarty']->security = false;
    $content = @$serendipity['smarty']->fetch('file:'. $tfile);
    $serendipity['smarty']->security_settings[@INCLUDE_ANY] = $inclusion;
}
Regards,
Ian

Serendipity Styx Edition and additional_plugins @ https://ophian.github.io/ @ https://github.com/ophian
garvinhicking
Core Developer
Posts: 30022
Joined: Tue Sep 16, 2003 9:45 pm
Location: Cologne, Germany
Contact:

Re: Smarty incompatible in S9Y 1.7?

Post by garvinhicking »

Hi!

I am thinking if we can maybe intercept setting $serendipity['smarty']->security in our smarty class of s9y 1.7? Then we could catch the error, and use the proper disableSecurity() call in the wrapper.

Like this patch:

Code: Select all

diff --git a/include/serendipity_smarty_class.inc.php b/include/serendipity_smarty_class.inc.php
index 8ac8bfc..9ece7f2 100644
--- a/include/serendipity_smarty_class.inc.php
+++ b/include/serendipity_smarty_class.inc.php
@@ -47,6 +47,19 @@ class Serendipity_Smarty extends Smarty
     // bc mode for plugins Smarty2 compat INCLUDE_ANY fetch() calls - to avoid an undefinied property error.
     public $security_settings = false;
     
+    
+    public function __set($name, $value) {
+        if ($name == 'security') {
+            if ($value) {
+                $this->enableSecurity('Serendipity_Smarty_Security_Policy');
+            } else {
+                $this->disableSecurity();
+            }    
+        } else {
+            parent::__set($name, $value);
+        }
+    }
+    
     /**
      * It is often helpful to access the Smarty object from anywhere in your code, e.g in Plugins.
      * Enables the Smarty object by instance always. The singleton pattern ensures that there is only one instance of the object available.
What do you guys think?

Apart from that, maybe depending what you actually entered into the template that requires security=false, we could try to add some internal methods to achieve the same, so that the plugin doesn't need to disable security?

Regards,
Garvin
# Garvin Hicking (s9y Developer)
# Did I help you? Consider making me happy: http://wishes.garv.in/
# or use my PayPal account "paypal {at} supergarv (dot) de"
# My "other" hobby: http://flickr.garv.in/
Timbalu
Regular
Posts: 4598
Joined: Sun May 02, 2004 3:04 pm

Re: Smarty incompatible in S9Y 1.7?

Post by Timbalu »

Hi Garvin

Looks great! ;-) But I am not really common with magic methods...

Does that mean we have to keep all these old unnecessary

Code: Select all

        $inclusion = $serendipity['smarty']->security_settings[@INCLUDE_ANY];
        $serendipity['smarty']->security_settings[@INCLUDE_ANY] = true;
        $content = $serendipity['smarty']->fetch('file:'. $tfile);
        $serendipity['smarty']->security_settings[@INCLUDE_ANY] = $inclusion;
        return $content;
in every smartified plugin, unless some very rare ones disable security?

Then what happens if one plugin disables security globally, if we use the same object Instance everywhere else?

I would prefer to find out why some coding throw security errors and how to solve them without loosing security itself.
Regards,
Ian

Serendipity Styx Edition and additional_plugins @ https://ophian.github.io/ @ https://github.com/ophian
garvinhicking
Core Developer
Posts: 30022
Joined: Tue Sep 16, 2003 9:45 pm
Location: Cologne, Germany
Contact:

Re: Smarty incompatible in S9Y 1.7?

Post by garvinhicking »

Hi!

One more thing: Reading the oembed.tpl file, I don't actually think that disabling security should be necessary?

However the __set() thing might be important for old plugins that use the security_settings arrays; I haven't tried plugins like these yet, do they work or do they throw errors with smarty3/s9y 1.7?

I think for old plugins we should just make sure that a call to ->security_settings does not throw any errors, because they only really used it so that smarty would load templates from the /plugins directory, which was only possible with changing the INCLUDE_ANY constant.

Regards,
Garivn
# Garvin Hicking (s9y Developer)
# Did I help you? Consider making me happy: http://wishes.garv.in/
# or use my PayPal account "paypal {at} supergarv (dot) de"
# My "other" hobby: http://flickr.garv.in/
Timbalu
Regular
Posts: 4598
Joined: Sun May 02, 2004 3:04 pm

Re: Smarty incompatible in S9Y 1.7?

Post by Timbalu »

garvinhicking wrote:One more thing: Reading the oembed.tpl file, I don't actually think that disabling security should be necessary?
Yes, that was wondering me too.
garvinhicking wrote:However the __set() thing might be important for old plugins that use the security_settings arrays; I haven't tried plugins like these yet, do they work or do they throw errors with smarty3/s9y 1.7?
That is why I added the

Code: Select all

    // backward compat for plugins INCLUDE_ANY fetch calls - undefinied property Serendipity_Smarty::security_settings
    public $security_settings = false;
Then all possible errors vanished.
garvinhicking wrote:I think for old plugins we should just make sure that a call to ->security_settings does not throw any errors, because they only really used it so that smarty would load templates from the /plugins directory, which was only possible with changing the INCLUDE_ANY constant.
see second
Regards,
Ian

Serendipity Styx Edition and additional_plugins @ https://ophian.github.io/ @ https://github.com/ophian
blog.brockha.us
Regular
Posts: 695
Joined: Tue Jul 03, 2007 3:34 am
Location: Berlin, Germany
Contact:

Re: Smarty incompatible in S9Y 1.7?

Post by blog.brockha.us »

Whew! Much stuff to read. :mrgreen:

Well, there were 2 things, that caused Security issues:

1. I loaded the oembed.tpl from the plugins dir, if not found in the template dir. This caused an Smarty error.
2. I tried to do some string searches in the tpl. I used "stripos". This seems to be a PHP and not a smarty function, so it caused another security error. In the actual tpl I managed to do w/o this comparing.

The INCLUDE_ALL is needed for Smarty 2.x templated plugins, if you want to load a tpl from the plugins directory. So it has to be there in order to make it work in older versions (if you don't switch off the security completely as I did). I found many plugins setting this btw, so this should be supported by the Smarty 3 class, too. Maybe simply ignored, if not needed anymore, but it should not produce errors (it doesn't at the moment, if I'm right).

I'm not sure, if there were more security problems, I didn't try more, as this was consuming more time than the ridiculous simple tpl.. ;)
- Grischa Brockhaus - http://blog.brockha.us
- Want to make me happy? http://wishes.brockha.us/
Timbalu
Regular
Posts: 4598
Joined: Sun May 02, 2004 3:04 pm

Re: Smarty incompatible in S9Y 1.7?

Post by Timbalu »

blog.brockha.us wrote:1. I loaded the oembed.tpl from the plugins dir, if not found in the template dir. This caused an Smarty error.
...would have been good to know here what this error said in detail. Can you reproduce it?
blog.brockha.us wrote:2. I tried to do some string searches in the tpl. I used "stripos". This seems to be a PHP and not a smarty function, so it caused another security error. In the actual tpl I managed to do w/o this comparing.
That is usual, as not allowed by S9y, in all bundled Smarty libs. No change to prev versions.
blog.brockha.us wrote:The INCLUDE_ALL is needed for Smarty 2.x templated plugins, if you want to load a tpl from the plugins directory. So it has to be there in order to make it work in older versions (if you don't switch off the security completely as I did). I found many plugins setting this btw, so this should be supported by the Smarty 3 class, too. Maybe simply ignored, if not needed anymore, but it should not produce errors (it doesn't at the moment, if I'm right).
Yes, true, this 5-liner (see above) is needed in all smartified plugins with Smarty2 bundled. With Smarty3 there is no need for this, as we decided to allow the plugins dir in general. While the constant is silenced, we just needed to define the undefined old property security_settings, which is already set.

The problem of your code is, you are using the global security, not the allowed path property.
These smartified plugins set the globally allowed path property security_settings[@INCLUDE_ANY] from false to true and immediately back to false after the fetch call. So disabling $serendipity['smarty']->security = false; is something wrong here, as the original 5-liner should have allowed to fetch the tpl inside the plugins dir with S9y Smarty 2x versions and with 1.7 even without. So your error must have another reason, IMHO.

That is why I'd really like to know what kind of security load error happend there before, which could only be silenced by disableSecurity().
Regards,
Ian

Serendipity Styx Edition and additional_plugins @ https://ophian.github.io/ @ https://github.com/ophian
blog.brockha.us
Regular
Posts: 695
Joined: Tue Jul 03, 2007 3:34 am
Location: Berlin, Germany
Contact:

Re: Smarty incompatible in S9Y 1.7?

Post by blog.brockha.us »

Hmm.. It seems, I can't reproduce it anymore. :| I commented out the Security disabling and it is working, even when changing the tpl (to be reloaded) or killing teplages_c.
I've sent this version to my (1.6) beta testers, lets see, if they get into trouble with that version.
- Grischa Brockhaus - http://blog.brockha.us
- Want to make me happy? http://wishes.brockha.us/
Timbalu
Regular
Posts: 4598
Joined: Sun May 02, 2004 3:04 pm

Re: Smarty incompatible in S9Y 1.7?

Post by Timbalu »

Yes please, as allowing a plugin to set back smarty security at all, is nothing I would expect Serendipity to have allowed, else any security setting would just be a fake ...

Code: Select all

#    $serendipity['smarty']->disableSecurity();
#    $serendipity['smarty']->security = false;
Regards,
Ian

Serendipity Styx Edition and additional_plugins @ https://ophian.github.io/ @ https://github.com/ophian
Timbalu
Regular
Posts: 4598
Joined: Sun May 02, 2004 3:04 pm

Re: Smarty incompatible in S9Y 1.7?

Post by Timbalu »

... loud thinking ...

The more I think about it, I presume, enabling Smarty-Security per default is something useless in S9y, as this is good for situations when you have untrusted parties partitially editing the templates via ftp, and you want to reduce the risk of system security compromises through the template language.

But we must allow S9y-plugins to use their own Smarty-templates, which makes it a need to fetch tpls by demand (see 5-liner) or in general for the S9y-plugins dir (preferable). As we have seen, S9y-plugins are able to switch on/off Smarty-Security as they want. This can't be prevented via the general Smarty settings.

We do not provide Serendipity specific ftp settings others than the installed Server has set. Therefore setting Smarty-(Path-)Security is useless here, as we have to trust our S9y-template and S9y-plugin designers as long as they don't open ftp in allowed and trusted Smarty-template dirs against the Server settings, don't we? BTW, we can't open ftp access to S9y-template dirs, as there are php files residing, which are able to do above. Autsch!

Is there more :?:

PS.
The old 5-liner can be easily replaces by

Code: Select all

    $serendipity['smarty']->security = false;
    $content = $serendipity['smarty']->fetch('file:'. $tfile);
    $serendipity['smarty']->security = true;
which shows the un-necessity of this manner.
Regards,
Ian

Serendipity Styx Edition and additional_plugins @ https://ophian.github.io/ @ https://github.com/ophian
Post Reply