Summary: | PSGI Koha does not use custom ErrorDocument pages | ||
---|---|---|---|
Product: | Koha | Reporter: | David Cook <dcook> |
Component: | Architecture, internals, and plumbing | Assignee: | David Cook <dcook> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | barton, caroline.cyr-la-rose, dcook, fridolin.somers, jonathan.druart, nick |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24574 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25026 |
||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
This change allows PSGI (Plack) Koha to use the custom ErrorDocument pages that CGI Koha already uses. Without this change, a 500 error will show a white page with only "Internal server error" and a 404 will show a white page with only "not found". This change aligns the error reporting for the two different Koha web modes.
|
Version(s) released in: |
21.05.00
|
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 27556, 27557, 27555 | ||
Attachments: |
Bug 26048: POC - use ErrorDocument for 500
Bug 26048: Use ErrorDocument middleware for Plack HTTP errors Bug 26048: Use ErrorDocument middleware for Plack HTTP errors Bug 26048: Use ErrorDocument middleware for Plack HTTP errors Bug 26048: Use ErrorDocument middleware for Plack HTTP errors Bug 26048: Use ErrorDocument middleware for Plack HTTP errors |
Description
David Cook
2020-07-23 00:45:42 UTC
Actually, I don't think it is using Apache's generic error response document. My mistake! Apache's generic 500 response is different* * Content-language: en Content-type: text/html; charset=UTF-8 Body:----------en-- <!--#set var="TITLE" value="Server error!" --><!--#include virtual="include/top.html" --> <!--#if expr="-n v('REDIRECT_ERROR_NOTES')" --> The server encountered an internal error and was unable to complete your request. <!--#include virtual="include/spacer.html" --> Error message: <br /><!--#echo encoding="none" var="REDIRECT_ERROR_NOTES" --> <!--#else --> The server encountered an internal error and was unable to complete your request. Either the server is overloaded or there was an error in a CGI script. <!--#endif --> <!--#include virtual="include/bottom.html" --> ----------en-- And I think the answer can be found by reading https://docstore.mik.ua/orelly/linux/cgi/ch05_05.htm In C4::Context, in a BEGIN block, we do the following: require CGI::Carp; import CGI::Carp qw(fatalsToBrowser); If you look at https://metacpan.org/release/CGI/source/lib/CGI/Carp.pm, you'll see the HTML that is used when a Perl script dies with a 500 error - at least when using CGI Koha. If you're using Plack Koha (in dev mode), you'll get a stack trace instead. I have to try Plack Koha in prod mode... With Plack Koha in "deployment" (prod) mode, you get a HTTP 500 response back with just the following text: "Internal Server Error". Very different to the CGI::Carp generated response when using CGI Koha. According to the stack trace from Plack Koha (development), "Internal Server Error" comes from Plack::Util::run_app. You can see the line at https://metacpan.org/release/Plack/source/lib/Plack/Util.pm#L146 That's interesting. That seems like a very minimal non-user friendly catch all. I wonder what Plack-specific options we have... This one is cool but would require extensive refactoring. https://metacpan.org/pod/Plack::Middleware::HTTPExceptions That being said, we could write our own Middleware using the model used by Miyagawa's above example. The one downside is that this also creates a discrepancy between Plack and CGI, although... I think at some point we should probably deprecate CGI mode. That leads us to Apache's directive "ProxyErrorOverride On". Interestingly, I just enabled that directive, and now I really am getting Apache's generic HTTP 500 error response document. And that's because the ErrorDocument scripts are scripts which are proxied to Plack, so Apache won't use them for ProxyErrorOverride. But we can get around that by adding the following to the *-plack.conf Apache files: ProxyPass "/cgi-bin/koha/errors/500.pl" "!" At the moment, I don't have a perfect solution to cover both Plack Koha and CGI Koha, but I think that we do need something. Currently in CGI Koha, CGI::Carp overrides the SIGDIE handler. Currently in Plack Koha, Starman calls Plack::Util::run_app (during dispatch_request) and that function does an eval{} around the whole PSGI app to trap the die(), so it doesn't even reach the SIGDIE handler. -- I think the most obvious solution is to use the ErrorDocument defined in Apache. For Plack Koha, we can do that by turning on ProxyErrorOverride and preventing the proxying of /cgi-bin/koha/errors/500.pl. For CGI Koha, we could remove usage of CGI::Carp. -- Alternatively, we could have Koha output its own 500 response without relying on Apache. For Plack Koha, we'd need to write our own Plack::Middleware:: module. For CGI Koha, we could add a handler to CGI::Carp. The downside of this is that we'd have to maintain 2 different code flows. That said, the upside is that we'd have more control over the error response. We could start passing context-specific user-friendly messages in the 500 response. This could allow us to do error-handling which is a bit more graceful. *** Bug 21067 has been marked as a duplicate of this bug. *** There is Plack::Middleware::ErrorDocument (In reply to Jonathan Druart from comment #9) > There is Plack::Middleware::ErrorDocument Nice find. If we use 'subrequest => 1', it looks like that would probably do the trick. Jonathan mentioned Plack::Middleware::DiePretty on IRC too Created attachment 115434 [details] [review] Bug 26048: POC - use ErrorDocument for 500 Proof of concept to use ErrorDocument to handle our 500. This is not ready at all, we need to tweak the mount and enable statements to make it possible to have a separate 500 for OPAC and intranet To test you need to: 1. Edit /usr/sbin/koha-plack Force environment to be set to deployment 109 environment="deployment"; 2. Add a die somewhere and hit the page I've been procrastinating this one, but maybe I should just sit down and do it. I think I have a fairly simple idea in mind. Created attachment 115488 [details] [review] Bug 26048: Use ErrorDocument middleware for Plack HTTP errors This patch uses the ErrorDocument middleware to use Koha's custom error pages instead of generic Plack error responses Test plan: 0. Apply patch 1. vi /usr/sbin/koha-plack (and change "development" to "deployment") 2. vi ./opac/opac-main.pl 3. Add "die" to line 2 4. vi ./mainpage.pl 5. Add "die" to line 2 6. cp ./debian/templates/plack.psgi /etc/koha/sites/kohadev/plack.psgi 7. koha-plack --restart kohadev 8. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl 9. See a beautiful OPAC 500 error instead of "Internal Server Error" 10. Go to http://localhost:8080/cgi-bin/koha/blah.pl 11. See a beautiful OPAC 404 error instead of "not found" 12. Go to http://localhost:8081/cgi-bin/koha/mainpage.pl 13. See a beautiful Staff interface 500 error instead of "Internal Server Error" 14. Go to http://localhost:8081/cgi-bin/koha/blah.pl 15. See a beautiful Staff interface 404 error instead of "not found" For bonus points: 16. koha-plack --disable kohadev 17. koha-plack --stop kohadev 18. service apache restart 19. Repeat the above test plan to show CGI still works for 404 (although 500 will show "Software Error" due to C4::Context needing some improvements) 20. Using the "Network" tab on your developer tools, make sure 404 and 500 are returned by the appropriate error pages We should rename this bug since "Apache does not use /cgi-bin/koha/errors/500.pl if Perl script dies" is actually relevant for CGI mode and is irrelevant to what we're trying to fix here. We should probably rename this to something like "Starman does not use Koha custom error pages". Btw, this was driving me crazy. I actually had to hack on Plack::Middleware::ErrorDocument to realize I needed to use relative URLs in the .psgi file. And actually... the StackTrace part was a gotcha as well. Now that I read about Plack::Middleware::StackTrace, it seems like it's only recommended for development and not production. But the Plack::Middleware::HTTPExceptions recommendation doesn't look right for us either. Maybe I should make a simple middleware to replace it. I think I know what we need to do.. Ok I wrote my own Koha::Middleware::PreventDie but Plack::Middlware::HTTPExceptions actually does the same thing and is more feature rich for the future, so I'll just use it instead. Created attachment 115493 [details] [review] Bug 26048: Use ErrorDocument middleware for Plack HTTP errors This patch uses the ErrorDocument middleware to use Koha's custom error pages instead of generic Plack error responses Test plan: 0. Apply patch 1. vi /usr/sbin/koha-plack (and change "development" to "deployment") 2. vi ./opac/opac-main.pl 3. Add "die" to line 2 4. vi ./mainpage.pl 5. Add "die" to line 2 6. cp ./debian/templates/plack.psgi /etc/koha/sites/kohadev/plack.psgi 7. koha-plack --restart kohadev 8. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl 9. See a beautiful OPAC 500 error instead of "Internal Server Error" 10. Go to http://localhost:8080/cgi-bin/koha/blah.pl 11. See a beautiful OPAC 404 error instead of "not found" 12. Go to http://localhost:8081/cgi-bin/koha/mainpage.pl 13. See a beautiful Staff interface 500 error instead of "Internal Server Error" 14. Go to http://localhost:8081/cgi-bin/koha/blah.pl 15. See a beautiful Staff interface 404 error instead of "not found" For bonus points: 16. koha-plack --disable kohadev 17. koha-plack --stop kohadev 18. service apache restart 19. Repeat the above test plan to show CGI still works for 404 (although 500 will show "Software Error" due to C4::Context needing some improvements) 20. Using the "Network" tab on your developer tools, make sure 404 and 500 are returned by the appropriate error pages Thanks David, I was missing HTTPExceptions to make it works that way! We should update the list of reasons of the 500 """ You made use of an external link to an item that is no longer available You followed an outdated link e.g. from a search engine or a bookmark You tried to access a page that needs authentication An internal link in the client is broken and the page does not exist """ At least the 3rd one is wrong. The most probable reason is that there is a bug :D Yesterday I was thinking we could have a syspref to display/hide the error (not the stacktrace). Or enable it by default for superlibrarian. What do you think (on its own bug report of course)? Created attachment 115619 [details] [review] Bug 26048: Use ErrorDocument middleware for Plack HTTP errors This patch uses the ErrorDocument middleware to use Koha's custom error pages instead of generic Plack error responses Test plan: 0. Apply patch 1. vi /usr/sbin/koha-plack (and change "development" to "deployment") 2. vi ./opac/opac-main.pl 3. Add "die" to line 2 4. vi ./mainpage.pl 5. Add "die" to line 2 6. cp ./debian/templates/plack.psgi /etc/koha/sites/kohadev/plack.psgi 7. koha-plack --restart kohadev 8. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl 9. See a beautiful OPAC 500 error instead of "Internal Server Error" 10. Go to http://localhost:8080/cgi-bin/koha/blah.pl 11. See a beautiful OPAC 404 error instead of "not found" 12. Go to http://localhost:8081/cgi-bin/koha/mainpage.pl 13. See a beautiful Staff interface 500 error instead of "Internal Server Error" 14. Go to http://localhost:8081/cgi-bin/koha/blah.pl 15. See a beautiful Staff interface 404 error instead of "not found" For bonus points: 16. koha-plack --disable kohadev 17. koha-plack --stop kohadev 18. service apache restart 19. Repeat the above test plan to show CGI still works for 404 (although 500 will show "Software Error" due to C4::Context needing some improvements) 20. Using the "Network" tab on your developer tools, make sure 404 and 500 are returned by the appropriate error pages Signed-off-by: Eden Bacani <eden.bacani@gmail.com> (In reply to Jonathan Druart from comment #19) > Thanks David, I was missing HTTPExceptions to make it works that way! > > We should update the list of reasons of the 500 > """ > You made use of an external link to an item that is no longer available > You followed an outdated link e.g. from a search engine or a bookmark > You tried to access a page that needs authentication > An internal link in the client is broken and the page does not exist > """ > At least the 3rd one is wrong. > > The most probable reason is that there is a bug :D > Mmm good point. I'd say that whole list is wrong heh. Separate bug report though, yeah? > Yesterday I was thinking we could have a syspref to display/hide the error > (not the stacktrace). Or enable it by default for superlibrarian. What do > you think (on its own bug report of course)? Hmm, you mean display/hide the error that caused the 500? The only way we could do that in my mind would be with a custom middleware before HTTPExceptions. Given Koha's current structure, that's the only way I can think to do it. (In reply to Eden from comment #20) > Signed-off-by: Eden Bacani <eden.bacani@gmail.com> Thanks Eden :D Created attachment 115682 [details] [review] Bug 26048: Use ErrorDocument middleware for Plack HTTP errors This patch uses the ErrorDocument middleware to use Koha's custom error pages instead of generic Plack error responses Test plan: 0. Apply patch 1. vi /usr/sbin/koha-plack (and change "development" to "deployment") 2. vi ./opac/opac-main.pl 3. Add "die" to line 2 4. vi ./mainpage.pl 5. Add "die" to line 2 6. cp ./debian/templates/plack.psgi /etc/koha/sites/kohadev/plack.psgi 7. koha-plack --restart kohadev 8. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl 9. See a beautiful OPAC 500 error instead of "Internal Server Error" 10. Go to http://localhost:8080/cgi-bin/koha/blah.pl 11. See a beautiful OPAC 404 error instead of "not found" 12. Go to http://localhost:8081/cgi-bin/koha/mainpage.pl 13. See a beautiful Staff interface 500 error instead of "Internal Server Error" 14. Go to http://localhost:8081/cgi-bin/koha/blah.pl 15. See a beautiful Staff interface 404 error instead of "not found" For bonus points: 16. koha-plack --disable kohadev 17. koha-plack --stop kohadev 18. service apache restart 19. Repeat the above test plan to show CGI still works for 404 (although 500 will show "Software Error" due to C4::Context needing some improvements) 20. Using the "Network" tab on your developer tools, make sure 404 and 500 are returned by the appropriate error pages Signed-off-by: Eden Bacani <eden.bacani@gmail.com> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> I also sign, it is a super wanted change Created attachment 115684 [details] [review] Bug 26048: Use ErrorDocument middleware for Plack HTTP errors This patch uses the ErrorDocument middleware to use Koha's custom error pages instead of generic Plack error responses Test plan: 0. Apply patch 1. vi /usr/sbin/koha-plack (and change "development" to "deployment") 2. vi ./opac/opac-main.pl 3. Add "die" to line 2 4. vi ./mainpage.pl 5. Add "die" to line 2 6. cp ./debian/templates/plack.psgi /etc/koha/sites/kohadev/plack.psgi 7. koha-plack --restart kohadev 8. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl 9. See a beautiful OPAC 500 error instead of "Internal Server Error" 10. Go to http://localhost:8080/cgi-bin/koha/blah.pl 11. See a beautiful OPAC 404 error instead of "not found" 12. Go to http://localhost:8081/cgi-bin/koha/mainpage.pl 13. See a beautiful Staff interface 500 error instead of "Internal Server Error" 14. Go to http://localhost:8081/cgi-bin/koha/blah.pl 15. See a beautiful Staff interface 404 error instead of "not found" For bonus points: 16. koha-plack --disable kohadev 17. koha-plack --stop kohadev 18. service apache restart 19. Repeat the above test plan to show CGI still works for 404 (although 500 will show "Software Error" due to C4::Context needing some improvements) 20. Using the "Network" tab on your developer tools, make sure 404 and 500 are returned by the appropriate error pages Signed-off-by: Eden Bacani <eden.bacani@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> QA tools complain about authnotrequired in 404, but I think it is okay If only we could find a way to display the error in 500.pl. Would be so useful for support. (In reply to Nick Clemens from comment #26) > QA tools complain about authnotrequired in 404, but I think it is okay I think so. I modelled it off the other 4xx errors in the intranet, so I think it's a spurious complaint. (In reply to Fridolin Somers from comment #27) > If only we could find a way to display the error in 500.pl. > Would be so useful for support. The errors should still be in your web server logs. See https://metacpan.org/release/Plack/source/lib/Plack/Middleware/HTTPExceptions.pm#L63 In terms of displaying on the screen, we could probably do it a number of ways. For instance, we could do our own middleware to catch errors and then throw a HTTP::Exception and let Plack::Middleware::HTTPException handle it. Or we could replace HTTPException with our own middleware which sets an environmental variable that we then print in the subrequest done by ErrorDocument. Or we could replace both HTTPException and ErrorDocument with a middleware that renders the template we want and shows the message we want. It wouldn't be very hard. But in most production situations I don't think that we'd want to show system generated error messages. It's not a good look. Ok thanks a lot David Pushed to master for 21.05, thanks to everybody involved! Pushed to 20.11.x for 20.11.03 I wouldn't have backported it now. We should write the patches for the follow-up bugs first. OK i revert (In reply to Fridolin Somers from comment #35) > OK i revert Ouf I had not yet pushed to remote git server, so I just remove from my local branch |