Fixed wrong sql cause reseller cannot shadow to there client.

[llun]

My clients told me they can't shadow to there sub-client and I saw this error when they done that.

[Sun Dec 29 19:12:47 2013] [error] [client 168.63.240.195] PHP Notice:  Undefined index: ac_id_pk in /etc/zpanel/panel/modules/shadowing/code/controller.ext.php on line 98, referer: http://***/?module=shadowing

I changed the sql which solved the issue but got another two errors.

[Sun Dec 29 19:26:24 2013] [error] [client 168.63.240.195] PHP Warning:  Missing argument 2 for ctrl_auth::SetUserSession(), called in /etc/zpanel/panel/modules/shadowing/code/controller.ext.php on line 102 and defined in /etc/zpanel/panel/dryden/ctrl/auth.class.php on line 57, referer: http://***/?module=shadowing
[Sun Dec 29 19:26:24 2013] [error] [client 168.63.240.195] PHP Notice:  Undefined variable: sessionSecuirty in /etc/zpanel/panel/dryden/ctrl/auth.class.php on line 61, referer: http://***/?module=shadowing

Not sure is it cause another problem but shadow is working fine on my server now.

[llun]

I founded the function that cause error but don't know what's the second parameter for? Should I pass it true for making it more secure?

[motters]

The second value is whether to enable session security for that particular session (https://github.com/bobsta63/zpanelx/blob/master/dryden/runtime/sessionsecurity.class.php).

Set to true will stop sessions from being able to be hi-jacked (Encase of an unknown xss that has been exploited).

But some times if the network your on has a fluctuating ip address it can cause the anti session hi-jacking to think the session is being exploited.

What would be best would be to run an if statement and if the admin has anti session hi-jacking enabled then set it to true is not then set it to false. EG change line 101 (ctrl_auth::SetUserSession($rowclients['ac_id_pk']);) to the below

if( runtime_sessionsecurity::getSessionSecurityEnabled ){
    ctrl_auth::SetUserSession($rowclients['ac_id_pk'], true); 
}else{
    ctrl_auth::SetUserSession($rowclients['ac_id_pk'], false);
}

If you could give that a go that would be awesome mate :) I'm amazed this has not been picked up tbh

[5050]

Oh !

if( runtime_sessionsecurity::getSessionSecurityEnabled ){
    ctrl_auth::SetUserSession($rowclients['ac_id_pk'], true); 
}else{
    ctrl_auth::SetUserSession($rowclients['ac_id_pk'], false);
}

Can be simplifyed as :

ctrl_auth::SetUserSession($rowclients['ac_id_pk'], runtime_sessionsecurity::getSessionSecurityEnabled)

Small-er is beautifull-er ! :-p

(In fact, I am a dangerous maniac who hate structures like

if (condition)
then 
  xxxxx true;
else 
  xxxxx false;

that duplicates uselessly the state of "condition" and are a lot of better exprimed directely as

 xxxxx (condition);

lol)

[llun]

@5050 Yes I have done that in the commit.

But from this code Can I remove the second argument and check the session security flag in this function and remove all flag that pass to it (Actually I don't see other functions pass the second argument to it except what is in my commit)

[5050]

WARNING : BUG !!! Thanks, liun, you opened a bug box !

The declaration:

static function SetUserSession($zpuid = 0, $sessionSecuirty)

Is wrong. Parameters that have default values ($zpuid here) MUST be placed AFTER parameters without default value, else the result wilt be wrong. (php manual : http://www.php.net/manual/en/functions.arguments.php#functions.arguments.default)

Solution:

• add a default value also for "$sessionSecuirty" (an rename it to sessionSecurity ! :-p) or:
• remove default value for $zpuid and check (and correct) every call.

[allebb]

Well spotted 5050! - Revoking the commit... My bad really as I should have tested before merging but I was doing it from work (in a rush!)

I'll revert the commit now!

Kudo's to 5050! - Well spotted sir!

On 8 January 2014 14:20, 5050 notifications@github.com wrote:

WARNING : BUG !!! Thnks, liun, you opened a bug box !

The declaration:

static function SetUserSession($zpuid = 0, $sessionSecuirty)

Is wrong. Parameters that have default values ($zpuid here) MUST be placed AFTER parameters without default value, else the result wilt be wrong. (php manual : http://www.php.net/manual/en/functions.arguments.php#functions.arguments.default)

— Reply to this email directly or view it on GitHubhttps://github.com/zpanel/zpanelx/pull/90#issuecomment-31834932 .

[5050]

It also applies to

 static function Authenticate($username, $password, $rememberme = false, $iscookie = false, $sessionSecuirty)

in the same class

[allebb]

It appears to have made its way in on this commit:- https://github.com/zpanel/zpanelx/commit/c961ca7b466389dab95df3db4482bbe30528000e

I can only assume that until now both method parameters have been supplied and therefore would not have flagged this previously.

Cheers, Bobby

On 8 January 2014 14:30, 5050 notifications@github.com wrote:

It also applies to

static function Authenticate($username, $password, $rememberme = false, $iscookie = false, $sessionSecuirty)

in the same class

— Reply to this email directly or view it on GitHubhttps://github.com/zpanel/zpanelx/pull/90#issuecomment-31836047 .

[allebb]

I'll review Motter's code and re-factor when I get a chance this evening.

On 8 January 2014 14:37, Bobby Allen bobbyallen.uk@gmail.com wrote:

It appears to have made its way in on this commit:-

https://github.com/zpanel/zpanelx/commit/c961ca7b466389dab95df3db4482bbe30528000e

I can only assume that until now both method parameters have been supplied and therefore would not have flagged this previously.

Cheers, Bobby

On 8 January 2014 14:30, 5050 notifications@github.com wrote:

It also applies to

static function Authenticate($username, $password, $rememberme = false, $iscookie = false, $sessionSecuirty)

in the same class

— Reply to this email directly or view it on GitHubhttps://github.com/zpanel/zpanelx/pull/90#issuecomment-31836047 .

[motters]

Areee shit, the amount of testing i did on the sessions security as well :S

TBH i say we just set the sessionSecurity pram default to false. And that should pretty much solve the issue :)

[allebb]

Yeah bud, that is what I was planning on doing anyways, seems a simpler fix than going back through all the code :) - Do you think default it to 'false' though... I personally think we should prob default it to 'true' unless you know a reason why this is not a better idea? - I'll wait to hear abck from you budd :)

On 8 January 2014 21:39, motters notifications@github.com wrote:

Areee shit, the amount of testing i did on the sessions security as well :S

TBH i say we just set the sessionSecurity pram default to false. And that should pretty much solve the issue :)

— Reply to this email directly or view it on GitHubhttps://github.com/zpanel/zpanelx/pull/90#issuecomment-31880704 .

[motters]

The only reason I said false is that if some one is at a place of work like yours that has a fluctuating outbound ip address then the session security can think that the session has been comprised and will terminate the session.

That is why on the login page the user can turn sessions security off.

But thinking about it mate we can do something simple like the below to the methods that have the problem

static function SetUserSession($zpuid = 0, $sessionSecuirty = true){
         $sessionSecuirty = runtime_sessionsecurity::getSessionSecurityEnabled; 
         // Other method code
}

So we just set the variable to true or false and then we reset the variable at the start of the method if you get what I mean :) That way the session security variable will always be correctly set dependant on the users selection :)

[allebb]

Yeah, although it is not the perfect solution I think that rather than re-factor for this release, we'll implement as you've suggested above buddy.

The more stuff we add to ZPanelX the more I hate my original implementation of the framework and code lol, Stormwind is going to have much better separation of concerns etc :)

I do feel that although users may experience issues when on outbound connections it is more important to choose security first (as a default :))

I'll push the change now :)

Cheers guys, Bobby

[allebb]

Pushed to GitHub in: https://github.com/zpanel/zpanelx/commit/df22cecb57498c8a55da4f5fe91f3e60deffea79

[motters]

I agree mate its not perfect but would be a major fag to re-factor like you said.

haha ZPX is awesome mate just with stormwind it be a lot more decoupled like you said :p

True mate, true lol At least with is fixed now :D