Bug 25548 - Package install Apache performs unnecessary redirects
Summary: Package install Apache performs unnecessary redirects
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Packaging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on: 7607 18669
Blocks:
  Show dependency treegraph
 
Reported: 2020-05-20 00:21 UTC by David Cook
Modified: 2020-12-16 14:35 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.07, 19.11.13


Attachments
Bug 25548: Remove Apache rewrite directives that trigger redirects (2.63 KB, patch)
2020-05-20 05:16 UTC, David Cook
Details | Diff | Splinter Review
Bug 25548: (follow-up) Remove those rewrite redirects in koha-httpd.conf (1.46 KB, patch)
2020-09-30 12:57 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 25548: Remove Apache rewrite directives that trigger redirects (2.69 KB, patch)
2020-09-30 12:57 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 25548: (follow-up) Remove those rewrite redirects in koha-httpd.conf (1.46 KB, patch)
2020-09-30 12:57 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 25548: Remove Apache rewrite directives that trigger redirects (2.75 KB, patch)
2020-10-02 07:54 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25548: (follow-up) Remove those rewrite redirects in koha-httpd.conf (1.52 KB, patch)
2020-10-02 07:54 UTC, Julian Maurice
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-05-20 00:21:56 UTC
Package install Apache performs unnecessary redirects using the following directives:

RewriteCond %{QUERY_STRING} (.*?)(?:[A-Za-z0-9_-]+)=&(.*)
RewriteRule (.+) $1?%1%2 [N,R,NE]

These directives were removed from etc/koha-httpd.conf many years ago, but they still persist in the Debian Apache templates.

It means that every simple search to /cgi-bin/koha/opac-search.pl from the masthead will redirect from /cgi-bin/koha/opac-search.pl?idx=&q=blah to /cgi-bin/koha/opac-search.pl?q=blah. 

It's OK if we have idx= in the URL. 

Let's reduce the amount of HTTP traffic we're generating.
Comment 1 David Cook 2020-05-20 00:28:02 UTC
Also, if you have a reverse proxy in front of Apache and you proxy using HTTP rather than HTTPS, Apache will generate fully qualified HTTP URLs for the Redirect, which cause one of the two scenarios:

Scenario 1:
1) Client (C) requests HTTPS from Reverse Proxy (RP)
2) RP requests HTTP from Apache (A)
3) A sends HTTP redirect to RP
4) RP sends HTTP redirect to C
5) C requests HTTP from RP

Now you've just downgraded your HTTPS to HTTP by accident

Scenario 2 (Force HTTPS):
1) Client (C) requests HTTPS from Reverse Proxy (RP)
2) RP requests HTTP from Apache (A)
3) A sends HTTP redirect to RP
4) RP sends HTTP redirect to C
5) C requests HTTP to RP
6) RP sends HTTPS redirect to C

Now you've just redirected twice bouncing from HTTPS to HTTP to HTTPS for just 1 search.
Comment 2 David Cook 2020-05-20 05:16:40 UTC
Created attachment 105136 [details] [review]
Bug 25548: Remove Apache rewrite directives that trigger redirects

Historically, we have used Apache rewrite directives to strip
query parameters with no arguments from the query
string (e.g. PARAM=&), and then redirect to the new URL.

However, this slows down user activity, and depending on your
HTTPS and proxy setup can cause suboptimal HTTP downgrades and
upgrades.

Test Plan:
0) Apply patch
1) Build Debian package
2) Install Debian package and reload Apache
3) Open developer tools on your favourite browser
4) Go to Network tab in developer tools
5) Go to /cgi-bin/koha/opac-main.pl
6) Search for "test"
7) Note a 200 status for "opac-search.pl?idx=&q=test"

If you got a 302 status for "opac-search.pl?idx=&q=test"
followed by a 200 status for "opac-search.pl?q=test", you
will know that your Apache configuration wasn't updated.
Comment 3 Fridolin Somers 2020-09-29 08:35:07 UTC
Ah and it causes Bug 7607

+1 to remove useless rewrites.
Comment 4 Katrin Fischer 2020-09-29 18:09:45 UTC
Frido, maybe a sign-off then? :)
Comment 5 Fridolin Somers 2020-09-30 12:57:19 UTC
Created attachment 110984 [details] [review]
Bug 25548: (follow-up) Remove those rewrite redirects in koha-httpd.conf

In etc/koha-httpd.conf for dev install, those rewrite redirects are already commented.
Just remove them.
Comment 6 Fridolin Somers 2020-09-30 12:57:40 UTC
Created attachment 110985 [details] [review]
Bug 25548: Remove Apache rewrite directives that trigger redirects

Historically, we have used Apache rewrite directives to strip
query parameters with no arguments from the query
string (e.g. PARAM=&), and then redirect to the new URL.

However, this slows down user activity, and depending on your
HTTPS and proxy setup can cause suboptimal HTTP downgrades and
upgrades.

Test Plan:
0) Apply patch
1) Build Debian package
2) Install Debian package and reload Apache
3) Open developer tools on your favourite browser
4) Go to Network tab in developer tools
5) Go to /cgi-bin/koha/opac-main.pl
6) Search for "test"
7) Note a 200 status for "opac-search.pl?idx=&q=test"

If you got a 302 status for "opac-search.pl?idx=&q=test"
followed by a 200 status for "opac-search.pl?q=test", you
will know that your Apache configuration wasn't updated.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 7 Fridolin Somers 2020-09-30 12:57:53 UTC
Created attachment 110986 [details] [review]
Bug 25548: (follow-up) Remove those rewrite redirects in koha-httpd.conf

In etc/koha-httpd.conf for dev install, those rewrite redirects are already commented.
Just remove them.
Comment 8 Fridolin Somers 2020-09-30 12:59:20 UTC
(In reply to Katrin Fischer from comment #4)
> Frido, maybe a sign-off then? :)

Allez ;)

Tested on Strech with Koha 20.05
Comment 9 Fridolin Somers 2020-09-30 13:00:17 UTC
Looks like this rewite is disabled in koha-httpd.conf since Koha 3.12 ^^
Comment 10 David Cook 2020-09-30 23:00:33 UTC
(In reply to Fridolin SOMERS from comment #9)
> Looks like this rewite is disabled in koha-httpd.conf since Koha 3.12 ^^

I think that you and I were the ones who did that one back in the day! Heh.

And thanks for the sign off and follow-up!
Comment 11 Julian Maurice 2020-10-02 07:54:38 UTC
Created attachment 111062 [details] [review]
Bug 25548: Remove Apache rewrite directives that trigger redirects

Historically, we have used Apache rewrite directives to strip
query parameters with no arguments from the query
string (e.g. PARAM=&), and then redirect to the new URL.

However, this slows down user activity, and depending on your
HTTPS and proxy setup can cause suboptimal HTTP downgrades and
upgrades.

Test Plan:
0) Apply patch
1) Build Debian package
2) Install Debian package and reload Apache
3) Open developer tools on your favourite browser
4) Go to Network tab in developer tools
5) Go to /cgi-bin/koha/opac-main.pl
6) Search for "test"
7) Note a 200 status for "opac-search.pl?idx=&q=test"

If you got a 302 status for "opac-search.pl?idx=&q=test"
followed by a 200 status for "opac-search.pl?q=test", you
will know that your Apache configuration wasn't updated.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 12 Julian Maurice 2020-10-02 07:54:43 UTC
Created attachment 111063 [details] [review]
Bug 25548: (follow-up) Remove those rewrite redirects in koha-httpd.conf

In etc/koha-httpd.conf for dev install, those rewrite redirects are already commented.
Just remove them.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 13 Marcel de Rooy 2020-10-02 09:16:57 UTC
Looks good to me too. But a rather intriguing question is: is there some script somewhere that depended somehow on the logic of removed empty parameters?
(Apart from seeing the other way around on the see also bug.)
Have the impression that David already had this in production for longer, so it is probably highly fictitious ;)
Comment 14 David Cook 2020-10-06 00:04:55 UTC
(In reply to Marcel de Rooy from comment #13)
> Looks good to me too. But a rather intriguing question is: is there some
> script somewhere that depended somehow on the logic of removed empty
> parameters?
> (Apart from seeing the other way around on the see also bug.)

I've traced the Apache lines back to 2007 when MJ Ray added them via commit e449c139fff, but there's really no information provided. 

I imagine that it was just cosmetic. That empty URL query parameters look ugly so that they stripped them off. MJ or Chris Cormack (who signed off on the patch) might know more.

> Have the impression that David already had this in production for longer, so
> it is probably highly fictitious ;)

Yep, I've been running this in production since May 20th 2020.
Comment 15 Jonathan Druart 2020-10-08 09:21:46 UTC
Hum, funny how this one and bug 7607 end up PQA at the same time.
I won't push both as this one will fix the problem from bug 7607.

(In reply to David Cook from comment #0)
> It means that every simple search to /cgi-bin/koha/opac-search.pl from the
> masthead will redirect from /cgi-bin/koha/opac-search.pl?idx=&q=blah to
> /cgi-bin/koha/opac-search.pl?q=blah. 
> 
> It's OK if we have idx= in the URL. 

However this is not only having ONE idx=, an adv search will become

/cgi-bin/koha/catalogue/search.pl?advsearch=1&idx=kw&q=ti&idx=kw&q=&idx=kw&q=&limit=&limit=&limit=&limit=&limit=&sort_by=relevance

That's starting to be ugly.

was: 
/cgi-bin/koha/catalogue/search.pl?advsearch=1&idx=kw&q=ti&idx=kw&idx=kw&sort_by=relevance

This has been an historical behaviour, how can we know it won't impact people?

At the minimum we should comment them I'd say.

Tomas, Nick, what are your thoughts on this one?
Comment 16 Marcel de Rooy 2020-10-08 09:54:56 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2020-10-08 09:56:10 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2020-10-08 09:58:29 UTC
(In reply to Jonathan Druart from comment #15)
> /cgi-bin/koha/catalogue/search.
> pl?advsearch=1&idx=kw&q=ti&idx=kw&q=&idx=kw&q=&limit=&limit=&limit=&limit=&li
> mit=&sort_by=relevance
> 
> That's starting to be ugly.

I agree that it is ugly. But does it really matter the user?
Comment 19 Katrin Fischer 2020-10-08 10:39:51 UTC
(In reply to Marcel de Rooy from comment #18)
> (In reply to Jonathan Druart from comment #15)
> > /cgi-bin/koha/catalogue/search.
> > pl?advsearch=1&idx=kw&q=ti&idx=kw&q=&idx=kw&q=&limit=&limit=&limit=&limit=&li
> > mit=&sort_by=relevance
> > 
> > That's starting to be ugly.
> 
> I agree that it is ugly. But does it really matter the user?

We show the lines in search history and other places... and users are sharing them on social media, in emails etc. I also think they are much less readable/guessable for users. I'd much prefer the shorter links.
Comment 20 Nick Clemens 2020-10-08 13:29:29 UTC
(In reply to Katrin Fischer from comment #19)
> (In reply to Marcel de Rooy from comment #18)
> > (In reply to Jonathan Druart from comment #15)
> > > /cgi-bin/koha/catalogue/search.
> > > pl?advsearch=1&idx=kw&q=ti&idx=kw&q=&idx=kw&q=&limit=&limit=&limit=&limit=&li
> > > mit=&sort_by=relevance
> > > 
> > > That's starting to be ugly.
> > 
> > I agree that it is ugly. But does it really matter the user?
> 
> We show the lines in search history and other places... and users are
> sharing them on social media, in emails etc. I also think they are much less
> readable/guessable for users. I'd much prefer the shorter links.

Would it be reasonable to clear empty inputs with javascript before the submission?
Comment 21 David Cook 2020-10-08 22:37:30 UTC
(In reply to Nick Clemens from comment #20)
> Would it be reasonable to clear empty inputs with javascript before the
> submission?

I like your thinking here, Nick. It would be better to prevent creating ugly query strings rather than trying to fix them after the fact.

Plus, in a future where we may just use Starman/Hypnotoad to serve Koha, we won't be able on Apache URL rewriting trickery. 


So maybe Javascript to clear empty inputs with Javascript, or Javascript to build the request. The former would probably be easier to bolt on to the status quo.
Comment 22 Jonathan Druart 2020-10-09 08:15:42 UTC
Who is volunteering?
Comment 23 Katrin Fischer 2020-10-09 19:43:06 UTC
(In reply to Jonathan Druart from comment #22)
> Who is volunteering?

Hm, just clearning the inputs won't fix the problem of bug 7607 I think? The problem is, that people leave search fields empty so idx and search terms don't match up. 

kw - no search term entered
title - some title words

Turns into: kw search for some title words

We have found this workaround from the other bug to be working, but I am not sure if that will fix the ugliness of the URL.

$(document).ready(function(){
$("input[name='do']").click(function(){
    $("input[name='q']").each(function(i){
        if (!$(this).val()){
            $("select[name='idx']").eq(i).append('<option value="" selected="selected"></option>'); 
        }
    }); 
});
});

and in intranetuserjs :

$(document).ready(function() {
$("#submit1,#submit2").click(function(){
    $("input[name='q']").each(function(i){
        if (!$(this).val()){
            $("select[name='idx']").eq(i).append('<option value="" selected="selected"></option>'); 
        }
    }); 
});
});
Comment 24 David Cook 2020-11-03 04:11:47 UTC
(In reply to Nick Clemens from comment #20)
> Would it be reasonable to clear empty inputs with javascript before the
> submission?

On reflection, that won't work the way you think it would work. It would create the problems described in Bug 7607.
Comment 25 David Cook 2020-11-03 04:13:13 UTC
(In reply to Katrin Fischer from comment #23)
> (In reply to Jonathan Druart from comment #22)
> > Who is volunteering?
> 
> Hm, just clearning the inputs won't fix the problem of bug 7607 I think? The
> problem is, that people leave search fields empty so idx and search terms
> don't match up. 
> 
> kw - no search term entered
> title - some title words
> 
> Turns into: kw search for some title words
> 
> We have found this workaround from the other bug to be working, but I am not
> sure if that will fix the ugliness of the URL.

With the Apache change, you don't need to do the workaround. The URL is equally as ugly as well. 

At the moment, it looks like the only way of having a less ugly URL is by removing empty search boxes from the DOM.
Comment 26 David Cook 2020-11-03 04:14:32 UTC
I am tempted to move this back to "Passed QA" because it actually resolves the problem from Bug 7607. 

While it makes for uglier URLs, it is functionally superior.
Comment 27 Jonathan Druart 2020-11-03 08:30:16 UTC
(In reply to David Cook from comment #24)
> (In reply to Nick Clemens from comment #20)
> > Would it be reasonable to clear empty inputs with javascript before the
> > submission?
> 
> On reflection, that won't work the way you think it would work. It would
> create the problems described in Bug 7607.

It won't if you remove the related idx.

I don't think we should push this patch without having both sides happy.
Comment 28 David Cook 2020-11-03 23:09:56 UTC
(In reply to Jonathan Druart from comment #27)
> (In reply to David Cook from comment #24)
> > (In reply to Nick Clemens from comment #20)
> > > Would it be reasonable to clear empty inputs with javascript before the
> > > submission?
> > 
> > On reflection, that won't work the way you think it would work. It would
> > create the problems described in Bug 7607.
> 
> It won't if you remove the related idx.
> 

What I meant was that if you set the idx value to '' (ie if you "clear empty inputs"), it will actually create an inconsistency in the form submission where the idx param is *not* sent in the form submission but the q param *is* sent. That is, unless you include an option in the select list for the idx that has a value of ''. It seems like glitchy/unexpected browser behaviour (at least in Chrome). 

But, as you say, removing the elements from the HTML form all together would work.

> I don't think we should push this patch without having both sides happy.

Sounds all right to me. I already applied these patches locally about 6 months ago, so I don't mind too much. Happy to help with Bug 7607 and then we can push this one once that's sorted.
Comment 29 David Cook 2020-11-03 23:31:43 UTC
(In reply to Jonathan Druart from comment #27)
>
> I don't think we should push this patch without having both sides happy.

David Nind and I have signed off on Nick's patch on Bug 7607. Once QA pass it, then we should all be happy ^_^.
Comment 30 David Cook 2020-11-03 23:32:49 UTC
Moving back to Passed QA and adding a dependency on Bug 7607. Just so that it doesn't get lost.
Comment 31 Jonathan Druart 2020-11-20 14:42:48 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 32 Andrew Fuerste-Henry 2020-12-10 22:12:20 UTC
Pushed to 20.05.x for 20.05.07
Comment 33 Victor Grousset/tuxayo 2020-12-11 16:44:38 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.
Comment 34 Tomás Cohen Arazi 2020-12-14 22:07:33 UTC
(In reply to Victor Grousset/tuxayo from comment #33)
> Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.

It is needed!
Comment 35 Tomás Cohen Arazi 2020-12-14 22:11:14 UTC
This did actually break sql reports with empty parameters.
Comment 36 Agustín Moyano 2020-12-14 22:42:06 UTC
(In reply to Tomás Cohen Arazi from comment #35)
> This did actually break sql reports with empty parameters.

Yes, we just found out this was the reason why filtered reports didn't work as expected when leaving blank inputs... For example, If you define a report like this:

select <<Library|branches:all>> as lib, 
           <<Item type|itemtypes:all>> as itype,
           <<CC|CC:all>> as cc,
           <<Date from>> as date_from,
           <<Date to>> as date_to,
           <<Invoice>> as invoice

and submit the form with one parameter.. for example the one that says 'Invoice' the form posts to the report with the following query string parameters:

reports: 41
phase: Run this report
param_name: Library|branches:all
sql_params: %
param_name: Item type|itemtypes:all
sql_params: %
param_name: CC|CC:all
sql_params: %
param_name: Date from
sql_params: 
param_name: Date to
sql_params: 
param_name: Invoice
sql_params: invoice_1 


But after redirection, empty parameters are filtered, leaving things like this

reports: 41
phase: Run this report
param_name: Library|branches:all
sql_params: %
param_name: Item type|itemtypes:all
sql_params: %
param_name: CC|CC:all
sql_params: %
param_name: Date from
param_name: Date to
param_name: Invoice
sql_params: invoice_1

Notice missing sql_params. This ended up the query to be built like this:

select '%' as lib, 
           '%' as itype,
           '%' as cc,
           'invoice_1' as date_from,
           NULL as date_to,
           NULL as invoice

notice that 'invoice_1' is placed in date_from instead of invoice column.

This fix is more awesome than you realized, ;)
Comment 37 David Cook 2020-12-14 22:43:18 UTC
(In reply to Agustín Moyano from comment #36)
> This fix is more awesome than you realized, ;)

That's a good way to start the day!
Comment 38 Victor Grousset/tuxayo 2020-12-16 14:35:20 UTC
Great news then :)

Backported to 19.11.x branch for 19.11.13