Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Upgrades, PHP 7.3.2 #436

Closed
deklar opened this Issue Mar 18, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@deklar
Copy link

commented Mar 18, 2019

Apologies for lack of testing recently - truth is facileManager & fmDNS have been running so smoothly, I didn't want to upgrade it at all.

But, I got around to it.

Some minor issues and mostly FYI - in case these come up for others:

fM Version : 3.4
{Module Name} Version : fmdns-3.3.1

Couple of things - all of which I corrected. I believe most of these are changes with PHP 7.3.x.

This was an upgrade from facilemanager-core-3.1.1 to facilemanager-core-3.4 as well as fmDNS 3.2.1 to fmDNS 3.3.1. PHP was also upgraded from 5.6.x to 7.3.2 at the same time.

Upgrading - issue with fm-modules/facileManager/upgrade.php and fm-modules/fmDNS/upgrade.php: Several extend an array ($inserts) with "<<<INSERT. The next line in the feed is 'INSERT ' - PHP is seeing that INSERT as a match and ending the EOL. fix: Just rename INSERT to INSERTSQL or something else.

Misuse of 'continue' statement in fm-modules/shared/classes/class_servers.php (line 117 I think it was) - PHP reports: Did you mean to use "continue 2"? -- but "continue 2" is not possible in that switch statement, requires a break instead.

fm-modules/facileManager/classes/class_users.php - around line 356-357, "if (!$fmdb->last_result || $fmdb->sql_errors) " - the last_result is empty on the first SQL sequence (which sets user_group to 0 and empties user_caps for everyone assigned to a group). That prevents the second update to run (setting up the groups). I removed the last_result check and it seems to work fine now.

Only PHP errors being generated with this setup right now are in fm-includes/init.php: PHP Warning: dns_get_record(): DNS Query failed on line 149 (previous version did this, doesn't seem to affect anything?) .. this is appearing even without logging in (just hitting the page).

I think that about covers it for the server side. The client side had no issues during the upgrade. I'll continue testing ... but so far it looks intact and functioning.

Other items you may want to know about - very minor items that may not be worth pointing out - but there appears to be a potential security issue with "user management" - if someone is not a super admin, but is part of the "user management", they can create a super admin account (or even a super admin group). They also can apparently turn off super users - even though they are not one themselves.

Another minor item I haven't looked into but just noticed ... if user is added with limited capabilities in fmDNS (say, Build Server configs, zone management, record management and reload zones) - with "all zones" listed for "limit access to the following zones" or with all the specific zones listed (including the "all zones" option) - the center top option to display only specific zones is missing. As a super admin, that option is shown.

@WillyXJ

This comment has been minimized.

Copy link
Owner

commented Mar 18, 2019

Thanks for the report @deklar. I have PHP 5.6.x and 7.2.x development/test environments. I'll have to get a 7.3.x environment now and fixed these errors you've reported.

WillyXJ pushed a commit that referenced this issue Mar 18, 2019

WillyXJ
Core - #436 - Fixed user security issue
Users without the super-admin privilege can no longer create or edit super-admin user or group privileges.

WillyXJ pushed a commit that referenced this issue Mar 19, 2019

WillyXJ pushed a commit that referenced this issue Mar 19, 2019

WillyXJ pushed a commit that referenced this issue Mar 19, 2019

@WillyXJ

This comment has been minimized.

Copy link
Owner

commented Mar 21, 2019

These are all now fixed in Core 3.4.1 and fmDNS 3.3.2 and later.

@WillyXJ WillyXJ closed this Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.