Bug 27849 - Koha::Token may access undefined C4::Context->userenv
Summary: Koha::Token may access undefined C4::Context->userenv
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: MJ Ray (software.coop)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 34163
  Show dependency treegraph
 
Reported: 2021-03-03 11:51 UTC by MJ Ray (software.coop)
Modified: 2023-09-12 11:35 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00,21.11, 22.05.06


Attachments
[PATCH] Bug 27849: Koha::Token may access undefined C4::Context->userenv (1.67 KB, patch)
2021-03-04 16:12 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
Bug 27849: Koha::Token may access undefined C4::Context->userenv (1.77 KB, patch)
2022-07-18 12:46 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 27849: Koha::Token may access undefined C4::Context->userenv (1.86 KB, patch)
2022-08-01 08:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27849: (follow-up) Add test (3.06 KB, patch)
2022-08-01 08:12 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 MJ Ray (software.coop) 2021-03-03 11:51:48 UTC
The _add_default_csrf_params internal function accesses C4::Context->userenv without checking that it has been defined. I think not all of the potential callers of it declare that they require a defined userenv, so we should test and provide defaults for required values if it is not defined, to avoid some "Can't use an undefined value as a HASH reference" HTTP 500 Internal Server Errors.

Step to Reproduce: write some code that results in that function being used before a set_userenv call

Actual Result: error logged, code exits

Expected Result: code runs to completion

Additional Information: I am not sure whether this is currently triggered by any released core koha code, or only a few plugins and mods. It looks to me like a bug waiting to happen, based on the documentation, which can be avoided simply.
Comment 1 MJ Ray (software.coop) 2021-03-04 16:12:01 UTC
Created attachment 117744 [details] [review]
[PATCH] Bug 27849: Koha::Token may access undefined C4::Context->userenv
Comment 2 Victor Grousset/tuxayo 2021-05-02 03:57:04 UTC
Saluton :)

Ĉu estas bona maniero por testi?
Is that a good way to test:

Add the following code:
        Koha::Token->new->check_csrf({
            session_id => scalar $query->cookie('CGISESSID'),
            token  => scalar $query->param('csrf_token'),
        });

In opac/opac-user.pl

After
C4::Context->preference('casAuthentication') and C4::Auth_with_cas::logout_if_required($query);

And before
my ( $template, $borrowernumber, $cookie ) = get_template_and_user(

It seems to be the case since it crashes without the patch and work with it.
Comment 3 Paul Derscheid 2022-07-18 12:46:42 UTC
Created attachment 137802 [details] [review]
Bug 27849: Koha::Token may access undefined C4::Context->userenv

The _add_default_csrf_params internal function accesses
C4::Context->userenv without checking that it has been defined. I think
not all of the potential callers of it declare that they require a
defined userenv, so we should test and provide defaults for required
values if it is not defined, to avoid some "Can't use an undefined value
as a HASH reference" HTTP 500 Internal Server Errors.

To test:

Do anything that requires a form with CSRF token, such as editing your
details. Behaviour should be unchanged. To test the failure case, you
would need some customised code that indirectly generates a CSRF token
before setting the userenv up and I am not sure there is any in released
Koha yet.

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>

Looks good to me. Working as expected.
Comment 4 Marcel de Rooy 2022-07-29 08:38:31 UTC
QA: Looking here
Comment 5 Marcel de Rooy 2022-07-29 09:06:04 UTC
Will finish this Monday.
Comment 6 Marcel de Rooy 2022-08-01 08:12:05 UTC
Created attachment 138448 [details] [review]
Bug 27849: Koha::Token may access undefined C4::Context->userenv

The _add_default_csrf_params internal function accesses
C4::Context->userenv without checking that it has been defined. I think
not all of the potential callers of it declare that they require a
defined userenv, so we should test and provide defaults for required
values if it is not defined, to avoid some "Can't use an undefined value
as a HASH reference" HTTP 500 Internal Server Errors.

To test:

Do anything that requires a form with CSRF token, such as editing your
details. Behaviour should be unchanged. To test the failure case, you
would need some customised code that indirectly generates a CSRF token
before setting the userenv up and I am not sure there is any in released
Koha yet.

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>

Looks good to me. Working as expected.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2022-08-01 08:12:09 UTC
Created attachment 138449 [details] [review]
Bug 27849: (follow-up) Add test

Compacting the code a bit too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Tomás Cohen Arazi 2022-08-01 13:02:44 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 9 Lucas Gass 2022-09-29 15:49:06 UTC
Backported to 22.05.x for 22.05.06
Comment 10 Victor Grousset/tuxayo 2022-10-13 16:11:31 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.
Comment 11 Arthur Suzuki 2022-10-22 22:07:02 UTC
Thanks!

Pushed to 21.11 for 21.11.12