Bug 30261 - opac/tracklinks.pl renders 404 incorrectly
Summary: opac/tracklinks.pl renders 404 incorrectly
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 23329
Blocks:
  Show dependency treegraph
 
Reported: 2022-03-10 03:28 UTC by David Cook
Modified: 2022-05-19 06:48 UTC (History)
3 users (show)

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


Attachments
Bug 30261 - Render opac 404 for opac/tracklinks.pl (2.44 KB, patch)
2022-03-10 03:37 UTC, David Cook
Details | Diff | Splinter Review
Bug 30261 - Render opac 404 for opac/tracklinks.pl (2.55 KB, patch)
2022-05-12 10:09 UTC, Paul Derscheid
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2022-03-10 03:28:52 UTC
As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23329#c87, tracklinks.pl will try to generate an intranet 404 instead of an OPAC 404 if the script is visited without the syspref being activated or if the URL is not a valid one to redirect to.
Comment 1 David Cook 2022-03-10 03:37:22 UTC
Created attachment 131533 [details] [review]
Bug 30261 - Render opac 404 for opac/tracklinks.pl

This patch updates C4::Output::output_error to take
an additional argument which contains an "interface"
parameter, so that opac/tracklinks.pl can generate
an OPAC error instead of an Intranet error.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Go to http://localhost:8081/test.pl
2. Note normal Intranet 404 error
3. Go to http://localhost:8080/cgi-bin/koha/tracklinks.pl?uri=https%3A%2F%2Ftest.com&biblionumber=29
4. Note normal Opac 404 error
4b. For extra points, note that tracklinks.pl returns a 404 instead of a 302 to the 404.pl page
5. Change 'TrackClicks' syspref to "Track"
6. Go to http://localhost:8080/cgi-bin/koha/tracklinks.pl?uri=https%3A%2F%2Ftest.com&biblionumber=29
7. Note normal Opac 404 error
7b. For extra points, note that tracklinks.pl returns a 404 instead of a 302 to the 404.pl page
8. Rejoice!
Comment 2 Paul Derscheid 2022-05-12 10:09:28 UTC
Created attachment 134921 [details] [review]
Bug 30261 - Render opac 404 for opac/tracklinks.pl

This patch updates C4::Output::output_error to take
an additional argument which contains an "interface"
parameter, so that opac/tracklinks.pl can generate
an OPAC error instead of an Intranet error.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Go to http://localhost:8081/test.pl
2. Note normal Intranet 404 error
3. Go to http://localhost:8080/cgi-bin/koha/tracklinks.pl?uri=https%3A%2F%2Ftest.com&biblionumber=29
4. Note normal Opac 404 error
4b. For extra points, note that tracklinks.pl returns a 404 instead of a 302 to the 404.pl page
5. Change 'TrackClicks' syspref to "Track"
6. Go to http://localhost:8080/cgi-bin/koha/tracklinks.pl?uri=https%3A%2F%2Ftest.com&biblionumber=29
7. Note normal Opac 404 error
7b. For extra points, note that tracklinks.pl returns a 404 instead of a 302 to the 404.pl page
8. Rejoice!

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>

Passes all tests. No unexpected behaviour could be identified.


Current status: Signed Off
Comment 3 David Cook 2022-05-12 23:59:31 UTC
Thanks Paul :)
Comment 4 Marcel de Rooy 2022-05-13 07:59:05 UTC
QAing
Comment 5 Marcel de Rooy 2022-05-13 08:03:03 UTC
Processing additional checks

        * Commit title does not start with 'Bug XXXXX: ' - 2dc735c225

You should know by now, haha
Comment 6 Marcel de Rooy 2022-05-13 09:07:11 UTC
There is something more going on here behind the scenes since we applied the errordocument changes to plack.psgi.

Whatever we are doing now in the arguments to output_error like:
-    output_error( $cgi, '404' );
+    output_error( $cgi, '404', { interface => 'opac' } );
is completely useless.
Why?

There is a fundamental flaw in output_error:
    output_with_http_headers $query, $cookie, $template->output, 'html', '404 Not Found';
Although we pass $error, it always outputs a 404. I tried sending a 403, and it does not work!

The 404 from output_error is caught by Plack middleware and it will output the 404 error document. In this case the OPAC 404 error document!

In other words, the bug description is invalid too. There is actually no problem when it is a 404.

But we found another problem. We should fix the hardcoded 404 for other errors.
Comment 7 David Cook 2022-05-15 23:27:53 UTC
(In reply to Marcel de Rooy from comment #5)
> Processing additional checks
> 
>         * Commit title does not start with 'Bug XXXXX: ' - 2dc735c225
> 
> You should know by now, haha

Huh... that is weird! Must've been an off day.
Comment 8 David Cook 2022-05-15 23:33:23 UTC
(In reply to Marcel de Rooy from comment #6)
> There is something more going on here behind the scenes since we applied the
> errordocument changes to plack.psgi.
> 
> Whatever we are doing now in the arguments to output_error like:
> -    output_error( $cgi, '404' );
> +    output_error( $cgi, '404', { interface => 'opac' } );
> is completely useless.
> Why?
> 
> There is a fundamental flaw in output_error:
>     output_with_http_headers $query, $cookie, $template->output, 'html',
> '404 Not Found';
> Although we pass $error, it always outputs a 404. I tried sending a 403, and
> it does not work!
> 
> The 404 from output_error is caught by Plack middleware and it will output
> the 404 error document. In this case the OPAC 404 error document!
> 
> In other words, the bug description is invalid too. There is actually no
> problem when it is a 404.
> 
> But we found another problem. We should fix the hardcoded 404 for other
> errors.

I'll have another look at this one. 

Can you open a different bug for what you describe? Feel free to CC me in and I'll take a look.
Comment 9 Marcel de Rooy 2022-05-19 06:48:21 UTC
(In reply to David Cook from comment #8)
> Can you open a different bug for what you describe? Feel free to CC me in
> and I'll take a look.

Sure. Bug 30803 is yours :)