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.)
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.
Oh and the file I tested was /cgi-bin/koha/tools/stage-marc-import.pl
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"
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>
Works as expected.. Signing off, thanks David
(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?
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!
(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?
(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.
(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.
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
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"
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>
Signed alternate patch, works as described, leaving as 'signed off' to get a second qa check here
Is there an easy way to update the uploaded file names? I think that second patch has lost its "Alternate" flag.
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>
QAing
> 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.
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.
Nice work everyone! Pushed to master for 20.05
Backported to 19.11.x for 19.11.05
the sub set_remote_address does not exist in C4/Context.pm in 19.05.x, no backport