Bug 13779 - sessionID declared twice in C4::Auth::checkauth()
Summary: sessionID declared twice in C4::Auth::checkauth()
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-03 06:09 UTC by David Cook
Modified: 2018-09-24 09:40 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13779 - sessionID declared twice in C4::Auth::checkauth() (1.33 KB, patch)
2018-07-19 07:01 UTC, David Cook
Details | Diff | Splinter Review
Bug 13779: sessionID declared twice in C4::Auth::checkauth() (1.96 KB, patch)
2018-09-06 00:16 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 13779: sessionID declared twice in C4::Auth::checkauth() (2.63 KB, patch)
2018-09-07 15:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13779: (QA follow-up) Correct checkauth call in offline_circ/download.pl (1.20 KB, patch)
2018-09-07 15:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2015-03-03 06:09:26 UTC
"$sessionID" is declared twice in C4::Auth::checkauth().

The first time is at the start of C4::Auth::checkauth(), while the second is in the "unless ($userid || $sessionID)" block. 

This means that "return ( $userid, $cookie, $sessionID, $flags )", which is called within the "if ( $loggedin || $authnotrequired )" block, will never ever return the $sessionID created in the "unless ($userid || $sessionID)" block, since it's out of scope.

The only time that "return ( $userid, $cookie, $sessionID, $flags )" will return "$sessionID" is if the "elsif ( $sessionID = $query->cookie("CGISESSID") )" block evaluates to true.

--

In other words, "my ( $userid, $cookie, $sessionID ) = checkauth( $query, 0,  { borrow => 1 }, 'opac', '');" will only have a $sessionID if the user already has the CGISESSID cookie set.

This doesn't prevent checkauth() from setting the session or the cookie properly. It just means $sessionID isn't set even though it should be, because it's the value that is stored in the cookie!
Comment 1 David Cook 2018-07-19 06:55:59 UTC
This is still an issue. I'm tempted to write a patch for it, but I fear that it'll be too scary for people to test since it's in C4::Auth.

At the moment, it doesn't really affect anyone (except for me with code which isn't in Koha). We can verify this by running the following in the root git directory:

grep -l -R "checkauth" --exclude="*.t" --exclude="*.pm" | xargs -n1 grep "sessionID"

For the most part, nothing gets done with sessionID. The one exception seems to be opac/external/overdrive/auth.pl but it doesn't actually require auth so it doesn't go down the bad path in C4::Auth.
Comment 2 David Cook 2018-07-19 07:01:03 UTC
Created attachment 77099 [details] [review]
Bug 13779 - sessionID declared twice in C4::Auth::checkauth()

The bug is that $sessionID is declared twice in C4::Auth::checkauth().
At the moment, this doesn't actually create a problem, because no
one seems to be using the $sessionID which is returned by checkauth(),
except in the case of opac/external/overdrive/auth.pl which skips
the second declartion as it doesn't require auth.

This patch removes the redefining of the $sessionID variable.

In terms of testing, try logging in with a username and password
and see if it works. The only risk this patch would pose is breaking
auth I would think, since nothing is actually using the return value
from checkauth() for $sessionID.
Comment 3 M. Tompsett 2018-09-06 00:16:13 UTC
Created attachment 78452 [details] [review]
Bug 13779: sessionID declared twice in C4::Auth::checkauth()

The bug is that $sessionID is declared twice in C4::Auth::checkauth().
At the moment, this doesn't actually create a problem, because no
one seems to be using the $sessionID which is returned by checkauth(),
except in the case of opac/external/overdrive/auth.pl which skips
the second declartion as it doesn't require auth.

This patch removes the redefining of the $sessionID variable.

In terms of testing, try logging in with a username and password
and see if it works. The only risk this patch would pose is breaking
auth I would think, since nothing is actually using the return value
from checkauth() for $sessionID.

NOTE:
It was initially defined near the top of the function (~line 791).
I believe the scoping would mean the correct version of $sessionID
would be used in the latter lines for the unset'ing.
I have skimmed code to see if the sessionID return value is used.
I did not test overdrive, as I do not know how. However, this is
the only area, I think this could possibly break. This change makes
sense to me.

QA: Please test overdrive.
opac/external/overdrive/auth.pl only checks if the value is set, so
   this patch might fix/break something there?
opac/svc/overdrive same kind of check.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 4 Marcel de Rooy 2018-09-07 06:31:44 UTC
Please use the assignee field, David. Thx.
Comment 5 Marcel de Rooy 2018-09-07 06:36:39 UTC
QA: Looking here
Comment 6 Marcel de Rooy 2018-09-07 07:42:31 UTC
This looks good to me so far, but I will finish it just later today or Monday.
Comment 7 Marcel de Rooy 2018-09-07 15:02:57 UTC
Created attachment 78527 [details] [review]
Bug 13779: sessionID declared twice in C4::Auth::checkauth()

The bug is that $sessionID is declared twice in C4::Auth::checkauth().
At the moment, this doesn't actually create a problem, because no
one seems to be using the $sessionID which is returned by checkauth(),
except in the case of opac/external/overdrive/auth.pl which skips
the second declartion as it doesn't require auth.

This patch removes the redefining of the $sessionID variable.

In terms of testing, try logging in with a username and password
and see if it works. The only risk this patch would pose is breaking
auth I would think, since nothing is actually using the return value
from checkauth() for $sessionID.

NOTE:
It was initially defined near the top of the function (~line 791).
I believe the scoping would mean the correct version of $sessionID
would be used in the latter lines for the unset'ing.
I have skimmed code to see if the sessionID return value is used.
I did not test overdrive, as I do not know how. However, this is
the only area, I think this could possibly break. This change makes
sense to me.

QA: Please test overdrive.
opac/external/overdrive/auth.pl only checks if the value is set, so
   this patch might fix/break something there?
opac/svc/overdrive same kind of check.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
This is a (tiny) code improvement. Now we consistently return the session
id that is also stored in the returned cookie. (Which can be an 'anon'
session.)
Fact is that Koha almost everywhere ignores the returned session id and
sometimes gets the session from the cookie (obviously). The session id is
also passed to the template by get_template_and_user but never used in
templates.
As mentioned, the two overdrive scripts are the exception. But since both
test on both $user && $sessionID, they will not choke on an anynonomous
session id without userid. So theoretically fine, but not tested.
Comment 8 Marcel de Rooy 2018-09-07 15:03:02 UTC
Created attachment 78528 [details] [review]
Bug 13779: (QA follow-up) Correct checkauth call in offline_circ/download.pl

Script offline_circ/download.pl does not use the return values of
checkauth. So should not assign them to bad chosen variables that do
not correspond with checkauth but with get_template_and_user.

Test plan:
Look for $template, $loggedinuser, $cookie, $flags in the code.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Nick Clemens 2018-09-14 19:57:27 UTC
Awesome work all!

Pushed to master for 18.11
Comment 10 Martin Renvoize 2018-09-17 13:27:48 UTC
Pushed to 18.05.x for 18.05.04
Comment 11 Fridolin SOMERS 2018-09-24 09:40:45 UTC
Pushed to 17.11.x for 17.11.10