Minor bug in Login2()

[snoopy_virtual]

There is something I don't understand in the file LogInOut.php

Find:

Code Select Expand


elseif (!empty($_POST['cookielength']) && ($_POST['cookielength'] >= 1 || $_POST['cookielength'] <= 525600))
$modSettings['cookieTime'] = (int) $_POST['cookielength'];


Anybody can explain to me why is there an OR instead of an AND?

I mean, shouldn't it be this instead:

Code Select Expand


elseif (!empty($_POST['cookielength']) && $_POST['cookielength'] >= 1 && $_POST['cookielength'] <= 525600)
$modSettings['cookieTime'] = (int) $_POST['cookielength'];


It looks like a mistake to me.

[Kindred]

no... I think that or is correct in this code...

[JBlaze]

Code Select Expand

elseif (!empty($_POST['cookielength']) && ($_POST['cookielength'] >= 1 || $_POST['cookielength'] <= 525600))
$modSettings['cookieTime'] = (int) $_POST['cookielength'];

That statement will always be true, so long as $_POST['cookielength'] is set. The way it reads now, all I can see is

Code Select Expand

elseif (!empty($_POST['cookielength']))
$modSettings['cookieTime'] = (int) $_POST['cookielength'];

I think OP has a point, and this needs to be looked at.

[Oldiesmann]

It's definitely weird...

Code Select Expand

if (isset($_POST['cookieneverexp']) || (!empty($_POST['cookielength']) && $_POST['cookielength'] == -1))
$modSettings['cookieTime'] = 3153600;
elseif (!empty($_POST['cookielength']) && ($_POST['cookielength'] >= 1 || $_POST['cookielength'] <= 525600))
$modSettings['cookieTime'] = (int) $_POST['cookielength']

I'm not entirely sure what we're trying to accomplish here. If we tell it to keep us logged in permanently or $_POST['cookielength'] is set to -1, we set the cookie time to 3153600 (36.5 days or 1/10th of a year). Otherwise, as long as it's set and not less than 1, any value will be accepted. I'll have to dig into this a bit further and see if I can figure it out before making any code changes.

[snoopy_virtual]

... we set the cookie time to 3153600 (36.5 days or 1/10th of a year). ...

Not really.

$modSettings['cookieTime'] and $_POST['cookielength'] is the time we want to stay logged in minutes, not seconds.

[live627]

I mean, shouldn't it be this instead:

Code Select Expand


elseif (!empty($_POST['cookielength']) && $_POST['cookielength'] >= 1 && $_POST['cookielength'] <= 525600)
$modSettings['cookieTime'] = (int) $_POST['cookielength'];


It looks like a mistake to me.

Quite possible. It looks to me like it's supposed to check a range.

[JBlaze]

Tracked: https://github.com/SimpleMachines/SMF2.1/issues/2969

[live627]

Thanks.

[Ninja ZX-10RR]

What about 2.0.11, if any? Can this be investigated and included?

[Kindred]

Mayne...  we will see when 2,0,11 is even considered

[nend]

It's definitely weird...

Code Select Expand

if (isset($_POST['cookieneverexp']) || (!empty($_POST['cookielength']) && $_POST['cookielength'] == -1))
$modSettings['cookieTime'] = 3153600;
elseif (!empty($_POST['cookielength']) && ($_POST['cookielength'] >= 1 || $_POST['cookielength'] <= 525600))
$modSettings['cookieTime'] = (int) $_POST['cookielength']

I'm not entirely sure what we're trying to accomplish here. If we tell it to keep us logged in permanently or $_POST['cookielength'] is set to -1, we set the cookie time to 3153600 (36.5 days or 1/10th of a year). Otherwise, as long as it's set and not less than 1, any value will be accepted. I'll have to dig into this a bit further and see if I can figure it out before making any code changes.

Actually a value of 0 would be acceptable. Just break the or into two ifs. The encapsulation with the or means only one has to be true. Should be a and in there unless it is always wanted to return true, I doubt.

Code Select Expand


if (!empty($_POST['cookielength']) && $_POST['cookielength'] >= 1)
$modSettings['cookieTime'] = (int) $_POST['cookielength'];

if (!empty($_POST['cookielength']) && $_POST['cookielength'] <= 525600)
$modSettings['cookieTime'] = (int) $_POST['cookielength'];


[live627]

Wouldn't thew cookie be unset if it's length be zero?

Also, I addressed this in https://github.com/SimpleMachines/SMF2.1/commit/d40e65dff125e40b3c95b20d479e8d926854903b

[nend]

I don't see why SMF even gives the user the option to enter a value at login.

I haven't visited a site that allows configuration of the cookie length by time. Most sites give, browsing from public or private computer, options which set the lenght accordingly.

Either way is fine though.

[Kindred]

and what is "the length accordingly"?

If I am going to be on a public PC for an hour, I'll set my session length...  if I am going to be on for 2 hours, I will as well...

Would you force people to expire sessions in the middle of a browse just because you think they should only be online for 30 minutes at a time from a public computer?

I much prefer the SMF way

[nend]

and what is "the length accordingly"?

If I am going to be on a public PC for an hour, I'll set my session length...  if I am going to be on for 2 hours, I will as well...

Would you force people to expire sessions in the middle of a browse just because you think they should only be online for 30 minutes at a time from a public computer?

I much prefer the SMF way

I don't know what sites you must be visiting but the most common method is to renew the length with activity. Say the renewal is a lease, every time you do some sort of activity your lease is pushed back 10 minutes.

This way you don't have to be worried about being logged off after x amount of time, just as long as your active. Then when you leave the machine your cookie will expire.

[Kindred]

Meh... I prefer the fixed session length... Besides that pushing the cookie back is more code.

[live627]

and what is "the length accordingly"?

"remember me on this computer"

I much prefer the SMF way

ironyyyyy

Besides that pushing the cookie back is more code

I agree that one line of code is a lot of work (sarcasm)

#sessiongate

[nend]

more code.

More code!, Did you say, MORE CODE!
CODE, your speaking my language.
CODE, I can deal with.
CODE, easy as 123.
CODE, we can build it yes we can. ♪

More code isn't necessarily bad, it is how it is coded. However in this case we are talking about cookies and maybe at most a few line edits. I am not proposing it as a feature request,  meh what do I care, if I need it I can build it.

[JBlaze]

I prefer sessions only last until either the browser is closed, or a user clicks "logout". But still, to each their own.