[Bug 24719] New: C4::Context::set_remote_address() prevents file upload for non-Plack Koha
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Bug ID: 24719 Summary: C4::Context::set_remote_address() prevents file upload for non-Plack Koha Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: critical Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: dcook@prosentient.com.au QA Contact: testopia@bugs.koha-community.org 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.) -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 --- Comment #1 from David Cook <dcook@prosentient.com.au> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23068 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- Oh and the file I tested was /cgi-bin/koha/tools/stage-marc-import.pl -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- Created attachment 99550 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99550&action=edit 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" -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 99594 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99594&action=edit 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> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99550|0 |1 is obsolete| | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #5 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Works as expected.. Signing off, thanks David -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- (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? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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! -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #8 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 --- Comment #9 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 --- Comment #10 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |dcook@prosentient.com.au |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 --- Comment #11 from David Cook <dcook@prosentient.com.au> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 --- Comment #12 from David Cook <dcook@prosentient.com.au> --- Created attachment 99680 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99680&action=edit [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" -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99680|0 |1 is obsolete| | --- Comment #13 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 100095 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100095&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #14 from Nick Clemens <nick@bywatersolutions.com> --- Signed alternate patch, works as described, leaving as 'signed off' to get a second qa check here -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 --- Comment #15 from David Cook <dcook@prosentient.com.au> --- Is there an easy way to update the uploaded file names? I think that second patch has lost its "Alternate" flag. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100095|0 |1 is obsolete| | --- Comment #16 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 100181 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100181&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QAing -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> ---
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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 100211 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100211&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99594|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100181|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |20.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|20.05.00 |20.05.00, 19.11.05 released in| | CC| |joy@bywatersolutions.com --- Comment #21 from Joy Nelson <joy@bywatersolutions.com> --- Backported to 19.11.x for 19.11.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24719 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #22 from Lucas Gass <lucas@bywatersolutions.com> --- the sub set_remote_address does not exist in C4/Context.pm in 19.05.x, no backport -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org