|
Description
Liz Rea
2021-11-04 23:55:20 UTC
Created attachment 127328 [details] [review] Bug 29420 - 401 Unauthorized pages come back is 200 OK under plack To test: - create an alias in your /etc/koha/sites/<site>.conf file for something like files Alias /files "/var/lib/koha/kohadev/public_html/" <Directory "/var/lib/koha/kohadev/public_html/"> Options +Indexes AuthUserFile /var/lib/koha/kohadev/.htpasswd AuthName ByPassword AuthType Basic <Limit GET POST PUT> require valid-user </Limit> </Directory> - make a .htpasswd file: https://hostingcanada.org/htpasswd-generator/ and put it in /var/lib/koha/kohadev - restart apache - navigate to http://<kohadev url>/files, note that the response header in the browser inspector says "200 OK" and the basic auth user/pass is not shown - Apply this patch, restart the things - navigate to http://<kohadev url>/files, note that the response header in the browser inspector says "401 Unauthorized" and the basic auth user/pass is now shown Created attachment 127364 [details] [review] Bug 29420 - 401 Unauthorized pages come back is 200 OK under plack To test: - create an alias in your /etc/koha/sites/<site>.conf file for something like files Alias /files "/var/lib/koha/kohadev/public_html/" <Directory "/var/lib/koha/kohadev/public_html/"> Options +Indexes AuthUserFile /var/lib/koha/kohadev/.htpasswd AuthName ByPassword AuthType Basic <Limit GET POST PUT> require valid-user </Limit> </Directory> - make a .htpasswd file: https://hostingcanada.org/htpasswd-generator/ and put it in /var/lib/koha/kohadev - restart apache - navigate to http://<kohadev url>/files, note that the response header in the browser inspector says "200 OK" and the basic auth user/pass is not shown - Apply this patch, restart the things - navigate to http://<kohadev url>/files, note that the response header in the browser inspector says "401 Unauthorized" and the basic auth user/pass is now shown Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Looking at Bug 26048, it looks like I added 200 OK for a reason, although it's not 100% clear to me why now. I suspect something weird with the HTTPExceptions or ErrorDocument modules. I suspect that this patch will stop Bug 26048 from working for 401. So if you're not using HTTP Basic Auth, you'll probably get the Plack generic error page instead of the Koha 401 error page. But I haven't checked yet. I'd suggest to QA to check whether the Koha error page flows through or if it's the Plack generic error message. It does not stop it from working, it shows the error page under the login screen for 401's. Do we really need to pass error pages through CGI/Perl? (In reply to Liz Rea from comment #4) > It does not stop it from working, it shows the error page under the login > screen for 401's. Hmm weird. I have no idea then without testing and digging in (which I don't have time for at the moment). If there are any issues, I can find them later. (In reply to Tomás Cohen Arazi from comment #5) > Do we really need to pass error pages through CGI/Perl? You mean instead of using static HTML pages or...? (In reply to David Cook from comment #6) > (In reply to Liz Rea from comment #4) > > It does not stop it from working, it shows the error page under the login > > screen for 401's. > > Hmm weird. I have no idea then without testing and digging in (which I don't > have time for at the moment). If there are any issues, I can find them later. This is an effective way of blocking development :) It must be wrong, but I do not have time to look further.. (In reply to Tomás Cohen Arazi from comment #5) > Do we really need to pass error pages through CGI/Perl? I wondered this too, but the improvement in the error pages is nice. Just wish it worked for the 401 case. (In reply to Marcel de Rooy from comment #8) > This is an effective way of blocking development :) > It must be wrong, but I do not have time to look further.. Have seen a lot of this lately, upsetting. Take a step back everyone ;) David was merely suggesting things to check to whoever takes on the QA.. I don't see that as a blocker. I see that as someone trying to lend a hand where they can. We're all battling with various pressures on our time and we do what we can do. It's not at all constructive to call people out on their lack of time unless you can somehow help free up some time for them to contribute more ;). Also, remember that Koha is an aging beast... we're trying hard to make it more modern and the move to Plack is part of that.. David has taken us in the right direction by improving our error pages when they're served under this way of running. None of us are experts in everything.. and our poor QA team are expected to dig in and get to grips with all area's even if their expertise is being stretched.. we're all Human. OK.. the documentation for Plack::Middleware::ErrorDocument clearly states:
> When using a subrequest, the subrequest should return a regular '200' response.
So, as we're enabling subrequest => 1 I'm sure David has got a point.. Now, I've not dug into it deeply myself yet and I'm no expert in this area.. but that does raise alarm bells in my mind.. We certainly need to be diligent in checking for regressions here.
Thanks, Martin. I really appreciate your comments. Since I authored Bug 26048, it probably does make sense for me to make more of an effort here... I found something interesting with the status quo: http://localhost:8081/files/blah This returns a 404 page with a 200 status. That's bad! http://localhost:8081/cgi-bin/koha/circ/blahblah This returns a 404 page with a 404 status. That's good but a bit confusing! (NOTE: If you do change 404.pl to return a 404 status instead of a 200 status, it does cause a regression, so that this page now returns a blank page that just says "not found".) If we look at /etc/koha/apache-shared-intranet-plack.conf, only /index.html and path /cgi-bin/koha are proxied to Starman. So it makes sense that they're the only ones invoking the middleware. But if http://localhost:8081/blahblah is using Apache and not Starman, it shouldn't be setting the environmental variables that cause 404.pl to return a 200 status instead of a 404 status... Except that Apache's error documents themselves end up calling Starman! ErrorDocument 400 /cgi-bin/koha/errors/400.pl ErrorDocument 401 /cgi-bin/koha/errors/401.pl ErrorDocument 403 /cgi-bin/koha/errors/403.pl ErrorDocument 404 /cgi-bin/koha/errors/404.pl ErrorDocument 500 /cgi-bin/koha/errors/500.pl So of course the error pages called by Apache will return a 200 page whereas the ones using the ErrorDocument middleware return the actual error code. (Let me know if I'm not being clear enough here.) -- So that's... challenging. We need a way of differentiating if these error scripts are being called directly (e.g. Apache asking Starman for it) or by the ErrorDocument middleware... One way of doing that might be to inspect the REQUEST_URI environmental variable. When coming from ErrorDocument, the REQUEST_URI is "errors/404.pl". When coming from Apache, it's "/intranet/errors/404.pl". It doesn't seem like a super robust method, but it's the best I have at the moment. Side note: In /etc/koha/sites/kohadev/plack.psgi I should've put '/errors/404.pl' instead of 'errors/404.pl' but it seems to work anyway. An easier to maintain option would be to not proxy "/cgi-bin/koha/errors/*" files to Starman, but... that would probably be short-sighted, since there are times where we redirect manually to 404.pl as well. For instance:
members/apikeys.pl: print $cgi->redirect("/cgi-bin/koha/errors/404.pl"); # escape early
So that's not that long-term of a solution...
The ErrorDocument middleware does set up environmental variables prefixed with "psgix.errordocument" but CGI::Emulate::PSGI strips those out... https://metacpan.org/dist/CGI-Emulate-PSGI/source/lib/CGI/Emulate/PSGI.pm#L53 So that's annoying. It's too bad we don't have a Plack-enabled controller for handling these so we didn't have to emulate the PSGI for CGI scripts, but that's easier said than done. (I don't have the time/energy for refactoring C4/Templates.pm and C4/Auth.pm to make things like Bug 26791 or Bug 28325 possible ;).) Created attachment 128115 [details] [review] Bug 29420: HTTP status code incorrect when calling error pages directly under Plack/PSGI The error pages wrote a HTTP status code of 200 for all PSGI requests, even though it should have only done it for PSGI requests from the ErrorDocument middleware. This patch fixes that. 0) Do not apply patch 1) Open F12 dev tools and go to Network tab 2) Go to http://localhost:8081/files/blah 3) Note that the webpage is a 404 error but HTTP status code is 200 4) Go to http://localhost:8081/cgi-bin/koha/circ/blah 5) Note that the webpage is a 404 error and HTTP status code is 404 6) Apply patch 7) Go to http://localhost:8081/files/blah 8) Note that the webpage is a 404 error and HTTP status code is 404 9) Go to http://localhost:8081/cgi-bin/koha/circ/blah 10) Note that the webpage is a 404 error and HTTP status code is 404 For Liz's use case:
To test:
- mkdir /var/lib/koha/kohadev/public_html
- create an alias in your /etc/apache2/sites-enabled/kohadev.conf file for something like files
Alias /files "/var/lib/koha/kohadev/public_html/"
<Directory "/var/lib/koha/kohadev/public_html/">
Options +Indexes
AuthUserFile /var/lib/koha/kohadev/.htpasswd
AuthName ByPassword
AuthType Basic
<Limit GET POST PUT>
require valid-user
</Limit>
</Directory>
- make a .htpasswd file: https://hostingcanada.org/htpasswd-generator/ and put it in /var/lib/koha/kohadev
- service apache2 reload
- navigate to http://localhost:8080/files/, note that the response header in
the browser inspector says "200 OK" and the basic auth user/pass is not shown
- Apply this patch, restart the things
- navigate to http://localhost:8080/files/, note that the response header in
the browser inspector says "401 Unauthorized" and the basic auth user/pass
is now shown
I don't love the solution in my patch as it seems a bit fragile, but it seems to work in koha-testing-docker and I think it might be the best we can do for now. I think the only other viable alternative right now would be not proxying the ErrorDocument pages from Apache to Starman, which would also be suboptimal. But open to other solutions. Created attachment 128204 [details] [review] Bug 29420: HTTP status code incorrect when calling error pages directly under Plack/PSGI The error pages wrote a HTTP status code of 200 for all PSGI requests, even though it should have only done it for PSGI requests from the ErrorDocument middleware. This patch fixes that. 0) Do not apply patch 1) Open F12 dev tools and go to Network tab 2) Go to http://localhost:8081/files/blah 3) Note that the webpage is a 404 error but HTTP status code is 200 4) Go to http://localhost:8081/cgi-bin/koha/circ/blah 5) Note that the webpage is a 404 error and HTTP status code is 404 6) Apply patch 7) Go to http://localhost:8081/files/blah 8) Note that the webpage is a 404 error and HTTP status code is 404 9) Go to http://localhost:8081/cgi-bin/koha/circ/blah 10) Note that the webpage is a 404 error and HTTP status code is 404 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> This all works as expected for me now, no regressions found. Thanks for finding some time to fit this in David. :) I'd love to see a follow-up bug later to use the new function to replace the various places we look for a psgi/plack app already.. but that can certainly wait. Nice work, signing off. This method exists, and is C4::Auth::psgi_env And this patch exists, on bug 27555. Why are we duplicating work? *** Bug 27555 has been marked as a duplicate of this bug. *** (In reply to Martin Renvoize from comment #20) > This all works as expected for me now, no regressions found. Thanks for > finding some time to fit this in David. :) > > I'd love to see a follow-up bug later to use the new function to replace the > various places we look for a psgi/plack app already.. but that can certainly > wait. > > Nice work, signing off. I cannot PQA without a follow-up bug report and patch. (In reply to Jonathan Druart from comment #21) > This method exists, and is C4::Auth::psgi_env > And this patch exists, on bug 27555. Why are we duplicating work? It looks like I avoided C4::Auth::psgi_env at the time since it didn't actually work: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27555#c1 (CGI::Emulate::PSGI strips out environmental variables starting with "psgi") (In reply to Jonathan Druart from comment #23) > > I cannot PQA without a follow-up bug report and patch. Could you elaborate a bit? Do you mean a follow-up bug report that depends on this one? And in that bug report one would replace C4::Auth::psgi_env with C4::Context->is_psgi_or_plack() (or vice versa) and check that it doesn't cause a regression in the multiple different calls to C4::Auth::safe_exit? (In reply to David Cook from comment #25) > (In reply to Jonathan Druart from comment #23) > > > > I cannot PQA without a follow-up bug report and patch. > > Could you elaborate a bit? > > Do you mean a follow-up bug report that depends on this one? And in that bug > report one would replace C4::Auth::psgi_env with > C4::Context->is_psgi_or_plack() (or vice versa) and check that it doesn't > cause a regression in the multiple different calls to C4::Auth::safe_exit? Yes, we should use a single method for the same thing. Here you introduce is_psgi_or_plack then we should use it from where psgi_env and other similar checks exist. Either here or a separate bug report. `git grep "psgi.*ENV"` shows the different occurrences. (In reply to Jonathan Druart from comment #26) > Either here or a separate bug report. > > `git grep "psgi.*ENV"` shows the different occurrences. I think that's way outside the scope of this bug, so I think a separate bug report for sure. (In reply to Jonathan Druart from comment #26) > Yes, we should use a single method for the same thing. Here you introduce > is_psgi_or_plack then we should use it from where psgi_env and other similar > checks exist. > > Either here or a separate bug report. > > `git grep "psgi.*ENV"` shows the different occurrences. Ok I've opened bug 29744. Anyone interested in this one? (In reply to David Cook from comment #29) > Anyone interested in this one? I will have a look again if you provide a patch on the follow-up bug report. Done, over to you Jonathan ;P Created attachment 132974 [details] [review] Bug 29420: HTTP status code incorrect when calling error pages directly under Plack/PSGI The error pages wrote a HTTP status code of 200 for all PSGI requests, even though it should have only done it for PSGI requests from the ErrorDocument middleware. This patch fixes that. 0) Do not apply patch 1) Open F12 dev tools and go to Network tab 2) Go to http://localhost:8081/files/blah 3) Note that the webpage is a 404 error but HTTP status code is 200 4) Go to http://localhost:8081/cgi-bin/koha/circ/blah 5) Note that the webpage is a 404 error and HTTP status code is 404 6) Apply patch 7) Go to http://localhost:8081/files/blah 8) Note that the webpage is a 404 error and HTTP status code is 404 9) Go to http://localhost:8081/cgi-bin/koha/circ/blah 10) Note that the webpage is a 404 error and HTTP status code is 404 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed to master for 22.05, thanks to everybody involved [U+1F984] *** Bug 29444 has been marked as a duplicate of this bug. *** |