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

With MySQL use cast(okey as unsigned) instead of cast(okey as integer) #632

Merged
merged 1 commit into from
Sep 6, 2019

Conversation

erAck
Copy link
Contributor

@erAck erAck commented Sep 4, 2019

The autologin ("Save information") functionality in 2.3.1 is broken since

commit 52a41b37d554da11acc932eeec44c5fb1414a492
CommitDate: Fri Mar 23 18:01:32 2018 +0100

Rework autologin to use a token approach

Although a cookie serendipity[author_autologintoken] with correct
expiration (one month) which random data content is present as value
in the serendipity_options table with name autologin_Username and
correct timestamp as okey and that is found with manually executing
the SQL statement

SELECT name, value, okey FROM serendipity_options WHERE name = 'autologin_Username' AND okey > 1565801743 LIMIT 1

like done in include/functions_config.inc.php
serendipity_checkAutologin(), the login is forgotten after 30 minutes
or so. That was not the case with 2.1.5 where the login was valid for
weeks.

Of

if (stristr($serendipity['dbType'], 'sqlite')) {
    $cast = "okey";
} else {
    // Adds explicits casting for mysql, postgresql and others.
    $cast = "cast(okey as integer)";
}

from which $cast then is used in the SQL statement instead of a plain
okey; when doing that manually with

SELECT name, value, okey FROM serendipity_options WHERE name = 'autologin_Username' AND cast(okey as integer) > 1565801743 LIMIT 1

it produces the MySQL error

#1064 - You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'integer) > 1565801743 LIMIT 1' at line 1

This also with $serendipity['dbType'] = 'mysqli' for the above code.

Indeed, cast(okey as integer) is invalid in MySQL and should be
cast(okey as unsigned) instead which then also works manually, see
https://stackoverflow.com/a/12127022 and
https://dev.mysql.com/doc/refman/5.7/en/cast-functions.html#function_cast

Same in serendipity_issueAutologin().

Changing those two places accordingly resolves the autologin not
persistent problem.

Additionally, inspecting the serendipity_options table revealed loads
of old serendipity[author_authorinformation] cookie information that
was never deleted in serendipity_issueAutologin() with the

OR (okey LIKE 'l_%' AND $cast < " . (time() - 1814400) . ")")

expression producing a MySQL error. This has to be done manually
once as also 2.3.1 will not delete it anymore.

The autologin ("Save information") functionality in 2.3.1 is broken since

    commit 52a41b3
    CommitDate: Fri Mar 23 18:01:32 2018 +0100

	Rework autologin to use a token approach

Although a cookie serendipity[author_autologintoken] with correct
expiration (one month) which random data content is present as value
in the serendipity_options table with name autologin_Username and
correct timestamp as okey and that is found with manually executing
the SQL statement

  SELECT name, value, okey FROM serendipity_options WHERE name = 'autologin_Username' AND okey > 1565801743 LIMIT 1

like done in include/functions_config.inc.php
serendipity_checkAutologin(), the login is forgotten after 30 minutes
or so. That was not the case with 2.1.5 where the login was valid for
weeks.

Of

    if (stristr($serendipity['dbType'], 'sqlite')) {
        $cast = "okey";
    } else {
        // Adds explicits casting for mysql, postgresql and others.
        $cast = "cast(okey as integer)";
    }

from which $cast then is used in the SQL statement instead of a plain
okey; when doing that manually with

  SELECT name, value, okey FROM serendipity_options WHERE name = 'autologin_Username' AND cast(okey as integer) > 1565801743 LIMIT 1

it produces the MySQL error

  #1064 - You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'integer) > 1565801743 LIMIT 1' at line 1

This also with $serendipity['dbType'] = 'mysqli' for the above code.

Indeed, cast(okey as integer) is invalid in MySQL and should be
cast(okey as unsigned) instead which then also works manually, see
https://stackoverflow.com/a/12127022 and
https://dev.mysql.com/doc/refman/5.7/en/cast-functions.html#function_cast

Same in serendipity_issueAutologin().

Changing those two places accordingly resolves the autologin not
persistent problem.

Additionally, inspecting the serendipity_options table revealed loads
of old serendipity[author_authorinformation] cookie information that
was never deleted in serendipity_issueAutologin() with the

  OR (okey LIKE 'l_%' AND $cast < " . (time() - 1814400) . ")")

expression producing a MySQL error. This has to be done manually
once as also 2.3.1 will not delete it anymore.
@erAck
Copy link
Contributor Author

erAck commented Sep 4, 2019

This commit should also be cherry-picked to the 2.3 branch.

@onli
Copy link
Member

onli commented Sep 5, 2019

Hi. First, thanks for looking into this! I'm very happy to know that more people looked into this functionality.

How sure are you about this? If the integer is wrong, it would have died before. 52a41b3 did not introduce the code, it only changed the column that is used from name to okey. See 52a41b3#diff-f3383c67d4cb22067df9aa4d87697a23L437.

I did not test this with MySQL, iirc - my dev blog uses sqlite. So it's absolutely possible this code is wrong. But it's not a good start for this fix if the diagnosis is wrong. Am I missing something here?

@th-h th-h added backport needed Fix that has to be backported to older release branches. bugs labels Sep 5, 2019
@th-h th-h added this to the 2.3.2 milestone Sep 5, 2019
@erAck
Copy link
Contributor Author

erAck commented Sep 5, 2019

How sure are you about this?

Quite sure, as my blog is now running successfully preserving the autologin since the change ;-)

If the integer is wrong, it would have died before.

Only the cast(okey as integer) is wrong and apparently the MySQL error is silent in this case. Either swallowed by PHP, or munged by the logout / forced login following that. However, the old (pre-2.3) code using the same wrong cast for the l_% cookie data must had silently survived as the autologin worked, but never deleted any of those records.

52a41b3 did not introduce the code, it only changed the column that is used from name to okey.

No, the commit also introduced the code at line 460 then with the cast at line 464.

See 52a41b3#diff-f3383c67d4cb22067df9aa4d87697a23L437.

That's exactly what I'm looking at now.

I did not test this with MySQL, iirc - my dev blog uses sqlite. So it's absolutely possible this code is wrong. But it's not a good start for this fix if the diagnosis is wrong. Am I missing something here?

Looking at a different commit diff? I don't know.

@onli
Copy link
Member

onli commented Sep 6, 2019

No, the commit also introduced the code at line 460 then with the cast at line 464.

Okay. My point was that it just copied the cast from line 443 :)

@onli onli merged commit 07a2417 into s9y:master Sep 6, 2019
onli added a commit that referenced this pull request Sep 6, 2019
@erAck erAck deleted the mysql-cast branch September 6, 2019 15:56
robelix pushed a commit to robelix/Serendipity that referenced this pull request Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport needed Fix that has to be backported to older release branches. bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants