Template parsing function

Question

I am using preg_replace_callback(), and I am concerned that I might have some sort of security risk. I am hoping to get a review of the below substitute() function. The purpose of the function is to parse a user provided template with user provided data. A quick description on how it is used is shown below.

The user posts data to the server, which is saved in a database (escaped for SQL injection of course). Note that the application will limit which array indexes are allowed (i.e. firstname, lastname, and phone only in this example).

$userProvidedString='Hello {?firstname?} {?lastname?}, Our database shows that your phone number is {?phone?}.';

$userProvidedArray=array(
    0 => array('firstname'=>'John','lastname'=>'Doe','phone'=>'(555) 222-1212'),
    1 => array('firstname'=>'Jane','lastname'=>'Doe','phone'=>'(555) 333-1212'),
    2 => array('firstname'=>'Mike','lastname'=>'Moe','phone'=>'(444) 222-1212'),
);

Later the data is pulled from the database, and parsed as follows:

foreach($userProvidedArray AS $contact)
{
    $customMessage=substitute($userProvidedString,$contact);
}

My question relates to the substitute() function. Specifically, does my use of preg_replace_callback() pose any security issues? Previously, I was using

$template_new= preg_replace('/\{\?(\w+)\?\}/e', '$contact["$1"]', $userProvidedString);

which uses the /e flag which I assume can be dangerous.

Does that danger still remain? Any other recommendations for it? I am currently using PHP 5.3.18, but will hopefully upgrade to 5.4 soon.

function substitute($template, $values, $deliminator='{??}') {
    //Replaces occurrences of {?name?} with $values['name] if it exists in $values.  Deliminators can be changed
    $deliminator="\\".implode("\\",str_split($deliminator));
    $half = (int) ( (strlen($deliminator) / 2) );
    $pattern = '/'.substr($deliminator, 0, $half).'(\w+)'.substr($deliminator, $half).'/';
    return preg_replace_callback(
        $pattern,
        function ($matches) use ($values) {
            if (isset($values[$matches[1]])) {
                return $values[$matches[1]];
            }
            return $matches[0];
        },
        $template);
}

Answer 1

Escaping the RegExp

You should never do manual escaping, if an appropriate function exists. For RegExp, this is preg_quote. Another risk is that you'r escaping before splitting, so you might destroy an escaping sequence. Better do

function substitute($template, $values, $delimiter = '{??}')
{
    $half    = (int) ( (strlen($delimiter) / 2) );
    $begin   = preg_quote(substr($delimiter, 0, $half), '/');
    $end     = preg_quote(substr($delimiter, $half), '/');
    $pattern = '/' . $begin . '(\w+)' . $end . '/';

Escaping the Values

The user provided values maust be escaped according to the context, where they are used. In this case it is HTML, so you must suppress unwanted tags, especially scripting tags. You can use strip_tags() or htmlspecialchars() for that purpose. I prefer the latter, because the user's value is not changed.

    return preg_replace_callback(
        $pattern,
        function ($matches) use ($values) {
            if (isset($values[$matches[1]])) {
                return htmlspecialchars($values[$matches[1]]);
            }
            return $matches[0];
        },
        $template);
    }
}

Conclusion

With these two small changes you have done everything to make that function save.