View Issue Details

IDProjectCategoryView StatusLast Update
0026798mantisbtadministrationpublic2020-03-28 12:57
Reporteratrol Assigned Todregad  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Target Version2.25.0Fixed in Version2.25.0 
Summary0026798: PHP warning in config_get_global
Description

In some cases config_get_global gives ERROR_CONFIG_OPT_NOT_FOUND warning

PHP Warning:  100 in .../core/config_api.php on line 192

This is WAD at the moment and happens for any string configuration option where function config_eval is not able to reference a variable.

/**
 * check for recursion in defining configuration variables
 * If there is a %text% in the returned value, re-evaluate the "text" part and replace the string
 *
 * @param string  $p_value  Configuration variable to evaluate.
 * @param boolean $p_global If true, gets %text% as a global configuration, defaults to false.
 * @return string
 */
function config_eval( $p_value, $p_global = false ) {

In typical use cases there is no problem, but the approach itself is not clean and can lead to real life issues, e.g this post in forum

Most of the time there is just the warning without any visible side effect, e.g. set

$g_crypto_master_salt     = '%1%C*&^$#Gand$%^$#%@UOOMm60hvxe8AngM=';

but there are also strange restrictions because of this, e.g. you can't set something like

$g_db_password            = '%1%myPasswordThatCantainsTwoDollars'
TagsNo tags attached.

Relationships

related to 0021604 new Discuss recursive configuration options 

Activities

atrol

atrol

2020-03-19 12:28

developer   ~0063778

Last edited: 2020-03-19 12:29

View 2 revisions

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

dregad

dregad

2020-03-21 10:37

developer   ~0063782

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
dregad

dregad

2020-03-21 11:21

developer   ~0063783

Last edited: 2020-03-21 11:28

View 2 revisions

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...

dregad

dregad

2020-03-21 13:09

developer   ~0063784

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.

atrol

atrol

2020-03-21 20:30

developer   ~0063785

Last edited: 2020-03-26 09:19

View 3 revisions

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

Related Changesets

MantisBT: master 7fffd663

2020-03-21 13:02:01

dregad

Details Diff
Fix warning in config_get_global()

When config_eval() references an unknown config option, the code throws
an ERROR_CONFIG_OPT_NOT_FOUND warning.

To avoid this unwanted behavior, we now check that the config option to
replace actually exists before calling config_get()/config_get_global().
If not, the variable will be left as-is in the returned value.

Fixes 0026798
Affected Issues
0026798
mod - core/config_api.php Diff File

MantisBT: master c2d6c3dd

2020-03-28 12:51:37

dregad

Details Diff
Fix warning and code cleanup in config_eval()

Issue 0026798
PR https://github.com/mantisbt/mantisbt/pull/1635
Affected Issues
0026798
mod - core/config_api.php Diff File

Issue History

Date Modified Username Field Change
2020-03-19 12:26 atrol New Issue
2020-03-19 12:28 atrol Description Updated View Revisions
2020-03-19 12:28 atrol Note Added: 0063778
2020-03-19 12:28 atrol Relationship added related to 0021604
2020-03-19 12:29 atrol Note Edited: 0063778 View Revisions
2020-03-19 12:34 atrol Description Updated View Revisions
2020-03-21 10:37 dregad Note Added: 0063782
2020-03-21 11:21 dregad Note Added: 0063783
2020-03-21 11:21 dregad Status new => confirmed
2020-03-21 11:28 dregad Note Edited: 0063783 View Revisions
2020-03-21 13:09 dregad Assigned To => dregad
2020-03-21 13:09 dregad Status confirmed => assigned
2020-03-21 13:09 dregad Note Added: 0063784
2020-03-21 20:30 atrol Note Added: 0063785
2020-03-21 20:31 atrol Target Version => 2.25.0
2020-03-23 17:24 atrol Note Edited: 0063785 View Revisions
2020-03-26 09:19 atrol Note Edited: 0063785 View Revisions
2020-03-28 12:57 dregad Changeset attached => MantisBT master 7fffd663
2020-03-28 12:57 dregad Changeset attached => MantisBT master c2d6c3dd
2020-03-28 12:57 dregad Status assigned => resolved
2020-03-28 12:57 dregad Resolution open => fixed
2020-03-28 12:57 dregad Fixed in Version => 2.25.0