Bug 27849

Summary: Koha::Token may access undefined C4::Context->userenv
Product: Koha Reporter: MJ Ray (software.coop) <mjr>
Component: Architecture, internals, and plumbingAssignee: MJ Ray (software.coop) <mjr>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: lucas, m.de.rooy, victor
Version: Main   
Hardware: All   
OS: All   
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
Bug Depends on:    
Bug Blocks: 34163    
Attachments: [PATCH] Bug 27849: Koha::Token may access undefined C4::Context->userenv
Bug 27849: Koha::Token may access undefined C4::Context->userenv
Bug 27849: Koha::Token may access undefined C4::Context->userenv
Bug 27849: (follow-up) Add test

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