Jeff-Lewis / smarty-php

Automatically exported from code.google.com/p/smarty-php
0 stars 0 forks source link

SmartyException shouldn't output (html)encoded text #130

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
...or at least make it user configurable via a define (keeps it backwards 
compatible if fixed). 

Smarty shouldn't just assume HTML is being used in the presentation or 
exception handling layer. In fact I'm not. This is why the exception definition 
below is screwing me around. As a dirty fix, I made a special exception for 
SmartyExceptions in my exception handler where I pass the message through 
html_entity_decode() first. 

Code:

/** 
 * Smarty exception class 
 * @package Smarty 
 */ 
class SmartyException extends Exception { 
    public function __construct($message) { 
        $this->message = htmlentities($message); 
    } 
} 

I'm using version 3.1-DEV (Debian package).

Original issue reported on code.google.com by craig.ma...@gmail.com on 13 Jan 2013 at 4:28

GoogleCodeExporter commented 9 years ago
This was introduced to close a sceurity hole because it was possible provoke an 
exception with injected script execution by an URL with parameters under 
certain conditions. As the usage of Smarty in an none HTML environment is the 
exception it should be acceptable to handle this case in the exception handler.

Original comment by Uwe.Tews@googlemail.com on 13 Jan 2013 at 5:35

GoogleCodeExporter commented 9 years ago
You can keep the vague security hole closed by using a define constant.

Original comment by craig.ma...@gmail.com on 13 Jan 2013 at 5:40

GoogleCodeExporter commented 9 years ago
It's so easy to fix this poor design. Like this for example:

/** 
 * Smarty exception class 
 * @package Smarty 
 */ 
class SmartyException extends Exception { 
    public function __construct($message) { 
        if (!(defined('SMARTY_LEAVE_MY_EXCEPTIONS_ALONE') && SMARTY_LEAVE_MY_EXCEPTIONS_ALONE)) { 
            $this->message = htmlentities($message); 
        }
    } 
} 

All it takes is a attitude with less laziness and arrogance.

Original comment by craig.ma...@gmail.com on 13 Jan 2013 at 5:47

GoogleCodeExporter commented 9 years ago
B.t.w, I hardly see it as being the exception to log Smarty exceptions to the 
server error log for example instead of to the HTML page.

Original comment by craig.ma...@gmail.com on 13 Jan 2013 at 5:52

GoogleCodeExporter commented 9 years ago
Another solution is to define the exception classes to use in static methods so 
that users can subclass Smarty and define their own exception classes if they 
want to.

public static function exceptionClassBase() {
  return 'SmartyException';
}
public static function exceptionClassCompiler() {
  return 'SmartyCompilerException';
}

etc.

Original comment by craig.ma...@gmail.com on 13 Jan 2013 at 5:57

GoogleCodeExporter commented 9 years ago
I don't particularly like global constants and I wouldn't want to pollute 
Smarty any further. But I have absolutely no problem with

class SmartyException extends Exception {
    public static $escape = true;
    public function __construct($message) {
        $this->message = self::$escape ? htmlentities($message) : $message;
    }
}

allowing you to disable encoding by

SmartyException::$escape = false;

By keeping encoding the default we're preserving backward compatibility.

Original comment by rodneyrehm on 13 Jan 2013 at 8:40

GoogleCodeExporter commented 9 years ago
Yes that's even better.

Original comment by craig.ma...@gmail.com on 13 Jan 2013 at 8:46

GoogleCodeExporter commented 9 years ago
After asking Rodney for his opinion and his idea to use local constants I 
commited this solution to the SVN trunk.  It will later be included in 3.1.13

Original comment by Uwe.Tews@googlemail.com on 13 Jan 2013 at 9:16

GoogleCodeExporter commented 9 years ago
Thanks

Original comment by craig.ma...@gmail.com on 13 Jan 2013 at 9:28