Bug 26048 - PSGI Koha does not use custom ErrorDocument pages
Summary: PSGI Koha does not use custom ErrorDocument pages
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
: 21067 (view as bug list)
Depends on:
Blocks: 27555 27556 27557
  Show dependency treegraph
 
Reported: 2020-07-23 00:45 UTC by David Cook
Modified: 2021-04-21 13:50 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
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


Attachments
Bug 26048: POC - use ErrorDocument for 500 (3.16 KB, patch)
2021-01-20 14:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26048: Use ErrorDocument middleware for Plack HTTP errors (12.26 KB, patch)
2021-01-21 02:01 UTC, David Cook
Details | Diff | Splinter Review
Bug 26048: Use ErrorDocument middleware for Plack HTTP errors (12.39 KB, patch)
2021-01-21 02:46 UTC, David Cook
Details | Diff | Splinter Review
Bug 26048: Use ErrorDocument middleware for Plack HTTP errors (12.44 KB, patch)
2021-01-21 20:41 UTC, Eden
Details | Diff | Splinter Review
Bug 26048: Use ErrorDocument middleware for Plack HTTP errors (12.50 KB, patch)
2021-01-22 12:46 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26048: Use ErrorDocument middleware for Plack HTTP errors (12.49 KB, patch)
2021-01-22 12:54 UTC, Nick Clemens
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-07-23 00:45:42 UTC
I've noticed Apache isn't using /cgi-bin/koha/errors/500.pl if the Perl script dies.

If you don't have the executable bit set and you're using CGI Koha, then it'll use /cgi-bin/koha/errors/500.pl, but otherwise Apache will use its generic error response document.
Comment 1 David Cook 2020-07-23 01:01:33 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--
Comment 2 David Cook 2020-07-23 01:06:07 UTC
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...
Comment 3 David Cook 2020-07-23 01:18:10 UTC
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.
Comment 4 David Cook 2020-07-23 01:22:22 UTC
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.
Comment 5 David Cook 2020-07-23 01:30:26 UTC
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.
Comment 6 David Cook 2020-07-23 01:37:38 UTC
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" "!"
Comment 7 David Cook 2020-07-23 02:09:38 UTC
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.
Comment 8 Jonathan Druart 2020-07-23 06:33:30 UTC
*** Bug 21067 has been marked as a duplicate of this bug. ***
Comment 9 Jonathan Druart 2020-07-23 06:33:43 UTC
There is Plack::Middleware::ErrorDocument
Comment 10 David Cook 2020-07-23 06:55:44 UTC
(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.
Comment 11 David Cook 2020-07-23 07:16:31 UTC
Jonathan mentioned  Plack::Middleware::DiePretty on IRC too
Comment 12 Jonathan Druart 2021-01-20 14:15:43 UTC
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
Comment 13 David Cook 2021-01-20 23:46:01 UTC
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.
Comment 14 David Cook 2021-01-21 02:01:49 UTC
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
Comment 15 David Cook 2021-01-21 02:04:05 UTC
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".
Comment 16 David Cook 2021-01-21 02:17:46 UTC
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..
Comment 17 David Cook 2021-01-21 02:44:45 UTC
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.
Comment 18 David Cook 2021-01-21 02:46:55 UTC
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
Comment 19 Jonathan Druart 2021-01-21 09:45:37 UTC
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)?
Comment 20 Eden 2021-01-21 20:41:15 UTC
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>
Comment 21 David Cook 2021-01-22 01:22:20 UTC
(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.
Comment 22 David Cook 2021-01-22 01:22:44 UTC
(In reply to Eden from comment #20)
> Signed-off-by: Eden Bacani <eden.bacani@gmail.com>

Thanks Eden :D
Comment 23 Fridolin Somers 2021-01-22 12:46:03 UTC
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>
Comment 24 Fridolin Somers 2021-01-22 12:46:42 UTC
I also sign, it is a super wanted change
Comment 25 Nick Clemens 2021-01-22 12:54:26 UTC
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>
Comment 26 Nick Clemens 2021-01-22 12:55:52 UTC
QA tools complain about authnotrequired in 404, but I think it is okay
Comment 27 Fridolin Somers 2021-01-22 13:00:09 UTC
If only we could find a way to display the error in 500.pl.
Would be so useful for support.
Comment 28 David Cook 2021-01-24 22:35:56 UTC
(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.
Comment 29 David Cook 2021-01-24 22:50:06 UTC
(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.
Comment 30 David Cook 2021-01-24 22:51:12 UTC
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.
Comment 31 Fridolin Somers 2021-01-25 14:18:07 UTC
Ok thanks a lot David
Comment 32 Jonathan Druart 2021-01-27 13:37:26 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 33 Fridolin Somers 2021-01-29 14:03:56 UTC
Pushed to 20.11.x for 20.11.03
Comment 34 Jonathan Druart 2021-01-29 14:34:13 UTC
I wouldn't have backported it now.
We should write the patches for the follow-up bugs first.
Comment 35 Fridolin Somers 2021-01-29 14:35:44 UTC
OK i revert
Comment 36 Fridolin Somers 2021-01-29 14:38:06 UTC
(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