Options to fix

• Remove config_eval, see discussion in 0021604 and my former try
• Introduce new parameter $p_eval = true in functions config_get and config_get_global and set it to false for some options
• Introduce new functions config_get_no_eval and config_get_global_no_eval and use it for some options
• Introduce new functions config_get_no_eval and access the global variable instead of calling config_get_global for some options
  ...

Comments welcome

Not sure if you are aware that since 1.2.0rc1, as a workaround \ can be used to escape the % in configs, so it does not get interpreted by config_eval(), e.g.

php > $g_db_password            = '\%1\%myPasswordThatCantainsTwoDollars'
php > echo config_eval($g_db_password,true);
%1%myPasswordThatCantainsTwoDollars

While it is true that there are a few config options such as db_password and crypto_master_salt that should probably never be eval'ed, I'm wondering whether it's worth the effort to implement new, specific API functions.

A simple check in config_eval(), making sure that the matched value is an existing global variable would probably do the trick (except for an improbable scenario where $g_db_password = 'Password%some_existing_config%';, in that case user should escape the % as explained in 0026798:0063782).

An alternative would be to define a global variable, whitelisting all the configs that should never be eval'ed, similar to $g_public_config_names, e.g.

$g_no_eval_config_names = array( 'db_password', 'crypto_master_salt', ... ); 

Then a simple in_array() lookup in config_eval() could skip the preg_match and replacement loop for those configs.

Of course we could also consider reversing the logic, i.e. implement this array as a list of configs where eval is allowed; I guess this would be closer to your intention of removing config_eval() (0021604), which I'm not convinced is something we should do.

EDIT: submitted the note by mistake, before I was done typing...

PR https://github.com/mantisbt/mantisbt/pull/1635 implements the simple sanity check I mentioned in 0026798:0063783, which resolves the issue described here.

@atrol let me know if you feel it's worth pursuing the whitelist config option alternative, in that case it should probably be tracked separately.

Not sure if you are aware that since 1.2.0rc1, as a workaround \ can be used to escape the % in configs

Thanks for the hint, I wasn't aware of that

A simple check in config_eval(), making sure that the matched value is an existing global variable would probably do the trick ...

Sounds good as it fixes > 99,99% of real use cases, but complicates the rule when you have to use the escape workaround and when it works without escaping.
In a very improbable case an option might work in version x, but will no longer work in version x+1 as a new global variable has been introduced.
I am aware that I am a nitpicker, but I like clean and simple code ;-)

let me know if you feel it's worth pursuing the whitelist config option alternative

Not as an alternative, but an additional functionality.
After your changes this would just be needed for very improbable cases, so there is no priority for that.

removing config_eval() (0021604), which I'm not convinced is something we should do.

I still think that removing wouldn't be that bad.
Independent from the current issue and 0021604 there are other reasons,
e.g. our users are hardly aware that they change $g_update_bug_assign_threshold if they change $g_handle_bug_threshold in config_inc.php.
It's even more complicated if there is a combination of this setting in config_inc.php with changes via manage_config_work_threshold_page