Bug 24719 - C4::Context::set_remote_address() prevents file upload for non-Plack Koha
Summary: C4::Context::set_remote_address() prevents file upload for non-Plack Koha
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-25 02:16 UTC by David Cook
Modified: 2020-04-09 19:52 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05


Attachments
Remove use of CGI in C4::Context::set_remote_address() (1.36 KB, patch)
2020-02-25 02:34 UTC, David Cook
Details | Diff | Splinter Review
Bug 24719: Remove use of CGI in C4::Context::set_remote_address() (1.38 KB, patch)
2020-02-25 16:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
[Alternate PATCH] Bug 24719: Remove use of CGI object in C4::Context::set_remote_address() (1.46 KB, patch)
2020-02-27 00:31 UTC, David Cook
Details | Diff | Splinter Review
Bug 24719: Remove use of CGI object in C4::Context::set_remote_address() (1.49 KB, patch)
2020-03-04 11:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24719: [Alternate PATCH] Remove use of CGI object in C4::Context::set_remote_address() (1.51 KB, patch)
2020-03-05 12:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24719: Remove use of CGI object in C4::Context::set_remote_address() (1.62 KB, patch)
2020-03-06 07:35 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 2020-02-25 02:16:20 UTC
Unfortunately, https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23068 appears to have broken file upload in non-Plack CGI Koha (I haven't checked Plack enabled Koha).

If you have koha_trusted_proxies set, when you load C4::Auth, it'll create a CGI object. 

It appears that creating this CGI object does some global work, which causes CGI::init to malfunction, and as a result Koha::Uploader's CGI upload hook never runs.

--

I was comparing two 19.11 instances. File upload worked on one but not the other. The likely culprit looked like system preferences or koha-conf.xml and thus code being invoked by C4::Context or C4::Auth. In the end it was C4::Auth calling C4::Context->set_remote_address(). (Not a fun experience troubleshooting this one by any means O_O.)
Comment 1 David Cook 2020-02-25 02:17:07 UTC
My immediate solution is to replace the use of the CGI module by just checking the %ENV hash directly for HTTP_X_FORWARDED_FOR, but I don't know if that solution works with Plack.

I'll include a patch shortly in any case.
Comment 2 David Cook 2020-02-25 02:18:37 UTC
Oh and the file I tested was /cgi-bin/koha/tools/stage-marc-import.pl
Comment 3 David Cook 2020-02-25 02:34:02 UTC
Created attachment 99550 [details] [review]
Remove use of CGI in C4::Context::set_remote_address()

This patch removes the CGI module usage, as it was causing file upload
problems for scripts like /cgi-bin/koha/tools/stage-marc-import.pl

Test plan:
1. Set koha_trusted_proxies in koha-conf.xml to 1.1.1.1
2. Clear Memcached
3. Try to upload MARCXML file to /cgi-bin/koha/tools/stage-marc-import.pl
4. Note that form below "Upload progress" never appears and errors show
in browser console
5. Apply the patch
6. Try to upload MARCXML file to /cgi-bin/koha/tools/stage-marc-import.pl
7. Note that form appears below "Upload progress"
Comment 4 Martin Renvoize 2020-02-25 16:38:42 UTC
Created attachment 99594 [details] [review]
Bug 24719: Remove use of CGI in C4::Context::set_remote_address()

This patch removes the CGI module usage, as it was causing file upload
problems for scripts like /cgi-bin/koha/tools/stage-marc-import.pl

Test plan:
1. Set koha_trusted_proxies in koha-conf.xml to 1.1.1.1
2. Clear Memcached
3. Try to upload MARCXML file to /cgi-bin/koha/tools/stage-marc-import.pl
4. Note that form below "Upload progress" never appears and errors show
in browser console
5. Apply the patch
6. Try to upload MARCXML file to /cgi-bin/koha/tools/stage-marc-import.pl
7. Note that form appears below "Upload progress"

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2020-02-25 16:39:35 UTC
Works as expected.. Signing off, thanks David
Comment 6 David Cook 2020-02-26 01:36:23 UTC
(In reply to Martin Renvoize from comment #5)
> Works as expected.. Signing off, thanks David

Thanks, Martin! Was that for a Plack-enabled Koha or non-Plack?
Comment 7 Jonathan Druart 2020-02-26 09:43:32 UTC
CGI->http code is

sub http {
    my ($self,$parameter) = self_or_CGI(@_);
    if ( defined($parameter) ) {
        $parameter =~ tr/-a-z/_A-Z/;
        if ( $parameter =~ /^HTTP(?:_|$)/ ) {
            return $ENV{$parameter};
        }
        return $ENV{"HTTP_$parameter"};
    }
    return grep { /^HTTP(?:_|$)/ } keys %ENV;
}

So that could should be safe.
But, are we suppose to suppose non-plack? How slow that should be!
Comment 8 Katrin Fischer 2020-02-26 20:15:42 UTC
(In reply to Jonathan Druart from comment #7)
> CGI->http code is
> 
> sub http {
>     my ($self,$parameter) = self_or_CGI(@_);
>     if ( defined($parameter) ) {
>         $parameter =~ tr/-a-z/_A-Z/;
>         if ( $parameter =~ /^HTTP(?:_|$)/ ) {
>             return $ENV{$parameter};
>         }
>         return $ENV{"HTTP_$parameter"};
>     }
>     return grep { /^HTTP(?:_|$)/ } keys %ENV;
> }
> 
> So that could should be safe.
> But, are we suppose to suppose non-plack? How slow that should be!

Until not so long ago Shibboleth did not work with Plack enabled - I am not sure if there are other features that block use of Plack still... but there might?
Comment 9 David Cook 2020-02-26 23:36:15 UTC
(In reply to Jonathan Druart from comment #7)
> CGI->http code is
...
> 
> So that could should be safe.

Oh... that's a good point! Yeah if we use it as a class method rather than an object method, that should be fine, and it would be more robust than my patch (I recently got bitten by lowercase HTTP headers on a different project).

I'll try that and report back.
Comment 10 David Cook 2020-02-26 23:40:35 UTC
(In reply to Jonathan Druart from comment #7)
> But, are we suppose to suppose non-plack? How slow that should be!

I haven't found it to be too slow, unless you're using the API. That's super slow in CGI. 

I have been wondering though when the community will commit to dropping CGI support and then making it obsolete all together.

Running Plack on a large multitenant server can be challenging, although I think Julian and some others have done some interesting work in a multitenant Mojolicious powered Koha.
Comment 11 David Cook 2020-02-27 00:20:35 UTC
Ok a few things:

1. I can confirm that CGI->http works with CGI. Thanks for the suggestion, Jonathan! 

2. $cgi->http worked fine for Plack-enabled Koha presumably because C4::Context->set_remote_address is just called by C4::Auth when it's loaded into memory the first time. After that the Koha::Middleware::RealIP and other Plack CGI emulation does all the heavy lifting I think.

So really C4::Context->set_remote_address is really only relevant for CGI in the first place, so I guess it's worth fixing the bug, since that function just exists for CGI anyway.

Will provide an alternate patch with CGI->http
Comment 12 David Cook 2020-02-27 00:31:32 UTC
Created attachment 99680 [details] [review]
[Alternate PATCH] Bug 24719: Remove use of CGI object in C4::Context::set_remote_address()

This patch replaces the CGI "http" object method with its equivalent
class method, which doesn't require object instantiation and thus skips
global initialization and premature handling of the incoming HTTP request.

Test plan:
0. Disable Plack if it is enabled
1. Set koha_trusted_proxies in koha-conf.xml to 1.1.1.1
2. Clear Memcached
3. Try to upload MARCXML file to /cgi-bin/koha/tools/stage-marc-import.pl
4. Note that form below "Upload progress" never appears and errors show
in browser console
5. Apply the patch
6. Try to upload MARCXML file to /cgi-bin/koha/tools/stage-marc-import.pl
7. Note that form appears below "Upload progress"
Comment 13 Nick Clemens 2020-03-04 11:11:23 UTC
Created attachment 100095 [details] [review]
Bug 24719: Remove use of CGI object in C4::Context::set_remote_address()

This patch replaces the CGI "http" object method with its equivalent
class method, which doesn't require object instantiation and thus skips
global initialization and premature handling of the incoming HTTP request.

Test plan:
0. Disable Plack if it is enabled
1. Set koha_trusted_proxies in koha-conf.xml to 1.1.1.1
2. Clear Memcached
3. Try to upload MARCXML file to /cgi-bin/koha/tools/stage-marc-import.pl
4. Note that form below "Upload progress" never appears and errors show
in browser console
5. Apply the patch
6. Try to upload MARCXML file to /cgi-bin/koha/tools/stage-marc-import.pl
7. Note that form appears below "Upload progress"

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens 2020-03-04 11:12:00 UTC
Signed alternate patch, works as described, leaving as 'signed off' to get a second qa check here
Comment 15 David Cook 2020-03-04 23:25:23 UTC
Is there an easy way to update the uploaded file names? I think that second patch has lost its "Alternate" flag.
Comment 16 Nick Clemens 2020-03-05 12:19:21 UTC
Created attachment 100181 [details] [review]
Bug 24719: [Alternate PATCH] Remove use of CGI object in C4::Context::set_remote_address()

This patch replaces the CGI "http" object method with its equivalent
class method, which doesn't require object instantiation and thus skips
global initialization and premature handling of the incoming HTTP request.

Test plan:
0. Disable Plack if it is enabled
1. Set koha_trusted_proxies in koha-conf.xml to 1.1.1.1
2. Clear Memcached
3. Try to upload MARCXML file to /cgi-bin/koha/tools/stage-marc-import.pl
4. Note that form below "Upload progress" never appears and errors show
in browser console
5. Apply the patch
6. Try to upload MARCXML file to /cgi-bin/koha/tools/stage-marc-import.pl
7. Note that form appears below "Upload progress"

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Marcel de Rooy 2020-03-06 07:18:57 UTC
QAing
Comment 18 Marcel de Rooy 2020-03-06 07:33:38 UTC
> Test plan:
> 0. Disable Plack if it is enabled

I am having the impression that this bug has nothing to do with Plack. It is about creating another CGI object that "steals the upload".

> 3. Try to upload MARCXML file to /cgi-bin/koha/tools/stage-marc-import.pl

See debian/templates/apache-shared-intranet-plack.conf
ProxyPass "/cgi-bin/koha/tools/stage-marc-import.pl" "!"
This script does not even function under Plack, it is handled by Apache!

But it is clear to me that preventing the CGI->new call and calling http as a class method resolves the situation. So passing QA on the second patch. Note that the first patch does the same thing, but lets keep that shuffling of env vars in the module.
Comment 19 Marcel de Rooy 2020-03-06 07:35:15 UTC
Created attachment 100211 [details] [review]
Bug 24719: Remove use of CGI object in C4::Context::set_remote_address()

This patch replaces the CGI "http" object method with its equivalent
class method, which doesn't require object instantiation and thus skips
global initialization and premature handling of the incoming HTTP request.

Test plan:
0. Disable Plack if it is enabled
1. Set koha_trusted_proxies in koha-conf.xml to 1.1.1.1
2. Clear Memcached
3. Try to upload MARCXML file to /cgi-bin/koha/tools/stage-marc-import.pl
4. Note that form below "Upload progress" never appears and errors show
in browser console
5. Apply the patch
6. Try to upload MARCXML file to /cgi-bin/koha/tools/stage-marc-import.pl
7. Note that form appears below "Upload progress"

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
See comment18 on Bugzilla.
Comment 20 Martin Renvoize 2020-03-06 10:05:03 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 21 Joy Nelson 2020-04-01 22:25:29 UTC
Backported to 19.11.x for 19.11.05
Comment 22 Lucas Gass 2020-04-09 19:52:18 UTC
the sub set_remote_address does not exist in C4/Context.pm in 19.05.x, no backport