Bug 29420 - HTTP status code incorrect when calling error pages directly under Plack/PSGI
Summary: HTTP status code incorrect when calling error pages directly under Plack/PSGI
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
: 29444 (view as bug list)
Depends on:
Blocks: 29744
  Show dependency treegraph
 
Reported: 2021-11-04 23:55 UTC by Liz Rea
Modified: 2023-06-08 22:26 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 29420 - 401 Unauthorized pages come back is 200 OK under plack (1.98 KB, patch)
2021-11-05 00:01 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 29420 - 401 Unauthorized pages come back is 200 OK under plack (2.03 KB, patch)
2021-11-05 13:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29420: HTTP status code incorrect when calling error pages directly under Plack/PSGI (7.76 KB, patch)
2021-12-01 01:34 UTC, David Cook
Details | Diff | Splinter Review
Bug 29420: HTTP status code incorrect when calling error pages directly under Plack/PSGI (7.83 KB, patch)
2021-12-03 08:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29420: HTTP status code incorrect when calling error pages directly under Plack/PSGI (7.92 KB, patch)
2022-04-05 13:33 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Liz Rea 2021-11-04 23:55:20 UTC
We have a use case where we define Aliases in Apache for things like some.koha.url/files that are .htpasswd protected. In 21.05 and later, 401 Unauthorized responses go through Koha and become 200 OK. 

We should make the 401 Unauthorized pages come back as 401 Unauthorized, so that browsers know what to do when BasicAuth is requested.
Comment 1 Liz Rea 2021-11-05 00:01:39 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
Comment 2 Nick Clemens 2021-11-05 13:11:07 UTC
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>
Comment 3 David Cook 2021-11-10 00:10:45 UTC
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.
Comment 4 Liz Rea 2021-11-17 22:24:20 UTC
It does not stop it from working, it shows the error page under the login screen for 401's.
Comment 5 Tomás Cohen Arazi 2021-11-17 22:26:54 UTC
Do we really need to pass error pages through CGI/Perl?
Comment 6 David Cook 2021-11-17 22:40:45 UTC
(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.
Comment 7 David Cook 2021-11-17 22:41:06 UTC
(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...?
Comment 8 Marcel de Rooy 2021-11-26 09:04:51 UTC
(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..
Comment 9 Liz Rea 2021-11-29 18:30:08 UTC
(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.
Comment 10 Martin Renvoize 2021-11-30 15:29:29 UTC
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.
Comment 11 Martin Renvoize 2021-11-30 15:41:03 UTC
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.
Comment 12 David Cook 2021-12-01 00:18:32 UTC
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.
Comment 13 David Cook 2021-12-01 00:23:28 UTC
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.
Comment 14 David Cook 2021-12-01 00:35:50 UTC
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...
Comment 15 David Cook 2021-12-01 01:06:18 UTC
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 ;).)
Comment 16 David Cook 2021-12-01 01:34:53 UTC
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
Comment 17 David Cook 2021-12-01 01:35:35 UTC
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
Comment 18 David Cook 2021-12-01 01:37:29 UTC
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.
Comment 19 Martin Renvoize 2021-12-03 08:35:06 UTC
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>
Comment 20 Martin Renvoize 2021-12-03 08:36:24 UTC
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.
Comment 21 Jonathan Druart 2021-12-13 14:56:01 UTC
This method exists, and is C4::Auth::psgi_env
And this patch exists, on bug 27555. Why are we duplicating work?
Comment 22 Jonathan Druart 2021-12-13 14:58:49 UTC
*** Bug 27555 has been marked as a duplicate of this bug. ***
Comment 23 Jonathan Druart 2021-12-13 15:00:07 UTC
(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.
Comment 24 David Cook 2021-12-17 02:49:56 UTC
(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")
Comment 25 David Cook 2021-12-17 03:01:16 UTC
(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?
Comment 26 Jonathan Druart 2021-12-17 09:28:38 UTC
(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.
Comment 27 David Cook 2021-12-19 23:37:54 UTC
(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.
Comment 28 David Cook 2021-12-21 02:28:21 UTC
(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.
Comment 29 David Cook 2022-02-09 02:11:01 UTC
Anyone interested in this one?
Comment 30 Jonathan Druart 2022-02-09 07:43:12 UTC
(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.
Comment 31 Martin Renvoize 2022-03-28 09:27:28 UTC
Done, over to you Jonathan ;P
Comment 32 Jonathan Druart 2022-04-05 13:33:45 UTC
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>
Comment 33 Fridolin Somers 2022-04-20 19:04:48 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 34 Jonathan Druart 2022-08-15 08:12:50 UTC
*** Bug 29444 has been marked as a duplicate of this bug. ***